Skip to content

Set#divide with a 2-arity block: 4.0 changed the algorithm, not just the yields - #902

Merged
tannevaled merged 2 commits into
mainfrom
set-divide-409
Nov 5, 2026
Merged

tannevaled merged 2 commits into
mainfrom
set-divide-409

Conversation

@tannevaled

Copy link
Copy Markdown
Contributor

Closes core/set/divide_spec.rb from #799: pass=8 fail=1 → pass=9 fail=0 error=0.

ruby/spec asserts this one both ways, five lines apart — which is how a reader tells a version swap from a defect:

ruby_version_is "4.0"       # ret.sort.should == [[1, 2], [2, 1]]
ruby_version_is ""..."4.0"  # ret.sort.should == [[1, 1], [1, 2], [2, 1], [2, 2]]

(core/set/divide_spec.rb:35 and :43.)

Upstream

set_divide_arity2 is union-find over each unordered pair once:

for (long i = 0; i < size - 1; i++) {
    VALUE item1 = RARRAY_AREF(items, i);
    for (long j = i + 1; j < size; j++) {
        VALUE item2 = RARRAY_AREF(items, j);
        if (RTEST(rb_yield_values(2, item1, item2)) &&
            RTEST(rb_yield_values(2, item2, item1))) {
            set_divide_union_find_merge(uf_parents, i, j, tmp_array);
        }
    }
}

rbgo ran Tarjan's strongly-connected components over the full n×n relation — what MRI's lib/set.rb did up to 3.x.

⛔ These are different functions, not two spellings of one

The spec example checks only which pairs are yielded. The result changed too, and no ruby/spec example covers it. A directed cycle 1→2→3→1 with no reverse edge is one strongly-connected component, and has no mutually-related pair at all:

f = ->(x, y) { [[1, 2], [2, 3], [3, 1]].include?([x, y]) }
Set[1, 2, 3].divide(&f)
# SCC (before):       [[1, 2, 3]]
# MRI 4.0.7 (after):  [[1], [2], [3]]

That case is why the algorithm is replaced rather than the loop bounds adjusted.

⛔ The && short-circuits, and the spec cannot see it

set.c:940 is RTEST(yield(a,b)) && RTEST(yield(b,a)), so a falsy first answer means the reversed pair is never yielded. The spec's own example cannot detect this — its block body is ret << [x, y], which returns the Array and is always truthy. Measured on 4.0.7 with a block answering false: 3 yields for a 3-element Set, where rbgo made 9.

⛔ A one-element Set yields nothing

No pairs exist, so the block is never called. rbgo called it once, with the element against itself.

The outer Set is a plain Set on both paths

set_i_divide builds it with set_alloc_with_size(rb_cSet, …) (set.c:1002) and set_divide_arity2 with set_s_create(0, 0, rb_cSet) (set.c:946). The arity-1 path here said self.class. Only the subsets take the receiver's class.

Measured

rubydiff against /opt/homebrew/opt/ruby@4.0/bin/ruby over 19 shapes plus 5 algorithm discriminators: DIVERGE on 4 before, AGREE after.

20 rows in internal/vm/set_divide_test.go, each measured on 4.0.7. Nine of them cover behaviour no ruby/spec example asserts.

⛔ Deferred: the full specdelta sweep

The 1-minute load average was 70.52 on this host when the change was ready. specdelta has a per-file timeout, so a sweep under that load manufactures regressions that are not there. I will run it and report it on this PR when the machine is quiet. core/set ran clean in the meantime — divide 9/9, classify 4/4, subtract 2/2.

Not fixed here, filed instead

Set cannot be subclassed at all in rbgo: set.go:144 and :151 ignore the receiver and always build a plain Set, so Class.new(Set)[1,2].class is Set. That is why the subsets' class is not asserted yet. It is a wider change and belongs on its own.

🤖 Generated with Claude Code

…ields

core/set/divide_spec.rb: pass=8 fail=1 -> pass=9 fail=0 error=0.

ruby/spec asserts this one both ways five lines apart, which is how a reader
tells a version swap from a defect:

    ruby_version_is "4.0"       ret.sort.should == [[1, 2], [2, 1]]
    ruby_version_is ""..."4.0"  ret.sort.should == [[1, 1], [1, 2], [2, 1], [2, 2]]

(core/set/divide_spec.rb:35 and :43.)

## Upstream

set_divide_arity2 (set.c:923 at v4.0.9) is UNION-FIND over each unordered pair
once:

    for (long i = 0; i < size - 1; i++) {
        VALUE item1 = RARRAY_AREF(items, i);
        for (long j = i + 1; j < size; j++) {
            VALUE item2 = RARRAY_AREF(items, j);
            if (RTEST(rb_yield_values(2, item1, item2)) &&
                RTEST(rb_yield_values(2, item2, item1))) {
                set_divide_union_find_merge(uf_parents, i, j, tmp_array);
            }
        }
    }

rbgo ran Tarjan's strongly-connected components over the full n x n relation --
what MRI's lib/set.rb did up to 3.x.

## ⛔ These are different FUNCTIONS, not two spellings of one

The spec example only checks which pairs are yielded. The result changed too,
and nothing in ruby/spec covers it. A directed cycle 1->2->3->1 with no reverse
edge is ONE strongly-connected component and has no mutually-related pair at
all:

    f = ->(x, y) { [[1, 2], [2, 3], [3, 1]].include?([x, y]) }
    Set[1, 2, 3].divide(&f)
    # SCC (before):        [[1, 2, 3]]
    # MRI 4.0.7 (after):   [[1], [2], [3]]

