From db2e093e055c9e359745fdf0f2eaf35858455185 Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Sat, 19 Aug 2017 02:06:27 -0300
Subject: [PATCH] assert: handle enumerable symbol keys
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

PR-URL: https://github.com/nodejs/node/pull/15169
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
---
 doc/api/assert.md                        |  25 +++--
 lib/assert.js                            | 125 ++++++++++++++---------
 test/parallel/test-assert-deep.js        |  28 +++++
 test/parallel/test-net-normalize-args.js |  11 +-
 4 files changed, 127 insertions(+), 62 deletions(-)

diff --git a/doc/api/assert.md b/doc/api/assert.md
index ca7f048f9e04f1..a3cb02c7bca625 100644
--- a/doc/api/assert.md
+++ b/doc/api/assert.md
@@ -46,12 +46,11 @@ Primitive values are compared with the [Abstract Equality Comparison][]
 
 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
-potentially surprising results. For example, the following example does not
-throw an `AssertionError` because the properties on the [`RegExp`][] object are
-not enumerable:
+[`[[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:
 
 ```js
 // WARNING: This does not throw an AssertionError!
@@ -109,6 +108,9 @@ parameter is an instance of an `Error` then it will be thrown instead of the
 <!-- YAML
 added: v1.2.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/15169
+    description: Enumerable symbol properties are now compared.
   - version: REPLACEME
     pr-url: https://github.com/nodejs/node/pull/15036
     description: NaN is now compared using the [SameValueZero][] comparison.
@@ -132,7 +134,7 @@ changes:
 * `expected` {any}
 * `message` {any}
 
-Generally identical to `assert.deepEqual()` with a few exceptions:
+Similar to `assert.deepEqual()` with the following exceptions:
 
 1. Primitive values besides `NaN` are compared using the [Strict Equality
    Comparison][] ( `===` ). Set and Map values, Map keys and `NaN` are compared
@@ -143,6 +145,7 @@ Generally identical to `assert.deepEqual()` with a few exceptions:
 3. [Type tags][Object.prototype.toString()] of objects should be the same.
 4. [Object wrappers][] are compared both as objects and unwrapped values.
 5. `0` and `-0` are not considered equal.
+6. Enumerable own [`Symbol`][] properties are compared as well.
 
 ```js
 const assert = require('assert');
@@ -185,6 +188,13 @@ assert.deepStrictEqual(-0, -0);
 // OK
 assert.deepStrictEqual(0, -0);
 // AssertionError: 0 deepStrictEqual -0
+
+const symbol1 = Symbol();
+const symbol2 = Symbol();
+assert.deepStrictEqual({ [symbol1]: 1 }, { [symbol1]: 1 });
+// OK, because it is the same symbol on both objects.
+assert.deepStrictEqual({ [symbol1]: 1 }, { [symbol2]: 1 });
+// Fails because symbol1 !== symbol2!
 ```
 
 If the values are not equal, an `AssertionError` is thrown with a `message`
@@ -712,6 +722,7 @@ For more information, see
 [`Object.is()`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/is
 [`RegExp`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions
 [`Set`]: https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Set
+[`Symbol`]: https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Symbol
 [`TypeError`]: errors.html#errors_class_typeerror
 [`assert.deepEqual()`]: #assert_assert_deepequal_actual_expected_message
 [`assert.deepStrictEqual()`]: #assert_assert_deepstrictequal_actual_expected_message
diff --git a/lib/assert.js b/lib/assert.js
index b3e66aa74fd9be..2aaf80eecc7e77 100644
--- a/lib/assert.js
+++ b/lib/assert.js
@@ -24,6 +24,7 @@ const { compare } = process.binding('buffer');
 const { isSet, isMap, isDate, isRegExp } = process.binding('util');
 const { objectToString } = require('internal/util');
 const errors = require('internal/errors');
+const { propertyIsEnumerable } = Object.prototype;
 
 // The assert module provides functions that throw
 // AssertionError's when particular conditions are not met. The
@@ -165,7 +166,7 @@ function isObjectOrArrayTag(tag) {
 // For strict comparison, objects should have
 // a) The same built-in type tags
 // b) The same prototypes.
-function strictDeepEqual(actual, expected) {
+function strictDeepEqual(actual, expected, memos) {
   if (typeof actual !== 'object') {
     return typeof actual === 'number' && Number.isNaN(actual) &&
       Number.isNaN(expected);
@@ -186,12 +187,12 @@ function strictDeepEqual(actual, expected) {
     // Check for sparse arrays and general fast path
     if (actual.length !== expected.length)
       return false;
-    // Skip testing the part below and continue in the callee function.
-    return;
+    // Skip testing the part below and continue with the keyCheck.
+    return keyCheck(actual, expected, true, memos);
   }
   if (actualTag === '[object Object]') {
-    // Skip testing the part below and continue in the callee function.
-    return;
+    // Skip testing the part below and continue with the keyCheck.
+    return keyCheck(actual, expected, true, memos);
   }
   if (isDate(actual)) {
     if (actual.getTime() !== expected.getTime()) {
@@ -215,10 +216,8 @@ function strictDeepEqual(actual, expected) {
     }
     // 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;
-    }
+    return keyCheck(actual, expected, true, memos, actual.length,
+                    expected.length);
   } else if (typeof actual.valueOf === 'function') {
     const actualValue = actual.valueOf();
     // Note: Boxed string keys are going to be compared again by Object.keys
@@ -232,15 +231,14 @@ function strictDeepEqual(actual, expected) {
         lengthActual = actual.length;
         lengthExpected = expected.length;
       }
-      if (Object.keys(actual).length === lengthActual &&
-        Object.keys(expected).length === lengthExpected) {
-        return true;
-      }
+      return keyCheck(actual, expected, true, memos, lengthActual,
+                      lengthExpected);
     }
   }
+  return keyCheck(actual, expected, true, memos);
 }
 
-function looseDeepEqual(actual, expected) {
+function looseDeepEqual(actual, expected, memos) {
   if (actual === null || typeof actual !== 'object') {
     if (expected === null || typeof expected !== 'object') {
       // eslint-disable-next-line eqeqeq
@@ -274,26 +272,10 @@ function looseDeepEqual(actual, expected) {
   } else if (isArguments(actualTag) || isArguments(expectedTag)) {
     return false;
   }
+  return keyCheck(actual, expected, false, memos);
 }
 
-function innerDeepEqual(actual, expected, strict, memos) {
-  // All identical values are equivalent, as determined by ===.
-  if (actual === expected) {
-    if (actual !== 0)
-      return true;
-    return strict ? Object.is(actual, expected) : true;
-  }
-
-  // Returns a boolean if (not) equal and undefined in case we have to check
-  // further.
-  const partialCheck = strict ?
-    strictDeepEqual(actual, expected) :
-    looseDeepEqual(actual, expected);
-
-  if (partialCheck !== undefined) {
-    return partialCheck;
-  }
-
+function keyCheck(actual, expected, strict, memos, lengthA, lengthB) {
   // For all remaining Object pairs, including Array, objects and Maps,
   // equivalence is determined by having:
   // a) The same number of owned enumerable properties
@@ -301,6 +283,51 @@ function innerDeepEqual(actual, expected, strict, memos) {
   // 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.
+  var aKeys = Object.keys(actual);
+  var bKeys = Object.keys(expected);
+  var i;
+
+  // The pair must have the same number of owned properties.
+  if (aKeys.length !== bKeys.length)
+    return false;
+
+  if (strict) {
+    var symbolKeysA = Object.getOwnPropertySymbols(actual);
+    var symbolKeysB = Object.getOwnPropertySymbols(expected);
+    if (symbolKeysA.length !== 0) {
+      symbolKeysA = symbolKeysA.filter((k) =>
+        propertyIsEnumerable.call(actual, k));
+      symbolKeysB = symbolKeysB.filter((k) =>
+        propertyIsEnumerable.call(expected, k));
+      if (symbolKeysA.length !== symbolKeysB.length)
+        return false;
+    } else if (symbolKeysB.length !== 0 && symbolKeysB.filter((k) =>
+      propertyIsEnumerable.call(expected, k)).length !== 0) {
+      return false;
+    }
+    if (lengthA !== undefined) {
+      if (aKeys.length !== lengthA || bKeys.length !== lengthB)
+        return false;
+      if (symbolKeysA.length === 0)
+        return true;
+      aKeys = [];
+      bKeys = [];
+    }
+    if (symbolKeysA.length !== 0) {
+      aKeys.push(...symbolKeysA);
+      bKeys.push(...symbolKeysB);
+    }
+  }
+
+  // Cheap key test:
+  const keys = {};
+  for (i = 0; i < aKeys.length; i++) {
+    keys[aKeys[i]] = true;
+  }
+  for (i = 0; i < aKeys.length; i++) {
+    if (keys[bKeys[i]] === undefined)
+      return false;
+  }
 
   // Use memos to handle cycles.
   if (memos === undefined) {
@@ -323,25 +350,6 @@ function innerDeepEqual(actual, expected, strict, memos) {
     memos.position++;
   }
 
-  const aKeys = Object.keys(actual);
-  const bKeys = Object.keys(expected);
-  var i;
-
-  // The pair must have the same number of owned properties
-  // (keys incorporates hasOwnProperty).
-  if (aKeys.length !== bKeys.length)
-    return false;
-
-  // Cheap key test:
-  const keys = {};
-  for (i = 0; i < aKeys.length; i++) {
-    keys[aKeys[i]] = true;
-  }
-  for (i = 0; i < aKeys.length; i++) {
-    if (keys[bKeys[i]] === undefined)
-      return false;
-  }
-
   memos.actual.set(actual, memos.position);
   memos.expected.set(expected, memos.position);
 
@@ -353,6 +361,21 @@ function innerDeepEqual(actual, expected, strict, memos) {
   return areEq;
 }
 
+function innerDeepEqual(actual, expected, strict, memos) {
+  // All identical values are equivalent, as determined by ===.
+  if (actual === expected) {
+    if (actual !== 0)
+      return true;
+    return strict ? Object.is(actual, expected) : true;
+  }
+
+  // Check more closely if actual and expected are equal.
+  if (strict === true)
+    return strictDeepEqual(actual, expected, memos);
+
+  return looseDeepEqual(actual, expected, memos);
+}
+
 function setHasEqualElement(set, val1, strict, memo) {
   // Go looking.
   for (const val2 of set) {
diff --git a/test/parallel/test-assert-deep.js b/test/parallel/test-assert-deep.js
index b84ceaff896688..2236a76a3c3502 100644
--- a/test/parallel/test-assert-deep.js
+++ b/test/parallel/test-assert-deep.js
@@ -507,8 +507,36 @@ assert.doesNotThrow(
   boxedSymbol.slow = true;
   assertNotDeepOrStrict(boxedSymbol, {});
 }
+
 // Minus zero
 assertOnlyDeepEqual(0, -0);
 assertDeepAndStrictEqual(-0, -0);
 
+// Handle symbols (enumerable only)
+{
+  const symbol1 = Symbol();
+  const obj1 = { [symbol1]: 1 };
+  const obj2 = { [symbol1]: 1 };
+  const obj3 = { [Symbol()]: 1 };
+  // Add a non enumerable symbol as well. It is going to be ignored!
+  Object.defineProperty(obj2, Symbol(), { value: 1 });
+  assertOnlyDeepEqual(obj1, obj3);
+  assertDeepAndStrictEqual(obj1, obj2);
+  // TypedArrays have a fast path. Test for this as well.
+  const a = new Uint8Array(4);
+  const b = new Uint8Array(4);
+  a[symbol1] = true;
+  b[symbol1] = false;
+  assertOnlyDeepEqual(a, b);
+  b[symbol1] = true;
+  assertDeepAndStrictEqual(a, b);
+  // The same as TypedArrays is valid for boxed primitives
+  const boxedStringA = new String('test');
+  const boxedStringB = new String('test');
+  boxedStringA[symbol1] = true;
+  assertOnlyDeepEqual(boxedStringA, boxedStringB);
+  boxedStringA[symbol1] = true;
+  assertDeepAndStrictEqual(a, b);
+}
+
 /* eslint-enable */
diff --git a/test/parallel/test-net-normalize-args.js b/test/parallel/test-net-normalize-args.js
index fe5444e299b644..4677b396e25890 100644
--- a/test/parallel/test-net-normalize-args.js
+++ b/test/parallel/test-net-normalize-args.js
@@ -13,10 +13,13 @@ function validateNormalizedArgs(input, output) {
 }
 
 // Test creation of normalized arguments.
-validateNormalizedArgs([], [{}, null]);
-validateNormalizedArgs([{ port: 1234 }], [{ port: 1234 }, null]);
-validateNormalizedArgs([{ port: 1234 }, assert.fail],
-                       [{ port: 1234 }, assert.fail]);
+const res = [{}, null];
+res[normalizedArgsSymbol] = true;
+validateNormalizedArgs([], res);
+res[0].port = 1234;
+validateNormalizedArgs([{ port: 1234 }], res);
+res[1] = assert.fail;
+validateNormalizedArgs([{ port: 1234 }, assert.fail], res);
 
 // Connecting to the server should fail with a standard array.
 {