Conversation
Bun__deepMatch walks the expected object's enumerable properties. Temporal objects keep their value in internal slots and have none, so a Temporal expected value matched any received object in toMatchObject, Bun.deepMatch, expect.objectContaining, and snapshot property matchers. Route a Temporal expected value through the comparator deepEquals already uses, and replace the local Temporal class check with JSC::temporalType.
|
Warning Review limit reached
Next review available in: 35 seconds Limit details: You’ve used all 5 included reviews currently available under your plan. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: ready for review. Automated review left no requested changes; CI is running. Reproduced on bun 1.4.0 with |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-scoped correctness fix reusing the existing temporalObjectsDequal comparator.
What was reviewed:
- Traced
temporalObjectsDequal(obj, subsetObj)— whensubsetObjis Temporal it never returnsnullopt, so.value()is safe. - Confirmed the check is on the subset side only, so
expect(plainDate).toMatchObject({ year: 2024 })still walks prototype getters (covered by a test). - Verified
isTemporalObjecthad no other callers; its replacementJSC::temporalTypeis already used at both aJSObject*and aJSValuecall site in this file. - New path does no JS execution (only
dynamicDowncast+ internal-slot reads), so no exception check is needed.
Extended reasoning...
Overview
The PR fixes Bun__deepMatch (backing toMatchObject, Bun.deepMatch, expect.objectContaining, and snapshot property matchers) to compare Temporal objects by their internal fields instead of vacuously accepting any object. The C++ change is four new lines in bindings.cpp plus replacing the local isTemporalObject helper with JSC::temporalType. ~87 lines of new tests in deep-equals-temporal.test.ts cover all eight Temporal classes across every entry point.
Security risks
None. This is test-matcher comparison logic; no untrusted input parsing, no allocation, no I/O.
Level of scrutiny
Low-to-medium. The change reuses temporalObjectsDequal (added in #37024 for deepEquals) verbatim, guarded by the same JSC::temporalType classifier already used elsewhere in the file. I traced the optional: when the guard holds (subsetValue is Temporal), o2 is Temporal, and temporalObjectsDequal returns a definite bool on every branch — either o1 matches one of the eight dynamicDowncast arms, or the trailing temporalType(o2) != None check returns false. .value() cannot throw. The new code runs no user JS, so no RETURN_IF_EXCEPTION is needed (consistent with the PR's BUN_JSC_validateExceptionChecks=1 claim). The pre-existing invariant that obj/subsetObj are non-null objects is unchanged — both were already dereferenced unconditionally below.
Other factors
Test coverage is thorough: each of the eight classes is exercised through toMatchObject (nested, array, top-level), expect.objectContaining, Bun.deepMatch, plus cross-class mismatch, plain-object received, plain-object expected (asymmetry preserved), extra own properties, and snapshot property matchers with a specific error-message assertion. The PR description documents 35 of these fail on 1.4.0. Related but distinct vacuous-match cases (Date/Error/Set/Map, Headers/URLSearchParams, pretty-format) are explicitly deferred to their own tracked issues, keeping scope tight. No CODEOWNERS on the touched paths and no prior human review comments to address.
This branch has not been deployed
2 participants
Problem
toEqual,toStrictEqualandBun.deepEqualshave compared Temporal values by their internal fields since Compare Temporal objects by value in Bun.deepEquals and toEqual #37024, so the matchers disagree with each other inside one test file.Bun__deepMatch(src/jsc/bindings/bindings.cpp, shared bytoMatchObject,Bun.deepMatch,expect.objectContaining, and the property matchers oftoMatchSnapshot/toMatchInlineSnapshot) collects the expected value's enumerable properties and checks each against the received value. A Temporal object has none (its fields are internal slots behind non-enumerable prototype getters), so the loop runs zero times and returns true.Fix
Bun__deepMatch, an expected value thatJSC::temporalType()classifies as Temporal is compared withtemporalObjectsDequal, the comparatordeepEqualsalready uses, instead of the property walk. Same class and same fields match; a different class, a different value, or a non-Temporal received value does not. Extra own properties are ignored, astoEqualalready does for Temporal andDate.{}still matches, andexpect(plainDate).toMatchObject({ year: 2024 })still reads the prototype getter.isTemporalObjecthelper is replaced byJSC::temporalType, which performs the same eight-class check (this is the same replacement Format Temporal values in console.log, util.inspect, and test pretty-format #37043 makes, so the two merge cleanly in either order).BUN_JSC_validateExceptionChecks=1.test/js/bun/bun-object/deep-equals-temporal.test.ts: newsubset matching on Temporal valuesblock covering all eight classes throughtoMatchObject(nested, in arrays, and top level),expect.objectContaining,Bun.deepMatch, cross-class and plain-object expected values, extra own properties, and snapshot property matchers. 35 of the new cases fail on bun 1.4.0 (also withCI=true), all 113 pass with this change.test/js/bun/test/expect.test.js(415 pass),deep-match.spec.ts,snapshot-tests/bun-snapshots.test.ts,node/assert/deep-equal.test.ts, both TOML suites andweb/temporal/temporal.test.ts: unchanged.Date/Error/Set/Mapexpected values is expect: fix toBeCloseTo opposite-sign Infinity and toMatchObject Date/Error/Set/Map leaves #32870, and forHeaders/URLSearchParamsit is Make toMatchObject and Bun.deepMatch compare Headers and URLSearchParams by their entries #37904; both are separate decisions about how those leaves should compare. The snapshot text itself (PlainDate {}for every value, so a snapshot of a Temporal value never fails, andtoEqualfailures print an empty diff) is the pretty-format side and is addressed by Format Temporal values in console.log, util.inspect, and test pretty-format #37043; after this PR a mismatching Temporal property matcher fails, but the diff printed for it still looks empty until Format Temporal values in console.log, util.inspect, and test pretty-format #37043 lands.Background
toMatchObject: the expected value lists properties the received value must have; extra received properties are fine. In Bun it is implemented once inBun__deepMatchand reused byBun.deepMatch(subset, object),expect.objectContaining, and the optional property-matchers argument of the snapshot matchers.Instant,PlainDate,PlainDateTime,PlainTime,ZonedDateTime,PlainYearMonth,PlainMonthDay,Duration) are immutable values whose state lives in JSC internal slots;year,epochNanoseconds, etc. are non-enumerable getters on the prototype. Any property-based comparison therefore sees two instances of the same class as identical.Datehas the same shape, which is whydeepEqualsspecial-cases both.temporalObjectsDequalis the field comparison added in Compare Temporal objects by value in Bun.deepEquals and toEqual #37024: same class required, then exact time forInstant, ISO fields plus calendar for the plain types, exact time plus time zone plus calendar forZonedDateTime, and unit-by-unit forDuration(soPT1HandPT60Mstay different).JSC::temporalType(JSValue)is JSC's classifier for these eight classes (returnsTemporalType::Nonefor anything else); Bun already uses it for TOML serialization.Entry points probed on bun 1.4.0 before the change
Value-blind (a different Temporal value on the expected side was accepted):
toMatchObject, nested property and top leveltoMatchObjectwith a plain object received and a Temporal expected valuetoMatchObjectacross classes (PlainDateTimereceived,PlainDateexpected, same fields)toMatchObjectonDurationPT60MvsPT1H.not.toMatchObjectBun.deepMatchtoMatchSnapshot(propertyMatchers)expect.objectContaining(temporalValue)used as the whole patternAlready correct: a Temporal value nested inside an
expect.objectContaining({...})pattern, because nestedobjectContainingproperties are compared withdeepEquals, which #37024 fixed.[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file