fix(codegen): compute numeric provenance after the Ptr<Shape> receiver proofs it depends on - #10929
Closed
proggeramlug wants to merge 1 commit into
Closed
proggeramlug wants to merge 1 commit into
proggeramlug wants to merge 1 commit into
Conversation
…r proofs it depends on
`collectors/hir_facts.rs` computed `number_by_construction_locals` before
`collect_shape_proven_ptr_locals`. For `h = h + o.a` that asks "is h
Number-producing?" before o's receiver proof exists, and
`expr_numeric_by_construction`'s PropertyGet arm is gated on the receiver being
a tracked member — so the function-scope entry point passed `empty_members` /
`empty_fields` HARDCODED (ptr_shape_numeric.rs) and that arm could never fire.
The accumulator was therefore never admitted, however completely the receiver's
shape was proven. A probe on the `+` routing decision (expr/binary.rs
`both_numeric`) reports, on a fixture whose opt report says
`Ptr<Shape> 1 selected / 1 CONSUMED`:
left=LocalGet(num=false canon=false) right=PropertyGet(num=true canon=true)
both_numeric=false => GUARDED
The slot is proven; the local is not. Writing the same read as `o.a * 1` makes
the accumulator's own fixpoint close (its write becomes a Binary), which is why
that spelling reaches INLINE_FADD and costs half — not because the multiply
normalises anything.
This moves the computation after the receiver proofs and threads them in. Two
parameters that were hardcoded empty become real. No new admission arm, no new
provenance class, no new fact.
The shape inputs are the INTERSECTION of the proven receivers' numeric field
sets: the arm consumes one set and does not re-check which receiver a property
belongs to, so the set must be numeric on every admitted receiver. A union
would be a wrong answer, not a weaker one.
Default OFF behind PERRY_L14_NBC_ORDER=1 and keyed into the object cache; with
it off the inputs are empty and the fixpoint computes exactly what it computed
before, so the reorder is a no-op.
Refs PerryTS#10777
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe numeric-by-construction analysis now runs after shape proofs. When ChangesNumeric provenance ordering
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant collect_type_facts
participant collect_shape_proven_ptr_locals
participant shape_numeric_inputs
participant collect_number_by_construction_locals
collect_type_facts->>collect_shape_proven_ptr_locals: collect shape proofs
collect_type_facts->>shape_numeric_inputs: derive shape numeric inputs
shape_numeric_inputs->>collect_number_by_construction_locals: pass receiver members and numeric fields
collect_number_by_construction_locals-->>collect_type_facts: return numeric locals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
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 |
proggeramlug
pushed a commit
that referenced
this pull request
Sep 21, 2026
#10929 keyed the knob into the OBJECT cache but not the BUILD cache, so codegen_env_vars_are_build_cache_inputs failed (#6394's rule). The two settings emit different code -- on, 'h = h + o.a' is admitted and the '+' routes to INLINE_FADD; off, the shape inputs are empty and it stays guarded -- so it is a cache input, not an exclusion.
proggeramlug
pushed a commit
that referenced
this pull request
Sep 22, 2026
…issed collect_numeric_by_construction_locals gained shape_members and shape_numeric_fields (#10777). Four callers in ptr_shape_group_numeric_tests.rs were not updated, so the crate failed to compile as a test target while `cargo check --lib` stayed green -- the hole CI's `warnings` gate (--all-targets) exists to close. Empty sets at every updated site: that is exactly what the function computed before the reorder, so the tests keep asserting what they asserted.
proggeramlug
pushed a commit
that referenced
this pull request
Sep 22, 2026
#10929 keyed the knob into the OBJECT cache but not the BUILD cache, so codegen_env_vars_are_build_cache_inputs failed (#6394's rule). The two settings emit different code -- on, 'h = h + o.a' is admitted and the '+' routes to INLINE_FADD; off, the shape inputs are empty and it stays guarded -- so it is a cache input, not an exclusion.
Contributor
Author
|
Landed on Two defects were fixed in the train, both of which would have turned CI red:
One thing left open, filed separately: nothing exercises the ON path. The train was validated as one tree: all ratchets, |
proggeramlug
pushed a commit
that referenced
this pull request
Sep 22, 2026
#10936/#10946 added PERRY_REGION_READS and PERRY_REGION_DIAG without keying either, so codegen_env_vars_are_build_cache_inputs failed (#6394's rule). PERRY_REGION_READS is a kill switch: =0 makes both region slices decline and every guarded run lowers as individual reads instead of one shape compare plus a slot load. Emitted code differs, so it is a cache INPUT. PERRY_REGION_DIAG runs statement_run_census over the HIR and prints the counts from ModuleDiag::drop. The census result is read in exactly one place -- that eprintln! -- and nothing in lowering consults it, so the object is byte-identical with the report on and off: an EXCLUSION, with the reason. The kill switch is keyed into the OBJECT cache as well. Keying one of the two caches is exactly what #10929 got wrong one train ago, and the gate only checks the build cache, so the same gap was sitting here unreported.
proggeramlug
pushed a commit
that referenced
this pull request
Sep 22, 2026
#10936/#10946 added PERRY_REGION_READS and PERRY_REGION_DIAG without keying either, so codegen_env_vars_are_build_cache_inputs failed (#6394's rule). PERRY_REGION_READS is a kill switch: =0 makes both region slices decline and every guarded run lowers as individual reads instead of one shape compare plus a slot load. Emitted code differs, so it is a cache INPUT. PERRY_REGION_DIAG runs statement_run_census over the HIR and prints the counts from ModuleDiag::drop. The census result is read in exactly one place -- that eprintln! -- and nothing in lowering consults it, so the object is byte-identical with the report on and off: an EXCLUSION, with the reason. The kill switch is keyed into the OBJECT cache as well. Keying one of the two caches is exactly what #10929 got wrong one train ago, and the gate only checks the build cache, so the same gap was sitting here unreported.
proggeramlug
pushed a commit
that referenced
this pull request
Sep 22, 2026
#10936/#10946 added PERRY_REGION_READS and PERRY_REGION_DIAG without keying either, so codegen_env_vars_are_build_cache_inputs failed (#6394's rule). PERRY_REGION_READS is a kill switch: =0 makes both region slices decline and every guarded run lowers as individual reads instead of one shape compare plus a slot load. Emitted code differs, so it is a cache INPUT. PERRY_REGION_DIAG runs statement_run_census over the HIR and prints the counts from ModuleDiag::drop. The census result is read in exactly one place -- that eprintln! -- and nothing in lowering consults it, so the object is byte-identical with the report on and off: an EXCLUSION, with the reason. The kill switch is keyed into the OBJECT cache as well. Keying one of the two caches is exactly what #10929 got wrong one train ago, and the gate only checks the build cache, so the same gap was sitting here unreported.
proggeramlug
pushed a commit
that referenced
this pull request
Sep 22, 2026
#10936/#10946 added PERRY_REGION_READS and PERRY_REGION_DIAG without keying either, so codegen_env_vars_are_build_cache_inputs failed (#6394's rule). PERRY_REGION_READS is a kill switch: =0 makes both region slices decline and every guarded run lowers as individual reads instead of one shape compare plus a slot load. Emitted code differs, so it is a cache INPUT. PERRY_REGION_DIAG runs statement_run_census over the HIR and prints the counts from ModuleDiag::drop. The census result is read in exactly one place -- that eprintln! -- and nothing in lowering consults it, so the object is byte-identical with the report on and off: an EXCLUSION, with the reason. The kill switch is keyed into the OBJECT cache as well. Keying one of the two caches is exactly what #10929 got wrong one train ago, and the gate only checks the build cache, so the same gap was sitting here unreported.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
A bug fix, not a performance change. On the program it targets it takes a loop from 32 to 16
instructions per iteration; on a real 1,201-line TypeScript compile it changes nothing — measured,
byte-identical binary. Both numbers are below.
Refs #10777.
The defect
collectors/hir_facts.rscomputednumber_by_construction_localsbeforecollect_shape_proven_ptr_locals. Forh = h + o.a, admittinghrequires judgingo.aNumber-producing, and
expr_numeric_by_construction'sPropertyGetarm is gated on the receiver beinga tracked member. So the function-scope entry point passed:
hardcoded (
ptr_shape_numeric.rs). That arm could never fire for a function-scope walk — for anyspelling, construction or field type — so an accumulator was never admitted however completely its
receiver's shape was proven.
This is why #10777's landed read-side fix (
72206caa71) changed nothing on its target: the read wasvouched correctly and the accumulator was not. A probe on the
+routing decision(
expr/binary.rs,both_numeric) on a fixture whose opt report saysPtr<Shape> 1 selected / 1 CONSUMED:It also explains the old
o.a * 1result: that spelling gives the accumulator aBinarywrite, so itsown fixed point closes. The multiply was never normalising anything.
The change
Move the computation after the receiver proofs and thread them in. Two parameters that were
hardcoded empty become real. No new admission arm, no new provenance class, no new fact.
127 insertions, 16 deletions, five files.
One soundness decision: the shape inputs are the intersection of the proven receivers' numeric
field sets. The arm consumes one set and does not re-check which receiver a property belongs to, so the
set must be numeric on every admitted receiver. A union would be a wrong answer, not a weaker one:
anumeric onCbut not onDwould license a barefaddonD.a. Exact in the single-receivercase; an under-approximation when receivers disagree.
Default OFF behind
PERRY_L14_NBC_ORDER=1, keyed into the object cache. With it off the inputs areempty and the fixpoint computes exactly what it computed before — the reorder is pure.
Evidence
Every count below was predicted and committed before the build existed. Instrument asserted present in
the compiler before any measurement.
+GUARDEDINLINE_FADDjs_dynamic_string_or_number_addin loop bodyrun()code, off arm vs pre-change buildnode on the same fixture: 8.79–9.77 across runs, so 1.7–1.8×. Not parity, and not claimed as such.
Blast radius — the number to weigh this by:
+sitestranspileModule, 1,201-line inputsafeParse*zod's binaries differ between arms, but the same arm built twice differs by the same 38 bytes — a
string-pool permutation in perry's own output (commented on #10590), not this change. Symbol-level
diff: 0 functions change.
Why zero on tsc: the fix only helps an accumulator whose writes read a field of a receiver with a
Ptr<Shape>proof. tsc's+sites read fromany-typed AST nodes, parameters and receivers thecompiler cannot name — exactly where
Ptr<Shape>fails. The fixture shows the effect because it wasbuilt so the proof would fire.
Differential suite — 893-program gap suite on exactly this diff (no probes, no other changes;
compiler asserted to carry the change and not the diagnostics), with the change ON
(
PERRY_L14_NBC_ORDER=1), pinned node v26.5.1:Both flagged regressions were rerun on the same PR compiler, with the change off and on, three
runs each against node:
test_gap_6558_webassembly_graceful_failtest_gap_9421_async_output_flushNeither is caused by this change:
that rebuild measured 223 s here, against the harness's 300 s per-compile limit on a
contended host. With room to finish it builds and passes both ways. (Related harness issue: harness: run_parity_tests.sh discards its build error (2>/dev/null), and a missing cargo turns an auto-optimize fallback into 16 false compile_fail regressions #10920.)
FAIL PASS FAIL. Output that varies between runs of the same binary cannot come from a compile-time
change. It may be worth marking.
With the change off, the emitted
run()is identical to the pre-change build — the knob is free when off.Why land it anyway
It removes a defect that made an existing rule permanently unreachable: without it, no accumulator
whose writes read a proven field could be admitted to
number_by_construction_locals, whatever thereceiver's proof. That is worth fixing on its own terms.
What it does not do, stated so a reviewer does not infer it: it does not prepare the object-model
work's region path. Versioned loop regions classify accumulators with a different walker
(
stable_packed_accumulator::collect_numeric_accumulatorsplus a preheader admission test on eachaccumulator's entry value), not with
collect_numeric_by_construction_locals, which is what this PRparameterises. The fix is a bug fix to the function-wide fact and nothing more.
Summary by CodeRabbit
Compiler Improvements
PERRY_L14_NBC_ORDERenvironment variable.Bug Fixes