That case is why the whole algorithm is replaced rather than the loop bounds
adjusted.

## ⛔ The && short-circuits, and the spec cannot see it

set.c:940 is `RTEST(yield(a,b)) && RTEST(yield(b,a))`, so a falsy first answer
means the reversed pair is never yielded. The spec's own example cannot detect
this, because its block body is `ret << [x, y]` -- which returns the Array and
is always truthy. Measured on 4.0.7 with a block that answers false: 3 yields
for a 3-element Set, where rbgo made 9.

## ⛔ A one-element Set yields nothing

No pairs exist, so the block is never called. rbgo called it once, with the
element against itself.

## The outer Set is a plain Set on BOTH paths

set_i_divide builds it with set_alloc_with_size(rb_cSet, ...) (set.c:1002) and
set_divide_arity2 with set_s_create(0, 0, rb_cSet) (set.c:946). The arity-1
path here said self.class, so a Set subclass got a subclass back. Only the
SUBSETS take the receiver's class.

## Measured

rubydiff against /opt/homebrew/opt/ruby@4.0/bin/ruby, 19 shapes plus 5
algorithm discriminators: DIVERGE on 4 before, AGREE after.

20 rows in internal/vm/set_divide_test.go, each measured on 4.0.7. Nine of them
cover behaviour no ruby/spec example asserts.

## ⛔ Deferred: the full specdelta sweep

1-minute load average was 70.52 on this host when the change was ready.
specdelta has a per-file timeout, so a sweep under that load manufactures
regressions that are not there. It will be run and reported on this PR when the
machine is quiet; core/set ran clean in the meantime (divide 9/9, classify 4/4,
subtract 2/2).

## Not fixed here, and filed instead

Set cannot be subclassed at all in rbgo: set.go:144 and :151 ignore the
receiver and always build a plain Set, so `Class.new(Set)[1,2].class` is Set.
That is why the subsets' class cannot be asserted yet. It is a wider change
than this one and belongs on its own.

Refs #799

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@tannevaled

Copy link
Copy Markdown
Contributor Author

The deferred specdelta sweep, now run

This PR said the full sweep was deferred because the 1-minute load average was 70.52, and promised it when the machine was quiet. Here it is.

PROVENANCE
  control   rbgo-ctl902 — built from 86d69f17b2c4
  treatment rbgo-trt902 — built from c3544f0b1b11
MOVE none declared: both engines read the same corpus, so every delta below is the ENGINE

file                                                     control   treat  delta
core/set/divide_spec.rb                                        8       9     +1

2206 file(s): control 23687 passing, treatment 23688 passing, +1
1 file(s) moved, 0 regressed

-dir core,language -move none -timeout 5m, every file printed (no -quiet), exit 0.

One file moved, and it is the one this PR is about. 23687 → 23688 over 2206 files: core/set/divide_spec.rb 8 → 9, nothing else in core/ or language/ changed by a single example. That matters here more than usual, because the change regenerates prelude_frozen_gen.go and renumbers ~864 Line: entries across the whole prelude — a diff that could have reached anything.

Also zero on every other channel specdelta reports: 0 regressed, 0 examples WENT RED, 0 UNSTABLE, 0 files with fewer detail lines than their own fail+error tally. The red-example check is the one a +1 cannot speak for — a net gain hides a swap — and it is clean.

⛔ How this was made load-proof rather than load-free

The machine never did get quiet. The 1-minute load was 7.15 when the run started, 14.06 eight minutes in, and 39.38 at one point — on 16 cores.

The first attempt was stopped and restarted, because -timeout 90s under that load is the exact mechanism that corrupts a sweep: a heavy file that would pass exceeds the clock and is recorded as not loading. Raising it to 5 minutes — roughly 60× the work a spec file does — removes the clock as a variable instead of waiting for a quiet machine that was not coming.

That turns "was the load a problem?" into something readable rather than something to argue about, because cmd/specdelta names every file it kills (main.go:271):

$ grep -c "KILLED on timeout" sweep-902.txt
0

Zero. No file was killed, so no count in the table was decided by the clock, so the load did not reach the numbers. Had that grep returned anything, the run would have been void and re-run — the control is what makes the table quotable, not the quiet.

Limits of this run, stated

  • -repeat 1. One run per engine cannot tell a flake from a difference, so this pass locates movers rather than judging them. Exactly one moved, it is the file this PR changes, and its delta is the one the PR already measured directly against MRI 4.0.7 — there is nothing for a -repeat 3 to adjudicate. Had an unexpected file moved, it would have been re-run at -repeat 3 before being reported.
  • Four files are DID NOT RUN on either: core/kernel/exit_spec.rb, core/mutex/lock_spec.rb, core/process/exit_spec.rb, core/string/unpack/carret_spec.rb. Identical on both engines, and not timeouts (those print KILLED), so they are pre-existing and outside this comparison — but they are four files this sweep says nothing about, and that is worth writing down rather than folding into a clean total.
  • library/, optional/, command_line/ and security/ are not measured; core,language is the surface, as cmd/specdelta's own refusal computes it.

🤖 Generated with Claude Code

@tannevaled
tannevaled merged commit e561921 into main Nov 5, 2026
26 checks passed
@tannevaled
tannevaled deleted the set-divide-409 branch November 5, 2026 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant