Uh oh!
There was an error while loading. Please reload this page.
Fix Duration Hash.symbol violating the Hash/Equal contract - #7153
Conversation
Duration.value is a tagged union that can represent the same span of time as either Millis or Nanos (e.g. seconds(5) vs nanos(5_000_000_000n)), and Duration.equals/Equal.equals correctly normalize both representations before comparing. But DurationProto[Hash.symbol] hashed the raw tagged value directly, so two durations that compare as equal could hash differently -- which Equal.ts's own generic comparison exploits as a fast-path (Hash.hash(self) !== Hash.hash(that) short-circuits to false before ever calling [Equal.symbol]), and which silently breaks HashSet/ HashMap lookups keyed by Duration. Hash the canonical nanoseconds form for finite durations instead. Infinity/NegativeInfinity have no numeric representation to normalize, but each already has exactly one tagged form, so their existing structural hash is unaffected.
🦋 Changeset detectedLatest commit: 511a59a The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Duration hash fix —
DurationProto[Hash.symbol]now hashes the canonical nanoseconds form (Hash.hash(toNanosUnsafe(this))) for finite durations instead of the rawMillis/Nanostag structure, whileInfinity/NegativeInfinitykeep their structural hash. This aligns the hash with the normalized basisEquivalencealready compares on. - Regression test — new test asserting
Duration.equals,Equal.equals, equalHash.hash, and aHashSetlookup across theMillis/Nanosrepresentations. - Changeset —
patchbump that accurately describes the behavior fix.
I verified the fix end-to-end: with it applied, all 44 Duration.test.ts tests pass and oxlint is clean; I confirmed the diagnosis in the PR description is correct (Equal.ts:232 short-circuits to false on a hash mismatch, so Equal.equals/HashSet/HashMap disagreed with Duration.equals), and temporarily reverting the change makes the new test fail exactly at the Equal.equals assertion, so the coverage is genuine. The hash change is consistent with equals in every case: millis-vs-millis, nanos-vs-nanos, and cross-representation comparisons all funnel through the same roundMillisToNanos canonicalization, so equal durations always produce equal hashes (collisions where distinct durations normalize to the same nanos are inherent to the chosen equality basis and correct). Nice, minimal fix.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Overflow guard for the
Millishashing path — the[Hash.symbol]switch now returnsHash.number(millis)whenmillis * 1_000_000overflowsNumber.MAX_VALUE, instead of funneling throughroundMillisToNanos/toNanosUnsafe. I confirmed the prior code crashed withRangeError(BigInt(Infinity)) onHash.hash(Duration.millis(1e303)); the new guard boundary matches exactly where that bigint conversion throws, and equality for such a duration against anyNanossibling throws rather than returningtrue, so no equal pair can span the fallback — the Hash/Equal invariant is preserved in every reachable case. - Extended contract test — added negative cross-representation (
millis(-5000)vsnanos(-5_000_000_000n)), a determinism assertion forinfinity's structural hash, and anegativeInfinity ≠ infinitycheck. - Overflow regression test —
Duration.millis(1e303)self-equality plusHashSetround-trip; this genuinely fails (throws) on the prior commit's code.
Verified locally: 45/45 Duration.test.ts and 26/26 HashSet.test.ts pass; oxlint and dprint check clean on the changed files. Nice, tightly-scoped follow-up.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Avoided duplicate nanos conversion in the
Millishashing path — commit511a59arefactors the[Hash.symbol]Milliscase to computenanos = this.value.millis * 1_000_000once and callHash.hash(roundTiesAwayFromZero(nanos))directly (reusing the samenanosin theNumber.isFiniteguard) instead of going through theroundMillisToNanoswrapper.
I confirmed this preserves behavior exactly: roundMillisToNanos(millis) is defined as roundTiesAwayFromZero(millis * 1_000_000) (Duration.ts:41), so the emitted hash, the overflow guard, and the Hash.number(millis) fallback are byte-identical to the previous commit's output. 45/45Duration.test.ts tests pass, and oxlint + dprint are clean on the changed file. The refactor also marginally tightens correctness since the guard and the rounding now provably see the same nanos value.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Uh oh!
There was an error while loading. Please reload this page.
Summary
Duration.valueis a tagged union that can represent the same span of time as eitherMillisorNanos(e.g.Duration.seconds(5)→{ _tag: "Millis", millis: 5000 },Duration.nanos(5_000_000_000n)→{ _tag: "Nanos", nanos: 5000000000n }), andDuration.equals/Equal.equalscorrectly normalize both before comparing. ButDurationProto[Hash.symbol]hashes the raw taggedvaluedirectly:Equal.ts's own generic comparison documents and relies on the contract (Hash.hash(self) !== Hash.hash(that)short-circuits tofalsebefore ever calling[Equal.symbol]), so this silently breaksHashSet/HashMaplookups keyed byDurationwhenever the same duration was constructed via a different code path than the one used to build/insert into the set/map — a very ordinary occurrence, since library internals mixDuration.millis,Duration.seconds, andDuration.nanosfreely.Changes
packages/effect/src/Duration.ts:[Hash.symbol]now hashes the canonical nanoseconds form (toNanosUnsafe) for finite durations, matching the basisEquivalence/matchPairalready normalize to for cross-representation comparisons.Infinity/NegativeInfinitykeep their existing structural hash, since they have no numeric representation to normalize and each already has exactly one tagged form.packages/effect/test/Duration.test.ts: one new test confirmingHash.symbolagrees withequals/Equal.equalsacross representations, and that aHashSetlookup succeeds across them.Testing
Duration.test.ts: 44/44 passing (up from 43).Equal.equalsfast-path check, before even reaching my ownHash.hashassertion) and passes with the fix.Duration.test.ts/HashSet.test.ts: 70/70 passing, no regressions.tsc -b,oxlint,dprint checkclean.