From 48c525619645177be548710b4e494ce8097f7e66 Mon Sep 17 00:00:00 2001 From: 190n Date: Tue, 1 Jul 2025 23:36:56 -0700 Subject: [PATCH] Fix assertions_only usage in fromJSHostCallGeneric (#20733) --- src/bun.js/ModuleLoader.zig | 2 +- src/bun.js/bindings/AnyPromise.zig | 2 +- src/bun.js/bindings/CatchScope.zig | 286 ++++++++++++--------- src/bun.js/bindings/CatchScopeBinding.cpp | 4 +- src/bun.js/bindings/JSBigInt.zig | 2 +- src/bun.js/bindings/JSObject.zig | 2 +- src/bun.js/bindings/JSPromise.zig | 2 +- src/bun.js/bindings/JSPropertyIterator.zig | 6 +- src/bun.js/bindings/JSValue.zig | 41 +-- src/bun.js/event_loop.zig | 4 +- src/bun.js/event_loop/JSCScheduler.zig | 4 +- src/bun.js/jsc.zig | 3 +- src/bun.js/jsc/host_fn.zig | 23 +- src/string.zig | 6 +- 14 files changed, 219 insertions(+), 168 deletions(-) diff --git a/src/bun.js/ModuleLoader.zig b/src/bun.js/ModuleLoader.zig index 2a9af6ece3..40e3783866 100644 --- a/src/bun.js/ModuleLoader.zig +++ b/src/bun.js/ModuleLoader.zig @@ -473,7 +473,7 @@ pub const AsyncModule = struct { var specifier = specifier_; var referrer = referrer_; var scope: JSC.CatchScope = undefined; - scope.init(globalThis, @src(), .enabled); + scope.init(globalThis, @src()); defer { specifier.deref(); referrer.deref(); diff --git a/src/bun.js/bindings/AnyPromise.zig b/src/bun.js/bindings/AnyPromise.zig index 9d12999dee..c22bac69dc 100644 --- a/src/bun.js/bindings/AnyPromise.zig +++ b/src/bun.js/bindings/AnyPromise.zig @@ -81,7 +81,7 @@ pub const AnyPromise = union(enum) { }; var scope: JSC.CatchScope = undefined; - scope.init(globalObject, @src(), .enabled); + scope.init(globalObject, @src()); defer scope.deinit(); var ctx = Wrapper{ .args = args }; JSC__AnyPromise__wrap(globalObject, this.asValue(), &ctx, @ptrCast(&Wrapper.call)); diff --git a/src/bun.js/bindings/CatchScope.zig b/src/bun.js/bindings/CatchScope.zig index 2d07c70ddd..dca41bd0d1 100644 --- a/src/bun.js/bindings/CatchScope.zig +++ b/src/bun.js/bindings/CatchScope.zig @@ -1,149 +1,183 @@ -//! Binding for JSC::CatchScope. This should be used rarely, only at translation boundaries between -//! JSC's exception checking and Zig's. Make sure not to move it after creation. For instance: -//! -//! ```zig -//! // Declare a CatchScope surrounding the call that may throw an exception -//! var scope: CatchScope = undefined; -//! scope.init(global, @src(), .assertions_only); -//! defer scope.deinit(); -//! -//! const value = external_call(vm, foo, bar, baz); -//! // Calling hasException() suffices to prove that we checked for an exception. -//! // This function's caller does not need to use a CatchScope or ThrowScope -//! // because it can use Zig error unions. -//! if (Environment.allow_assert) assert((value == .zero) == scope.hasException()); -//! return if (value == .zero) error.JSError else value; -//! ``` - -const CatchScope = @This(); - -/// TODO determine size and alignment automatically +// TODO determine size and alignment automatically const size = 56; const alignment = 8; -bytes: [size]u8 align(alignment), -/// Pointer to `bytes`, set by `init()`, used to assert that the location did not change -location: if (Environment.ci_assert) *u8 else void, -enabled: bool, +/// Binding for JSC::CatchScope. This should be used rarely, only at translation boundaries between +/// JSC's exception checking and Zig's. Make sure not to move it after creation. Use this if you are +/// making an external call that has no other way to indicate an exception. +/// +/// ```zig +/// // Declare a CatchScope surrounding the call that may throw an exception +/// var scope: CatchScope = undefined; +/// scope.init(global, @src()); +/// defer scope.deinit(); +/// +/// const value: i32 = external_call(vm, foo, bar, baz); +/// // Calling returnIfException() suffices to prove that we checked for an exception. +/// // This function's caller does not need to use a CatchScope or ThrowScope +/// // because it can use Zig error unions. +/// try scope.returnIfException(); +/// return value; +/// ``` +pub const CatchScope = struct { + bytes: [size]u8 align(alignment), + /// Pointer to `bytes`, set by `init()`, used to assert that the location did not change + location: if (Environment.ci_assert) *u8 else void, -pub const Enable = enum { - /// You are using the CatchScope to check for exceptions. - enabled, - /// You have another way to detect exceptions and are only using the CatchScope to prove that - /// exceptions are checked. - /// - /// This CatchScope will only do anything when assertions are enabled. Otherwise, init and - /// deinit do nothing and it always reports there is no exception. - assertions_only, -}; - -pub fn init( - self: *CatchScope, - global: *jsc.JSGlobalObject, - src: std.builtin.SourceLocation, - /// If not enabled, the scope does nothing (it never has an exception). - /// If you need to do something different when there is an exception, leave enabled. - /// If you are only using the scope to prove you handle exceptions correctly, you can pass - /// `Environment.allow_assert` as `enabled`. - comptime enable_condition: Enable, -) void { - const enabled = comptime switch (enable_condition) { - .enabled => true, - .assertions_only => Environment.ci_assert, - }; - if (comptime enabled) { + pub fn init( + self: *CatchScope, + global: *jsc.JSGlobalObject, + src: std.builtin.SourceLocation, + ) void { CatchScope__construct( &self.bytes, global, src.fn_name, src.file, src.line, - @sizeOf(@TypeOf(self.bytes)), - @typeInfo(CatchScope).@"struct".fields[0].alignment, + size, + alignment, ); + + self.* = .{ + .bytes = self.bytes, + .location = if (Environment.ci_assert) &self.bytes[0], + }; } - self.* = .{ - .bytes = self.bytes, - .location = if (Environment.ci_assert) &self.bytes[0], - .enabled = enabled, - }; -} -/// Generate a useful message including where the exception was thrown. -/// Only intended to be called when there is a pending exception. -fn assertionFailure(self: *CatchScope, proof: *jsc.Exception) noreturn { - _ = proof; - if (Environment.allow_assert) bun.assert(self.location == &self.bytes[0]); - CatchScope__assertNoException(&self.bytes); - @panic("assertionFailure called without a pending exception"); -} - -pub fn hasException(self: *CatchScope) bool { - return self.exception() != null; -} - -/// Get the thrown exception if it exists (like scope.exception() in C++) -pub fn exception(self: *CatchScope) ?*jsc.Exception { - if (comptime Environment.ci_assert) bun.assert(self.location == &self.bytes[0]); - if (!self.enabled) return null; - return CatchScope__pureException(&self.bytes); -} - -/// Get the thrown exception if it exists, or if an unhandled trap causes an exception to be thrown -pub fn exceptionIncludingTraps(self: *CatchScope) ?*jsc.Exception { - if (comptime Environment.ci_assert) bun.assert(self.location == &self.bytes[0]); - if (!self.enabled) return null; - return CatchScope__exceptionIncludingTraps(&self.bytes); -} - -/// Intended for use with `try`. Returns if there is already a pending exception or if traps cause -/// an exception to be thrown (this is the same as how RETURN_IF_EXCEPTION behaves in C++) -pub fn returnIfException(self: *CatchScope) bun.JSError!void { - if (self.exceptionIncludingTraps() != null) return error.JSError; -} - -/// Asserts there has not been any exception thrown. -pub fn assertNoException(self: *CatchScope) void { - if (comptime Environment.ci_assert) { - if (self.exception()) |e| self.assertionFailure(e); + /// Generate a useful message including where the exception was thrown. + /// Only intended to be called when there is a pending exception. + fn assertionFailure(self: *CatchScope, proof: *jsc.Exception) noreturn { + _ = proof; + bun.assert(self.location == &self.bytes[0]); + CatchScope__assertNoException(&self.bytes); + @panic("assertionFailure called without a pending exception"); } -} -/// Asserts that there is or is not an exception according to the value of `should_have_exception`. -/// Prefer over `assert(scope.hasException() == ...)` because if there is an unexpected exception, -/// this function prints a trace of where it was thrown. -pub fn assertExceptionPresenceMatches(self: *CatchScope, should_have_exception: bool) void { - if (comptime Environment.ci_assert) { - // paranoid; will only fail if you manually changed enabled to false - bun.assert(self.enabled); - if (should_have_exception) { - bun.assertf(self.hasException(), "Expected an exception to be thrown", .{}); - } else { - self.assertNoException(); + pub fn hasException(self: *CatchScope) bool { + return self.exception() != null; + } + + /// Get the thrown exception if it exists (like scope.exception() in C++) + pub fn exception(self: *CatchScope) ?*jsc.Exception { + if (comptime Environment.ci_assert) bun.assert(self.location == &self.bytes[0]); + return CatchScope__pureException(&self.bytes); + } + + /// Get the thrown exception if it exists, or if an unhandled trap causes an exception to be thrown + pub fn exceptionIncludingTraps(self: *CatchScope) ?*jsc.Exception { + if (comptime Environment.ci_assert) bun.assert(self.location == &self.bytes[0]); + return CatchScope__exceptionIncludingTraps(&self.bytes); + } + + /// Intended for use with `try`. Returns if there is already a pending exception or if traps cause + /// an exception to be thrown (this is the same as how RETURN_IF_EXCEPTION behaves in C++) + pub fn returnIfException(self: *CatchScope) bun.JSError!void { + if (self.exceptionIncludingTraps() != null) return error.JSError; + } + + /// Asserts there has not been any exception thrown. + pub fn assertNoException(self: *CatchScope) void { + if (comptime Environment.ci_assert) { + if (self.exception()) |e| self.assertionFailure(e); } } -} -/// If no exception, returns. -/// If termination exception, returns JSExecutionTerminated (so you can `try`) -/// If non-termination exception, assertion failure. -pub fn assertNoExceptionExceptTermination(self: *CatchScope) bun.JSExecutionTerminated!void { - bun.assert(self.enabled); - if (self.exception()) |e| { - if (jsc.JSValue.fromCell(e).isTerminationException()) - return error.JSExecutionTerminated - else if (comptime Environment.ci_assert) - self.assertionFailure(e); - // Unconditionally panicing here is worse for our users. + /// Asserts that there is or is not an exception according to the value of `should_have_exception`. + /// Prefer over `assert(scope.hasException() == ...)` because if there is an unexpected exception, + /// this function prints a trace of where it was thrown. + pub fn assertExceptionPresenceMatches(self: *CatchScope, should_have_exception: bool) void { + if (comptime Environment.ci_assert) { + if (should_have_exception) { + bun.assertf(self.hasException(), "Expected an exception to be thrown", .{}); + } else { + self.assertNoException(); + } + } } -} -pub fn deinit(self: *CatchScope) void { - if (comptime Environment.ci_assert) bun.assert(self.location == &self.bytes[0]); - if (!self.enabled) return; - CatchScope__destruct(&self.bytes); - self.bytes = undefined; -} + /// If no exception, returns. + /// If termination exception, returns JSExecutionTerminated (so you can `try`) + /// If non-termination exception, assertion failure. + pub fn assertNoExceptionExceptTermination(self: *CatchScope) bun.JSExecutionTerminated!void { + if (self.exception()) |e| { + if (jsc.JSValue.fromCell(e).isTerminationException()) + return error.JSExecutionTerminated + else if (comptime Environment.ci_assert) + self.assertionFailure(e); + // Unconditionally panicking here is worse for our users. + } + } + + pub fn deinit(self: *CatchScope) void { + if (comptime Environment.ci_assert) bun.assert(self.location == &self.bytes[0]); + CatchScope__destruct(&self.bytes); + self.bytes = undefined; + } +}; + +/// Limited subset of CatchScope functionality, for when you have a different way to detect +/// exceptions and you only need a CatchScope to prove that you are checking exceptions correctly. +/// Gated by `Environment.ci_assert`. +/// +/// ```zig +/// var scope: ExceptionValidationScope = undefined; +/// // these do nothing when ci_assert == false +/// scope.init(global, @src()); +/// defer scope.deinit(); +/// +/// const maybe_empty: JSValue = externalFunction(global, foo, bar, baz); +/// // does nothing when ci_assert == false +/// // with assertions on, this call serves as proof that you checked for an exception +/// scope.assertExceptionPresenceMatches(maybe_empty == .zero); +/// // you decide whether to return JSError using the return value instead of the scope +/// return if (value == .zero) error.JSError else value; +/// ``` +pub const ExceptionValidationScope = struct { + scope: if (Environment.ci_assert) CatchScope else void, + + pub fn init( + self: *ExceptionValidationScope, + global: *jsc.JSGlobalObject, + src: std.builtin.SourceLocation, + ) void { + if (Environment.ci_assert) self.scope.init(global, src); + } + + /// Asserts there has not been any exception thrown. + pub fn assertNoException(self: *ExceptionValidationScope) void { + if (Environment.ci_assert) { + self.scope.assertNoException(); + } + } + + /// Asserts that there is or is not an exception according to the value of `should_have_exception`. + /// Prefer over `assert(scope.hasException() == ...)` because if there is an unexpected exception, + /// this function prints a trace of where it was thrown. + pub fn assertExceptionPresenceMatches(self: *ExceptionValidationScope, should_have_exception: bool) void { + if (Environment.ci_assert) { + self.scope.assertExceptionPresenceMatches(should_have_exception); + } + } + + /// If no exception, returns. + /// If termination exception, returns JSExecutionTerminated (so you can `try`) + /// If non-termination exception, assertion failure. + pub fn assertNoExceptionExceptTermination(self: *ExceptionValidationScope) bun.JSExecutionTerminated!void { + if (Environment.ci_assert) { + return self.scope.assertNoExceptionExceptTermination(); + } + } + + /// Inconveniently named on purpose; this is only needed for some weird edge cases + pub fn hasExceptionOrFalseWhenAssertionsAreDisabled(self: *ExceptionValidationScope) bool { + return if (Environment.ci_assert) self.scope.hasException() else false; + } + + pub fn deinit(self: *ExceptionValidationScope) void { + if (Environment.ci_assert) self.scope.deinit(); + } +}; extern fn CatchScope__construct( ptr: *align(alignment) [size]u8, diff --git a/src/bun.js/bindings/CatchScopeBinding.cpp b/src/bun.js/bindings/CatchScopeBinding.cpp index 3efe668735..46b9a5458d 100644 --- a/src/bun.js/bindings/CatchScopeBinding.cpp +++ b/src/bun.js/bindings/CatchScopeBinding.cpp @@ -53,5 +53,7 @@ extern "C" void CatchScope__destruct(void* ptr) extern "C" void CatchScope__assertNoException(void* ptr) { ASSERT((uintptr_t)ptr % alignof(CatchScope) == 0); - static_cast(ptr)->assertNoException(); + // this function assumes it should assert in all build modes, anything else would be confusing. + // Zig should only call CatchScope__assertNoException if it wants the assertion. + static_cast(ptr)->releaseAssertNoException(); } diff --git a/src/bun.js/bindings/JSBigInt.zig b/src/bun.js/bindings/JSBigInt.zig index 8e6249aba7..19fe87ec93 100644 --- a/src/bun.js/bindings/JSBigInt.zig +++ b/src/bun.js/bindings/JSBigInt.zig @@ -38,7 +38,7 @@ pub const JSBigInt = opaque { extern fn JSC__JSBigInt__toString(*JSBigInt, *JSGlobalObject) bun.String; pub fn toString(this: *JSBigInt, global: *JSGlobalObject) JSError!bun.String { var scope: jsc.CatchScope = undefined; - scope.init(global, @src(), .enabled); + scope.init(global, @src()); defer scope.deinit(); const str = JSC__JSBigInt__toString(this, global); try scope.returnIfException(); diff --git a/src/bun.js/bindings/JSObject.zig b/src/bun.js/bindings/JSObject.zig index 188ae137ee..7046faed46 100644 --- a/src/bun.js/bindings/JSObject.zig +++ b/src/bun.js/bindings/JSObject.zig @@ -150,7 +150,7 @@ pub const JSObject = opaque { // with an exception: // https://github.com/oven-sh/WebKit/blob/397dafc9721b8f8046f9448abb6dbc14efe096d3/Source/JavaScriptCore/runtime/JSObjectInlines.h#L112 var scope: JSC.CatchScope = undefined; - scope.init(globalThis, @src(), .enabled); + scope.init(globalThis, @src()); defer scope.deinit(); const value = JSC__JSObject__getIndex(this, globalThis, i); try scope.returnIfException(); diff --git a/src/bun.js/bindings/JSPromise.zig b/src/bun.js/bindings/JSPromise.zig index 2fbc6e2084..ef3c62147e 100644 --- a/src/bun.js/bindings/JSPromise.zig +++ b/src/bun.js/bindings/JSPromise.zig @@ -194,7 +194,7 @@ pub const JSPromise = opaque { }; var scope: JSC.CatchScope = undefined; - scope.init(globalObject, @src(), .enabled); + scope.init(globalObject, @src()); defer scope.deinit(); var ctx = Wrapper{ .args = args }; const promise = JSC__JSPromise__wrap(globalObject, &ctx, @ptrCast(&Wrapper.call)); diff --git a/src/bun.js/bindings/JSPropertyIterator.zig b/src/bun.js/bindings/JSPropertyIterator.zig index 55bae0558e..3d1bb1d34a 100644 --- a/src/bun.js/bindings/JSPropertyIterator.zig +++ b/src/bun.js/bindings/JSPropertyIterator.zig @@ -117,7 +117,7 @@ const JSPropertyIteratorImpl = opaque { only_non_index_properties: bool, ) bun.JSError!?*JSPropertyIteratorImpl { var scope: JSC.CatchScope = undefined; - scope.init(globalObject, @src(), .enabled); + scope.init(globalObject, @src()); defer scope.deinit(); const iter = Bun__JSPropertyIterator__create(globalObject, object.toJS(), count, own_properties_only, only_non_index_properties); try scope.returnIfException(); @@ -128,7 +128,7 @@ const JSPropertyIteratorImpl = opaque { pub fn getNameAndValue(iter: *JSPropertyIteratorImpl, globalObject: *JSC.JSGlobalObject, object: *JSC.JSObject, propertyName: *bun.String, i: usize) bun.JSError!JSC.JSValue { var scope: bun.jsc.CatchScope = undefined; - scope.init(globalObject, @src(), .enabled); + scope.init(globalObject, @src()); defer scope.deinit(); const value = Bun__JSPropertyIterator__getNameAndValue(iter, globalObject, object, propertyName, i); try scope.returnIfException(); @@ -137,7 +137,7 @@ const JSPropertyIteratorImpl = opaque { pub fn getNameAndValueNonObservable(iter: *JSPropertyIteratorImpl, globalObject: *JSC.JSGlobalObject, object: *JSC.JSObject, propertyName: *bun.String, i: usize) bun.JSError!JSC.JSValue { var scope: bun.jsc.CatchScope = undefined; - scope.init(globalObject, @src(), .enabled); + scope.init(globalObject, @src()); defer scope.deinit(); const value = Bun__JSPropertyIterator__getNameAndValueNonObservable(iter, globalObject, object, propertyName, i); try scope.returnIfException(); diff --git a/src/bun.js/bindings/JSValue.zig b/src/bun.js/bindings/JSValue.zig index 2407342255..4a100cd5ee 100644 --- a/src/bun.js/bindings/JSValue.zig +++ b/src/bun.js/bindings/JSValue.zig @@ -91,7 +91,7 @@ pub const JSValue = enum(i64) { callback: PropertyIteratorFn, ) JSError!void { var scope: CatchScope = undefined; - scope.init(globalThis, @src(), .enabled); + scope.init(globalThis, @src()); defer scope.deinit(); JSC__JSValue__forEachPropertyNonIndexed(this, globalThis, ctx, callback); try scope.returnIfException(); @@ -104,7 +104,7 @@ pub const JSValue = enum(i64) { callback: PropertyIteratorFn, ) JSError!void { var scope: CatchScope = undefined; - scope.init(globalThis, @src(), .enabled); + scope.init(globalThis, @src()); defer scope.deinit(); JSC__JSValue__forEachProperty(this, globalThis, ctx, callback); try scope.returnIfException(); @@ -117,7 +117,7 @@ pub const JSValue = enum(i64) { callback: PropertyIteratorFn, ) JSError!void { var scope: CatchScope = undefined; - scope.init(globalThis, @src(), .enabled); + scope.init(globalThis, @src()); defer scope.deinit(); JSC__JSValue__forEachPropertyOrdered(this, globalThis, ctx, callback); try scope.returnIfException(); @@ -130,7 +130,7 @@ pub const JSValue = enum(i64) { /// https://tc39.es/ecma262/#sec-tonumber pub fn toNumber(this: JSValue, global: *JSGlobalObject) bun.JSError!f64 { var scope: bun.jsc.CatchScope = undefined; - scope.init(global, @src(), .enabled); + scope.init(global, @src()); defer scope.deinit(); const result = Bun__JSValue__toNumber(this, global); try scope.returnIfException(); @@ -790,7 +790,7 @@ pub const JSValue = enum(i64) { /// If the object is not an object, it will crash. **You must check if the object is an object before calling this function.** pub fn hasOwnPropertyValue(this: JSValue, global: *JSGlobalObject, key: JSValue) JSError!bool { var scope: CatchScope = undefined; - scope.init(global, @src(), .enabled); + scope.init(global, @src()); defer scope.deinit(); const result = JSC__JSValue__hasOwnPropertyValue(this, global, key); try scope.returnIfException(); @@ -1251,8 +1251,8 @@ pub const JSValue = enum(i64) { extern fn JSC__JSValue__toStringOrNull(this: JSValue, globalThis: *JSGlobalObject) ?*JSString; // Calls JSValue::toStringOrNull. Returns error on exception. pub fn toJSString(this: JSValue, globalThis: *JSGlobalObject) bun.JSError!*JSString { - var scope: CatchScope = undefined; - scope.init(globalThis, @src(), .assertions_only); + var scope: ExceptionValidationScope = undefined; + scope.init(globalThis, @src()); defer scope.deinit(); const maybe_string = JSC__JSValue__toStringOrNull(this, globalThis); scope.assertExceptionPresenceMatches(maybe_string == null); @@ -1429,7 +1429,7 @@ pub const JSValue = enum(i64) { extern fn JSC__JSValue__getIfPropertyExistsFromPath(this: JSValue, global: *JSGlobalObject, path: JSValue) JSValue; pub fn getIfPropertyExistsFromPath(this: JSValue, global: *JSGlobalObject, path: JSValue) JSError!JSValue { var scope: CatchScope = undefined; - scope.init(global, @src(), .enabled); + scope.init(global, @src()); defer scope.deinit(); const result = JSC__JSValue__getIfPropertyExistsFromPath(this, global, path); try scope.returnIfException(); @@ -1458,7 +1458,7 @@ pub const JSValue = enum(i64) { pub fn _then2(this: JSValue, global: *JSGlobalObject, ctx: JSValue, resolve: *const JSC.JSHostFn, reject: *const JSC.JSHostFn) void { var scope: CatchScope = undefined; - scope.init(global, @src(), .enabled); + scope.init(global, @src()); defer scope.deinit(); JSC__JSValue___then(this, global, ctx, resolve, reject); bun.debugAssert(!scope.hasException()); // TODO: properly propagate exception upwards @@ -1466,7 +1466,7 @@ pub const JSValue = enum(i64) { pub fn then(this: JSValue, global: *JSGlobalObject, ctx: ?*anyopaque, resolve: JSC.JSHostFnZig, reject: JSC.JSHostFnZig) void { var scope: CatchScope = undefined; - scope.init(global, @src(), .enabled); + scope.init(global, @src()); defer scope.deinit(); this._then(global, JSValue.fromPtrAddress(@intFromPtr(ctx)), resolve, reject); bun.debugAssert(!scope.hasException()); // TODO: properly propagate exception upwards @@ -1545,7 +1545,7 @@ pub const JSValue = enum(i64) { pub fn getOwn(this: JSValue, global: *JSGlobalObject, property_name: anytype) bun.JSError!?JSValue { var property_name_str = bun.String.init(property_name); var scope: CatchScope = undefined; - scope.init(global, @src(), .enabled); + scope.init(global, @src()); defer scope.deinit(); const value = JSC__JSValue__getOwn(this, global, &property_name_str); try scope.returnIfException(); @@ -1648,7 +1648,7 @@ pub const JSValue = enum(i64) { /// - an empty string pub fn getStringish(this: JSValue, global: *JSGlobalObject, property: []const u8) bun.JSError!?bun.String { var scope: CatchScope = undefined; - scope.init(global, @src(), .enabled); + scope.init(global, @src()); defer scope.deinit(); const prop = try get(this, global, property) orelse return null; if (prop.isNull() or prop == .false) { @@ -1910,7 +1910,7 @@ pub const JSValue = enum(i64) { pub fn isSameValue(this: JSValue, other: JSValue, global: *JSGlobalObject) JSError!bool { if (@intFromEnum(this) == @intFromEnum(other)) return true; var scope: CatchScope = undefined; - scope.init(global, @src(), .enabled); + scope.init(global, @src()); defer scope.deinit(); const same = JSC__JSValue__isSameValue(this, other, global); try scope.returnIfException(); @@ -1920,7 +1920,7 @@ pub const JSValue = enum(i64) { extern fn JSC__JSValue__deepEquals(this: JSValue, other: JSValue, global: *JSGlobalObject) bool; pub fn deepEquals(this: JSValue, other: JSValue, global: *JSGlobalObject) JSError!bool { var scope: CatchScope = undefined; - scope.init(global, @src(), .enabled); + scope.init(global, @src()); defer scope.deinit(); const result = JSC__JSValue__deepEquals(this, other, global); try scope.returnIfException(); @@ -1930,7 +1930,7 @@ pub const JSValue = enum(i64) { /// same as `JSValue.deepEquals`, but with jest asymmetric matchers enabled pub fn jestDeepEquals(this: JSValue, other: JSValue, global: *JSGlobalObject) JSError!bool { var scope: CatchScope = undefined; - scope.init(global, @src(), .enabled); + scope.init(global, @src()); defer scope.deinit(); const result = JSC__JSValue__jestDeepEquals(this, other, global); try scope.returnIfException(); @@ -1940,7 +1940,7 @@ pub const JSValue = enum(i64) { extern fn JSC__JSValue__strictDeepEquals(this: JSValue, other: JSValue, global: *JSGlobalObject) bool; pub fn strictDeepEquals(this: JSValue, other: JSValue, global: *JSGlobalObject) JSError!bool { var scope: CatchScope = undefined; - scope.init(global, @src(), .enabled); + scope.init(global, @src()); defer scope.deinit(); const result = JSC__JSValue__strictDeepEquals(this, other, global); try scope.returnIfException(); @@ -1950,7 +1950,7 @@ pub const JSValue = enum(i64) { /// same as `JSValue.strictDeepEquals`, but with jest asymmetric matchers enabled pub fn jestStrictDeepEquals(this: JSValue, other: JSValue, global: *JSGlobalObject) JSError!bool { var scope: CatchScope = undefined; - scope.init(global, @src(), .enabled); + scope.init(global, @src()); defer scope.deinit(); const result = JSC__JSValue__jestStrictDeepEquals(this, other, global); try scope.returnIfException(); @@ -1960,7 +1960,7 @@ pub const JSValue = enum(i64) { /// same as `JSValue.deepMatch`, but with jest asymmetric matchers enabled pub fn jestDeepMatch(this: JSValue, subset: JSValue, global: *JSGlobalObject, replace_props_with_asymmetric_matchers: bool) JSError!bool { var scope: CatchScope = undefined; - scope.init(global, @src(), .enabled); + scope.init(global, @src()); defer scope.deinit(); const result = JSC__JSValue__jestDeepMatch(this, subset, global, replace_props_with_asymmetric_matchers); try scope.returnIfException(); @@ -2181,7 +2181,7 @@ pub const JSValue = enum(i64) { /// TODO this should probably just return an optional pub fn getLengthIfPropertyExistsInternal(this: JSValue, globalThis: *JSGlobalObject) JSError!f64 { var scope: CatchScope = undefined; - scope.init(globalThis, @src(), .enabled); + scope.init(globalThis, @src()); defer scope.deinit(); const length = JSC__JSValue__getLengthIfPropertyExistsInternal(this, globalThis); try scope.returnIfException(); @@ -2217,7 +2217,7 @@ pub const JSValue = enum(i64) { extern fn JSC__JSValue__isIterable(this: JSValue, globalObject: *JSGlobalObject) bool; pub fn isIterable(this: JSValue, globalObject: *JSGlobalObject) JSError!bool { var scope: CatchScope = undefined; - scope.init(globalObject, @src(), .enabled); + scope.init(globalObject, @src()); defer scope.deinit(); const is_iterable = JSC__JSValue__isIterable(this, globalObject); try scope.returnIfException(); @@ -2487,6 +2487,7 @@ const JSArrayIterator = JSC.JSArrayIterator; const JSCell = JSC.JSCell; const fromJSHostCall = JSC.fromJSHostCall; const CatchScope = JSC.CatchScope; +const ExceptionValidationScope = JSC.ExceptionValidationScope; const AnyPromise = JSC.AnyPromise; const DOMURL = JSC.DOMURL; diff --git a/src/bun.js/event_loop.zig b/src/bun.js/event_loop.zig index a583403ba0..e84d5b2934 100644 --- a/src/bun.js/event_loop.zig +++ b/src/bun.js/event_loop.zig @@ -110,7 +110,7 @@ extern fn JSC__JSGlobalObject__drainMicrotasks(*JSC.JSGlobalObject) void; pub fn drainMicrotasksWithGlobal(this: *EventLoop, globalObject: *JSC.JSGlobalObject, jsc_vm: *JSC.VM) bun.JSExecutionTerminated!void { JSC.markBinding(@src()); var scope: JSC.CatchScope = undefined; - scope.init(globalObject, @src(), .enabled); + scope.init(globalObject, @src()); defer scope.deinit(); jsc_vm.releaseWeakRefs(); @@ -449,7 +449,7 @@ pub fn processGCTimer(this: *EventLoop) void { pub fn tick(this: *EventLoop) void { JSC.markBinding(@src()); var scope: JSC.CatchScope = undefined; - scope.init(this.global, @src(), .enabled); + scope.init(this.global, @src()); defer scope.deinit(); this.entered_event_loop_count += 1; this.debug.enter(); diff --git a/src/bun.js/event_loop/JSCScheduler.zig b/src/bun.js/event_loop/JSCScheduler.zig index 6a9a9dacd8..09c4c72205 100644 --- a/src/bun.js/event_loop/JSCScheduler.zig +++ b/src/bun.js/event_loop/JSCScheduler.zig @@ -4,8 +4,8 @@ pub const JSCDeferredWorkTask = opaque { extern fn Bun__runDeferredWork(task: *JSCScheduler.JSCDeferredWorkTask) void; pub fn run(task: *JSCScheduler.JSCDeferredWorkTask) void { const globalThis = bun.jsc.VirtualMachine.get().global; - var scope: bun.jsc.CatchScope = undefined; - scope.init(globalThis, @src(), .assertions_only); + var scope: bun.jsc.ExceptionValidationScope = undefined; + scope.init(globalThis, @src()); defer scope.deinit(); Bun__runDeferredWork(task); scope.assertNoExceptionExceptTermination() catch return; diff --git a/src/bun.js/jsc.zig b/src/bun.js/jsc.zig index 2442416cf6..ec091dafed 100644 --- a/src/bun.js/jsc.zig +++ b/src/bun.js/jsc.zig @@ -80,7 +80,8 @@ pub const Weak = @import("Weak.zig").Weak; pub const WeakRefType = @import("Weak.zig").WeakRefType; pub const Exception = @import("bindings/Exception.zig").Exception; pub const SourceProvider = @import("bindings/SourceProvider.zig").SourceProvider; -pub const CatchScope = @import("bindings/CatchScope.zig"); +pub const CatchScope = @import("bindings/CatchScope.zig").CatchScope; +pub const ExceptionValidationScope = @import("bindings/CatchScope.zig").ExceptionValidationScope; // JavaScript-related pub const Errorable = @import("bindings/Errorable.zig").Errorable; diff --git a/src/bun.js/jsc/host_fn.zig b/src/bun.js/jsc/host_fn.zig index 9ac2f008a4..3b8677944b 100644 --- a/src/bun.js/jsc/host_fn.zig +++ b/src/bun.js/jsc/host_fn.zig @@ -90,8 +90,8 @@ pub fn toJSHostCall( // runtime tuple values args: anytype, ) JSValue { - var scope: jsc.CatchScope = undefined; - scope.init(globalThis, src, .assertions_only); + var scope: jsc.ExceptionValidationScope = undefined; + scope.init(globalThis, src); defer scope.deinit(); const returned: error{ OutOfMemory, JSError }!JSValue = @call(.auto, function, args); @@ -105,6 +105,9 @@ pub fn toJSHostCall( /// Convert the return value of a function returning a maybe-empty JSValue into an error union. /// The wrapped function must return an empty JSValue if and only if it has thrown an exception. +/// If your function does not follow this pattern (if it can return empty without an exception, or +/// throw an exception and return non-empty), either fix the function or write a custom wrapper with +/// CatchScope. pub fn fromJSHostCall( globalThis: *JSGlobalObject, /// For attributing thrown exceptions @@ -112,8 +115,8 @@ pub fn fromJSHostCall( comptime function: anytype, args: std.meta.ArgsTuple(@TypeOf(function)), ) bun.JSError!JSValue { - var scope: jsc.CatchScope = undefined; - scope.init(globalThis, src, .assertions_only); + var scope: jsc.ExceptionValidationScope = undefined; + scope.init(globalThis, src); defer scope.deinit(); const value = @call(.auto, function, args); @@ -129,10 +132,20 @@ pub fn fromJSHostCallGeneric( args: std.meta.ArgsTuple(@TypeOf(function)), ) bun.JSError!@typeInfo(@TypeOf(function)).@"fn".return_type.? { var scope: jsc.CatchScope = undefined; - scope.init(globalThis, src, .assertions_only); + scope.init(globalThis, src); defer scope.deinit(); const result = @call(.auto, function, args); + // supporting JSValue would make it too easy to mix up this function with fromJSHostCall + // fromJSHostCall has the benefit of checking that the function is correctly returning an empty + // value if and only if it has thrown. + // fromJSHostCallGeneric is only for functions where the return value tells you nothing about + // whether an exception was thrown. + // + // alternatively, we could consider something like `comptime exception_sentinel: ?T` + // to generically support using a value of any type to signal exceptions (INT_MAX, infinity, + // nullptr...?) but it's unclear how often that would be useful + if (@TypeOf(result) == JSValue) @compileError("fromJSHostCallGeneric does not support JSValue"); try scope.returnIfException(); return result; } diff --git a/src/string.zig b/src/string.zig index be35bbd54e..4c95e61b68 100644 --- a/src/string.zig +++ b/src/string.zig @@ -514,15 +514,15 @@ pub const String = extern struct { } pub fn fromJS(value: bun.JSC.JSValue, globalObject: *JSC.JSGlobalObject) bun.JSError!String { - var scope: JSC.CatchScope = undefined; - scope.init(globalObject, @src(), .assertions_only); + var scope: JSC.ExceptionValidationScope = undefined; + scope.init(globalObject, @src()); defer scope.deinit(); var out: String = String.dead; const ok = BunString__fromJS(globalObject, value, &out); // If there is a pending exception, but stringifying succeeds, we don't return JSError. // We do need to always call hasException() to satisfy the need for an exception check. - const has_exception = scope.hasException(); + const has_exception = scope.hasExceptionOrFalseWhenAssertionsAreDisabled(); if (ok) { bun.debugAssert(out.tag != .Dead); } else {