Skip to content
Closed
49 changes: 48 additions & 1 deletion lib/assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -262,11 +263,18 @@ function _deepEqual(actual, expected, strict, memos) {
}
}

// For all other Object pairs, including Array objects,
if (isSet(actual)) {
return isSet(expected) && setEquiv(actual, expected);
} else if (isSet(expected)) {
return false;
}

// 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 Maps, strict-equal keys mapping to deep-equal values
// Note: this accounts for both named and indexed properties on Arrays.

// Use memos to handle cycles.
Expand All @@ -283,6 +291,26 @@ function _deepEqual(actual, expected, strict, memos) {
return objEquiv(actual, expected, strict, memos);
}

function setEquiv(a, b) {
// This behaviour will work for any sets with contents that have
// strict-equality. That is, it will return false if the two sets contain
// equivalent objects that aren't reference-equal. We could support that, but
// it would require scanning each pairwise set of not strict-equal items,
// which is O(n^2), and would get exponentially worse if sets are nested. So
// for now we simply won't support deep equality checking set items or map
// keys.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would actually prefer the awful performance over always using strict equality… @nodejs/collaborators thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, I think I'd value correct over performant, so yeah, I agree with @addaleax.

Copy link
Contributor Author

@josephg josephg Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For tests, I agree. I'm nervous people might be using assert.deepEqual inside runtime code, although I just checked the node_modules directory of a project I'm working on with ~400 transitive dependancies and only jsprim calls assert.deepEqual outside of a test, and thats a in a very minor way. I'll wait for some more feedback, but unless there's any strong objections I'll change the behaviour to be correct and slow.

Copy link
Contributor Author

@josephg josephg Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... Another strong argument in favor of making it correct > fast is that it'll be more compatible with the current implementation. If someone currently has a test that reads assert.deepStrictEqual(new Set([{x:5}]), new Set([{x:5}])) that test will currently pass because the sets won't be compared. With the PR in its current state, that test will start to fail.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josephg Maybe just update this PR and see if somebody objects? If we need to go back, the current HEAD is at f051840.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... Done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#14258 just landed and reduces the complexity to O(n log n). Therefore the performance should not be of much concern anymore.

if (a.size !== b.size)
return false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I think deepEqual should be correct to reject sets of different sizes.


var val;
for (val of a) {
if (!b.has(val))
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))
Expand All @@ -307,6 +335,25 @@ function objEquiv(a, b, strict, actualVisitedObjects) {
return false;
}

if (isMap(a)) {
if (!isMap(b))
return false;

if (a.size !== b.size)
return false;

var item;
for ([key, item] of a) {
if (!b.has(key))
return false;

if (!_deepEqual(item, b.get(key), 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--) {
Expand Down
101 changes: 101 additions & 0 deletions test/parallel/test-assert-deep.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,4 +107,105 @@ for (const a of similar) {
}
}

function assertDeepAndStrictEqual(a, b) {
assert.doesNotThrow(() => assert.deepEqual(a, b));
assert.doesNotThrow(() => assert.deepStrictEqual(a, b));

assert.doesNotThrow(() => assert.deepEqual(b, a));
assert.doesNotThrow(() => 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));
}

// es6 Maps and Sets
assertDeepAndStrictEqual(new Set(), new Set());
assertDeepAndStrictEqual(new Map(), new Map());

// deepEqual only works with primitive key values and reference-equal values in
// sets and map keys avoid O(n^d) complexity (where d is depth)
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 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(), {});

{
const values = [
123,
Infinity,
0,
null,
undefined,
false,
true,
{}, // Objects, lists and functions are ok if they're in by reference.
[],
() => {},
];
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']]))
);

/* eslint-enable */