fix(hir): parenthesized / cast assignment target must honour const immutability (#6300)#6308
Conversation
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughChangesConst assignment lowering
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 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 `@test-files/test_gap_6300_const_cast_assign.ts`:
- Around line 48-52: The `satisfies` assignment cases in the test still produce
expected TypeScript errors. Add `@ts-expect-error` immediately before both
`satisfies` assignment targets, including the `(c4 satisfies number) = 9` case
and the second `satisfies` site, matching the suppression used for the `!` case.
🪄 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: c79188e3-0c30-49d9-8e11-da9d96ef8dd6
📒 Files selected for processing (4)
crates/perry-hir/src/lower/expr_assign.rscrates/perry-hir/src/lower/expr_misc.rscrates/perry-hir/src/lower/lower_expr/assignment.rstest-files/test_gap_6300_const_cast_assign.ts
| const c4 = 4; | ||
| expectThrow("satisfies", () => { | ||
| (c4 satisfies number) = 9; | ||
| }); | ||
| console.log("c4 =", c4); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if tsc flags `satisfies` in assignment target position.
# Create a minimal test file
cat > /tmp/test_satisfies_assign.ts << 'EOF'
const c4 = 4;
(c4 satisfies number) = 9;
let d6 = 1;
(d6 satisfies number) = 6;
EOF
# Run tsc in check-only mode (if available in the project)
npx tsc --noEmit --strict /tmp/test_satisfies_assign.ts 2>&1 || echo "TSC_NOT_AVAILABLE"Repository: PerryTS/perry
Length of output: 416
Add @ts-expect-error on the satisfies assignment targets
(c4 satisfies number) = 9 still errors as an invalid assignment target/const assignment under tsc, so the same suppression used for the ! case is needed here too (and at the second satisfies site).
🤖 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_6300_const_cast_assign.ts` around lines 48 - 52, The
`satisfies` assignment cases in the test still produce expected TypeScript
errors. Add `@ts-expect-error` immediately before both `satisfies` assignment
targets, including the `(c4 satisfies number) = 9` case and the second
`satisfies` site, matching the suppression used for the `!` case.
…mutability (#6300) `const c = 1; (c as any) = 9;` silently mutated the binding and printed 9; node throws `TypeError: Assignment to constant variable.`. Every wrapper spelling of the same target was affected — `(c) = 9`, `(c as any) = 9`, `(c satisfies number) = 9`, `(c!) = 9`, `<any>c = 9`, and the compound and logical forms (`(c as any) += 1`, `(c as any) ||= 9`). Root cause: two divergent identifier-store arms. `lower_assignment_target` (expr_assign.rs) handled a bare `Ident` target with the full binding resolution — active `with` env, const-immutability throw, class-inner-name throw, class/function binding, strict/sloppy global — while its `Paren`/`TsAs`/`TsNonNull`/`TsTypeAssertion`/`TsSatisfies` arms delegated to `lower_expr_assignment` (lower_expr/assignment.rs), whose own `Ident` arm was a stripped-down copy that went straight to `Expr::LocalSet` with no immutability check. Any wrapper on the LHS therefore routed around the const check. Extract the resolution into one shared `lower_ident_assignment` and call it from both, so the check can't be bypassed. `x++` had the same hole, from the other direction: `lower_update` emitted `Expr::Update` for any local without consulting `is_local_immutable`, so even the bare `const c = 1; c++` mutated silently — and `(c as any)++` didn't compile at all ("Update expression only supports identifiers and member expressions", the unwrap loop only knew `Paren`/`TsNonNull`). Teach the loop the three TS cast nodes and throw on an immutable target. The throw is sequenced after the operand's `js_to_numeric`, not before it: per ES 13.4.2.1 `ToNumeric(GetValue(lhs))` runs ahead of `PutValue`, and the coercion is observable — `const s = Symbol(); s++` reports "Cannot convert a Symbol value to a number" while `const b = 1n; b++` passes ToNumeric and does get the const TypeError. Both now match V8. Reachability of the shared helper is confined to the wrapper arms (`lower_expr_assignment` has no other callers), and every `mark_local_immutable` site is a genuine `const` binding, so the bare-ident and `let` paths are untouched. test-files/test_const_cast_assign_6300.ts covers all wrapper spellings, compound/update/logical forms, RHS-side-effect ordering, the `||=` short-circuit that must NOT throw, for-of const bindings, and the legal writes that must keep working (`let` through a cast, `(obj as any).x = 1`, mutating a const array). Byte-for-byte identical to `node --experimental-strip-types`.
proggeramlug
force-pushed
the
fix/6300-const-cast-assign
branch
from
fc6cfb9 to
6117cfb
Compare
1 participant
Fixes #6300.
The bug
Every wrapper spelling of the same target was affected, not just
as:(c) = 9,(c as any) = 9,(c satisfies number) = 9,(c!) = 9,<any>c = 9,plus the compound and logical forms (
(c as any) += 1,(c as any) ||= 9).Root cause
Two divergent identifier-store arms.
lower_assignment_target(expr_assign.rs) handled a bareIdenttarget with thefull binding resolution — active
withenv, const-immutability throw,class-inner-name throw, class/function binding, strict/sloppy global. But its
Paren/TsAs/TsNonNull/TsTypeAssertion/TsSatisfiesarms delegate tolower_expr_assignment(lower_expr/assignment.rs), and that function'sIdentarm was a stripped-down copy that went straight to
Expr::LocalSetwith noimmutability check. So any wrapper on the LHS routed around the const check.
The fix extracts the resolution into one shared
lower_ident_assignmentand calls itfrom both arms, so the check can't be bypassed.
lower_expr_assignmenthas no callersother than those wrapper arms, so the bare-ident path is byte-identical to before.
x++had the same hole, from the other directionlower_updateemittedExpr::Updatefor any local without consultingis_local_immutable, so even the bareconst c = 1; c++mutated silently — and(c as any)++didn't compile at all (Update expression only supports identifiers and member expressions; the unwrap loop only knewParen/TsNonNull). This teaches theloop the three TS cast nodes and throws on an immutable target.
The throw is sequenced after the operand's
js_to_numeric, not before it. Per ES13.4.2.1,
ToNumeric(GetValue(lhs))runs ahead ofPutValue, and that coercion isobservable:
const s = Symbol(); s++TypeError: Cannot convert a Symbol value to a numberconst b = 1n; b++TypeError: Assignment to constant variable.A BigInt passes
ToNumericcleanly and does reach the constTypeError; a Symbolthrows in the coercion first. Both now match V8.
Before / after vs
node --experimental-strip-typesmain(c) = 9c→ 9(c as any) = 9c→ 9(c satisfies number) = 9c→ 9(c!) = 9c→ 9(c as any) += 1c→ 2(c as any)++c++(bare)c→ 2(c as any) ||= 9,ctruthycunchangedcunchangedlet d = 1; (d as any) = 9d→ 9d→ 9d→ 9(obj as any).x = 1obj.x→ 1obj.x→ 1obj.x→ 1Blast radius
lower_expr_assignmenthas no other callers — so bare-identifier assignment lowering is unchanged.
mark_local_immutablecall site (7 of them) is a genuineconstbinding(
kind == Const/!mutable), so the newlower_updatethrow cannot fire on alet/var/ param.letthrough acast,
(obj as any).x = 1/.x++/["x"] += 10, mutating aconstarray.Test
test-files/test_gap_6300_const_cast_assign.ts— all wrapper spellings,compound / update / logical forms, RHS-side-effect ordering (
rhsRunsis incrementedbefore the
TypeError), the||=short-circuit that must not throw, BigInt++,for (const x of …)bindings, function-local consts, and the legal writes above.Byte-for-byte identical to
node --experimental-strip-types.Regression checks
cargo-testis green in CI. Locally under--profile perry-dev,cargo test -p perry-hiris byte-for-byte unchanged frommain: same 223 + 2 + 18 passing, and thesame single
logical_property_assignment_short_circuits_the_store_4586failure, whichreproduces by building main's exact source in the same tree (it asserts on the
member-target
??=shape, untouched here).A dual sweep of the full 286-test
test_gap_*corpus — same source compiled and runwith a
main-baseline binary and with this branch, diffing output — reports 2 changed,both noise:
test_gap_console_methods:console.timejitter (timer1: 0.014ms→0.013ms).test_gap_module_const_local_shadow: prints a different garbage float from anuninitialized read. Already in
test-parity/known_failures.jsonunder parity diff v0.5.1205 → main 2026-07-03: net +32/−10; 10 newly-failing tests need triage (incl. issue-945 guard confirmation) #5917, and thevalue is nondeterministic run-to-run on the
mainbinary as well(
1.678e-311/2.538e-311/3.021e-311across three runs).No real behavioural change outside the new fixture.
Note:
lintandconformance-smokeare already red on cleanmain(addr-classratchet on
map.rs; #6297 fixes it).