Skip to content

bun:test: compare an expected function by identity in toMatchObject and Bun.deepMatch - #38339

Open
robobun wants to merge 9 commits into
robobun/fd48c6eb/deep-match-map-set-functionfrom
farm/bcbc6d2d/deepmatch-functions-by-identity
Open

robobun wants to merge 9 commits into
robobun/fd48c6eb/deep-match-map-set-functionfrom
farm/bcbc6d2d/deepmatch-functions-by-identity

Conversation

@robobun

@robobun robobun commented Aug 14, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #15674

Problem

  • toMatchObject, Bun.deepMatch and snapshot property matchers accept any function or object for an expected function: expect({ cb: f }).toMatchObject({ cb: g }) passes. Jest, Vitest and toEqual fail it.
  • Bun__deepMatch (src/jsc/bindings/bindings.cpp) walks the enumerable keys of the expected value. A function has none that identify it.

Fix

  • subsetSpecialObjectMatches refuses an expected function that is not the received value. It runs before the key listing, so those keys are not read.
  • Under bun:test, toMatchObject(fn) gives Jest's matcher error. Under vitest it compares by identity.
  • Verified: the six test files here. The base build fails 20 of their tests. Self-reviewed: 20 concerns raised, 15 addressed (Notes).

Background

Downsides

  • It fails tests that pass today, so it needs a maintainer's yes. The August approval predates these facts. elysia 1.4.28 fails 8 tests (as with Jest), and test/vendor.json skips their file.
  • Three shapes pass in Jest, Vitest and the base, and fail here: an inherited method of the expected object (until bun:test: compare only the expected value's own properties in toMatchObject and Bun.deepMatch #46171), two iterables with their own Symbol.iterator method, and a Set, Map or Date with a function property.
  • Cost (release, linux x64): text +256 bytes, and 6 to 8 instructions for each plain toMatchObject call.
Notes

Order

The rule

  • An expected function matches only the same function. This holds under a string key, under an own symbol key, as an array element, as a member of a Set, as a key or a value of a Map, and as the subset of Bun.deepMatch.
  • Each callable value counts: a class, a bound, async or generator function, jest.fn(), a built-in constructor and a callable Proxy.
  • A tester of expect.addEqualityTesters still decides first.
  • A plain expected object still accepts a received function: { cb: f } matches { cb: {} }.
  • As the whole expected value of toMatchObject: Jest's matcher error under bun:test and @jest/globals, also with .not. Under vitest it is compared by identity.
  • As the property matchers of toMatchSnapshot: a mismatch.
  • A test that needs the old answer can register this tester. It restores the answer for two functions, and for items 1 to 3 of the list of differences below:
expect.addEqualityTesters([
  (a, b) => (typeof a === "function" && typeof b === "function" ? true : undefined),
])

Rows ("pass" means that expect(received).toMatchObject(expected) passes. The base is #47702 at b0f58a7. The references are expect 30.5.2 and @vitest/expect 4.1.12 on Node 26.3.0.)

received / expected base this PR Jest Vitest
{ cb: f } / { cb: g } pass fail fail fail
{ cb: {} } / { cb: f } pass fail fail fail
{ cb: class A {} } / { cb: class B {} } pass fail fail fail
{ cb: jest.fn() } / { cb: jest.fn() } pass fail fail fail
new TypeError("x") / { constructor: RangeError } pass fail fail fail
{ s: new Set([f]) } / { s: new Set([g]) } pass fail fail fail
two instances of a class with the field handler = () => 1 pass fail fail fail
{} / f pass error under bun:test, fail under vitest error fail
{} / f, with .not fail error under bun:test, pass under vitest error pass
{ [symbol]: f } / { [symbol]: g } pass fail fail pass
two copies of a class with an enumerable prototype method pass fail pass pass
two iterables, each with its own Symbol.iterator method pass fail pass pass
two instances of class M extends Map { h = () => 1 } pass fail pass pass
two instances of class E extends Error { h = () => 1 } pass fail pass fail
{ a: 1 } / expect.objectContaining(f) pass pass throws throws
{ cb: f } with f.x = 1 / { cb: { x: 1 } } pass pass fail fail
f / {} pass pass error pass
two callable Proxies, with toEqual pass pass fail fail
  • 91 rows ran on release builds of the base and of this PR, under both imports, and on the two references.
  • Under bun:test, 48 rows change. Agreement with Jest goes from 42 to 60 rows. 14 rows that agree with Jest on the base fail here.
  • Under the vitest import, 46 rows change. Agreement with Vitest goes from 44 to 56 rows. 17 rows that agree with Vitest on the base fail here.
  • The rows are chosen to cover the places where a function can be. They are not a sample of user tests.

What differs from the references after this PR

13 of the 91 rows pass on the base, in Jest and in Vitest, and fail here. They are the first three items.

  1. Inherited keys (7 rows). Bun walks the enumerable keys that the expected object inherits (toMatchObject compares inherited properties, so a null-prototype array fails (Bun.deepMatch too) #45950). Jest and Vitest read own keys only. So a method on the prototype of the expected object is compared, and two copies of one class with an enumerable prototype method do not match. The same holds for an object, an array or a Set of functions on the prototype. Two instances of one class share their methods and match. Methods of a class declaration are not enumerable and are not compared. bun:test: compare only the expected value's own properties in toMatchObject and Bun.deepMatch #46171 changes the walk to own keys.
  2. An own Symbol.iterator method (2 rows). Jest and Vitest compare two iterables by what they yield (iterableEquality), before they read the keys. Bun has no rule for an iterable, so it compares the two methods. Bun's toEqual fails the same pair on main. No issue or PR covers an iterable rule.
  3. A function that an expected Set, Map or Date holds as a property (4 rows). Jest and Vitest do not read the keys of those values. Bun reads them after the members, as on main, so it compares the two functions. The same holds for an Error against Jest (Vitest fails that pair): two equal Zod 3 ZodError values are an example, because each holds its own addIssue closure.
  4. Symbol keys under the vitest import (4 rows). Vitest reads Object.keys(expected). Bun reads symbol keys under both imports, as bun:test: compare an expected Map and Set in toMatchObject and Bun.deepMatch #47702 lists. So two objects that each own a function under a symbol key fail under both imports: a TypeBox schema, or an object with its own [Symbol.asyncIterator](). vitest-flavor.test.ts marks the row as a known difference.
  5. expect.objectContaining(fn) still matches each object. Jest and Vitest throw You must provide an object to ObjectContaining, not 'function'. A function with properties as the sample is a working comparison in Bun today, so this PR does not change it.
  6. Two different callable Proxies fail toMatchObject and still pass toEqual, toStrictEqual and a key of expect.objectContaining. Bun__deepEquals has its own list of the types of a function, and a callable Proxy is not on it. Make deepEquals and toEqual compare built-in constructors, mocks and callable Proxies by identity #38334 is the PR for that.
  7. A received function is still accepted, as on main. Jest refuses it.
  8. Inside expect.poll under the vitest import, toMatchObject(fn) gives the matcher error of bun:test. is_vitest() is false inside an attempt (expect_deferred.rs, parent: None). On main, .not.toMatchObject() with a value that is not an object has the same answer there.
  9. toMatchSnapshot(fn) is a mismatch under both imports.

expect.test.js has a test.todo for items 1, 2, 3 and 6. Each passes on the two references.

Tests that the rule changes

Measurements (release builds for linux x64 in one build directory. The base is the tree of #47702 at b0f58a7, and this PR is a14502c.)

  • llvm-size: text 140,296,431 to 140,296,687 (+256). data and bss do not change. The file grows by one page (140,482,848 to 140,486,944).

  • llvm-nm -S: Bun__deepMatch<true> 3,346 to 3,331 bytes, Bun__deepMatch<false> 2,251 to 2,277, subsetSpecialObjectMatches 6,256 to 6,380, the toMatchObject closure 1,133 to 1,204. Both subsetValueMatches copies do not change (1,184 and 700).

  • Android arm64: 132,896,904 bytes for the stripped binary on both builds, so +0 (binary-size step of CI, build 137119 against build 137030 of bun:test: compare an expected Map and Set in toMatchObject and Bun.deepMatch #47702). The step passes. The Linux, FreeBSD and macOS x64 binaries do not change either. macOS arm64 grows by one page (16,512 bytes), Windows arm64 grows by 3,584 bytes and Windows x64 shrinks by 512 bytes.

  • Instructions for each call (user space, perf_event_open, JIT off, a loop of 20,000 calls minus an empty run, the lowest of three runs, without --aot):

    call base this PR
    toMatchObject, 4 key object, 2 key pattern 3,354 3,360
    toMatchObject, 5 key object, the same 5 keys 4,557 4,563
    toMatchObject, 3 levels of plain objects and an array 19,911 19,896
    toMatchObject, array of 10 objects 40,189 40,127
    toMatchObject, two instances of a class 3,736 3,744
    toMatchObject, pattern with the same function under a key 3,844 3,847
    toMatchObject, pattern with an equal Date under a key 5,806 5,810
    Bun.deepMatch({}, {}) 1,470 1,461
    Bun.deepMatch, 5 key object, the same 5 keys 3,387 3,361
    Bun.deepMatch, 3 levels and an array 17,658 17,580
    expect.objectContaining, 4 key object 3,579 3,577
    Bun.deepMatch({ cb: g }, { cb: f }) 4,436 2,160
    Bun.deepMatch(g, f) 2,942 683
    Bun.deepMatch(new Date(2), new Date(1)) 2,779 549
    Bun.deepMatch({ s: new Set([1, 2]) }, { s: new Set([1]) }) 4,648 2,177
  • The spread of the three runs is 3 or less, except for the array of 10 objects (43 on the base, 34 here).

  • The 6 to 8 instructions of a plain toMatchObject call are the type test in toMatchObject.rs: 6 instructions with 2 conditional branches in the disassembly (llvm-objdump). It has three plain types: FinalObject, Array and CompiledObject, the type of an object literal under bun test --aot.

  • Calls of JSC__JSValue__isCallable from toMatchObject (gdb hit counts over 1,000 calls): 0 to 0 for a plain object or an array as the expected value, also under --aot. 0 to 1 for a Date or another value that is not a plain object. is_in_vitest_test runs only if the expected value is a function.

  • Under --aot the instruction counts of one call differ by several hundred between runs, so there is no table for that mode. There, the check of bun:test: compare an expected Date and Error by value in toMatchObject and Bun.deepMatch #44497 already calls subsetSpecialObjectMatches for each object literal of the pattern on the base (1,000 calls for 1,000 patterns, gdb). This PR adds one isCallable() to that call.

  • Key lists (one 184 byte allocation each, gdb hit counts at the call site over 1,000 calls): Bun.deepMatch(g, f) 1 to 0 for each call, a Date pair that fails 1 to 0, a plain pair 1 to 1.

  • Proxy traps on an expected callable Proxy, for one compare: 4 to 0 (ownKeys, two of getOwnPropertyDescriptor, getPrototypeOf). This holds for the subset of Bun.deepMatch, under a key and as a member of a Set. A trap that throws no longer turns the mismatch into an error.

Tests

  • With the test files of this PR, a release build of the base fails 20 tests: expect.test.js 6, deep-match.spec.ts 10, bun-snapshots.test.ts 1, server.spec.ts 1, vitest-apis.test.ts 1, vitest-flavor.test.ts 1. A debug build and a release build of this PR pass all of them.
  • The toMatchObject tests of expect.test.js and all of deep-match.spec.ts pass with BUN_JSC_validateExceptionChecks=1 on the debug build.
  • "the keys of an expected function are not read" fails without the move of the check: the base runs 4 traps there.
  • "a tester decides for two functions before toMatchObject compares them by identity" holds the order: testers, then identity.
  • The bodies of the new tests that are not under if (isBun), the todo tests too, pass on expect 30.5.2. Those that expect.test.js does not skip for Vitest pass on @vitest/expect 4.1.12.
  • 346 test files use toMatchObject, Bun.deepMatch or snapshot property matchers. Each ran on a release build of the first commit of this step (d8bb63d). 292 ran tests and exit 0. 21 run no test on this machine. 33 exit non-zero, and each of those ran on the base build too: 31 have the same counts there (tests that need the network, a database or a WebKit checkout). The other two are fetch.test.ts (11 failures here, 14 on the base) and napi.test.ts. Its one failure is "behaves like node on edge cases", a compare of two strings, and it also fails on the base build in 3 of 12 more runs. So no file outside this PR changes its result.

What changed from the earlier shape of this PR (29a7a0a)

  • The private-name change and the rule that an expected array needs an array are in bun:test: leave private names out of the key walk of toMatchObject and Bun.deepMatch #47736.
  • The two isCallable() checks in the walk (one for each call of Bun__deepMatch, one for each pair) are gone. The rule is one line in the cold arm.
  • The root check of the earlier shape also ran for the sample of expect.objectContaining(). Now that sample keeps its answer.
  • .not.toMatchObject(fn) under bun:test is a matcher error. In the earlier shape it passed.
  • The note in test/vendor.json says that Jest fails the elysia tests and Vitest passes them.

Self-review

20 concerns raised, 15 addressed. What the review changed:

Not taken:

Bun__deepMatch walked into any pair of objects, and functions are
objects with (usually) no enumerable properties, so a function in the
expected object matched any function in the received one:
expect({ p: () => 1 }).toMatchObject({ p: () => 2 }) passed, as did
Array vs Object and two distinct jest.fn() mocks. Jest compares
function leaves with Object.is.

A callable on the subset side is now compared with SameValue, both for
property values and at the root (Bun.deepMatch(a, b), toMatchObject,
snapshot property matchers, objectContaining). The check is made before
the seen-set bookkeeping so a function reused under several keys is
compared under each of them.

Comparing functions by identity exposed that the property walk included
JSC private symbols: Array.prototype's private builtin methods are
enumerable, so every array pattern would have compared those builtins
and rejected arrays from another realm (node:vm). Private symbols are
now excluded like everywhere else in deepEquals, and the check those
private names were accidentally providing, that an array pattern only
matches an array, is made explicit.
@coderabbitai

coderabbitai Bot commented Aug 14, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 2 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: d966148c-efdf-4478-ab90-d9ca0818f22c

📥 Commits

Reviewing files that changed from the base of the PR and between 1cf8af0 and 29a7a0a.

📒 Files selected for processing (6)
  • src/jsc/bindings/bindings.cpp
  • test/js/bun/bun-object/deep-match.spec.ts
  • test/js/bun/test/expect.test.js
  • test/js/bun/test/snapshot-tests/bun-snapshots.test.ts
  • test/js/node/net/server.spec.ts
  • test/vendor.json

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Aug 14, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 9:53 PM PT - Nov 2nd, 2026

❌ @robobun, your commit a14502c has 40 failures in Build #137119 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 38339

That installs a local version of the PR into your bun-38339 executable, so you can run:

bun-38339 --bun

@robobun

robobun commented Aug 14, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: ready for review at a14502c. This PR is now the function step alone. Its base is the branch of #47702.

Reproduced on a release build of #47702 (b0f58a7) with bun test: expect({ cb: () => 1 }).toMatchObject({ cb: () => 2 }) passes, and Bun.deepMatch(function a() {}, function b() {}) is true. On this branch both fail, as they do in Jest 30.5.2 and Vitest 4.1.12.

The stack, bottom to top: #47736, #44497, #47702, this PR. The step fails tests that pass today, so it waits for a maintainer's decision. The body lists them.

CI (build 137119): the tests of this PR pass on each lane that ran. 28 test files are red, and none of them is in this diff. 27 are red on the builds of recently merged PRs too (test/aot/* and compile-aot-size). test/aot/engine/native-5.test.ts is red on the build of #47702 (137030) too. The other failures passed on a retry. The macOS x64 test jobs have not started yet.

With FORCE_COLOR=1, which the CI runner sets, the word propertyMatchers
in the message is wrapped in ANSI codes, so a substring spanning it does
not match.
Comment thread src/jsc/bindings/bindings.cpp Resolved
Comment thread src/jsc/bindings/bindings.cpp Resolved
Comment thread src/jsc/bindings/bindings.cpp Resolved
Comment thread src/jsc/bindings/bindings.cpp Resolved
dylan-conway
dylan-conway previously approved these changes Aug 14, 2026
@dylan-conway
dylan-conway enabled auto-merge (squash) August 14, 2026 03:41

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional findings (outside current diff — PR may have been updated during review):

  • 🔴 src/jsc/bindings/bindings.cpp:2069 — This guard is correct, but two existing CI tests relied on the old any-function-matches behavior and neither is updated in this PR — Build #95364 is red on both. (1) test/js/node/net/server.spec.ts:83 — expect(net.Server.prototype).toMatchObject(EventEmitter.prototype): EventEmitter.prototype.constructor is an own enumerable property (direct assignment at src/js/node/events.ts:100) whose value is EventEmitter, while net.Server.prototype.constructor is Server (set by $toClass), so the callable subset prop now falls through to sameValue(Server, EventEmitter) → false; drop line 83 (line 82 already asserts the prototype chain) or rewrite with expect.any(Function). (2) vendor/elysia/test/units/replace-schema-type.test.ts — elysia's t.Numeric() etc. embed fresh Decode/Encode closures under the enumerable public symbol [TransformKind], so two independent t.Numeric() schemas now fail toMatchObject; Bun can't edit elysia's tests, so add a documented skipTests entry for this file to test/vendor.json (matching the four existing precedents there).

    Extended reasoning...

    What breaks

    The new !subsetProp.isCallable() guard (and the equivalent root-level subsetValue.isCallable() check) is the correct, Jest-aligned behavior. But it changes what "match" means for any subset property whose value is a function: previously Bun__deepMatch recursed into the function as an object, found no enumerable properties, and vacuously returned true; now it falls through to sameValue and requires reference identity. Two existing tests in Bun's CI gate depended on the old vacuous-walk behavior, and neither is touched in this PR's changed-files list. CI Build #95364 (robobun comment) shows both failing.

    REVIEW.md: "When changing output/defaults/messages, grep the suite for assertions on the old behavior and update them in the same PR." The PR body's scan of test/ for toMatchObject arguments containing function values missed (1) because the argument is a prototype object rather than a literal with a visible function value, and missed (2) because vendor tests aren't checked into test/ — they're fetched at CI time.

    (1) test/js/node/net/server.spec.ts:83

    expect(net.Server.prototype).toMatchObject(EventEmitter.prototype);

    Step-by-step:

    1. src/js/node/events.ts:91 sets EventEmitter.prototype = {} (a plain object), and line 100 does EventEmitterPrototype.constructor = EventEmitter — a direct assignment on a plain object, which creates an own enumerable data property. So when Bun__deepMatch enumerates the subset (EventEmitter.prototype) with DontEnumPropertiesMode::Exclude, constructor is included.
    2. src/js/node/net.ts:3532 calls $toClass(Server, "Server", EventEmitter), which gives net.Server.prototype its own (non-enumerable) constructor = Server, shadowing the inherited EventEmitter. obj->getIfPropertyExists(globalObject, "constructor") therefore returns Server.
    3. At the property loop, subsetProp = EventEmitter and prop = Server. With the new guard subsetProp.isObject() && !subsetProp.isCallable() && prop.isObject(), !subsetProp.isCallable() is false (EventEmitter is callable), so control skips the recursive branch and reaches sameValue(Server, EventEmitter) → false → toMatchObject throws.
    4. Every other enumerable method on EventEmitter.prototype (on, emit, …) is inherited by identity on net.Server.prototype via __proto__, so those keys still sameValue-match; constructor is the one shadowed key that breaks.
    5. Before this PR, both Server and EventEmitter are objects → recurse → neither has enumerable own properties → vacuously matched → test passed.

    CI shows this file failing with code 1 on all 9 platforms, consistent with a deterministic behavior change rather than a flake.

    Fix: the new behavior is correct (Jest agrees), so update the test in this PR — either delete line 83 (line 82 already asserts net.Server.prototype.__proto__ === EventEmitter.prototype, which is the actual invariant), or replace it with an explicit shape check using expect.any(Function) for the methods.

    (2) vendor/elysia/test/units/replace-schema-type.test.ts

    test/vendor.json pins elysia at 1.4.28; the vendor lane fetches its tests at CI time and runs them under Bun. The failing file does e.g.:

    expect(replaceSchemaType(t.Object({ id: t.Number(), ... }), { from: t.Number(), to: () => t.Numeric() }))
      .toMatchObject(t.Object({ id: t.Numeric(), ... }))

    Step-by-step:

    1. Elysia's t.Numeric() (also t.BooleanString(), t.ObjectString(), t.Date()) is built as t.Transform(...).Decode(fn).Encode(fn) (elysia src/type-system/index.ts). TypeBox's .Encode() returns { ...schema, [TransformKind]: { Decode, Encode } } where TransformKind = Symbol.for('TypeBox.Transform') — a public registered Symbol assigned via computed-key spread, so it's an own enumerable symbol property. Every t.Numeric() call therefore produces fresh Decode/Encode closures.
    2. Bun__deepMatch enumerates with PropertyNameMode::StringsAndSymbols, and the new PrivateSymbolMode::Exclude does not hide TransformKind (it's a public JS Symbol, not a JSC private symbol). So it walks into [TransformKind] on both sides (both plain objects), then reaches Decode where subsetProp is a callable.
    3. With the new guard, Decode no longer recurses; it falls through to sameValue(receivedDecode, expectedDecode) on two distinct closures → false.
    4. Before this PR both closures were walked (no enumerable props) → vacuously matched → test passed. So this failure is new to this PR, not pre-existing.

    Fix: Bun cannot edit elysia's tests. Following the four existing precedents in test/vendor.json, add a skipTests entry:

    "units*replace-schema-type.test.ts": "TEMPORARY: elysia 1.4.28 relies on Bun's old any-function-matches-any-function toMatchObject behavior — t.Numeric()/t.BooleanString() embed fresh Decode/Encode closures under [TransformKind], so two independent schemas no longer deep-match now that Bun compares functions by identity (Jest-aligned, oven-sh/bun#<this PR>). Remove once elysia's tests use expect.any(Function) or a shared schema instance."

    Why both, in this PR

    These are two independent actionable items with the same root cause: (1) is a test-file edit; (2) is a vendor.json addition. Fixing one does not fix the other, and merging without both leaves CI red.

net/server.spec.ts matched Server.prototype against EventEmitter.prototype,
whose enumerable own constructor is EventEmitter while Server.prototype's
is Server; compare the inherited members instead. elysia's
replace-schema-type tests toMatchObject independently created TypeBox
transform schemas, whose Decode/Encode closures now compare by identity;
skip that file until it is updated upstream.
@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

CI turned up two existing tests that relied on the old behavior; both confirmed locally and handled in 29a7a0a:

  • test/js/node/net/server.spec.ts matched net.Server.prototype against EventEmitter.prototype, whose own enumerable constructor is EventEmitter while Server.prototype.constructor is Server. The test now matches against the inherited members, which still checks what its name says.
  • elysia 1.4.28's test/units/replace-schema-type.test.ts (8 tests) matches one t.Numeric() / t.ObjectString() schema against another; each call creates fresh Decode / Encode closures under the enumerable Symbol.for("TypeBox.Transform") key, so they no longer match (39 pass on the released binary, 31 pass / 8 fail on this branch). The file is unchanged upstream as of elysia 1.4.29, so it is skipped in test/vendor.json with a note in the same style as the existing entries.

The push dismissed the earlier approval; the PR description has been updated with the above.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs (the earlier FORCE_COLOR concern was addressed in d7b4007). Because this is a user-facing behavior change to toMatchObject/Bun.deepMatch semantics that breaks downstream tests — one in-tree test was rewritten and an elysia vendor file is now skipped — a human look would still be worthwhile to confirm the behavior change and the vendor skip are wanted.

What was reviewed:

  • The isCallable() guards at the root and property level fall through to sameValue correctly; asymmetric matchers are handled earlier and are not callable, so expect.any(Function) is unaffected.
  • isArray calls now have exception checks (they can throw on revoked Proxy); the explicit subsetIsArray && !objIsArray rule preserves the expect({}).toMatchObject([]) failure that private symbols were previously enforcing accidentally.
  • PrivateSymbolMode::Exclude matches every enumeration in Bun__deepEquals; verified the cross-realm array tests pin the behavior it preserves.
  • The net/server.spec.ts rewrite still exercises what its name says: spread copies own-enumerable EventEmitter methods, and toMatchObject resolves each on Server.prototype's chain to the identical function.
Extended reasoning...

Overview

This PR changes Bun__deepMatch in src/jsc/bindings/bindings.cpp so that a callable value on the subset (pattern) side is compared by SameValue identity instead of being walked as an object with (typically) no enumerable properties. It also switches property enumeration to PrivateSymbolMode::Exclude, adds exception checks after the two isArray calls, and makes the array-subset-needs-array-received rule explicit (previously enforced only as a side effect of private-symbol enumeration). Tests are added to expect.test.js, deep-match.spec.ts, and bun-snapshots.test.ts; net/server.spec.ts is updated because it relied on any-function-matches-any-function; and an elysia vendor test file is skipped in test/vendor.json for the same reason.

Security risks

None. This is test-runner comparison logic; no auth, crypto, filesystem, or network paths are touched. isCallable() does not run user code (Proxy callability is fixed at construction), and the new isArray calls now correctly propagate exceptions from revoked Proxies where the old combined && expression did not.

Level of scrutiny

Medium-high. Bun__deepMatch backs toMatchObject, Bun.deepMatch, snapshot property matchers, and expect.objectContaining — a bug here would affect many test suites silently. The C++ change is small and mechanically correct (I traced the fall-through: when !subsetProp.isCallable() is false, control reaches the else branch which does JSC::sameValue, i.e. identity for two objects). The coupled PrivateSymbolMode and array-rule changes are each individually justified and pinned by cross-realm and {}-vs-[] tests.

Other factors

  • This is a behavior change users can observe: two independently-created functions that used to match no longer do. The PR itself demonstrates real breakage (the in-tree net/server.spec.ts and 8 elysia vendor tests). Aligning with Jest/Vitest is the right call, but shipping a change that requires a vendor-test skip is a decision a maintainer should sign off on rather than an automated approval.
  • My prior review flagged the FORCE_COLOR/ANSI issue in the snapshot test; the author confirmed and fixed it in d7b4007 by asserting only the uncolored substring. All comment-cop feedback on verbose C++ comments was addressed in 1df2009.
  • Test coverage is thorough: identity/non-identity, nested, in-array, native constructors, jest.fn(), functions with own props, reused-under-multiple-keys, root-level, objectContaining, snapshot property matchers, cross-realm arrays, and the array-like/typed-array negative cases. The PR description documents that the new tests fail on the released binary and pass under BUN_JSC_validateExceptionChecks=1.
  • The bug-hunting system found no issues on the current revision.

@robobun

robobun commented Nov 2, 2026 •

Copy link
Copy Markdown
Collaborator Author

This PR stays open. #47702 does not replace it. The plan for the toMatchObject changes is now:

The function rule stays a separate step because it has a known breakage. It fails 8 elysia tests, which this PR skips in test/vendor.json. A maintainer can then decide on it separately.

Take Bun__deepMatch and its tests as they are in 08eceab. The private-name
change and the rule that an expected array needs an array are in that branch
through #47736.
…nd Bun.deepMatch

Bun__deepMatch walks the enumerable keys of the expected value. A function has
none that say which function it is, so each function matched each other
function, and each object.

The check for the kinds that the key walk cannot read (Date, Error, Set, Map)
now runs before the keys are listed, and its last arm refuses an expected
function that is not the received value. The keys of an expected function are
not read.

Under bun:test, toMatchObject gives the matcher error of Jest for a function as
the whole expected value. Under the vitest import that value is compared by
identity, as in Vitest.
@robobun
robobun disabled auto-merge November 3, 2026 00:59
…of toMatchObject, and mark the known differences

Under `bun test --aot` an object literal has the type CompiledObject, so
toMatchObject asked JSC whether each expected value is callable. It is now
on the list of plain types beside FinalObject and Array.

Tests: a todo for each pair that Jest and Vitest pass and Bun does not (a
method that the expected object inherits, a function that an expected Set,
Map or Date holds as a property, two callable Proxies in toEqual), and a
comment on the symbol key row of the vitest import.
@robobun
robobun changed the base branch from main to robobun/fd48c6eb/deep-match-map-set-function November 3, 2026 01:38
@robobun
robobun requested a review from alii as a code owner November 3, 2026 01:39
@robobun robobun changed the title Compare functions by identity in toMatchObject and Bun.deepMatch bun:test: compare an expected function by identity in toMatchObject and Bun.deepMatch Nov 3, 2026
@robobun

robobun commented Nov 3, 2026

Copy link
Copy Markdown
Collaborator Author

@dylan-conway I disabled the auto-merge that was armed on this PR on 2026-08-14.

Arm it again if this shape is what you want.

Comment thread src/jsc/bindings/bindings.cpp Resolved
Comment thread src/jsc/bindings/bindings.cpp Resolved
Comment thread src/jsc/bindings/bindings.cpp Resolved

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants