fix(error): forward ES2022 cause through super(message, options) (#5127)#5158
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughFixes issue ChangesES2022
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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: 2
🧹 Nitpick comments (1)
crates/perry/tests/issue_5127_error_cause_super.rs (1)
60-76: ⚡ Quick winAdd a multi-hop subclass case to guard the still-separate super() code path
Please extend this regression with
Leaf extends Mid extends Errorandsuper(msg, opts)assertion. That path is lowered differently and should be covered explicitly to prevent silent regressions.🤖 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/tests/issue_5127_error_cause_super.rs` around lines 60 - 76, The current test in issue_5127_error_cause_super.rs only covers single-level Error subclasses (AppError extends Error), but there is a separate code path for multi-level inheritance chains that needs explicit test coverage. Add a test case with a two-level inheritance hierarchy (Leaf extends Mid extends Error) where the Leaf constructor invokes super(msg, opts) to verify that the cause option is properly forwarded through multiple levels of inheritance, ensuring the regression guard covers this distinct lowering path.
🤖 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-codegen/src/expr/this_super_call.rs`:
- Around line 603-613: The cause forwarding fix for the Error-like branch needs
to be mirrored in the fallback Error-chain branch to handle multi-level
inheritance chains. Locate the fallback Error-chain branch handling code and
apply the same cause forwarding logic: add a conditional check for args[1]
existence and call js_error_apply_cause_to_object with the instance handle and
the options value, using the same approach as in the direct Error-like branch
shown in the diff (checking opts_val from lowered_args.get(1), creating a block,
and calling js_error_apply_cause_to_object with the appropriate parameters).
In `@crates/perry-runtime/src/error.rs`:
- Around line 517-529: The function js_error_apply_cause_to_object is a new FFI
entrypoint that is not protected from dead-code elimination during optimized
builds. Add the symbol name js_error_apply_cause_to_object to the existing
#[used] keepalive anchor attribute in this file (typically found as a string
concatenation or list near the top or bottom of the file) that is used to
preserve FFI symbols during LTO optimization. This ensures the symbol remains
available when the code is compiled with optimizations.
---
Nitpick comments:
In `@crates/perry/tests/issue_5127_error_cause_super.rs`:
- Around line 60-76: The current test in issue_5127_error_cause_super.rs only
covers single-level Error subclasses (AppError extends Error), but there is a
separate code path for multi-level inheritance chains that needs explicit test
coverage. Add a test case with a two-level inheritance hierarchy (Leaf extends
Mid extends Error) where the Leaf constructor invokes super(msg, opts) to verify
that the cause option is properly forwarded through multiple levels of
inheritance, ensuring the regression guard covers this distinct lowering path.
🪄 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: c2a7692d-f884-4058-9e34-feb06301c1f2
📒 Files selected for processing (4)
crates/perry-codegen/src/expr/this_super_call.rscrates/perry-codegen/src/runtime_decls/objects.rscrates/perry-runtime/src/error.rscrates/perry/tests/issue_5127_error_cause_super.rs
Comment on lines
+603
to
+613
| // #5127: `super(message, options)` must forward the | ||
| // ES2022 `cause` option. The instance is a generic | ||
| // object, so install a non-enumerable `cause` | ||
| // property from args[1] when present. | ||
| if let Some(opts_val) = lowered_args.get(1) { | ||
| let blk = ctx.block(); | ||
| blk.call_void( | ||
| "js_error_apply_cause_to_object", | ||
| &[(I64, &this_handle), (DOUBLE, opts_val)], | ||
| ); | ||
| } |
There was a problem hiding this comment.
Cause forwarding is only fixed for direct extends Error, not multi-hop chains
This new call covers the direct Error-like branch, but the fallback Error-chain branch (around Line 726-Line 804) still omits args[1] forwarding. class Mid extends Error {}; class Leaf extends Mid { constructor(m,o){ super(m,o) } } will still lose cause.
Suggested fix (mirror the same forwarding in the fallback Error-chain branch)
@@
blk.call_void(
"js_object_set_field_by_name",
&[
(I64, &this_handle),
(I64, &name_key_raw),
(DOUBLE, &name_val_box),
],
);
+ if let Some(opts_val) = lowered_args.get(1) {
+ let blk = ctx.block();
+ blk.call_void(
+ "js_error_apply_cause_to_object",
+ &[(I64, &this_handle), (DOUBLE, opts_val)],
+ );
+ }
}🤖 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-codegen/src/expr/this_super_call.rs` around lines 603 - 613, The
cause forwarding fix for the Error-like branch needs to be mirrored in the
fallback Error-chain branch to handle multi-level inheritance chains. Locate the
fallback Error-chain branch handling code and apply the same cause forwarding
logic: add a conditional check for args[1] existence and call
js_error_apply_cause_to_object with the instance handle and the options value,
using the same approach as in the direct Error-like branch shown in the diff
(checking opts_val from lowered_args.get(1), creating a block, and calling
js_error_apply_cause_to_object with the appropriate parameters).
Comment on lines
+517
to
+529
| #[no_mangle] | ||
| pub extern "C" fn js_error_apply_cause_to_object(obj: *mut crate::object::ObjectHeader, options: f64) { | ||
| let opts = crate::value::JSValue::from_bits(options.to_bits()); | ||
| if !opts.is_pointer() { | ||
| return; | ||
| } | ||
| let key = js_string_from_bytes(b"cause".as_ptr(), 5); | ||
| let key_f64 = crate::value::js_nanbox_string(key as i64); | ||
| let cause = crate::value::js_dyn_index_get(options, key_f64); | ||
| if cause.to_bits() != TAG_UNDEFINED_BITS { | ||
| crate::object::js_object_set_field_by_name_nonenum(obj, key as *const StringHeader, cause); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing keepalive anchor for new codegen-only FFI symbol
js_error_apply_cause_to_object is a generated-code entrypoint, but it is not added to the #[used] keepalive anchors used in this file for auto-optimize/LTO dead-strip protection. This can make the symbol disappear in optimized links.
Suggested fix
@@
#[used]
static KEEP_ERROR_IS_ERROR: extern "C" fn(f64) -> f64 = js_error_is_error;
+#[used]
+static KEEP_ERROR_APPLY_CAUSE_TO_OBJECT: extern "C" fn(
+ *mut crate::object::ObjectHeader,
+ f64,
+) = js_error_apply_cause_to_object;🤖 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-runtime/src/error.rs` around lines 517 - 529, The function
js_error_apply_cause_to_object is a new FFI entrypoint that is not protected
from dead-code elimination during optimized builds. Add the symbol name
js_error_apply_cause_to_object to the existing #[used] keepalive anchor
attribute in this file (typically found as a string concatenation or list near
the top or bottom of the file) that is used to preserve FFI symbols during LTO
optimization. This ensures the symbol remains available when the code is
compiled with optimizations.
added 2 commits
June 15, 2026 07:37
…rror subclasses (#5127) An Error subclass that forwarded options via super(message, options) lost the cause — this.cause was undefined afterward, even though plain new Error(msg, { cause }) worked. Root cause: the Error-like super(...) codegen arm only assigned this.message = args[0] and this.name = <parent>; it ignored args[1] (the options object), so the cause never reached the (generic-object) subclass instance. Fix: add js_error_apply_cause_to_object(this, options) — mirrors the existing apply_cause_from_options but installs a non-enumerable own cause property on the instance object (matching Node's InstallErrorCause). The super-call arm now calls it whenever a second argument is forwarded.
proggeramlug
force-pushed
the
fix/5127-error-cause-super
branch
from
a081213 to
44c905e
Compare
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.
Fixes #5127.
Problem
An
Errorsubclass that forwards options viasuper(message, options)lost the ES2022cause:A plain
new Error(msg, { cause })(no subclass) already worked, so the option was dropped specifically when passed up throughsuper(...).Root cause
The Error-like
super(...)codegen arm (this_super_call.rs) only assignedthis.message = args[0]andthis.name = <parent>. It ignoredargs[1](the options object), socausenever reached the subclass instance (which is a generic heap object, not anErrorHeader).Fix
js_error_apply_cause_to_object(this, options)(error.rs) — mirrors the existingapply_cause_from_optionsbut installs a non-enumerable owncauseproperty on the instance object, matching Node'sInstallErrorCause. No-op for non-object options.super(...)arm now calls it whenever a second argument is forwarded.Verification
Output matches Node exactly, including
causebeing a non-enumerable own property, plain-Errorcause still working, and no-options ⇒ no cause:New regression test:
crates/perry/tests/issue_5127_error_cause_super.rs(green).No changelog/version bump per maintainer's release-at-merge workflow.
Summary by CodeRabbit
New Features
{ cause }fromsuper(message, options)into the created error instance, supporting standard error-chaining patterns.Bug Fixes
causeis applied correctly when usingsuper(message, options)(including the expected non-enumerable behavior on instances).Tests
super(message, options)propagation for customErrorsubclasses, plus coverage for plainErrorand missing options.