fix(hir): class own-name binding is const inside the class body - #6021
Merged
Conversation
Per the class definition semantics, the binding for a class's own name is
an *immutable* (const) binding in the class scope — the environment record
created for the class body. Assigning to that name from anywhere inside the
class body (constructor, method, accessor, or an expression in the heritage
clause) must throw a TypeError. The outer declaration binding introduced by
a `class C {}` statement is a separate, mutable binding, so reassigning `C`
from *outside* the body stays legal.
Perry previously dropped such an assignment silently (the `<Name> = X`
branch in the ident-assignment lowering evaluated the RHS for side effects
and returned, treating the class binding as a no-op target), so none of the
`assert.throws(TypeError, ...)` cases fired.
This tracks the source-level inner binding name on the lowering context
(`current_class_inner_name`, saved/restored around each class body, and fed
for class *expressions* via `pending_class_inner_name` since those receive a
synthetic dedup registration key rather than the user-visible name). When an
identifier assignment target matches that inner name — and no local/param
shadows it — the lowering now evaluates the RHS and then throws via the
existing `js_throw_type_error_const_assignment` runtime helper.
Fixes the test262 language/{statements,expressions}/class name-binding
const cases and the inner-binding-immutability assertions in the
scope-name-lex heritage/no-heritage tests, with no change to outer-binding
reassignment.
📝 WalkthroughWalkthroughThis PR adds two new fields to ChangesConst assignment guard for class inner binding names
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant Source as Class source (decl/expr)
participant Lowerer as Lowering entry points
participant Ctx as LoweringContext
participant AssignLowering as lower_assignment_target
Source->>Lowerer: class declaration/expression with ident
Lowerer->>Ctx: set pending_class_inner_name (if applicable)
Lowerer->>Ctx: snapshot current_class_inner_name
Lowerer->>Ctx: set current_class_inner_name from pending/decl ident
Note over Lowerer: lower class body statements
AssignLowering->>Ctx: read current_class_inner_name
alt assignment target matches inner name
AssignLowering->>AssignLowering: lower RHS for side effects
AssignLowering->>AssignLowering: throw TypeError (const assignment)
else no match
AssignLowering->>AssignLowering: lower normal assignment
end
Lowerer->>Ctx: restore previous current_class_inner_name
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.github.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/class_decl.rs`:
- Around line 1431-1437: The class lowering in `class_decl.rs` is incorrectly
falling back to `name` when `pending_class_inner_name` is absent, which makes
anonymous `class {}` expressions inherit an inner binding they should not have.
Update the `current_class_inner_name` setup in the class declaration lowering
path so it is only set from `pending_class_inner_name` and remains `None`
otherwise, preserving anonymous class behavior in
`lower_decl`/`current_class_inner_name` handling.
🪄 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: 0860c23a-b012-4101-bcba-c79c97d12a87
📒 Files selected for processing (7)
crates/perry-hir/src/lower/context.rscrates/perry-hir/src/lower/expr_assign.rscrates/perry-hir/src/lower/expr_new/non_ident.rscrates/perry-hir/src/lower/lower_expr/arm_class.rscrates/perry-hir/src/lower/lowering_context.rscrates/perry-hir/src/lower/stmt.rscrates/perry-hir/src/lower_decl/class_decl.rs
Comment on lines
+1431
to
+1437
| let old_inner_name = ctx.current_class_inner_name.take(); | ||
| // A class-expression caller stashes the source ident here; fall back | ||
| // to the (possibly synthetic) registration name when absent. | ||
| ctx.current_class_inner_name = ctx | ||
| .pending_class_inner_name | ||
| .take() | ||
| .or_else(|| Some(name.to_string())); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file list ==\n'
git ls-files | rg 'crates/perry-hir/src/lower/(decl/class_decl\.rs|stmt\.rs|arm_class\.rs|non_ident\.rs)$'
printf '\n== ast outlines ==\n'
ast-grep outline crates/perry-hir/src/lower/decl/class_decl.rs --view expanded | sed -n '1,220p'
printf '\n--- stmt.rs ---\n'
ast-grep outline crates/perry-hir/src/lower/stmt.rs --view expanded | sed -n '1,260p'
printf '\n--- arm_class.rs ---\n'
ast-grep outline crates/perry-hir/src/lower/arm_class.rs --view expanded | sed -n '1,220p'
printf '\n--- non_ident.rs ---\n'
ast-grep outline crates/perry-hir/src/lower/non_ident.rs --view expanded | sed -n '1,220p'
printf '\n== relevant text search ==\n'
rg -n "current_class_inner_name|pending_class_inner_name|class \\{|class-expression|bind_name|anon_class|__anon_class" crates/perry-hir/src/lower -SRepository: PerryTS/perry
Length of output: 10378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== lowering_context.rs relevant range ==\n'
sed -n '220,245p;570,690p' crates/perry-hir/src/lower/lowering_context.rs
printf '\n== lower_expr/arm_class.rs relevant range ==\n'
sed -n '1,130p;240,285p' crates/perry-hir/src/lower/lower_expr/arm_class.rs
printf '\n== expr_new/non_ident.rs relevant range ==\n'
sed -n '110,175p' crates/perry-hir/src/lower/expr_new/non_ident.rs
printf '\n== stmt.rs relevant range ==\n'
sed -n '630,805p' crates/perry-hir/src/lower/stmt.rs
printf '\n== expr_assign.rs relevant range ==\n'
sed -n '420,470p' crates/perry-hir/src/lower/expr_assign.rsRepository: PerryTS/perry
Length of output: 37316
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
files = [
"crates/perry-hir/src/lower/lowering_context.rs",
"crates/perry-hir/src/lower/lower_expr/arm_class.rs",
"crates/perry-hir/src/lower/expr_new/non_ident.rs",
"crates/perry-hir/src/lower/stmt.rs",
"crates/perry-hir/src/lower/expr_assign.rs",
]
for f in files:
p = Path(f)
print(f"\n## {f} lines={sum(1 for _ in p.open())}")
PYRepository: PerryTS/perry
Length of output: 447
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "current_class_inner_name\s*=|pending_class_inner_name\s*=|current_class_inner_name\.take\(|pending_class_inner_name\.take\(" crates/perry-hir/src/lower -S
printf '\n== expr_assign inner-name branch ==\n'
sed -n '430,455p' crates/perry-hir/src/lower/expr_assign.rs
printf '\n== lower_expr/arm_class.rs start ==\n'
sed -n '1,110p' crates/perry-hir/src/lower/lower_expr/arm_class.rs
printf '\n== expr_new/non_ident.rs start ==\n'
sed -n '120,155p' crates/perry-hir/src/lower/expr_new/non_ident.rsRepository: PerryTS/perry
Length of output: 9461
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "or_else\(\|\| Some\(name\.to_string\(\)\)\)|pending_class_inner_name.*name\.to_string\(\)|current_class_inner_name = ctx" crates/perry-hir/src/lower -SRepository: PerryTS/perry
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "pending_class_inner_name\.take\(\)\.or_else|current_class_inner_name = .*pending_class_inner_name|old_inner_name = ctx\.current_class_inner_name\.take\(\)" crates/perry-hir/src -SRepository: PerryTS/perry
Length of output: 376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== all current_class_inner_name references ==\n'
rg -n "current_class_inner_name" .
printf '\n== all pending_class_inner_name references ==\n'
rg -n "pending_class_inner_name" .
printf '\n== all or_else fallback references ==\n'
rg -n "pending_class_inner_name\.take\(\)\.or_else|or_else\(\|\| Some\(name\.to_string\(\)\)\)" .Repository: PerryTS/perry
Length of output: 2066
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== class_decl.rs around 1418-1450 ==\n'
sed -n '1418,1450p' crates/perry-hir/src/lower_decl/class_decl.rs
printf '\n== class_decl.rs function signature / entry context ==\n'
rg -n "fn lower_class_from_ast|current_class_inner_name = ctx|pending_class_inner_name" crates/perry-hir/src/lower_decl/class_decl.rs -n -S
sed -n '1,120p' crates/perry-hir/src/lower_decl/class_decl.rsRepository: PerryTS/perry
Length of output: 7037
Anonymous class expressions should not fall back to name Leave current_class_inner_name as None unless pending_class_inner_name is set; otherwise an anonymous class {} inherits the outer registration name as an inner binding and assignments in the body hit the const-binding TypeError path incorrectly.
🤖 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/class_decl.rs` around lines 1431 - 1437, The
class lowering in `class_decl.rs` is incorrectly falling back to `name` when
`pending_class_inner_name` is absent, which makes anonymous `class {}`
expressions inherit an inner binding they should not have. Update the
`current_class_inner_name` setup in the class declaration lowering path so it is
only set from `pending_class_inner_name` and remains `None` otherwise,
preserving anonymous class behavior in `lower_decl`/`current_class_inner_name`
handling.
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.
Summary
The binding for a class's own name is an immutable (const) binding in the class scope — the declarative environment record created for the class body. Assigning to that name from anywhere inside the class body (constructor, method, accessor, or an expression in the heritage clause) must throw a
TypeError. The outer declaration binding introduced by aclass C {}statement is a separate, mutable binding, so reassigningCfrom outside the body stays legal.Perry previously dropped such an assignment silently: the
<Name> = Xbranch in the identifier-assignment lowering evaluated the RHS for side effects and returned, treating the class binding as a no-op target. So none of theassert.throws(TypeError, ...)cases fired.Fix
Track the source-level inner binding name on the lowering context:
current_class_inner_name— saved/restored around each class body (bothlower_class_declandlower_class_from_ast).pending_class_inner_name— fed by the class-expression call sites (arm_class.rs,expr_new/non_ident.rs, and thevar C = class Named {...}fast path instmt.rs), because those receive a synthetic dedup registration key rather than the user-visible name.When an identifier assignment target matches that inner name — and no local/param shadows it (the
lookup_localarm runs first, so a shadowing binding wins) — the lowering now evaluates the RHS and then throws via the existingjs_throw_type_error_const_assignmentruntime helper.Handles constructor, method, accessor (get/set), and heritage-expression forms, for both class declarations and named class expressions. Outer-binding reassignment is unchanged.
Tests (internal Linux sweep host, test262 class cluster)
language/statements/class/name-binding/const.js.language/statements/class+language/expressions/classsweep: +1 pass, zero regressions (diffed the failure sidecar before/after).TypeError; shadowing param/local, nested-class own-name, and outer-scope reassignment do not false-throw.Part of the test262 class-cluster parity work (tracker #793).
Summary by CodeRabbit
TypeError.