[Bug or FeatureRequest]: collection.rotate(toStartAt: Int) crashes with negative argument
Opened this issue · 2 comments
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
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.
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.