Add missing exception checks in expect matchers, mock functions and error construction - #40068
Conversation
…rror construction - matchAsymmetricMatcher: check after isArray (expect.any(Array)), after both getIndex calls in expect.arrayContaining, after toString/RegExp::match in expect.stringMatching, after toInt32 in expect.closeTo (and read the two already-validated numbers with asNumber). - Bun__deepMatch / getIfPropertyExistsFromPath: check after isArray before using its result (revoked Proxy). - mockResolvedValue[Once]: check after JSPromise::resolvedPromise (reads the value's `constructor`) instead of storing a null implementation. - mockRestore / jest.restoreAllMocks: clearSpy's put-back can throw (module-namespace export, indexed slot); give it a scope and propagate. - createInvalidThisError → throwInvalidThisError(scope, …): describing the receiver reads `constructor.name`; check after it instead of handing a pending exception to createError (whose catch scope then adopted the thrown value as the error). All callers (Mock, BunRequest, MIMEType/MIMEParams, node:sqlite) switched. - $ERR_OUT_OF_RANGE: check after building the message before createError. - test runner DiffFormatter: format both values in a fallible constructor (JestPrettyFormat runs getters/Proxy traps) rather than inside Display::fmt with the result dropped; snapshot matchers propagate the pretty-format error instead of throwing a second one over it.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 40 minutes Limit details: You’ve used the included review currently available. Your 61 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 29 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (25)
Comment |
|
Updated 12:13 PM PT - Aug 22nd, 2026
✅ @dylan-conway, your commit e8ac715313feeb678323a73588ec78dc46916f55 passed in 🧪 To try this PR locally: bunx bun-pr 40068That installs a local version of the PR into your bun-40068 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked: the otherProp.asNumber() change in expect.closeTo is guarded by if (!otherProp.isNumber()) at bindings.cpp:560 (the comment about construction-time validation only covers expectedValue, not otherProp); the removed JSMock__resetSpies extern symbol has no remaining callers; and the surviving createInvalidThisError callers in JSDOMExceptionHandling.cpp / JSDOMPromiseDeferred.cpp all use the string-message overload, not the removed JSValue one.
Extended reasoning...
This PR touches 24 files across native JSC bindings and the Rust test runner, refactoring exception-check discipline and the createInvalidThisError → throwInvalidThisError API. Two nits were surfaced as inline comments. I verified a few additional concerns that looked risky in the diff but turned out safe: the toNumber → asNumber change on otherProp (guarded by an isNumber() check just above, not by construction as the comment might suggest for that particular value), the removed extern C symbol has no remaining Rust/C++ callers, and the remaining createInvalidThisError call sites use the still-declared string overload. Given the scope (native exception handling across many bindings, a public-shape refactor of DiffFormatter, and the restoreAllMocks ordering change flagged inline), this warrants a human look.
… the Bun-specific test
| received_ptr: *const core::ffi::c_char, | ||
| received_len: usize, | ||
| global_this: &JSGlobalObject, | ||
| _global_this: &JSGlobalObject, |
There was a problem hiding this comment.
Deleted in 1cded53 (Rust extern and the C++ caller).
There was a problem hiding this comment.
I re-reviewed after eabeb4b and didn't find any bugs — both earlier nits are addressed (the activeSpies set is now cleared only after the restore loop completes, and the new mockResolvedValue test is guarded by if (isBun)). Given this spans 24 files of JSC exception-check plumbing plus the DiffFormatter eager-format refactor, a human pass is still worthwhile.
What was reviewed:
expect.closeTo's switch tootherProp.asNumber()— guarded by the!otherProp.isNumber()early-return above it.- The removed
extern "C" JSMock__resetSpies— no remaining callers in Rust or C++. clearSpynow nullsspyTarget/spyIdentifierbefore the fallible put, so a retry after a mid-loop throw no-ops on already-cleared spies.- Every
DiffFormattercall site: received/expected argument order matches the old struct-literal field assignments.
Extended reasoning...
Overview
Second batch of an exception-check audit: adds RETURN_IF_EXCEPTION after fallible JSC operations in bun:test matchers, mock functions, and error construction. The C++ side (bindings.cpp, JSMockFunction.cpp, ErrorCode.cpp, NodeSqlite.cpp, JSMIMEType/Params.cpp, JSBunRequest.cpp) inserts checks after isArray, getIndex, toString, match, toInt32, resolvedPromise, overrideExportValue, and putDirectIndex, and reworks createInvalidThisError into throwInvalidThisError(scope, …) so a throwing constructor.name getter propagates rather than being adopted as the ERR_INVALID_THIS body. The Rust side refactors DiffFormatter so JestPrettyFormat (which runs user getters/Proxy traps) executes in a fallible new() constructor instead of inside Display::fmt, and updates 12 matcher call sites plus snapshot.rs/expect.rs accordingly.
Since the last review
My earlier run flagged two nits, both fixed in eabeb4b and confirmed in the current diff: (1) restoreAllMocks now clears activeSpies after the loop, so a mid-loop throw leaves un-restored spies tracked for a retry; and (2) the new mockResolvedValue throw-propagation test is wrapped in if (isBun) per the file-header convention. Both threads are marked resolved.
Security risks
None. This is exception-propagation hygiene in test-runner and error-message code paths; no auth, crypto, network, or filesystem surface changes.
Level of scrutiny
High. REVIEW.md flags exception checks after JS-entering calls as the most-blocked native-code category, and this PR touches ~20 such sites plus a control-flow refactor (DiffFormatter) and an API signature change (createInvalidThisError → throwInvalidThisError) fanned out across 6 files. Individual edits are mechanical, but the aggregate is large enough that it doesn't fit the "simple/mechanical/obvious" auto-approve bar.
Other factors
I spot-checked the non-obvious edits: otherProp.asNumber() in expect.closeTo is safe because !otherProp.isNumber() returns FAIL a few lines earlier; expectedValue.asNumber() relies on construction-time validation as the comment states. The removed extern "C" JSMock__resetSpies has no remaining callers (grep across the tree). clearSpy now snapshots target/identifier/attributes into locals and nulls the fields before the potentially-throwing put — so a throw leaves the spy already detached, and a subsequent restoreAllMocks retry hits the if (!target) return no-op. The DiffFormatter::new argument order (received, expected) matches every previous struct-literal at the converted call sites. The PR description says the affected suites pass under BUN_JSC_validateExceptionChecks=1.
Findings of scripts/jsc-exception-lint over src/jsc/bindings and src/jsc/modules: a RETURN_IF_EXCEPTION after each call that can enter JS (get, toString, toWTFString, getIndex, putDirectIndex, JSString value and view, JSC::call, JSON parsing, iterator steps) where the code went on to another JSC operation or returned from its ThrowScope without one, RELEASE_AND_RETURN for tail calls that can throw, asNumber()/asInt32()/JSC::toUInt32(asNumber()) after a type check in place of the throwing coercions, and one nested DECLARE_THROW_SCOPE removed from Bun.deepEquals. No behavior changes beyond stopping at the first pending exception. Not touched: napi.cpp, v8/, JSMockFunction.cpp, ErrorCode.cpp, JSMIMEParams.cpp, JSMIMEType.cpp and the asymmetric matcher, deepMatch, getIfPropertyExistsFromPath and dlopen functions, which #40068 and
…40410) ### Problem - Native code must check for a pending JS exception before the next JS-observable call and before it returns from a `ThrowScope`. A missing check asserts on debug builds (`ERROR: Unchecked JS exception` from `VM::verifyExceptionCheckNeedIsSatisfied`). On release builds the code runs on a dummy result, a later unrelated check misattributes the error, or a second throw overwrites it. - Coverage was dynamic only (`BUN_JSC_validateExceptionChecks=1` on the ASAN lane), so a path no test executes was never checked. ### Fix - `scripts/jsc-exception-lint`: a clang LibTooling checker that models the validator's state machine over the CFG of every function in `src/**/*.cpp`. Callees are classified from visible bodies, then from summary passes over the JavaScriptCore sources and Bun's bindings, then from the `JSGlobalObject*` / `ThrowScope&` convention. `run.ts` drives it; `rust-externs.ts` cross-checks hand-declared Rust externs. - Fixes its findings: `RETURN_IF_EXCEPTION` after the throwing call, `RELEASE_AND_RETURN` for tail calls, `asNumber()`/`asInt32()` after a type check instead of the throwing coercion, one nested `DECLARE_THROW_SCOPE` removed. Rust externs whose C++ body throws go through the scope helpers and return `JsResult`. No termination special cases, no `clearException`. - Not touched: the files and functions #40068 and #40249 cover (napi, v8, JSMockFunction, ErrorCode, MIME, asymmetric matchers). The JSC-side sites are in oven-sh/WebKit#514; their skip-list entries stay until that bump. - Verified: `test/js/bun/jsc/exception-checks.test.ts` (new; each snippet aborted the validator before), the affected suites under the validator (sqlite, process, ffi, headers, streams, workers, vm, buffer, crypto), and `bun run rust:check` for linux, windows and macOS. ### Background - The validator: every `ThrowScope` destructor sets `VM::m_needExceptionCheck`. The next `ThrowScope` constructor or non-released destructor asserts if it is still set. Only `exception()` (what `RETURN_IF_EXCEPTION` expands to), `clearException()` and `assertNoException` clear it. The tool reports the states in which those asserts fire, plus a call made after the function already threw. - A summary is a callee's exit state set: clean, check pending, thrown, or conditional thrower (the caller tests the return value; reported only with `--kind maybe-thrown-call`). Each pass resolves one more level of cross-file calls, so JSC gets three passes (cached per WebKit version). - Rust-implemented `extern "C"` functions run under their own scope and signal a throw with a sentinel, so the C++ side sees them as conditional throwers. <details><summary>Notes</summary> Numbers. First pass over `src/jsc/bindings` with only the signature convention: 2194 findings. With JSC and Bun summaries, the TopExceptionScope model, template-aware carrier detection, and the Rust-extern rule: 667 findings in 103 files (480 pending-call, 164 unchecked-exit, 13 nested scope, 10 call-after-throw). 603 were in scope for this PR after excluding the files above. After the fixes: 113 in 21 files, of which 72 are in the excluded files and the rest were reviewed as false positives (generated `JSSink` and `ZigGeneratedClasses` code, `JSSetIterator::next` in `Keys` mode, `JSFunction::name`, `getCalculatedDisplayName`, `rejectWithCaughtException` right after a throw, global object construction). Those need entries in `nothrow.txt` or the summary pass over `build/*/codegen` the driver now does. What the tool does not see: a throwing call whose result is passed straight into another call and then `RELEASE_AND_RETURN` (the JSMockFunction pattern #40068 fixes) is legal for the validator and not reported. That needs value tracking. Exceptions observed only through a return value (`if (!result) return {}` after a helper that throws into the caller's scope) are reported as `maybe-thrown-call` and hidden by default. Running it: `bun scripts/jsc-exception-lint/run.ts` needs a configured debug build (`build/debug/compile_commands.json`) and the LLVM 21 development package (`libclang-cpp`, headers; CI's `llvm.sh 21 all` installs them). The first run parses the JSC sources three times (about 30 minutes, cached in `build/debug/jsc-exception-lint/`); later runs take about 15 minutes. A CI step on the linux debug lane after the C++ build is the natural next step; this PR does not add it. How the fixes were made: the findings were split by file and fixed in parallel under one written rule set (`RETURN_IF_EXCEPTION` only, no termination special cases, report false positives instead of editing), then the tree was rebuilt, re-analyzed, and a second pass handled the remainder. I reverted two of the resulting hunks by hand (a scope added to `rsisDetachNativeTransform`, a no-op branch in `JSCTaskScheduler.cpp`) because they rested on a stale classification of callees that cannot throw. Dynamic runs: `test/js/bun/util/BunObject.test.ts` and `test/js/bun/jsonl/jsonl-parse.test.ts` still abort under the validator on the JSC-side sites (oven-sh/WebKit#514). `test/js/node/test/parallel/test-repl-inspect-defaults.js` is the JSONP `doGet` case in the same PR. Tests that failed in my container (worker message flood, ffi FTL warm-up, node:util parseArgs stress, stdin fixtures, IPv6 fetch, root-permission checks) fail identically on an unmodified main build there. </details> <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 0 · 115 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 1 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/bun/jsc/exception-checks.test.ts bun test v1.4.1 (4448a2e) test/js/bun/jsc/exception-checks.test.ts: (pass) process.exitCode assigned a rope string [272.39ms] (pass) process.kill with an unknown rope signal name [268.19ms] (pass) process.umask with a rope string [340.13ms] 42 | // them in the comparison so a failure names the call site. 43 | const unchecked = stderr 44 | .split("\n") 45 | .map(line => line.trim()) 46 | .filter(line => line.startsWith("This scope can throw") || line.startsWith("But the exception was unchecked")); 47 | expect({ stdout: stdout.trim(), unchecked, exitCode }).toEqual({ stdout: expected, unchecked: [], exitCode: 0 }); ^ error: expect(received).toEqual(expected) { - "exitCode": 0, - "stdout": "TypeError: Expected 2 values to compare", - "unchecked": [], + "exitCode": 134, + "stdout": "", + "unchecked": [ + "This scope can throw a JS exception: functionBunDeepEquals @ ../../src/jsc/bindin ... (truncated) release without fix: all passed bun test v1.4.1-canary.1 (a95369a) test/js/bun/jsc/exception-checks.test.ts: (pass) process.umask with a rope string [4.80ms] (pass) Bun.deepEquals with one argument [5.74ms] (pass) process.exitCode assigned a rope string [4.57ms] (pass) process.kill with an unknown rope signal name [4.33ms] 4 pass 0 fail 4 expect() calls Ran 4 tests across 1 file. [91.00ms] __F:0:S:0 ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/bun/jsc/exception-checks.test.ts bun test v1.4.1 (4448a2e) test/js/bun/jsc/exception-checks.test.ts: (pass) Bun.deepEquals with one argument [313.84ms] (pass) process.umask with a rope string [277.49ms] (pass) process.exitCode assigned a rope string [276.65ms] (pass) process.kill with an unknown rope signal name [272.38ms] 4 pass 0 fail 4 expect() calls Ran 4 tests across 1 file. [2.32s] __F:0:S:0 release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 628ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/60] gen ProcessBindingHTTPParser.lut.h Generating /workspace/bun/build/release/codegen/ProcessBindingHTTPParser.lut.h from /workspace/bun/src/jsc/bindings/ProcessBindingHTTPParser.cpp [2/60] gen JSBuffer.lut.h Generating /workspace/bun/build/release/codegen/JSBuffer.lut.h from /workspace/bun/src/jsc/bindings/JSBuffer.cpp [3/60] gen cpp.rs (cppbind) [4/60] gen BunProcess.lut.h Generating /workspace/bun/build/release/codegen/BunProcess.lut.h from /workspace/bun/src/jsc/bindings/BunProcess.cpp [5/60] gen generated_host_exports.rs generated_host_exports.rs: 120 exports (host=5, lazy=10, generic=105, rust=0); 242 extern-C blocks audited [6/60] gen BunObject.lut.h Generating /workspace/bun/build/release/codegen/BunObject.lut.h from /workspace/bun/src/jsc/bindings/BunObject.cpp [7/60] gen JS modules (bundle-modules) Preprocess modules (7492ms) Bundle modules (48ms) Postprocesss modules (96ms) Bundle Functions (491ms) Generate Code (32ms) [8.17s] Bundled "src/js" for production 2594 kb 197 internal modules ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` scripts/jsc-exception-lint/README.md | 83 ++ scripts/jsc-exception-lint/jsc-exception-lint.cpp | 1184 ++++++++++++++++++++ scripts/jsc-exception-lint/nothrow.txt | 112 ++ scripts/jsc-exception-lint/run.ts | 450 ++++++++ scripts/jsc-exception-lint/rust-externs.ts | 142 +++ src/http_jsc/headers_jsc.rs | 2 - src/jsc/ConsoleObject.rs | 4 +- src/jsc/FetchHeaders.rs | 18 +- src/jsc/JSGlobalObject.rs | 34 +- src/jsc/JSObject.rs | 18 +- src/jsc/JSUint8Array.rs | 23 +- src/jsc/JSValue.rs | 43 +- src/jsc/VirtualMachine.rs | 7 +- src/jsc/array_buffer.rs | 16 +- src/jsc/bindings/BunDebugger.cpp | 7 +- src/jsc/bindings/BunInjectedScriptHost.cpp | 34 +- src/jsc/bindings/BunObject.cpp | 13 +- src/jsc/bindings/BunPlugin.cpp | 20 +- src/jsc/bindings/BunProcess.cpp | 72 +- src/jsc/bindings/BunProcessReportObjectWindows.cpp | 1 + src/jsc/bindings/BunString.cpp | 2 + src/jsc/bindings/CallSite.cpp | 5 + src/jsc/bindings/CallSitePrototype.cpp | 1 + src/jsc/bindings/ConsoleObject.cpp | 7 +- src/jsc/bindings/ErrorStackTrace.cpp | 39 +- src/jsc/bindings/FormatStackTraceForJS.cpp | 9 +- src/jsc/bindings/HTMLEntryPoint.cpp | 2 +- src/jsc/bindings/ImportMetaObject.cpp | 1 - src/jsc/bindings/InspectorLifecycleAgent.cpp | 1 + src/jsc/bindings/InternalModuleRegistry.cpp | 1 + src/jsc/bindings/JSBuffer.cpp | 12 +- src/jsc/bindings/JSCTestingHelpers.cpp ... (truncated) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests scripts/jsc-exception-lint/README.md 0 1 0 scripts/jsc-exception-lint/jsc-exception-lint.cpp 2 4 0 scripts/jsc-exception-lint/nothrow.txt 0 1 0 scripts/jsc-exception-lint/run.ts 0 1 0 scripts/jsc-exception-lint/rust-externs.ts 0 1 0 src/http_jsc/headers_jsc.rs 0 0 0 src/jsc/ConsoleObject.rs 0 0 0 src/jsc/FetchHeaders.rs 0 0 0 src/jsc/JSGlobalObject.rs 0 0 0 src/jsc/JSObject.rs 0 0 0 src/jsc/JSUint8Array.rs 0 0 0 src/jsc/JSValue.rs 0 0 0 src/jsc/VirtualMachine.rs 0 0 0 src/jsc/array_buffer.rs 0 0 0 src/jsc/bindings/BunDebugger.cpp 0 0 0 src/jsc/bindings/BunInjectedScriptHost.cpp 0 0 0 (+ 99 more files) ``` </details> <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
|
Four open PRs fixed the same two lines in DiffFormatter with one test each: #40883, #40390, #40555 and #39565. I ran each of their test files against main with this branch merged. None aborts. The only difference from their expectations is that this PR propagates the error from the user code, where those PRs made the matcher throw its own error. I closed the four in favor of this PR. Their coverage is in #40919, a test-only PR that targets this branch (one new file, test/js/bun/test/expect-diff-format-throw.test.ts). Merge it into this branch if you want the diff path covered here, or let it retarget to main after this lands. Without this branch, all 10 of its tests abort a debug build with the assertNoExceptionExceptTermination assertion. |
…pect matchers (#40981) ### Problem - #40068 (3ee9801) added exception checks after `JSC::isArray()` at three call sites in `src/jsc/bindings/bindings.cpp`: `expect.any(Array)` (`matchAsymmetricMatcherAndGetFlags`), `toMatchObject` (`Bun__deepMatch`), and `toHaveProperty` with an array path (`JSC__JSValue__getIfPropertyExistsFromPath`). - That PR added a test only for `mockResolvedValue`. Nothing in the tree runs these three matchers with a Proxy under `BUN_JSC_validateExceptionChecks=1`. A future edit can drop one of the checks and no test fails. - Without a check, a debug build aborts with `ERROR: Unchecked JS exception: This scope can throw a JS exception: isArraySlowInline @ JavaScriptCore/runtime/ArrayConstructor.cpp ... ASSERTION FAILED: exception check validation failed`. ### Fix - Test only. It carries the test from #34753 into `test/js/bun/test/expect.test.js`. #34753 fixed the same three sites and is closed as superseded by #40068. - The test spawns a child with `BUN_JSC_validateExceptionChecks=1`. The child runs each matcher with transparent and revoked Proxy values and prints `ok`. The test asserts `stdout`, `exitCode` 0, and no signal. On a release build the option is a no-op and the child exits 0. - Verified: `bun bd test test/js/bun/test/expect.test.js -t isArray` passes on main. With the three checks removed from `bindings.cpp` and rebuilt, the same test fails with `exitCode: 134, signalCode: SIGABRT` and the abort message above. The full file passes (416 pass, 2 todo). ### Background - `JSC::isArray()` follows the `Array.isArray` spec. For a Proxy it walks to the target and throws a `TypeError` if the Proxy is revoked. So it declares a throw scope, and the caller must check for an exception before the next JSC call. - `BUN_JSC_validateExceptionChecks=1` makes a debug JSC assert when a throw scope is left unchecked. It is the tool that finds these sites. Release builds ignore it. - The test lives inside the `if (isBun)` block and reads `harness` with `require` inside the test body. This file also runs under Jest and Vitest, and the existing `test("()")` uses the same pattern for that reason. <details><summary>Notes</summary> Fail-before run on main with the three `RETURN_IF_EXCEPTION` lines after `isArray()` removed from `bindings.cpp`: ``` error: expect(received).toMatchObject(expected) + "exitCode": 134, + "signalCode": "SIGABRT", + "stderr": + "ERROR: Unchecked JS exception: + This scope can throw a JS exception: isArraySlowInline @ vendor/WebKit/Source/JavaScriptCore/runtime/ArrayConstructor.cpp:130 + But the exception was unchecked as of this scope: hasInstance @ vendor/WebKit/Source/JavaScriptCore/runtime/JSObject.cpp:2686 + ASSERTION FAILED: exception check validation failed ``` Pass-after on main as is: `1 pass, 417 filtered out`. The three repro snippets from #34753 also run without the abort on main under `BUN_JSC_validateExceptionChecks=1 BUN_JSC_dumpSimulatedThrows=1`: ```js expect(new Proxy({}, {})).toEqual(expect.any(Array)); expect(new Proxy([], {})).toMatchObject([]); expect({ a: 1 }).toHaveProperty(new Proxy(new Set(["a"]), {})); ``` Each throws the normal matcher failure and the process exits 0. </details> <!-- robobun:evidence:begin --> --- **[stamp-90s]** gate passed · iteration 2 · 1 files touched <details><summary>passes on PR (with fix)</summary> ```console Test-only change. Debug/ASAN (expected pass): $ bun bd test 'test/js/bun/test/expect.test.js' $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test test/js/bun/test/expect.test.js bun test v1.4.1 (d578a8c) test/js/bun/test/expect.test.js: (pass) expect() > () [302.05ms] (pass) expect() > toBe() > expect(0).toBe(0) == true [1.56ms] (pass) expect() > toBe() > expect(0).toBe(0) == true [0.44ms] (pass) expect() > toBe() > expect(0).toBe(0) == true [0.26ms] (pass) expect() > toBe() > expect(-0).toBe(-0) == true [0.23ms] (pass) expect() > toBe() > expect(1).toBe(1) == true [0.23ms] (pass) expect() > toBe() > expect(1).toBe(1) == true [0.24ms] (pass) expect() > toBe() > expect(NaN).toBe(NaN) == true [0.23ms] (pass) expect() > toBe() > expect(Infinity).toBe(Infinity) == true [0.23ms] (pass) expect() > toBe() > expect({}).toBe({}) == true [0.23ms] (pass) expect() > toBe() > expect(Symbol(a)).toBe(Symbol(a)) == true [0.27ms] (pass) expect() > toBe() > expect(0).toBe(false) == false [2.35ms] (pass) expect() > toBe() > expect(0).toBe("") == false [0.50ms] (pass) expect() > toBe() > expect(0).toBe(-0) == false [0.30ms] (pass) expect() > toBe() > expect(0).toBe(-0) == false [0.29ms] (pass) expect() > toBe() > expect(1).toBe(2) == false [0.32ms] (pass) expect() > toBe() > expect(1).toBe(true) == false [0.30ms] (pass) expect() > toBe() > expect(1).toBe("1") == false [0.45ms] (pass) expect() > toBe() > expect(Infinity).toBe(-Infinity) == false [0.31ms] (pass) expect() > toBe() > expect("foo").toBe("Foo") == false [0.30ms] (pass) expect() > toBe() > expect("foo").toBe("bar") == false [0.29ms] (pass) expect() > toBe() > expect("").toBe(" ") == false [0.33ms] (pass) expect() > toBe() > expect("").toBe(" ") == false [0.29ms] (pass) expect() > toBe() > expect("").toBe(true) == false [0.29ms] (pass) expect() > toBe() > expect({}).toBe({}) == false [0.31ms] (pass) expect() > toBe() > expect(Set {}).toBe(Set {}) == false [0.29ms] (pass) expect() > toBe() > expect([Function: a]).toBe([Function: a]) == false [0.31ms] (pass) expect() > toBe() > e ... (truncated) Exit: 0 ``` </details> <details><summary>diff hotspot</summary> ``` test/js/bun/test/expect.test.js | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 2 <details><summary>evidence per changed file</summary> ``` file reads edits tests test/js/bun/test/expect.test.js 1 1 0 ``` </details> <!-- robobun:evidence:end -->
…b5cc2, baseline refresh The JSC cell boilerplate (create, createStructure, finishCreation, createPrototype, createConstructor, getConstructor, prototype, prototypeForStructure, getDOMStructure, getDOMPrototype, getDOMConstructor, initializeProperties, subspaceFor) with a VM& first parameter installs properties and structures and does not run JavaScript. The fallback classification treats it as non-throwing, so the committed summaries no longer carry those rows: bun.tsv goes from 8440 rows to 6477, webkit.tsv from 3250 to 2937. A function of that shape whose body does throw keeps its row. The fallback rules are one function now, used by the classification and by the export's "conventional" column. Both summaries are regenerated for the WebKit bump on main, and their headers say what a row is. The baseline goes from 81 entries to 58: #40068 fixed its entries, the refreshed summaries cleared stale NodeVM rows, and one new finding in NodeVMSourceTextModule::create is listed.
3 participants
What does this PR do?
Second batch from an audit of native code that performs another JSC operation (or returns) while a JS exception may be pending. This one covers
bun:testmatchers/mocks and error construction; every site is reachable from user JS.bindings.cpp): check afterisArrayinexpect.any(Array); after eachgetIndexinexpect.arrayContaining(a throwing index getter previously reachedBun__deepEqualswith the exception pending); aftertoString/RegExp::matchinexpect.stringMatching; aftertoInt32inexpect.closeTo(its two numbers are validated at construction, so they are read withasNumber).Bun__deepMatchandtoHaveProperty(path)check afterisArray(revoked Proxy) before using the result.mockResolvedValue/mockResolvedValueOnce:JSPromise::resolvedPromisereads the value'sconstructorand can throw; that was stored as a null implementation. Now propagates.mockRestore/jest.restoreAllMocks: putting the original back (overrideExportValueon a module namespace,putDirectIndex) can throw;clearSpygets a scope and its callers propagate.createInvalidThisError→throwInvalidThisError(scope, …): describing the receiver readsconstructor.name. The old function handed the pending exception tocreateError, whose catch scope then adopted the thrown value as the ERR_INVALID_THIS error. It now propagates the getter's exception; all callers (Mock, BunRequest, MIMEType/MIMEParams accessors, node:sqlite) are switched.$ERR_OUT_OF_RANGE: check after building the message, same reason.DiffFormatterformatted both values withJestPrettyFormat(runs getters / Proxy traps) insideDisplay::fmtand dropped the result; it now formats in a fallibleDiffFormatter::new(...)?andfmtonly prints. Snapshot matchers propagate the pretty-format error instead of throwing "Failed to pretty format value" over it.How did you verify your code works?
New test for
mockResolvedValue(fails on release).mock-fn,expect,mock-module,node-sqlitesuites pass underBUN_JSC_validateExceptionChecks=1on a debug build. The matcher cases (arrayContainingwith a throwing index getter, revoked-Proxy receivers) already surfaced the right error in release; the missing checks there are what the validator flags.