1berry/nodejs
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

assert: fix deepEqual similar sets and maps bug

Browse Source 
This fixes a bug where deepEqual and deepStrictEqual would have
incorrect behaviour in sets and maps containing multiple equivalent
keys.

PR-URL: https://github.com/nodejs/node/pull/13426
Fixes: https://github.com/nodejs/node/issues/13347
Refs: https://github.com/nodejs/node/pull/12142
Reviewed-By: Refael Ackermann <refack@gmail.com>
This commit is contained in:
Joseph Gentle 2017-06-02 20:26:25 -04:00 committed by Refael Ackermann
parent 30a20bda7d
commit 7cddcc9715
2 changed files with 64 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

50
lib/assert.js
View File

@ -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).
@ -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;
@ -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:
@ -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;
@ -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;
}
}
@ -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;
}

22
test/parallel/test-assert-deep.js
View File

@ -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']]),