Skip to content

fix(hir): #5703 — apply default-parameter prologue for class setters - #5744

Merged
proggeramlug merged 1 commit into
mainfrom
fix/setter-default-param-5703
Jun 28, 2026
Merged

fix(hir): #5703 — apply default-parameter prologue for class setters#5744
proggeramlug merged 1 commit into
mainfrom
fix/setter-default-param-5703

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes 1 of the 11 residual language/expressions/class failures tracked in #5703 — the static-setter case scope-static-setter-paramsbody-var-open.js ("value is not a function"). While triaging the other 10 I found the ticket's premise is stale/mis-scoped (details below), so this PR ships the one genuinely-tractable, in-theme fix and documents the rest precisely.

What this fixes

A class setter with a defaulted parameter (set a(_ = expr)) silently dropped its default. lower_setter_method_with_name built the Param with default: None and emitted no if (param === undefined) param = <default> prologue — only destructuring was handled. So C.a = undefined never ran the default expression.

Fix mirrors the constructor/method path:

  • lower the default via get_param_default in the param loop, before define_local, so the default's references resolve to the enclosing scope rather than the param or the body's hoisted vars — the separate parameter/body VariableEnvironment the spec mandates;
  • prepend build_default_param_stmts(&params) to the body.

The test's probeParams closure (created in the param default) then binds the outer x ("outside") while the body's var x = "inside" closure binds the body x — byte-for-byte node parity. Applies to instance and static setters; inert for setters with no declared default.

var C = class { static set a(_ = (probeParams = function(){ return x })) { var x = "inside"; probeBody = function(){ return x } } };
C.a = undefined;
// before: probeParams === undefined  →  TypeError: value is not a function
// after:  probeParams() === "outside",  probeBody() === "inside"   (== node)

Regression test: class_expr_static_setter_default_param_scope (all 6 cases in issue_5703_class_expr_static_dflt_params green).

Before / after (language/expressions/class)

The remaining 10 — NOT a class-expression regression

The other 10 listed cases (async-gen-method-static/yield-star-{sync,async}-{return,throw}, yield-star-next-non-object-ignores-then, and their elements/async-gen-private-method-static twins) are not about class-expression default params and not a #5662 regression. They are a general yield* return/throw/next delegation gap in async generators:

  • They fail identically for class statements (language/statements/class/async-gen-method-static), for plain top-level async function*, and for instance methods — the class form is irrelevant. (The statement copies are bucketed diff rather than runtime-fail only because an async test's failure still exits 0, so the parity gate on expressions/class is what surfaced them.)
  • Root cause: a yield*-delegating generator resumed via .return(v)/.throw(e) never forwards to the delegated iterator's return/throw. Perry does route .throw()→catch and .return()→finally into linearized states, but for async generators the abrupt-resume path (generator/lower/abrupt.rs::build_async_catch_route_body) hard-codes a {value: undefined, done: false} return and has no continuation loop (throw_continuation = None at generator/lower.rs, deliberately, "to stay byte-identical"). A desugar that drives the delegation through a try/catch makes the inner iterator's methods fire in the correct spec order, but the value yielded after the resume is dropped and the loop can't continue.

Closing those 10 requires enabling async-generator abrupt-resume continuation (real yielded-value packaging + continuation loop, like sync generators already have) — a substantial, higher-risk core change to the generator state machine, orthogonal to class-expression lowering. Recommend tracking it as its own issue rather than expanding this PR.

Refs #5703.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed setter parameters with default values so the default is applied when the setter is called with undefined.
    • Improved handling of setter parameters in class expressions to preserve the expected scope behavior.
  • Tests
    • Added a regression test covering a static class setter with a defaulted parameter to verify the correct runtime output and scope capture.

A setter with a defaulted parameter (`set a(_ = expr)`) dropped its
default: `lower_setter_method_with_name` built the `Param` with
`default: None` and emitted no `if (param === undefined) param = <default>`
prologue (only destructuring was handled). So invoking the setter with
`undefined` (`C.a = undefined`) never ran the default expression — test262
`language/expressions/class/scope-static-setter-paramsbody-var-open` left
`probeParams` undefined ("value is not a function").

Fix: mirror the constructor/method path — lower the default via
`get_param_default` in the param loop (BEFORE `define_local`, so the
default's references resolve to the enclosing scope rather than the
param or the body's hoisted `var`s — the separate parameter/body
VariableEnvironment the spec mandates) and prepend
`build_default_param_stmts` to the body. The default's `x` closure then
binds the OUTER `x` ("outside") while the body's `var x = "inside"` closure
binds the body `x`, matching node byte-for-byte. Applies to instance and
static setters alike; affects only setters that actually declare a
default (`build_default_param_stmts` is inert otherwise).

Regression test: class_expr_static_setter_default_param_scope.
@coderabbitai

coderabbitai Bot commented Jun 28, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 6109a78d-a155-4d38-85e9-847294ae2242

📥 Commits

Reviewing files that changed from the base of the PR and between 64aed03 and d83bd04.

📒 Files selected for processing (2)
  • crates/perry-hir/src/lower_decl/class_members.rs
  • crates/perry/tests/issue_5703_class_expr_static_dflt_params.rs

📝 Walkthrough

Walkthrough

lower_setter_method_with_name is updated to populate each parameter's default field and prepend if (param === undefined) param = <default> statements before destructuring in the setter body. A regression test for issue #5703 verifies a static class-expression setter with a defaulted parameter invoked as C.a = undefined.

Setter default parameter lowering

Layer / File(s) Summary
Default param storage and application
crates/perry-hir/src/lower_decl/class_members.rs, crates/perry/tests/issue_5703_class_expr_static_dflt_params.rs
lower_setter_method_with_name now sets Param.default from computed param_default and prepends default-application statements via build_default_param_stmts before destructuring. Regression test asserts correct scoping when C.a = undefined triggers the default.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 A setter called with undefined in tow,
Now checks for defaults before the body can go.
The param gets its value, the scope stays in line,
inside and outside both print just fine.
Hop hop, the bug is fixed — oh how divine!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise, specific, and accurately summarizes the main change: applying default-parameter prologue handling to class setters.
Description check ✅ Passed The description covers the summary, implementation details, related issue, and testing context, though several template sections are left unfilled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/setter-default-param-5703

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proggeramlug
proggeramlug merged commit 1d32628 into main Jun 28, 2026
15 checks passed
@proggeramlug
proggeramlug deleted the fix/setter-default-param-5703 branch June 28, 2026 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant