assert: add support for Map and Set in deepEqual

assert.deepEqual and assert.deepStrictEqual currently return true for
any pair of Maps and Sets regardless of content. This patch adds
support in deepEqual and deepStrictEqual to verify the contents of Maps
and Sets.

Deeo equivalence checking is currently an
O(n^2) operation, and worse, it gets slower exponentially if maps
and sets were nested.

Note that this change breaks compatibility with previous versions of
deepEqual and deepStrictEqual if consumers were depending on all maps
and sets to be seen as equivalent. The old behaviour was never
documented, but nevertheless there are certainly some tests out there
which depend on it.

Support has stalled because the assert API was frozen, but was recently
unfrozen in CTC#63.

---

Later squashed in:

This change updates the checks for deep equality checking on Map and Set
to check all set values / all map keys to see if any of them match the
expected result.

This change is much slower, but based on the conversation in the pull
request its probably the right approach.

Fixes: #2309
Refs: tape-testing/tape#342
Refs: #2315
Refs: nodejs/CTC#63
PR-URL: #12142
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>

doc/api/assert.md

@@ -18,6 +18,9 @@ An alias of [`assert.ok()`][].
 <!-- YAML
 added: v0.1.21
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/12142
+    description: Set and Map content is also compared
   - version: v6.4.0, v4.7.1
     pr-url: https://github.com/nodejs/node/pull/8002
     description: Typed array slices are handled correctly now.
@@ -40,7 +43,7 @@ Only [enumerable "own" properties][] are considered. The
 [`assert.deepEqual()`][] implementation does not test the
 [`[[Prototype]]`][prototype-spec] of objects, attached symbols, or
 non-enumerable properties — for such checks, consider using
-[assert.deepStrictEqual()][] instead. This can lead to some
+[`assert.deepStrictEqual()`][] instead. This can lead to some
 potentially surprising results. For example, the following example does not
 throw an `AssertionError` because the properties on the [`Error`][] object are
 not enumerable:
