fix(hir): destructured var binding reuses its hoisted boxed slot for forward closure capture#5474
Merged
Merged
Conversation
…r forward closure capture
A `var { x } = ...` whose name is captured by a closure declared EARLIER in the
same function read `undefined`:
function consumer() {
function parse() { return dSq.COMPARATOR; } // captures dSq
var { t: dSq } = factory(); // destructuring var, declared after
return parse();
}
The function-body var-hoist pre-pass correctly pre-defines + boxes a forward-
captured `var` slot, but the destructuring leaf lowering allocated a FRESH local
for the bound name instead of reusing the pre-hoisted boxed id (the plain
`Pat::Ident` var path already reused it; the destructuring leaf did not). So the
`= factory()` write landed in a different slot than the box the closure captured,
and the closure read the never-written box.
- destructuring/pattern_binding.rs: the `Pat::Ident` leaf reuses an existing
var-hoisted binding of the same name before allocating fresh (gated on
var_hoisted_ids membership so shadowing let/const/catch still get fresh slots).
- lower/expr_function.rs: the fn-expression body's inline var pre-pass now walks
the whole pattern (via collect_var_binding_names_from_pat), pre-registering
destructured var leaves (esbuild's CJS module factory is a fn-expression).
- lower_decl/{block,mod}.rs: expose collect_var_binding_names_from_pat.
Surfaced bringing up `@anthropic-ai/claude-code` natively: semver's
`var { safeRe, t } = require('./re')` with a class method capturing `t`.
📝 WalkthroughWalkthroughFixes a compiler bug where destructuring ChangesDestructured var forward capture fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
🤖 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 `@crates/perry-hir/src/destructuring/pattern_binding.rs`:
- Around line 375-409: The hoisted-slot reuse logic in the `None` branch does
not check whether the current destructuring binding is a `var` declaration or a
non-`var` declaration (let/const), causing non-var destructuring to incorrectly
reuse an outer hoisted var slot instead of creating a fresh binding. Modify the
reuse condition to only allow slot reuse for `var` declarations, ensuring that
`let`/`const` destructuring bindings always create fresh local bindings instead
of reusing existing hoisted var slots. This requires checking the declaration
kind before the existing find operation that searches through
ctx.var_hoisted_ids.
🪄 Autofix (Beta)
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: 063a1979-c571-4fea-9e78-3e48eeab0f61
📒 Files selected for processing (5)
crates/perry-hir/src/destructuring/pattern_binding.rscrates/perry-hir/src/lower/expr_function.rscrates/perry-hir/src/lower_decl/block.rscrates/perry-hir/src/lower_decl/mod.rscrates/perry/tests/issue_cjs_destructured_var_forward_capture.rs
CodeRabbit: `lower_pattern_binding_into` is shared by `let`/`const`
destructuring too, so the var-hoisted-slot reuse in the `Pat::Ident` leaf
could make an inner `let { x }` alias an outer hoisted `var x` of the same
name, breaking lexical shadowing. Thread an `is_var_decl` flag through
`lower_pattern_binding`/`_into`/`lower_array_pattern_binding` and gate the
reuse arm on it; non-`var` leaves always allocate a fresh binding.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SQ4RdAWQhxvqyS9vWeLoTB
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/perry-hir/src/lower/stmt.rs (1)
1391-1430:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
constimmutability infordestructuring.Both non-
varpaths calllower_pattern_binding(..., true, false), sofor (const { a } = obj; ...)creates mutable locals. Usevar_decl.kind != ast::VarDeclKind::Constfor themutableargument while keepingis_var_decl=false.🐛 Proposed fix
} else { + let mutable = var_decl.kind != ast::VarDeclKind::Const; for decl in var_decl.decls.iter().skip(1) { @@ let stmts = crate::destructuring::lower_pattern_binding( - ctx, &decl.name, init_expr, true, false, + ctx, &decl.name, init_expr, mutable, false, )?; @@ module.init.push(Stmt::Let { id, name, ty: Type::Any, - mutable: true, + mutable, init: init_expr, }); @@ let stmts = crate::destructuring::lower_pattern_binding( - ctx, &decl.name, init_expr, true, false, + ctx, &decl.name, init_expr, mutable, false, )?; @@ Some(Box::new(Stmt::Let { id, name, ty: Type::Any, - mutable: true, + mutable, init: init_expr, }))🤖 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 1391 - 1430, The lower_pattern_binding calls in the for loop destructuring handling have hardcoded true for the mutable parameter, causing const destructuring declarations to incorrectly create mutable locals. Replace the hardcoded true value with var_decl.kind != ast::VarDeclKind::Const for both lower_pattern_binding calls (one around line 1393 and one around line 1419) to properly preserve const immutability, while keeping the is_var_decl parameter as false.crates/perry-hir/src/destructuring/pattern_binding.rs (1)
386-423:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMirror hoisted-slot reuse for object shorthand leaves.
This fixes
Pat::Identleaves reached through recursive lowering, but{ dSq }shorthand is handled byObjectPatProp::Assignand still falls through toctx.define_local(...). A forward-capturedvar { dSq } = factory()can therefore still write a fresh slot while the earlier closure reads the pre-hoisted boxed slot.🐛 Proposed fix
None if ctx.scope_depth == 0 && ctx.inside_block_scope == 0 && ctx.pre_registered_module_vars.remove(&name) => { ctx.pre_registered_module_var_decls.remove(&name); let id = ctx.lookup_local(&name).unwrap(); if let Some(existing_ty) = ctx.locals.lookup_type_mut(&name) { *existing_ty = ty.clone(); } id } + None if is_var_decl => { + let reuse = { + let hoisted = &ctx.var_hoisted_ids; + ctx.locals + .iter_named(&name) + .find(|(_, (_, lid, _))| hoisted.contains(lid)) + .map(|(pos, (_, lid, _))| (pos, *lid)) + }; + if let Some((reuse_pos, id)) = reuse { + *ctx.locals.type_mut_at(reuse_pos) = ty.clone(); + id + } else { + ctx.define_local(name.clone(), ty.clone()) + } + } None => ctx.define_local(name.clone(), ty.clone()), };🤖 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/pattern_binding.rs` around lines 386 - 423, The hoisted-slot reuse logic for `var` declarations in destructuring is currently only applied to `Pat::Ident` leaves in the `None if is_var_decl =>` block, but it is not applied to object shorthand destructuring handled by `ObjectPatProp::Assign`. This causes object shorthand patterns like `{ dSq }` to create fresh slots instead of reusing pre-hoisted boxed slots, resulting in forward-captured closures reading undefined values. Apply the same hoisted-slot reuse logic (checking `ctx.var_hoisted_ids` and reusing existing locals when appropriate) to the `ObjectPatProp::Assign` case to ensure consistency between named and shorthand destructuring patterns.
🤖 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.
Outside diff comments:
In `@crates/perry-hir/src/destructuring/pattern_binding.rs`:
- Around line 386-423: The hoisted-slot reuse logic for `var` declarations in
destructuring is currently only applied to `Pat::Ident` leaves in the `None if
is_var_decl =>` block, but it is not applied to object shorthand destructuring
handled by `ObjectPatProp::Assign`. This causes object shorthand patterns like
`{ dSq }` to create fresh slots instead of reusing pre-hoisted boxed slots,
resulting in forward-captured closures reading undefined values. Apply the same
hoisted-slot reuse logic (checking `ctx.var_hoisted_ids` and reusing existing
locals when appropriate) to the `ObjectPatProp::Assign` case to ensure
consistency between named and shorthand destructuring patterns.
In `@crates/perry-hir/src/lower/stmt.rs`:
- Around line 1391-1430: The lower_pattern_binding calls in the for loop
destructuring handling have hardcoded true for the mutable parameter, causing
const destructuring declarations to incorrectly create mutable locals. Replace
the hardcoded true value with var_decl.kind != ast::VarDeclKind::Const for both
lower_pattern_binding calls (one around line 1393 and one around line 1419) to
properly preserve const immutability, while keeping the is_var_decl parameter as
false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c77ac44a-c9ec-4a95-b402-2f2834c91248
📒 Files selected for processing (5)
crates/perry-hir/src/destructuring/pattern_binding.rscrates/perry-hir/src/destructuring/var_decl.rscrates/perry-hir/src/lower/stmt.rscrates/perry-hir/src/lower_decl/body_stmt.rscrates/perry-hir/src/lower_patterns.rs
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
2 participants
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.
Symptom
A
var { x } = ...whose bound name is captured by a closure declared earlier in the same function readundefined:Root cause
The function-body
var-hoist pre-pass correctly pre-defines and boxes a forward-capturedvarslot. But the destructuring-leaf lowering (destructuring/pattern_binding.rs,Pat::Identarm) allocated a fresh local for the bound name instead of reusing that pre-hoisted boxed id — the plainPat::Identvar-decl path already reused it, the destructuring leaf did not. So the= factory()assignment wrote a different slot than the box the closure captured, and the closure read the never-written box. Additionally the function-expression body's inline var pre-pass (lower/expr_function.rs) only registered plainPat::Identvars, never destructuring-varleaves (esbuild's CJS module factory is a function expression).Fix
destructuring/pattern_binding.rs: thePat::Identleaf reuses an existingvar-hoisted binding of the same name before allocating fresh (gated onvar_hoisted_idsmembership, so shadowinglet/const/catch params still get fresh slots).lower/expr_function.rs: the inline pre-pass walks the whole pattern viacollect_var_binding_names_from_pat, pre-registering destructuredvarleaves.lower_decl/{block,mod}.rs: exposecollect_var_binding_names_from_pat.Validation
crates/perry/tests/issue_cjs_destructured_var_forward_capture.rs— passes (incl. multi-consumer + circular-module high-fidelity semver shape).cargo test -p perry-hir -p perry-codegen: all pass, no regressions (incl.issue_4841_namespace_cjs_reexport,issue_4972_derived_class_capture_super).Note
A distinct, deeper edge case (a class instance constructed before the capturing
varis assigned, same scope) remains — perry's class-capture snapshots by value at construction time. semver doesn't hit it (its instances are built by later callers). Documented as a follow-up.Surfaced bringing up
@anthropic-ai/claude-codenatively (semver'svar { safeRe, t } = require('./re')).Summary by CodeRabbit
Bug Fixes
vardeclarations being inaccessible to closures that capture the binding.vardeclarators, including scenarios involvingfor-loop destructuring and parameter destructuring, to ensure correct reassignment/visibility behavior.Tests
varforward-capture semantics, validating expected runtime output.