Skip to content

Commit fa5a5bb

Browse files
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>
1 parent 1e86ceb commit fa5a5bb

File tree

4 files changed

+72
-79
lines changed

4 files changed

+72
-79
lines changed

src/bun.js/bindings/v8/V8Value.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ bool Value::IsNumber() const
2626

2727
bool Value::IsUint32() const
2828
{
29-
return localToJSValue().isUInt32();
29+
return localToJSValue().isUInt32AsAnyInt();
3030
}
3131

3232
bool Value::IsUndefined() const
@@ -81,7 +81,7 @@ bool Value::IsArray() const
8181

8282
bool Value::IsInt32() const
8383
{
84-
return localToJSValue().isInt32();
84+
return localToJSValue().isInt32AsAnyInt();
8585
}
8686

8787
bool Value::IsBigInt() const

test/v8/v8-module/main.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1120,21 +1120,21 @@ void test_v8_value_type_checks(const FunctionCallbackInfo<Value> &info) {
11201120
}
11211121

11221122
Local<Value> value = info[0];
1123-
1123+
11241124
// Test all type checks
11251125
printf("IsMap: %s\n", value->IsMap() ? "true" : "false");
11261126
printf("IsArray: %s\n", value->IsArray() ? "true" : "false");
11271127
printf("IsInt32: %s\n", value->IsInt32() ? "true" : "false");
11281128
printf("IsBigInt: %s\n", value->IsBigInt() ? "true" : "false");
1129-
1129+
11301130
// Also test some existing checks for comparison
11311131
printf("IsNumber: %s\n", value->IsNumber() ? "true" : "false");
11321132
printf("IsUint32: %s\n", value->IsUint32() ? "true" : "false");
11331133
printf("IsObject: %s\n", value->IsObject() ? "true" : "false");
11341134
printf("IsBoolean: %s\n", value->IsBoolean() ? "true" : "false");
11351135
printf("IsString: %s\n", value->IsString() ? "true" : "false");
11361136
printf("IsFunction: %s\n", value->IsFunction() ? "true" : "false");
1137-
1137+
11381138
return ok(info);
11391139
}
11401140

test/v8/v8-module/main.js

Lines changed: 2 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -5,28 +5,15 @@ const buildMode = process.argv[5];
55

66
const tests = require("./module")(buildMode === "debug");
77

8-
// Custom JSON reviver to handle BigInt
9-
function parseArgs(str) {
10-
return JSON.parse(str, (_, value) =>
11-
value && typeof value === "object" && "__bigint__" in value ? BigInt(value.__bigint__) : value,
12-
);
13-
}
14-
158
const testName = process.argv[2];
16-
const args = parseArgs(process.argv[3] ?? "[]");
9+
const args = eval(process.argv[3] ?? "[]");
1710
const thisValue = JSON.parse(process.argv[4] ?? "null");
1811

