From fa5a5bbe556a4bda5bde77b4013aa6c3bb4ec9ab Mon Sep 17 00:00:00 2001 From: 190n Date: Wed, 17 Dec 2025 00:52:16 -0800 Subject: [PATCH] fix: v8::Value::IsInt32()/IsUint32() edge cases (#25548) ### What does this PR do? - fixes both functions returning false for double-encoded values (even if the numeric value is a valid int32/uint32) - fixes IsUint32() returning false for values that don't fit in int32 - fixes the test from #22462 not testing anything (the native functions were being passed a callback to run garbage collection as the first argument, so it was only ever testing what the type check APIs returned for that function) - extends the test to cover the first edge case above ### How did you verify your code works? The new tests fail without these fixes. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> --- src/bun.js/bindings/v8/V8Value.cpp | 4 +- test/v8/v8-module/main.cpp | 6 +- test/v8/v8-module/main.js | 17 +--- test/v8/v8.test.ts | 124 +++++++++++++++-------------- 4 files changed, 72 insertions(+), 79 deletions(-) diff --git a/src/bun.js/bindings/v8/V8Value.cpp b/src/bun.js/bindings/v8/V8Value.cpp index 223c45cb00..469f86ca44 100644 --- a/src/bun.js/bindings/v8/V8Value.cpp +++ b/src/bun.js/bindings/v8/V8Value.cpp @@ -26,7 +26,7 @@ bool Value::IsNumber() const bool Value::IsUint32() const { - return localToJSValue().isUInt32(); + return localToJSValue().isUInt32AsAnyInt(); } bool Value::IsUndefined() const @@ -81,7 +81,7 @@ bool Value::IsArray() const bool Value::IsInt32() const { - return localToJSValue().isInt32(); + return localToJSValue().isInt32AsAnyInt(); } bool Value::IsBigInt() const diff --git a/test/v8/v8-module/main.cpp b/test/v8/v8-module/main.cpp index 6262d6f63f..9d7483f731 100644 --- a/test/v8/v8-module/main.cpp +++ b/test/v8/v8-module/main.cpp @@ -1120,13 +1120,13 @@ void test_v8_value_type_checks(const FunctionCallbackInfo &info) { } Local value = info[0]; - + // Test all type checks printf("IsMap: %s\n", value->IsMap() ? "true" : "false"); printf("IsArray: %s\n", value->IsArray() ? "true" : "false"); printf("IsInt32: %s\n", value->IsInt32() ? "true" : "false"); printf("IsBigInt: %s\n", value->IsBigInt() ? "true" : "false"); - + // Also test some existing checks for comparison printf("IsNumber: %s\n", value->IsNumber() ? "true" : "false"); printf("IsUint32: %s\n", value->IsUint32() ? "true" : "false"); @@ -1134,7 +1134,7 @@ void test_v8_value_type_checks(const FunctionCallbackInfo &info) { printf("IsBoolean: %s\n", value->IsBoolean() ? "true" : "false"); printf("IsString: %s\n", value->IsString() ? "true" : "false"); printf("IsFunction: %s\n", value->IsFunction() ? "true" : "false"); - + return ok(info); } diff --git a/test/v8/v8-module/main.js b/test/v8/v8-module/main.js index 6bd8224ae5..bb8e7f8ddb 100644 --- a/test/v8/v8-module/main.js +++ b/test/v8/v8-module/main.js @@ -5,28 +5,15 @@ const buildMode = process.argv[5]; const tests = require("./module")(buildMode === "debug"); -// Custom JSON reviver to handle BigInt -function parseArgs(str) { - return JSON.parse(str, (_, value) => - value && typeof value === "object" && "__bigint__" in value ? BigInt(value.__bigint__) : value, - ); -} - const testName = process.argv[2]; -const args = parseArgs(process.argv[3] ?? "[]"); +const args = eval(process.argv[3] ?? "[]"); const thisValue = JSON.parse(process.argv[4] ?? "null"); -function runGC() { - if (typeof Bun !== "undefined") { - Bun.gc(true); - } -} - const fn = tests[testName]; if (typeof fn !== "function") { throw new Error("Unknown test:", testName); } -const result = fn.apply(thisValue, [runGC, ...args]); +const result = fn.apply(thisValue, [...args]); if (result) { console.log(result == global); throw new Error(result); diff --git a/test/v8/v8.test.ts b/test/v8/v8.test.ts index 4ad9b72a28..ca183047c7 100644 --- a/test/v8/v8.test.ts +++ b/test/v8/v8.test.ts @@ -1,4 +1,5 @@ import { spawn } from "bun"; +import { jscDescribe } from "bun:jsc"; import { beforeAll, describe, expect, it } from "bun:test"; import { bunEnv, bunExe, isASAN, isBroken, isMusl, isWindows, nodeExe, tmpdirSync } from "harness"; import assert from "node:assert"; @@ -124,135 +125,141 @@ describe.todoIf(isBroken && isMusl)("node:v8", () => { describe("module lifecycle", () => { it("can call a basic native function", async () => { - await checkSameOutput("test_v8_native_call", []); + await checkSameOutput("test_v8_native_call"); }); }); describe("primitives", () => { it("can create and distinguish between null, undefined, true, and false", async () => { - await checkSameOutput("test_v8_primitives", []); + await checkSameOutput("test_v8_primitives"); }); }); describe("Value type checks", () => { - it("matches Node for IsMap/IsArray/IsInt32/IsBigInt on representative values", async () => { - // Each entry is an argument list; we wrap each value so the addon receives it as info[0]. - const cases: any[][] = [ - [new Map()], - [[]], - [42], - [2147483647], // INT32_MAX - [2147483648], // INT32_MAX + 1 (should not be Int32) - [-2147483648], // INT32_MIN - [-2147483649], // INT32_MIN - 1 (should not be Int32) - [123n], - [3.14], - ["string"], - [{}], - ]; - for (const args of cases) { - await checkSameOutput("test_v8_value_type_checks", args); - } + it("Math.fround returns a double-encoded value", () => { + // If this fails, you need to find a new way to make a JSValue which uses the double encoding + // but holds an int32 value (maybe Float64Array?) + expect(jscDescribe(Math.fround(1))).toBe("Double: 4607182418800017408, 1.000000"); + }); + + it.each([ + // Each entry should eval() to an array of arguments + "[new Map()]", + "[[]]", + "[42]", + "[2 ** 31 - 1]", // INT32_MAX + "[2 ** 31]", // INT32_MAX + 1 (should not be Int32) + "[-(2 ** 31)]", // INT32_MIN + "[-(2 ** 31) - 1]", // INT32_MIN - 1 (should not be Int32) + "[2 ** 32 - 1]", // UINT32_MAX + "[2 ** 32]", // UINT32_MAX + 1 + "[Math.fround(1)]", // Value represented as a double but whose numeric value fits in the int32 range (should be int32) + "[123n]", + "[3.14]", + "['string']", + "[{}]", + ])("matches Node for IsMap/IsArray/IsInt32/IsBigInt on %s", async args => { + await checkSameOutput("test_v8_value_type_checks", args); }); }); describe("Number", () => { it("can create small integer", async () => { - await checkSameOutput("test_v8_number_int", []); + await checkSameOutput("test_v8_number_int"); }); // non-i32 v8::Number is not implemented yet it("can create large integer", async () => { - await checkSameOutput("test_v8_number_large_int", []); + await checkSameOutput("test_v8_number_large_int"); }); it("can create fraction", async () => { - await checkSameOutput("test_v8_number_fraction", []); + await checkSameOutput("test_v8_number_fraction"); }); }); describe("String", () => { it("can create and read back strings with only ASCII characters", async () => { - await checkSameOutput("test_v8_string_ascii", []); + await checkSameOutput("test_v8_string_ascii"); }); // non-ASCII strings are not implemented yet it("can create and read back strings with UTF-8 characters", async () => { - await checkSameOutput("test_v8_string_utf8", []); + await checkSameOutput("test_v8_string_utf8"); }); it("handles replacement correctly in strings with invalid UTF-8 sequences", async () => { - await checkSameOutput("test_v8_string_invalid_utf8", []); + await checkSameOutput("test_v8_string_invalid_utf8"); }); it("can create strings from null-terminated Latin-1 data", async () => { - await checkSameOutput("test_v8_string_latin1", []); + await checkSameOutput("test_v8_string_latin1"); }); describe("WriteUtf8", () => { it("truncates the string correctly", async () => { - await checkSameOutput("test_v8_string_write_utf8", []); + await checkSameOutput("test_v8_string_write_utf8"); }); }); }); describe("External", () => { it("can create an external and read back the correct value", async () => { - await checkSameOutput("test_v8_external", []); + await checkSameOutput("test_v8_external"); }); }); describe("Value", () => { it("can compare values using StrictEquals", async () => { - await checkSameOutput("test_v8_strict_equals", []); + await checkSameOutput("test_v8_strict_equals"); }); }); describe("Object", () => { it("can create an object and set properties", async () => { - await checkSameOutput("test_v8_object", []); + await checkSameOutput("test_v8_object"); }); it("can get properties by key using Object::Get(context, key)", async () => { - await checkSameOutput("test_v8_object_get_by_key", []); + await checkSameOutput("test_v8_object_get_by_key"); }); it("can get array elements by index using Object::Get(context, index)", async () => { - await checkSameOutput("test_v8_object_get_by_index", []); + await checkSameOutput("test_v8_object_get_by_index"); }); it("correctly handles exceptions from get and set", async () => { - await checkSameOutput("test_v8_object_get_set_exceptions", []); + await checkSameOutput("test_v8_object_get_set_exceptions"); }); }); describe("Array", () => { it("can create an array from a C array of Locals", async () => { - await checkSameOutput("test_v8_array_new", []); + await checkSameOutput("test_v8_array_new"); }); it("can create an array with a specific length", async () => { - await checkSameOutput("test_v8_array_new_with_length", []); + await checkSameOutput("test_v8_array_new_with_length"); }); it("can create an array from a callback", async () => { - await checkSameOutput("test_v8_array_new_with_callback", []); + await checkSameOutput("test_v8_array_new_with_callback"); }); it("correctly reports array length", async () => { - await checkSameOutput("test_v8_array_length", []); + await checkSameOutput("test_v8_array_length"); }); it("can iterate over array elements with callbacks", async () => { - await checkSameOutput("test_v8_array_iterate", []); + await checkSameOutput("test_v8_array_iterate"); }); }); describe("ObjectTemplate", () => { it("creates objects with internal fields", async () => { - await checkSameOutput("test_v8_object_template", []); + await checkSameOutput("test_v8_object_template"); }); }); describe("FunctionTemplate", () => { it("keeps the data parameter alive", async () => { - await checkSameOutput("test_v8_function_template", []); + await checkSameOutput("test_v8_function_template"); }); }); describe("Function", () => { it("correctly receives all its arguments from JS", async () => { - await checkSameOutput("print_values_from_js", [5.0, true, null, false, "async meow", {}]); - await checkSameOutput("print_native_function", []); + await checkSameOutput("print_values_from_js", "[5.0, true, null, false, 'async meow', {}]"); + await checkSameOutput("print_native_function"); }); it("correctly receives the this value from JS", async () => { - await checkSameOutput("call_function_with_weird_this_values", []); + await checkSameOutput("call_function_with_weird_this_values"); }); }); @@ -272,20 +279,20 @@ describe.todoIf(isBroken && isMusl)("node:v8", () => { describe("Global", () => { it("can create, modify, and read the value from global handles", async () => { - await checkSameOutput("test_v8_global", []); + await checkSameOutput("test_v8_global"); }); }); describe("HandleScope", () => { it("can hold a lot of locals", async () => { - await checkSameOutput("test_many_v8_locals", []); + await checkSameOutput("test_many_v8_locals"); }); // Skip on ASAN: false positives due to dynamic library boundary crossing where // Bun is built with ASAN+UBSAN but the native addon is not it.skipIf(isASAN)( "keeps GC objects alive", async () => { - await checkSameOutput("test_handle_scope_gc", []); + await checkSameOutput("test_handle_scope_gc"); }, 10000, ); @@ -293,30 +300,30 @@ describe.todoIf(isBroken && isMusl)("node:v8", () => { describe("EscapableHandleScope", () => { it("keeps handles alive in the outer scope", async () => { - await checkSameOutput("test_v8_escapable_handle_scope", []); + await checkSameOutput("test_v8_escapable_handle_scope"); }); }); describe("MaybeLocal", () => { it("correctly handles ToLocal and ToLocalChecked operations", async () => { - await checkSameOutput("test_v8_maybe_local", []); + await checkSameOutput("test_v8_maybe_local"); }); }); describe("uv_os_getpid", () => { it.skipIf(isWindows)("returns the same result as getpid on POSIX", async () => { - await checkSameOutput("test_uv_os_getpid", []); + await checkSameOutput("test_uv_os_getpid"); }); }); describe("uv_os_getppid", () => { it.skipIf(isWindows)("returns the same result as getppid on POSIX", async () => { - await checkSameOutput("test_uv_os_getppid", []); + await checkSameOutput("test_uv_os_getppid"); }); }); }); -async function checkSameOutput(testName: string, args: any[], thisValue?: any) { +async function checkSameOutput(testName: string, args?: string, thisValue?: any) { const [nodeResultResolution, bunReleaseResultResolution, bunDebugResultResolution] = await Promise.allSettled([ runOn(Runtime.node, BuildMode.release, testName, args, thisValue), runOn(Runtime.bun, BuildMode.release, testName, args, thisValue), @@ -344,12 +351,11 @@ async function checkSameOutput(testName: string, args: any[], thisValue?: any) { return nodeResult; } -// Custom JSON serialization that handles BigInt -function serializeArgs(args: any[]): string { - return JSON.stringify(args, (_, value) => (typeof value === "bigint" ? { __bigint__: value.toString() } : value)); -} - -async function runOn(runtime: Runtime, buildMode: BuildMode, testName: string, jsArgs: any[], thisValue?: any) { +/** + * @param jsArgs should eval() to an array + * @param thisValue will be JSON stringified + */ +async function runOn(runtime: Runtime, buildMode: BuildMode, testName: string, jsArgs?: string, thisValue?: any) { if (runtime == Runtime.node) { assert(buildMode == BuildMode.release); } @@ -366,7 +372,7 @@ async function runOn(runtime: Runtime, buildMode: BuildMode, testName: string, j ...(runtime == Runtime.bun ? ["--smol"] : []), join(baseDir, "main.js"), testName, - serializeArgs(jsArgs), + jsArgs ?? "[]", JSON.stringify(thisValue ?? null), ]; if (buildMode == BuildMode.debug) {