Skip to content

assert: use same value equal for deepStrictEqual NaN #15036

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
BridgeAR wants to merge 2 commits into from
Closed

assert: use same value equal for deepStrictEqual NaN #15036

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0bf1af3
doc: add missing changelogs to assert docs
BridgeAR Aug 25, 2017
07e36df
assert: use Same-value eql. for deepStrictEqual NaN
BridgeAR Aug 19, 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
Prev Previous commit
assert: use Same-value eql. for deepStrictEqual NaN
  • Loading branch information
@BridgeAR
BridgeAR committed Sep 1, 2017
commit 07e36df483bac14f847a36c3e804fb2baa2eea61
15 changes: 12 additions & 3 deletions doc/api/assert.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,9 @@ parameter is undefined, a default error message is assigned.
<!-- YAML
added: v1.2.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/15036
description: NaN is now compared using the [SameValueZero][] comparison.
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/15001
description: Error names and messages are now properly compared
Expand All @@ -129,9 +132,10 @@ changes:

Generally identical to `assert.deepEqual()` with three exceptions:

1. Primitive values are compared using the [Strict Equality Comparison][]
( `===` ). Set values and Map keys are compared using the [SameValueZero][]
comparison. (Which means they are free of the [caveats][]).
1. Primitive values besides `NaN` are compared using the [Strict Equality
Comparison][] ( `===` ). Set and Map values, Map keys and `NaN` are compared
using the [SameValueZero][] comparison (which means they are free of the
[caveats][]).
2. [`[[Prototype]]`][prototype-spec] of objects are compared using
the [Strict Equality Comparison][] too.
3. [Type tags][Object.prototype.toString()] of objects should be the same.
Expand Down Expand Up @@ -164,6 +168,8 @@ assert.deepEqual(date, fakeDate);
assert.deepStrictEqual(date, fakeDate);
// AssertionError: 2017-03-11T14:25:31.849Z deepStrictEqual Date {}
// Different type tags
assert.deepStrictEqual(NaN, NaN);
// OK, because of the SameValueZero comparison
```

If the values are not equal, an `AssertionError` is thrown with a `message`
Expand Down Expand Up @@ -412,6 +418,9 @@ parameter is undefined, a default error message is assigned.
<!-- YAML
added: v1.2.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/15036
description: NaN is now compared using the [SameValueZero][] comparison.
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/15001
description: Error names and messages are now properly compared
Expand Down
7 changes: 5 additions & 2 deletions lib/assert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,11 @@ function isObjectOrArrayTag(tag) {
// a) The same built-in type tags
// b) The same prototypes.
function strictDeepEqual(actual, expected) {
if (actual === null || expected === null ||
typeof actual !== 'object' || typeof expected !== 'object') {
if (typeof actual !== 'object') {
return typeof actual === 'number' && Number.isNaN(actual) &&
Copy link
Copy Markdown
Member

@lpinca lpinca Aug 26, 2017

Choose a reason for hiding this comment

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

I think typeof actual === 'number' can be removed, Number.isNaN(actual) is sufficient.

Copy link
Copy Markdown
Member Author

@BridgeAR BridgeAR Aug 26, 2017

Choose a reason for hiding this comment

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

That is correct but it is faster for all other types to check for number first and if it is a number it is not a real penalty because that check is super fast.

Copy link
Copy Markdown
Member

@lpinca lpinca Aug 26, 2017

Choose a reason for hiding this comment

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

Does it makes sense to do the same for expected then?

Copy link
Copy Markdown
Member Author

@BridgeAR BridgeAR Aug 26, 2017

Choose a reason for hiding this comment

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

We could but I feel like that is overdoing it as it would only apply for the case where Number.isNaN(actual), especially as I would expect expected to also be NaN in that case more often then not being of type number.

Number.isNaN(expected);
}
if (typeof expected !== 'object' || actual === null || expected === null) {
return false;
}
const actualTag = objectToString(actual);
Expand Down
8 changes: 8 additions & 0 deletions test/parallel/test-assert-deep.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,7 @@ assertOnlyDeepEqual(
assertDeepAndStrictEqual(m3, m4);
}

// Handle sparse arrays
assertDeepAndStrictEqual([1, , , 3], [1, , , 3]);
assertOnlyDeepEqual([1, , , 3], [1, , , 3, , , ]);

Expand All @@ -481,4 +482,11 @@ assertOnlyDeepEqual([1, , , 3], [1, , , 3, , , ]);
assertOnlyDeepEqual(err1, {}, assert.AssertionError);
}

// Handle NaN
assert.throws(() => { assert.deepEqual(NaN, NaN); }, assert.AssertionError);
Copy link
Copy Markdown
Contributor

@refack refack Aug 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This is not intuitive. I'd assume that
image
(deepStrictEqual should be more strict than deepEqual).
What's the rationale for not allowing assert.deepEqual(NaN, NaN)?

Copy link
Copy Markdown
Member Author

@BridgeAR BridgeAR Aug 26, 2017

Choose a reason for hiding this comment

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

That is the bad part about the wording with strict. The strict stands for more exact in this case. deepEqual is actually more like a looksSimilar and it actually fails at seeing the similarity in this case even though it exists. I can add that check to deepEqual as well but I think it would be weird to have it there (even though I would call deepEqual broken anyway - we actually also use a own lint rule to prevent usage of it!).

Copy link
Copy Markdown
Contributor

@refack refack Aug 26, 2017

Choose a reason for hiding this comment

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

Actually that is IMHO a great idea, create a better named superset assertion. That would also make this change semver-minor.

sameValues might be a better name (that way neither the word "strict" nor "deep" are necessary)

Copy link
Copy Markdown
Member Author

@BridgeAR BridgeAR Aug 26, 2017

Choose a reason for hiding this comment

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

I think this is absolutely not the right thing to do. And as I tried to explain earlier deepStrictEqual is not strictly a superset of deepEqual because it was just a poor naming from the beginning on. Now we have to live with that naming. Most people do for example not know that NaN is not equal to itself when compared with strict equality. And they also do not know about the other existing equality comparisons in JS. They mostly do not even know how abstract equality is defined - at least that is what my experience tells me.
The comparisons in general are well described here. Adding a new API would not help anyone and would instead create more confusion.

Copy link
Copy Markdown
Contributor

@refack refack Aug 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I agree that assert.deepStrictEqual(NaN,NaN) makes sense intuitively.
But ECMAScript abstract equal is "broken" in this sense ((NaN == NaN) === false) and strict equal is broken ((NaN === NaN) === false) is the exact same way.
So IMHO we should be consistent with the standard if we keep using the term equal in our end-point names.

That's why I think breaking away forfrom legacy with a same end-point might be a good idea.

Copy link
Copy Markdown
Member Author

@BridgeAR BridgeAR Aug 26, 2017

Choose a reason for hiding this comment

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

The deepEqual and deepStrictEqual functions have much more then just the "basic" comparison and therefore do not correspond to these properly. I would love if that would be the only distinction because in that case deepEqual would not be so badly broken but that is not the case and it will not change either.
Adding another random name makes the issue not less an issue but just added more problems.

assert.doesNotThrow(() => { assert.deepStrictEqual(NaN, NaN); });
assert.doesNotThrow(() => { assert.deepStrictEqual({ a: NaN }, { a: NaN }); });
assert.doesNotThrow(
() => { assert.deepStrictEqual([ 1, 2, NaN, 4 ], [ 1, 2, NaN, 4 ]); });

/* eslint-enable */