From 7ad60385b5ff43d9a6e49e046ba024a5636888ac Mon Sep 17 00:00:00 2001 From: Cristiano Calcagno Date: Fri, 5 Dec 2025 05:36:03 +0100 Subject: [PATCH] Add corner cases in the JS bridge The tests document corner cases in the JS bridge (NaN, infinity, large), so one can assess whether they should be addressed. The .md document gives an overview of the cases. --- package-lock.json | 2 +- skipruntime-ts/tests/BRIDGE_EDGE_CASES.md | 70 +++++++++++ skipruntime-ts/tests/src/tests.ts | 138 ++++++++++++++++++++++ 3 files changed, 209 insertions(+), 1 deletion(-) create mode 100644 skipruntime-ts/tests/BRIDGE_EDGE_CASES.md diff --git a/package-lock.json b/package-lock.json index c977943e8..a3eaf1562 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,5 +1,5 @@ { - "name": "skip", + "name": "work", "lockfileVersion": 3, "requires": true, "packages": { diff --git a/skipruntime-ts/tests/BRIDGE_EDGE_CASES.md b/skipruntime-ts/tests/BRIDGE_EDGE_CASES.md new file mode 100644 index 000000000..cf5eb859a --- /dev/null +++ b/skipruntime-ts/tests/BRIDGE_EDGE_CASES.md @@ -0,0 +1,70 @@ +**JS↔Skip Bridge Edge Cases** + +This document describes edge case tests added to verify the behavior of the JavaScript↔Skip bridge. These tests document current behavior, some of which is problematic. + +## Overview + +The Skip Runtime has two implementations: +- **WASM**: WebAssembly-based (`@skipruntime/wasm`) +- **Native**: Node.js C++ addon (`@skipruntime/native`) + +Both share the same TypeScript interface and JSON serialization layer. The tests below verify that both implementations handle JavaScript edge cases consistently. + +--- + +## Test: `testNaNHandling` + +**What it tests**: How `NaN` (Not a Number) is handled when stored in collections. + +**Current behavior**: NaN passes through unchanged. + +**Why this is problematic**: +- `NaN` is **not a valid JSON value** per RFC 8259 +- `JSON.stringify(NaN)` returns `"null"`, not `"NaN"` +- Passing NaN through could cause issues when data is serialized/deserialized +- Different systems may handle NaN inconsistently + +**Recommendation**: Should throw an error: "Cannot export NaN: not a valid JSON value" + +--- + +## Test: `testInfinityHandling` + +**What it tests**: How `Infinity` and `-Infinity` are handled when stored in collections. + +**Current behavior**: 🔴 **SILENT DATA CORRUPTION** +``` +Infinity → 9223372036854776000 (≈ 2^63) +-Infinity → -9223372036854776000 +``` + +**Why this is problematic**: +- `Infinity` is **not a valid JSON value** per RFC 8259 +- The current code path causes integer overflow: + ```typescript + // In exportJSON(): + if (value === Math.trunc(value)) { // Math.trunc(Infinity) === Infinity → true! + return binding.SKIP_SKJSON_createCJInt(value); // Overflows! + } + ``` +- Users store `Infinity`, get back a huge but finite number - **data corruption** +- No error is thrown, making this very hard to debug + +**Recommendation**: Should throw an error: "Cannot export Infinity: not a valid JSON value" + +--- + +## Test: `testLargeIntegerPrecision` + +**What it tests**: How integers larger than `Number.MAX_SAFE_INTEGER` (2^53 - 1) are handled. + +**Current behavior**: Large integers are preserved through the Skip runtime. + +**Why this could be problematic**: +- JavaScript numbers lose precision beyond 2^53 +- The value `9007199254740993` cannot be exactly represented in JS +- Skip uses 64-bit integers internally, which can represent larger values +- Round-tripping through JS could lose precision for values > 2^53 + +**Note**: This is a fundamental JavaScript limitation, not a Skip bug. Users working with large integers should be aware of this. + diff --git a/skipruntime-ts/tests/src/tests.ts b/skipruntime-ts/tests/src/tests.ts index f797c8513..e14be0360 100644 --- a/skipruntime-ts/tests/src/tests.ts +++ b/skipruntime-ts/tests/src/tests.ts @@ -579,6 +579,32 @@ const booleanRoundtripService: SkipService = { }, }; +//// Edge case tests for JS↔Skip bridge + +// Identity mapper that preserves JSON values through roundtrip +class IdentityMapper implements Mapper { + mapEntry(key: Json, values: Values): Iterable<[Json, Json]> { + return values.toArray().map((v) => [key, v]); + } +} + +type Input_JJ = { input: EagerCollection }; + +class IdentityResource implements Resource { + instantiate(cs: Input_JJ): EagerCollection { + return cs.input.map(IdentityMapper); + } +} + +const identityService: SkipService = { + initialData: { input: [] }, + resources: { identity: IdentityResource }, + + createGraph(inputCollections: Input_JJ) { + return inputCollections; + }, +}; + //// testExternalService async function timeout(ms: number) { @@ -1697,6 +1723,118 @@ export function initTests( await service.close(); }); + // Edge case tests for JS↔Skip bridge + // These tests verify correct handling of JavaScript edge cases + // See BRIDGE_EDGE_CASES.md for documentation of what each test verifies + + it("testNaNHandling", async () => { + // NaN is not a valid JSON value - ideally should be rejected + // Currently it passes through as NaN (WASM) which is questionable + const service = await initService(identityService); + try { + const resource = "identity"; + try { + await service.update("input", [["nan_key", [NaN]]]); + const result = await service.getAll(resource); + // If we get here, NaN was accepted - check what it became + const nanVal = (result.find((e) => e[0] === "nan_key") as Entry< + Json, + Json + >)[1][0]; + console.log("NaN became:", nanVal, "type:", typeof nanVal); + // Document current behavior: + // - WASM: NaN passes through as NaN (questionable - not valid JSON) + // - Native: may differ + // Ideally this should throw an error since NaN is not valid JSON + expect(typeof nanVal).toEqual("number"); + } catch (e) { + // This is the PREFERRED behavior - rejecting NaN + expect(e).toBeA(Error); + } + } finally { + await service.close(); + } + }); + + it("testInfinityHandling", async () => { + // BUG: Infinity is not a valid JSON value but is silently corrupted! + // Math.trunc(Infinity) === Infinity, so it goes to createCJInt which overflows + const service = await initService(identityService); + try { + const resource = "identity"; + try { + await service.update("input", [ + ["pos_inf", [Infinity]], + ["neg_inf", [-Infinity]], + ]); + const result = await service.getAll(resource); + // If we get here, Infinity was accepted - check what it became + const posInf = (result.find((e) => e[0] === "pos_inf") as Entry< + Json, + Json + >)[1][0] as number; + const negInf = (result.find((e) => e[0] === "neg_inf") as Entry< + Json, + Json + >)[1][0] as number; + console.log("Infinity became:", posInf, "type:", typeof posInf); + console.log("-Infinity became:", negInf, "type:", typeof negInf); + // BUG: Currently Infinity silently overflows to a huge number (~2^63) + // This is silent data corruption! Should throw instead. + // The test documents the bug - Infinity should NOT become a finite number + if (Number.isFinite(posInf)) { + // This is the bug - we're getting a corrupted value + console.error( + "BUG: Infinity was silently corrupted to finite number:", + posInf, + ); + } + // For now, just verify we don't crash - the real fix should throw + expect(typeof posInf).toEqual("number"); + expect(typeof negInf).toEqual("number"); + } catch (e) { + // This SHOULD happen - rejecting Infinity is correct behavior + expect(e).toBeA(Error); + } + } finally { + await service.close(); + } + }); + + it("testLargeIntegerPrecision", async () => { + // Numbers larger than Number.MAX_SAFE_INTEGER may lose precision + const service = await initService(identityService); + try { + const resource = "identity"; + const maxSafeInt = Number.MAX_SAFE_INTEGER; // 2^53 - 1 = 9007199254740991 + const beyondSafe = maxSafeInt + 2; // 9007199254740993 - not representable exactly + await service.update("input", [ + ["max_safe", [maxSafeInt]], + ["beyond_safe", [beyondSafe]], + ]); + const result = await service.getAll(resource); + const maxSafeResult = (result.find((e) => e[0] === "max_safe") as Entry< + Json, + Json + >)[1][0] as number; + const beyondSafeResult = ( + result.find((e) => e[0] === "beyond_safe") as Entry + )[1][0] as number; + // MAX_SAFE_INTEGER should be preserved exactly + expect(maxSafeResult).toEqual(maxSafeInt); + // Beyond safe integer - document the behavior (may lose precision) + console.log( + "Beyond safe integer:", + beyondSafe, + "became:", + beyondSafeResult, + ); + // This test documents the limitation - ideally we'd validate or warn + } finally { + await service.close(); + } + }); + it("testExternal", async () => { const serviceDef = testExternalService(); const mockExternal = serviceDef.externalServices![