assert: fix premature deep strict comparison

Refactors _deepEqual and fixes a few code paths that lead to
behaviors contradicting what the doc says. Before this commit
certain types of objects (Buffers, Dates, etc.) are not checked
properly, and can get away with different prototypes AND different
enumerable owned properties because _deepEqual would jump to
premature conclusion for them.

Since we no longer follow CommonJS unit testing spec,
the checks for primitives and object prototypes are moved
forward for faster failure.

Improve regexp and float* array checks:

* Don't compare lastIndex of regexps, because they are not
  enumerable, so according to the docs they should not be compared
* Compare flags of regexps instead of separate properties
* Use built-in tags to test for float* arrays instead of using
  instanceof

Use full link to the archived GitHub repository.

Use util.objectToString for future improvements to that function
that makes sure the call won't be tampered with.

PR-URL: #11128
Refs: #10282 (comment)
Refs: #10258 (comment)
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

lib/assert.js

@@ -23,8 +23,8 @@
 // UTILITY
 const compare = process.binding('buffer').compare;
 const util = require('util');
+const objectToString = require('internal/util').objectToString;
 const Buffer = require('buffer').Buffer;
-const pToString = (obj) => Object.prototype.toString.call(obj);
 
 // The assert module provides functions that throw
 // AssertionError's when particular conditions are not met. The
@@ -136,117 +136,177 @@ assert.deepStrictEqual = function deepStrictEqual(actual, expected, message) {
   }
 };
 
