assert: fix deepEqual similar sets and maps bug by josephg · Pull Request #13426 · nodejs/node

josephg · 2017-06-03T00:41:15Z

This fixes a bug where deepEqual and deepStrictEqual would have
incorrect behaviour in sets and maps containing multiple equivalent
keys.

Fixes: #13347
Refs: #12142

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

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

refack

LGTM % nits & CI

refack · 2017-06-03T12:16:36Z


 assertDeepAndStrictEqual(new Set([{}]), new Set([{}]));

+// Discussion of these test cases here - https://github.com/nodejs/node/issues/13347


nit: just put Ref: https://github.com/nodejs/node/issues/13347

refack · 2017-06-03T12:19:29Z

+ // 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;


nit: I'd rather you name it something like usedEntries, since bEntriesUsed sounds like a boolean predicating a need to use .entries()

If you choose to accept this nit, rename everywhere else as well...

Sure - I don't have a strong opinion so I've renamed it.
My concern with usedEntries is that it doesn't make it clear that we're marking entries in set b.

refack · 2017-06-03T12:23:10Z

CI: https://ci.nodejs.org/job/node-test-commit/10336/

rmdm · 2017-06-03T12:25:07Z

 // 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')


Why is it important that a current val1 is an object? And why it should prevent initializing bEntriesUsed "for now" if it is not?

The bug doesn't affect value types because its impossible for two strings, numbers, bools, etc to be deepEqual to one another without being reference-equal. So in the case where the set or map only contains value typed entries, there's no need to allocate an extra set for bEntriesUsed.
Lazily allocating the set when needed is a microoptimization.

Then maybe it worth to continue that optimization and checking type of val2 before adding it to set of usedEntries? Because presence of even single object in a set with large number of typed entries would lead to adding them to usedEntries without any need.

Maybe.. but in practice its very rare to mix types in set / map keys. And it still won't matter in strict mode because there's an early return for value types anyway.
Uhhhh but not in non-strict mode. Huh - looks like this also doesn't throw:
assert.deepEqual(newSet([3,'3']),newSet([3,4]));
fixes

... Fixed. PR updated so this behaves correctly too.

refack

💯

refack · 2017-06-03T16:36:01Z

CI 2: https://ci.nodejs.org/job/node-test-commit/10339/

refack · 2017-06-03T16:41:47Z

Lint errors:

not ok 27 - /usr/home/iojs/build/workspace/node-test-linter/test/parallel/test-assert-deep.js
---
message: Missing space before value for key 'a'.
severity: error
data:
line: 208
column: 15
ruleId: key-spacing
messages:
- message: Missing space before value for key 'a'.
severity: error
data:
line: 208
column: 22
ruleId: key-spacing
- message: Missing space before value for key 'a'.
severity: error
data:
line: 208
column: 29
ruleId: key-spacing
- message: Missing space before value for key 'a'.
severity: error
data:
line: 209
column: 15
ruleId: key-spacing
- message: Missing space before value for key 'a'.
severity: error
data:
line: 209
column: 22
ruleId: key-spacing
- message: Missing space before value for key 'a'.
severity: error
data:
line: 209
column: 29
ruleId: key-spacing
...

josephg · 2017-06-03T22:16:46Z

Oops! Sorry about that. Fixed!

$ make lint
Running JS linter...
./node tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules --ext=.js,.md \
benchmark doc lib test tools
Running C++ linter...
Total errors found: 0

refack · 2017-06-04T01:53:53Z

CI: https://ci.nodejs.org/job/node-test-commit/10342/

refack · 2017-06-04T02:49:13Z

Linux CI fail was infrastructure related, but still rerunning: https://ci.nodejs.org/job/node-test-commit-linux/10349/

josephg · 2017-06-05T09:37:06Z

Unless there's any more comments, I think this is ready to merge 👓

refack · 2017-06-05T12:46:26Z

I'll land this later today (let PDT people a last chance to chime in)
/cc @nodejs/testing

This fixes a bug where deepEqual and deepStrictEqual would have incorrect behaviour in sets and maps containing multiple equivalent keys. PR-URL: nodejs#13426Fixes: nodejs#13347 Refs: nodejs#12142 Reviewed-By: Refael Ackermann <refack@gmail.com>

refack · 2017-06-05T22:34:56Z

landed in 7cddcc9

refack · 2017-06-05T22:36:50Z

Extra quick sanity of master: https://ci.nodejs.org/job/node-test-commit-linuxone/6404/

This fixes a bug where deepEqual and deepStrictEqual would have incorrect behaviour in sets and maps containing multiple equivalent keys. PR-URL: #13426Fixes: #13347 Refs: #12142 Reviewed-By: Refael Ackermann <refack@gmail.com>

assert: fix deepEqual similar sets and maps bug
3995b72
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

nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Jun 3, 2017

refack approved these changes Jun 3, 2017
View reviewed changes

rmdm reviewed Jun 3, 2017
View reviewed changes

assert: Fix nits from review by@refack
067be29

refack approved these changes Jun 3, 2017
View reviewed changes

assert: Added and fixed more failing test cases comparing maps and sets
957d610

assert: Fixed linter errors
4b97dee

refack self-assigned this Jun 5, 2017

refack closed this Jun 5, 2017

josephg deleted the deepequal-bug branch June 5, 2017 22:40

gibfahn mentioned this pull request Jun 15, 2017
Auditing for 6.11.1 nodejs/Release#230
Closed
3 tasks

MylesBorins added the dont-land-on-v6.x label Jul 17, 2017

refack removed their assignment Oct 20, 2018


		assertDeepAndStrictEqual(new Set([{}]), new Set([{}]));

		// Discussion of these test cases here - https://github.com/nodejs/node/issues/13347

Uh oh!

Uh oh!

Conversation

josephg commented Jun 3, 2017

Checklist

Affected core subsystem(s)

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Jun 3, 2017

Uh oh!

rmdmJun 3, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Jun 3, 2017

Uh oh!

refack commented Jun 3, 2017

Uh oh!

josephg commented Jun 3, 2017

Uh oh!

refack commented Jun 4, 2017

Uh oh!

refack commented Jun 4, 2017

Uh oh!

josephg commented Jun 5, 2017

Uh oh!

refack commented Jun 5, 2017

Uh oh!

refack commented Jun 5, 2017

Uh oh!

refack commented Jun 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rmdmJun 3, 2017•
edited
Loading