assert: enforce type check in deepStrictEqual #10282

joyeecheung · 2016-12-15T08:41:44Z

For a write-up on this PR, see #10282 (comment)

The less-breaking part of this PR has been moved to #11128

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

assert, test

Description of change

Add checks for the built-in tags and refactor _deepEqual,
most user-land modules do this for a strict, deep comparison.
nodejs/node-v0.x-archive#7178 already added
this check for argument objects, even in non-strict tests.

Fixes #10258.
Docs updates are postponed due to #7815.

See https://tc39.github.io/ecma262/#sec-object.prototype.tostring
for a partial list of the tags.

addaleax

I’ve labelled this semver-major since it looks like a pretty clear behavioural change to me, even if it seems to make a lot of sense from the first look at it.

Fixes #10258.

Could you add a Fixes: https://github.com/nodejs/node/issues/10258 line at the end of the commit message? :)

addaleax · 2016-12-15T13:10:36Z

lib/assert.js

+ return a.source === b.source &&
+ a.global === b.global &&
+ a.multiline === b.multiline &&
+ a.lastIndex === b.lastIndex &&


So, this is not related to the changes you are making here, but I would have a couple of suggestions:

I guess you could probably make arguments both for including lastIndex here and for leaving it out, and I’m tempted to say “drop it”. I probably wouldn’t expect this behaviour.

Maybe instead of the global, multiline and ignoreCase checks a single check for .flags would be a good idea? There are new flags like u that would currently be ignored.

(If you think these suggestions make sense, it’s probably best to keep that for a separate commit/separate PR.)

Good suggestion, I think I will just submit another commit in this PR.

addaleax · 2016-12-15T13:15:28Z

lib/assert.js

+ if (ArrayBuffer.isView(actual) && ArrayBuffer.isView(expected) &&
+ getTag(actual) === getTag(expected) &&
+ !(actual instanceof Float32Array ||
+ actual instanceof Float64Array)) {


The indentation seems off here?

Also, could you update the instanceof checks here to use getTag, too? (probably also best kept for a separate commit or PR)

Oops, why my make lint doesn't scream at me about this :/? I will update these indentations, thanks. And the tag checks can go into another commit I think.

addaleax · 2016-12-15T13:19:07Z

test/parallel/test-assert-strict-typecheck.js

+
+const date = new Date('2016');
+
+// See #10258


nit: Can I suggest that you update this to use the full URL here?

addaleax · 2016-12-15T13:20:21Z

test/parallel/test-assert-strict-typecheck.js

+const fake = new FakeDate();
+
+/* eslint-disable no-restricted-properties */
+// We are testing deepEqual!


Is the eslint-disable tag to that deepEqual can be used here?

no-restricted-properties is the one I get from make lint, I can't find a specific eslint-disable tag for deepEqual, do you know what that is?

@joyeecheung Nope, and to be clear, I wasn’t requesting you to change anything here, just making sure that I understood why the line is there in the first place

Oh yes, this disable tag is for all the deepEquals below :).

I think it's fine the way you did it, but if you want to get even more granular with the linting options, you can disable the check on just the lines that need it by either using // eslint-disable-line no-restricted-properties or // eslint-disable-next-line no-restricted-properties (depending on if you can fit the comment on the same line or if it needs to go on the line right before the assert.deepEqual()).

Thank you for the tips!

addaleax · 2016-12-15T13:22:49Z

test/parallel/test-assert-strict-typecheck.js

+// For deepStrictEqual we check the runtime type,
+// then reveal the fakeness of the fake date
+assert.throws(() => assert.deepStrictEqual(date, fake),
+ `AssertionError: ${date} deepStrictEqual Date { }`);


The indentation is off here, and there’s a assert.throws pitfall: You can’t specify a string to check the error message against; only a constructor, RegExp or function that checks the argument work here. (You can probably just wrap the expression in new RegExp or something)

(edit: link to docs: https://nodejs.org/api/assert.html#assert_assert_throws_block_error_message)

Sorry for not reading the docs more thoroughly. I'll wrap them up in new RegExp, thanks!

Sorry for not reading the docs more thoroughly.

@joyeecheung If it helps… that mistake is something found a lot in our own tests, at least until very recently ;)

addaleax · 2016-12-15T13:23:46Z

test/parallel/test-assert-strict-typecheck.js

+const date2 = new MyDate('2016');
+
+// deepEqual return true as long as the time are the same,
+// but deepStrictEqual check properties


typos: returns and checks (here and below)

joyeecheung · 2016-12-15T16:33:33Z

