From 7493db21b667ed746d39c9b54357eac4287232e3 Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Thu, 13 Dec 2018 07:33:31 +0100
Subject: [PATCH] assert: adjust loose assertions

This changes the loose deep equal comparison by using the same logic
as done in the strict deep equal comparison besides comparing
primitives loosely, not comparing symbol properties and not comparing
the prototype.

`assert.deepEqual` is still commenly used and this is likely the
biggest pitfall.

Most changes are only minor and won't have a big impact besides
likely fixing user expectations.

PR-URL: https://github.com/nodejs/node/pull/25008
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
---
 doc/api/assert.md                             |  47 ++++--
 lib/internal/util/comparisons.js              | 153 ++++++------------
 test/es-module/test-esm-dynamic-import.js     |   9 +-
 test/parallel/test-assert-checktag.js         |   8 +-
 test/parallel/test-assert-deep.js             |  50 +++---
 .../test-assert-typedarray-deepequal.js       |  10 +-
 6 files changed, 116 insertions(+), 161 deletions(-)

diff --git a/doc/api/assert.md b/doc/api/assert.md
index 4c4b2c4250f581..67bc9afbabccbe 100644
--- a/doc/api/assert.md
+++ b/doc/api/assert.md
@@ -163,6 +163,10 @@ An alias of [`assert.ok()`][].
 <!-- YAML
 added: v0.1.21
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/25008
+    description: The type tags are now properly compared and there are a couple
+                 minor comparison adjustments to make the check less surprising.
   - version: v9.0.0
     pr-url: https://github.com/nodejs/node/pull/15001
     description: The `Error` names and messages are now properly compared
@@ -191,26 +195,39 @@ An alias of [`assert.deepStrictEqual()`][].
 
 > Stability: 0 - Deprecated: Use [`assert.deepStrictEqual()`][] instead.
 
-Tests for deep equality between the `actual` and `expected` parameters.
-Primitive values are compared with the [Abstract Equality Comparison][]
-( `==` ).
+Tests for deep equality between the `actual` and `expected` parameters. Consider
+using [`assert.deepStrictEqual()`][] instead. [`assert.deepEqual()`][] can have
+potentially surprising results.
+
+"Deep" equality means that the enumerable "own" properties of child objects
+are also recursively evaluated by the following rules.
+
+### Comparison details
+
+* Primitive values are compared with the [Abstract Equality Comparison][]
+  ( `==` ).
+* [Type tags][Object.prototype.toString()] of objects should be the same.
+* Only [enumerable "own" properties][] are considered.
+* [`Error`][] names and messages are always compared, even if these are not
+  enumerable properties.
+* [Object wrappers][] are compared both as objects and unwrapped values.
+* `Object` properties are compared unordered.
+* [`Map`][] keys and [`Set`][] items are compared unordered.
+* Recursion stops when both sides differ or both sides encounter a circular
+  reference.
+* Implementation does not test the [`[[Prototype]]`][prototype-spec] of
+  objects.
+* [`Symbol`][] properties are not compared.
+* [`WeakMap`][] and [`WeakSet`][] comparison does not rely on their values.
 
-Only [enumerable "own" properties][] are considered. The
-[`assert.deepEqual()`][] implementation does not test the
-[`[[Prototype]]`][prototype-spec] of objects or enumerable own [`Symbol`][]
-properties. For such checks, consider using [`assert.deepStrictEqual()`][]
-instead. [`assert.deepEqual()`][] can have potentially surprising results. The
-following example does not throw an `AssertionError` because the properties on
-the [`RegExp`][] object are not enumerable:
+The following example does not throw an `AssertionError` because the primitives
+are considered equal by the [Abstract Equality Comparison][] ( `==` ).
 
 ```js
 // WARNING: This does not throw an AssertionError!
-assert.deepEqual(/a/gi, new Date());
+assert.deepEqual('+00000000', false);
 ```
 
-An exception is made for [`Map`][] and [`Set`][]. `Map`s and `Set`s have their
-contained items compared too, as expected.
-
 "Deep" equality means that the enumerable "own" properties of child objects
 are evaluated also:
 
