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
wants to merge 13 commits into
base: master
from

Conversation

@josephg
Contributor

josephg commented Mar 31, 2017

assert.deepEqual and assert.deepStrictEqual currently return true for
any pair of Maps and Sets regardless of content. This patch adds
support in deepEqual and deepStrictEqual to verify the contents of Maps
and Sets.

Unfortunately because there's no way to pairwise fetch set values or map
values which are equivalent but not reference-equal, this change
currently only supports reference equality checking in set values and
map keys. Equivalence checking could be done, but it would be an
O(n^2) operation, and worse, it would get slower exponentially if maps
and sets were nested.

Update: Based on conversation below, the implementation now incurs an O(n^2) cost if the sets or maps are not equal, because to be correct we need to check all pairs of values in sets, and all pairs of keys in a map. This may become exponentially more expensive with input which does deep equality checking on nested maps. But given these checks are almost always only done during testing, and on small objects its probably the right approach.

Update 2: Based on a clever suggestion by @TimothyGu the O(n^2) cost is now only paid when you're either in non-strict mode or when you're using set values & map keys which have typeof object.

Note that this change breaks compatibility with previous versions of
deepEqual and deepStrictEqual if consumers were depending on all maps
and sets to be seen as equivalent. The old behaviour was never
documented, but nevertheless there are certainly some tests out there somewhere
which depend on it.

Support had been stalled because the assert API was frozen, but assert was recently
unfrozen in CTC#63

Fixes: #2309
Refs: substack/tape#342
Refs: #2315
Refs: nodejs/CTC#63

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)
  • Assert
assert: Add support for Map and Set in deepEqual
assert.deepEqual and assert.deepStrictEqual currently return true for
any pair of Maps and Sets regardless of content. This patch adds
support in deepEqual and deepStrictEqual to verify the contents of Maps
and Sets.

Unfortunately because there's no way to pairwise fetch set values or map
values which are equivalent but not reference-equal, this change
currently only supports reference equality checking in set values and
map key values. Equivalence checking could be done, but it would be an
O(n^2) operation, and worse, it would get slower exponentially if maps
and sets were nested.

Note that this change breaks compatibility with previous versions of
deepEqual and deepStrictEqual if consumers were depending on all maps
and sets to be seen as equivalent. The old behaviour was never
documented, but nevertheless there are certainly some tests out there
which depend on it.

Support has stalled because the assert API was frozen, but was recently
unfrozen in CTC#63

Fixes: #2309
Refs: substack/tape#342
Refs: #2315
Refs: nodejs/CTC#63
@Trott

This comment has been minimized.

Show comment
Hide comment
Member

Trott commented Mar 31, 2017

Show outdated Hide outdated lib/assert.js
Show outdated Hide outdated lib/assert.js

@addaleax addaleax requested review from evanlucas, Trott and joyeecheung Mar 31, 2017

assert: Use isSet and isMap from process.binding
Updated PR based on comments

Ref: #12142
@Trott

Trott approved these changes Mar 31, 2017 edited

LGTM if CI comes back green and with or without addression @addaleax's comments (but preferably with!)

assert: Added deeper equality checking for Map,Set
This change updates the checks for deep equality checking on Map and Set
to check all set values / all map keys to see if any of them match the
expected result.

This change is much slower, but based on the conversation in the pull
request its probably the right approach.

Ref: #12142
@mcollina

LGTM

@josephg

This comment has been minimized.

Show comment
Hide comment
@josephg

josephg Mar 31, 2017

Contributor

I've updated the PR to do the quadratic pairwise checking based on the conversation above.

Contributor

josephg commented Mar 31, 2017

I've updated the PR to do the quadratic pairwise checking based on the conversation above.

@addaleax

Awesome PR. 👍

Show outdated Hide outdated lib/assert.js
Show outdated Hide outdated lib/assert.js
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Mar 31, 2017

Member

@nodejs/ctc semver-major or semver-minor or semver-minor but all dont-land labels?

Member

addaleax commented Mar 31, 2017

@nodejs/ctc semver-major or semver-minor or semver-minor but all dont-land labels?

Show outdated Hide outdated lib/assert.js
@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Mar 31, 2017

Member

I think it's semver-major. This can and probably will break something.

Member

targos commented Mar 31, 2017

I think it's semver-major. This can and probably will break something.

@vsemozhetbyt

This comment has been minimized.

Show comment
Hide comment
@vsemozhetbyt

vsemozhetbyt Mar 31, 2017

Member

@josephg A nit: traditionally, commit title should use lower case imperative verb after subsystem prefix. Buth this can be fixed by anybody who will land the commits.

Member

vsemozhetbyt commented Mar 31, 2017