@addaleax Thanks for the review, I've updated this PR, PTAL.

addaleax · 2016-12-15T22:01:23Z

Thanks! This looks pretty good so far, I’ll have time for a full review of this later.

CI: https://ci.nodejs.org/job/node-test-commit/6668/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/481/

Trott

Thanks for doing this! Mostly seems good to me. Not sure if we'd have to "unlock" the API, but (as of right now, at least) I'd be in favor of doing that. Some small comments.

Trott · 2016-12-16T04:58:51Z

lib/assert.js

- // equivalent if it is also a Date object that refers to the same time.
- } else if (util.isDate(actual) && util.isDate(expected)) {
- return actual.getTime() === expected.getTime();
+ // 7.2. If both values are Date objects,


These numbers (7.2) correspond to the sections of a specification that assert followed in its initial implementation. http://wiki.commonjs.org/wiki/Unit_Testing/1.0

If we're going to change the algorithm, I'd recommend removing the numbers, since we're not following the spec anymore.

Didn't know this is a thing :O I will remove those numbers. Thanks.

Trott · 2016-12-16T05:04:41Z

test/parallel/test-assert.js

- const re1 = /a/;
- re1.lastIndex = 3;
- assert.throws(makeBlock(a.deepEqual, re1, /a/));
-}


Instead of removing this test, would it make sense to change it to assert.doesNotThrow?

Good suggestion. I will update these. Thanks.

Trott · 2016-12-16T05:04:59Z

test/parallel/test-assert.js

-{
- const re1 = /a/;
- re1.lastIndex = 3;
- assert.throws(makeBlock(a.deepStrictEqual, re1, /a/));


Same here: assert.doesNotThrow()?

joyeecheung · 2016-12-16T15:35:02Z

@Trott Thank you for the review, I have updated this PR, PTAL.

Trott · 2016-12-16T21:38:47Z

@nodejs/ctc Would we need to "unlock" the assert module to land this? I think the answer is: "technically yes" but there might be some wiggle room on something like this?

Trott · 2016-12-16T21:39:13Z

Added the blocked label until the "Locked API" issue is sorted out.

Trott

LGTM if CI is ✅ and CITGM is ✨

addaleax

LGTM. Maybe read me adding the “semver-major” label more as a precaution than anything else; I am fine considering this a bugfix.

Extend the assert-throws-arguments custom ESLint rule to also check for the use of template literals as a second argument to assert.throws. PR-URL: nodejs#10301 Ref: nodejs#10282 (comment) Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Italo A. Casas <me@italoacasas.com>

Extend the assert-throws-arguments custom ESLint rule to also check for the use of template literals as a second argument to assert.throws. PR-URL: #10301 Ref: #10282 (comment) Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Italo A. Casas <me@italoacasas.com>

TimothyGu · 2017-02-02T23:30:02Z

lib/assert.js

- return true;
- }
+ // Notes: Type tags are [[Class]] properties returned by
+ // obj->class_name() in C++ or Object.prototype.toString.call(obj) in JS


[[Class]] is gone in ES6, and obj->class_name() doesn't seem to be part of the public V8 API. I don't think we really need to mention them.

I mentioned obj->class_name() because that looks like it's what util.isDate() (Value::IsDate) and alike come down to in a macro. Also it's why process and a lot of other objects created in the C++ land has their own tags, although after a quick search looks like they actually use FunctionTemplate::SetClassName() to set this, and never use the getter, so probably we can just mention FunctionTemplate::SetClassName() would alter this.

[[Class]] is described as "historical" in another PR, just as it is in the spec note.

ljharb

I've left a few comments, but in general, it'd be awesome if new code in node core was robust against user modification of the language builtins.

ljharb · 2017-02-06T07:43:01Z

lib/assert.js

