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

NumericRange.max returns unexpected value on empty ranges #10060

Closed
scabug opened this Issue Nov 15, 2016 · 4 comments

Comments

Projects
None yet
2 participants
@scabug
Copy link

scabug commented Nov 15, 2016

scala> Range.Long.inclusive(111, 999, -1).max
res28: Long = 111

I expect Range.Long.max to throw java.util.NoSuchElementException to be consistent with Range.Long.min and Range.max instead of returning a wrong value.

More examples here https://gist.github.com/tabdulradi/b3ff116032f2eb16aedf077d8e5f8427

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Nov 15, 2016

Imported From: https://issues.scala-lang.org/browse/SI-10060?orig=1
Reporter: @tabdulradi
Affected Versions: 2.11.8, 2.12.0

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Nov 15, 2016

@Blaisorblade said:
I agree this is a bug, and it seems the minimal fix could be taking this patch for #7432:
scala/scala@357c2df

and porting it to NumericRange:
https://github.com/scala/scala/blob/v2.12.0/src/library/scala/collection/immutable/NumericRange.scala#L116-L126

Looking at https://github.com/scala/scala/blob/v2.12.0/src/library/scala/collection/immutable/Range.scala#L78-L82, it seems the intention is that range creation should create an empty range.

But looking at https://github.com/scala/scala/blob/v2.12.0/src/library/scala/collection/immutable/NumericRange.scala#L49-L53 (which seems relevant for Range.Long stuff), and at NumericRange.count, it's not obvious that's actually the case.

Since you wrote on Gitter you'd like to work on this: it'd be great, if you can, to also add some testcases at least for this behavior (possibly applying to both sort of ranges?). But a patch would already help.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Nov 15, 2016

@tabdulradi said:
Thanks for your help. I'll start working on the test cases first, then I'll port the patch.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Nov 16, 2016

@tabdulradi said:
PR ready for review scala/scala#5531

@scabug scabug closed this Dec 12, 2016

@scabug scabug added this to the 2.12.2 milestone Apr 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment