a

src/bun.js/bindings/bindings.cpp

@@ -3787,6 +3787,8 @@ JSC__JSValue JSC__JSValue__createObject2(JSC__JSGlobalObject* globalObject, cons
     return JSC::JSValue::encode(object);
 }
 
+// Returns empty for exception, returns deleted if not found.
+// Be careful when handling the return value.
 JSC__JSValue JSC__JSValue__getIfPropertyExistsImpl(JSC__JSValue JSValue0,
     JSC__JSGlobalObject* globalObject,
     const unsigned char* arg1, uint32_t arg2)
@@ -3798,6 +3800,7 @@ JSC__JSValue JSC__JSValue__getIfPropertyExistsImpl(JSC__JSValue JSValue0,
     JSC::VM& vm = globalObject->vm();
     JSC::JSObject* object = value.getObject();
     if (UNLIKELY(!object)) {
+        // It is possible to pass the isObject() check and still have a null object.
         return JSValue::encode(JSValue::decode(JSC::JSValue::ValueDeleted));
     }
 
@@ -5175,6 +5178,8 @@ JSC__JSValue JSC__JSValue__fastGetDirect_(JSC__JSValue JSValue0, JSC__JSGlobalOb
     return JSValue::encode(value.getObject()->getDirect(globalObject->vm(), PropertyName(builtinNameMap(globalObject->vm(), arg2))));
 }
 
+// Returns empty for exception, returns deleted if not found.
+// Be careful when handling the return value.
 JSC__JSValue JSC__JSValue__fastGet(JSC__JSValue JSValue0, JSC__JSGlobalObject* globalObject, unsigned char arg2)
 {
     JSC::JSValue value = JSC::JSValue::decode(JSValue0);

src/bun.js/bindings/bindings.zig

@@ -5284,26 +5284,20 @@ pub const JSValue = enum(i64) {
         };
     }
 
-    // `this` must be known to be an object
-    // intended to be more lightweight than ZigString.
+    /// Deprecated: `JSValue.get` already performs this optimization for you
+    /// Deprecated: This masks thrown exceptions. `null` may either have an exception or not
+    ///
+    /// `this` must be known to be an object
+    /// intended to be more lightweight than ZigString.
     pub fn fastGet(this: JSValue, global: *JSGlobalObject, builtin_name: BuiltinName) ?JSValue {
         if (bun.Environment.isDebug)
             bun.assert(this.isObject());
 
-        return switch (JSC__JSValue__fastGet(this, global, @intFromEnum(builtin_name))) {
-            .zero, .undefined, .property_does_not_exist_on_object => null,
-            else => |val| val,
-        };
-    }
+        const unwrapped = JSC__JSValue__fastGet(this, global, @intFromEnum(builtin_name)).deprecatedUnwrapHidingException() orelse
+            return null;
 
-    pub fn fastGetWithError(this: JSValue, global: *JSGlobalObject, builtin_name: BuiltinName) JSError!?JSValue {
-        if (bun.Environment.isDebug)
-            bun.assert(this.isObject());
-
-        return switch (JSC__JSValue__fastGet(this, global, @intFromEnum(builtin_name))) {
-            .zero => error.JSError,
+        return switch (unwrapped) {
             .undefined => null,
-            .property_does_not_exist_on_object => null,
             else => |val| val,
         };
     }
@@ -5317,7 +5311,6 @@ pub const JSValue = enum(i64) {
         return result;
     }
 
-    extern fn JSC__JSValue__fastGet(value: JSValue, global: *JSGlobalObject, builtin_id: u8) JSValue;
     extern fn JSC__JSValue__fastGetOwn(value: JSValue, globalObject: *JSGlobalObject, property: BuiltinName) JSValue;
     pub fn fastGetOwn(this: JSValue, global: *JSGlobalObject, builtin_name: BuiltinName) ?JSValue {
         const result = JSC__JSValue__fastGetOwn(this, global, builtin_name);
@@ -5332,7 +5325,42 @@ pub const JSValue = enum(i64) {
         return cppFn("fastGetDirect_", .{ this, global, builtin_name });
     }
 
-    extern fn JSC__JSValue__getIfPropertyExistsImpl(target: JSValue, global: *JSGlobalObject, ptr: [*]const u8, len: u32) JSValue;
+    /// Problem: The `get` needs to model !?JSValue
+    /// - null  -> the property does not exist
+    /// - error -> the get operation threw
+    /// - any other JSValue -> success. this could be jsNull() or jsUndefined()
+    ///
+    /// `.zero` is already used for the error state
+    ///
+    /// Deleted is a special encoding used in JSC hash map internals used for
+    /// the null state. It is re-used here for encoding the "not present" state.
+    const GetResult = enum(i64) {
+        thrown_exception = 0,
+        does_not_exist = 0x4, // JSC::JSValue::ValueDeleted
+        _,
+
+        fn deprecatedUnwrapHidingException(value: GetResult) ?JSValue {
+            return switch (value) {
+                // footgun! caller must check hasException on every `get` or else Bun will crash
+                .thrown_exception => null,
+                .does_not_exist => null,
+                else => @enumFromInt(@intFromEnum(value)),
+            };
+        }
+
+        fn unwrap(value: GetResult, global: *JSGlobalObject) JSError!?JSValue {
+            return switch (value) {
+                .thrown_exception => {
+                    bun.assert(global.hasException());
+                    return error.JSError;
+                },
+                .does_not_exist => null,
+                else => @enumFromInt(@intFromEnum(value)),
+            };
+        }
+    };
+    extern fn JSC__JSValue__getIfPropertyExistsImpl(target: JSValue, global: *JSGlobalObject, ptr: [*]const u8, len: u32) GetResult;
+    extern fn JSC__JSValue__fastGet(value: JSValue, global: *JSGlobalObject, builtin_id: u8) GetResult;
 
     pub fn getIfPropertyExistsFromPath(this: JSValue, global: *JSGlobalObject, path: JSValue) JSValue {
         return cppFn("getIfPropertyExistsFromPath", .{ this, global, path });
@@ -5381,10 +5409,7 @@ pub const JSValue = enum(i64) {
             }
         }
 
-        return switch (JSC__JSValue__getIfPropertyExistsImpl(this, global, property.ptr, @intCast(property.len))) {
-            .undefined, .zero, .property_does_not_exist_on_object => null,
-            else => |val| val,
-        };
+        return JSC__JSValue__getIfPropertyExistsImpl(this, global, property.ptr, @intCast(property.len)).deprecatedUnwrapHidingException();
     }
 
     /// Equivalent to `target[property]`. Calls userland getters/proxies.  Can
@@ -5401,16 +5426,12 @@ pub const JSValue = enum(i64) {
 
         // This call requires `get` to be `inline`
         if (bun.isComptimeKnown(property_slice)) {
-            if (comptime BuiltinName.get(property_slice)) |builtin_name| {
-                return target.fastGetWithError(global, builtin_name);
+            if (comptime BuiltinName.get(property_slice)) |builtin| {
+                return JSC__JSValue__fastGet(target, global, @intFromEnum(builtin)).unwrap(global);
             }
         }
 
-        return switch (JSC__JSValue__getIfPropertyExistsImpl(target, global, property_slice.ptr, @intCast(property_slice.len))) {
-            .zero => error.JSError,
-            .undefined, .property_does_not_exist_on_object => null,
-            else => |val| val,
-        };
+        return JSC__JSValue__getIfPropertyExistsImpl(target, global, property_slice.ptr, @intCast(property_slice.len)).unwrap(global);
     }
 
     extern fn JSC__JSValue__getOwn(value: JSValue, globalObject: *JSGlobalObject, propertyName: *const bun.String) JSValue;
@@ -7266,3 +7287,23 @@ pub const DeferredError = struct {
         return err;
     }
 };
+
+/// Used to test binding functions directly.
+pub const test_apis = struct {
+    /// One can verify this takes the correct code path by placing a debugger
+    /// breakpoint into the `JSC__JSValue__fastGet` line, and then see that it
+    /// steps into the fastGet code path.
+    pub fn fastGet(global: *JSC.JSGlobalObject, callframe: *JSC.CallFrame) !JSValue {
+        const arg = callframe.argumentsAsArray(1)[0];
+        if (!arg.isObject()) return .undefined; // assertion
+        return try arg.get(global, "headers") orelse JSC.jsNumber(404);
+    }
+    /// One can verify this takes the correct code path by placing a debugger
+    /// breakpoint into the `JSC__JSValue__getIfPropertyExistsImpl` line, and
+    /// then see that it steps into the getter code path.
+    pub fn slowGet(global: *JSC.JSGlobalObject, callframe: *JSC.CallFrame) !JSValue {
+        const arg = callframe.argumentsAsArray(1)[0];
+        if (!arg.isObject()) return .undefined; // assertion
+        return try arg.get(global, "something") orelse JSC.jsNumber(404);
+    }
+};

src/js/internal-for-testing.ts

@@ -149,3 +149,8 @@ export const frameworkRouterInternals = $zig("FrameworkRouter.zig", "JSFramework
     new(opts: any): any;
   };
 };
+
+export const bindingTests = {
+  fastGet: $newZigFunction("bindings.zig", "test_apis.fastGet", 1),
+  slowGet: $newZigFunction("bindings.zig", "test_apis.slowGet", 1),
+}

test/js/bun/bindings.test.ts

@@ -0,0 +1,20 @@
+import { describe, expect, test } from "bun:test";
+import { bindingTests } from 'bun:internal-for-testing';
+
+/// Tests for bindings.zig
+test('JSC.JSValue.get', () => {
+  console.log('get');
+  expect(bindingTests.slowGet({ something: 'hello' })).toBe('hello');
+  expect(bindingTests.slowGet({ })).toBe(404);
+  expect(bindingTests.slowGet({ get something() { return 'hello' } })).toBe('hello');
+  expect(() => bindingTests.slowGet({ get something() { throw 'error'; } })).toThrow('error');
+  console.log('end get');
+});
+test('JSC.JSValue.get (known property fast path)', () => {
+  console.log('get fast path');
+  expect(bindingTests.fastGet({ headers: 'hello' })).toBe('hello');
+  expect(bindingTests.fastGet({ })).toBe(404);
+  expect(bindingTests.fastGet({ get headers() { return 'hello' } })).toBe('hello');
+  expect(() => bindingTests.fastGet({ get headers() { throw 'error'; } })).toThrow('error');
+  console.log('end get fast path');
+});