Skip to content

test(#10937): pin declared-number addition evaluation order - #10959

Closed
proggeramlug wants to merge 2 commits into
mainfrom
fix/10937-declared-number-fold
Closed

proggeramlug wants to merge 2 commits into
mainfrom
fix/10937-declared-number-fold

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Closes #10937.

The production fix already landed through #10921 in 2d370f625, before this branch's assigned base a022cf2e4. The shared lower_guarded_numeric_add entry checks evaluation-order faithfulness for both callers. No production change is needed.

This PR brings the declared-number counterexamples into the per-PR gap shards with a dedicated test_gap_ fixture. It covers number[] elements and fields on this, plus right-associated and explicitly snapshotted controls. Each case requires exactly one conversion and a late-slot mutation to 100; the fixture asserts that all six cases executed.

The test was committed first in 722f34710, after observing its failure and before any production edit.

Arm Elements this fields New fixture
Node 26.5.1 103 107 6 pass
Installed Perry 0.5.1520 6 10 2 fail, 4 controls pass
Assigned source 103 107 6 pass
Targeted sabotage 6 10 2 fail, 4 controls pass
Restored source 103 107 6 pass

Root cause: the fused fold loads every leaf before conversion. A left-associated chain must convert its inner pair before reading a later mutable element/field. The cold arm uses the already-loaded values, so it cannot repair the stale read.

Sabotage moved the existing faithfulness check from the shared fold to only the dynamic caller, then rebuilt the compiler and both static archives in the owned Linux worktree. The fixture failed with its own #10937 declared-number addition order failures: 2 message, while the separate dynamic-entry counterexample still returned Node's 102. All four controls passed, with one conversion and the mutation observed. Restoring the shared guard byte-for-byte returned all six cases to green. No sabotage remains in the diff.

A planted compile_error!("ISSUE_10937_COMPILE_SENTINEL") also made the wrapped cargo build fail with that exact diagnostic, proving the build check was live; it was removed before behavioral validation.

Validation on Linux x86-64, perry-dev profile, --test-threads=1, through /root/perry-suite.sh:

  • Full codegen suite: 2,170 passed, 0 failed, 6 ignored; minimum 2,170 asserted.
  • Runtime library suite: 4,246 passed, 0 failed, 6 ignored; minimum 4,215 asserted. Child-process summaries were excluded from this total.
  • Official parity runner, fast mode with paired prebuilt archives and cache disabled: 2 passed, 0 failed, 0 skipped — the new fixture and fix(#10904): a + tree must not read a leaf after a conversion that precedes it #10921's original test_parity_region_guards.ts. The new fixture contains six checked cases.
  • Node-version consistency, file-size gate, and diff whitespace checks passed.

Exact Rust suite commands (all builds/tests over SSH; push and gh local):

export PATH=/root/.cargo/bin:$PATH
/root/perry-build-slot.sh /root/perry-suite.sh cargo test --profile perry-dev -j 4 -p perry-codegen -p perry-runtime --lib -- --test-threads=1
/root/perry-build-slot.sh /root/perry-suite.sh cargo test --profile perry-dev -j 4 -p perry-codegen -- --test-threads=1

The first command's codegen library count was 1,663; the full second command supplies the 2,170 total above. Runtime was not rerun.

Lint was opened and inspected at the current head. Its only failed step is Public benchmark evidence freshness: public artifact benchmark inputs changed; regenerate it with ./benchmarks/run_public_baseline.sh. This is the campaign's known structural failure; no other lint step failed.

Ralph Küpper added 2 commits September 22, 2026 09:23
Written and run before any production edit. Node 26.5.1 passes all six cases; installed Perry 0.5.1520 reports the two expected failures (6 vs 103, 10 vs 107), with exactly one conversion and a mutated late slot. Four right-associated/snapshot controls pass. The fixture asserts all six cases executed. The assigned base already carries the production fix from #10921.
@coderabbitai

coderabbitai Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 1591e00b-6cf2-45e6-8657-d4d6febcc29f

📥 Commits

Reviewing files that changed from the base of the PR and between a022cf2 and baa4a21.

📒 Files selected for processing (2)
  • changelog.d/10959-declared-number-add-order.md
  • test-files/test_gap_10937_declared_number_add_order.ts

Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review.


📝 Walkthrough

Walkthrough

The pull request adds regression coverage for declared-number addition evaluation order. The test covers array elements and class fields across left-associated, right-associated, and snapshot expressions. A changelog fragment records the coverage.

Changes

Declared-number addition regression test

Layer / File(s) Summary
Regression test assertions
test-files/test_gap_10937_declared_number_add_order.ts, changelog.d/10959-declared-number-add-order.md
The test adds shared assertions for results, one valueOf call, and the mutated late operand. The changelog records the regression coverage and existing production fix.
Array and field evaluation cases
test-files/test_gap_10937_declared_number_add_order.ts
The test covers six array-element and class-field cases for left-associated, right-associated, and snapshot additions. It requires all cases to execute and pass.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Other · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to baa4a

This change strengthens coverage for declared-number addition evaluation order without changing production behavior. It is ready to merge.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 1 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Issue #10937 requires declared-number addition-order regression coverage for array elements and this fields. The new fixture tests left-associated chains with expected late mutations to 100. It te…
Out of Scope Changes check ✅ Passed The changes are limited to the #10937 regression fixture and its changelog entry. The fixture assertions and documentation directly support the linked issue. No unrelated production logic or unrelated…
Title check ✅ Passed The title clearly identifies the regression test and the declared-number addition evaluation-order behavior addressed by the pull request.
Description check ✅ Passed The description provides the issue reference, change summary, detailed test coverage, validation commands, results, and known lint status. It does not use all template headings or include the checklis…
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 1 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Landed on main in merge train 256 (#11018, v0.5.1638), main f5cfbff882.

Carried at head baa4a2102a. The landed tree is byte-identical to the validated train tree (9b108dd3c9), and CI on the train head passed every job except the known public-baseline lint step: all 6 gap shards, cargo-test, e2e-scoped, gc-stress, check, warnings and security-audit green.

Trains rebase-merge, so commits get new SHAs and GitHub cannot mark this PR merged. Closed as landed.

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

Labels

None yet

Projects

None yet

1 participant