perf(hir): type a for-initializer declaration instead of registering it as Any (#7547) - #7552
Merged
Merged
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 (7)
📝 WalkthroughWalkthroughThe change adds shared type inference for ChangesFor-initializer typing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ForLoopLowering
participant for_init_decl_type
participant infer_decl_type
participant LocalDefinition
participant StmtLet
ForLoopLowering->>for_init_decl_type: compute declarator type
for_init_decl_type->>infer_decl_type: infer identifier binding
infer_decl_type-->>for_init_decl_type: return Type
for_init_decl_type-->>ForLoopLowering: return inferred Type
ForLoopLowering->>LocalDefinition: define loop local
ForLoopLowering->>StmtLet: emit typed binding
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/7552-for-init-local-types.md`:
- Around line 20-24: Revise the changelog wording to scope the `infer_decl_type`
change to plain-identifier `let` and `const` bindings in for-init declarators.
Explicitly exclude non-identifier patterns and `var` declarations, which retain
their existing behavior.
- Around line 39-43: Fix the benchmark paragraph’s Markdown formatting by
closing the bold delimiter after the `188,048` value in the
`pointer_free_slots_skipped` comparison. Preserve the surrounding benchmark text
and metrics unchanged.
In `@crates/perry-hir/src/destructuring/var_decl/type_infer.rs`:
- Around line 28-34: Update the for-initializer lowering flow around
for_init_decl_type and infer_decl_type so plain_object_locals is scoped
consistently with block locals: preserve the set before entering the loop/block
scope and restore it on scope exit, or remove each loop-local name when popping
the scope. Ensure names such as an inner x do not remain classified as plain
objects for later outer-scope static-call dispatch.
In `@crates/perry-hir/src/lower_decl/body_stmt.rs`:
- Around line 754-763: Split one coherent lowering path from the oversized
body_stmt module into a separate module, preserving the existing behavior and
visibility needed by its callers, including the for-loop declaration handling
around for_init_decl_type and define_local. Update module wiring and imports
accordingly, then run scripts/check_file_size.sh to verify all files remain
under 2,000 lines.
In `@crates/perry-hir/src/lower/stmt.rs`:
- Around line 1518-1523: Lower all declarators in each multi-declarator for
initializer in source order, rather than processing decls.skip(1) before the
first declarator. Update the for-initializer lowering around for_init_decl_type
and ctx.define_local in crates/perry-hir/src/lower/stmt.rs:1518-1523 and apply
the same ordering fix in crates/perry-hir/src/lower_decl/body_stmt.rs:710-713.
🪄 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: 5dcfaaee-5dce-4abf-80e5-407124156d08
📒 Files selected for processing (6)
changelog.d/7552-for-init-local-types.mdcrates/perry-hir/src/destructuring/mod.rscrates/perry-hir/src/destructuring/var_decl.rscrates/perry-hir/src/destructuring/var_decl/type_infer.rscrates/perry-hir/src/lower/stmt.rscrates/perry-hir/src/lower_decl/body_stmt.rs
Comment on lines
+20
to
+24
| For-init declarators now route through the same `infer_decl_type` the ordinary | ||
| declaration path uses (annotation first, else the initializer's inferred type, | ||
| else the tsgo-resolved fallback). **`var` is deliberately left alone**: it is | ||
| function-scoped and hoisted, so it can be written before its declaration runs | ||
| and its assignment story is not the same one. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Limit the changelog claim to plain-identifier let/const bindings.
Line 20 says that all for-init declarators use infer_decl_type. The helper returns Type::Any for non-identifier patterns, and both lowering paths leave var declarations unchanged. State the exact scope of the change.
Proposed wording
-For-init declarators now route through the same `infer_decl_type` the ordinary
-declaration path uses
+Plain-identifier `let`/`const` for-init declarators now route through the same
+`infer_decl_type` the ordinary declaration path uses📝 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.
Suggested change
| For-init declarators now route through the same `infer_decl_type` the ordinary | |
| declaration path uses (annotation first, else the initializer's inferred type, | |
| else the tsgo-resolved fallback). **`var` is deliberately left alone**: it is | |
| function-scoped and hoisted, so it can be written before its declaration runs | |
| and its assignment story is not the same one. | |
| Plain-identifier `let`/`const` for-init declarators now route through the same | |
| `infer_decl_type` the ordinary declaration path uses (annotation first, else the initializer's inferred type, | |
| else the tsgo-resolved fallback). **`var` is deliberately left alone**: it is | |
| function-scoped and hoisted, so it can be written before its declaration runs | |
| and its assignment story is not the same one. |
🤖 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/7552-for-init-local-types.md` around lines 20 - 24, Revise the
changelog wording to scope the `infer_decl_type` change to plain-identifier
`let` and `const` bindings in for-init declarators. Explicitly exclude
non-identifier patterns and `var` declarations, which retain their existing
behavior.
Comment on lines
+39
to
+43
| The mechanism is directly visible in the collector trace, not inferred: | ||
| `churn_alloc`'s `pointer_slots_read` falls **376,504 → 188,456** while | ||
| `pointer_free_slots_skipped` rises **0 → 188,048`. The literal's payload is now | ||
| skipped instead of scanned. Cycles (105), bytes copied (0.0036 GB), promoted | ||
| bytes (64) and peak RSS (24.2 MB) are unchanged. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Close the bold delimiter in the benchmark paragraph.
Line 41 starts bold text with ** but ends the number with a single backtick. This can render the following text incorrectly. Close the bold span after 188,048.
Proposed fix
-`pointer_free_slots_skipped` rises **0 → 188,048`. The literal's payload is now
+`pointer_free_slots_skipped` rises **0 → 188,048**. The literal's payload is now📝 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.
Suggested change
| The mechanism is directly visible in the collector trace, not inferred: | |
| `churn_alloc`'s `pointer_slots_read` falls **376,504 → 188,456** while | |
| `pointer_free_slots_skipped` rises **0 → 188,048`. The literal's payload is now | |
| skipped instead of scanned. Cycles (105), bytes copied (0.0036 GB), promoted | |
| bytes (64) and peak RSS (24.2 MB) are unchanged. | |
| The mechanism is directly visible in the collector trace, not inferred: | |
| `churn_alloc`'s `pointer_slots_read` falls **376,504 → 188,456** while | |
| `pointer_free_slots_skipped` rises **0 → 188,048**. The literal's payload is now | |
| skipped instead of scanned. Cycles (105), bytes copied (0.0036 GB), promoted | |
| bytes (64) and peak RSS (24.2 MB) are unchanged. |
🤖 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/7552-for-init-local-types.md` around lines 39 - 43, Fix the
benchmark paragraph’s Markdown formatting by closing the bold delimiter after
the `188,048` value in the `pointer_free_slots_skipped` comparison. Preserve the
surrounding benchmark text and metrics unchanged.
Comment on lines
+28
to
+34
| pub(crate) fn for_init_decl_type(ctx: &mut LoweringContext, decl: &ast::VarDeclarator) -> Type { | ||
| let ast::Pat::Ident(ident) = &decl.name else { | ||
| return Type::Any; | ||
| }; | ||
| let name = ident.id.sym.to_string(); | ||
| infer_decl_type(ctx, decl, ident, &name) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 \
'plain_object_locals|push_block_scope|pop_block_scope|enter_scope|exit_scope' \
crates/perry-hir/srcRepository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "## files containing plain_object_locals declarations and methods"
rg -n 'plain_object_locals' crates/perry-hir/src -g '*.rs'
echo
echo "## LoweringContext definitions/usages of plain_object_locals"
rg -n -C 6 'plain_object_locals|struct.*LoweringContext|fn push_block_scope|fn pop_block_scope|fn enter_scope|fn exit_scope' crates/perry-hir/src -g '*.rs'Repository: PerryTS/perry
Length of output: 22497
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "## context.rs push/pop block scope implementations"
sed -n '1629,1675p' crates/perry-hir/src/lower/context.rs
echo
echo "## static receiver plain_object_locals call"
sed -n '130,165p' crates/perry-hir/src/lower/expr_call/static_receiver.rs
echo
echo "## inspect var_decl type_infer relevant implementation"
sed -n '1,95p' crates/perry-hir/src/destructuring/var_decl/type_infer.rsRepository: PerryTS/perry
Length of output: 8903
Scope plain_object_locals like block locals.
push_block_scope/pop_block_scope restore locals but do not track plain_object_locals, and infer_decl_type inserts plain-object names into it. As written, for (let x = {}; ...) leaves x in that set after ctx.pop_block_scope, so a later same-named outer x can resolve as a plain object at static-call dispatch. Save the current set or remove the loop-local name when popping the block scope.
🤖 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/src/destructuring/var_decl/type_infer.rs` around lines 28 -
34, Update the for-initializer lowering flow around for_init_decl_type and
infer_decl_type so plain_object_locals is scoped consistently with block locals:
preserve the set before entering the loop/block scope and restore it on scope
exit, or remove each loop-local name when popping the scope. Ensure names such
as an inner x do not remain classified as plain objects for later outer-scope
static-call dispatch.
Comment on lines
+754
to
+763
| // #7547: the loop variable of | ||
| // `for (let j = 0; …)`. Typing it is what | ||
| // unblocks every expression computed from | ||
| // it — see `for_init_decl_type`. | ||
| let ty = crate::destructuring::for_init_decl_type(ctx, decl); | ||
| let id = ctx.define_local(name.clone(), ty.clone()); | ||
| Some(Box::new(Stmt::Let { | ||
| id, | ||
| name, | ||
| ty: Type::Any, | ||
| ty, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Split this module to satisfy the file-size limit.
The supplied file reaches Line 2078. This exceeds the repository limit of 2,000 lines. Split a lowering path into a separate module and run scripts/check_file_size.sh before pushing.
As per coding guidelines, files must stay within the repository’s 2,000-line limit and scripts/check_file_size.sh must run before pushing.
🤖 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/src/lower_decl/body_stmt.rs` around lines 754 - 763, Split
one coherent lowering path from the oversized body_stmt module into a separate
module, preserving the existing behavior and visibility needed by its callers,
including the for-loop declaration handling around for_init_decl_type and
define_local. Update module wiring and imports accordingly, then run
scripts/check_file_size.sh to verify all files remain under 2,000 lines.
Source: Coding guidelines
Comment on lines
+1518
to
+1523
| // #7547: mirrors the function-body path in | ||
| // `lower_decl/body_stmt.rs` — a for-init | ||
| // declarator gets the ordinary declaration's | ||
| // type, not a hardcoded `Any`. | ||
| let ty = crate::destructuring::for_init_decl_type(ctx, decl); | ||
| let id = ctx.define_local(name.clone(), ty.clone()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Lower multi-declarator for initializers in source order.
Both implementations process decls.skip(1) before the first declarator. This reverses initialization order and can run the new type inference before earlier bindings exist.
- crates/perry-hir/src/lower/stmt.rs#L1518-L1523: lower the full declarator list in source order instead of emitting secondary declarators before the first.
- crates/perry-hir/src/lower_decl/body_stmt.rs#L710-L713: apply the same source-order lowering to function-body
forinitializers.
📍 Affects 2 files
crates/perry-hir/src/lower/stmt.rs#L1518-L1523(this comment)crates/perry-hir/src/lower_decl/body_stmt.rs#L710-L713
🤖 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/src/lower/stmt.rs` around lines 1518 - 1523, Lower all
declarators in each multi-declarator for initializer in source order, rather
than processing decls.skip(1) before the first declarator. Update the
for-initializer lowering around for_init_decl_type and ctx.define_local in
crates/perry-hir/src/lower/stmt.rs:1518-1523 and apply the same ordering fix in
crates/perry-hir/src/lower_decl/body_stmt.rs:710-713.
added 4 commits
August 7, 2026 02:29
…it as Any
A declaration in a `for` initializer registered `Type::Any` — the annotation
was discarded and the initializer was never inferred from. The cost is not the
loop variable but everything computed from it: `base + j` infers Any once `j`
is Any, so an object literal in the loop body mints an anon-shape class whose
fields are all Any.
Any is pointer-bearing, so `{v: number, w: number}` was handed to the collector
as TWO TRACED POINTER SLOTS with an empty raw-f64 mask — no POINTER_FREE, no
raw-f64 store path, and #7532's declare-at-allocation gate refused the shape.
For-init declarators now route through the same `infer_decl_type` the ordinary
let/const path uses. `var` is left alone: it is function-scoped and hoisted, so
its assignment story differs.
churn_alloc 1.60s -> 1.33s (1.20x), unmodified source.
proggeramlug
force-pushed
the
perf/7547-for-init-local-types
branch
from
August 7, 2026 00:37
dc97cbb to
39f04fe
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. |
This was referenced Aug 7, 2026
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.
Fixes #7547. This is what blocks #7510's
churn_alloc≥1.5× criterion, and it is not layout bookkeeping — it is type inference.The defect
ctx.define_local(name, Type::Any)at the for-init declarator sites discarded both the annotation and the initializer. The cost was never the loop variable itself — it was everything computed from it.base + jinfersAnythe momentjisAny, so an object literal in the loop body minted an__AnonShape_…class whose fields were allAny.Anyis pointer-bearing, and that is where it stops being a missed optimisation:{v: number, w: number}literal was handed to the collector as two traced pointer slots,SIDE_MASK, neverPOINTER_FREE;The isolation that pinned it (same compiler, best-of-9 interleaved) is in the issue: moving the identical annotation out of the for-initializer was worth 1.18×, while annotating it inside the initializer changed nothing.
The change
For-init declarators route through the same
infer_decl_typethe ordinary declaration path uses — annotation, else the initializer's inferred type, else the tsgo-resolved fallback. Five files, 52 lines.varis deliberately untouched. It is function-scoped and hoisted, so it can be written before its declaration runs; its assignment story is not the same one and it deserves its own analysis.Measured
Only three benchmarks compile to different code at all. The rest produce byte-identical generated objects across the two arms, so their ±2% readings are noise by construction rather than by assertion. (Compared as object bytes — the cache filename hashes codegen env and compiler identity per #6394 and always differs.)
churn_allocchurnchurn_readpush_num,push_cls,push_cls_read,deeplist,treeThe mechanism is visible in the collector trace, not inferred:
churn_alloc'spointer_slots_readfalls 376,504 → 188,456 whilepointer_free_slots_skippedrises 0 → 188,048. The literal's payload is skipped instead of scanned. Cycles (105), copied (0.0036 GB), promoted (64 B) and peak RSS (24.2 MB) unchanged.GC ratchet
All 12 probes:
correctnessidentical to the baseline arm. Every metric beyond ±5% improved, except onewall_mson a host at load 33:03_cross_gen_writesheap_used_bytes04_dead_after_deep_stackcopied_bytes04_dead_after_deep_stackrss_bytes03_cross_gen_writesrss_bytesOne item to look at rather than bank.
03_cross_gen_writesnow promotes zero bytes (was 210,700 / 4,752 objects), and a probe with no old generation has fewer old→young edges to remember. Its mutator subject is provably still live —remembered_set_insert_attemptsis 491,520 on both arms, identical — and its correctness assertion still passes, so it has not gone silently vacuous. Butremembered_set_markingdrops 1,344 → 866, so its remembered-set coverage is genuinely reduced. The probe may want a change that forces promotion to keep measuring what it was written to measure. I have not touched it.Blast radius
This is a "more type visibility" change and #6377 is the standing lesson for that class, so the testing is behavioural rather than compile-level:
test-files/programs, spread deterministically across the corpus, compiled and run under both arms: identical output, and identical again underPERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1. Three initial "diffs" were thread ids inside a pre-existing panic message — they differ run-to-run on the same binary — and are normalised out.perry-hirgreen,perry-runtimegreen (1799),perry-codegenfailure set identical toorigin/main's.Pre-existing and unrelated, but found here and worth its own issue:
test-files/test_gap_webcrypto_async_threadpool.tscrashes (Bus error, rc=138) underPERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1on both arms, with nondeterministic output. Not introduced by this change.Effect on #7510
churn_allocgoes 1.586 s → 1.325 s against the 2.44 s in #7510's description — 1.84× cumulative with #7525 and #7532, past its ≥1.5× criterion. I will confirm thegc::layoutshare separately before claiming criterion 1.Summary by CodeRabbit
Bug Fixes
forloop initializers.Documentation
Chores