+}
+
+function isArguments(tag) {
+ return tag == '[object Arguments]';


why is this == and a few lines above, ===? I assume this should be ===?

Oops, it's mistake, thanks for catching that.

ljharb · 2017-02-06T07:43:34Z

lib/assert.js

+ // See https://tc39.github.io/ecma262/#sec-object.prototype.tostring
+ // for a list of tags pre-defined in the spec.
+ // There are some unspecified tags in the wild too(e.g. typed array tags).
+ const actualTag = Object.prototype.toString.call(actual);


For the sake of robustness, please cache Object.prototype.toString at module level, and don't rely on nobody having monkeyed with Object.prototype at runtime :-)

ljharb · 2017-02-06T07:44:30Z

lib/assert.js

+ // If both values are Date objects,
+ // check if the time underneath are equal first.
+ if (util.isDate(actual) && util.isDate(expected)) {
+ if (actual.getTime() !== expected.getTime()) {


It's safer to cache Date.prototype.getTime at module level, and .call it here - rather than relying both on .getTime not being shadowed on these two objects, and on Date.prototype being unbroken.

ljharb · 2017-02-06T07:44:57Z

lib/assert.js

+ // Here compare returns true, so actual.length === expected.length
+ // if they both only contain numeric keys,
+ // we don't need to exam further
+ if (Object.keys(actual).length === actual.length &&


Similarly, Object.keys should be cached at module level, not runtime-looked-up on Object

joyeecheung · 2017-02-17T16:11:49Z

This is blocked by #11128

Refactors _deepEqual and fixes a few code paths that lead to behaviors contradicting what the doc says. Before this commit certain types of objects (Buffers, Dates, etc.) are not checked properly, and can get away with different prototypes AND different enumerable owned properties because _deepEqual would jump to premature conclusion for them. Since we no longer follow CommonJS unit testing spec, the checks for primitives and object prototypes are moved forward for faster failure. Improve regexp and float* array checks: * Don't compare lastIndex of regexps, because they are not enumerable, so according to the docs they should not be compared * Compare flags of regexps instead of separate properties * Use built-in tags to test for float* arrays instead of using instanceof Use full link to the archived GitHub repository. Use util.objectToString for future improvements to that function that makes sure the call won't be tampered with. Refs: nodejs#10282 (comment) Refs: nodejs#10258 (comment)

Refactors _deepEqual and fixes a few code paths that lead to behaviors contradicting what the doc says. Before this commit certain types of objects (Buffers, Dates, etc.) are not checked properly, and can get away with different prototypes AND different enumerable owned properties because _deepEqual would jump to premature conclusion for them. Since we no longer follow CommonJS unit testing spec, the checks for primitives and object prototypes are moved forward for faster failure. Improve regexp and float* array checks: * Don't compare lastIndex of regexps, because they are not enumerable, so according to the docs they should not be compared * Compare flags of regexps instead of separate properties * Use built-in tags to test for float* arrays instead of using instanceof Use full link to the archived GitHub repository. Use util.objectToString for future improvements to that function that makes sure the call won't be tampered with. PR-URL: #11128 Refs: #10282 (comment) Refs: #10258 (comment) Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

ChALkeR · 2017-03-06T16:21:42Z

Added the blocked label until the "Locked API" issue is sorted out.

assert was unlocked in #11304, removing blocked label.

This is blocked by #11128

That one landed too.

joyeecheung · 2017-03-07T01:40:39Z

@ChALkeR Yeah this is not blocked anymore. Thanks!

Did a rebase locally, haven't decided about #10282 (comment), I'll push it later.

joyeecheung · 2017-03-11T14:53:07Z

Rebased with documentation and a few more test cases (for faked global and process)

Add checks for the built-in type tags to catch objects with faked prototypes. See https://tc39.github.io/ecma262/#sec-object.prototype.tostring for a partial list of built-in tags. Fixes: nodejs#10258

addaleax

Still LGTM :)

addaleax · 2017-03-11T15:11:10Z

doc/api/assert.md


 1. Primitive values are compared using the [Strict Equality Comparison][]
 ( `===` ).
 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.


I can’t find the definition of [Object.prototype.toString()] … am I missing something?

Uh, forgot to put the reference link below :P

addaleax · 2017-03-11T15:11:22Z

doc/api/assert.md

@@ -141,6 +142,25 @@ assert.deepEqual({a: 1}, {a: '1'});
 assert.deepStrictEqual({a: 1}, {a: '1'});
 // AssertionError: { a: 1 } deepStrictEqual { a: '1' }
 // because 1 !== '1' using strict equality
+
+// The following objects don't have owned properties


typo: own properties?

joyeecheung · 2017-03-12T05:16:27Z

@addaleax Thanks for the review, fixed the typo and the missing link.

CI: https://ci.nodejs.org/job/node-test-pull-request/6804/

addaleax · 2017-03-12T12:55:15Z

The last CITGM run is also a bit old, so:

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/641/

addaleax · 2017-03-12T16:11:48Z

Went ahead and landed this in efec14a :)

Add checks for the built-in type tags to catch objects with faked prototypes. See https://tc39.github.io/ecma262/#sec-object.prototype.tostring for a partial list of built-in tags. Fixes: #10258 PR-URL: #10282 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

Add checks for the built-in type tags to catch objects with faked prototypes. See https://tc39.github.io/ecma262/#sec-object.prototype.tostring for a partial list of built-in tags. Fixes: nodejs#10258 PR-URL: nodejs#10282 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added assert Issues and PRs related to the assert subsystem. lts-watch-v6.x labels Dec 15, 2016

joyeecheung force-pushed the fix-assert branch from ecdd024 to d065c7c Compare December 15, 2016 09:00

nodejs-github-bot added the lts-watch-v6.x label Dec 15, 2016

addaleax added semver-major PRs that contain breaking changes and should be released in the next major version. and removed lts-watch-v6.x labels Dec 15, 2016

addaleax reviewed Dec 15, 2016

View reviewed changes

joyeecheung force-pushed the fix-assert branch from d065c7c to 89cdd8f Compare December 15, 2016 16:29

nodejs-github-bot added the lts-watch-v6.x label Dec 15, 2016

joyeecheung force-pushed the fix-assert branch from 89cdd8f to df77ce1 Compare December 15, 2016 16:32

nodejs-github-bot added the lts-watch-v6.x label Dec 15, 2016

joyeecheung force-pushed the fix-assert branch from df77ce1 to 5cdfc0e Compare December 15, 2016 16:34

nodejs-github-bot added the lts-watch-v6.x label Dec 15, 2016

addaleax removed the lts-watch-v6.x label Dec 15, 2016

addaleax requested a review from Trott December 15, 2016 22:01

Trott requested changes Dec 16, 2016

View reviewed changes

targos mentioned this pull request Dec 16, 2016

tools: forbid template literals in assert.throws #10301

Merged

4 tasks

nodejs-github-bot added the lts-watch-v6.x label Dec 16, 2016

Trott approved these changes Dec 16, 2016

View reviewed changes

Trott added the blocked PRs that are blocked by other issues or PRs. label Dec 16, 2016

Trott approved these changes Dec 16, 2016

View reviewed changes

addaleax approved these changes Dec 17, 2016

View reviewed changes

TimothyGu reviewed Feb 2, 2017

View reviewed changes

ljharb suggested changes Feb 6, 2017

View reviewed changes

ChALkeR mentioned this pull request Feb 8, 2017

Changing Definition of "Locked" for Stability Index in API Docs #11200

Closed

joyeecheung added the blocked PRs that are blocked by other issues or PRs. label Feb 17, 2017

joyeecheung mentioned this pull request Feb 18, 2017

util: eliminate unecessary internal/util export #11455

Closed

2 tasks

ChALkeR removed the blocked PRs that are blocked by other issues or PRs. label Mar 6, 2017

joyeecheung force-pushed the fix-assert branch from d27e611 to 855ac04 Compare March 11, 2017 13:49

joyeecheung mentioned this pull request Mar 11, 2017

assert.deepStrictEqual() fails to compare ES6 style object properties #11803

Closed

joyeecheung force-pushed the fix-assert branch 2 times, most recently from 2dabb67 to 3efc514 Compare March 11, 2017 14:51

assert: enforce type check in deepStrictEqual

3efc514

Add checks for the built-in type tags to catch objects with faked prototypes. See https://tc39.github.io/ecma262/#sec-object.prototype.tostring for a partial list of built-in tags. Fixes: nodejs#10258

addaleax approved these changes Mar 11, 2017

View reviewed changes

add missing link, fix typo

b8ed11b

addaleax closed this Mar 12, 2017

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

assert: enforce type check in deepStrictEqual #10282

assert: enforce type check in deepStrictEqual #10282

Conversation

joyeecheung commented Dec 15, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

addaleax left a comment

Choose a reason for hiding this comment

addaleax Dec 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax Dec 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung commented Dec 15, 2016

addaleax commented Dec 15, 2016

Trott left a comment

Choose a reason for hiding this comment

Trott Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung commented Dec 16, 2016

Trott commented Dec 16, 2016

Trott commented Dec 16, 2016

Trott left a comment

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung commented Feb 17, 2017

ChALkeR commented Mar 6, 2017

joyeecheung commented Mar 7, 2017

joyeecheung commented Mar 11, 2017

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung commented Mar 12, 2017

addaleax commented Mar 12, 2017

addaleax commented Mar 12, 2017

joyeecheung commented Dec 15, 2016 •

edited

Loading

addaleax Dec 15, 2016 •

edited

Loading

joyeecheung Dec 16, 2016 •

edited

Loading

addaleax Dec 15, 2016 •

edited

Loading

Trott Dec 16, 2016 •

edited

Loading