-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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.
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: Add support for Map and Set in deepEqual #12142
Changes from 5 commits
561561a
f051840
1d6cda6
800ae46
031f6f3
d6baaee
8fb6ebf
acef701
ee131e8
7bc29b0
6bdfcaf
fc5196a
7f9d4d8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
||
|
@@ -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 Maps, strict-equal keys mapping to deep-equal values | ||
// Note: this accounts for both named and indexed properties on Arrays. | ||
|
||
// Use memos to handle cycles. | ||
|
@@ -280,9 +282,87 @@ function _deepEqual(actual, expected, strict, memos) { | |
memos.actual.push(actual); | ||
memos.expected.push(expected); | ||
|
||
|
||
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. Should they be | ||
// considered to be non-strict deep equal to one another? Its a silly case, | ||
// and more evidence that deepStrictEqual should always be preferred over | ||
// deepEqual. The implementation currently returns false, which is a simpler | ||
// and faster implementation. | ||
if (a.size !== b.size) | ||
return false; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 I think |
||
|
||
var val1, val2; | ||
outer: for (val1 of a) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you use |
||
if (!b.has(val1)) { | ||
// 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to optimize this by only starting the additional search if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. Thats really clever, and very obvious now that you've pointed it out. Fixed. |
||
for (val2 of b) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you use |
||
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; | ||
|
||
var key1, key2, item1, item2; | ||
outer: for ([key1, item1] of a) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you use |
||
// 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, but its here to improve | ||
// 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). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto. |
||
for ([key2, item2] of b) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you use |
||
// 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)) | ||
|
@@ -307,6 +387,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--) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,11 +100,149 @@ 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); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe add a case where there is a circular reference? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
|
||
/* eslint-enable */ |
There was a problem hiding this comment.
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?