From 0f96c1a584deeae8c5cb812dde42b7b3fa4b4ff0 Mon Sep 17 00:00:00 2001 From: Gourav Kumar Dhiman Date: Fri, 23 Oct 2026 18:42:49 +0530 Subject: [PATCH 1/4] fix(store): match Set elements once in compareDeep compareDeep let several elements of one Set match the same element of the other, and kept failed pair comparisons in its circular-reference cache, so a later comparison of that pair returned true. Match each element at most once and only treat pairs still being compared as equal. Fixes #399 Co-Authored-By: Claude Opus 5.5 --- .changeset/fix-compare-deep-sets.md | 5 +++ packages/store/src/compare.ts | 46 +++++++++++++++++++++------- packages/store/tests/compare.spec.ts | 35 +++++++++++++++++++++ 3 files changed, 75 insertions(+), 11 deletions(-) create mode 100644 .changeset/fix-compare-deep-sets.md diff --git a/.changeset/fix-compare-deep-sets.md b/.changeset/fix-compare-deep-sets.md new file mode 100644 index 00000000..d39dead8 --- /dev/null +++ b/.changeset/fix-compare-deep-sets.md @@ -0,0 +1,5 @@ +--- +'@tanstack/store': patch +--- + +fix(store): `compareDeep` no longer reports unequal Sets as equal. Each element is matched at most once, and failed comparisons are no longer cached as equal diff --git a/packages/store/src/compare.ts b/packages/store/src/compare.ts index 36404460..a285e8df 100644 --- a/packages/store/src/compare.ts +++ b/packages/store/src/compare.ts @@ -25,20 +25,38 @@ function _evaluate( return false } - // guards against circular references - if (config.mode === 'deep') { - let seenB = seen.get(objA as object) + if (config.mode === 'shallow') { + return _evaluateObjects(objA, objB, config, seen) + } - if (seenB?.has(objB as object)) return true + // guards against circular references: a pair that is still being compared + // further up the stack is assumed equal. The pair is released afterwards so + // a failed comparison (e.g. while matching Set elements) is not remembered + // as a success. + let seenB = seen.get(objA as object) - if (!seenB) { - seenB = new WeakSet() - seen.set(objA as object, seenB) - } + if (seenB?.has(objB as object)) return true + + if (!seenB) { + seenB = new WeakSet() + seen.set(objA as object, seenB) + } - seenB.add(objB as object) + seenB.add(objB as object) + + try { + return _evaluateObjects(objA, objB, config, seen) + } finally { + seenB.delete(objB as object) } +} +function _evaluateObjects( + objA: T & object, + objB: T & object, + config: { mode: 'shallow' | 'deep' }, + seen: WeakMap>, +): boolean { // guards against runtime cross type evaluation if (Object.getPrototypeOf(objA) !== Object.getPrototypeOf(objB)) { return false @@ -84,9 +102,15 @@ function _evaluate( if (objA.size !== objB.size) return false if (config.mode === 'deep') { + // each element of objB may only be matched once + const unmatched = [...objB] for (const v of objA) { - if (![...objB].some((bv) => _evaluate(v, bv, config, seen))) - return false + let index = unmatched.indexOf(v) + if (index === -1) { + index = unmatched.findIndex((bv) => _evaluate(v, bv, config, seen)) + } + if (index === -1) return false + unmatched.splice(index, 1) } } diff --git a/packages/store/tests/compare.spec.ts b/packages/store/tests/compare.spec.ts index dcb479b5..51f450a8 100644 --- a/packages/store/tests/compare.spec.ts +++ b/packages/store/tests/compare.spec.ts @@ -420,6 +420,41 @@ describe('evaluate', () => { ).toEqual(true) }) + it('should match each Set element at most once', () => { + expect( + compareDeep( + new Set([{ x: 1 }, { x: 1 }]), + new Set([{ x: 1 }, { x: 2 }]), + ), + ).toEqual(false) + expect( + compareDeep( + new Set([{ x: 1 }, { x: 2 }]), + new Set([{ x: 1 }, { x: 1 }]), + ), + ).toEqual(false) + expect( + compareDeep( + new Set([{ x: 2 }, { x: 1 }]), + new Set([{ x: 1 }, { x: 2 }]), + ), + ).toEqual(true) + }) + + it('should not remember failed Set element comparisons as equal', () => { + const p = { x: 1 } + const q = { x: 2 } + + // While matching the Sets, `p` is first tried against `q2` and fails. + // That failed attempt must not make the later `p` vs `q2` check pass. + const q2 = { x: 2 } + const p2 = { x: 1 } + + expect( + compareDeep([new Set([p, q]), p], [new Set([q2, p2]), q2]), + ).toEqual(false) + }) + it('should test equality between File objects', () => { const file1 = new File(['hello'], 'hello.txt', { type: 'text/plain', From e4c54eb42c311fc651e72f335cad9383b114192d Mon Sep 17 00:00:00 2001 From: Gourav Kumar Dhiman Date: Fri, 23 Oct 2026 21:11:44 +0530 Subject: [PATCH 2/4] fix(store): remember completed equal pairs in compareDeep Releasing every pair after it is compared made shared children compare again on each path to them, which is exponential for a deep DAG. Cache pairs proven equal, but only when the result did not rely on assuming an in-progress (circular) pair equal. Co-Authored-By: Claude Opus 5.5 --- packages/store/src/compare.ts | 65 ++++++++++++++++++++-------- packages/store/tests/compare.spec.ts | 11 +++++ 2 files changed, 59 insertions(+), 17 deletions(-) diff --git a/packages/store/src/compare.ts b/packages/store/src/compare.ts index a285e8df..2c8e17a8 100644 --- a/packages/store/src/compare.ts +++ b/packages/store/src/compare.ts @@ -1,16 +1,44 @@ export function compareShallow(objA: T, objB: T): boolean { - return _evaluate(objA, objB, { mode: 'shallow' }, new WeakMap()) + return _evaluate(objA, objB, { mode: 'shallow' }, createSeen()) } export function compareDeep(objA: T, objB: T): boolean { - return _evaluate(objA, objB, { mode: 'deep' }, new WeakMap()) + return _evaluate(objA, objB, { mode: 'deep' }, createSeen()) +} + +type PairSet = WeakMap> + +interface Seen { + // pairs still being compared further up the stack + inProgress: PairSet + // pairs proven equal without assuming an in-progress pair + equal: PairSet + // how many times an in-progress pair was assumed equal + assumptions: number +} + +function createSeen(): Seen { + return { inProgress: new WeakMap(), equal: new WeakMap(), assumptions: 0 } +} + +function hasPair(pairs: PairSet, a: object, b: object): boolean { + return pairs.get(a)?.has(b) ?? false +} + +function addPair(pairs: PairSet, a: object, b: object): void { + let set = pairs.get(a) + if (!set) { + set = new WeakSet() + pairs.set(a, set) + } + set.add(b) } function _evaluate( objA: T, objB: T, config: { mode: 'shallow' | 'deep' }, - seen: WeakMap>, + seen: Seen, ): boolean { if (Object.is(objA, objB)) { return true @@ -29,25 +57,28 @@ function _evaluate( return _evaluateObjects(objA, objB, config, seen) } - // guards against circular references: a pair that is still being compared - // further up the stack is assumed equal. The pair is released afterwards so - // a failed comparison (e.g. while matching Set elements) is not remembered - // as a success. - let seenB = seen.get(objA as object) - - if (seenB?.has(objB as object)) return true + if (hasPair(seen.equal, objA, objB)) return true - if (!seenB) { - seenB = new WeakSet() - seen.set(objA as object, seenB) + // guards against circular references: a pair that is still being compared + // further up the stack is assumed equal. A result that relied on such an + // assumption is not cached, so a failed comparison (e.g. while matching Set + // elements) is never remembered as a success. + if (hasPair(seen.inProgress, objA, objB)) { + seen.assumptions++ + return true } - seenB.add(objB as object) + addPair(seen.inProgress, objA, objB) + const assumptions = seen.assumptions try { - return _evaluateObjects(objA, objB, config, seen) + const result = _evaluateObjects(objA, objB, config, seen) + if (result && seen.assumptions === assumptions) { + addPair(seen.equal, objA, objB) + } + return result } finally { - seenB.delete(objB as object) + seen.inProgress.get(objA)?.delete(objB) } } @@ -55,7 +86,7 @@ function _evaluateObjects( objA: T & object, objB: T & object, config: { mode: 'shallow' | 'deep' }, - seen: WeakMap>, + seen: Seen, ): boolean { // guards against runtime cross type evaluation if (Object.getPrototypeOf(objA) !== Object.getPrototypeOf(objB)) { diff --git a/packages/store/tests/compare.spec.ts b/packages/store/tests/compare.spec.ts index 51f450a8..8c9290e9 100644 --- a/packages/store/tests/compare.spec.ts +++ b/packages/store/tests/compare.spec.ts @@ -455,6 +455,17 @@ describe('evaluate', () => { ).toEqual(false) }) + it('should compare a shared child only once', () => { + const build = () => { + let node: object = { x: 1 } + for (let i = 0; i < 30; i++) node = { left: node, right: node } + return node + } + + // 2^30 comparisons if completed pairs are not remembered + expect(compareDeep(build(), build())).toEqual(true) + }) + it('should test equality between File objects', () => { const file1 = new File(['hello'], 'hello.txt', { type: 'text/plain', From 83f5cd802035dd5a1c585985679ce2402b03e096 Mon Sep 17 00:00:00 2001 From: Gourav Kumar Dhiman Date: Sat, 24 Oct 2026 16:07:59 +0530 Subject: [PATCH 3/4] fix(store): find a Set matching that needs an earlier match to move Taking the first deep match for each Set element could leave a later element without one, even when a one-to-one matching exists, if a custom equals is not transitive. Use augmenting paths, comparing each pair at most once and only when it is needed. Co-Authored-By: Claude Opus 5.5 --- packages/store/src/compare.ts | 51 +++++++++++++++++++++++----- packages/store/tests/compare.spec.ts | 24 +++++++++++++ 2 files changed, 67 insertions(+), 8 deletions(-) diff --git a/packages/store/src/compare.ts b/packages/store/src/compare.ts index 2c8e17a8..5fda858f 100644 --- a/packages/store/src/compare.ts +++ b/packages/store/src/compare.ts @@ -133,15 +133,50 @@ function _evaluateObjects( if (objA.size !== objB.size) return false if (config.mode === 'deep') { - // each element of objB may only be matched once - const unmatched = [...objB] - for (const v of objA) { - let index = unmatched.indexOf(v) - if (index === -1) { - index = unmatched.findIndex((bv) => _evaluate(v, bv, config, seen)) + // Match each element of objA to its own element of objB. A later + // element may take an earlier one's match if that one can move to + // another (augmenting paths), so a custom non-transitive `equals` + // cannot hide a valid matching. Pairs are compared lazily, once. + const values = [...objA] + const others = [...objB] + const owner: Array = others.map(() => -1) + const compared = new Map() + const matches = (value: number, index: number): boolean => { + const key = value * others.length + index + let result = compared.get(key) + if (result === undefined) { + result = + values[value] === others[index] || + _evaluate(values[value], others[index], config, seen) + compared.set(key, result) + } + return result + } + const assign = (value: number, visited: Array): boolean => { + for (let index = 0; index < others.length; index++) { + if (visited[index] || !matches(value, index)) continue + visited[index] = true + const current = owner[index] ?? -1 + if (current === -1 || assign(current, visited)) { + owner[index] = value + return true + } } - if (index === -1) return false - unmatched.splice(index, 1) + return false + } + for (let value = 0; value < values.length; value++) { + const same = others.indexOf(values[value]) + if (same !== -1 && owner[same] === -1) { + owner[same] = value + continue + } + if ( + !assign( + value, + others.map(() => false), + ) + ) + return false } } diff --git a/packages/store/tests/compare.spec.ts b/packages/store/tests/compare.spec.ts index 8c9290e9..d2219a91 100644 --- a/packages/store/tests/compare.spec.ts +++ b/packages/store/tests/compare.spec.ts @@ -441,6 +441,30 @@ describe('evaluate', () => { ).toEqual(true) }) + it('should find a Set matching that needs an earlier match to move', () => { + // `equals` that is not transitive: a1 equals b1 and b2, a2 only b1 + class Item { + constructor( + readonly name: string, + readonly peers: Array, + ) {} + equals(other: Item) { + return ( + this.peers.includes(other.name) || other.peers.includes(this.name) + ) + } + } + const a1 = new Item('a1', ['b1', 'b2']) + const a2 = new Item('a2', ['b1']) + const b1 = new Item('b1', []) + const b2 = new Item('b2', []) + + expect(compareDeep(new Set([a1, a2]), new Set([b1, b2]))).toEqual(true) + expect( + compareDeep(new Set([a2, new Item('a3', ['b1'])]), new Set([b1, b2])), + ).toEqual(false) + }) + it('should not remember failed Set element comparisons as equal', () => { const p = { x: 1 } const q = { x: 2 } From e4861a1a0d6e910c1988238516eed3a2650bd8b7 Mon Sep 17 00:00:00 2001 From: Gourav Kumar Dhiman Date: Sun, 25 Oct 2026 08:26:06 +0530 Subject: [PATCH 4/4] fix(store): cache pairs whose circular assumptions are resolved A single counter of circular assumptions stopped every enclosing pair from being cached, so a shared self-referencing child was compared again on every path. Record the stack depth each in-progress pair was assumed at, and cache a pair when it assumed only itself or pairs below it. Also look up direct Set matches with a Map instead of indexOf. Co-Authored-By: Claude Opus 5.5 --- packages/store/src/compare.ts | 58 ++++++++++++++++++++-------- packages/store/tests/compare.spec.ts | 13 +++++++ 2 files changed, 54 insertions(+), 17 deletions(-) diff --git a/packages/store/src/compare.ts b/packages/store/src/compare.ts index 5fda858f..b9287d57 100644 --- a/packages/store/src/compare.ts +++ b/packages/store/src/compare.ts @@ -9,16 +9,23 @@ export function compareDeep(objA: T, objB: T): boolean { type PairSet = WeakMap> interface Seen { - // pairs still being compared further up the stack - inProgress: PairSet - // pairs proven equal without assuming an in-progress pair + // stack depth of each pair still being compared further up the stack + inProgress: WeakMap> + // pairs proven equal equal: PairSet - // how many times an in-progress pair was assumed equal - assumptions: number + // depth of the pair being compared + depth: number + // shallowest in-progress pair assumed equal below the current one + lowestAssumed: number } function createSeen(): Seen { - return { inProgress: new WeakMap(), equal: new WeakMap(), assumptions: 0 } + return { + inProgress: new WeakMap(), + equal: new WeakMap(), + depth: 0, + lowestAssumed: Infinity, + } } function hasPair(pairs: PairSet, a: object, b: object): boolean { @@ -60,25 +67,41 @@ function _evaluate( if (hasPair(seen.equal, objA, objB)) return true // guards against circular references: a pair that is still being compared - // further up the stack is assumed equal. A result that relied on such an - // assumption is not cached, so a failed comparison (e.g. while matching Set - // elements) is never remembered as a success. - if (hasPair(seen.inProgress, objA, objB)) { - seen.assumptions++ + // further up the stack is assumed equal. A pair is cached as equal only if + // every pair it assumed was itself or one below it, all now complete, so a + // failed comparison (e.g. while matching Set elements) is never remembered + // as a success. + const assumedDepth = seen.inProgress.get(objA)?.get(objB) + if (assumedDepth !== undefined) { + seen.lowestAssumed = Math.min(seen.lowestAssumed, assumedDepth) return true } - addPair(seen.inProgress, objA, objB) - const assumptions = seen.assumptions + let pairs = seen.inProgress.get(objA) + if (!pairs) { + pairs = new Map() + seen.inProgress.set(objA, pairs) + } + const depth = ++seen.depth + pairs.set(objB, depth) + const outerLowestAssumed = seen.lowestAssumed + seen.lowestAssumed = Infinity try { const result = _evaluateObjects(objA, objB, config, seen) - if (result && seen.assumptions === assumptions) { + if (result && seen.lowestAssumed >= depth) { addPair(seen.equal, objA, objB) } return result } finally { - seen.inProgress.get(objA)?.delete(objB) + pairs.delete(objB) + seen.depth-- + // assumptions about pairs above this one still apply to them + const innerLowestAssumed = seen.lowestAssumed + seen.lowestAssumed = Math.min( + outerLowestAssumed, + innerLowestAssumed < depth ? innerLowestAssumed : Infinity, + ) } } @@ -139,6 +162,7 @@ function _evaluateObjects( // cannot hide a valid matching. Pairs are compared lazily, once. const values = [...objA] const others = [...objB] + const indexOfOther = new Map(others.map((other, index) => [other, index])) const owner: Array = others.map(() => -1) const compared = new Map() const matches = (value: number, index: number): boolean => { @@ -165,8 +189,8 @@ function _evaluateObjects( return false } for (let value = 0; value < values.length; value++) { - const same = others.indexOf(values[value]) - if (same !== -1 && owner[same] === -1) { + const same = indexOfOther.get(values[value]) + if (same !== undefined && owner[same] === -1) { owner[same] = value continue } diff --git a/packages/store/tests/compare.spec.ts b/packages/store/tests/compare.spec.ts index d2219a91..1590e961 100644 --- a/packages/store/tests/compare.spec.ts +++ b/packages/store/tests/compare.spec.ts @@ -441,6 +441,19 @@ describe('evaluate', () => { ).toEqual(true) }) + it('should compare a shared self-referencing child only once', () => { + const build = () => { + const leaf: Record = {} + leaf.self = leaf + let node: object = leaf + for (let i = 0; i < 30; i++) node = { left: node, right: node } + return node + } + + // 2^30 comparisons if a pair that assumed only itself is not cached + expect(compareDeep(build(), build())).toEqual(true) + }) + it('should find a Set matching that needs an earlier match to move', () => { // `equals` that is not transitive: a1 equals b1 and b2, a2 only b1 class Item {