assert: refactored based on PR

Refactored setEquiv and mapEquiv based on @joyeecheung's stylistic preference to avoid labels.
nodejs · josephg · Mar 31, 2017 · Mar 31, 2017 · Mar 31, 2017 · Mar 31, 2017
commit 7f9d4d8b13b482f21dce59abfca4be4833eeb446
diff --git a/lib/assert.js b/lib/assert.js
@@ -285,15 +285,25 @@ function _deepEqual(actual, expected, strict, memos) {
   return objEquiv(actual, expected, strict, memos);
 }
 
-function setHasSimilarElement(set, val1, strict, actualVisitedObjects) {
+function setHasSimilarElement(set, val1, strict, memo) {
+  if (set.has(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).
+  if (strict && (util.isPrimitive(val1) || util.isFunction(val1)))
+    return false;
+
+  // Otherwise go looking.
   for (const val2 of set) {
-    if (_deepEqual(val1, val2, strict, actualVisitedObjects))
+    if (_deepEqual(val1, val2, strict, memo))
       return true;
   }
+
   return false;
 }
 
-function setEquiv(a, b, strict, actualVisitedObjects) {
+function setEquiv(a, b, strict, memo) {
   // This code currently returns false for this pair of sets:
   //   assert.deepEqual(new Set(['1', 1]), new Set([1]))
   //
@@ -310,56 +320,57 @@ function setEquiv(a, b, strict, actualVisitedObjects) {
     // 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 (!b.has(val1)
-        && (!strict || (!util.isObject(val1) && !util.isFunction(val1)))
-        && !setHasSimilarElement(b, val1, strict, actualVisitedObjects)) {
+    if (!setHasSimilarElement(b, val1, strict, memo)) {
       return false;
     }
   }
 
   return true;
 }
 
-function mapEquiv(a, b, strict, actualVisitedObjects) {
+function mapHasSimilarEntry(map, key1, item1, strict, memo) {
+  // To be able to handle cases like:
+  //   Map([[1, 'a'], ['1', 'b']]) vs Map([['1', 'a'], [1, 'b']])
+  // or:
+  //   Map([[{}, 'a'], [{}, 'b']]) vs Map([[{}, 'b'], [{}, 'a']])
+  // ... we need to consider *all* matching keys, not just the first we find.
+
+  // 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))
+    return true;
+
+  if (strict && (util.isPrimitive(key1) || util.isFunction(key1)))
+    return false;
+
+  for (const [key2, item2] of map) {
+    // This case is checked above.
+    if (key2 === key1)
+      continue;
+
+    if (_deepEqual(key1, key2, strict, memo) &&
+        _deepEqual(item1, item2, strict, memo)) {
+      return true;
+    }
+  }
+
+  return false;
+}
+
+function mapEquiv(a, b, strict, memo) {
   // Caveat: In non-strict mode, this implementation does not handle cases
   // where maps contain two equivalent-but-not-reference-equal keys.
   //
   // For example, maps like this are currently considered not equivalent:
   if (a.size !== b.size)
     return false;
 
-  outer: for (const [key1, item1] of a) {
-    // To be able to handle cases like:
-    //   Map([[1, 'a'], ['1', 'b']]) vs Map([['1', 'a'], [1, 'b']])
-    // or:
-    //   Map([[{}, 'a'], [{}, 'b']]) vs Map([[{}, 'b'], [{}, 'a']])
-    // ... we need to consider *all* matching keys, not just the first we find.
-
-    // This check is not strictly necessary if we run the loop below, but it
-    // improves performance of the common case when reference-equal keys exist
-    // (which includes all primitive-valued keys).
-    if (b.has(key1)) {
-      if (_deepEqual(item1, b.get(key1), strict, actualVisitedObjects))
-        continue outer;
-    }
-
-    // Hunt for keys which are deep-equal to key1 in b. Just like setEquiv
-    // above, this hunt makes this function O(n^2) when using objects and lists
-    // as keys
-    if (!strict || (typeof key1 === 'object' && key1 !== null)) {
-      for (const [key2, item2] of b) {
-        // Just for performance. We already checked these keys above.
-        if (key2 === key1)
-          continue;
-
-        if (_deepEqual(key1, key2, strict, actualVisitedObjects) &&
-            _deepEqual(item1, item2, strict, actualVisitedObjects)) {
-          continue outer;
-        }
-      }
-    }
-
-    return false;
+  for (const [key1, item1] of a) {
+    // 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))
+      return false;
   }
 
   return true;

diff --git a/test/parallel/test-assert-deep.js b/test/parallel/test-assert-deep.js
@@ -158,6 +158,8 @@ assertOnlyDeepEqual(new Set(['1']), new Set([1]));
 assertOnlyDeepEqual(new Map([['1', 'a']]), new Map([[1, 'a']]));
 assertOnlyDeepEqual(new Map([['a', '1']]), new Map([['a', 1]]));
 
+assertDeepAndStrictEqual(new Set([{}]), new Set([{}]));
+
 // This is an awful case, where a map contains multiple equivalent keys:
 assertOnlyDeepEqual(
   new Map([[1, 'a'], ['1', 'b']]),