-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
Closed
Closed
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
561561a
assert: Add support for Map and Set in deepEqual
josephg f051840
assert: Use isSet and isMap from process.binding
josephg 1d6cda6
assert: Added deeper equality checking for Map,Set
josephg 800ae46
assert: Fixed nit in code review
josephg 031f6f3
assert: Fixed typo in comment
josephg d6baaee
assert: var -> const and added tests
josephg 8fb6ebf
assert: restrict O(n^2) search
josephg acef701
assert: added null check in set/map short circuit
josephg ee131e8
assert: update documentation to reflect changes
josephg 7bc29b0
assert: Updated based on PR
josephg 6bdfcaf
assert: updated docs based on PR
josephg fc5196a
assert: fixed docs # link
josephg 7f9d4d8
assert: refactored based on PR
josephg File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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,18 @@ function _deepEqual(actual, expected, strict, memos) { | |
} | ||
} | ||
|
||
// For all other Object pairs, including Array objects, | ||
if (isSet(actual)) { | ||
return isSet(expected) && setEquiv(actual, expected); | ||
} else if (isSet(expected)) { | ||
return false; | ||
} | ||
|
||
// 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. | ||
|
@@ -283,6 +291,26 @@ function _deepEqual(actual, expected, strict, memos) { | |
return objEquiv(actual, expected, strict, memos); | ||
} | ||
|
||
function setEquiv(a, b) { | ||
// This behaviour will work for any sets with contents that have | ||
// strict-equality. That is, it will return false if the two sets contain | ||
// equivalent objects that aren't reference-equal. We could support that, but | ||
// it would require scanning each pairwise set of not strict-equal items, | ||
// which is O(n^2), and would get exponentially worse if sets are nested. So | ||
// for now we simply won't support deep equality checking set items or map | ||
// keys. | ||
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 val; | ||
for (val of a) { | ||
if (!b.has(val)) | ||
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 +335,25 @@ function objEquiv(a, b, strict, actualVisitedObjects) { | |
return false; | ||
} | ||
|
||
if (isMap(a)) { | ||
if (!isMap(b)) | ||
return false; | ||
|
||
if (a.size !== b.size) | ||
return false; | ||
|
||
var item; | ||
for ([key, item] of a) { | ||
if (!b.has(key)) | ||
return false; | ||
|
||
if (!_deepEqual(item, b.get(key), 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--) { | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I think I would actually prefer the awful performance over always using strict equality… @nodejs/collaborators thoughts?
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.
In this case, I think I'd value correct over performant, so yeah, I agree with @addaleax.
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.
For tests, I agree. I'm nervous people might be using
assert.deepEqual
inside runtime code, although I just checked thenode_modules
directory of a project I'm working on with ~400 transitive dependancies and onlyjsprim
calls assert.deepEqual outside of a test, and thats a in a very minor way. I'll wait for some more feedback, but unless there's any strong objections I'll change the behaviour to be correct and slow.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.
... Another strong argument in favor of making it correct > fast is that it'll be more compatible with the current implementation. If someone currently has a test that reads
assert.deepStrictEqual(new Set([{x:5}]), new Set([{x:5}]))
that test will currently pass because the sets won't be compared. With the PR in its current state, that test will start to fail.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.
@josephg Maybe just update this PR and see if somebody objects? If we need to go back, the current HEAD is at f051840.
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.
... Done.
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.
#14258 just landed and reduces the complexity to O(n log n). Therefore the performance should not be of much concern anymore.