codegen: Number locals are one scoped rule; the shape-field leaf is unconditional (step 5L, P5) - #11662
Merged
Conversation
…p 5L, P5) A local is Number in scope S iff every write reaching a read in S is Number-producing. The function scope is number_by_construction_locals; a guarded loop clone opens its own scope in the receiver descriptor table (materialize_number_locals), ended by dematerialize_scope like every other scoped payload. type_analysis::local_is_number is the one query. Deleted: ElementShapeLoopFact::numeric_accumulator, StablePackedLoopFact::numeric_accumulators, PackedF64LoopFact and MaskedWindowArrayFact numeric_accumulators, StringWindowArrayFact::numeric_accumulator, and the per-family LocalGet disjunction in is_numeric_expr. Wired to the query: is_numeric_expr, expr_produces_canonical_raw_f64 (the LocalGet arm admitted integer locals only), the shadow-slot mirror skip, the non-pointer shadow value test, temp-root inertness, the Update coerce skip, the bitwise leaf and the declared-only violability test.
… (charter step 5L, P5) The sixth Number-local set, FnCtx::masked_region_scalar_locals, is deleted. A masked-window fast copy now admits a flow-refined local to its own scope (ReceiverDescriptorTable::admit_number_local) after the statement that wrote a Number, and withdraws it at the first write it cannot prove Number; the copy's dematerialize_scope ends the rest. type_analysis::local_is_number is the one query: the shadow-mirror skip no longer has a second arm, and every consumer of the query (is_numeric_expr, raw-f64 LocalGet, bitwise leaf, Update coerce, temp-root inertness) now sees the refinement too. The refinement lands strictly after its statement and the region admits only top-level scalar LocalSet/Update/pure statements, so a member holds a Number at every read inside the scope. Tests: receiver_regions_tests a_flow_refined_number_local_joins_and_leaves_its_copy_scope; gap fixture test_gap_masked_region_number_scope.ts (un-refine to a numeric string, refined reads in and after the region, ta_i32 / plain_f64 / slow copies). Sabotage: withdraw_number_local as a no-op turns the unit test red.
…al (charter step 5L) h = h + o.a with o = new C(...) never admitted h as a Number: the #10777 shape-field leaf (a read of a proven-numeric field on a shape-proven receiver) was computed in the right order but its inputs were gated behind PERRY_L14_NBC_ORDER, default off, so collect_number_by_construction_locals always saw empty shape members. The knob, its build-cache entry and its object-cache key are deleted; shape_numeric_inputs always hands the fixpoint the receivers and the intersection of their numeric fields. The fields come from prove_numeric_fields (every reachable store is Number-producing by construction, never the declared type), the same proof that already licenses a bare load double of o.a. fpxnum (5L fixture): 25 -> 10 instr/iter; the loop no longer calls js_dynamic_string_or_number_add and h takes no root barrier. Tests: gap fixture test_gap_number_local_shape_field_leaf.ts (conditional and missing constructor stores, delete, string stores through any, escaping sink, computed key, method store, Object.assign, getter, NaN, the accumulator). Sabotage: shape_numeric_inputs returning empty sets turns shape_inputs_intersect_numeric_fields_before_proving_property_locals red.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
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 (30)
💤 Files with no reviewable changes (7)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughCodegen now uses one query to determine whether a local holds a Number, combining function-wide proofs with proofs scoped to guarded clones. Fast-loop and masked-copy lowering manage scoped proofs. Shape-based numeric inputs no longer depend on ChangesNumber-local proof and lowering
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to The change unifies how code generation decides that a local holds a Number and makes shape-field inputs unconditional. No concrete merge-blocking issue was identified in the supplied material, so it is mergeable after normal checks. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The change affects how compiled programs classify numeric values and maintain GC roots. The reviewed proof and cleanup paths have safeguards, and no specific security defect was established, but the wider activation and security-sensitive consumers warrant design review. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 55.77% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 52 functions across 20 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
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.
Charter step 5L, slice P5: Number locals as ONE scoped rule.
What
type_analysis::local_is_numberanswers from the function-widenumber_by_construction_localsset plus a scope opened by each guarded loop clone. The scope closes withdematerialize_scope, like the table's other scoped facts. It is the only way to ask whether a local is a Number.LocalGetspecial cases inis_numeric_expr, and the sixth set,masked_region_scalar_locals. A masked-window fast copy now admits a flow-refined local into its own scope and withdraws it at the first write it can't prove is a Number.expr_produces_canonical_raw_f64) and the shadow-slot skip.PERRY_L14_NBC_ORDER(fix(codegen): compute numeric provenance after the Ptr<Shape> receiver proofs it depends on #10929, default off) is deleted, along with its build-cache and object-cache keys. With it off,shape_numeric_inputsreturned empty sets, so a proven numeric field read (acc += o.awitho = new C(...)) never made its local a Number.Results (Linux x86_64, base = main 58d8680, outputs identical to node)
js_dynamic_string_or_number_add;run()root barriers 12 → 4)tsc and Zod don't contain the pattern the leaf targets. The slices that move the param/lit matrix rows are P6 (barrier elision) and P7 (region F64 reads).
Verification
test_gap_number_local_shape_field_leaf.tscovers where the field proof must refuse (a conditional or missing ctor store, delete, a string store throughany, an escaping sink, a computed key, a method store, Object.assign, a getter, NaN)shape_numeric_inputsreturning empty, each turn their unit tests redSummary by CodeRabbit