@josephg A nit: traditionally, commit title should use lower case imperative verb after subsystem prefix. Buth this can be fixed by anybody who will land the commits.

Show outdated Hide outdated lib/assert.js
Show outdated Hide outdated lib/assert.js
Show outdated Hide outdated lib/assert.js
Show outdated Hide outdated lib/assert.js
const m2 = new Map();
m1.x = 5;
assertNotDeepOrStrict(m1, m2);
}

This comment has been minimized.

@evanlucas

evanlucas Mar 31, 2017

Member

Maybe add a case where there is a circular reference?

@evanlucas

evanlucas Mar 31, 2017

Member

Maybe add a case where there is a circular reference?

This comment has been minimized.

@josephg

josephg Mar 31, 2017

Contributor

Done.

@josephg

josephg Mar 31, 2017

Contributor

Done.

assert: var -> const and added tests
Cleaned up as per comments in issue

Ref: #6416
Show outdated Hide outdated lib/assert.js
Show outdated Hide outdated lib/assert.js
assert: restrict O(n^2) search
Based on comments in the PR, this change restricts an O(n^2) to only
happen when your set contains object-like objects, your map contains
object-like keys or you're not in strict mode.

ref: #12142 (review)
Show outdated Hide outdated lib/assert.js

@addaleax addaleax added semver-major and removed semver-minor labels Mar 31, 2017

@addaleax addaleax added this to the 8.0.0 milestone Mar 31, 2017

@TimothyGu

This comment has been minimized.

Show comment
Hide comment
@TimothyGu

TimothyGu Apr 1, 2017

Member

Not sure how important this is to the discussion. There are also some subtle differences in SameValueZero (the set of comparison semantics .has() uses), abstract equality (==, the operator assert.equal uses), and strict equality (===, the operator assert.strictEqual uses). Concretely, this means code such as the following may yield surprising results:

assert.equal(NaN, NaN);
    // AssertionError: NaN === NaN
assert.strictEqual(NaN, NaN);
    // AssertionError: NaN === NaN

const map1 = new Map([[NaN, 0]]);
const map2 = new Map([[NaN, 0]]);
assert.deepEqual(map1, map2);
     // No error
assert.strictDeepEqual(map1, map2);
     // No error
Member

TimothyGu commented Apr 1, 2017

Not sure how important this is to the discussion. There are also some subtle differences in SameValueZero (the set of comparison semantics .has() uses), abstract equality (==, the operator assert.equal uses), and strict equality (===, the operator assert.strictEqual uses). Concretely, this means code such as the following may yield surprising results:

assert.equal(NaN, NaN);
    // AssertionError: NaN === NaN
assert.strictEqual(NaN, NaN);
    // AssertionError: NaN === NaN

const map1 = new Map([[NaN, 0]]);
const map2 = new Map([[NaN, 0]]);
assert.deepEqual(map1, map2);
     // No error
assert.strictDeepEqual(map1, map2);
     // No error
@josephg

This comment has been minimized.

Show comment
Hide comment
@josephg

josephg Apr 1, 2017

Contributor

@joyeecheung I've cleaned up the documentation. Left the little inline YAML changelog alone for now.

Contributor

josephg commented Apr 1, 2017

@joyeecheung I've cleaned up the documentation. Left the little inline YAML changelog alone for now.

Show outdated Hide outdated lib/assert.js
@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Apr 1, 2017

Member

@josephg I believe the format for a not-yet-released changelog is...

changes:
  - version: REPLACEME
    pr-url: https://github.com/nodejs/node/pull/12142
    description: ....
Member

joyeecheung commented Apr 1, 2017

@josephg I believe the format for a not-yet-released changelog is...

changes:
  - version: REPLACEME
    pr-url: https://github.com/nodejs/node/pull/12142
    description: ....
assert: Updated based on PR
- Added changes: entries in assert API documentation
- Refactored setEquiv based on @joyeecheung's comments

ref: #12142
@joyeecheung

minus some nits, I think this can go ahead now...thanks for bearing with me and for the contribution, your hard work is defnitely appreciated :D

@josephg

This comment has been minimized.

Show comment
Hide comment
@josephg

josephg Apr 1, 2017

Contributor

Cheers :)

Contributor

josephg commented Apr 1, 2017

Cheers :)

assert: refactored based on PR
Refactored setEquiv and mapEquiv based on @joyeecheung's stylistic
preference to avoid labels.
@josephg

This comment has been minimized.

Show comment
Hide comment
@josephg

josephg Apr 1, 2017

Contributor

