Skip to content
Closed
100 changes: 99 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,12 @@ function _deepEqual(actual, expected, strict, memos) {
}
}

// For all other Object pairs, including Array objects,
// 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 Sets and Maps, equal contents
// Note: this accounts for both named and indexed properties on Arrays.

// Use memos to handle cycles.
Expand All @@ -280,9 +282,89 @@ function _deepEqual(actual, expected, strict, memos) {
memos.actual.push(actual);
memos.expected.push(expected);


Copy link
Member

Choose a reason for hiding this comment

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

Just noticed..unrelated white space change?

return objEquiv(actual, expected, strict, memos);
}

function setEquiv(a, b, strict, actualVisitedObjects) {
// This code currently returns false for this pair of sets:
// assert.deepEqual(new Set(['1', 1]), new Set([1]))
//
// In theory, all the items in the first set have a corresponding == value in
// the second set, but the sets have different sizes. Its a silly case,
// and more evidence that deepStrictEqual should always be preferred over
// deepEqual. The implementation currently returns false, which is simpler
// and slightly faster.
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.


outer: for (const val1 of a) {
if (!b.has(val1)) {
if (!strict || (typeof val1 === 'object' && val1 !== null)) {
// The value doesn't exist in the second set by reference, so we'll go
// hunting for something thats deep-equal to it. Note that this is
// O(n^2) complexity, and will get slower if large, very similar sets /
// maps are nested inside. Unfortunately there's no real way around
// this.
for (const val2 of b) {
Copy link
Member

Choose a reason for hiding this comment

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

If I am reading the code correctly, is it similar to...

for (const val1 of a) {
  if (!b.has(val1)) {
    if (strict) { return false; }
    if (typeof val1 !== 'object' || val1 === null)) { return false; }

   // the following can be wrapped into a funciton...
    var bHasSimilarElement = false;
    for (const val2 of b) {
      if (_deepEqual(val1, val2, strict, actualVisitedObjects)) {
        bHasSimilarElement = true;
        break;
      }
    }

    if (!bHasSimilarElement) {
      return false;
    }
  }
}

?

Personally I prefer to stay away from labels...

Copy link
Member

@joyeecheung joyeecheung Apr 1, 2017

Choose a reason for hiding this comment

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

Also on #12142 (comment), I think the primitive check can be replaced with util.isPrimitive(val) so it is more inline with objEquiv and probably more future proof

Copy link
Contributor Author

@josephg josephg Apr 1, 2017

Choose a reason for hiding this comment

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

That transformation is close... Your version is equivalent to if (strict || ...) whereas it should be if (strict && ....)

But yes, I could write the function that way. I'm usually a little happier with early returns for stuff like that, but I know some people don't like them and I'm not sure where nodejs stands on it.

As for ditching the loop labels, I'm not convinced. Your version is twice as long. If I pull that code out into a single-use function then it'd lose some locality. And we'd have to have another similar-but-different copy of that function for mapEquiv as well. (I'd love to write something we can reuse, but it would need to yield back all of the matches for mapEquiv, which means taking a callback, returning an iterator or being a generator function. I'd rather avoid all of those options for this.

Copy link
Contributor Author

@josephg josephg Apr 1, 2017

Choose a reason for hiding this comment

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

As for replacing it with stuff in util, I can either do (!isPrimitive(val1) && !isFunction(val1)) or isObject(val1). The latter has exactly the behaviour we want, but the fact that it returns true for arrays is weird and non-obvious to me. Do you have a preference?

Copy link
Member

@joyeecheung joyeecheung Apr 1, 2017

Choose a reason for hiding this comment

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

Oh yes, missed the conjunction..so on a higher-level, this is:

for (const val1 of a) {
  if (!b.has(val1) &&
      (strict && util.isPrimitive(val1) || isFunction(val1)) ||
      !hasDeepEqual(b, val1, strict, actualVisitedObjects)) {
    return false;
  }
}

?
EDIT: the first version is..totally wrong.. :S

Copy link
Member

@joyeecheung joyeecheung Apr 1, 2017

Choose a reason for hiding this comment

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

Also I don't think creating a function that takes callbacks is a good idea either, but that's not what I am proposing here..just think small functions and conjunctions would be easier to read than labels

EDIT: .. doesn't really matter if those small functions can be reused
(mapEquiv can call its own small functions too..). Although function calls incur overhead, but since deepEqual has too many complex logic and we use a pattern like this already, I think it's fine to trade this overhead for readability. As you can see we don't inline setEquiv even though it's not reusable, and we don't make a function taking callbacks to abstract over setEquiv/mapEquiveither. All for readability, eh ;)

Copy link
Contributor Author

@josephg josephg Apr 1, 2017

Choose a reason for hiding this comment

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

Yes thats right - except we can also exclude functions from the search. And I think the logic is still wrong. I'm reasonably sure this is correct:

  for (const val1 of a) {
    if (!b.has(val1)
      && ((strict && (util.isObject(val1) || util.isFunction(val1)))
        || setHasSimilarElement(b, val11, strict, actualVisitedObjects)))
      return false;
    }
  }

(And if its not correct I think the tests should catch any mistakes.)

I just deleted a bunch of text justifying not changing the code, but I think I'm just getting exhausted 7 reviewers rubbing away at, now, stylistic changes. But I agree - the code is cleaner the way you've suggested. I'll update the PR...

Copy link
Member

@joyeecheung joyeecheung Apr 1, 2017

Choose a reason for hiding this comment

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

Also also..to be clear, I don't actually feel strong enough about the labels to block this PR, because support for Maps and Sets are definitely appreciated and outweight readability preferences! Although on the other hand, I do feel stronger about the doc update ...even the comments in_deepEqual are updated, surely the documentation deserves some love right ;)
EDIT: oh I saw the doc update now, thanks

Copy link
Contributor Author

@josephg josephg Apr 1, 2017

Choose a reason for hiding this comment

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

See my comments below - I updated the documentation a couple hours ago.

Also I was still wrong - here's the actually correct logic. At least, the tests pass with this version. I'm pretty sure its right (though I convinced myself the last version was right too).

    if (!b.has(val1)
        && (!strict || (!util.isObject(val1) && !util.isFunction(val1)))
        && !setHasSimilarElement(b, val1, strict, actualVisitedObjects)) {
      return false;
    }

if (_deepEqual(val1, val2, strict, actualVisitedObjects))
continue outer;
}
}

