Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug or FeatureRequest]: collection.rotate(toStartAt: Int) crashes with negative argument #239

Open
2 tasks done
Loradim opened this issue Aug 11, 2024 · 2 comments
Open
2 tasks done

Comments

@Loradim
Copy link

Loradim commented Aug 11, 2024

Replace this paragraph with a short description of the incorrect incorrect behavior. If this is a regression, please note the last version that the behavior was correct in addition to your current version.

Swift Algorithms version:
1.2.0 (= main branch currently)

Swift version:
swift-driver version: 1.90.11.1 Apple Swift version 5.10 (swiftlang-5.10.0.13 clang-1500.3.9.4)
Target: arm64-apple-macosx14.0

Checklist

  • If possible, I've reproduced the issue using the main branch of this package
  • I've searched for existing GitHub issues

Steps to Reproduce

calling .rotate(toStartAt: ) with a negative number causes a run-time error

var array = [1, 2, 3, 4, 5, 6, 7, 8]
array.rotate(toStartAt: -2)

Expected behavior

This function shouldn't crash. Considering what .rotate() does (positive values rotate elements to the left) being called with a negative value should rotate to the right.
Another option would be to only allow for positive values as arguments, e.g. UInt as parameter.

Actual behavior

Code crashes with an fatal error:
Swift/arm64e-apple-macos.swiftinterface:16236: Fatal error: Range requires lowerBound <= upperBound

Kind regards,
Heiko

@xwu
Copy link
Contributor

xwu commented Aug 12, 2024

Hi Heiko, the argument to rotate isn't the number of elements to rotate left or right, but rather it is the current index of the element that should be first after rotating (and therefore must be a valid index). This is why the argument label is toStartAt, not something like by.

Note that not all collections with integer indices start at 0, so confusing the index with an offset will cause unexpected results as it will in many other Swift collection APIs.

@Loradim
Copy link
Author

Loradim commented Aug 12, 2024

Hi Heiko, the argument to rotate isn't the number of elements to rotate left or right, but rather it is the current index of the element that should be first after rotating (and therefore must be a valid index). This is why the argument label is toStartAt, not something like by.

@xwu Thanks for your remark - given some thoughts on your response, I can follow your argumentation (especially with setting a start index instead of an offset for rotation. I left my original comment for reference purpose only.

You are right, the argument is the index of the first element after .rotate() finished its job, it's not an offset for rotation, which could go either way (sorry for being un-precise on this). My main concern is that crashing instead of gracefully failing is a harsh reaction (I recognise that it's a mutating function on the collection itself though)

Swift as a language is designed to be safe to use (strongly and statically typed language, optionals, basically no manual memory management, etc). It's really hard to "shoot yourself in the foot" with Swift unless you purposely opt-in into such a behaviour. In my opinion, frameworks / packages should follow the same idea of safety and should fail gracefully. A fatal error seems a bit to harsh of a reaction for a function that simply could return "sorry, can't do that" (again, more difficult for an inplace change of order of elements) - throw might be another option here, as most probably it's still in a recoverable state.

A possible solution (sorry again, I mixed up an encountered issue with possible solution) would be to interpret a "negative index" as an index starting from the end (seems to be reasonable to me)

I'm fully aware, that it's my responsibility as a programmer / developer to pass in the correct arguments as function parameters and it's fine to get an error - getting a crash feels like an overreaction (at least in a debug build) - it might be right for a mutating func, but not really happy with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants