Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

assert: fix deepEqual similar sets and maps bug #13426

Closed
wants to merge 4 commits into from
Closed

assert: fix deepEqual similar sets and maps bug #13426

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3995b72
assert: fix deepEqual similar sets and maps bug
josephg Jun 3, 2017
067be29
assert: Fix nits from review by @refack
josephg Jun 3, 2017
957d610
assert: Added and fixed more failing test cases comparing maps and sets
josephg Jun 3, 2017
4b97dee
assert: Fixed linter errors
josephg Jun 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 42 additions & 8 deletions lib/assert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,9 +285,12 @@ function _deepEqual(actual, expected, strict, memos) {
return areEq;
}

function setHasSimilarElement(set, val1, strict, memo) {
if (set.has(val1))
function setHasSimilarElement(set, val1, usedEntries, strict, memo) {
if (set.has(val1)) {
if (usedEntries)
usedEntries.add(val1);
return true;
}

// In strict mode the only things which can match a primitive or a function
// will already be detected by set.has(val1).
Expand All @@ -296,8 +299,14 @@ function setHasSimilarElement(set, val1, strict, memo) {

// Otherwise go looking.
for (const val2 of set) {
if (_deepEqual(val1, val2, strict, memo))
if (usedEntries && usedEntries.has(val2))
continue;

if (_deepEqual(val1, val2, strict, memo)) {
if (usedEntries)
usedEntries.add(val2);
return true;
}
}

return false;
Expand All @@ -314,21 +323,33 @@ function setEquiv(a, b, strict, memo) {
if (a.size !== b.size)
return false;

// This is a set of the entries in b which have been consumed in our pairwise
// comparison.
//
// When the sets contain only value types (eg, lots of numbers), and we're in
// strict mode, we don't need to match off the entries in a pairwise way. In
// that case this initialization is done lazily to avoid the allocation &
// bookkeeping cost. Unfortunately, we can't get away with that in non-strict
// mode.
let usedEntries = null;

for (const val1 of a) {
if (usedEntries == null && (!strict || typeof val1 === 'object'))
usedEntries = new Set();

// If the value doesn't exist in the second set by reference, and its an
// object or an array we'll need to 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.
if (!setHasSimilarElement(b, val1, strict, memo)) {
if (!setHasSimilarElement(b, val1, usedEntries, strict, memo))
return false;
}
}

return true;
}

function mapHasSimilarEntry(map, key1, item1, strict, memo) {
function mapHasSimilarEntry(map, key1, item1, usedEntries, strict, memo) {
// To be able to handle cases like:
// Map([[1, 'a'], ['1', 'b']]) vs Map([['1', 'a'], [1, 'b']])
// or:
Expand All @@ -338,8 +359,11 @@ function mapHasSimilarEntry(map, key1, item1, strict, memo) {
// This check is not strictly necessary. The loop performs this check, but
// doing it here improves performance of the common case when reference-equal
// keys exist (which includes all primitive-valued keys).
if (map.has(key1) && _deepEqual(item1, map.get(key1), strict, memo))
if (map.has(key1) && _deepEqual(item1, map.get(key1), strict, memo)) {
if (usedEntries)
usedEntries.add(key1);
return true;
}

if (strict && (util.isPrimitive(key1) || util.isFunction(key1)))
return false;
Expand All @@ -349,8 +373,13 @@ function mapHasSimilarEntry(map, key1, item1, strict, memo) {
if (key2 === key1)
continue;

if (usedEntries && usedEntries.has(key2))
continue;

if (_deepEqual(key1, key2, strict, memo) &&
_deepEqual(item1, item2, strict, memo)) {
if (usedEntries)
usedEntries.add(key2);
return true;
}
}
Expand All @@ -366,10 +395,15 @@ function mapEquiv(a, b, strict, memo) {
if (a.size !== b.size)
return false;

let usedEntries = null;

for (const [key1, item1] of a) {
if (usedEntries == null && (!strict || typeof key1 === 'object'))
usedEntries = new Set();

// Just like setEquiv above, this hunt makes this function O(n^2) when
// using objects and lists as keys
if (!mapHasSimilarEntry(b, key1, item1, strict, memo))
if (!mapHasSimilarEntry(b, key1, item1, usedEntries, strict, memo))
return false;
}

Expand Down
22 changes: 22 additions & 0 deletions test/parallel/test-assert-deep.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,28 @@ assertOnlyDeepEqual(new Map([['a', '1']]), new Map([['a', 1]]));

assertDeepAndStrictEqual(new Set([{}]), new Set([{}]));

// Ref: https://github.com/nodejs/node/issues/13347
assertNotDeepOrStrict(
new Set([{a: 1}, {a: 1}]),
new Set([{a: 1}, {a: 2}])
);
assertNotDeepOrStrict(
new Set([{a: 1}, {a: 1}, {a: 2}]),
new Set([{a: 1}, {a: 2}, {a: 2}])
);
assertNotDeepOrStrict(
new Map([[{x: 1}, 5], [{x: 1}, 5]]),
new Map([[{x: 1}, 5], [{x: 2}, 5]])
);

assertNotDeepOrStrict(new Set([3, '3']), new Set([3, 4]));
assertNotDeepOrStrict(new Map([[3, 0], ['3', 0]]), new Map([[3, 0], [4, 0]]));

assertNotDeepOrStrict(
new Set([{a: 1}, {a: 1}, {a: 2}]),
new Set([{a: 1}, {a: 2}, {a: 2}])
);

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