Compare Temporal objects by value in Bun.deepEquals and toEqual - #37024
Conversation
Temporal objects keep their state in internal slots and have no own
properties, so Bun.deepEquals (and expect().toEqual/toStrictEqual,
which share the implementation) treated any two instances of a class as
equal: deepEquals(PlainDate.from("2020-01-01"),
PlainDate.from("1999-12-31")) was true.
Compare them the way JSDateType compares Dates: same class required,
then the internal fields (exact time for Instant, ISO fields plus
calendar for the Plain types, exact time plus time zone plus calendar
for ZonedDateTime, and field-wise for Duration, so PT1H and PT60M stay
distinct). Extra own properties are ignored, matching Date. Different
Temporal classes never compare equal.
WalkthroughTemporal objects now compare by internal state in deep equality. Tests cover Temporal values, nested structures, differing calendars and time zones, extra properties, and multiple equality APIs. ChangesTemporal equality
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
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/bun-object/deep-equals.spec.ts`:
- Around line 224-233: Expand the “expect().toEqual on Temporal values” tests to
cover direct util.isDeepStrictEqual comparisons for equal and unequal Temporal
values, plus toEqual and toStrictEqual cases where an otherwise equal Temporal
value has an extra own property. Cover the relevant Temporal variants and
preserve the existing equal/unequal value assertions.
- Around line 152-201: Expand the “different values of the same class” matrix to
cover calendar differences for every calendar-bearing Temporal class, not only
Temporal.PlainDate, while keeping the represented date/time values otherwise
equivalent. Add unequal Duration pairs that vary each remaining unit beyond
hours, including minutes, seconds, milliseconds, microseconds, and nanoseconds,
and retain the existing field-wise and sign cases.
🪄 Autofix
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: 112a4c96-da4c-440e-a3c4-cbec32271eab
📒 Files selected for processing (2)
src/jsc/bindings/bindings.cpptest/js/bun/bun-object/deep-equals.spec.ts
Calendar-difference pairs for every calendar-bearing class, per-unit Duration pairs, util.isDeepStrictEqual cases, and extra-own-property parity through toEqual/toStrictEqual.
|
let's check what other deep equals libraries and functions do for Temporal. i haven't seen this before |
|
Surveyed what deep-equality implementations do with Temporal values (probed with @js-temporal/polyfill where the library is runtime-agnostic; Temporal's own valueOf throws by spec, which shapes some results):
The strongest precedent is Deno, the other runtime that ships Temporal: One semantic fork worth your eye: Duration. |
deep-equals.spec.ts returns to its original contents; the Temporal coverage lives in its own dedicated test file beside it.
When the first object is not Temporal but the second is, return false directly instead of relying on the caller's swapped retry.
Expect wrappers not yet finalized at process exit report as direct leaks; the sibling deep-equals/deep-match spec files show the same class under the identical invocation.
|
The 13 x64-asan failure on deep-equals-temporal.test.ts in build 89504 (direct leak of 624b, icu_75::HebrewCalendar::clone under JSC::TemporalCore::buildCalendarTemplate) is an LSan false positive from Temporal's process-lifetime ICU calendar template cache, not something this diff leaks. #37034 adds the missing test/leaksan.supp entry for it (the calendar twin of the existing TemporalCore::withTimeZone suppression). Once that lands, the test/no-validate-leaksan.txt entry for deep-equals-temporal.test.ts here can be dropped so the file keeps leak validation. |
There was a problem hiding this comment.
Beyond the inline finding on the LSAN opt-out, I checked the new temporalObjectsDequal accessors (exactTime(), plainDate(), calendarID(), timeZone(), duration()[i]) — they are pure C++ getters on internal slots with no JS re-entry, so the absence of a ThrowScope/RETURN_IF_EXCEPTION here is correct and matches the JSDateType case. The o1-non-Temporal / o2-Temporal asymmetry dylan-conway raised is closed by the trailing isTemporalObject(o2) check in 2e5de81.
Extended reasoning...
The C++ comparison logic reads sound; the one open item is the whole-file LSAN suppression added in the final commit, which the inline comment covers. A maintainer is already engaged on the Duration semantics question, so deferring rather than approving.
|
Follow-up on the The review is right that the skip-list comment misattributed the leak. The leak CI actually reported (build 89504) was the ICU calendar template cache: The targeted fix is in flight as #37034, which adds |
…e cache (#37034) ### Problem On the `13 x64-asan` lane, a test that exercises non-ISO Temporal calendars from a test callback can abort after a fully green run with a LeakSanitizer report. Seen in build 89504 on #37024, whose `test/js/bun/bun-object/deep-equals-temporal.test.ts` uses `[u-ca=hebrew]`: ``` Direct leak of 624 byte(s) in 1 object(s) allocated from: #1 icu_75::HebrewCalendar::clone() const #2 icu_75::Calendar::createInstance(icu_75::TimeZone*, icu_75::Locale const&, UErrorCode&) #3 ucal_open_75 #4 JSC::TemporalCore::buildCalendarTemplate(WTF::AbstractLocker const&, unsigned int) #5 JSC::TemporalCore::withCalendar<JSC::TemporalCore::calendarYear(...)::$_0>(...) ``` The CI annotation titles this `direct leak of 624b in {closure#0} (src/jsc/JSValue.rs:1664:22)` because that is the first in-repo frame (the test-runner's `JSValue::call`); everything below it is WebKit/ICU. ### Cause `TemporalCore::withCalendar` (`vendor/WebKit/.../temporal/core/CalendarICUBridge.cpp`) keeps up to 8 open `UCalendar` templates in a process-lifetime `LazyNeverDestroyed` `TinyLRUCache`, one per calendar ID (non-ISO arithmetic, plus pure-ISO `PlainDateTime.prototype.with`, which reaches the same path unguarded); LRU eviction `ucal_close`s them, so the set is bounded. The `CalendarCacheEntry` that owns each `UCalendar` is `WTF_MAKE_TZONE_ALLOCATED` (bmalloc), which LSan does not scan, so the libc-allocated `UCalendar` (and the ICU `TimeZone` inside it) is reported as a direct leak even though it is reachable. Whether a given run aborts depends on whether some stale stack or register value still points at the ICU object when LSan scans at exit, hence the intermittence. This is the calendar twin of the already-suppressed `TemporalCore::withTimeZone` entry (same cache design, same TZone-allocated owner). ### Fix - Add a `leak:TemporalCore::buildCalendarTemplate` suppression to `test/leaksan.supp`, mirroring the `withTimeZone` entry. The pattern anchors on the template builder rather than `withCalendar` itself so that a future real leak inside one of the many op lambdas `withCalendar` runs would still be reported; every cached-template allocation carries the builder frame. (`withTimeZone` has no such builder frame, its `ucal_open` is inline, so that entry keeps its existing pattern.) - Drop the `test/no-validate-leaksan.txt` escape hatch #37024 added for `deep-equals-temporal.test.ts`, re-enabling leak validation for it; that file exercises the suppressed path on the asan lane. ### Verification On a debug ASAN build, running `bun test test/js/bun/bun-object/deep-equals-temporal.test.ts` under the CI leak-validation env (`BUN_DESTRUCT_VM_ON_EXIT=1`, `detect_leaks=1:abort_on_error=1`, repo suppression file): - with the new entry: clean exit, 5/5 runs - without it: LSan abort with the calendar-template stacks above, 3/3 runs A standalone probe exercising 8 non-ISO calendars plus pure-ISO `PlainDateTime.with` from a timer callback shows the same split (10/10 aborts without, 10/10 clean with; `print_suppressions=1` attributes exactly the ICU template allocations to the new entry). Top-level module code cannot reproduce this: its allocation stacks carry `JSC::JSModuleLoader::evaluateNonVirtual`, which the suppression file already covers wholesale. An ASAN-gated test pinning the entry was part of an earlier revision and was dropped per review; the re-enabled `deep-equals-temporal.test.ts` covers the path in CI instead. The Expect-wrapper shutdown leak mentioned in the dropped no-validate comment is a separate issue tracked in #32180: that is `bun test`'s own finalizer-owned memory, while this cache deliberately survives VM teardown, so #32180 would not prevent this report. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 1 · docs-only change; test-proof not applicable <!-- robobun:evidence:end --> --------- Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
…gify (#37018) TOML.parse returned all four TOML date/time types as strings of their source text. They now map 1:1 and losslessly onto Temporal, which is enabled by default on main: | TOML | JS | |---|---| | offset date-time (`1979-05-27T00:32:00-07:00`) | `Temporal.Instant` | | local date-time (`1979-05-27T07:32:00`, 1.1's `1979-05-27 07:32`) | `Temporal.PlainDateTime` | | local date (`1979-05-27`) | `Temporal.PlainDate` | | local time (`07:32:00`, `07:32`) | `Temporal.PlainTime` | Per the spec an offset date-time "specifies an instant", so it is `Temporal.Instant` and the written offset normalizes to UTC. Sub-second digits are preserved (Temporal carries nanoseconds; fractional seconds beyond 9 digits are truncated as the TOML spec directs, since Temporal rejects them). Leap-second `:60` clamps to `:59` through Temporal's own ISO parsing. ```js const doc = Bun.TOML.parse("d = 1979-05-27T00:32:00-07:00"); doc.d instanceof Temporal.Instant; // true doc.d.toString(); // "1979-05-27T07:32:00Z" Bun.TOML.stringify(doc); // "d = 1979-05-27T07:32:00Z\n" (unquoted date-time literal) ``` ### How - The TOML parser tags the `E::String` it produces with the date/time kind (`toml_datetime` on `EString`); the lexer already distinguished the four kinds. The TOML AST never enters the JS visit/transform passes, so only the TOML sinks check the tag. - All three sinks of the TOML AST stay consistent: - `Bun.TOML.parse` and `import`/`require` of `.toml` construct the object through the same JSC code paths `Temporal.*.from(string)` uses (new bindings in `bindings.cpp`). - The bundler lowers the node to a `Temporal.*.from("...")` call over a real unbound `Temporal` symbol, so the chunk renamer renames a user binding named `Temporal` instead of letting it capture the reference, and the calls are pure-annotated so unused exports tree-shake. - `bun build --no-bundle` of a `.toml` file (which prints the data AST directly, without a symbol table) prints the tagged string as a bare `Temporal.*.from("...")` call. That path turns each top-level key into a module-scope `var`, so a document with both a date/time and a top-level key literally named `Temporal` fails at evaluation with a `TypeError`; renaming without a symbol table was tried and dropped as open-ended. - `TOML.stringify` emits `Temporal.Instant`/`PlainDateTime`/`PlainDate`/`PlainTime` as unquoted TOML literals, so `stringify(parse(doc))` round-trips date/times. `Temporal.ZonedDateTime` is also accepted and emits the offset at that instant, dropping the `[Time/Zone]` annotation (TOML has no zone syntax); non-ISO `[u-ca=...]` calendar annotations are dropped the same way (the stored ISO fields are emitted). `PlainYearMonth`/`PlainMonthDay`/`Duration` have no TOML form and throw. Values TOML's 4-digit years cannot spell throw like `Date` already did, with one refinement for instants: an offset date-time at a year edge (`0000-01-01T00:00:00+01:00` is valid TOML but its UTC year is -1) is emitted with the nearest offset whose local year fits, so everything `parse` accepts also stringifies; only instants a day or more outside 0000..9999 throw. `Date` output now trims trailing fraction zeros (`Z` rather than `.000Z`) so `Date` and `Instant` spell the same instant identically; otherwise `Date` handling is unchanged. - With `BUN_JSC_useTemporal=0`, a date/time value throws a `TypeError` ("Date/time values require Temporal, which is disabled in this process"); the module-import path previously would have panicked on any conversion error and now fails the load with the pending exception. - bunfig.toml uses no date values, so config parsing is unaffected. Everything else about parse/stringify (error messages, layout, numbers) is byte-for-byte unchanged. ### Tests - Regenerated `toml-test-suite.test.ts` (708 cases) from the same pinned toml-test commit: datetime expectations construct the Temporal value and compare with plain `toEqual` (deepEquals learned Temporal objects in #37024). All pass, including the `parse(stringify(parse(input)))` lap each valid case asserts. - Hand-written coverage in `toml.test.ts`: the four mappings, instant semantics, TOML spellings Temporal does not print (space separator, lowercase `t`/`z`, omitted seconds, leap second), fraction truncation, nesting, both `useTemporal=0` behaviors, stringify of all eight Temporal types, year bounds, calendar/zone annotation dropping, array layout. - Import fixtures (`test/js/bun/resolve/toml`) extended with a `[dates]` table; bundler tests assert the emitted `Temporal.*.from` modules run and that a user `var Temporal` in the same bundle is renamed instead of captured. Types and docs updated (`bun.d.ts` JSDoc, `docs/runtime/toml.mdx`). <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 3 · 25 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 53 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/bundler/bundler_loader.test.ts test/js/bun/resolve/toml/toml.test.js test/js/bun/toml/toml-test-suite.test.ts test/js/bun/toml/toml.test.ts bun test v1.4.0 (48b6af9) test/bundler/bundler_loader.test.ts: (pass) bundler > bun loader > bun/loader-yaml-file [1013.43ms] (pass) bundler > bun loader > bun/loader-text-file [412.02ms] (pass) bundler > bun loader > bun/loader-json-file [489.14ms] (pass) bundler > bun loader > bun/loader-toml-file [429.12ms] (pass) bundler > bun loader > bun/loader-toml-datetime-shadowed-temporal-global [413.44ms] runtime failed file: /tmp/bun-build-tests/bun-avOXit/bun/loader-toml-datetime-imported-temporal-binding/out.js stdout output: polyfill false 1979-05-27 --- expected stdout: polyfill true 1979-05-27 --- 1843 | console.log(`---`); 1844 | console.log(`expected ${name}:`); 1845 | console.log(expected); 1846 | console.log(`---`); 1847 | } 1848 | expect(result).toBe(expected); ^ error: exp ... (truncated) release without fix: 53 FAILED bun test v1.4.0-canary.1 (b7a0431) test/bundler/bundler_loader.test.ts: (pass) bundler > bun loader > bun/loader-yaml-file [31.84ms] (pass) bundler > bun loader > bun/loader-text-file [14.09ms] (pass) bundler > bun loader > bun/loader-json-file [18.21ms] (pass) bundler > bun loader > bun/loader-toml-file [13.89ms] (pass) bundler > bun loader > bun/loader-toml-datetime-shadowed-temporal-global [22.56ms] runtime failed file: /tmp/bun-build-tests/bun-Rboeq6/bun/loader-toml-datetime-imported-temporal-binding/out.js stdout output: polyfill false 1979-05-27 --- expected stdout: polyfill true 1979-05-27 --- 1843 | console.log(`---`); 1844 | console.log(`expected ${name}:`); 1845 | console.log(expected); 1846 | console.log(`---`); 1847 | } 1848 | expect(result).toBe(expected); ^ error: expect(received).toBe(expected) Expected: "polyfill true 1979-05-27" Received: "polyfill false 1979-05-27" at <anonymous> (/workspace/bun/test/bundler/expectBundled.ts:1848:28) (fail) bundler > bun loader > bun/loader-toml-datetime-imported-temporal-binding [18.93ms] 93 ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/bundler/bundler_loader.test.ts test/js/bun/resolve/toml/toml.test.js test/js/bun/toml/toml-test-suite.test.ts test/js/bun/toml/toml.test.ts bun test v1.4.0 (48b6af9) test/bundler/bundler_loader.test.ts: (pass) bundler > bun loader > bun/loader-yaml-file [1011.62ms] (pass) bundler > bun loader > bun/loader-text-file [508.58ms] (pass) bundler > bun loader > bun/loader-json-file [360.53ms] (pass) bundler > bun loader > bun/loader-toml-file [381.91ms] (pass) bundler > bun loader > bun/loader-toml-datetime-shadowed-temporal-global [387.83ms] (pass) bundler > bun loader > bun/loader-toml-datetime-imported-temporal-binding [377.21ms] (pass) bundler > bun loader > bun/loader-toml-datetime-no-bundle [464.49ms] (pass) bundler > bun loader > bun/loader-toml-datetime [433.16ms] (pass) bundler > bun loader > bun/loader-text-file [398.02ms] (pass) bundler > bun loader > bun/loader-xml-file [432.77ms] (pass) bundler > node loader > bun/loader-yaml-file [427.98ms] (pass) bundler > node loader > bun/loader-text-file [385.65ms] (pass) bundler > ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 652ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/23] gen generated_host_exports.rs generated_host_exports.rs: 92 exports (host=3, lazy=10, generic=79, rust=0); 240 extern-C blocks audited [2/23] gen cpp.rs (cppbind) [3/23] gen JS modules (bundle-modules) Preprocess modules (8539ms) Bundle modules (64ms) Postprocesss modules (324ms) Bundle Functions (858ms) Generate Code (29ms) [9.83s] Bundled "src/js" for production 2632 kb 197 internal modules 13 native modules 91 internal functions across 17 files [3/8] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu) nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19) �[1m�[92m Compiling�[0m bun_ast v0.0.0 (/workspace/bun/src/ast) �[1m�[92m Compiling�[0m bun_install_types v0.0.0 (/workspace/bun/src/install_types) �[1m�[92m Compiling�[0m bun_parsers v0.0.0 (/workspace/bun/src/parsers) �[1m�[92m Compiling�[0m bun_react_compiler v0.0.0 (/workspace/bun/src/react_compiler) �[1m�[92m Compiling�[0m bun_css v0.0.0 (/workspace/bun/src/ ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` docs/runtime/toml.mdx | 38 +- packages/bun-types/bun.d.ts | 20 +- src/ast/e.rs | 45 ++ src/ast/expr.rs | 1 + src/bundler/ParseTask.rs | 30 +- src/bundler/bundle_v2.rs | 2 +- src/bundler/transpiler.rs | 1 + src/codegen/cppbind.ts | 1 + src/js_parser/parse/parse_entry.rs | 117 +++ src/js_parser_jsc/expr_jsc.rs | 26 +- src/js_parser_jsc/lib.rs | 3 +- src/js_printer/lib.rs | 21 + src/jsc/JSValue.rs | 19 + src/jsc/bindings/bindings.cpp | 155 ++++ src/jsc/lib.rs | 1 + src/parsers/toml.rs | 129 ++-- src/runtime/api.rs | 63 +- src/runtime/api/TOMLObject.rs | 150 +++- test/bundler/bundler_loader.test.ts | 67 ++ test/js/bun/resolve/toml/toml-fixture.toml | 6 + test/js/bun/resolve/toml/toml-fixture.toml.txt | 6 + test/js/bun/resolve/toml/toml.test.js | 9 + test/js/bun/toml/generate_toml_test_suite.ts | 93 +-- test/js/bun/toml/toml-test-suite.test.ts | 951 ++++++++++++------------- test/js/bun/toml/toml.test.ts | 297 ++++++-- 25 files changed, 1494 insertions(+), 757 deletions(-) ``` </details> **gate history** · 7 passed · 0 rejected · iteration 3 <details><summary>evidence per changed file</summary> ``` file reads edits tests docs/runtime/toml.mdx 1 1 0 packages/bun-types/bun.d.ts 1 1 0 src/ast/e.rs 6 8 0 src/ast/expr.rs 5 6 0 src/bundler/ParseTask.rs 2 1 0 src/bundler/bundle_v2.rs 1 1 0 src/bundler/transpiler.rs 5 7 0 src/codegen/cppbind.ts 0 0 0 src/js_parser/parse/parse_entry.rs 4 10 0 src/js_parser_jsc/expr_jsc.rs 4 7 0 src/js_parser_jsc/lib.rs 1 2 0 src/js_printer/lib.rs 3 7 0 src/jsc/JSValue.rs 2 4 0 src/jsc/bindings/bindings.cpp 7 11 0 src/jsc/lib.rs 0 0 0 src/parsers/toml.rs 5 7 0 (+ 9 more files) ``` </details> <!-- robobun:evidence:end --> --------- Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
Temporal objects keep their state in internal slots and have no own enumerable properties, so the deep-equality own-property walk had nothing to compare and any two instances of a class were equal:
This matters more now that values of these types come out of runtime APIs (#37018 returns them from
Bun.TOML.parse).The fix adds Temporal handling to the shared special-object comparison in
bindings.cpp, following the existingJSDateTypecase (Dates compare by internal number and ignore own properties):Instant: exact time (nanoseconds)PlainDate/PlainDateTime/PlainYearMonth/PlainMonthDay: ISO fields plus calendarPlainTime: time fieldsZonedDateTime: exact time plus time zone plus calendarDuration: field-wise, soPT1HandPT60Mremain distinct valuesDateBun.deepEquals,toEqual,toStrictEqual, andutil.isDeepStrictEqual(all share the template)Tests cover all eight types (equal and unequal pairs in both argument orders), cross-class pairs at the same moment, zone and calendar differences, nesting inside arrays/maps, the own-property parity with
Date, and the expect matchers. The new assertions fail on the current release and pass with this change;expect.test.js(417 tests) and the Temporal suites are unaffected.Follow-up requested in #37018 review; once this lands, the TOML conformance suite generator there goes back to plain
toEqual.[review] gate passed · iteration 1 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 1 rejected · iteration 1
evidence per changed file