fix(hir): skip AnnexB B.3.3 legacy var on for-head/destructuring-catch early errors (#5346)#5356
Conversation
…h early errors (#5346) The #5319 AnnexB B.3.3 collection (`collect_annexb_block_fn_decl_names`) suppresses the legacy enclosing-scope `var` for a block-nested sloppy function whenever the equivalent `var` would be an early error, but it seeded the `forbidden` set only from block/switch/param lexical names. It missed two enclosing-binding sources, so the legacy `var` was wrongly created — leaking the function name to the outer scope: - **`for`/`for-in`/`for-of` lexical heads** (`for (let f; …) { function f(){} }`, `for (let f in/of …) { … }`): the loop binding scopes the body, and a same-named `var` in the body is an early error (14.7.4.1 / 14.7.5.1). - **destructuring CatchParameter** (`catch ({ f }) { function f(){} }`): B.3.5 makes a same-named `var` an early error unless the catch param is a simple `catch (e)` BindingIdentifier, which stays exempt (no-skip). Add the for-head lexical names and pattern catch-param bound names to the `forbidden` set before descending into those bodies. A simple identifier catch param is left untouched so its no-skip `var` is still created. test262 annexB/language/eval-code/direct skip-early-err cluster (node v26 differential): 42/96 -> 72/96 passing (+30). The residual failures are the `switch`-case-prefixed templates (a block function declared directly in a switch case), a separate pre-existing issue. Verified no regression: the no-skip cases fail identically on both binaries (cache-disabled), and `cargo test -p perry-hir` passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthrough
ChangesAnnex B B.3.3/B.3.5 Forbidden-Set Refinements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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/lower_decl/block.rs`:
- Around line 718-726: The `for_head_lexical_names` function currently only
handles `ast::ForHead::VarDecl` and returns an empty vector for all other
variants, including `UsingDecl`. However, `using` declarations in for-of/for-in
loops also introduce lexically scoped bindings that should be extracted. Add a
new match arm for `ast::ForHead::UsingDecl` that extracts the binding names from
the using declaration by iterating through its declarations (following the same
pattern used in `fn_ctor_env.rs` at lines 957-961). Return the collected binding
names instead of returning an empty vector for this variant.
🪄 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: b71f6b64-02b5-4024-abf9-2f7b1cb5bf19
📒 Files selected for processing (1)
crates/perry-hir/src/lower_decl/block.rs
| /// Lexical binding names of a `for-in` / `for-of` head (`for (let f in …)`). | ||
| /// A `var` head or a bare assignment-target pattern introduces no lexical | ||
| /// binding here and yields an empty list. | ||
| fn for_head_lexical_names(head: &ast::ForHead) -> Vec<String> { | ||
| match head { | ||
| ast::ForHead::VarDecl(vd) => var_decl_lexical_names(vd), | ||
| _ => Vec::new(), | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if ForHead enum has a UsingDecl variant in swc_ecma_ast
rg -n 'enum ForHead' --type rust -A 10 | head -30
# Also check if there are any for-of/for-in tests involving `using` in the test suite
rg -n 'for.*using.*of' --type rust -C 2 | head -30Repository: PerryTS/perry
Length of output: 999
🏁 Script executed:
# Check the imports in block.rs to identify what 'ast' refers to
head -50 crates/perry-hir/src/lower_decl/block.rs | grep -E '^use|^extern'Repository: PerryTS/perry
Length of output: 314
🏁 Script executed:
# Find where ForHead is defined (could be swc_ecma_ast or local)
fd -t f '*.rs' | xargs rg 'enum ForHead' -B 2 -A 15Repository: PerryTS/perry
Length of output: 1945
🏁 Script executed:
# Check if UsingDecl appears anywhere in ForHead-related code
rg 'UsingDecl' crates/perry-hir/src/ -B 2 -A 2Repository: PerryTS/perry
Length of output: 483
🏁 Script executed:
# Read the full for_head_lexical_names function and surrounding context in block.rs
sed -n '705,745p' crates/perry-hir/src/lower_decl/block.rsRepository: PerryTS/perry
Length of output: 1546
🏁 Script executed:
# Check how var_decl_lexical_names is defined
sed -n '685,720p' crates/perry-hir/src/lower_decl/block.rsRepository: PerryTS/perry
Length of output: 1408
🏁 Script executed:
# Check the fn_ctor_env.rs to see full UsingDecl handling for comparison
rg -n 'UsingDecl' crates/perry-hir/src/lower/fn_ctor_env.rs -B 5 -A 10Repository: PerryTS/perry
Length of output: 565
Handle ForHead::UsingDecl in for_head_lexical_names.
The function currently returns an empty list for any ForHead variant other than VarDecl, including UsingDecl. However, using declarations in for-of/for-in loops introduce lexically scoped bindings (per the explicit resource management proposal), similar to let and const. These bindings should be extracted and added to the forbidden set to correctly apply AnnexB legacy var restrictions.
Compare with fn_ctor_env.rs (line 957–961), which explicitly handles UsingDecl by iterating through its declarations. The fix should extract binding names from UsingDecl in the same way.
🤖 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/block.rs` around lines 718 - 726, The
`for_head_lexical_names` function currently only handles `ast::ForHead::VarDecl`
and returns an empty vector for all other variants, including `UsingDecl`.
However, `using` declarations in for-of/for-in loops also introduce lexically
scoped bindings that should be extracted. Add a new match arm for
`ast::ForHead::UsingDecl` that extracts the binding names from the using
declaration by iterating through its declarations (following the same pattern
used in `fn_ctor_env.rs` at lines 957-961). Return the collected binding names
instead of returning an empty vector for this variant.
1 participant
Summary
Partial fix for #5346 — targets the largest cited cluster, the 94× "An initialized binding is not created" failures, which the issue flags as a likely #5319 B.3.3 residual.
These are test262
annexB/language/eval-code/directcases that verify the AnnexB B.3.3 legacy block-functionvaris not created when the equivalentvarwould be an early error. The #5319 collection (collect_annexb_block_fn_decl_names) seeded itsforbiddenset only from block/switch/parameter lexical names and missed two enclosing-binding sources, so the legacyvarwas wrongly created and the function name leaked to the outer scope:for/for-in/for-oflexical heads —for (let f; …) { function f(){} },for (let f in/of …) { … }. The loop binding scopes the body; a same-namedvarin the body is an early error (14.7.4.1 / 14.7.5.1).CatchParameter—catch ({ f }) { function f(){} }. B.3.5 makes a same-namedvaran early error unless the catch param is a simplecatch (e)BindingIdentifier (which stays exempt — the no-skip case).The fix adds the for-head lexical names and pattern catch-param bound names to the
forbiddenset before descending into those bodies. A simple-identifier catch param is deliberately left untouched so its allowedvaris still created.Verification (node v26 differential, cache-disabled)
annexB/language/eval-code/directskip-early-err cluster: 42/96 → 72/96 passing (+30)try(destructuring catch)forfor-infor-ofblockswitchThe residual (4 per suffix = 24) are all
switch-case-prefixed templates (a block function declared directly in aswitchcase) — a separate, pre-existing root cause, left for follow-up.No regressions:
no-skipcases (where thevarshould be created) fail identically on baseline and fixed binaries (12 = 12, same files), confirming no over-suppression.cargo test -p perry-hirpasses.Strict bodies and ES modules are unaffected (the AnnexB var only exists in sloppy mode).
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes