Skip to content

assert: Add support for Map and Set in deepEqual #12142

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

Closed
josephg wants to merge 13 commits into from
Closed

assert: Add support for Map and Set in deepEqual #12142

Changes from 1 commit
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 Mar 31, 2017
f051840
assert: Use isSet and isMap from process.binding
josephg Mar 31, 2017
1d6cda6
assert: Added deeper equality checking for Map,Set
josephg Mar 31, 2017
800ae46
assert: Fixed nit in code review
josephg Mar 31, 2017
031f6f3
assert: Fixed typo in comment
josephg Mar 31, 2017
d6baaee
assert: var -> const and added tests
josephg Mar 31, 2017
8fb6ebf
assert: restrict O(n^2) search
josephg Mar 31, 2017
acef701
assert: added null check in set/map short circuit
josephg Apr 1, 2017
ee131e8
assert: update documentation to reflect changes
josephg Apr 1, 2017
7bc29b0
assert: Updated based on PR
josephg Apr 1, 2017
6bdfcaf
assert: updated docs based on PR
josephg Apr 1, 2017
fc5196a
assert: fixed docs # link
josephg Apr 1, 2017
7f9d4d8
assert: refactored based on PR
josephg Apr 1, 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
43 changes: 24 additions & 19 deletions lib/assert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,13 +300,15 @@ function setEquiv(a, b, strict, actualVisitedObjects) {

outer: for (const val1 of a) {
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.
for (const val2 of b) {
if (_deepEqual(val1, val2, strict, actualVisitedObjects)) {
continue outer;
if (!strict || typeof val1 === 'object') {
Copy link
Member

@addaleax addaleax Mar 31, 2017

Choose a reason for hiding this comment

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

(!strict || (typeof val1 === 'object' && val1 !== null) || typeof val1 === 'function')?

Copy link
Contributor Author

@josephg josephg Mar 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

We don't need to search for functions because the only way deepEqual will consider functions equivalent is through reference equality. The null check makes some sense, although I think it'll make a very small difference in practice because putting null in a set and using null as a key are rare. (In my code, null popping up in either of those places is usually an indication of a bug)

Copy link
Contributor Author

@josephg josephg Apr 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Its a minor point, but do you think its still worth putting in the null check? I'm indifferent either way, but happy to add it if you think the marginal speed improvement is worth the marginal extra complexity of that check.

Copy link
Member

@addaleax addaleax Apr 1, 2017

Choose a reason for hiding this comment

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

@josephg Yes, I’d say it’s worth it… there might be cases where the difference is not so marginal, and I think it’s okay for Node core to sacrifice a tiny bit of readability for a small performance gain.

Copy link
Contributor Author

@josephg josephg Apr 1, 2017

Choose a reason for hiding this comment

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

Done

// 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.
for (const val2 of b) {
Copy link
Member

@joyeecheung joyeecheung Apr 1, 2017

Choose a reason for hiding this comment

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

If I am reading the code correctly, is it similar to...

for (const val1 of a) {
  if (!b.has(val1)) {
    if (strict) { return false; }
    if (typeof val1 !== 'object' || val1 === null)) { return false; }

   // the following can be wrapped into a funciton...
    var bHasSimilarElement = false;
    for (const val2 of b) {
      if (_deepEqual(val1, val2, strict, actualVisitedObjects)) {
        bHasSimilarElement = true;
        break;
      }
    }

    if (!bHasSimilarElement) {
      return false;
    }
  }
}

?

Personally I prefer to stay away from labels...

Copy link
Member

@joyeecheung joyeecheung Apr 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Also on #12142 (comment), I think the primitive check can be replaced with util.isPrimitive(val) so it is more inline with objEquiv and probably more future proof

Copy link
Contributor Author

@josephg josephg Apr 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

That transformation is close... Your version is equivalent to if (strict || ...) whereas it should be if (strict && ....)

But yes, I could write the function that way. I'm usually a little happier with early returns for stuff like that, but I know some people don't like them and I'm not sure where nodejs stands on it.

As for ditching the loop labels, I'm not convinced. Your version is twice as long. If I pull that code out into a single-use function then it'd lose some locality. And we'd have to have another similar-but-different copy of that function for mapEquiv as well. (I'd love to write something we can reuse, but it would need to yield back all of the matches for mapEquiv, which means taking a callback, returning an iterator or being a generator function. I'd rather avoid all of those options for this.

Copy link
Contributor Author

@josephg josephg Apr 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

As for replacing it with stuff in util, I can either do (!isPrimitive(val1) && !isFunction(val1)) or isObject(val1). The latter has exactly the behaviour we want, but the fact that it returns true for arrays is weird and non-obvious to me. Do you have a preference?

Copy link
Member

@joyeecheung joyeecheung Apr 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Oh yes, missed the conjunction..so on a higher-level, this is:

for (const val1 of a) {
  if (!b.has(val1) &&
      (strict && util.isPrimitive(val1) || isFunction(val1)) ||
      !hasDeepEqual(b, val1, strict, actualVisitedObjects)) {
    return false;
  }
}

?
EDIT: the first version is..totally wrong.. :S

Copy link
Member

@joyeecheung joyeecheung Apr 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Also I don't think creating a function that takes callbacks is a good idea either, but that's not what I am proposing here..just think small functions and conjunctions would be easier to read than labels

EDIT: .. doesn't really matter if those small functions can be reused
(mapEquiv can call its own small functions too..). Although function calls incur overhead, but since deepEqual has too many complex logic and we use a pattern like this already, I think it's fine to trade this overhead for readability. As you can see we don't inline setEquiv even though it's not reusable, and we don't make a function taking callbacks to abstract over setEquiv/mapEquiveither. All for readability, eh ;)

Copy link
Contributor Author

@josephg josephg Apr 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yes thats right - except we can also exclude functions from the search. And I think the logic is still wrong. I'm reasonably sure this is correct:

  for (const val1 of a) {
    if (!b.has(val1)
      && ((strict && (util.isObject(val1) || util.isFunction(val1)))
        || setHasSimilarElement(b, val11, strict, actualVisitedObjects)))
      return false;
    }
  }

(And if its not correct I think the tests should catch any mistakes.)

I just deleted a bunch of text justifying not changing the code, but I think I'm just getting exhausted 7 reviewers rubbing away at, now, stylistic changes. But I agree - the code is cleaner the way you've suggested. I'll update the PR...

Copy link
Member

@joyeecheung joyeecheung Apr 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Also also..to be clear, I don't actually feel strong enough about the labels to block this PR, because support for Maps and Sets are definitely appreciated and outweight readability preferences! Although on the other hand, I do feel stronger about the doc update ...even the comments in_deepEqual are updated, surely the documentation deserves some love right ;)
EDIT: oh I saw the doc update now, thanks

Copy link
Contributor Author

@josephg josephg Apr 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

See my comments below - I updated the documentation a couple hours ago.

Also I was still wrong - here's the actually correct logic. At least, the tests pass with this version. I'm pretty sure its right (though I convinced myself the last version was right too).

    if (!b.has(val1)
        && (!strict || (!util.isObject(val1) && !util.isFunction(val1)))
        && !setHasSimilarElement(b, val1, strict, actualVisitedObjects)) {
      return false;
    }

if (_deepEqual(val1, val2, strict, actualVisitedObjects))
continue outer;
}
}

Expand All @@ -333,24 +335,27 @@ function mapEquiv(a, b, strict, actualVisitedObjects) {
// 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).
// This check is not strictly necessary if we run the loop below, but it
// improves 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).
for (const [key2, item2] of b) {
// 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;
// above, this hunt makes this function O(n^2) when using objects and lists
// as keys
if (!strict || typeof key1 === 'object') {
for (const [key2, item2] of b) {
// 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;
}
}
}

Expand Down