+function areSimilarRegExps(a, b) {
+  return a.source === b.source && a.flags === b.flags;
+}
+
+function areSimilarTypedArrays(a, b) {
+  return compare(Buffer.from(a.buffer,
+                             a.byteOffset,
+                             a.byteLength),
+                 Buffer.from(b.buffer,
+                             b.byteOffset,
+                             b.byteLength)) === 0;
+}
+
+function isNullOrNonObj(object) {
+  return object === null || typeof object !== 'object';
+}
+
+function isFloatTypedArrayTag(tag) {
+  return tag === '[object Float32Array]' || tag === '[object Float64Array]';
+}
+
+function isArguments(tag) {
+  return tag === '[object Arguments]';
+}
+
 function _deepEqual(actual, expected, strict, memos) {
   // All identical values are equivalent, as determined by ===.
   if (actual === expected) {
     return true;
+  }
 
-  // If both values are instances of buffers, equivalence is
-  // determined by comparing the values and ensuring the result
-  // === 0.
-  } else if (actual instanceof Buffer && expected instanceof Buffer) {
-    return compare(actual, expected) === 0;
-
-  // If the expected value is a Date object, the actual value is
-  // equivalent if it is also a Date object that refers to the same time.
-  } else if (util.isDate(actual) && util.isDate(expected)) {
-    return actual.getTime() === expected.getTime();
-
-  // If the expected value is a RegExp object, the actual value is
-  // equivalent if it is also a RegExp object with the same source and
-  // properties (`global`, `multiline`, `lastIndex`, `ignoreCase`).
-  } else if (util.isRegExp(actual) && util.isRegExp(expected)) {
-    return actual.source === expected.source &&
-           actual.global === expected.global &&
-           actual.multiline === expected.multiline &&
-           actual.lastIndex === expected.lastIndex &&
-           actual.ignoreCase === expected.ignoreCase;
-
-  // If both values are primitives, equivalence is determined by
-  // == or, if checking for strict equivalence, ===.
-  } else if ((actual === null || typeof actual !== 'object') &&
-             (expected === null || typeof expected !== 'object')) {
+  // For primitives / functions
+  // (determined by typeof value !== 'object'),
+  // or null, equivalence is determined by === or ==.
+  if (isNullOrNonObj(actual) && isNullOrNonObj(expected)) {
     return strict ? actual === expected : actual == expected;
+  }
 
-  // If both values are instances of typed arrays, wrap their underlying
-  // ArrayBuffers in a Buffer to increase performance.
-  // This optimization requires the arrays to have the same type as checked by
-  // Object.prototype.toString (pToString). Never perform binary
-  // comparisons for Float*Arrays, though, since +0 === -0 is true despite the
-  // two values' bit patterns not being identical.
-  } else if (ArrayBuffer.isView(actual) && ArrayBuffer.isView(expected) &&
-             pToString(actual) === pToString(expected) &&
-             !(actual instanceof Float32Array ||
-               actual instanceof Float64Array)) {
-    return compare(Buffer.from(actual.buffer,
-                               actual.byteOffset,
-                               actual.byteLength),
-                   Buffer.from(expected.buffer,
-                               expected.byteOffset,
-                               expected.byteLength)) === 0;
-
-  // For all other Object pairs, including Array objects, equivalence is
-  // determined by having the same number of owned properties (as verified
-  // with Object.prototype.hasOwnProperty.call), the same set of keys
-  // (although not necessarily the same order), equivalent values for every
-  // corresponding key, and an identical 'prototype' property. Note: this
-  // accounts for both named and indexed properties on Arrays.
-  } else {
-    memos = memos || {actual: [], expected: []};
+  // If they bypass the previous check, then at least
+  // one of them must be an non-null object.
+  // If the other one is null or undefined, they must not be equal.
+  if (actual === null || actual === undefined ||
+      expected === null || expected === undefined)
+    return false;
 
-    const actualIndex = memos.actual.indexOf(actual);
-    if (actualIndex !== -1) {
-      if (actualIndex === memos.expected.indexOf(expected)) {
-        return true;
-      }
+  // Notes: Type tags are historical [[Class]] properties that can be set by
+  // FunctionTemplate::SetClassName() in C++ or Symbol.toStringTag in JS
+  // and retrieved using Object.prototype.toString.call(obj) in JS
+  // See https://tc39.github.io/ecma262/#sec-object.prototype.tostring
+  // for a list of tags pre-defined in the spec.
+  // There are some unspecified tags in the wild too (e.g. typed array tags).
+  // Since tags can be altered, they only serve fast failures
+  const actualTag = objectToString(actual);
+  const expectedTag = objectToString(expected);
+
+  // Passing null or undefined to Object.getPrototypeOf() will throw
+  // so this must done after previous checks.
+  // For strict comparison, objects should have
+  // a) The same prototypes.
+  // b) The same built-in type tags
+  if (strict) {
+    if (Object.getPrototypeOf(actual) !== Object.getPrototypeOf(expected)) {
+      return false;
     }
+  }
 
-    memos.actual.push(actual);
-    memos.expected.push(expected);
+  // Do fast checks for builtin types.
+  // If they don't match, they must not be equal.
+  // If they match, return true for non-strict comparison.
+  // For strict comparison we need to exam further.
 
-    return objEquiv(actual, expected, strict, memos);
+  // If both values are Date objects,
+  // check if the time underneath are equal first.
+  if (util.isDate(actual) && util.isDate(expected)) {
+    if (actual.getTime() !== expected.getTime()) {
+      return false;
+    } else if (!strict) {
+      return true;  // Skip further checks for non-strict comparison.
+    }
   }
-}
 
-function isArguments(object) {
-  return Object.prototype.toString.call(object) === '[object Arguments]';
-}
+  // If both values are RegExp, check if they have
+  // the same source and flags first
+  if (util.isRegExp(actual) && util.isRegExp(expected)) {
+    if (!areSimilarRegExps(actual, expected)) {
+      return false;
+    } else if (!strict) {
+      return true;  // Skip further checks for non-strict comparison.
+    }
+  }
 
-function objEquiv(a, b, strict, actualVisitedObjects) {
-  if (a === null || a === undefined || b === null || b === undefined)
+  // Ensure reflexivity of deepEqual with `arguments` objects.
+  // See https://github.com/nodejs/node-v0.x-archive/pull/7178
+  if (isArguments(actualTag) !== isArguments(expectedTag)) {
     return false;
+  }
+
+  // Check typed arrays and buffers by comparing the content in their
+  // underlying ArrayBuffer. This optimization requires that it's
+  // reasonable to interpret their underlying memory in the same way,
+  // which is checked by comparing their type tags.
+  // (e.g. a Uint8Array and a Uint16Array with the same memory content
+  // could still be different because they will be interpreted differently)
+  // Never perform binary comparisons for Float*Arrays, though,
+  // since e.g. +0 === -0 is true despite the two values' bit patterns
+  // not being identical.
+  if (ArrayBuffer.isView(actual) && ArrayBuffer.isView(expected) &&
+      actualTag === expectedTag && !isFloatTypedArrayTag(actualTag)) {
+    if (!areSimilarTypedArrays(actual, expected)) {
+      return false;
+    } else if (!strict) {
+      return true;  // Skip further checks for non-strict comparison.
+    }
+
+    // Buffer.compare returns true, so actual.length === expected.length
+    // if they both only contain numeric keys, we don't need to exam further
+    if (Object.keys(actual).length === actual.length &&
+        Object.keys(expected).length === expected.length) {
+      return true;
+    }
+  }
+
+  // For all other Object pairs, including Array objects,
+  // 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
+  // Note: this accounts for both named and indexed properties on Arrays.
+
+  // Use memos to handle cycles.
+  memos = memos || { actual: [], expected: [] };
+  const actualIndex = memos.actual.indexOf(actual);
+  if (actualIndex !== -1) {
+    if (actualIndex === memos.expected.indexOf(expected)) {
+      return true;
+    }
+  }
+  memos.actual.push(actual);
+  memos.expected.push(expected);
 
-  // If one is a primitive, the other must be the same.
+  return objEquiv(actual, expected, strict, memos);
+}
+
+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))
     return a === b;
-  if (strict && Object.getPrototypeOf(a) !== Object.getPrototypeOf(b))
-    return false;
-  const aIsArgs = isArguments(a);
-  const bIsArgs = isArguments(b);
-  if ((aIsArgs && !bIsArgs) || (!aIsArgs && bIsArgs))
-    return false;
-  const ka = Object.keys(a);
-  const kb = Object.keys(b);
+
+  const aKeys = Object.keys(a);
+  const bKeys = Object.keys(b);
   var key, i;
 
