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.

Sign up for GitHub

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

Jump to bottom

Fix Number::StepIterator overflow #10295

Merged
merged 3 commits into from
Jan 25, 2021
Merged

Fix Number::StepIterator overflow #10295

merged 3 commits into from
Jan 25, 2021

Conversation

bcardiff
Copy link
Member

Before this PR the Number::StepIterator#next could raise because of limit - @current
Fixes a regression introduced in #10130

Failures:

  1) Number #step whole range iterates UInt8 downwards

       Arithmetic overflow (OverflowError)
         from src/number.cr:249:21 in 'next'
         from src/iterator.cr:488:15 in '->'
         from src/primitives.cr:255:3 in 'internal_run'
         from src/spec/example.cr:33:16 in 'run'
         from src/spec/context.cr:18:23 in 'internal_run'
         from src/spec/context.cr:330:7 in 'run'
         from src/spec/context.cr:18:23 in 'internal_run'
         from src/spec/context.cr:330:7 in 'run'
         from src/spec/context.cr:18:23 in 'internal_run'
         from src/spec/context.cr:330:7 in 'run'
         from src/spec/context.cr:18:23 in 'internal_run'
         from src/spec/context.cr:147:7 in 'run'
         from src/spec/dsl.cr:274:7 in '->'
         from src/primitives.cr:255:3 in 'run'
         from src/crystal/main.cr:45:14 in 'main'
         from src/crystal/main.cr:119:3 in 'main'
       

  2) Number #step whole range iterates Int8 upwards

       Arithmetic overflow (OverflowError)
         from src/number.cr:249:21 in 'next'
         from src/iterator.cr:488:15 in '->'
         from src/primitives.cr:255:3 in 'internal_run'
         from src/spec/example.cr:33:16 in 'run'
         from src/spec/context.cr:18:23 in 'internal_run'
         from src/spec/context.cr:330:7 in 'run'
         from src/spec/context.cr:18:23 in 'internal_run'
         from src/spec/context.cr:330:7 in 'run'
         from src/spec/context.cr:18:23 in 'internal_run'
         from src/spec/context.cr:330:7 in 'run'
         from src/spec/context.cr:18:23 in 'internal_run'
         from src/spec/context.cr:147:7 in 'run'
         from src/spec/dsl.cr:274:7 in '->'
         from src/primitives.cr:255:3 in 'run'
         from src/crystal/main.cr:45:14 in 'main'
         from src/crystal/main.cr:119:3 in 'main'
       

  3) Number #step whole range iterates Int8 downwards

       Arithmetic overflow (OverflowError)
         from src/number.cr:249:21 in 'next'
         from src/iterator.cr:488:15 in '->'
         from src/primitives.cr:255:3 in 'internal_run'
         from src/spec/example.cr:33:16 in 'run'
         from src/spec/context.cr:18:23 in 'internal_run'
         from src/spec/context.cr:330:7 in 'run'
         from src/spec/context.cr:18:23 in 'internal_run'
         from src/spec/context.cr:330:7 in 'run'
         from src/spec/context.cr:18:23 in 'internal_run'
         from src/spec/context.cr:330:7 in 'run'
         from src/spec/context.cr:18:23 in 'internal_run'
         from src/spec/context.cr:147:7 in 'run'
         from src/spec/dsl.cr:274:7 in '->'
         from src/primitives.cr:255:3 in 'run'
         from src/crystal/main.cr:45:14 in 'main'
         from src/crystal/main.cr:119:3 in 'main'
       

  4) Number #step whole range iterates Int16 upwards

       Arithmetic overflow (OverflowError)
         from src/number.cr:249:21 in 'next'
         from src/iterator.cr:488:15 in '->'
         from src/primitives.cr:255:3 in 'internal_run'
         from src/spec/example.cr:33:16 in 'run'
         from src/spec/context.cr:18:23 in 'internal_run'
         from src/spec/context.cr:330:7 in 'run'
         from src/spec/context.cr:18:23 in 'internal_run'
         from src/spec/context.cr:330:7 in 'run'
         from src/spec/context.cr:18:23 in 'internal_run'
         from src/spec/context.cr:330:7 in 'run'
         from src/spec/context.cr:18:23 in 'internal_run'
         from src/spec/context.cr:147:7 in 'run'
         from src/spec/dsl.cr:274:7 in '->'
         from src/primitives.cr:255:3 in 'run'
         from src/crystal/main.cr:45:14 in 'main'
         from src/crystal/main.cr:119:3 in 'main'
       

  5) Number #step whole range iterates Int16 downwards

       Arithmetic overflow (OverflowError)
         from src/number.cr:249:21 in 'next'
         from src/iterator.cr:488:15 in '->'
         from src/primitives.cr:255:3 in 'internal_run'
         from src/spec/example.cr:33:16 in 'run'
         from src/spec/context.cr:18:23 in 'internal_run'
         from src/spec/context.cr:330:7 in 'run'
         from src/spec/context.cr:18:23 in 'internal_run'
         from src/spec/context.cr:330:7 in 'run'
         from src/spec/context.cr:18:23 in 'internal_run'
         from src/spec/context.cr:330:7 in 'run'
         from src/spec/context.cr:18:23 in 'internal_run'
         from src/spec/context.cr:147:7 in 'run'
         from src/spec/dsl.cr:274:7 in '->'
         from src/primitives.cr:255:3 in 'run'
         from src/crystal/main.cr:45:14 in 'main'
         from src/crystal/main.cr:119:3 in 'main'
       

