refactor(codegen): defuse the guarded-wildcard trap in accumulator_rhs_is_numeric - #9308
refactor(codegen): defuse the guarded-wildcard trap in accumulator_rhs_is_numeric#9308proggeramlug wants to merge 1 commit into
Conversation
…s_is_numeric `_ if offset_reads_inlined =>` was a guarded wildcard: any match arm placed after it was reachable only while the flag was false, with no possible compiler warning since reachability depends on the runtime guard. PerryTS#9303's first cut added an index form below it and verified as a complete no-op in the range tier while passing every flag-false test. Semantically identical — the guard moves into the body of a single unguarded catch-all, and future index forms extend that body where the same tests cover both flag states. Two-liner agreed with the PerryTS#9303 author, who rebases on it.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe change consolidates the non-local ChangesNumeric index admission
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This refactor consolidates equivalent guard behavior without changing the intended result, so no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the refactor, its rationale, semantic equivalence, and related issues. It does not follow the repository template because it omits the required Summary, Changes, Related issue, Test plan, and Checklist sections, including verification results.
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
Reviewed — the reasoning is right, and I've confirmed the rewrite is exactly semantics-preserving: It turns out #9303 already carries this same fix as part of its own change, with the masked-reads arm added inside the combined catch-all. Since #9303 is what motivated the trap in the first place, I've taken it from there rather than merging the two versions of the same edit — this branch conflicts against it. Closing as superseded, with the fix landing via #9303. The failure mode you named is worth keeping in mind generally: a guarded catch-all makes reachability depend on a runtime flag, so the compiler can't warn, and a later arm can verify as a complete no-op while every flag-false test still passes. |
|
Correction to what I said above — I was wrong that #9303 already carried this fix, and you should have the accurate version. #9303 adds the combined So your change wasn't a subset of #9303 — it was the complete version. Removing that second arm is exactly what makes the combined form correct, and #9303 alone doesn't compile warning-free without it. I've applied the deletion in the batch that carries #9303, so the fix lands there and CI stays green. Leaving this closed only because the code merges by the other route, not because the change was unnecessary — the diagnosis in your description was right, and it caught something #9303 got wrong. |
* codegen: a float accumulator over masked reads earns the dense range clone
17_loop_data_dependent: 475 ms -> 219 ms against node's 220 ms on an idle
Mac mini -- parity, from 2.16x. Sums bit-identical across 100M data-dependent
float recurrence steps.
sum = sum * x[i & 63] + x[(i * 7) & 63] // rejected
sum = sum * x[i & 63] // admitted
The discriminator was the accumulator's static numeric proof. `+` can be
concatenation, so the dense tier's per-statement proof demands both operands
numeric; a reassigned accumulator has no such proof, because its own writes
read the guarded array, whose element proof only exists once the guard has
run. A chicken-and-egg that `*` never faces -- multiplication needs only the
weaker inert fact. Confirmed by instrumenting the two conjuncts of the dense
LocalSet arm: the failing one is the proof, on exactly the fixtures whose
accumulator writes contain a plain-array read.
The matcher now peels the accumulator: when the proof fails on the LocalSet
target of a self-accumulating write, it retries with the target treated as
numeric BY CONTRACT, records it pending, and then verifies every pending
local with the same collector the lowering runs
(`collect_numeric_accumulators`), rejecting the whole dense match with its
own named trace reasons (`accumulator_needs_single_array`,
`accumulator_not_provable`) if the two disagree -- so the clone can never
contain a dynamic `+` under facts that forbid one.
The contract is enforced at run time twice over: the clone's entry emits a
genuine-double tag check on the accumulator, and the dense entry guard
validates the whole masked window hole-free. A string-seeded accumulator and
a string element both route to the slow copy and produce node's
concatenation, verified under PERRY_GC_FORCE_EVACUATE.
Supporting changes:
* `accumulator_rhs_is_numeric` accepts masked static-window reads of the
tracked array (`masked_reads_validated`), sound because the dense guard
validated the window union hole-free. Fixing that exposed a match-arm
reachability bug: `_ if offset_reads_inlined` was a guarded catch-all, so
ANY arm placed after it was unreachable whenever the flag was set -- the
first version of this change sat exactly there and verified as a no-op.
The two tests are now one combined catch-all.
* `emit_range_loop_accumulator_admission` admits a masked-only single array
(counter-bearing arrays keep priority; multiple arrays still decline).
* `MaskedWindowArrayFact` carries `numeric_accumulators` so `is_numeric_expr`
sees admitted accumulators while the clone lowers -- without this the add
inside the clone would stay dynamic, which is a collecting call under facts
that assume none (the #9259 cascade shape). Mirrors the string-window
fact's field (#9160).
perry-codegen lib 1378/0; packed-loop integration suite 59/0 across 11 files;
3 new regression tests (admission + node-identical result, string-seeded
accumulator, string element), each under forced evacuation.
Claude-Session: https://claude.ai/code/session_01Pcq6j6y57TdKSR2Zx2D187
* refactor(runtime): split ic_miss.rs and array/tests.rs under the file cap
#9302 and #9307 each tipped a file that was already within ~35 lines of the
2000-line gate. Extracts the C3C PIC test module and the Array.prototype
method-discriminator tests into sibling files; no behaviour change.
* fix(codegen): drop the now-dead catch-all after the combined accumulator arm
#9303's combined `_ =>` arm made the trailing `_ => false` unreachable, which
is a `-D warnings` failure. Removing it is #9308's fix, which the combined arm
needs to be complete.
* style: rustfmt
* chore: changelog fragment for the train13 follow-up
---------
Co-authored-by: Ralph Küpper <ralph@skelpo.com>
1 participant
Semantically identical one-hunk refactor, agreed with @ECS1 as the backstop for #9303's rebase.
_ if offset_reads_inlined =>was a guarded wildcard: any match arm placed after it is reachable only while the flag is false, and the compiler cannot warn — reachability depends on the runtime guard. #9303's first cut added an index form below it and verified as a complete no-op in the range tier while passing every flag-false test, which is exactly how such an arm fails: the flag-false tests look like coverage.The guard moves into the body of a single unguarded catch-all; future index forms extend that body, where one set of tests covers both flag states.
Refs #9279 (which introduced the arm), #9303 (which rebases onto this).
Summary by CodeRabbit