diff --git a/src/jsc/JSValue.rs b/src/jsc/JSValue.rs index f4e08f90c79c..13eaa7aba4f1 100644 --- a/src/jsc/JSValue.rs +++ b/src/jsc/JSValue.rs @@ -375,19 +375,6 @@ impl JSValue { } JSC__JSValue__isAggregateError(self, global) } - /// `JSValue.getErrorsProperty(globalObject)`. Returns the - /// own `errors` data property via `JSObject::getDirect` — no prototype - /// walk, no getters invoked, nothrow. Used for `AggregateError.errors`. - #[inline] - pub(crate) fn get_errors_property(self, global: &JSGlobalObject) -> JSValue { - unsafe extern "C" { - safe fn JSC__JSValue__getErrorsProperty( - this: JSValue, - global: &JSGlobalObject, - ) -> JSValue; - } - JSC__JSValue__getErrorsProperty(self, global) - } /// `JSValue.isTerminationException()` — true if this /// value is the VM's termination-exception sentinel. #[inline] diff --git a/src/jsc/VirtualMachine.rs b/src/jsc/VirtualMachine.rs index 3a0e43b786b6..7f58442e3754 100644 --- a/src/jsc/VirtualMachine.rs +++ b/src/jsc/VirtualMachine.rs @@ -5149,7 +5149,7 @@ impl VirtualMachine { &mut self, value: JSValue, exception: Option<&Exception>, - exception_list: Option<&mut ExceptionList>, + mut exception_list: Option<&mut ExceptionList>, formatter: &mut crate::console_object::Formatter, writer: &mut bun_core::io::Writer, allow_ansi_color: bool, @@ -5160,7 +5160,18 @@ impl VirtualMachine { // once the AggregateError branch is taken). let global_ref = self.global(); - if value.is_aggregate_error(global_ref) { + let errors = if value.is_aggregate_error(global_ref) { + match value.fast_get(global_ref, jsc::BuiltinName::errors) { + Ok(errors) => errors, + Err(_) => { + global_ref.clear_exception(); + None + } + } + } else { + None + }; + if let Some(errors) = errors { // Note: `JSValue::for_each` takes a C-ABI fn // pointer + erased ctx, so thread the captures through a struct. // The C trampoline erases lifetimes via `*mut c_void`; round-trip @@ -5172,6 +5183,7 @@ impl VirtualMachine { exception_list: *mut ExceptionList, allow_ansi_color: bool, allow_side_effects: bool, + printed_member: bool, } extern "C" fn agg_iter( _vm: *mut crate::VM, @@ -5196,6 +5208,7 @@ impl VirtualMachine { // SAFETY: `ctx.writer` borrows the caller's stack local, // live across the synchronous `for_each` call. let writer = unsafe { &mut *ctx.writer }; + ctx.printed_member = true; vm.print_errorlike_object( next_value, None, @@ -5207,25 +5220,27 @@ impl VirtualMachine { ); } let mut ctx = AggCtx { - formatter: std::ptr::from_mut(formatter), - writer: std::ptr::from_mut(writer), + formatter: std::ptr::from_mut(&mut *formatter), + writer: std::ptr::from_mut(&mut *writer), exception_list: exception_list - .map(std::ptr::from_mut::) - .unwrap_or(core::ptr::null_mut()), + .as_deref_mut() + .map_or(core::ptr::null_mut(), std::ptr::from_mut::), allow_ansi_color, allow_side_effects, + printed_member: false, }; - // `getErrorsProperty` is - // `getDirect` (own data prop, nothrow); `for_each` may throw, in - // which case the error is swallowed. - let errors = value.get_errors_property(global_ref); - let _ = errors.for_each(global_ref, (&raw mut ctx).cast(), agg_iter); - return; + if errors + .for_each(global_ref, (&raw mut ctx).cast(), agg_iter) + .is_err() + { + global_ref.clear_exception(); + } + if ctx.printed_member { + return; + } + // `errors` is empty or not iterable: print the AggregateError itself. } - // Note: reborrow so the add-to-error-list tail can still see it after - // `print_error_from_maybe_private_data`. - let mut exception_list = exception_list; let was_internal = self.print_error_from_maybe_private_data( value, exception_list.as_deref_mut(), diff --git a/src/jsc/bindings/bindings.cpp b/src/jsc/bindings/bindings.cpp index 147f9a5ec434..11b7186026c3 100644 --- a/src/jsc/bindings/bindings.cpp +++ b/src/jsc/bindings/bindings.cpp @@ -4857,12 +4857,6 @@ CPP_DECL double Bun__JSValue__toNumber(JSC::EncodedJSValue JSValue0, JSC::JSGlob return value.toInt32(arg1); } -JSC::EncodedJSValue JSC__JSValue__getErrorsProperty(JSC::EncodedJSValue JSValue0, JSC::JSGlobalObject* global) -{ - JSC::JSObject* obj = JSC::JSValue::decode(JSValue0).getObject(); - return JSC::JSValue::encode(obj->getDirect(global->vm(), global->vm().propertyNames->errors)); -} - [[ZIG_EXPORT(nothrow)]] JSC::EncodedJSValue JSC__JSValue__jsTDZValue() { return JSC::JSValue::encode(JSC::jsTDZValue()); @@ -5316,6 +5310,7 @@ enum class BuiltinNamesMap : uint8_t { type, signal, cmd, + errors, // Private names below: set by builtins via $putByIdDirectPrivate, unreachable from user code. internal, sharedFd, @@ -5398,6 +5393,9 @@ static inline const JSC::Identifier& builtinNameMap(JSC::VM& vm, unsigned char n case BuiltinNamesMap::cmd: { return clientData->builtinNames().cmdPublicName(); } + case BuiltinNamesMap::errors: { + return vm.propertyNames->errors; + } case BuiltinNamesMap::internal: { return clientData->builtinNames().internalPrivateName(); } diff --git a/src/jsc/bindings/headers.h b/src/jsc/bindings/headers.h index 8a06f69c54c7..442d54ea9f09 100644 --- a/src/jsc/bindings/headers.h +++ b/src/jsc/bindings/headers.h @@ -229,7 +229,6 @@ CPP_DECL JSC::EncodedJSValue JSC__JSValue__bigIntFromLatin1(JSC::JSGlobalObject* CPP_DECL JSC::EncodedJSValue JSC__JSValue__fromTimevalNoTruncate(JSC::JSGlobalObject* arg0, int64_t nsec, int64_t sec); CPP_DECL JSC::EncodedJSValue JSC__JSValue__bigIntSum(JSC::JSGlobalObject* arg0, JSC::EncodedJSValue JSValue0, JSC::EncodedJSValue JSValue1); CPP_DECL void JSC__JSValue__getClassName(JSC::EncodedJSValue JSValue0, JSC::JSGlobalObject* arg1, ZigString* arg2); -CPP_DECL JSC::EncodedJSValue JSC__JSValue__getErrorsProperty(JSC::EncodedJSValue JSValue0, JSC::JSGlobalObject* arg1); CPP_DECL JSC::EncodedJSValue JSC__JSValue__getIfPropertyExistsFromPath(JSC::EncodedJSValue JSValue0, JSC::JSGlobalObject* arg1, JSC::EncodedJSValue JSValue2); CPP_DECL double JSC__JSValue__getLengthIfPropertyExistsInternal(JSC::EncodedJSValue JSValue0, JSC::JSGlobalObject* arg1); CPP_DECL void JSC__JSValue__getNameProperty(JSC::EncodedJSValue JSValue0, JSC::JSGlobalObject* arg1, ZigString* arg2); diff --git a/src/jsc/lib.rs b/src/jsc/lib.rs index f14e2e5be5fa..0c0f586bc92a 100644 --- a/src/jsc/lib.rs +++ b/src/jsc/lib.rs @@ -921,6 +921,7 @@ pub enum BuiltinName { type_, signal, cmd, + errors, /// Private name (`$internal` in builtins); user code cannot set it. internal, /// Private name (`$sharedFd` in builtins); user code cannot set it. diff --git a/test/internal/source-lints/jsresult-swallow.inventory.json b/test/internal/source-lints/jsresult-swallow.inventory.json index 060c603c28d6..199e1adfbe31 100644 --- a/test/internal/source-lints/jsresult-swallow.inventory.json +++ b/test/internal/source-lints/jsresult-swallow.inventory.json @@ -1,7 +1,7 @@ { "src/jsc/VirtualMachine.rs": { "JsResult collapsed on the spot": 1, - "discarded result of a call that enters script": 2 + "discarded result of a call that enters script": 1 }, "src/jsc/web_worker.rs": { "taken exception discarded": 1 diff --git a/test/js/bun/util/inspect-error.test.js b/test/js/bun/util/inspect-error.test.js index 61dcdc0de32f..dcf0cc2b7617 100644 --- a/test/js/bun/util/inspect-error.test.js +++ b/test/js/bun/util/inspect-error.test.js @@ -327,3 +327,132 @@ describe("source map remapping of the printed stack", () => { expect(exitCode).toBe(1); }); }); + +// The printer replaces an AggregateError with the members of its `errors` +// property. When there is nothing to walk it has to print the AggregateError +// itself. A deleted `errors` used to crash the process (the empty value was +// passed to the iteration, which read it as a cell at address 0), an accessor +// was passed to it as well, the other shapes printed nothing, and a +// non-iterable `errors` made console.error throw. +describe.concurrent("AggregateError whose errors cannot be walked", () => { + async function run(cmd, files) { + using dir = tempDir("inspect-aggregate-error", files ?? {}); + await using proc = Bun.spawn({ + cmd: [bunExe(), ...cmd], + cwd: String(dir), + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + return { stdout, stderr, exitCode }; + } + + const header = "AggregateError: outer message"; + const make = 'const e = new AggregateError([new Error("inner")], "outer message");'; + const shapes = { + "errors was deleted": "delete e.errors;", + "errors is an empty array": "e.errors = [];", + "errors is not iterable": "e.errors = {};", + "errors is null": "e.errors = null;", + "errors is a primitive": "e.errors = 1;", + "the errors getter throws": 'Object.defineProperty(e, "errors", { get() { throw new Error("getter"); } });', + }; + + for (const [name, shape] of Object.entries(shapes)) { + test(`console.error: ${name}`, async () => { + const { stdout, stderr, exitCode } = await run([ + "-e", + `${make} ${shape} console.error(e); console.log("after");`, + ]); + expect(stderr).toContain(header); + expect(stdout).toBe("after\n"); + expect(exitCode).toBe(0); + }); + + test(`uncaught: ${name}`, async () => { + const { stderr, exitCode } = await run(["-e", `${make} ${shape} throw e;`]); + expect(stderr).toContain(header); + expect(exitCode).toBe(1); + }); + } + + test("Bun.inspect: errors was deleted", async () => { + const { stdout, stderr, exitCode } = await run([ + "-e", + `${make} delete e.errors; console.log(JSON.stringify(Bun.inspect(e)));`, + ]); + expect(JSON.parse(stdout)).toContain(header); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + }); + + test("errors is an accessor: the members it returns are printed", async () => { + const { stdout, stderr, exitCode } = await run([ + "-e", + `${make} + Object.defineProperty(e, "errors", { get() { return [new Error("from the getter")]; } }); + console.error(e); + console.log("after");`, + ]); + expect(stderr).toContain("error: from the getter"); + expect(stderr).not.toContain(header); + expect(stdout).toBe("after\n"); + expect(exitCode).toBe(0); + }); + + test("members are still printed in place of the AggregateError", async () => { + const { stderr, exitCode } = await run([ + "-e", + 'throw new AggregateError([new Error("first member"), new TypeError("second member")], "outer message");', + ]); + expect(stderr).toContain("error: first member"); + expect(stderr).toContain("TypeError: second member"); + expect(stderr).not.toContain(header); + expect(exitCode).toBe(1); + }); + + // Without any user code: a module with two or more build errors rejects its + // first load with an AggregateError of BuildMessages. JSC settles every later + // load of it from the module registry with a copy made by + // JSModuleLoader::duplicateError, which keeps the error type and message but + // not the `errors` property. + const broken = { + "broken.ts": "export function f() {\n const v = {b: {},),r,};\n}\n", + }; + + test("the error a second load of a module that failed to build rejects with", async () => { + const { stdout, stderr, exitCode } = await run(["main.ts"], { + ...broken, + "main.ts": ` + const seen = []; + for (let i = 0; i < 2; i++) { + try { + await import("./broken.ts"); + } catch (e) { + seen.push(e.constructor.name); + if (i === 1) console.error(e); + } + } + console.log(JSON.stringify(seen)); + `, + }); + expect(stdout).toBe('["AggregateError","AggregateError"]\n'); + expect(stderr).toContain("broken.ts"); + expect(exitCode).toBe(0); + }); + + // https://github.com/oven-sh/bun/issues/36963 + test("bun test: two files import a module that failed to build", async () => { + const importer = 'import { f } from "./broken.ts";\nf();\n'; + const { stderr, exitCode } = await run(["test", "./a.test.ts", "./b.test.ts"], { + ...broken, + "a.test.ts": importer, + "b.test.ts": importer, + }); + expect(stderr).toContain("a.test.ts:"); + expect(stderr).toContain("b.test.ts:"); + expect(stderr).toContain("across 2 files"); + expect(exitCode).toBe(1); + }); +});