Skip to content

Range#max(n) ignores the block's ordering, and the one spec for it cannot tell #891

Open

Description

@tannevaled

Range#max(n) ignores the block's ordering

Found while porting range_max for beginless ranges (#799's five
core/range/max_spec.rb examples); pre-existing and unrelated, so it was
not folded into that change.

Measured against MRI 4.0.7 on darwin/arm64

(1..10).max(2) { |a, b| -(a <=> b) }
  rbgo  => [10, 9]
  MRI   => [1, 2]

(1..10).max(2) { |a, b| a <=> b }        # agree by coincidence
  rbgo  => [10, 9]
  MRI   => [10, 9]

The first case is the whole bug: the block reverses the ordering, so the two
maxima under that comparison are the two smallest elements. rbgo answers the
two largest, because it never consults the block.

Why

range_max dispatches on the block before the argument:

if (rb_block_given_p() || (EXCL(range) && !nm)) {
    ...
    return rb_call_super(argc, argv);     /* Enumerable#max(n) WITH the block */
}
else if (argc) { ... }                    /* the arithmetic path */

rbgo's native checks len(args) > 0 first and takes the arithmetic path, which
has no way to use a block. The block is silently dropped.

Why nothing caught it

core/range/max_spec.rb has exactly one example for this shape:

describe "Range#max given a block and an integer argument" do
  it "returns n elements the block determines to be the maximum" do
    (1..10).max(2) {|a,b| a <=> b }.should == [10, 9]
  end
end

Its block is the IDENTITY ordering, so the arithmetic answer and the block
answer are the same array. The example passes against an implementation that
ignores the block entirely — it cannot distinguish them. A second example with
a reversed comparison would fail today.

What would close it

Enumerable#max(n) with a block, reached when a block is given, with the
guard order matching range_max — the beginless RangeError already moved to
the top of the method in the port, so only the dispatch to the block path is
missing. Worth adding the reversed-comparison example to ruby/spec upstream at
the same time, since the gap is in the corpus and not only in rbgo.

🤖 Generated with Claude Code

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions