-
-
Notifications
You must be signed in to change notification settings - Fork 132
fix(error): forward ES2022 cause through super(message, options) (#5127)
#5158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -506,6 +506,31 @@ unsafe fn apply_cause_from_options(error: *mut ErrorHeader, options: f64) { | |
| } | ||
| } | ||
|
|
||
| /// #5127: apply the ES2022 `cause` option to a user `Error` *subclass* | ||
| /// instance when its constructor forwards `super(message, options)`. Such an | ||
| /// instance is a generic heap object (not an `ErrorHeader`) — the super-call | ||
| /// codegen sets `message`/`name` as object properties — so the cause must be | ||
| /// installed as a (non-enumerable) own `cause` property on the object too, | ||
| /// matching Node's `InstallErrorCause`. Mirrors `apply_cause_from_options`: | ||
| /// reads `options.cause` via the generic getter (works for object literals | ||
| /// and runtime-held options alike) and is a no-op for non-object options. | ||
| #[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); | ||
| } | ||
| } | ||
|
Comment on lines
+517
to
+532
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing keepalive anchor for new codegen-only FFI symbol
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 |
||
|
|
||
| /// #2836: allocate an Error (or native subclass) carrying a `{ cause }` | ||
| /// option read from an arbitrary runtime options value. `kind` selects the | ||
| /// ERROR_KIND_* discriminant so `instanceof TypeError`/etc. keep working. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| //! Regression test for #5127: an `Error` subclass that forwards options via | ||
| //! `super(message, options)` dropped the ES2022 `cause` — `this.cause` was | ||
| //! `undefined` afterward, even though a plain `new Error(msg, { cause })` | ||
| //! (no subclass) 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). Fix: when a second arg is present, call | ||
| //! `js_error_apply_cause_to_object`, which installs a non-enumerable own | ||
| //! `cause` property on the subclass instance from `options.cause` (matching | ||
| //! Node's `InstallErrorCause`). | ||
|
|
||
| use std::path::PathBuf; | ||
| use std::process::Command; | ||
|
|
||
| fn perry_bin() -> PathBuf { | ||
| PathBuf::from(env!("CARGO_BIN_EXE_perry")) | ||
| } | ||
|
|
||
| fn compile_and_run(dir: &std::path::Path, source: &str) -> String { | ||
| let entry = dir.join("main.ts"); | ||
| let output = dir.join("main_bin"); | ||
| std::fs::write(&entry, source).expect("write entry"); | ||
|
|
||
| let compile = Command::new(perry_bin()) | ||
| .current_dir(dir) | ||
| .arg("compile") | ||
| .arg(&entry) | ||
| .arg("-o") | ||
| .arg(&output) | ||
| .output() | ||
| .expect("run perry compile"); | ||
| assert!( | ||
| compile.status.success(), | ||
| "perry compile failed\nstdout:\n{}\nstderr:\n{}", | ||
| String::from_utf8_lossy(&compile.stdout), | ||
| String::from_utf8_lossy(&compile.stderr) | ||
| ); | ||
|
|
||
| let run = Command::new(&output) | ||
| .current_dir(dir) | ||
| .output() | ||
| .expect("run compiled binary"); | ||
| assert!( | ||
| run.status.success(), | ||
| "compiled binary failed\nstatus: {:?}\nstdout:\n{}\nstderr:\n{}", | ||
| run.status, | ||
| String::from_utf8_lossy(&run.stdout), | ||
| String::from_utf8_lossy(&run.stderr) | ||
| ); | ||
| String::from_utf8_lossy(&run.stdout).into_owned() | ||
| } | ||
|
|
||
| #[test] | ||
| fn error_subclass_forwards_cause_through_super() { | ||
| let dir = tempfile::tempdir().expect("tempdir"); | ||
| let stdout = compile_and_run( | ||
| dir.path(), | ||
| r#" | ||
| class AppError extends Error { | ||
| constructor(msg: string, opts?: ErrorOptions) { super(msg, opts); this.name = "AppError"; } | ||
| } | ||
| const e = new AppError("high-level", { cause: new TypeError("low-level") }); | ||
| console.log(e.message, (e.cause as Error)?.message); | ||
| console.log(e.name, e instanceof Error); | ||
| // `cause` is a non-enumerable own property (Node semantics). | ||
| console.log(Object.keys(e).includes("cause"), Object.prototype.hasOwnProperty.call(e, "cause")); | ||
|
|
||
| // Plain (non-subclass) Error with cause still works. | ||
| const p = new Error("m", { cause: 42 }); | ||
| console.log(p.cause); | ||
|
|
||
| // No options forwarded => no cause. | ||
| class B extends Error { constructor(m: string){ super(m); } } | ||
| console.log(new B("x").cause); | ||
| "#, | ||
| ); | ||
| assert_eq!( | ||
| stdout, | ||
| "high-level low-level\nAppError true\nfalse true\n42\nundefined\n" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cause forwarding is only fixed for direct
extends Error, not multi-hop chainsThis 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 losecause.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