Skip to content

fix(codegen): apply ctor return-override on standalone-symbol new path (#5345)#5388

Closed
proggeramlug wants to merge 1 commit into
mainfrom
fix/derived-ctor-return-override-symbol-path
Closed

fix(codegen): apply ctor return-override on standalone-symbol new path (#5345)#5388
proggeramlug wants to merge 1 commit into
mainfrom
fix/derived-ctor-return-override-symbol-path

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes part of #5345 (test262 class tail). new Derived() for a class with its own constructor takes the shared <class>_constructor symbol-call path (the default since the [#bloat] change — force_ctor_call in lower_new). That path called the standalone constructor but discarded its return value and yielded the freshly-allocated this, so ECMAScript constructor return-override semantics ([[Construct]] step 13) were never applied:

  • a derived ctor returning a non-object primitive did not throw the required TypeErrorclass D extends B { constructor(){ super(); return 0 } }; new D() silently returned the instance instead of throwing; and
  • a ctor returning an object did not override this.

The standalone symbol already returns the body's explicit return <value> (or NaN-boxed undefined on fall-through), so the fix applies js_ctor_return_override(this, ret, is_derived) at the construction site — exactly as the inline new path already does — and returns its result. is_derived mirrors the inline path's heritage check (extends / extends_name / native_extends / extends_expr).

Because the override runs at the construction site (outside any try in the body), a derived ctor's try { return 0; } catch {} still throws an uncatchable TypeError, per spec.

Verification

test262 language/statements/class/subclass (scripts/test262_subset.py, node v26):

pass runtime-fail parity
before 34 75 31.2%
after 44 65 40.4%

+10 fixed, 0 regressions. The fixed cases are the full derived-class-return-override-with-* cluster (boolean / null / number / string / symbol / object), the -catch / -catch-super / -catch-super-arrow uncatchable-TypeError variants, and class-definition-null-proto-contains-return-override.

language/statements/class/definition is byte-identical before/after (no regressions). Direct spot-checks confirm: derived return 0 → throws TypeError; derived return {obj} → overrides; derived return;this; base return 5 → primitive ignored (this).

Note: per repo convention for this work, no version bump / CHANGELOG entry — left for the maintainer to fold in at merge.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Fixed constructor return value handling to properly apply ECMAScript semantics.
    • Constructors now correctly propagate return values instead of being discarded.
    • Improved support for derived class constructors by properly detecting inheritance and applying override semantics.

#5345)

`new Derived()` for a class with its own constructor takes the shared
`<class>_constructor` symbol-call path (the default since the [#bloat]
change; `force_ctor_call`). That path called the standalone constructor
but discarded its return value and yielded the freshly-allocated `this`,
so ECMAScript constructor return-override semantics were never applied:

  - a derived ctor returning a non-object primitive did NOT throw the
    required TypeError (`new Derived(){ super(); return 0 }` silently
    returned the instance), and
  - a ctor returning an object did NOT override `this`.

The standalone symbol already returns the body's explicit `return <value>`
(or NaN-boxed `undefined` on fall-through), so the fix is to apply
`js_ctor_return_override(this, ret, is_derived)` at the construction site
— exactly as the inline `new` path already does — and return its result.
`is_derived` mirrors the inline path's heritage check.

test262 language/statements/class/subclass: +10 cases (parity 31.2% →
40.4%), 0 regressions — the full `derived-class-return-override-with-*`
cluster (boolean/null/number/string/symbol/object), the `-catch`/
`-catch-super`/`-catch-super-arrow` uncatchable-TypeError variants, and
`class-definition-null-proto-contains-return-override`. The
language/statements/class/definition dir is unchanged (no regressions).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 18, 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: 386318fe-f7e6-4b9a-9bea-e060e309d9a9

📥 Commits

Reviewing files that changed from the base of the PR and between 9d76a29 and 371d571.

📒 Files selected for processing (1)
  • crates/perry-codegen/src/lower_call/new.rs

📝 Walkthrough

Walkthrough

call_local_constructor_symbol in lower_call/new.rs is changed to return a String. It now captures the standalone <Class>_constructor return value, computes is_derived from class heritage fields, and calls js_ctor_return_override. The lower_new call site returns this result instead of always returning obj_box.

Changes

Constructor Return-Override Propagation

Layer / File(s) Summary
call_local_constructor_symbol signature and fallback
crates/perry-codegen/src/lower_call/new.rs
Function return type becomes String; the no-standalone-constructor branch now returns obj_box.to_string() instead of returning nothing.
Derived-class detection and js_ctor_return_override call
crates/perry-codegen/src/lower_call/new.rs
Captures the standalone constructor return value, computes is_derived from all heritage fields (extends, extends_name, native_extends, extends_expr), and applies js_ctor_return_override(obj_box, ret_val, is_derived).
lower_new call-site result propagation
crates/perry-codegen/src/lower_call/new.rs
The lower_new branch now returns Ok(result) from call_local_constructor_symbol instead of the unconditional Ok(obj_box).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • PerryTS/perry#5304: Also modifies lower_call/new.rs to route new constructor lowering through the shared standalone <Class>_constructor via call_local_constructor_symbol, directly preceding the result-propagation work in this PR.
  • PerryTS/perry#5362: Updates the standalone <class>_constructor symbol route in codegen/method.rs to enforce derived-constructor TDZ throw semantics, targeting the same standalone derived-constructor lowering path.

Poem

🐇 Hop, hop! The constructor's gift was once tossed away,
Now ret_val is caught and the override holds sway.
is_derived is checked with a wiggle of ears,
js_ctor_return_override resolves spec fears.
No more obj_box alone from lower_new
The right result hops through, fresh and true! 🥕

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically summarizes the main change: applying constructor return-override semantics on the standalone-symbol new path.
Description check ✅ Passed The pull request description is comprehensive and complete. It includes a clear summary of the fix, detailed explanation of the bug and solution, and comprehensive test results demonstrating correctness.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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/derived-ctor-return-override-symbol-path
⚔️ Resolve merge conflicts
  • Resolve merge conflict in branch fix/derived-ctor-return-override-symbol-path

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

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Closing as a duplicate. While I was working on this, #5386 (commit 9d76a29) landed on main with an essentially identical fix — call_local_constructor_symbol now returns the standalone ctor's result and lower_new feeds it through js_ctor_return_override at the construction site, with the same is_derived heritage check. We converged on the same solution independently, so this PR adds nothing on top of main. The derived-constructor return-override cluster in #5345 is resolved by #5386.

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