Conversation
…ams by their entries Bun__deepMatch only walked the enumerable properties of the expected value. Headers, URLSearchParams and URL keep their state in C++ and only expose prototype members, which are identical for every instance, so any two instances matched each other. Run specialObjectsDequal (the content comparison toEqual already uses) before the property walk when both sides are wrapper objects of the same kind.
WalkthroughChangesDOM wrapper matching
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: ready for review (head 9ead87e; the two follow-up commits only shorten a code comment). Reproduced on the released binary (1.4.0) and main 9a543cc: Fail-before / pass-after: Related open work, intentionally not folded in: #37881 and #37887 fix the underlying Headers / URLSearchParams comparisons in |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 3:06 PM PT - Aug 12th, 2026
❌ @robobun, your commit 9ead87e has 3 failures in
🧪 To try this PR locally: bunx bun-pr 37904That installs a local version of the PR into your bun-37904 --bun |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/bindings/bindings.cpp (1)
1986-1991: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCompare duplicate
URLSearchParamsentries by multiplicity.
specialObjectsDequalusesget(key), which checks only the first value for each key. Equal sizes do not detect different duplicate counts, soa=1&a=1&b=2anda=1&b=2&b=2compare equal. Compare all entries, such as withgetAll, and add a regression test.🤖 Prompt for 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. In `@src/jsc/bindings/bindings.cpp` around lines 1986 - 1991, Update the URLSearchParams comparison in specialObjectsDequal to compare every value for each key, using multiplicity-aware entry retrieval such as getAll rather than only the first value from get. Preserve asymmetric matching behavior and add a regression test covering equal-size parameters with different duplicate counts, such as a=1&a=1&b=2 versus a=1&b=2&b=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.
Outside diff comments:
In `@src/jsc/bindings/bindings.cpp`:
- Around line 1986-1991: Update the URLSearchParams comparison in
specialObjectsDequal to compare every value for each key, using
multiplicity-aware entry retrieval such as getAll rather than only the first
value from get. Preserve asymmetric matching behavior and add a regression test
covering equal-size parameters with different duplicate counts, such as
a=1&a=1&b=2 versus a=1&b=2&b=2.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 41b82e80-7713-4859-b6dd-53932a0b72be
📒 Files selected for processing (1)
src/jsc/bindings/bindings.cpp
|
On the review note about the URLSearchParams comparison in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and didn't find bugs. It's a small, well-tested fix that reuses specialObjectsDequal the same way Bun__deepEquals does, but since it changes user-visible toMatchObject semantics in the C++ bindings and sits adjacent to #32870's approach for other leaf types, a maintainer sign-off on the shape would be worthwhile.
Checked: the same-JSType gate keeps Blob/Response/Request and mixed pairs on the property walk (specialObjectsDequal returns nullopt for them with isStrict=false); the wrapper branch never returns true, so matching entries fall through to the existing walk as intended. Exception handling (RETURN_IF_EXCEPTION after each call) and the local Vector/MarkedArgumentBuffer lifetimes look correct. The two-direction call is load-bearing for the a=1&a=1 vs a=1&b=1 URLSearchParams case and is covered by a test.
Extended reasoning...
Overview
The PR adds ~12 lines to Bun__deepMatch in src/jsc/bindings/bindings.cpp: when both the received and expected values are wrapper objects of the same JSType (JSDOMWrapperType or JSAsJSONType), it calls specialObjectsDequal in both argument orders before the property walk, mirroring lines 834-839 in Bun__deepEquals. This makes toMatchObject, Bun.deepMatch, expect.objectContaining, and snapshot property matchers compare Headers/URLSearchParams/URL by their entries instead of vacuously matching. Tests are added to expect.test.js (5 new tests covering nested/top-level/array/snapshot/objectContaining variants) and deep-match.spec.ts (10 new it.each rows).
Security risks
None. This is test-matcher comparison logic with no untrusted-input parsing, no allocation sizing from external data, and no auth/crypto surface.
Level of scrutiny
Medium-high. The C++ change itself is small and mechanical — it reuses an existing helper with the same call pattern as Bun__deepEquals, has proper RETURN_IF_EXCEPTION after each fallible call, and the local Vector/MarkedArgumentBuffer are stack-scoped RAII. I traced the JSDOMWrapperType/JSAsJSONType branch of specialObjectsDequal (bindings.cpp:1743-1857): with isStrict=false it only returns false on entry mismatch and nullopt otherwise (including for non-URL/Headers/USP wrappers like Blob/Response), so unknown wrapper types and matching-entry cases correctly fall through to the existing property walk. The same-JSType gate means Headers vs URLSearchParams (different JSType bytes) and Headers vs plain object skip the new block entirely.
However, this is a user-visible behavior change to toMatchObject — a core test-runner primitive — and the PR explicitly chose specialObjectsDequal over full Bun__deepEquals to avoid regressing Blob/Response, while noting that #32870 takes a different approach (full deepEquals) for Date/Error/Set/Map leaves. A maintainer should confirm this shape is the one they want and that it composes with #32870.
Other factors
Test coverage is thorough (nested, top-level, array, both directions of the asymmetric USP repeated-key case, snapshot matchers, objectContaining). The PR description documents fail-before/pass-after against the released binary and Vitest cross-check. The comment-cop bot flagged the code comment twice; both threads are resolved and the comment is now a single line in the latest commit (9ead87e). No prior claude[bot] review exists on this PR.
There was a problem hiding this comment.
I reviewed this and didn't find any bugs. Because it changes user-visible toMatchObject / Bun.deepMatch semantics in bindings.cpp and is coordinated with several open PRs on the same comparison path (#32870, #37881, #37887), a maintainer sign-off would still be worthwhile.
Checked: the new block reuses specialObjectsDequal with the same both-direction call shape as Bun__deepEquals; for the JSDOMWrapperType/JSAsJSONType branch that helper only ever returns false or std::nullopt, so equal-entry pairs and unhandled wrapper classes (Blob, Request, Response) fall through to the existing property walk unchanged. Traced the asymmetric-iteration URLSearchParams cases (a=1&a=1 vs a=1&b=1) through both call orders — the second call catches the direction the first misses. Confirmed expect.any(Headers) as a property value is still handled by the per-property asymmetric-matcher check before recursion, so the new top-level gate does not interfere. RETURN_IF_EXCEPTION is placed after each call. The one CI failure is an unrelated Windows aarch64 winver.h clang warning.
Extended reasoning...
Overview
Adds a 13-line block at the top of Bun__deepMatch in src/jsc/bindings/bindings.cpp that, when both received and expected are the same wrapper JSType (JSDOMWrapperType or JSAsJSONType), invokes specialObjectsDequal in both argument orders — the same helper and call pattern Bun__deepEquals already uses — before the enumerable-property walk. This makes toMatchObject, Bun.deepMatch, expect.objectContaining, and snapshot property matchers compare Headers / URLSearchParams / URL by their entries instead of vacuously matching. New tests land in test/js/bun/test/expect.test.js (5 tests under the toMatchObject describe) and test/js/bun/bun-object/deep-match.spec.ts (10 new it.each rows).
Security risks
None. This is pure test-matcher comparison logic; no untrusted input parsing, no I/O, no auth/crypto surface.
Level of scrutiny
Moderate-to-high. bindings.cpp is a core, load-bearing file, and this changes observable behavior of four user-facing matcher APIs. The change itself is small and mechanically conservative — it reuses an existing helper with the exact call shape Bun__deepEquals uses at lines 834–837, has RETURN_IF_EXCEPTION after each throwing call, and gates on the same JSType values specialObjectsDequal already keys on. I traced the JSDOMWrapperType/JSAsJSONType case in specialObjectsDequal (lines 1743–1857): it returns false on entry mismatch and std::nullopt (via compareAsNormalValue: break) otherwise, never true, so equal-entry wrappers and every non-URL/Headers/URLSearchParams wrapper (Blob, Request, Response, FormData, Cookie) fall through to the pre-existing property walk. The fresh local stack and contentsGCBuffer are safe because that branch only compares native WTF::String values and does not recurse through the JS heap.
Other factors
- The bug is unambiguous: two different
Headersmatching undertoMatchObjectis wrong, and Jest/Vitest both reject it. The PR description includes a released-binary vs. this-branch vs. Vitest behavior table, and fail-before/pass-after was verified on 1.4.0. - Test coverage is thorough across the variant matrix (nested / top-level / array element / snapshot property matcher /
objectContaining/Bun.deepMatch), with negative cases for both subset-larger and superset-larger, and the both-direction URLSearchParams asymmetry case. - The comment-cop feedback was addressed (comment reduced to one line in 9ead87e).
- The reason I'm deferring rather than approving: this is a behavior change to matcher semantics that intersects with three open PRs (#32870 takes a different-but-compatible approach for Date/Error/Set/Map leaves; #37881 and #37887 fix the underlying
specialObjectsDequalcomparisons this now depends on). The design choice — full-entry equality on these leaves rather than subset matching — is well-argued as Jest-compatible, but it's the kind of decision a maintainer should confirm, especially given the related in-flight work.
This branch has not been deployed
1 participant
Problem
toMatchObject,Bun.deepMatch,expect.objectContainingand snapshot property matchers accept anyHeadersfor any otherHeaders, and anyURLSearchParamsfor any otherURLSearchParams(same entry count):iterableEqualitycompares the entries);toEqualin Bun already fails them.Bun__deepMatch(src/jsc/bindings/bindings.cpp:1960) only walks the enumerable properties of the expected value.Headers,URLSearchParamsandURLkeep their state in C++ and expose nothing but prototype members, which are the same function objects on every instance, so the walk has nothing that reflects the entries. (URLhappens to work already because its prototype members are accessors;Headers.count/URLSearchParams.sizeare why a different entry count was already rejected.)Fix
JSDOMWrapperType: Headers, URL;JSAsJSONType: URLSearchParams), runspecialObjectsDequal, the content comparisontoEqual/Bun.deepEqualsuse, in both directions the wayBun__deepEqualsdoes. A verdict (different entries) is returned; no verdict (same entries, or a wrapper type it does not know) falls through to the existing property walk, so expando properties are still subset matched and every other wrapper type behaves exactly as before.toEqualuses for the same values, sotoMatchObjectandtoEqualagree on these leaves, and it matches Jest, which compares iterable leaves by their entries and does not subset match inside them. Follow-up fixes to that comparison (Make deepEquals and toEqual compare Headers with several set-cookie values #37881 set-cookie, Make deepEquals and toEqual compare every URLSearchParams entry, not just the first value per name #37887 repeated URLSearchParams names) apply here automatically; until Make deepEquals and toEqual compare every URLSearchParams entry, not just the first value per name #37887 lands, a repeated-nameURLSearchParamsleaf is rejected the same waytoEqualrejects it today.Bun__deepEqualsas a whole would regress the other wrapper classes:deepEqualsonly looks at own properties for them, while today's property walk reads prototype getters, so{ b: new Blob(["a"]) }vs{ b: new Blob(["abc"]) }andResponsewith a different status would start to match. The same-JSType gate also keeps mixed pairs (HeadersvsURLSearchParams, plain object vsHeaders) on the property walk, where they fail as before.Bun__deepMatchrather than in the recursion covers the top level (expect(headers).toMatchObject(headers2),Bun.deepMatch(h1, h2)), which Jest also fails.test/js/bun/test/expect.test.js(describetoMatchObject): 5 new tests fail on the released binary, pass with this change; the two unguarded ones also pass under Vitest 4.1.9.test/js/bun/bun-object/deep-match.spec.ts: 4 of the new rows fail on the released binary, all pass with this change.expect.test.js,deep-match.spec.ts,deep-equals.test.ts,url.test.ts,headers.test.ts,filesystem_router.test.ts, snapshot tests: unchanged results. A grep oftest/found no existing test that passes aHeaders,URLSearchParamsorURLinstance as atoMatchObject/objectContainingexpected value, so nothing else changes behavior.toMatchObjectis still not applied, andHeadersstill pretty-prints asHeaders {}in the failure diff; both are pre-existing and tracked separately. expect: fix toBeCloseTo opposite-sign Infinity and toMatchObject Date/Error/Set/Map leaves #32870 (open) handles the Date/Error/Set/Map leaves with a different rule (fulldeepEquals), which is right for those types because Jest'ssubsetEqualityexcludes them entirely; the two changes are independent.Background
Bun__deepMatchis the engine behindtoMatchObject,Bun.deepMatch,expect.objectContainingand the property-matcher argument oftoMatchSnapshot/toMatchInlineSnapshot. It enumerates the expected value's enumerable properties (including inherited ones) and, for object-valued properties, recurses; values are only compared through those properties.specialObjectsDequalis the helperBun__deepEqualscalls first for types whose equality is not defined by their properties (Set, Map, ArrayBuffer, Date, ...). For the wrapper branch it returnsfalsewhen URL hrefs or Headers / URLSearchParams entries differ andstd::nulloptotherwise, meaning "nothing special, go on comparing properties".Bun__deepEqualscalls it with the arguments in both orders because some comparisons (today's URLSearchParams one) only iterate the first argument.JSDOMWrapperType(Headers, URL, FormData, and Bun's generated classes such as Blob, Request, Response);JSAsJSONTypemarks wrappers that print viatoJSON()(URLSearchParams, Cookie, CookieMap).specialObjectsDequalkeys its wrapper branch on these two values, so the new check uses the same two.Behavior on the released binary vs this branch (probe output)