-  // The pair must have the same number of owned properties (keys
-  // incorporates hasOwnProperty).
-  if (ka.length !== kb.length)
+  // The pair must have the same number of owned properties
+  // (keys incorporates hasOwnProperty).
+  if (aKeys.length !== bKeys.length)
     return false;
 
   // The pair must have the same set of keys (although not
   // necessarily in the same order).
-  ka.sort();
-  kb.sort();
+  aKeys.sort();
+  bKeys.sort();
   // Cheap key test:
-  for (i = ka.length - 1; i >= 0; i--) {
-    if (ka[i] !== kb[i])
+  for (i = aKeys.length - 1; i >= 0; i--) {
+    if (aKeys[i] !== bKeys[i])
       return false;
   }
+
   // The pair must have equivalent values for every corresponding key.
   // Possibly expensive deep test:
-  for (i = ka.length - 1; i >= 0; i--) {
-    key = ka[i];
+  for (i = aKeys.length - 1; i >= 0; i--) {
+    key = aKeys[i];
     if (!_deepEqual(a[key], b[key], strict, actualVisitedObjects))
       return false;
   }
@@ -269,7 +329,6 @@ function notDeepStrictEqual(actual, expected, message) {
   }
 }
 
-
 // The strict equality assertion tests strict equality, as determined by ===.
 // assert.strictEqual(actual, expected, message_opt);
 
@@ -295,7 +354,7 @@ function expectedException(actual, expected) {
     return false;
   }
 
-  if (Object.prototype.toString.call(expected) === '[object RegExp]') {
+  if (objectToString(expected) === '[object RegExp]') {
     return expected.test(actual);
   }
 

test/parallel/test-assert-deep.js

@@ -0,0 +1,110 @@
+'use strict';
+require('../common');
+const assert = require('assert');
+const util = require('util');
+
+// Template tag function turning an error message into a RegExp
+// for assert.throws()
+function re(literals, ...values) {
+  let result = literals[0];
+  for (const [i, value] of values.entries()) {
+    const str = util.inspect(value);
+    // Need to escape special characters.
+    result += str.replace(/[\\^$.*+?()[\]{}|=!<>:-]/g, '\\$&');
+    result += literals[i + 1];
+  }
+  return new RegExp(`^AssertionError: ${result}$`);
+}
+
+// The following deepEqual tests might seem very weird.
+// They just describe what it is now.
+// That is why we discourage using deepEqual in our own tests.
+
+// Turn off no-restricted-properties because we are testing deepEqual!
+/* eslint-disable no-restricted-properties */
+
+const arr = new Uint8Array([120, 121, 122, 10]);
+const buf = Buffer.from(arr);
+// They have different [[Prototype]]
+assert.throws(() => assert.deepStrictEqual(arr, buf));
+assert.doesNotThrow(() => assert.deepEqual(arr, buf));
+
+const buf2 = Buffer.from(arr);
+buf2.prop = 1;
+
+assert.throws(() => assert.deepStrictEqual(buf2, buf));
+assert.doesNotThrow(() => assert.deepEqual(buf2, buf));
+
+const arr2 = new Uint8Array([120, 121, 122, 10]);
+arr2.prop = 5;
+assert.throws(() => assert.deepStrictEqual(arr, arr2));
+assert.doesNotThrow(() => assert.deepEqual(arr, arr2));
+
+const date = new Date('2016');
+
+class MyDate extends Date {
+  constructor(...args) {
+    super(...args);
+    this[0] = '1';
+  }
+}
+
+const date2 = new MyDate('2016');
+
+// deepEqual returns true as long as the time are the same,
+// but deepStrictEqual checks own properties
+assert.doesNotThrow(() => assert.deepEqual(date, date2));
+assert.doesNotThrow(() => assert.deepEqual(date2, date));
+assert.throws(() => assert.deepStrictEqual(date, date2),
+              re`${date} deepStrictEqual ${date2}`);
+assert.throws(() => assert.deepStrictEqual(date2, date),
+              re`${date2} deepStrictEqual ${date}`);
+
+class MyRegExp extends RegExp {
+  constructor(...args) {
+    super(...args);
+    this[0] = '1';
+  }
+}
+
+const re1 = new RegExp('test');
+const re2 = new MyRegExp('test');
+
+// deepEqual returns true as long as the regexp-specific properties
+// are the same, but deepStrictEqual checks all properties
+assert.doesNotThrow(() => assert.deepEqual(re1, re2));
+assert.throws(() => assert.deepStrictEqual(re1, re2),
+              re`${re1} deepStrictEqual ${re2}`);
+
+// For these weird cases, deepEqual should pass (at least for now),
+// 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
+]);
+
+for (const a of similar) {
+  for (const b of similar) {
+    if (a !== b) {
+      assert.doesNotThrow(() => assert.deepEqual(a, b));
+      assert.throws(() => assert.deepStrictEqual(a, b),
+                    re`${a} deepStrictEqual ${b}`);
+    }
+  }
+}
+
+/* eslint-enable */