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: https://github.com/nodejs/node/pull/11128
Refs: https://github.com/nodejs/node/pull/10282#issuecomment-274267895
Refs: https://github.com/nodejs/node/issues/10258#issuecomment-266963234
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
+  // For primitives / functions
-  // determined by comparing the values and ensuring the result
+  // (determined by typeof value !== 'object'),
-  // === 0.
+  // or null, equivalence is determined by === or ==.
-  } else if (actual instanceof Buffer && expected instanceof Buffer) {
+  if (isNullOrNonObj(actual) && isNullOrNonObj(expected)) {
-    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')) {
     return strict ? actual === expected : actual == expected;
+  }
 
-  // If both values are instances of typed arrays, wrap their underlying
+  // If they bypass the previous check, then at least
-  // ArrayBuffers in a Buffer to increase performance.
+  // one of them must be an non-null object.
-  // This optimization requires the arrays to have the same type as checked by
+  // If the other one is null or undefined, they must not be equal.
-  // Object.prototype.toString (pToString). Never perform binary
+  if (actual === null || actual === undefined ||
-  // comparisons for Float*Arrays, though, since +0 === -0 is true despite the
+      expected === null || expected === undefined)
-  // two values' bit patterns not being identical.
+    return false;
-  } 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
+  // Notes: Type tags are historical [[Class]] properties that can be set by
-  // determined by having the same number of owned properties (as verified
+  // FunctionTemplate::SetClassName() in C++ or Symbol.toStringTag in JS
-  // with Object.prototype.hasOwnProperty.call), the same set of keys
+  // and retrieved using Object.prototype.toString.call(obj) in JS
-  // (although not necessarily the same order), equivalent values for every
+  // See https://tc39.github.io/ecma262/#sec-object.prototype.tostring
-  // corresponding key, and an identical 'prototype' property. Note: this
+  // for a list of tags pre-defined in the spec.
-  // accounts for both named and indexed properties on Arrays.
+  // There are some unspecified tags in the wild too (e.g. typed array tags).
-  } else {
+  // Since tags can be altered, they only serve fast failures
-    memos = memos || {actual: [], expected: []};
+  const actualTag = objectToString(actual);
+  const expectedTag = objectToString(expected);
 
-    const actualIndex = memos.actual.indexOf(actual);
+  // Passing null or undefined to Object.getPrototypeOf() will throw
-    if (actualIndex !== -1) {
+  // so this must done after previous checks.
-      if (actualIndex === memos.expected.indexOf(expected)) {
+  // For strict comparison, objects should have
-        return true;
+  // a) The same prototypes.
-      }
+  // b) The same built-in type tags
+  if (strict) {
+    if (Object.getPrototypeOf(actual) !== Object.getPrototypeOf(expected)) {
+      return false;
+    }
+  }
+
+  // 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.
+
+  // 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.
+    }
+  }
+
+  // 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.
+    }
+  }
+
+  // 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.
     }
 
-    memos.actual.push(actual);
+    // Buffer.compare returns true, so actual.length === expected.length
-    memos.expected.push(expected);
+    // if they both only contain numeric keys, we don't need to exam further
-
+    if (Object.keys(actual).length === actual.length &&
-    return objEquiv(actual, expected, strict, memos);
+        Object.keys(expected).length === expected.length) {
+      return true;
+    }
   }
-}
 