Finished in 53.71 milliseconds
733 examples, 0 failures, 5 errors, 0 pending

Failed examples:

crystal spec spec/std/number_spec.cr:309 # Number #step whole range iterates UInt8 downwards
crystal spec spec/std/number_spec.cr:313 # Number #step whole range iterates Int8 upwards
crystal spec spec/std/number_spec.cr:318 # Number #step whole range iterates Int8 downwards
crystal spec spec/std/number_spec.cr:322 # Number #step whole range iterates Int16 upwards
crystal spec spec/std/number_spec.cr:327 # Number #step whole range iterates Int16 downwards

@bcardiff bcardiff added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:numeric labels Jan 25, 2021
@bcardiff bcardiff added this to the 0.36.0 milestone Jan 25, 2021
@straight-shoota
Copy link
Member

The same overflow happens with the yielding method as well.

The implementation looks very hard to follow. I'll give it a try to find a better solution.

@straight-shoota
Copy link
Member

Oh damn: It just needs to be (limit - step) <=> current instead of (limit - current) <=> step 😆

straight-shoota
straight-shoota reviewed Jan 25, 2021
View reviewed changes
spec/std/number_spec.cr Outdated
Comment on lines 305 to 314
it_iterates "UInt8 upwards", (UInt8::MIN.to_i..UInt8::MAX.to_i).map(&.to_u8), (UInt8::MIN..UInt8::MAX).step(by: 1)
it_iterates "UInt8 downwards", (UInt8::MIN.to_i..UInt8::MAX.to_i).map(&.to_u8).reverse, (UInt8::MAX..UInt8::MIN).step(by: -1)

it { (Int8::MIN..Int8::MAX).each.count { true }.should eq(256) }
it_iterates "Int8 upwards", (Int8::MIN.to_i..Int8::MAX.to_i).map(&.to_i8), (Int8::MIN..Int8::MAX).step(by: 1)
it_iterates "Int8 downwards", (Int8::MIN.to_i..Int8::MAX.to_i).map(&.to_i8).reverse, (Int8::MAX..Int8::MIN).step(by: -1)

it { (Int16::MIN..Int16::MAX).each.count { true }.should eq(65536) }
it_iterates "Int16 upwards", (Int16::MIN.to_i..Int16::MAX.to_i).map(&.to_i16), (Int16::MIN..Int16::MAX).step(by: 1)
it_iterates "Int16 downwards", (Int16::MIN.to_i..Int16::MAX.to_i).map(&.to_i16).reverse, (Int16::MAX..Int16::MIN).step(by: -1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Collecting the entire ranges in arrays isn't super efficient, but not a big deal either. Using it_iterates ensures these test are run against both step methods.

@bcardiff
Copy link
Member Author

Cool LGTM. I will need your approval on the PR to merge your changes once the CI is happy.

@bcardiff bcardiff merged commit 1c1fc6c into crystal-lang:master Jan 25, 2021
@bcardiff bcardiff deleted the fix/number-step-iterator-overflow branch January 25, 2021 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@straight-shoota straight-shoota straight-shoota approved these changes

Assignees
No one assigned
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:numeric
Projects
None yet
Milestone
0.36.0
Development

Successfully merging this pull request may close these issues.
2 participants
@bcardiff @straight-shoota