assert: fix deepEqual similar sets and maps bug

This fixes a bug where deepEqual and deepStrictEqual would have incorrect behaviour in sets and maps containing multiple equivalent keys. PR-URL: #13426 Fixes: #13347 Refs: #12142 Reviewed-By: Refael Ackermann <[email protected]>
nodejs · Jun 7, 2017 · b8e90dd · b8e90dd
1 parent 2e3b758
commit b8e90dd
Show file tree

Hide file tree

Showing 2 changed files with 64 additions and 8 deletions.
diff --git a/lib/assert.js b/lib/assert.js
@@ -285,9 +285,12 @@ function _deepEqual(actual, expected, strict, memos) {
   return areEq;
 }
 
-function setHasSimilarElement(set, val1, strict, memo) {
-  if (set.has(val1))
+function setHasSimilarElement(set, val1, usedEntries, strict, memo) {
+  if (set.has(val1)) {
+    if (usedEntries)
+      usedEntries.add(val1);
     return true;
+  }
 
   // In strict mode the only things which can match a primitive or a function
   // will already be detected by set.has(val1).
@@ -296,8 +299,14 @@ function setHasSimilarElement(set, val1, strict, memo) {
 
   // Otherwise go looking.
   for (const val2 of set) {
-    if (_deepEqual(val1, val2, strict, memo))
+    if (usedEntries && usedEntries.has(val2))
+      continue;
+
+    if (_deepEqual(val1, val2, strict, memo)) {
+      if (usedEntries)
+        usedEntries.add(val2);
       return true;
+    }
   }
 
   return false;
@@ -314,21 +323,33 @@ function setEquiv(a, b, strict, memo) {
   if (a.size !== b.size)
     return false;
 
+  // This is a set of the entries in b which have been consumed in our pairwise
+  // comparison.
+  //
+  // When the sets contain only value types (eg, lots of numbers), and we're in
+  // strict mode, we don't need to match off the entries in a pairwise way. In
+  // that case this initialization is done lazily to avoid the allocation &
+  // bookkeeping cost. Unfortunately, we can't get away with that in non-strict
+  // mode.
+  let usedEntries = null;
+
   for (const val1 of a) {
+    if (usedEntries == null && (!strict || typeof val1 === 'object'))
+      usedEntries = new Set();
+
     // If the value doesn't exist in the second set by reference, and its an
     // object or an array we'll need to go hunting for something thats
     // deep-equal to it. Note that this is O(n^2) complexity, and will get
     // slower if large, very similar sets / maps are nested inside.
     // Unfortunately there's no real way around this.
-    if (!setHasSimilarElement(b, val1, strict, memo)) {
+    if (!setHasSimilarElement(b, val1, usedEntries, strict, memo))
       return false;
-    }
   }
 
   return true;
 }
 
-function mapHasSimilarEntry(map, key1, item1, strict, memo) {
+function mapHasSimilarEntry(map, key1, item1, usedEntries, strict, memo) {
   // To be able to handle cases like:
   //   Map([[1, 'a'], ['1', 'b']]) vs Map([['1', 'a'], [1, 'b']])
   // or:
@@ -338,8 +359,11 @@ function mapHasSimilarEntry(map, key1, item1, strict, memo) {
   // This check is not strictly necessary. The loop performs this check, but
   // doing it here improves performance of the common case when reference-equal
   // keys exist (which includes all primitive-valued keys).
-  if (map.has(key1) && _deepEqual(item1, map.get(key1), strict, memo))
+  if (map.has(key1) && _deepEqual(item1, map.get(key1), strict, memo)) {
+    if (usedEntries)
+      usedEntries.add(key1);
     return true;
+  }
 
   if (strict && (util.isPrimitive(key1) || util.isFunction(key1)))
     return false;
@@ -349,8 +373,13 @@ function mapHasSimilarEntry(map, key1, item1, strict, memo) {
     if (key2 === key1)
       continue;
 
+    if (usedEntries && usedEntries.has(key2))
+      continue;
+
     if (_deepEqual(key1, key2, strict, memo) &&
         _deepEqual(item1, item2, strict, memo)) {
+      if (usedEntries)
+        usedEntries.add(key2);
       return true;
     }
   }
@@ -366,10 +395,15 @@ function mapEquiv(a, b, strict, memo) {
   if (a.size !== b.size)
     return false;
 
+  let usedEntries = null;
+
   for (const [key1, item1] of a) {
+    if (usedEntries == null && (!strict || typeof key1 === 'object'))
+      usedEntries = new Set();
+
     // Just like setEquiv above, this hunt makes this function O(n^2) when
     // using objects and lists as keys
-    if (!mapHasSimilarEntry(b, key1, item1, strict, memo))
+    if (!mapHasSimilarEntry(b, key1, item1, usedEntries, strict, memo))
       return false;
   }
 

diff --git a/test/parallel/test-assert-deep.js b/test/parallel/test-assert-deep.js
@@ -187,6 +187,28 @@ assertOnlyDeepEqual(new Map([['a', '1']]), new Map([['a', 1]]));
 
 assertDeepAndStrictEqual(new Set([{}]), new Set([{}]));
 
+// Ref: https://github.com/nodejs/node/issues/13347
+assertNotDeepOrStrict(
+  new Set([{a: 1}, {a: 1}]),
+  new Set([{a: 1}, {a: 2}])
+);
+assertNotDeepOrStrict(
+  new Set([{a: 1}, {a: 1}, {a: 2}]),
+  new Set([{a: 1}, {a: 2}, {a: 2}])
+);
+assertNotDeepOrStrict(
+  new Map([[{x: 1}, 5], [{x: 1}, 5]]),
+  new Map([[{x: 1}, 5], [{x: 2}, 5]])
+);
+
+assertNotDeepOrStrict(new Set([3, '3']), new Set([3, 4]));
+assertNotDeepOrStrict(new Map([[3, 0], ['3', 0]]), new Map([[3, 0], [4, 0]]));
+
+assertNotDeepOrStrict(
+  new Set([{a: 1}, {a: 1}, {a: 2}]),
+  new Set([{a: 1}, {a: 2}, {a: 2}])
+);
+
 // This is an awful case, where a map contains multiple equivalent keys:
 assertOnlyDeepEqual(
   new Map([[1, 'a'], ['1', 'b']]),