-function isArguments(object) {
+  // For all other Object pairs, including Array objects,
-  return Object.prototype.toString.call(object) === '[object Arguments]';
+  // 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);
+
+  return objEquiv(actual, expected, strict, memos);
 }
 
 function objEquiv(a, b, strict, actualVisitedObjects) {
-  if (a === null || a === undefined || b === null || b === undefined)
+  // If one of them is a primitive, the other must be the same.
-    return false;
-
-  // If one 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 aKeys = Object.keys(a);
-  const aIsArgs = isArguments(a);
+  const bKeys = Object.keys(b);
-  const bIsArgs = isArguments(b);
-  if ((aIsArgs && !bIsArgs) || (!aIsArgs && bIsArgs))
-    return false;
-  const ka = Object.keys(a);
-  const kb = Object.keys(b);
   var key, i;
 
-  // The pair must have the same number of owned properties (keys
+  // The pair must have the same number of owned properties
-  // incorporates hasOwnProperty).
+  // (keys incorporates hasOwnProperty).
-  if (ka.length !== kb.length)
+  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();
+  aKeys.sort();
-  kb.sort();
+  bKeys.sort();
   // Cheap key test:
-  for (i = ka.length - 1; i >= 0; i--) {
+  for (i = aKeys.length - 1; i >= 0; i--) {
-    if (ka[i] !== kb[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--) {
+  for (i = aKeys.length - 1; i >= 0; i--) {
-    key = ka[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 */

test/parallel/test-assert.js

@@ -62,7 +62,6 @@ assert.doesNotThrow(makeBlock(a.notStrictEqual, 2, '2'),
                     'notStrictEqual(2, \'2\')');
 
 // deepEqual joy!
-// 7.2
 assert.doesNotThrow(makeBlock(a.deepEqual, new Date(2000, 3, 14),
                               new Date(2000, 3, 14)),
                     'deepEqual(new Date(2000, 3, 14), new Date(2000, 3, 14))');
@@ -84,7 +83,6 @@ assert.doesNotThrow(makeBlock(
                     'notDeepEqual(new Date(), new Date(2000, 3, 14))'
 );
 
-// 7.3
 assert.doesNotThrow(makeBlock(a.deepEqual, /a/, /a/));
 assert.doesNotThrow(makeBlock(a.deepEqual, /a/g, /a/g));
 assert.doesNotThrow(makeBlock(a.deepEqual, /a/i, /a/i));
@@ -104,20 +102,16 @@ assert.throws(makeBlock(a.deepEqual, /a/igm, /a/im),
 {
   const re1 = /a/g;
   re1.lastIndex = 3;
-
+  assert.doesNotThrow(makeBlock(a.deepEqual, re1, /a/g),
-  assert.throws(makeBlock(a.deepEqual, re1, /a/g),
+                      /^AssertionError: \/a\/g deepEqual \/a\/g$/);
-                /^AssertionError: \/a\/g deepEqual \/a\/g$/);
 }
 
-
-// 7.4
 assert.doesNotThrow(makeBlock(a.deepEqual, 4, '4'), 'deepEqual(4, \'4\')');
 assert.doesNotThrow(makeBlock(a.deepEqual, true, 1), 'deepEqual(true, 1)');
 assert.throws(makeBlock(a.deepEqual, 4, '5'),
               a.AssertionError,
               'deepEqual( 4, \'5\')');
 
-// 7.5
 // having the same number of owned properties && the same set of keys
 assert.doesNotThrow(makeBlock(a.deepEqual, {a: 4}, {a: 4}));
 assert.doesNotThrow(makeBlock(a.deepEqual, {a: 4, b: '2'}, {a: 4, b: '2'}));
@@ -210,7 +204,6 @@ assert.doesNotThrow(
   'notDeepStrictEqual(new Date(), new Date(2000, 3, 14))'
 );
 
-// 7.3 - strict
 assert.doesNotThrow(makeBlock(a.deepStrictEqual, /a/, /a/));
 assert.doesNotThrow(makeBlock(a.deepStrictEqual, /a/g, /a/g));
 assert.doesNotThrow(makeBlock(a.deepStrictEqual, /a/i, /a/i));
@@ -240,10 +233,9 @@ assert.throws(
 {
   const re1 = /a/;
   re1.lastIndex = 3;
-  assert.throws(makeBlock(a.deepStrictEqual, re1, /a/));
+  assert.doesNotThrow(makeBlock(a.deepStrictEqual, re1, /a/));
 }
 
-// 7.4 - strict
 assert.throws(makeBlock(a.deepStrictEqual, 4, '4'),
               a.AssertionError,
               'deepStrictEqual(4, \'4\')');
@@ -256,7 +248,6 @@ assert.throws(makeBlock(a.deepStrictEqual, 4, '5'),
               a.AssertionError,
               'deepStrictEqual(4, \'5\')');
 
-// 7.5 - strict
 // having the same number of owned properties && the same set of keys
 assert.doesNotThrow(makeBlock(a.deepStrictEqual, {a: 4}, {a: 4}));
 assert.doesNotThrow(makeBlock(a.deepStrictEqual,

test/parallel/test-child-process-spawnsync-input.js

@@ -87,7 +87,8 @@ options = {
 ret = spawnSync('cat', [], options);
 
 checkSpawnSyncRet(ret);
-assert.deepStrictEqual(ret.stdout, options.input);
+// Wrap options.input because Uint8Array and Buffer have different prototypes.
+assert.deepStrictEqual(ret.stdout, Buffer.from(options.input));
 assert.deepStrictEqual(ret.stderr, Buffer.from(''));
 
 verifyBufOutput(spawnSync(process.execPath, args));

test/parallel/test-fs-read.js

@@ -18,11 +18,11 @@ function test(bufferAsync, bufferSync, expected) {
           common.mustCall((err, bytesRead) => {
             assert.ifError(err);
             assert.strictEqual(bytesRead, expected.length);
-            assert.deepStrictEqual(bufferAsync, Buffer.from(expected));
+            assert.deepStrictEqual(bufferAsync, expected);
           }));
 
   const r = fs.readSync(fd, bufferSync, 0, expected.length, 0);
-  assert.deepStrictEqual(bufferSync, Buffer.from(expected));
+  assert.deepStrictEqual(bufferSync, expected);
   assert.strictEqual(r, expected.length);
 }