Bug #17034
closedUnexpected behavior in #max for beginless range
Description
When calling max
on a beginless range, a non-intuitive error gets raised:
r = ..9
r.max
# ArgumentError: comparison of NilClass with 9 failed
There's a check for NIL_P(RANGE_BEG(range))
but it's inside another check which is false for the example case above:
if (rb_block_given_p() || (EXCL(range) && !nm) || argc) {
if (NIL_P(RANGE_BEG(range))) {
rb_raise(rb_eRangeError, "cannot get the maximum of beginless range with custom comparison method");
}
return rb_call_super(argc, argv);
}
The first part of the condition is false since there is no block, and even though I'm not sure what EXCL
does the second part of the condition will be false due to !nm
(nm
will be true because of FIXNUM_P(e)
). So I think the error gets raised here:
int c = OPTIMIZED_CMP(b, e, cmp_opt);
I think this is not ideal. Possible solutions:
- Return
e
(RANGE_END(range)
for beginless ranges or - return a
RangeError
with a message like "cannot get the maximum of beginless range" (similar to.min
).
Happy to provide a patch if people want this changed and can agree on what the new behavior should be.
Files
Updated by citizen428 (Michael Kohl) over 4 years ago
citizen428 (Michael Kohl) wrote:
I think this is not ideal. Possible solutions:
- Return
e
(RANGE_END(range)
for beginless ranges or- return a
RangeError
with a message like "cannot get the maximum of beginless range" (similar to.min
).
I had some time and looked into this. For inclusive integer ranges r.end
will now be returned, no other behavior changes. Patch attached, but since the diff is very small I also opened a PR:
Updated by jeremyevans0 (Jeremy Evans) over 4 years ago
citizen428 (Michael Kohl) wrote:
I think this is not ideal. Possible solutions:
- Return
e
(RANGE_END(range)
for beginless ranges or- return a
RangeError
with a message like "cannot get the maximum of beginless range" (similar to.min
).Happy to provide a patch if people want this changed and can agree on what the new behavior should be.
I think a RangeError
is more correct. Without having a beginning, you cannot know the increment value, and therefore cannot know the maximum value. People that want the end of the range should use Range#end
, not Range#max
. However, I can see where Range#max
returning the end for an inclusive beginless range could potentially be more useful.
Updated by citizen428 (Michael Kohl) over 4 years ago
jeremyevans0 (Jeremy Evans) wrote in #note-2:
I think a
RangeError
is more correct. Without having a beginning, you cannot know the increment value, and therefore cannot know the maximum value. People that want the end of the range should useRange#end
, notRange#max
. However, I can see whereRange#max
returning the end for an inclusive beginless range could potentially be more useful.
The current PR only returns the last value if it's an inclusive beginless range of integers:
(..2).max
#=> 2
Exclusive range ((...2).max
): TypeError (cannot exclude end value with non Integer begin value)
Non-integer range ((...2.0).max
): TypeError (cannot exclude non Integer end value)
Non-numeric range ((...'c').max
): TypeError (cannot get the maximum of beginless range with custom comparison method)
That said I'm happy to change it to a RangeError
if that's the overall consensus.
Updated by nobu (Nobuyoshi Nakada) over 4 years ago
- Backport changed from 2.5: UNKNOWN, 2.6: UNKNOWN, 2.7: UNKNOWN to 2.5: DONTNEED, 2.6: DONTNEED, 2.7: REQUIRED
I agree that RangeError
would be better, but it seems a separate issue as TypeError
is used already in other cases.
Updated by Anonymous over 4 years ago
- Status changed from Open to Closed
Applied in changeset git|8a5ad2b77d7a24e4f8f4fef179ae5efced935f91.
Fix Range#max for beginless Integer ranges [Bug #17034]
- Fix Range#max for beginless Integer ranges
- Update test/ruby/test_range.rb
- Fix formatting
https://github.com/ruby/ruby/pull/3328
Co-authored-by: Nobuyoshi Nakada nobu@ruby-lang.org