@@ -304,7 +321,7 @@ are recursively evaluated also by the following rules.
 * Enumerable own [`Symbol`][] properties are compared as well.
 * [Object wrappers][] are compared both as objects and unwrapped values.
 * `Object` properties are compared unordered.
-* `Map` keys and `Set` items are compared unordered.
+* [`Map`][] keys and [`Set`][] items are compared unordered.
 * Recursion stops when both sides differ or both sides encounter a circular
   reference.
 * [`WeakMap`][] and [`WeakSet`][] comparison does not rely on their values. See
diff --git a/lib/internal/util/comparisons.js b/lib/internal/util/comparisons.js
index d0726089063852..3470f206661557 100644
--- a/lib/internal/util/comparisons.js
+++ b/lib/internal/util/comparisons.js
@@ -14,7 +14,9 @@ const {
   isStringObject,
   isBooleanObject,
   isBigIntObject,
-  isSymbolObject
+  isSymbolObject,
+  isFloat32Array,
+  isFloat64Array
 } = require('internal/util/types');
 const {
   getOwnNonIndexProperties,
@@ -84,18 +86,6 @@ function areEqualArrayBuffers(buf1, buf2) {
     compare(new Uint8Array(buf1), new Uint8Array(buf2)) === 0;
 }
 
-function isFloatTypedArrayTag(tag) {
-  return tag === '[object Float32Array]' || tag === '[object Float64Array]';
-}
-
-function isArguments(tag) {
-  return tag === '[object Arguments]';
-}
-
-function isObjectOrArrayTag(tag) {
-  return tag === '[object Array]' || tag === '[object Object]';
-}
-
 function isEqualBoxedPrimitive(val1, val2) {
   if (isNumberObject(val1)) {
     return isNumberObject(val2) &&
@@ -132,13 +122,38 @@ function isEqualBoxedPrimitive(val1, val2) {
 // For strict comparison, objects should have
 // a) The same built-in type tags
 // b) The same prototypes.
-function strictDeepEqual(val1, val2, memos) {
-  if (typeof val1 !== 'object') {
-    return typeof val1 === 'number' && numberIsNaN(val1) &&
-      numberIsNaN(val2);
+
+function innerDeepEqual(val1, val2, strict, memos) {
+  // All identical values are equivalent, as determined by ===.
+  if (val1 === val2) {
+    if (val1 !== 0)
+      return true;
+    return strict ? objectIs(val1, val2) : true;
   }
-  if (typeof val2 !== 'object' || val1 === null || val2 === null) {
-    return false;
+
+  // Check more closely if val1 and val2 are equal.
+  if (strict) {
+    if (typeof val1 !== 'object') {
+      return typeof val1 === 'number' && numberIsNaN(val1) &&
+        numberIsNaN(val2);
+    }
+    if (typeof val2 !== 'object' || val1 === null || val2 === null) {
+      return false;
+    }
+    if (getPrototypeOf(val1) !== getPrototypeOf(val2)) {
+      return false;
+    }
+  } else {
+    if (val1 === null || typeof val1 !== 'object') {
+      if (val2 === null || typeof val2 !== 'object') {
+        // eslint-disable-next-line eqeqeq
+        return val1 == val2;
+      }
+      return false;
+    }
+    if (val2 === null || typeof val2 !== 'object') {
+      return false;
+    }
   }
   const val1Tag = objectToString(val1);
   const val2Tag = objectToString(val2);
@@ -146,9 +161,6 @@ function strictDeepEqual(val1, val2, memos) {
   if (val1Tag !== val2Tag) {
     return false;
   }
-  if (getPrototypeOf(val1) !== getPrototypeOf(val2)) {
-    return false;
-  }
   if (Array.isArray(val1)) {
     // Check for sparse arrays and general fast path
     if (val1.length !== val2.length) {
@@ -159,10 +171,10 @@ function strictDeepEqual(val1, val2, memos) {
     if (keys1.length !== keys2.length) {
       return false;
     }
-    return keyCheck(val1, val2, kStrict, memos, kIsArray, keys1);
+    return keyCheck(val1, val2, strict, memos, kIsArray, keys1);
   }
   if (val1Tag === '[object Object]') {
-    return keyCheck(val1, val2, kStrict, memos, kNoIterator);
+    return keyCheck(val1, val2, strict, memos, kNoIterator);
   }
   if (isDate(val1)) {
     if (dateGetTime(val1) !== dateGetTime(val2)) {
@@ -174,13 +186,16 @@ function strictDeepEqual(val1, val2, memos) {
     }
   } else if (isNativeError(val1) || val1 instanceof Error) {
     // Do not compare the stack as it might differ even though the error itself
-    // is otherwise identical. The non-enumerable name should be identical as
-    // the prototype is also identical. Otherwise this is caught later on.
-    if (val1.message !== val2.message) {
+    // is otherwise identical.
+    if (val1.message !== val2.message || val1.name !== val2.name) {
       return false;
     }
   } else if (isArrayBufferView(val1)) {
-    if (!areSimilarTypedArrays(val1, val2)) {
+    if (!strict && (isFloat32Array(val1) || isFloat64Array(val1))) {
+      if (!areSimilarFloatArrays(val1, val2)) {
+        return false;
+      }
+    } else if (!areSimilarTypedArrays(val1, val2)) {
       return false;
     }
     // Buffer.compare returns true, so val1.length === val2.length. If they both
@@ -191,17 +206,17 @@ function strictDeepEqual(val1, val2, memos) {
     if (keys1.length !== keys2.length) {
       return false;
     }
-    return keyCheck(val1, val2, kStrict, memos, kNoIterator, keys1);
+    return keyCheck(val1, val2, strict, memos, kNoIterator, keys1);
   } else if (isSet(val1)) {
     if (!isSet(val2) || val1.size !== val2.size) {
       return false;
     }
-    return keyCheck(val1, val2, kStrict, memos, kIsSet);
+    return keyCheck(val1, val2, strict, memos, kIsSet);
   } else if (isMap(val1)) {
     if (!isMap(val2) || val1.size !== val2.size) {
       return false;
     }
-    return keyCheck(val1, val2, kStrict, memos, kIsMap);
+    return keyCheck(val1, val2, strict, memos, kIsMap);
   } else if (isAnyArrayBuffer(val1)) {
     if (!areEqualArrayBuffers(val1, val2)) {
       return false;
@@ -209,66 +224,7 @@ function strictDeepEqual(val1, val2, memos) {
   } else if (isBoxedPrimitive(val1) && !isEqualBoxedPrimitive(val1, val2)) {
     return false;
   }
-  return keyCheck(val1, val2, kStrict, memos, kNoIterator);
-}
-
-function looseDeepEqual(val1, val2, memos) {
-  if (val1 === null || typeof val1 !== 'object') {
-    if (val2 === null || typeof val2 !== 'object') {
-      // eslint-disable-next-line eqeqeq
-      return val1 == val2;
-    }
-    return false;
-  }
-  if (val2 === null || typeof val2 !== 'object') {
-    return false;
-  }
-  const val1Tag = objectToString(val1);
-  const val2Tag = objectToString(val2);
-  if (val1Tag === val2Tag) {
-    if (isObjectOrArrayTag(val1Tag)) {
-      return keyCheck(val1, val2, kLoose, memos, kNoIterator);
-    }
-    if (isArrayBufferView(val1)) {
-      if (isFloatTypedArrayTag(val1Tag)) {
-        return areSimilarFloatArrays(val1, val2);
-      }
-      return areSimilarTypedArrays(val1, val2);
-    }
-    if (isDate(val1) && isDate(val2)) {
-      return val1.getTime() === val2.getTime();
-    }
-    if (isRegExp(val1) && isRegExp(val2)) {
-      return areSimilarRegExps(val1, val2);
-    }
-    if (val1 instanceof Error && val2 instanceof Error) {
-      if (val1.message !== val2.message || val1.name !== val2.name)
-        return false;
-    }
-  // Ensure reflexivity of deepEqual with `arguments` objects.
-  // See https://github.com/nodejs/node-v0.x-archive/pull/7178
-  } else if (isArguments(val1Tag) || isArguments(val2Tag)) {
-    return false;
-  }
-  if (isSet(val1)) {
-    if (!isSet(val2) || val1.size !== val2.size) {
-      return false;
-    }
-    return keyCheck(val1, val2, kLoose, memos, kIsSet);
-  } else if (isMap(val1)) {
-    if (!isMap(val2) || val1.size !== val2.size) {
-      return false;
-    }
-    return keyCheck(val1, val2, kLoose, memos, kIsMap);
-  } else if (isSet(val2) || isMap(val2)) {
-    return false;
-  }
-  if (isAnyArrayBuffer(val1) && isAnyArrayBuffer(val2)) {
-    if (!areEqualArrayBuffers(val1, val2)) {
-      return false;
-    }
-  }
-  return keyCheck(val1, val2, kLoose, memos, kNoIterator);
+  return keyCheck(val1, val2, strict, memos, kNoIterator);
 }
 
 function getEnumerables(val, keys) {
@@ -370,21 +326,6 @@ function keyCheck(val1, val2, strict, memos, iterationType, aKeys) {
   return areEq;
 }
 
-function innerDeepEqual(val1, val2, strict, memos) {
-  // All identical values are equivalent, as determined by ===.
-  if (val1 === val2) {
-    if (val1 !== 0)
-      return true;
-    return strict ? objectIs(val1, val2) : true;
-  }
-
-  // Check more closely if val1 and val2 are equal.
-  if (strict === true)
-    return strictDeepEqual(val1, val2, memos);
-
-  return looseDeepEqual(val1, val2, memos);
-}
-
 function setHasEqualElement(set, val1, strict, memo) {
   // Go looking.
   for (const val2 of set) {
diff --git a/test/es-module/test-esm-dynamic-import.js b/test/es-module/test-esm-dynamic-import.js
index 887339977669ea..b271d43c805d0e 100644
--- a/test/es-module/test-esm-dynamic-import.js
+++ b/test/es-module/test-esm-dynamic-import.js
@@ -23,9 +23,12 @@ function expectMissingModuleError(result) {
 function expectOkNamespace(result) {
   Promise.resolve(result)
     .then(common.mustCall((ns) => {
-      // Can't deepStrictEqual because ns isn't a normal object
-      // eslint-disable-next-line no-restricted-properties
-      assert.deepEqual(ns, { default: true });
+      const expected = { default: true };
+      Object.defineProperty(expected, Symbol.toStringTag, {
+        value: 'Module'
+      });
+      Object.setPrototypeOf(expected, Object.getPrototypeOf(ns));
+      assert.deepStrictEqual(ns, expected);
     }));
 }
 
diff --git a/test/parallel/test-assert-checktag.js b/test/parallel/test-assert-checktag.js
index eeb25278ff186f..c0b80adffcaa1a 100644
--- a/test/parallel/test-assert-checktag.js
+++ b/test/parallel/test-assert-checktag.js
@@ -18,8 +18,8 @@ if (process.stdout.isTTY)
   FakeDate.prototype = Date.prototype;
   const fake = new FakeDate();
 
-  assert.deepEqual(date, fake);
-  assert.deepEqual(fake, date);
+  assert.notDeepEqual(date, fake);
+  assert.notDeepEqual(fake, date);
 
   // For deepStrictEqual we check the runtime type,
   // then reveal the fakeness of the fake date
@@ -45,7 +45,7 @@ if (process.stdout.isTTY)
   for (const prop of Object.keys(global)) {
     fakeGlobal[prop] = global[prop];
   }
-  assert.deepEqual(fakeGlobal, global);
+  assert.notDeepEqual(fakeGlobal, global);
   // Message will be truncated anyway, don't validate
   assert.throws(() => assert.deepStrictEqual(fakeGlobal, global),
                 assert.AssertionError);
@@ -57,7 +57,7 @@ if (process.stdout.isTTY)
   for (const prop of Object.keys(process)) {
     fakeProcess[prop] = process[prop];
   }
-  assert.deepEqual(fakeProcess, process);
+  assert.notDeepEqual(fakeProcess, process);
   // Message will be truncated anyway, don't validate
   assert.throws(() => assert.deepStrictEqual(fakeProcess, process),
                 assert.AssertionError);
diff --git a/test/parallel/test-assert-deep.js b/test/parallel/test-assert-deep.js
index f495e51ef603de..65ebec7bba9613 100644
--- a/test/parallel/test-assert-deep.js
+++ b/test/parallel/test-assert-deep.js
@@ -75,7 +75,7 @@ assert.deepEqual(arr, buf);
                '  ]'
     }
   );
-  assert.deepEqual(buf2, buf);
+  assert.notDeepEqual(buf2, buf);
 }
 
 {
@@ -94,7 +94,7 @@ assert.deepEqual(arr, buf);
                '  ]'
     }
   );
-  assert.deepEqual(arr, arr2);
+  assert.notDeepEqual(arr, arr2);
 }
 
 const date = new Date('2016');
@@ -110,8 +110,8 @@ const date2 = new MyDate('2016');
 
 // deepEqual returns true as long as the time are the same,
 // but deepStrictEqual checks own properties
-assert.deepEqual(date, date2);
-assert.deepEqual(date2, date);
+assert.notDeepEqual(date, date2);
+assert.notDeepEqual(date2, date);
 assert.throws(
   () => assert.deepStrictEqual(date, date2),
   {
@@ -143,7 +143,7 @@ const re2 = new MyRegExp('test');
 
 // deepEqual returns true as long as the regexp-specific properties
 // are the same, but deepStrictEqual checks all properties
-assert.deepEqual(re1, re2);
+assert.notDeepEqual(re1, re2);
 assert.throws(
   () => assert.deepStrictEqual(re1, re2),
   {
@@ -157,28 +157,24 @@ assert.throws(
 // but deepStrictEqual should throw.
 {
   const similar = new Set([
-    { 0: '1' },  // Object
     { 0: 1 },  // Object
     new String('1'),  // Object
-    ['1'],  // Array
     [1],  // Array
     date2,  // Date with this[0] = '1'
     re2,  // RegExp with this[0] = '1'
     new Int8Array([1]), // Int8Array
-    new Uint8Array([1]), // Uint8Array
     new Int16Array([1]), // Int16Array
     new Uint16Array([1]), // Uint16Array
     new Int32Array([1]), // Int32Array
     new Uint32Array([1]), // Uint32Array
-    Buffer.from([1]),
-    // Arguments {'0': '1'} is not here
-    // See https://github.com/nodejs/node-v0.x-archive/pull/7178
+    Buffer.from([1]), // Uint8Array
+    (function() { return arguments; })(1)
   ]);
 
   for (const a of similar) {
     for (const b of similar) {
       if (a !== b) {
-        assert.deepEqual(a, b);
+        assert.notDeepEqual(a, b);
         assert.throws(
           () => assert.deepStrictEqual(a, b),
           { code: 'ERR_ASSERTION' }
@@ -576,7 +572,7 @@ assertNotDeepOrStrict(
 // Handle sparse arrays.
 {
   assertDeepAndStrictEqual([1, , , 3], [1, , , 3]);
-  assertOnlyDeepEqual([1, , , 3], [1, , , 3, , , ]);
+  assertNotDeepOrStrict([1, , , 3], [1, , , 3, , , ]);
   const a = new Array(3);
   const b = new Array(3);
   a[2] = true;
@@ -594,13 +590,11 @@ assertNotDeepOrStrict(
   assertNotDeepOrStrict(err1, new Error('foo2'), assert.AssertionError);
   assertNotDeepOrStrict(err1, new TypeError('foo1'), assert.AssertionError);
   assertDeepAndStrictEqual(err1, new Error('foo1'));
-  // TODO: evaluate if this should throw or not. The same applies for RegExp
-  // Date and any object that has the same keys but not the same prototype.
-  assertOnlyDeepEqual(err1, {});
+  assertNotDeepOrStrict(err1, {}, AssertionError);
 }
 
 // Handle NaN
-assert.throws(() => { assert.deepEqual(NaN, NaN); }, assert.AssertionError);
+assert.notDeepEqual(NaN, NaN);
 assert.deepStrictEqual(NaN, NaN);
 assert.deepStrictEqual({ a: NaN }, { a: NaN });
 assert.deepStrictEqual([ 1, 2, NaN, 4 ], [ 1, 2, NaN, 4 ]);
@@ -609,11 +603,11 @@ assert.deepStrictEqual([ 1, 2, NaN, 4 ], [ 1, 2, NaN, 4 ]);
 {
   const boxedString = new String('test');
   const boxedSymbol = Object(Symbol());
-  assertOnlyDeepEqual(new Boolean(true), Object(false));
-  assertOnlyDeepEqual(Object(true), new Number(1));
-  assertOnlyDeepEqual(new Number(2), new Number(1));
-  assertOnlyDeepEqual(boxedSymbol, Object(Symbol()));
-  assertOnlyDeepEqual(boxedSymbol, {});
+  assertNotDeepOrStrict(new Boolean(true), Object(false));
+  assertNotDeepOrStrict(Object(true), new Number(1));
+  assertNotDeepOrStrict(new Number(2), new Number(1));
+  assertNotDeepOrStrict(boxedSymbol, Object(Symbol()));
+  assertNotDeepOrStrict(boxedSymbol, {});
   assertDeepAndStrictEqual(boxedSymbol, boxedSymbol);
   assertDeepAndStrictEqual(Object(true), Object(true));
   assertDeepAndStrictEqual(Object(2), Object(2));
@@ -645,7 +639,7 @@ assertDeepAndStrictEqual(-0, -0);
   const b = new Uint8Array(4);
   a[symbol1] = true;
   b[symbol1] = false;
-  assertOnlyDeepEqual(a, b);
+  assertNotDeepOrStrict(a, b);
   b[symbol1] = true;
   assertDeepAndStrictEqual(a, b);
   // The same as TypedArrays is valid for boxed primitives
@@ -700,7 +694,7 @@ assert.deepEqual({ a: 4, b: '2' }, { a: 4, b: '2' });
 assert.deepEqual([4], ['4']);
 assert.throws(
   () => assert.deepEqual({ a: 4 }, { a: 4, b: true }), AssertionError);
-assert.deepEqual(['a'], { 0: 'a' });
+assert.notDeepEqual(['a'], { 0: 'a' });
 assert.deepEqual({ a: 4, b: '1' }, { b: '1', a: 4 });
 const a1 = [1, 2, 3];
 const a2 = [1, 2, 3];
@@ -759,10 +753,10 @@ assertOnlyDeepEqual(true, 1);
 }
 
 // Primitive wrappers and object.
-assertOnlyDeepEqual(new String('a'), ['a']);
-assertOnlyDeepEqual(new String('a'), { 0: 'a' });
-assertOnlyDeepEqual(new Number(1), {});
-assertOnlyDeepEqual(new Boolean(true), {});
+assertNotDeepOrStrict(new String('a'), ['a']);
+assertNotDeepOrStrict(new String('a'), { 0: 'a' });
+assertNotDeepOrStrict(new Number(1), {});
+assertNotDeepOrStrict(new Boolean(true), {});
 
 // Same number of keys but different key names.
 assertNotDeepOrStrict({ a: 1 }, { b: 1 });
diff --git a/test/parallel/test-assert-typedarray-deepequal.js b/test/parallel/test-assert-typedarray-deepequal.js
index 1e9c8c8d1c247d..6be4c10595ab45 100644
--- a/test/parallel/test-assert-typedarray-deepequal.js
+++ b/test/parallel/test-assert-typedarray-deepequal.js
@@ -29,15 +29,15 @@ const equalArrayPairs = [
 ];
 
 const looseEqualArrayPairs = [
-  [new Float64Array([+0.0]), new Float32Array([-0.0])],
-  [new Int16Array(256), new Uint16Array(256)],
-  [new Int16Array([256]), new Uint16Array([256])],
   [new Float32Array([+0.0]), new Float32Array([-0.0])],
-  [new Float64Array([+0.0]), new Float64Array([-0.0])],
-  [new ArrayBuffer(3), new SharedArrayBuffer(3)]
+  [new Float64Array([+0.0]), new Float64Array([-0.0])]
 ];
 
 const notEqualArrayPairs = [
+  [new ArrayBuffer(3), new SharedArrayBuffer(3)],
+  [new Int16Array(256), new Uint16Array(256)],
+  [new Int16Array([256]), new Uint16Array([256])],
+  [new Float64Array([+0.0]), new Float32Array([-0.0])],
   [new Uint8Array(2), new Uint8Array(3)],
   [new Uint8Array([1, 2, 3]), new Uint8Array([4, 5, 6])],
   [new Uint8ClampedArray([300, 2, 3]), new Uint8Array([300, 2, 3])],