perf(hir): type a for-head binding so a loop-built object literal stops being declared as pointer slots (#7544) - #7550
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesFor-initializer type inference
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ForLowering
participant infer_decl_type
participant AnonymousShape
participant GarbageCollector
ForLowering->>infer_decl_type: infer let or const initializer type
infer_decl_type-->>ForLowering: return inferred Type
ForLowering->>AnonymousShape: create typed anonymous-shape fields
AnonymousShape->>GarbageCollector: publish numeric or pointer-bearing layout
GarbageCollector-->>AnonymousShape: retain values across collection
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@changelog.d/7550-anon-shape-numeric-field-types.md`:
- Around line 75-76: Clarify the two GC test counts in the changelog entry by
separating them with commas if each is a single count, or by labeling each value
if they represent distinct measurements; preserve the surrounding test
conditions and results.
In `@crates/perry-hir/tests/anon_shape_field_types.rs`:
- Around line 215-218: Update the test loop in anon_shape_field_types to declare
the inner shadowing binding as const i = z and construct the object with { r: i
}, replacing the i2 binding and reference so it verifies same-name shadowing of
the for-loop counter.
In `@test-files/test_gap_7544_anon_shape_numeric_fields.ts`:
- Around line 76-79: The dynamic-write test cases currently annotate object
literals, potentially preventing the raw-f64 initial shape from being exercised.
In test-files/test_gap_7544_anon_shape_numeric_fields.ts at lines 76-79 and
99-102, remove the type annotations from both objects (the `o` declarations) so
`{ n: i }` and `{ m: i }` are inferred naturally, while retaining the array type
`number | string`.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8637f5fb-5dce-4f12-b157-b9f5d6f1b712
📒 Files selected for processing (6)
changelog.d/7550-anon-shape-numeric-field-types.mdcrates/perry-hir/src/destructuring/mod.rscrates/perry-hir/src/destructuring/var_decl.rscrates/perry-hir/src/lower/stmt.rscrates/perry-hir/tests/anon_shape_field_types.rstest-files/test_gap_7544_anon_shape_numeric_fields.ts
| compile and run time — 100 131 copying minors, 100 131 quarantined from-space | ||
| page-sets, exit 0 — and under `PERRY_GC_VERIFY_EVACUATION=1`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify the GC test counts.
100 131 copying minors and 100 131 quarantined from-space page-sets are ambiguous. If each value is one count, use a separator such as 100,131. If they are separate values, label both values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@changelog.d/7550-anon-shape-numeric-field-types.md` around lines 75 - 76,
Clarify the two GC test counts in the changelog entry by separating them with
commas if each is a single count, or by labeling each value if they represent
distinct measurements; preserve the surrounding test conditions and results.
| declare const z: any; | ||
| for (let i = 0; i < 10; i++) { | ||
| const i2 = z; | ||
| const o = { r: i2 }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test actual same-name shadowing.
Line 217 declares i2, so this test only covers an unrelated local. Declare an inner const i = z and use { r: i }. This verifies that lookup resolves the shadowing binding instead of the for counter.
Proposed test correction
- const i2 = z;
- const o = { r: i2 };
+ const i = z;
+ const o = { r: i };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| declare const z: any; | |
| for (let i = 0; i < 10; i++) { | |
| const i2 = z; | |
| const o = { r: i2 }; | |
| declare const z: any; | |
| for (let i = 0; i < 10; i++) { | |
| const i = z; | |
| const o = { r: i }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-hir/tests/anon_shape_field_types.rs` around lines 215 - 218,
Update the test loop in anon_shape_field_types to declare the inner shadowing
binding as const i = z and construct the object with { r: i }, replacing the i2
binding and reference so it verifies same-name shadowing of the for-loop
counter.
| const healed: { n: number | string }[] = []; | ||
| for (let i = 0; i < 20000; i++) { | ||
| const o: { n: number | string } = { n: i }; | ||
| if (i % 2000 === 0) healed.push(o); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Start both dynamic-write cases from unannotated numeric literals.
The annotations can make the initial shape pointer-bearing before the string write. The test can then pass without exercising layout downgrade from a raw-f64 numeric field. Keep the array type as number | string, but infer each object literal from { n: i } or { m: i }.
test-files/test_gap_7544_anon_shape_numeric_fields.ts#L76-L79: Remove the annotation fromo.test-files/test_gap_7544_anon_shape_numeric_fields.ts#L99-L102: Remove the annotation fromo.
Proposed test correction
- const o: { n: number | string } = { n: i };
+ const o = { n: i };
...
- const o: { m: number | string } = { m: i };
+ const o = { m: i };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const healed: { n: number | string }[] = []; | |
| for (let i = 0; i < 20000; i++) { | |
| const o: { n: number | string } = { n: i }; | |
| if (i % 2000 === 0) healed.push(o); | |
| const healed: { n: number | string }[] = []; | |
| for (let i = 0; i < 20000; i++) { | |
| const o = { n: i }; | |
| if (i % 2000 === 0) healed.push(o); |
| const healed: { n: number | string }[] = []; | |
| for (let i = 0; i < 20000; i++) { | |
| const o: { n: number | string } = { n: i }; | |
| if (i % 2000 === 0) healed.push(o); | |
| const mixed: { m: number | string }[] = []; | |
| for (let i = 0; i < 20000; i++) { | |
| const o = { m: i }; | |
| if (i % 2000 === 0) mixed.push(o); |
📍 Affects 1 file
test-files/test_gap_7544_anon_shape_numeric_fields.ts#L76-L79(this comment)test-files/test_gap_7544_anon_shape_numeric_fields.ts#L99-L102
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test-files/test_gap_7544_anon_shape_numeric_fields.ts` around lines 76 - 79,
The dynamic-write test cases currently annotate object literals, potentially
preventing the raw-f64 initial shape from being exercised. In
test-files/test_gap_7544_anon_shape_numeric_fields.ts at lines 76-79 and 99-102,
remove the type annotations from both objects (the `o` declarations) so `{ n: i
}` and `{ m: i }` are inferred naturally, while retaining the array type `number
| string`.
Unrelated breakage found on
|
) `for (let i = 0; …)` hardcoded `Type::Any` for the head binding while the statement-level `let i = 0;` runs `infer_decl_type` and gets `Number`. The gap is GC-visible: a closed-shape object literal lowers to `new __AnonShape_<hash>(…)` whose `ClassField::ty` comes from `infer_type_from_expr` over each property's value, so inside a `for` loop `{ v: i, w: i + 1 }` minted two `Any` fields — and `Any` is pointer-bearing, so the most common allocation shape in the language was declared to the collector as two POINTER slots. Both for-head declarator sites now delegate to the same `infer_decl_type` the statement-level declarator uses. `var` heads keep `Type::Any` (they are function-scoped and var-hoisted, so the declarator is not the only writer). Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
…ling (#7544) `crates/perry-hir/tests/anon_shape_field_types.rs` pins both directions of what a closed-shape literal mints. The four positive cases fail when the change is stubbed out (verified by hand: replace `for_init_binding_type`'s body with `Type::Any` and 4 of 7 go red); the three boundary cases — a bare parameter, an explicit `any`, a `var` head, and an inner shadow — pass either way, which is the point: a value we cannot type must still mint `Any`. `test-files/test_gap_7544_anon_shape_numeric_fields.ts` is the runtime witness, byte-diffed against the node oracle. It builds literals through the new path, forces collections, and asserts every element survives; then it writes freshly allocated heap strings over already-constructed *numeric* fields, collects again, and reads them back. The subject is verified live in the emitted IR: three `js_gc_declare_typed_shape_layout` calls and a raw-f64 mask on the pure-numeric shapes, with a pointer mask only on the mixed one. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
8595988 to
7346528
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Full gap suite, both arms — zero status differences across all 490 testsThe #6377 gate. Two complete runs of the whole gap suite, same session, same
Not just equal totals — the named sets are identical: the same 18 output Mechanics, for the record: the baseline arm ran from this worktree at More unrelated breakage:
|
| test | status on main |
|---|---|
test_gap_6908_proxy_array_mutators |
crash |
test_gap_fetch_request_from_node_incoming_message |
crash |
test_gap_http_client_no_redirect_follow |
crash |
test_gap_http_overloads_3226plus |
crash |
test_gap_http_req_async_iterator |
crash |
test_gap_http_res_socket_writable_onfinished |
crash |
test_gap_net_connect_bound_value |
crash |
test_gap_diagchannel_3082_3084_3085_3086 |
parity_fail |
test_gap_zlib_3285_params |
parity_fail |
Nothing in the snapshot has started passing, so this is drift in one direction
only. scripts/run_gap_tests.sh would exit non-zero on main today.
test_gap_6908_proxy_array_mutators is not a load artefact — I reproduced it
standalone with the main binary on an idle box: it hangs (killed at a 120 s
timeout) while node completes normally. The http_*/net_*/fetch_* cluster
and zlib_3285_params look host-local (the harness binds a fixed port, 17891),
but they reproduced in the arm that ran alone, so that explanation is not
established either. Each wants triage on its own.
1 participant
Closes #7544.
The issue's premise is wrong, and the real gap is one line away
#7544 (and the note #7532 left behind) says
mint_anon_shape_class"gives everysynthesized field type
Any". It does not. It writes each property's inferredtype through verbatim, and
{ v: 1, w: 2 }already mints twoNumberfields, already gets a raw-f64 mask with a null pointer mask, and already takes
#7532's allocation-site declaration. Measured on
main, one literal in a file:What is actually
Anyis the loop form.for (let i = 0; …)hardcodedType::Anyfor its head binding, while the statement-levellet i = 0;runsinfer_decl_typeand getsNumber. Soinfer_type_from_exprsawi: Any,i + 1inherited it, and{ v: i, w: i + 1 }minted twoAnyfields.Anyispointer-bearing, so on
mainthat literal compiles to:Two pointer slots holding two doubles, installed after the constructor. That
is #7544's headline claim, and it is real — it just isn't
mint_anon_shape_class's doing.Here is the full map of what the boundary was, measured by lowering each form
and reading the minted
ClassField::ty:{a: 1}NumberNumberlet x = 0; {a: x}NumberNumberwhilecounterNumberNumberfor (let i = 0; …) {a: i}AnyNumberfor (let i = 0; …) {a: i + 1}AnyNumberfor (let i = 0; …) {a: i * 2}AnyNumberfor (var i = 0; …) {a: i}AnyAnyfunction g(p) { {a: p} }AnyAny{a: someAny}AnyAny{a: "s"}StringStringThe change
Both
for-head declarator sites inlower/stmt.rsnow call the sameinfer_decl_typethe statement-level declarator uses, instead of hardcodingType::Any. That is the whole diff;type_inferand its parent module go fromprivate to
pub(crate)to allow the call.varheads are deliberately left atType::Any: avarhead binding isfunction-scoped and var-hoisted, so its declarator is not the only writer of the
name and the statement-level parity argument does not carry over.
What is propagated, and its exact boundary
The propagated fact is the initializer-inferred type of the head binding —
byte-for-byte the same computation
let i = 0;has always performed. Nothingnew is invented and no new kind of fact enters the layout: the annotation
channel (
for (let i: number = 0; …)) is the onelet y: number = 0;alreadycarried into anon-shape fields on
main, which the table above shows.This is not a runtime type assertion, and the PR does not treat it as one.
Perry validates no declared type at runtime, so a
Number-typed field can stillreceive a pointer through a later dynamic write. That is discharged exactly
where #7532 discharged it for declared classes: the raw-f64 store guard rejects
the non-double bits, falls back to the boxed setter, and
layout_note_slotdowngrades the descriptor to
GC_LAYOUT_UNKNOWNso the collector scans the slotconservatively from then on. Verified, not assumed — see below.
Evidence
IR census —
{ v: i, w: i + 1 }in a loop, identical source, only the compiler differsjs_gc_init_typed_shape_layout(post-constructor)js_gc_declare_typed_shape_layout(at allocation)js_gc_note_slot_layoutjs_write_barrier_slotjs_string_addref_if_heap_stringjs_dynamic_string_or_number_addand the mask flips from
pointer_mask = [i64 3]toraw_f64_mask = [i64 3], pointer_mask = null— a pointer-free layout, which is #7544's statedacceptance.
Two corrections to the expected evidence, both worth recording. The anon-shape
constructor's stores never routed through
js_put_value_set— that was #7532'sdeclared-class path; the synthesized ctor's stores are direct-GEP, and what
they shed here is the three-call bookkeeping preamble above. And the collector's
byte counters do not move:
copied_bytes 361760,promoted_bytes 91216,8 cycles, byte-identical across both arms. That is expected on reflection —
mark_field_into_worklistre-validates every slot word and rejects a double, sodeclaring the slots pointers cost a visit and a reject, never retention. The
win is scan work and store bookkeeping, not retained bytes. "At or below
current" holds; "below" would have been a wrong claim.
Program output is unchanged (
1999999000000 1954on both arms).Survival and self-healing
test-files/test_gap_7544_anon_shape_numeric_fields.tsis byte-diffed againstthe pinned node oracle and asserts, in one program:
{ v: number, w: number, tag: string }literals built in a loop,10 retained across forced collections — every one readable, heap-string child
included (a wrongly-declared pointer-free layout would strand it);
{ a: number, b: number }literals — the shape that actually getsthe raw-f64 pointer-free descriptor;
allocated heap strings over it, collected twice, read back — the self-healing
path;
strings out of the same shape.
The subject is verified live in the emitted IR rather than assumed: three
js_gc_declare_typed_shape_layoutcalls, raw-f64 masks on the pure-numericshapes and on the two contradicted ones, and a pointer mask (
[i64 4], slot 2)only on the mixed
{v, w, tag}shape — which correctly does not qualify forthe allocation-site declaration.
Run under the sharpest instruments the repo has, compiled and run with
PERRY_GC_MOVING_LOOP_POLLS=1:The
retired_set=count is checked, not just the exit code: a run with zerocopying minors protects nothing.
Sabotage-tested
crates/perry-hir/tests/anon_shape_field_types.rshas 7 tests. Replacingfor_init_binding_type's body withType::Anyturns 4 of 7 red (thepositive cases) and leaves the 3 boundary cases green — verified by hand. A test
that passes with the change removed would not be evidence of anything.
Gates
raw_handle_debt.py999 (baseline 999) ·gc_store_site_inventory.pypass ·check_file_size.shpass ·cargo fmt --all -- --checkclean.addr_class_inventory.pyexits 0 but reports a stale ratchet baseline:lone-valid-obj-ptr | crates/perry-runtime/src/object/field_get_set.rs: baseline says 1, found 0. That file is not in this diff — it is pre-existing onmainand wants
--write-baselinein a separate commit.Summary by CodeRabbit
Bug Fixes
letandconstbindings inforloops.Tests