// Not found!
return false;
}
}

return true;
}

function mapEquiv(a, b, strict, actualVisitedObjects) {
// Caveat: In non-strict mode, this implementation does not handle cases
// where maps contain two equivalent-but-not-reference-equal keys.
//
// For example, maps like this are currently considered not equivalent:
if (a.size !== b.size)
return false;

outer: for (const [key1, item1] of a) {
// To be able to handle cases like:
// Map([[1, 'a'], ['1', 'b']]) vs Map([['1', 'a'], [1, 'b']])
// or:
// Map([[{}, 'a'], [{}, 'b']]) vs Map([[{}, 'b'], [{}, 'a']])
// ... we need to consider *all* matching keys, not just the first we find.

// This check is not strictly necessary if we run the loop below, but it
// improves performance of the common case when reference-equal keys exist
// (which includes all primitive-valued keys).
if (b.has(key1)) {
if (_deepEqual(item1, b.get(key1), strict, actualVisitedObjects))
continue outer;
}

// Hunt for keys which are deep-equal to key1 in b. Just like setEquiv
// above, this hunt makes this function O(n^2) when using objects and lists
// as keys
if (!strict || (typeof key1 === 'object' && key1 !== null)) {
for (const [key2, item2] of b) {
// Just for performance. We already checked these keys above.
if (key2 === key1)
continue;

if (_deepEqual(key1, key2, strict, actualVisitedObjects) &&
_deepEqual(item1, item2, strict, actualVisitedObjects)) {
continue outer;
}
}
}

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 +389,22 @@ function objEquiv(a, b, strict, actualVisitedObjects) {
return false;
}

// Sets and maps don't have their entries accessible via normal object
// properties.
if (isSet(a)) {
if (!isSet(b) || !setEquiv(a, b, strict, actualVisitedObjects))
return false;
} else if (isSet(b)) {
return false;
}

if (isMap(a)) {
if (!isMap(b) || !mapEquiv(a, b, 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
161 changes: 160 additions & 1 deletion test/parallel/test-assert-deep.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,11 +100,170 @@ const similar = new Set([
for (const a of similar) {
for (const b of similar) {
if (a !== b) {
assert.doesNotThrow(() => assert.deepEqual(a, b));
assert.deepEqual(a, b);
assert.throws(() => assert.deepStrictEqual(a, b),
re`${a} deepStrictEqual ${b}`);
}
}
}

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));
}

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

assert.doesNotThrow(() => assert.deepEqual(b, a));
assert.throws(() => assert.deepStrictEqual(b, a));
}

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

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 Set([[1, 2], [3, 4]]), new Set([[3, 4], [1, 2]]));

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(), {});

assertOnlyDeepEqual(new Set(['1']), new Set([1]));

assertOnlyDeepEqual(new Map([['1', 'a']]), new Map([[1, 'a']]));
assertOnlyDeepEqual(new Map([['a', '1']]), new Map([['a', 1]]));

// This is an awful case, where a map contains multiple equivalent keys:
assertOnlyDeepEqual(
new Map([[1, 'a'], ['1', 'b']]),
new Map([['1', 'a'], [1, 'b']])
);
assertDeepAndStrictEqual(
new Map([[{}, 'a'], [{}, 'b']]),
new Map([[{}, 'b'], [{}, 'a']])
);

{
const values = [
123,
Infinity,
0,
null,
undefined,
false,
true,
{},
[],
() => {},
];
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']]))
);

{
// Two equivalent sets / maps with different key/values applied shouldn't be
// the same. This is a terrible idea to do in practice, but deepEqual should
// still check for it.
const s1 = new Set();
const s2 = new Set();
s1.x = 5;
assertNotDeepOrStrict(s1, s2);

const m1 = new Map();
const m2 = new Map();
m1.x = 5;
assertNotDeepOrStrict(m1, m2);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a case where there is a circular reference?

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.


{
// Circular references.
const s1 = new Set();
s1.add(s1);
const s2 = new Set();
s2.add(s2);
assertDeepAndStrictEqual(s1, s2);

const m1 = new Map();
m1.set(2, m1);
const m2 = new Map();
m2.set(2, m2);
assertDeepAndStrictEqual(m1, m2);

const m3 = new Map();
m3.set(m3, 2);
const m4 = new Map();
m4.set(m4, 2);
assertDeepAndStrictEqual(m3, m4);
}

/* eslint-enable */