Project

General

Profile

Actions

Bug #17034

closed

Unexpected behavior in #max for beginless range

Added by citizen428 (Michael Kohl) over 1 year ago. Updated over 1 year ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Target version:
-
ruby -v:
2.7.1p83 (2020-03-31 revision a0c7c23c9c) [x86_64-darwin19]
[ruby-core:99198]

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:

  1. Return e (RANGE_END(range) for beginless ranges or
  2. 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

range-max-beginless.patch (1.26 KB) range-max-beginless.patch citizen428 (Michael Kohl), 07/17/2020 04:25 PM

Updated by citizen428 (Michael Kohl) over 1 year ago

citizen428 (Michael Kohl) wrote:

I think this is not ideal. Possible solutions:

  1. Return e (RANGE_END(range) for beginless ranges or
  2. 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:

https://github.com/ruby/ruby/pull/3328/

Updated by jeremyevans0 (Jeremy Evans) over 1 year ago

citizen428 (Michael Kohl) wrote:

I think this is not ideal. Possible solutions:

  1. Return e (RANGE_END(range) for beginless ranges or
  2. 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 1 year 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 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.

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 1 year 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.

Actions #5

Updated by Anonymous over 1 year 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

Actions

Also available in: Atom PDF