Conversation
|
Updated 8:21 PM PT - Jul 19th, 2026
❌ @robobun, your commit 4b23b02 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34753That installs a local version of the PR into your bun-34753 --bun |
WalkthroughChangesMatcher exception safety
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
robobun
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/test/expect.test.js`:
- Around line 681-683: Replace the explanatory regression-test comment near
expect.any(Array), toMatchObject, and toHaveProperty with only the relevant
issue URL, preserving the test behavior and removing all other comment text.
- Around line 684-711: Update the child script in the Proxy matcher test so
successful matcher calls remain uncaught, while expected failures are explicitly
asserted with their error messages. Add revoked-Proxy coverage for the
toMatchObject and toHaveProperty paths in addition to the existing toEqual case,
and ensure the assertions validate the expected outcomes rather than swallowing
all exceptions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f66c3c7e-4ee5-4087-b432-c6219444fe95
📒 Files selected for processing (2)
src/jsc/bindings/bindings.cpptest/js/bun/test/expect.test.js
…chObject, toHaveProperty JSC::isArray() declares a throw scope on its Proxy path (isArraySlowInline). Three call sites in bindings.cpp used it inside an if condition and then either entered another throw-scope-declaring call or returned with the outer ThrowScope still unsatisfied, which aborts under the JSC exception-scope validator: matchAsymmetricMatcherAndGetFlags: isArray then hasInstance Bun__deepMatch: two chained isArray calls then getPropertyNames JSC__JSValue__getIfPropertyExistsFromPath: isArray false then return Hoist each isArray() result out of the condition and RETURN_IF_EXCEPTION immediately after.
a8a0538 to
4b23b02
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical RETURN_IF_EXCEPTION insertion after three JSC::isArray() call sites, matching the pattern already used at the Bun__deepEquals sites in the same file.
What was reviewed
- Confirmed each site has a
ThrowScopein scope (throwScopeparam for the first two, localscopefor the third) and the sentinel return values match sibling error paths. Bun__deepMatchnow evaluates bothisArraycalls unconditionally instead of short-circuiting; verified this can't introduce a new throw becausesubsetObj->getPropertyNames()already runs (and throws for a revoked Proxy) before that point.- Test drains stdout/stderr/exited concurrently, spreads
bunEnv, and assertssignalCode: nullso a validator abort can't be masked by the innertry/catch. - Both CodeRabbit findings were responded to by the author and withdrawn.
Extended reasoning...
Overview
Three call sites in src/jsc/bindings/bindings.cpp used JSC::isArray() inline in an if condition without checking for an exception afterward. isArray() declares a throw scope on its Proxy path (isArraySlowInline), so under BUN_JSC_validateExceptionChecks=1 the next throw-scope-declaring call (or scope destruction) aborts. The fix hoists each result into a local and inserts RETURN_IF_EXCEPTION immediately after — the exact same shape already used for the v1Array/v2Array sites in Bun__deepEquals in this file, and the same fix applied to NodeVM.cpp in #34747. A subprocess test in expect.test.js runs each affected matcher shape under the validator flag and asserts clean exit.
Security risks
None. This is test-runner matcher code (expect.any, toMatchObject, toHaveProperty) with no auth, crypto, network, or filesystem exposure. The change adds exception checks; it removes nothing.
Level of scrutiny
Low-to-moderate. The transformation is purely mechanical and pattern-matched against neighboring code. I verified: (1) matchAsymmetricMatcherAndGetFlags receives ThrowScope& throwScope by reference and already returns AsymmetricMatcherResult::FAIL on other error paths; (2) Bun__deepMatch receives ThrowScope& throwScope and returns false on exception at the adjacent getPropertyNames check; (3) JSC__JSValue__getIfPropertyExistsFromPath declares a local ThrowScope scope and returns {} on exception throughout. The one behavioral delta — Bun__deepMatch no longer short-circuits the second isArray — is safe because subsetObj->getPropertyNames at line 1645 would already have thrown for any subsetValue that could make the second isArray throw.
Other factors
The robobun evidence gate shows fail-before/pass-after on both the ASAN debug build and a release build, and the author reports expect.test.js (408 pass), jest-extended.test.js, and deep-equal.test.ts all passing. CodeRabbit raised two findings on the test (comment style and try/catch swallowing outcomes); the author explained the test's scope is the validator abort — not matcher semantics, which are under active change in #32948/#34649 — and both were withdrawn. No prior reviews from me on this PR.
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Build 75937: |
|
Closing: #40068 (3ee9801) added the exception checks after isArray() at these three call sites in bindings.cpp (expect.any(Array), Bun__deepMatch, and the toHaveProperty array path). The test from this PR passes on main, and it fails with the same SIGABRT when those three checks are removed from main. #40068 did not add a test for the Proxy matcher cases, so #40981 carries this PR's test into expect.test.js. |
…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 -->
1 participant
Repro
Under
BUN_JSC_validateExceptionChecks=1on a debug build:Cause
JSC::isArray()declares a throw scope on its Proxy path (isArraySlowInline, for the revoked-Proxy case). Three call sites insrc/jsc/bindings/bindings.cppused it inside anifcondition and then either entered another throw-scope-declaring call, or returned with the scope'sm_needExceptionCheckstill set:matchAsymmetricMatcherAndGetFlags(AsymmetricMatcherConstructorType::Array):isArray()then falls through toconstructorObject->hasInstance().Bun__deepMatch:isArray(globalObject, objValue) && isArray(globalObject, subsetValue)chains two throw-scope calls, thengetPropertyNames().JSC__JSValue__getIfPropertyExistsFromPath(thetoHavePropertypath lookup): whenisArray()returns false for a Proxy over an iterable non-array (Set, Map, generator), the body is skipped and the function returns{}with the outerThrowScopestill unsatisfied.The
Bun__deepEqualssites (v1Array/v2Array) already had the check.Fix
Hoist each
isArray()result out of the condition andRETURN_IF_EXCEPTIONimmediately after it. Same pattern as #34747 for theNodeVM.cppinstances.Release behavior is effectively unchanged for
expect.any(Array)andtoHaveProperty(theisArrayrevocation error already reached the caller via a later check). FortoMatchObjectwith a revoked-Proxy receiver, the thrownTypeErrormessage changes from the generic revocation message toArray.isArray cannot be called on a Proxy that has been revoked, matching the operation that actually failed.Verification
New subprocess test in
test/js/bun/test/expect.test.jsspawns withBUN_JSC_validateExceptionChecks=1and runs every affected matcher shape with transparent and revoked Proxies. On builds without exception-scope verification the option is a no-op and the child exits 0 either way.expect.test.js(408 pass),jest-extended.test.js(57 pass),test/js/node/assert/deep-equal.test.ts(260 pass) all pass.Related
#32948 applies the same
Bun__deepMatchsplit as part of a larger Proxy-transparency change and composes with this one; it does not touch theexpect.any(Array)ortoHavePropertysites.[stamp-90s] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file