Treat Proxy as transparent in deep equality and toMatchObject - #32948
Treat Proxy as transparent in deep equality and toMatchObject#32948robobun wants to merge 7 commits into
Conversation
WalkthroughUpdates Proxy equality and matching fixes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Updated 4:05 AM PT - Aug 1st, 2026
❌ @robobun, your commit fbd8cb8 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 32948That installs a local version of the PR into your bun-32948 --bun |
838e593 to
f8f201b
Compare
|
Updated to also fix the
Those stray private names were also the only thing making The exact Vue + Pinia fixture from #9103 (both |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/jsc/bindings/bindings.cpp`:
- Around line 844-859: The strict equality path in specialObjectsDequal is
treating ProxyObjectType as a normal object after only comparing prototypes,
which lets transparent proxies over special objects slip through with ordinary
property comparison. Fix this by unwrapping transparent proxies before calling
specialObjectsDequal, or by teaching specialObjectsDequal to handle
ProxyObjectType directly so the Map/Set/Date/RegExp branches still run on the
underlying target. Use the existing ProxyObjectType checks and the
specialObjectsDequal entry point to keep the behavior consistent with the other
special-object comparisons.
🪄 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: 7bc45f1b-8b95-4e9f-af2e-36287d675b7c
📒 Files selected for processing (2)
src/jsc/bindings/bindings.cpptest/js/bun/test/expect.test.js
There was a problem hiding this comment.
All my earlier nits are addressed and I didn't find any correctness issues, but this changes core deep-equality/match semantics in C++ JSC bindings (prototype-identity gate, PrivateSymbolMode flip with a compensating array gate) and overlaps the same block as #29037/#32872, so I'd like a human to sign off on the semantic choices.
Extended reasoning...
Overview
This PR modifies two hot paths in src/jsc/bindings/bindings.cpp:
Bun__deepEquals(strict mode): when either side is aProxyObjectType, replaces thecalculatedClassNamestring comparison with an observable-prototype identity check (getPrototype+RETURN_IF_EXCEPTION,p1 != p2).Bun__deepMatch: switches bothPropertyNameArrayBuildersites fromPrivateSymbolMode::IncludetoExclude, adds an explicit "array expectation requires array received" gate, and readslengththrough traps when either sideisProxy().
It also adds ~90 lines of tests in expect.test.js covering proxied plain objects, arrays, class instances, nested/double proxies, get-trap honoring, revoked-proxy TypeError propagation, and node:util/node:assert parity.
Security risks
None identified. The change reads observable prototypes/lengths through Proxy traps (which can run user JS), but every such call is followed by RETURN_IF_EXCEPTION and these paths already invoke user traps elsewhere in the same functions. No new untrusted-input parsing, no auth/crypto/permissions surface.
Level of scrutiny
High. Bun__deepEquals and Bun__deepMatch back Bun.deepEquals, expect().toStrictEqual, expect().toMatchObject, util.isDeepStrictEqual, and assert.deepStrictEqual — user-facing equality semantics that every Bun test depends on. The change is not mechanical: it picks prototype identity for the Proxy branch while the non-Proxy branch keeps class-name string comparison (different behavior for cross-realm or distinct-but-same-named prototypes), and the PrivateSymbolMode flip would have regressed an existing test (expect({}).toMatchObject([])) absent the new compensating subsetIsArray && !objIsArray gate the author added. The PR description also explicitly notes overlap with #29037 and #32872 on the same strict-mode block.
Other factors
- All three of my prior inline nits (comment length/history, hoisting
require, baretoThrow()) were addressed in follow-up commits and are resolved. - The bug-hunting system found nothing on the current revision.
- CodeRabbit's special-objects concern was withdrawn after the author's empirical Node/Jest matrix; that discussion itself shows there are subtle reference-implementation disagreements in this space.
- Test coverage is thorough and the author reports cross-verification against Jest 30 and Node 26, plus the original Vue/Pinia repro.
- CI shows a
build-rust/build-bunfailure on the latest commit; likely infra, but worth confirming green before merge.
Given the semantic weight and the open-PR overlap on the same lines, I'm deferring rather than auto-approving.
|
On the one semantic reservation (prototype identity for the Proxy branch vs the class-name string for the non-Proxy branch, cross-realm and same-named-distinct classes): I probed both on the unpatched release binary and this branch so the answer is data rather than an argument.
Every row is unchanged. The two non-Proxy rows are not touched by this diff at all, and their looseness relative to Node (the There is also a structural reason no regression is possible on the Proxy branch. Before this change, on any input where a Proxy was involved, the old gate was one of exactly two things: unconditionally So the asymmetry is real but entirely inherited: the Proxy branch now uses the identity semantic that Node and Jest both use (prototype and |
There was a problem hiding this comment.
8ea48b2 addresses the array-length regression I flagged (and the PR description now clarifies #14250 is the issue / #14256 the fix, so the URL nit is moot) — nothing left from my side, but this is a multi-part semantic change to the core deepEquals/deepMatch C++ paths plus a reduction of an unrelated stress test's iteration count, so it's worth a human pass.
Extended reasoning...
Overview
The PR changes Bun__deepEquals and Bun__deepMatch in src/jsc/bindings/bindings.cpp so that transparent Proxy wrappers compare the way their targets would, fixing #9103 (Vue/Pinia/MobX reactive values failing toStrictEqual/toMatchObject). Concretely: (1) the strict-mode type gate compares observable prototypes via getPrototype() instead of calculatedClassName() when a Proxy is involved; (1b) when both sides are arrays and a Proxy forced them off the fast path, observable .length is compared explicitly; (2) Bun__deepMatch reads array length through traps when a Proxy is present and adds an explicit array-expectation-only-matches-array gate; (3) PrivateSymbolMode::Include → Exclude for both property-name collections in Bun__deepMatch. Tests add ~90 lines of Proxy coverage and separately reduce the Set/Map stress test from 150×2000 to 20×500 to fit the debug+ASAN budget.
Security risks
None identified. The new code paths call getPrototype/get/toLength (which can run user JS via traps), but each is followed by RETURN_IF_EXCEPTION so exceptions propagate cleanly, and the revoked-Proxy test pins TypeError. No auth, crypto, filesystem, or network surface.
Level of scrutiny
High. Bun__deepEquals backs Bun.deepEquals, expect().toStrictEqual, util.isDeepStrictEqual, and assert.deepStrictEqual; Bun__deepMatch backs toMatchObject. These are correctness primitives whose semantics users depend on across the whole test ecosystem, and the change introduces a deliberate asymmetry (prototype identity for the Proxy branch vs class-name string for the non-Proxy branch) that interacts with open work in #29037/#32872. The PrivateSymbolMode flip and the new array-vs-non-array gate in deepMatch also subtly change which inputs pass — the author caught and explained one such regression (expect({}).toMatchObject([])), but a human should sanity-check that reasoning.
Other factors
My prior 🔴 inline (Proxy-over-array vs trailing-hole array comparing equal) is fixed in 8ea48b2 with matching test coverage, and the 🟡 issue-URL nit was resolved by clarifying the PR description rather than the code (14250 = issue, 14256 = fix PR), which is fine. The author's empirical cross-runtime tables are thorough and the test coverage is good. The unrelated stress-test reduction is well-justified in the description but is the kind of drive-by change a maintainer should be aware of. No CODEOWNERS entry covers this path. Given the breadth of semantic surface and the number of review rounds it took to converge, deferring to a human reviewer rather than auto-approving.
Status: ready for reviewAll bot review threads are resolved (claude-bot's final summary: "No new issues found … the own-key-set check in fbd8cb8 addresses the last concern"; coderabbit withdrew its one finding after the cross-runtime matrix). The diff now covers the original report plus three slow-path cases review surfaced (array length with trailing holes; own-key sets under a lying Build 86854 (at
That test contains zero references to Locally, the full |
Bun.deepEquals(proxy, target, true) and toStrictEqual() rejected any
ProxyObject because the strict-mode type gate compares
JSObject::calculatedClassName(), which for a Proxy falls through to the
internal class name "ProxyObject" instead of deriving a name from the
prototype (ProxyObject overrides getPrototype, so that path is skipped).
"ProxyObject" never equals "Object", so a transparent new Proxy(t, {})
was reported unequal to its own target. Symmetrically, any two Proxies
both report "ProxyObject" and pass the gate regardless of what they
wrap. When either side is a Proxy, compare the observable prototypes
(through the getPrototypeOf trap) instead. Matches Node's
util.isDeepStrictEqual and Jest's toStrictEqual.
toMatchObject() on a Proxy over an array failed for two more reasons in
Bun__deepMatch:
- getArrayLength() reads the internal indexed butterfly, which a
ProxyObject does not have, so the "arrays should match exactly" gate
always compared 0 against the real length. Read the observable length
through the traps when a Proxy is involved, mirroring the !isProxy()
guard Bun__deepEquals already has on its array fast path.
- Property names were built with PrivateSymbolMode::Include (unlike
every site in Bun__deepEquals), sweeping JSC-internal private builtin
names from Array.prototype into the comparison. Plain arrays got away
with it because both sides resolve the same builtin function object
and reference equality short-circuits, but a Proxy breaks the
private-name chain walk. Users can never define private symbols, so
use Exclude. Those stray private names were also the only thing
making expect({}).toMatchObject([]) fail, so the real invariant (an
array expectation only matches an array) is now checked explicitly,
the same way Bun__deepEquals checks v1Array != v2Array.
The expect.test.js suite could not go green on a debug+ASAN build because the deepEquals Set/Map stress test took over four minutes and timed out at 5000ms (1s on a release build). Its cost is quadratic in the element count: every distinct array key misses the Set/Map identity lookup and takes the JSSetIterator/JSMapIterator linear fallback, and 150 elements across 3000 iterations is ~45M nested deepEquals calls. It is loose mode over Sets and untouched by the Proxy change; the timeout is pre-existing. Shrink the element count (the quadratic term) from 150 to 20 and the iterations from 2000/1000 to 500/250, which still constructs the fallback iterators thousands of times and runs in under 2 seconds on a debug+ASAN build. To confirm nothing is lost, the exact structure-confusion bug from the fix this test was added for (#14256: passing the Set's own Structure to JSSetIterator::create) was re-injected and the test passes both before and after this change, so the shrink does not reduce what it catches. The test also had zero assertions; every deepEquals result is now asserted, so a wrong answer on the Set/Map fallback path fails it too.
…ast path
The array fast path is the only place strict deepEquals compares array
length, and it is skipped whenever a Proxy is involved. The generic path it
falls through to only enumerates own enumerable properties, and a trailing
hole contributes none while length is not enumerable, so two arrays with the
same occupied indices but different lengths compared equal once the Proxy
type gate started letting the pair through:
Bun.deepEquals(new Proxy([1, 2, 3], {}), [1, 2, 3, ,], true)
// was false (the old gate rejected every Proxy), became true,
// Node's util.isDeepStrictEqual and Jest both say false
When both sides are arrays and the fast path was skipped, compare the
observable lengths through the traps, the same way Bun__deepMatch already
does. This also fixes the pre-existing false positive where two Proxies over
arrays of different lengths compared equal because both reported the class
name "ProxyObject".
8ea48b2 to
1b8420a
Compare
|
Rebased onto current main (was 837 commits behind, now mergeable). Two conflicts in Re-verified at 1b8420a:
Also dropped the two empty |
Each of the four new comments in bindings.cpp compresses to one line without losing the load-bearing fact, matching the file's existing style. The rebase onto main also layered the shrunk Set/Map stress test under an outer comment and a 480s per-test timeout that main had since added for the old four-minute workload. Both are now dead: the outer comment contradicts the inner one and the override guards a runtime that can no longer occur.
…olved
The generic slow path establishes key-set equality by (a) comparing key
counts and (b) probing each left-side key on the right with
getIfPropertyExists(), which uses InternalMethodType::HasProperty. That is
sound for ordinary objects, but a Proxy with a `has` trap that returns
true unconditionally makes the probe vacuous, so two objects with the same
key count but disjoint key sets compared equal (and asymmetrically, since
only the direction where the Proxy is on the right was fooled):
Bun.deepEquals({ b: 1 }, new Proxy({ a: 1 }, { get: () => 1, has: () => true }), true)
// was false (the old gate rejected every Proxy), became true, Node says false
When a Proxy is involved and the counts match, additionally verify every
left own key appears in the right own-key array (both come from
[[OwnPropertyKeys]]), which is what Node's util.isDeepStrictEqual does.
Note: a `get` trap alone cannot trigger this (getIfPropertyExists uses
HasProperty, not Get), so the reactive-proxy case from Vue / MobX / Immer,
which all forward `has` to the target, was already correct. Tests cover
both the get-only case and the lying `has` case.
There was a problem hiding this comment.
No new issues found — the own-key-set check in fbd8cb8 addresses the last concern I raised, and the bug-hunting pass came back clean. That said, this changes user-visible equality semantics for toStrictEqual / util.isDeepStrictEqual / toMatchObject and introduces a prototype-identity gate on the Proxy branch that intentionally diverges from the class-name gate on the non-Proxy branch (and overlaps #29037 / #32872), so a maintainer should sign off on the semantic choice.
What was reviewed:
- Exception discipline on every new trap-invoking call (
get,getPrototype,toLength,isArray) — all haveRETURN_IF_EXCEPTION. - The new
subsetIsArray && !objIsArraygate inBun__deepMatch— confirmed the reverse direction stays permissive per the existing test atexpect.test.js:3473-3474. - The O(n²) own-key-set comparison at lines 1039-1052 — Proxy-only, strict-mode-only, bounded by own-key count; not a hot-path concern.
- The stress-test shrink — outer comment and 480s override are gone, assertions now actually check the result.
Extended reasoning...
Overview
This PR fixes #9103 by making Bun__deepEquals (strict mode) and Bun__deepMatch treat Proxy wrappers as transparent, matching Node's util.isDeepStrictEqual and Jest's toStrictEqual/toMatchObject. It touches four sites in src/jsc/bindings/bindings.cpp: (1) a prototype-identity gate replaces calculatedClassName when either side is a Proxy; (2) an observable-length comparison for Proxy-over-array pairs that skip the array fast path; (3) an own-key-set comparison on the slow path when a Proxy's has trap could make the getIfPropertyExists probe vacuous; (4) in Bun__deepMatch, PrivateSymbolMode::Exclude, an explicit array-vs-non-array gate, and observable length reads for Proxies. It also adds ~100 lines of test coverage and shrinks an unrelated stress test that was timing out under debug+ASAN.
Security risks
None identified. The new code calls user-controllable Proxy traps (getPrototypeOf, get, has, ownKeys), but every call site has RETURN_IF_EXCEPTION and the values are only used for comparison — no allocation sizing, no pointer arithmetic, no privilege decisions. A revoked Proxy is covered by a test and correctly propagates a TypeError.
Level of scrutiny
High. This is core equality semantics reachable from expect().toStrictEqual, Bun.deepEquals, node:util.isDeepStrictEqual, node:assert.deepStrictEqual, and toMatchObject — a wide blast radius for any false-positive or false-negative. Review on this PR has already surfaced two real regressions (array length with trailing holes; own-key-set masking via a lying has trap), both now fixed with regression tests. That history is evidence the change is subtle, not evidence it's wrong.
Other factors
The prototype-identity semantic on the Proxy branch is stricter than the class-name-string semantic on the non-Proxy branch (cross-realm and same-named-distinct-class inputs behave differently on the two branches). The author defended this with an empirical before/after matrix showing no regression on those inputs and explicitly scoped unifying the non-Proxy branch to a separate PR that overlaps #29037 / #32872. That's a reasonable position, but it's a design call about how strict-equality should treat Proxies going forward and how it composes with those other PRs — a maintainer should own that decision rather than have it auto-approved.
The Bun__deepMatch change also adds a new failure mode (subsetIsArray && !objIsArray → false) that the author says was previously only enforced accidentally by private-symbol enumeration. The existing test at line 3478 covers it, but it is a user-visible behavioral clarification.
Given the semantic scope, the interaction with two other open PRs on the same block, and the review history, I'm deferring rather than approving.
Fixes #9103
A transparent
new Proxy(target, {})failstoStrictEqualagainst its own target, and a Proxy over an array failstoMatchObject. Both affect every value coming out of Vuereactive(), Pinia, MobX, Immer and most DI containers. Node and Jest report equal.Repro
The same root cause in strict mode also produces a false positive: any two Proxies pass the type gate, so two Proxies over different-class targets compare equal.
Review on this PR caught a third case: the array fast path is the only place
deepEqualscompares arraylength, and it is skipped when a Proxy is involved. The generic path only enumerates own enumerable properties, and a trailing hole contributes none whilelengthis not enumerable, so a Proxy-over-array compared equal to an array with a different length once the type gate let the pair through.Cause and fix
Three independent sites, all "an internal trap-bypassing accessor gives the wrong answer for a Proxy", in
src/jsc/bindings/bindings.cpp:1.
Bun__deepEquals, strict type gate. The gate comparesJSObject::calculatedClassName, which for aProxyObjectskips the__proto__.constructorderivation (proxies setOverridesGetPrototype, and the helper only followsgetPrototypeDirect()) and falls back toclassInfo()->className, the literal string"ProxyObject". That never equals"Object"/"Array"/"Foo", so a Proxy is unequal to every non-Proxy, and any two Proxies pass trivially.Fix: when either side is a
ProxyObjectType, compare the observable prototypes viagetPrototype(globalObject)(which goes through thegetPrototypeOftrap). A transparent Proxy reports its target's prototype, so it compares the way its target would. Non-Proxy comparisons are unchanged.1b.
Bun__deepEquals, array length for Proxies. When both sides are arrays and a Proxy forced them off the fast path, compare the observablelengththrough the traps before the generic property walk, the same way point 2 below handlesBun__deepMatch. This also fixes the pre-existing false positive where two Proxies over different-length arrays compared equal (both reported"ProxyObject").1c.
Bun__deepEquals, own-key sets for Proxies. The generic slow path establishes key-set equality by comparing counts and then probinggetIfPropertyExists()(which usesInternalMethodType::HasProperty). A Proxy with ahastrap that returnstrueunconditionally makes the probe vacuous, so two objects with disjoint key sets but equal counts compared equal (asymmetrically). When a Proxy is involved and counts match, additionally verify each left own-key appears in the right own-key array, as Node does. Agettrap alone cannot trigger this (the existence probe useshas, notget), so the Vue/MobX/Immer case was already correct.2.
Bun__deepMatch, array length gate.getArrayLength()reads the internal indexed butterfly, which aProxyObjectdoes not have, so the "arrays should match exactly" gate always compared0against the real length.Bun__deepEqualsalready guards its own array fast path with!isProxy();Bun__deepMatchwas the missing sibling. When a Proxy is involved, read the observable length through the traps.3.
Bun__deepMatch, property enumeration. Built withPrivateSymbolMode::Include, unlike every site inBun__deepEquals, which sweeps JSC-internal private builtin names fromArray.prototype(@entries,@values, ...) into the comparison. Plain arrays get away with it because both sides resolve the same builtin function object and reference equality short-circuits, but a Proxy breaks the private-name chain walk. Users can never define private symbols, so useExclude.Those stray private names turned out to be the only thing making
expect({}).toMatchObject([])fail (the existing test atexpect.test.js:3478caught the regression). The real invariant, an array expectation only matches an array, was never checked; it is now explicit, the same wayBun__deepEqualschecksv1Array != v2Array. The reverse direction is deliberately not gated: a plain-object expectation is a key subset, so a received array may satisfy it (expect.test.js:3473-3474asserts this).Exceptions from the traps (including a revoked Proxy) propagate via
RETURN_IF_EXCEPTION; Node throws there too.Verification
New cases live next to the existing Proxy coverage in
test/js/bun/test/expect.test.js. EverytoStrictEqual/toMatchObjectexpected value was confirmed against real Jest 30, and theutil.isDeepStrictEqual/assert.deepStrictEqualvalues against Node 26.test/js/node/assert/(150 pass),jest-extended.test.js,test/js/bun/util/inspect.test.jsall pass.toMatchSnapshot.test.ts'serror snapshotsfailure reproduces on the unpatched release binary and is pre-existing.One existing test in this file is modified:
deepEquals Set/Map stress testtook over four minutes under debug+ASAN and timed out at 5000ms (1s on a release build), which prevented the suite from going green. It is loose mode over Sets and untouched by this change. Its cost is quadratic in the element count, so that is reduced from 150 to 20 (and the iterations from 2000/1000 to 500/250), which still constructs theJSSetIterator/JSMapIteratorfallback thousands of times in under 2 seconds. To confirm nothing is lost, the exact structure-confusion bug it was added for (issue #14250, fixed in #14256) was re-injected and the test passes both before and after the shrink, so the reduction does not change what it catches. The test also asserted nothing (everydeepEqualsresult was discarded); each result is now assertedtrue.The exact Vue + Pinia fixture from the issue (both assertions, with real
vue@3andpinia@2) also passes.Related
#29037 and #32872 both touch the same strict-mode block (null-prototype vs
Object.prototype), but neither removes the unconditionalcalculatedClassNamecomparison, so neither fixes the Proxy case. This change only alters the branch taken when a Proxy is involved and composes with either.[review] gate passed · iteration 7 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 7
evidence per changed file