From 3995b728c1e6a0afc9f898ae0973146da94412ed Mon Sep 17 00:00:00 2001 From: Joseph Gentle Date: Sat, 3 Jun 2017 10:26:25 +1000 Subject: [PATCH 1/4] assert: fix deepEqual similar sets and maps bug This fixes a bug where deepEqual and deepStrictEqual would have incorrect behaviour in sets and maps containing multiple equivalent keys. Fixes: https://github.com/nodejs/node/issues/13347 Refs: https://github.com/nodejs/node/pull/12142 --- lib/assert.js | 34 ++++++++++++++++++++++++++----- test/parallel/test-assert-deep.js | 14 +++++++++++++ 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/lib/assert.js b/lib/assert.js index 5aca51119484b6..5c70e0ff805b17 100644 --- a/lib/assert.js +++ b/lib/assert.js @@ -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,13 +320,21 @@ 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; + 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') + bEntriesUsed = new Set(); + + if (!setHasSimilarElement(b, val1, bEntriesUsed, strict, memo)) { return false; } } @@ -328,7 +342,7 @@ function setEquiv(a, b, strict, memo) { 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; } diff --git a/test/parallel/test-assert-deep.js b/test/parallel/test-assert-deep.js index f6fed8411b5213..162499b224fd21 100644 --- a/test/parallel/test-assert-deep.js +++ b/test/parallel/test-assert-deep.js @@ -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 +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']]), From 067be29c9a3ad2b328f217b57e749c5e153189d1 Mon Sep 17 00:00:00 2001 From: Joseph Gentle Date: Sat, 3 Jun 2017 23:10:29 +1000 Subject: [PATCH 2/4] assert: Fix nits from review by @refack --- lib/assert.js | 32 +++++++++++++++---------------- test/parallel/test-assert-deep.js | 2 +- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/lib/assert.js b/lib/assert.js index 5c70e0ff805b17..66b2d18815386f 100644 --- a/lib/assert.js +++ b/lib/assert.js @@ -285,7 +285,7 @@ function _deepEqual(actual, expected, strict, memos) { return areEq; } -function setHasSimilarElement(set, val1, bEntriesUsed, strict, memo) { +function setHasSimilarElement(set, val1, usedEntries, strict, memo) { if (set.has(val1)) return true; @@ -296,12 +296,12 @@ function setHasSimilarElement(set, val1, bEntriesUsed, strict, memo) { // Otherwise go looking. for (const val2 of set) { - if (bEntriesUsed && bEntriesUsed.has(val2)) + if (usedEntries && usedEntries.has(val2)) continue; if (_deepEqual(val1, val2, strict, memo)) { - if (bEntriesUsed) - bEntriesUsed.add(val2); + if (usedEntries) + usedEntries.add(val2); return true; } } @@ -323,7 +323,7 @@ function setEquiv(a, b, strict, memo) { // 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; + let usedEntries = null; for (const val1 of a) { // If the value doesn't exist in the second set by reference, and its an @@ -331,10 +331,10 @@ function setEquiv(a, b, strict, memo) { // 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 (bEntriesUsed == null && typeof val1 === 'object') - bEntriesUsed = new Set(); + if (usedEntries == null && typeof val1 === 'object') + usedEntries = new Set(); - if (!setHasSimilarElement(b, val1, bEntriesUsed, strict, memo)) { + if (!setHasSimilarElement(b, val1, usedEntries, strict, memo)) { return false; } } @@ -342,7 +342,7 @@ function setEquiv(a, b, strict, memo) { return true; } -function mapHasSimilarEntry(map, key1, item1, bEntriesUsed, 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: @@ -363,13 +363,13 @@ function mapHasSimilarEntry(map, key1, item1, bEntriesUsed, strict, memo) { if (key2 === key1) continue; - if (bEntriesUsed && bEntriesUsed.has(key2)) + if (usedEntries && usedEntries.has(key2)) continue; if (_deepEqual(key1, key2, strict, memo) && _deepEqual(item1, item2, strict, memo)) { - if (bEntriesUsed) - bEntriesUsed.add(key2); + if (usedEntries) + usedEntries.add(key2); return true; } } @@ -385,15 +385,15 @@ function mapEquiv(a, b, strict, memo) { if (a.size !== b.size) return false; - let bEntriesUsed = null; + let usedEntries = null; for (const [key1, item1] of a) { - if (bEntriesUsed == null && typeof key1 === 'object') - bEntriesUsed = new Set(); + if (usedEntries == null && 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, bEntriesUsed, strict, memo)) + if (!mapHasSimilarEntry(b, key1, item1, usedEntries, strict, memo)) return false; } diff --git a/test/parallel/test-assert-deep.js b/test/parallel/test-assert-deep.js index 162499b224fd21..78390b15ad785b 100644 --- a/test/parallel/test-assert-deep.js +++ b/test/parallel/test-assert-deep.js @@ -187,7 +187,7 @@ 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 +// Ref: https://github.com/nodejs/node/issues/13347 assertNotDeepOrStrict( new Set([{a: 1}, {a: 1}]), new Set([{a: 1}, {a: 2}]) From 957d61088b3ed7671920ed7f1bf59d35172da9c3 Mon Sep 17 00:00:00 2001 From: Joseph Gentle Date: Sun, 4 Jun 2017 00:08:06 +1000 Subject: [PATCH 3/4] assert: Added and fixed more failing test cases comparing maps and sets --- lib/assert.js | 30 ++++++++++++++++++++---------- test/parallel/test-assert-deep.js | 8 ++++++++ 2 files changed, 28 insertions(+), 10 deletions(-) diff --git a/lib/assert.js b/lib/assert.js index 66b2d18815386f..bd36c9a16d1b38 100644 --- a/lib/assert.js +++ b/lib/assert.js @@ -286,8 +286,11 @@ function _deepEqual(actual, expected, strict, memos) { } function setHasSimilarElement(set, val1, usedEntries, strict, memo) { - if (set.has(val1)) + 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). @@ -321,22 +324,26 @@ function setEquiv(a, b, strict, memo) { 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. + // 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 (usedEntries == null && typeof val1 === 'object') - usedEntries = new Set(); - - if (!setHasSimilarElement(b, val1, usedEntries, strict, memo)) { + if (!setHasSimilarElement(b, val1, usedEntries, strict, memo)) return false; - } } return true; @@ -352,8 +359,11 @@ function mapHasSimilarEntry(map, key1, item1, usedEntries, 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; @@ -388,7 +398,7 @@ function mapEquiv(a, b, strict, memo) { let usedEntries = null; for (const [key1, item1] of a) { - if (usedEntries == null && typeof key1 === 'object') + if (usedEntries == null && (!strict || typeof key1 === 'object')) usedEntries = new Set(); // Just like setEquiv above, this hunt makes this function O(n^2) when diff --git a/test/parallel/test-assert-deep.js b/test/parallel/test-assert-deep.js index 78390b15ad785b..8afad527435224 100644 --- a/test/parallel/test-assert-deep.js +++ b/test/parallel/test-assert-deep.js @@ -201,6 +201,14 @@ assertNotDeepOrStrict( 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']]), From 4b97dee6d5ed61f4929e7a19066886747b432240 Mon Sep 17 00:00:00 2001 From: Joseph Gentle Date: Sun, 4 Jun 2017 08:15:56 +1000 Subject: [PATCH 4/4] assert: Fixed linter errors --- test/parallel/test-assert-deep.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-assert-deep.js b/test/parallel/test-assert-deep.js index 8afad527435224..fecb5c89dbe3e7 100644 --- a/test/parallel/test-assert-deep.js +++ b/test/parallel/test-assert-deep.js @@ -205,8 +205,8 @@ 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}]) + 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: