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
+64 −8
Closed

assert: fix deepEqual similar sets and maps bug #13426

Changes from 1 commit
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
34 changes: 29 additions & 5 deletions lib/assert.js
View file
Original file line number Diff line number Diff line change
@@ -285,7 +285,7 @@ function _deepEqual(actual, expected, strict, memos) {
return areEq;
}

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

@@ -296,8 +296,14 @@ function setHasSimilarElement(set, val1, strict, memo) {

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

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

return false;
@@ -314,21 +320,29 @@ 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. Initialized lazily so sets which only have value types can
// skip an extra allocation.
let bEntriesUsed = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'd rather you name it something like usedEntries, since bEntriesUsed sounds like a boolean predicating a need to use .entries()

Copy link
Contributor

Choose a reason for hiding this comment

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

If you choose to accept this nit, rename everywhere else as well...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure - I don't have a strong opinion so I've renamed it.

My concern with usedEntries is that it doesn't make it clear that we're marking entries in set b.


for (const val1 of a) {
// 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 (bEntriesUsed == null && typeof val1 === 'object')
Copy link
Contributor

@rmdm rmdm Jun 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Why is it important that a current val1 is an object? And why it should prevent initializing bEntriesUsed "for now" if it is not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bug doesn't affect value types because its impossible for two strings, numbers, bools, etc to be deepEqual to one another without being reference-equal. So in the case where the set or map only contains value typed entries, there's no need to allocate an extra set for bEntriesUsed.

Lazily allocating the set when needed is a microoptimization.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then maybe it worth to continue that optimization and checking type of val2 before adding it to set of usedEntries? Because presence of even single object in a set with large number of typed entries would lead to adding them to usedEntries without any need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe.. but in practice its very rare to mix types in set / map keys. And it still won't matter in strict mode because there's an early return for value types anyway.

Uhhhh but not in non-strict mode. Huh - looks like this also doesn't throw:

assert.deepEqual(new Set([3, '3']), new Set([3, 4]));

fixes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... Fixed. PR updated so this behaves correctly too.

bEntriesUsed = new Set();

if (!setHasSimilarElement(b, val1, bEntriesUsed, strict, memo)) {
return false;
}
}

return true;
}

function mapHasSimilarEntry(map, key1, item1, strict, memo) {
function mapHasSimilarEntry(map, key1, item1, bEntriesUsed, strict, memo) {
// To be able to handle cases like:
// Map([[1, 'a'], ['1', 'b']]) vs Map([['1', 'a'], [1, 'b']])
// or:
@@ -349,8 +363,13 @@ function mapHasSimilarEntry(map, key1, item1, strict, memo) {
if (key2 === key1)
continue;

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

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

let bEntriesUsed = null;

for (const [key1, item1] of a) {
if (bEntriesUsed == null && typeof key1 === 'object')
bEntriesUsed = 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, bEntriesUsed, strict, memo))
return false;
}

14 changes: 14 additions & 0 deletions test/parallel/test-assert-deep.js
View file
Original file line number Diff line number Diff line change
@@ -187,6 +187,20 @@ assertOnlyDeepEqual(new Map([['a', '1']]), new Map([['a', 1]]));

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

// Discussion of these test cases here - https://github.com/nodejs/node/issues/13347
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: just put Ref: https://github.com/nodejs/node/issues/13347

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

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

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