19-
function runGC() {
20-
if (typeof Bun !== "undefined") {
21-
Bun.gc(true);
22-
}
23-
}
24-
2512
const fn = tests[testName];
2613
if (typeof fn !== "function") {
2714
throw new Error("Unknown test:", testName);
2815
}
29-
const result = fn.apply(thisValue, [runGC, ...args]);
16+
const result = fn.apply(thisValue, [...args]);
3017
if (result) {
3118
console.log(result == global);
3219
throw new Error(result);

test/v8/v8.test.ts

Lines changed: 65 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { spawn } from "bun";
2+
import { jscDescribe } from "bun:jsc";
23
import { beforeAll, describe, expect, it } from "bun:test";
34
import { bunEnv, bunExe, isASAN, isBroken, isMusl, isWindows, nodeExe, tmpdirSync } from "harness";
45
import assert from "node:assert";
@@ -124,135 +125,141 @@ describe.todoIf(isBroken && isMusl)("node:v8", () => {
124125

125126
describe("module lifecycle", () => {
126127
it("can call a basic native function", async () => {
127-
await checkSameOutput("test_v8_native_call", []);
128+
await checkSameOutput("test_v8_native_call");
128129
});
129130
});
130131

131132
describe("primitives", () => {
132133
it("can create and distinguish between null, undefined, true, and false", async () => {
133-
await checkSameOutput("test_v8_primitives", []);
134+
await checkSameOutput("test_v8_primitives");
134135
});
135136
});
136137

137138
describe("Value type checks", () => {
138-
it("matches Node for IsMap/IsArray/IsInt32/IsBigInt on representative values", async () => {
139-
// Each entry is an argument list; we wrap each value so the addon receives it as info[0].
140-
const cases: any[][] = [
141-
[new Map()],
142-
[[]],
143-
[42],
144-
[2147483647], // INT32_MAX
145-
[2147483648], // INT32_MAX + 1 (should not be Int32)
146-
[-2147483648], // INT32_MIN
147-
[-2147483649], // INT32_MIN - 1 (should not be Int32)
148-
[123n],
149-
[3.14],
150-
["string"],
151-
[{}],
152-
];
153-
for (const args of cases) {
154-
await checkSameOutput("test_v8_value_type_checks", args);
155-
}
139+
it("Math.fround returns a double-encoded value", () => {
140+
// If this fails, you need to find a new way to make a JSValue which uses the double encoding
141+
// but holds an int32 value (maybe Float64Array?)
142+
expect(jscDescribe(Math.fround(1))).toBe("Double: 4607182418800017408, 1.000000");
143+
});
144+
145+
it.each([
146+
// Each entry should eval() to an array of arguments
147+
"[new Map()]",
148+
"[[]]",
149+
"[42]",
150+
"[2 ** 31 - 1]", // INT32_MAX
151+
"[2 ** 31]", // INT32_MAX + 1 (should not be Int32)
152+
"[-(2 ** 31)]", // INT32_MIN
153+
"[-(2 ** 31) - 1]", // INT32_MIN - 1 (should not be Int32)
154+
"[2 ** 32 - 1]", // UINT32_MAX
155+
"[2 ** 32]", // UINT32_MAX + 1
156+
"[Math.fround(1)]", // Value represented as a double but whose numeric value fits in the int32 range (should be int32)
157+
"[123n]",
158+
"[3.14]",
159+
"['string']",
160+
"[{}]",
161+
])("matches Node for IsMap/IsArray/IsInt32/IsBigInt on %s", async args => {
162+
await checkSameOutput("test_v8_value_type_checks", args);
156163
});
157164
});
158165
describe("Number", () => {
159166
it("can create small integer", async () => {
160-
await checkSameOutput("test_v8_number_int", []);
167+
await checkSameOutput("test_v8_number_int");
161168
});
162169
// non-i32 v8::Number is not implemented yet
163170
it("can create large integer", async () => {
164-
await checkSameOutput("test_v8_number_large_int", []);
171+
await checkSameOutput("test_v8_number_large_int");
165172
});
166173
it("can create fraction", async () => {
167-
await checkSameOutput("test_v8_number_fraction", []);
174+
await checkSameOutput("test_v8_number_fraction");
168175
});
169176
});
170177

171178
describe("String", () => {
172179
it("can create and read back strings with only ASCII characters", async () => {
173-
await checkSameOutput("test_v8_string_ascii", []);
180+
await checkSameOutput("test_v8_string_ascii");
174181
});
175182
// non-ASCII strings are not implemented yet
176183
it("can create and read back strings with UTF-8 characters", async () => {
177-
await checkSameOutput("test_v8_string_utf8", []);
184+
await checkSameOutput("test_v8_string_utf8");
178185
});
179186
it("handles replacement correctly in strings with invalid UTF-8 sequences", async () => {
180-
await checkSameOutput("test_v8_string_invalid_utf8", []);
187+
await checkSameOutput("test_v8_string_invalid_utf8");
181188
});
182189
it("can create strings from null-terminated Latin-1 data", async () => {
183-
await checkSameOutput("test_v8_string_latin1", []);
190+
await checkSameOutput("test_v8_string_latin1");
184191
});
185192
describe("WriteUtf8", () => {
186193
it("truncates the string correctly", async () => {
187-
await checkSameOutput("test_v8_string_write_utf8", []);
194+
await checkSameOutput("test_v8_string_write_utf8");
188195
});
189196
});
190197
});
191198

192199
describe("External", () => {
193200
it("can create an external and read back the correct value", async () => {
194-
await checkSameOutput("test_v8_external", []);
201+
await checkSameOutput("test_v8_external");
195202
});
196203
});
197204

198205
describe("Value", () => {
199206
it("can compare values using StrictEquals", async () => {
200-
await checkSameOutput("test_v8_strict_equals", []);
207+
await checkSameOutput("test_v8_strict_equals");
201208
});
202209
});
203210

204211
describe("Object", () => {
205212
it("can create an object and set properties", async () => {
206-
await checkSameOutput("test_v8_object", []);
213+
await checkSameOutput("test_v8_object");
207214
});
208215
it("can get properties by key using Object::Get(context, key)", async () => {
209-
await checkSameOutput("test_v8_object_get_by_key", []);
216+
await checkSameOutput("test_v8_object_get_by_key");
210217
});
211218
it("can get array elements by index using Object::Get(context, index)", async () => {
212-
await checkSameOutput("test_v8_object_get_by_index", []);
219+
await checkSameOutput("test_v8_object_get_by_index");
213220
});
214221
it("correctly handles exceptions from get and set", async () => {
215-
await checkSameOutput("test_v8_object_get_set_exceptions", []);
222+
await checkSameOutput("test_v8_object_get_set_exceptions");
216223
});
217224
});
218225
describe("Array", () => {
219226
it("can create an array from a C array of Locals", async () => {
220-
await checkSameOutput("test_v8_array_new", []);
227+
await checkSameOutput("test_v8_array_new");
221228
});
222229
it("can create an array with a specific length", async () => {
223-
await checkSameOutput("test_v8_array_new_with_length", []);
230+
await checkSameOutput("test_v8_array_new_with_length");
224231
});
225232
it("can create an array from a callback", async () => {
226-
await checkSameOutput("test_v8_array_new_with_callback", []);
233+
await checkSameOutput("test_v8_array_new_with_callback");
227234
});
228235
it("correctly reports array length", async () => {
229-
await checkSameOutput("test_v8_array_length", []);
236+
await checkSameOutput("test_v8_array_length");
230237
});
231238
it("can iterate over array elements with callbacks", async () => {
232-
await checkSameOutput("test_v8_array_iterate", []);
239+
await checkSameOutput("test_v8_array_iterate");
233240
});
234241
});
235242

236243
describe("ObjectTemplate", () => {
237244
it("creates objects with internal fields", async () => {
238-
await checkSameOutput("test_v8_object_template", []);
245+
await checkSameOutput("test_v8_object_template");
239246
});
240247
});
241248

242249
describe("FunctionTemplate", () => {
243250
it("keeps the data parameter alive", async () => {
244-
await checkSameOutput("test_v8_function_template", []);
251+
await checkSameOutput("test_v8_function_template");
245252
});
246253
});
247254

248255
describe("Function", () => {
249256
it("correctly receives all its arguments from JS", async () => {
250-
await checkSameOutput("print_values_from_js", [5.0, true, null, false, "async meow", {}]);
251-
await checkSameOutput("print_native_function", []);
257+
await checkSameOutput("print_values_from_js", "[5.0, true, null, false, 'async meow', {}]");
258+
await checkSameOutput("print_native_function");
252259
});
253260

254261
it("correctly receives the this value from JS", async () => {
255-
await checkSameOutput("call_function_with_weird_this_values", []);
262+
await checkSameOutput("call_function_with_weird_this_values");
256263
});
257264
});
258265

@@ -272,51 +279,51 @@ describe.todoIf(isBroken && isMusl)("node:v8", () => {
272279

273280
describe("Global", () => {
274281
it("can create, modify, and read the value from global handles", async () => {
275-
await checkSameOutput("test_v8_global", []);
282+
await checkSameOutput("test_v8_global");
276283
});
277284
});
278285

279286
describe("HandleScope", () => {
280287
it("can hold a lot of locals", async () => {
281-
await checkSameOutput("test_many_v8_locals", []);
288+
await checkSameOutput("test_many_v8_locals");
282289
});
283290
// Skip on ASAN: false positives due to dynamic library boundary crossing where
284291
// Bun is built with ASAN+UBSAN but the native addon is not
285292
it.skipIf(isASAN)(
286293
"keeps GC objects alive",
287294
async () => {
288-
await checkSameOutput("test_handle_scope_gc", []);
295+
await checkSameOutput("test_handle_scope_gc");
289296
},
290297
10000,
291298
);
292299
});
293300

294301
describe("EscapableHandleScope", () => {
295302
it("keeps handles alive in the outer scope", async () => {
296-
await checkSameOutput("test_v8_escapable_handle_scope", []);
303+
await checkSameOutput("test_v8_escapable_handle_scope");
297304
});
298305
});
299306

300307
describe("MaybeLocal", () => {
301308
it("correctly handles ToLocal and ToLocalChecked operations", async () => {
302-
await checkSameOutput("test_v8_maybe_local", []);
309+
await checkSameOutput("test_v8_maybe_local");
303310
});
304311
});
305312

306313
describe("uv_os_getpid", () => {
307314
it.skipIf(isWindows)("returns the same result as getpid on POSIX", async () => {
308-
await checkSameOutput("test_uv_os_getpid", []);
315+
await checkSameOutput("test_uv_os_getpid");
309316
});
310317
});
311318

312319
describe("uv_os_getppid", () => {
313320
it.skipIf(isWindows)("returns the same result as getppid on POSIX", async () => {
314-
await checkSameOutput("test_uv_os_getppid", []);
321+
await checkSameOutput("test_uv_os_getppid");
315322
});
316323
});
317324
});
318325

319-
async function checkSameOutput(testName: string, args: any[], thisValue?: any) {
326+
async function checkSameOutput(testName: string, args?: string, thisValue?: any) {
320327
const [nodeResultResolution, bunReleaseResultResolution, bunDebugResultResolution] = await Promise.allSettled([
321328
runOn(Runtime.node, BuildMode.release, testName, args, thisValue),
322329
runOn(Runtime.bun, BuildMode.release, testName, args, thisValue),
@@ -344,12 +351,11 @@ async function checkSameOutput(testName: string, args: any[], thisValue?: any) {
344351
return nodeResult;
345352
}
346353

347-
// Custom JSON serialization that handles BigInt
348-
function serializeArgs(args: any[]): string {
349-
return JSON.stringify(args, (_, value) => (typeof value === "bigint" ? { __bigint__: value.toString() } : value));
350-
}
351-
352-
async function runOn(runtime: Runtime, buildMode: BuildMode, testName: string, jsArgs: any[], thisValue?: any) {
354+
/**
355+
* @param jsArgs should eval() to an array
356+
* @param thisValue will be JSON stringified
357+
*/
358+
async function runOn(runtime: Runtime, buildMode: BuildMode, testName: string, jsArgs?: string, thisValue?: any) {
353359
if (runtime == Runtime.node) {
354360
assert(buildMode == BuildMode.release);
355361
}
@@ -366,7 +372,7 @@ async function runOn(runtime: Runtime, buildMode: BuildMode, testName: string, j
366372
...(runtime == Runtime.bun ? ["--smol"] : []),
367373
join(baseDir, "main.js"),
368374
testName,
369-
serializeArgs(jsArgs),
375+
jsArgs ?? "[]",
370376
JSON.stringify(thisValue ?? null),
371377
];
372378
if (buildMode == BuildMode.debug) {

0 commit comments

Comments
 (0)