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.

Fixes: nodejs#13347
Refs: nodejs#12142

Author: josephg

lib/assert.js

@@ -285,7 +285,7 @@ function _deepEqual(actual, expected, strict, memos) {
   return areEq;
 }
 
-function setHasSimilarElement(set, val1, strict, memo) {
+function setHasSimilarElement(set, val1, bEntriesUsed, strict, memo) {
   if (set.has(val1))
     return true;
 
@@ -296,8 +296,14 @@ function setHasSimilarElement(set, val1, strict, memo) {
 
   // Otherwise go looking.
   for (const val2 of set) {
-    if (_deepEqual(val1, val2, strict, memo))
+    if (bEntriesUsed && bEntriesUsed.has(val2))
+      continue;
+
+    if (_deepEqual(val1, val2, strict, memo)) {
+      if (bEntriesUsed)
+        bEntriesUsed.add(val2);
       return true;
+    }
   }
 
   return false;
@@ -314,21 +320,29 @@ 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. Initialized lazily so sets which only have value types can
+  // skip an extra allocation.
+  let bEntriesUsed = null;
+
   for (const val1 of a) {
     // 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 (bEntriesUsed == null && typeof val1 === 'object')
+      bEntriesUsed = new Set();
+
+    if (!setHasSimilarElement(b, val1, bEntriesUsed, strict, memo)) {
       return false;
     }
   }
 
   return true;
 }
 
-function mapHasSimilarEntry(map, key1, item1, strict, memo) {
+function mapHasSimilarEntry(map, key1, item1, bEntriesUsed, strict, memo) {
   // To be able to handle cases like:
   //   Map([[1, 'a'], ['1', 'b']]) vs Map([['1', 'a'], [1, 'b']])
   // or:
@@ -349,8 +363,13 @@ function mapHasSimilarEntry(map, key1, item1, strict, memo) {
     if (key2 === key1)
       continue;
 
+    if (bEntriesUsed && bEntriesUsed.has(key2))
+      continue;
+
     if (_deepEqual(key1, key2, strict, memo) &&
         _deepEqual(item1, item2, strict, memo)) {
+      if (bEntriesUsed)
+        bEntriesUsed.add(key2);
       return true;
     }
   }
@@ -366,10 +385,15 @@ function mapEquiv(a, b, strict, memo) {
   if (a.size !== b.size)
     return false;
 
+  let bEntriesUsed = null;
+
   for (const [key1, item1] of a) {
+    if (bEntriesUsed == null && typeof key1 === 'object')
+      bEntriesUsed = 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, bEntriesUsed, strict, memo))
       return false;
   }
 

test/parallel/test-assert-deep.js

@@ -187,6 +187,20 @@ assertOnlyDeepEqual(new Map([['a', '1']]), new Map([['a', 1]]));
 
 assertDeepAndStrictEqual(new Set([{}]), new Set([{}]));
 
+// Discussion of these test cases here - 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]])
+);
+
 // This is an awful case, where a map contains multiple equivalent keys:
 assertOnlyDeepEqual(
   new Map([[1, 'a'], ['1', 'b']]),