@@ -50,6 +53,9 @@ not enumerable:
 assert.deepEqual(Error('a'), Error('b'));
 ```
 
+An exception is made for [`Map`][] and [`Set`][]. Maps and Sets have their
+contained items compared too, as expected.
+
 "Deep" equality means that the enumerable "own" properties of child objects
 are evaluated also:
 
@@ -96,6 +102,9 @@ parameter is undefined, a default error message is assigned.
 <!-- YAML
 added: v1.2.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/12142
+    description: Set and Map content is also compared
   - version: v6.4.0, v4.7.1
     pr-url: https://github.com/nodejs/node/pull/8002
     description: Typed array slices are handled correctly now.
@@ -113,7 +122,8 @@ changes:
 Generally identical to `assert.deepEqual()` with three exceptions:
 
 1. Primitive values are compared using the [Strict Equality Comparison][]
-  ( `===` ).
+  ( `===` ). Set values and Map keys are compared using the [SameValueZero][]
+  comparison. (Which means they are free of the [caveats][]).
 2. [`[[Prototype]]`][prototype-spec] of objects are compared using
   the [Strict Equality Comparison][] too.
 3. [Type tags][Object.prototype.toString()] of objects should be the same.
@@ -576,10 +586,13 @@ For more information, see
 [`assert.ok()`]: #assert_assert_ok_value_message
 [`assert.throws()`]: #assert_assert_throws_block_error_message
 [`Error`]: errors.html#errors_class_error
+[caveats]: #assert_caveats
 [`RegExp`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions
 [`TypeError`]: errors.html#errors_class_typeerror
 [Abstract Equality Comparison]: https://tc39.github.io/ecma262/#sec-abstract-equality-comparison
 [Strict Equality Comparison]: https://tc39.github.io/ecma262/#sec-strict-equality-comparison
+[`Map`]: https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Map
+[`Set`]: https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Set
 [`Object.is()`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/is
 [SameValueZero]: https://tc39.github.io/ecma262/#sec-samevaluezero
 [prototype-spec]: https://tc39.github.io/ecma262/#sec-ordinary-object-internal-methods-and-internal-slots

lib/assert.js

@@ -23,6 +23,7 @@
 // UTILITY
 const compare = process.binding('buffer').compare;
 const util = require('util');
+const { isSet, isMap } = process.binding('util');
 const objectToString = require('internal/util').objectToString;
 const Buffer = require('buffer').Buffer;
 
@@ -262,11 +263,12 @@ function _deepEqual(actual, expected, strict, memos) {
     }
   }
 
-  // For all other Object pairs, including Array objects,
+  // For all other Object pairs, including Array objects and Maps,
   // equivalence is determined by having:
   // a) The same number of owned enumerable properties
   // b) The same set of keys/indexes (although not necessarily the same order)
   // c) Equivalent values for every corresponding key/index
+  // d) For Sets and Maps, equal contents
   // Note: this accounts for both named and indexed properties on Arrays.
 
   // Use memos to handle cycles.
@@ -283,6 +285,97 @@ function _deepEqual(actual, expected, strict, memos) {
   return objEquiv(actual, expected, strict, memos);
 }
 
+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, memo))
+      return true;
+  }
+
+  return false;
+}
+
+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]))
+  //
+  // In theory, all the items in the first set have a corresponding == value in
+  // the second set, but the sets have different sizes. Its a silly case,
+  // and more evidence that deepStrictEqual should always be preferred over
+  // deepEqual.
+  if (a.size !== b.size)
+    return false;
+
+  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)) {
+      return false;
+    }
+  }
+
+  return true;
+}
+
+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;
+
+  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;
+}
+
 function objEquiv(a, b, strict, actualVisitedObjects) {
   // If one of them is a primitive, the other must be the same.
   if (util.isPrimitive(a) || util.isPrimitive(b))
@@ -307,6 +400,22 @@ function objEquiv(a, b, strict, actualVisitedObjects) {
       return false;
   }
 
+  // Sets and maps don't have their entries accessible via normal object
+  // properties.
+  if (isSet(a)) {
+    if (!isSet(b) || !setEquiv(a, b, strict, actualVisitedObjects))
+      return false;
+  } else if (isSet(b)) {
+    return false;
+  }
+
+  if (isMap(a)) {
+    if (!isMap(b) || !mapEquiv(a, b, strict, actualVisitedObjects))
+      return false;
+  } else if (isMap(b)) {
+    return false;
+  }
+
   // The pair must have equivalent values for every corresponding key.
   // Possibly expensive deep test:
   for (i = aKeys.length - 1; i >= 0; i--) {

test/parallel/test-assert-deep.js

@@ -100,11 +100,172 @@ const similar = new Set([
 for (const a of similar) {
   for (const b of similar) {
     if (a !== b) {
-      assert.doesNotThrow(() => assert.deepEqual(a, b));
+      assert.deepEqual(a, b);
       assert.throws(() => assert.deepStrictEqual(a, b),
                     re`${a} deepStrictEqual ${b}`);
     }
   }
 }
 
+function assertDeepAndStrictEqual(a, b) {
+  assert.deepEqual(a, b);
+  assert.deepStrictEqual(a, b);
+
+  assert.deepEqual(b, a);
+  assert.deepStrictEqual(b, a);
+}
+
+function assertNotDeepOrStrict(a, b) {
+  assert.throws(() => assert.deepEqual(a, b));
+  assert.throws(() => assert.deepStrictEqual(a, b));
+
+  assert.throws(() => assert.deepEqual(b, a));
+  assert.throws(() => assert.deepStrictEqual(b, a));
+}
+
+function assertOnlyDeepEqual(a, b) {
+  assert.doesNotThrow(() => assert.deepEqual(a, b));
+  assert.throws(() => assert.deepStrictEqual(a, b));
+
+  assert.doesNotThrow(() => assert.deepEqual(b, a));
+  assert.throws(() => assert.deepStrictEqual(b, a));
+}
+
+// es6 Maps and Sets
+assertDeepAndStrictEqual(new Set(), new Set());
+assertDeepAndStrictEqual(new Map(), new Map());
+
+assertDeepAndStrictEqual(new Set([1, 2, 3]), new Set([1, 2, 3]));
+assertNotDeepOrStrict(new Set([1, 2, 3]), new Set([1, 2, 3, 4]));
+assertNotDeepOrStrict(new Set([1, 2, 3, 4]), new Set([1, 2, 3]));
+assertDeepAndStrictEqual(new Set(['1', '2', '3']), new Set(['1', '2', '3']));
+assertDeepAndStrictEqual(new Set([[1, 2], [3, 4]]), new Set([[3, 4], [1, 2]]));
+
+assertDeepAndStrictEqual(new Map([[1, 1], [2, 2]]), new Map([[1, 1], [2, 2]]));
+assertDeepAndStrictEqual(new Map([[1, 1], [2, 2]]), new Map([[2, 2], [1, 1]]));
+assertNotDeepOrStrict(new Map([[1, 1], [2, 2]]), new Map([[1, 2], [2, 1]]));
+
+assertNotDeepOrStrict(new Set([1]), [1]);
+assertNotDeepOrStrict(new Set(), []);
+assertNotDeepOrStrict(new Set(), {});
+
+assertNotDeepOrStrict(new Map([['a', 1]]), {a: 1});
+assertNotDeepOrStrict(new Map(), []);
+assertNotDeepOrStrict(new Map(), {});
+
+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']]),
+  new Map([['1', 'a'], [1, 'b']])
+);
+assertDeepAndStrictEqual(
+  new Map([[{}, 'a'], [{}, 'b']]),
+  new Map([[{}, 'b'], [{}, 'a']])
+);
+
+{
+  const values = [
+    123,
+    Infinity,
+    0,
+    null,
+    undefined,
+    false,
+    true,
+    {},
+    [],
+    () => {},
+  ];
+  assertDeepAndStrictEqual(new Set(values), new Set(values));
+  assertDeepAndStrictEqual(new Set(values), new Set(values.reverse()));
+
+  const mapValues = values.map((v) => [v, {a: 5}]);
+  assertDeepAndStrictEqual(new Map(mapValues), new Map(mapValues));
+  assertDeepAndStrictEqual(new Map(mapValues), new Map(mapValues.reverse()));
+}
+
+{
+  const s1 = new Set();
+  const s2 = new Set();
+  s1.add(1);
+  s1.add(2);
+  s2.add(2);
+  s2.add(1);
+  assertDeepAndStrictEqual(s1, s2);
+}
+
+{
+  const m1 = new Map();
+  const m2 = new Map();
+  const obj = {a: 5, b: 6};
+  m1.set(1, obj);
+  m1.set(2, 'hi');
+  m1.set(3, [1, 2, 3]);
+
+  m2.set(2, 'hi'); // different order
+  m2.set(1, obj);
+  m2.set(3, [1, 2, 3]); // deep equal, but not reference equal.
+
+  assertDeepAndStrictEqual(m1, m2);
+}
+
+{
+  const m1 = new Map();
+  const m2 = new Map();
+
+  // m1 contains itself.
+  m1.set(1, m1);
+  m2.set(1, new Map());
+
+  assertNotDeepOrStrict(m1, m2);
+}
+
+assert.deepEqual(new Map([[1, 1]]), new Map([[1, '1']]));
+assert.throws(() =>
+  assert.deepStrictEqual(new Map([[1, 1]]), new Map([[1, '1']]))
+);
+
+{
+  // Two equivalent sets / maps with different key/values applied shouldn't be
+  // the same. This is a terrible idea to do in practice, but deepEqual should
+  // still check for it.
+  const s1 = new Set();
+  const s2 = new Set();
+  s1.x = 5;
+  assertNotDeepOrStrict(s1, s2);
+
+  const m1 = new Map();
+  const m2 = new Map();
+  m1.x = 5;
+  assertNotDeepOrStrict(m1, m2);
+}
+
+{
+  // Circular references.
+  const s1 = new Set();
+  s1.add(s1);
+  const s2 = new Set();
+  s2.add(s2);
+  assertDeepAndStrictEqual(s1, s2);
+
+  const m1 = new Map();
+  m1.set(2, m1);
+  const m2 = new Map();
+  m2.set(2, m2);
+  assertDeepAndStrictEqual(m1, m2);
+
+  const m3 = new Map();
+  m3.set(m3, 2);
+  const m4 = new Map();
+  m4.set(m4, 2);
+  assertDeepAndStrictEqual(m3, m4);
+}
+
 /* eslint-enable */