@joyeecheung I've rewritten mapEquiv and setEquiv based on our conversation above. The tight if() statement logic we were talking about turned out to be completely unreadable. As proof that it was a bad idea neither of us was able to produce working code of that form. Even my final attempt had a small mistake and I completely confused myself trying to fix it, so I abandoned that style completely. The current code passed all tests on its first run.

I think its all very readable now - although using ancillary functions makes it more wordy than it was using loop labels. I'm not sure the code is any better for it. But, we're enough opinions deep that I'm getting exhausted from all this churn on such a minor patch. If anyone wants to make more purely stylistic changes I'm happy to add you as a contributor to josephg/node. You're welcome to modify the PR directly.

Otherwise I don't know the process, but I'm happy for it to be merged.

Contributor

josephg commented Apr 1, 2017

@joyeecheung I've rewritten mapEquiv and setEquiv based on our conversation above. The tight if() statement logic we were talking about turned out to be completely unreadable. As proof that it was a bad idea neither of us was able to produce working code of that form. Even my final attempt had a small mistake and I completely confused myself trying to fix it, so I abandoned that style completely. The current code passed all tests on its first run.

I think its all very readable now - although using ancillary functions makes it more wordy than it was using loop labels. I'm not sure the code is any better for it. But, we're enough opinions deep that I'm getting exhausted from all this churn on such a minor patch. If anyone wants to make more purely stylistic changes I'm happy to add you as a contributor to josephg/node. You're welcome to modify the PR directly.

Otherwise I don't know the process, but I'm happy for it to be merged.

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Apr 1, 2017

Member

@josephg Sorry if my reviews are disheartening and thanks for sticking with this PR!

CI: https://ci.nodejs.org/job/node-test-pull-request/7153/
CITGM since it's semver-major: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/683/

Member

joyeecheung commented Apr 1, 2017

@josephg Sorry if my reviews are disheartening and thanks for sticking with this PR!

CI: https://ci.nodejs.org/job/node-test-pull-request/7153/
CITGM since it's semver-major: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/683/

Oudated reviews..

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Apr 3, 2017

Member

Landed in 6481c93, and again thanks for the PR!

Member

addaleax commented Apr 3, 2017

Landed in 6481c93, and again thanks for the PR!

@addaleax addaleax closed this Apr 3, 2017

addaleax added a commit that referenced this pull request Apr 3, 2017

assert: add support for Map and Set in deepEqual
assert.deepEqual and assert.deepStrictEqual currently return true for
any pair of Maps and Sets regardless of content. This patch adds
support in deepEqual and deepStrictEqual to verify the contents of Maps
and Sets.

Deeo equivalence checking is currently an
O(n^2) operation, and worse, it gets slower exponentially if maps
and sets were nested.

Note that this change breaks compatibility with previous versions of
deepEqual and deepStrictEqual if consumers were depending on all maps
and sets to be seen as equivalent. The old behaviour was never
documented, but nevertheless there are certainly some tests out there
which depend on it.

Support has stalled because the assert API was frozen, but was recently
unfrozen in CTC#63.

---

Later squashed in:

This change updates the checks for deep equality checking on Map and Set
to check all set values / all map keys to see if any of them match the
expected result.

This change is much slower, but based on the conversation in the pull
request its probably the right approach.

Fixes: #2309
Refs: substack/tape#342
Refs: #2315
Refs: nodejs/CTC#63
PR-URL: #12142
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@josephg

This comment has been minimized.

Show comment
Hide comment
@josephg

josephg Apr 4, 2017

Contributor

Yay! Thanks for helping me get my first PR in node everyone!

Contributor

josephg commented Apr 4, 2017

Yay! Thanks for helping me get my first PR in node everyone!

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

@Trott Trott referenced this pull request Apr 11, 2017

Closed

test: complete coverage for lib/assert.js #12239

3 of 3 tasks complete

@TimothyGu TimothyGu referenced this pull request May 5, 2017

Closed

assert: improve deepEqual perf for large input #12849

0 of 4 tasks complete

@josephg josephg deleted the josephg:assert-sets-and-maps branch Jun 2, 2017

josephg added a commit to josephg/node that referenced this pull request Jun 3, 2017

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: nodejs#13347
Refs: nodejs#12142

@josephg josephg referenced this pull request Jun 3, 2017

Closed

assert: fix deepEqual similar sets and maps bug #13426

3 of 3 tasks complete

refack added a commit to refack/node that referenced this pull request Jun 5, 2017

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.

PR-URL: nodejs#13426
Fixes: nodejs#13347
Refs: nodejs#12142
Reviewed-By: Refael Ackermann <refack@gmail.com>

jasnell added a commit that referenced this pull request Jun 7, 2017

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.

PR-URL: #13426
Fixes: #13347
Refs: #12142
Reviewed-By: Refael Ackermann <refack@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment