fix(store): match Set elements once in compareDeep

.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

packages/store/src/compare.ts

@@ -1,16 +1,51 @@
 export function compareShallow<T>(objA: T, objB: T): boolean {
-  return _evaluate(objA, objB, { mode: 'shallow' }, new WeakMap())
+  return _evaluate(objA, objB, { mode: 'shallow' }, createSeen())
 }
 
 export function compareDeep<T>(objA: T, objB: T): boolean {
-  return _evaluate(objA, objB, { mode: 'deep' }, new WeakMap())
+  return _evaluate(objA, objB, { mode: 'deep' }, createSeen())
+}
+
+type PairSet = WeakMap<object, WeakSet<object>>
+
+interface Seen {
+  // stack depth of each pair still being compared further up the stack
+  inProgress: WeakMap<object, Map<object, number>>
+  // pairs proven equal
+  equal: PairSet
+  // 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(),
+    depth: 0,
+    lowestAssumed: Infinity,
+  }
+}
+
+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<T>(
   objA: T,
   objB: T,
   config: { mode: 'shallow' | 'deep' },
-  seen: WeakMap<object, WeakSet<object>>,
+  seen: Seen,
 ): boolean {
   if (Object.is(objA, objB)) {
     return true
@@ -25,20 +60,57 @@ function _evaluate<T>(
     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
+  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 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
+  }
 
-    seenB.add(objB as object)
+  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.lowestAssumed >= depth) {
+      addPair(seen.equal, objA, objB)
+    }
+    return result
+  } finally {
+    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,
+    )
   }
+}
 
+function _evaluateObjects<T>(
+  objA: T & object,
+  objB: T & object,
+  config: { mode: 'shallow' | 'deep' },
+  seen: Seen,
+): boolean {
   // guards against runtime cross type evaluation
   if (Object.getPrototypeOf(objA) !== Object.getPrototypeOf(objB)) {
     return false
@@ -84,8 +156,50 @@ function _evaluate<T>(
     if (objA.size !== objB.size) return false
 
     if (config.mode === 'deep') {
-      for (const v of objA) {
-        if (![...objB].some((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 indexOfOther = new Map(others.map((other, index) => [other, index]))
+      const owner: Array<number> = others.map(() => -1)
+      const compared = new Map<number, boolean>()
+      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>): 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
+          }
+        }
+        return false
+      }
+      for (let value = 0; value < values.length; value++) {
+        const same = indexOfOther.get(values[value])
+        if (same !== undefined && owner[same] === -1) {
+          owner[same] = value
+          continue
+        }
+        if (
+          !assign(
+            value,
+            others.map(() => false),
+          )
+        )
           return false
       }
     }

packages/store/tests/compare.spec.ts

@@ -420,6 +420,89 @@ 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 compare a shared self-referencing child only once', () => {
+      const build = () => {
+        const leaf: Record<string, unknown> = {}
+        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 {
+        constructor(
+          readonly name: string,
+          readonly peers: Array<string>,
+        ) {}
+        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 }
+
+      // 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 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',