Skip to content
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.deepStrictEqual is fooled by strange dates #10258

Closed
akdor1154 opened this issue Dec 13, 2016 · 20 comments
Closed

assert.deepStrictEqual is fooled by strange dates #10258

akdor1154 opened this issue Dec 13, 2016 · 20 comments
Labels
assert Issues and PRs related to the assert subsystem.

Comments

@akdor1154
Copy link

akdor1154 commented Dec 13, 2016

  • Version: v6.9.1
  • Platform: OS X
  • Subsystem: assert

assert.deepStrictEqual (and strictEqual) is fooled by the following code:

function FakeDate() {};
FakeDate.prototype = Date.prototype;
const f = new FakeDate();
const d = new Date();

f.toString();
//TypeError: Method Date.prototype.toString called on incompatible receiver [object Object]

d.toString();
//'Wed Dec 14 2016 09:53:28 GMT+1100 (AEDT)'

assert.deepStrictEqual(d, f);
// does not throw any error

I think it's reasonable to expect that assert.deepStrictEqual should consider d and f as unequal.

I have not tested on Node master, however there are no relevant changes in assert.js between 6.9.1 and master.

@addaleax addaleax added the assert Issues and PRs related to the assert subsystem. label Dec 13, 2016
@joyeecheung
Copy link
Member

joyeecheung commented Dec 14, 2016

Tested on master, for strictEqual I get

assert.js:85
  throw new assert.AssertionError({
  ^
AssertionError: 2016-12-14T04:59:37.022Z === Date {}
    at Object.<anonymous> (/Users/joyee/projects/node-test/fake-date.js:16:8)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Module.runMain (module.js:605:10)
    at run (bootstrap_node.js:420:7)
    at startup (bootstrap_node.js:139:9)
    at bootstrap_node.js:535:3

But deepStrictEqual does not throw indeed(I think you mean deepEqual & deepStrictEqual?). That's because Dates are not primitive values and don't have their own properties, and here f and d have the same [[Prototype]]. I think this behavior does not contradict what the doc says

assert.deepEqual(actual, expected[, message])

Tests for deep equality between the actual and expected parameters. Primitive values are compared with the equal comparison operator ( == ).
Only enumerable "own" properties are considered. The deepEqual() implementation does not test object prototypes, attached symbols, or non-enumerable properties.

assert.deepStrictEqual(actual, expected[, message])

Generally identical to assert.deepEqual() with two exceptions. First, primitive values are compared using the strict equality operator ( === ). Second, object comparisons include a strict equality check of their prototypes.

But in this case the more reasonable behavior should throw IMHO. I can open a PR adding a special case in assert it that seems reasonable to others.

@Trott
Copy link
Member

Trott commented Dec 14, 2016

I can open a PR adding a special case in assert it that seems reasonable to others.

I'm open to being persuaded on this, but given what we have here so far, I'm not convinced this is a bug.

The two objects have identical properties, including the toString() function. The toString() function is itself sufficiently aware of its context to throw an error on the FakeDate object. But that's a C++-land check. The toString() functions are in fact identical, even though the result of calling it are different on the two objects.

This seems basically the same as this:

const assert = require('assert');

function Foo() {}
function Bar() {}
Bar.prototype = Foo.prototype;

const f = new Foo();
const b = new Bar();

assert.deepStrictEqual(f, b);

If we decide that Date is special in this regard and should be treated differently than other objects, is there an easy way to distinguish between the situation described in this issue and a more prosaic case where two Date objects should be deepStrictEqual? I guess we can run toString() and wrap it in a try/catch, but that seems problematic.

@akdor1154
Copy link
Author

akdor1154 commented Dec 14, 2016

It's not quite the same as your example, as d has actual date information that f lacks (in my example, the Date constructor is never called. If f was initialised with super() then the examples would be more comparable). I guess the issue is that some types have important "comparable" data that is not directly accessible to js. I haven't tested, but I guess that the primitive wrapper objects (String etc) might show similar annoying behaviour.

#valueOf()seems to be a standard way of getting a comparable value out of such objects (i.e. not just Date), might it be worth calling this on comparisons?

@joyeecheung, yep I meant deepEqual, sorry.

@joyeecheung
Copy link
Member

joyeecheung commented Dec 14, 2016

Just did a quick research across some popular assertion libraries that I can think of(omitted power-assert because IIRC they just reuse assert underneath). Turns out everybody throws for this...expect Node core.

'use strict';
const assert = require('assert');
const chaiAssert = require('chai').assert;
const chaiExpect = require('chai').expect;
const expect = require('expect.js');
const should = require('should');
const codeExpect = require('code').expect;
const mustExpect = require('must');
const unexpectedExpect = require('unexpected');

function FakeDate() {};
FakeDate.prototype = Date.prototype;

const d = new Date();
const f = new FakeDate();

// to make sure I use these libs properly
// const d = { a: 1, b: 2 };
// const f = { a: 1, b: 2 };

let tests = {
  'assert': () => assert.deepEqual(d, f),
  'chaiAssert': () => chaiAssert.deepEqual(d, f),
  'should': () => should.deepEqual(d, f),
  'codeExpect': () => codeExpect(d).to.equal(f),
  'chaiExpect': () => chaiExpect(d).to.deep.equal(f),
  'mustExpect': () => mustExpect(d).to.eql(f),
  'unexpectedExpect': () => unexpectedExpect(d, 'to equal', f),
  'expect': () => expect(d).to.eql(f)
}

for (const name in tests) {
  try {
    tests[name]();
    console.log('\n### ' + name + ' does not throw');
  } catch(e) {
    console.log('\n### ' + name + ' throws:');
    console.log(e.message);
  }
}

The results are:

### assert does not throw

### chaiAssert throws:
expected Wed, 14 Dec 2016 05:58:25 GMT to deeply equal {}

### should throws:
expected 2016-12-14 13:58:25.447 +0800 deepEqual Date {}

### codeExpect throws:
this is not a Date object.

### chaiExpect throws:
expected Wed, 14 Dec 2016 05:58:25 GMT to deeply equal {}

### mustExpect throws:
2016-12-14T05:58:25.447Z must be equivalent to {}

### unexpectedExpect throws:

expected new Date('Wed, 14 Dec 2016 05:58:25.447 GMT') to equal Date({})


### expect throws:
this is not a Date object.

@joyeecheung
Copy link
Member

joyeecheung commented Dec 14, 2016

@Trott And for the Foo Bar example nobody throws. So other libraries treat Dates somewhat differently I guess? I think it's more about making sure fake dates are not equal to a real Date, no matter conceptually what the time underneath should be.

@Trott
Copy link
Member

Trott commented Dec 14, 2016

d has actual date information that f lacks

Hmmm... On the one hand: For both objects, it's all tucked away behind getters and setters, and those getters and setters are all identical.

But on the other hand: You certainly have a point that this behavior is kind of surprising and non-intuitive, at least at first blush.

And the fact that six out of six userland assertion libraries throw on this would seem to back that up.

I can go either way on this one right now. A PR might help clarify things. So I'll stop getting in the way. :-D

@joyeecheung
Copy link
Member

OK, I think I will try to read other libraries' code first and figure out why they all choose(or perhaps accidentally choose) to throw :).

@targos
Copy link
Member

targos commented Dec 14, 2016

Another unexpected result:

class MyDate extends Date {
  constructor(...args) {
    super(...args)
    this.myProp = 'value'
  }
}
var date1 = new Date('2016')
console.log(date1)
// 2016-01-01T00:00:00.000Z
var date2 = new MyDate('2016')
console.log(date2)
// { 2016-01-01T00:00:00.000Z myProp: 'value' }

assert.deepStrictEqual(date1, date2) // does not throw

Relevant lines from assert:

node/lib/assert.js

Lines 152 to 153 in 4f97a14

} else if (util.isDate(actual) && util.isDate(expected)) {
return actual.getTime() === expected.getTime();

@joyeecheung
Copy link
Member

Not quite relavent to this issue, this just came up to me while I am digging around the code:

assert.deepEqual(new Set([1, 2]), new Set([1, 2, 3]));  // does not throw
assert.deepEqual(new Map([['a', 1], ['b', 2]]), new Map([['a', 1], ['b', 3]]));  // does not throw

I know assert has a stability index of 3, so it's just a thought :P. Since we are using more and more ES6 in core and V8 have started to optimize iterators, adding checks for iterables might be a nice-to-have in the future.

@Fishrock123
Copy link
Contributor

@joyeecheung That last example is why deepStrictEqual is preferred over deepEqual. :D

@joyeecheung
Copy link
Member

@Fishrock123 Uh, which one do you mean?

@joyeecheung
Copy link
Member

I looked into the source code of other libraries, turns out

  • Some of them use Object.prototype.toString.call(obj) to check if the type of the two objects are the same first. A fake date would have [object Object] but a real date would have [object Date] in our current version of V8. IIRC an older version of v8 would return invalid Date but that was fixed.
  • Some of them just call getTime() on Date-like objects(and throw this is not a Date object.).

And in the user-land modules, deepEqual is usually closer to the semantics of deepStrictEqual in node core.

I think in addition to checking the prototype, we need to check the types returned by Object.prototype.toString.call(obj) too (it's already done for [object Arguments], but not for other built-in types).

@joyeecheung
Copy link
Member

I'm trying to put a PR together but there are always some really bizarre cases that I could break, like these introduced by 00a7456:

assert.doesNotThrow(makeBlock(a.deepEqual, new Number(1), {}),
                    a.AssertionError);
assert.doesNotThrow(makeBlock(a.deepEqual, new Boolean(true), {}),
                    a.AssertionError);

@akdor1154
Copy link
Author

@joyeecheung those are really weird tests, I would assume those should throw.
@teppeis, @bnoordhuis is this really desired behaviour?

@Fishrock123
Copy link
Contributor

@joyeecheung I meant assert.deepEqual(new Set([1, 2]), new Set([1, 2, 3]));


assert.doesNotThrow(makeBlock(a.deepEqual, new Number(1), {}),
                    a.AssertionError);
assert.doesNotThrow(makeBlock(a.deepEqual, new Boolean(true), {}),
                    a.AssertionError);

These look correct to me. deepEqual is not strict (only ==) so Number and Boolean Objects (not to be confused with Number(num) and Boolean(bool), the new makes a big difference!) just coerce to being "objects".

@akdor1154
Copy link
Author

@Fishrock123 the docs support what you are saying (and even warn about this surprising behaviour), fair enough. I guess deepStrictEqual should throw in these cases though? (as according to the docs it includes a prototype check?)

@Fishrock123
Copy link
Contributor

Fishrock123 commented Dec 14, 2016

I guess deepStrictEqual should throw in these cases though?

Yes. This is one of many reasons why you should probably never use the Object forms of Simple Primitives.

@akdor1154
Copy link
Author

I am thinking more on this, is there any actual use case for someone to want deepEqual to think those types are equal?
A simple (a.valueOf() ==[=] b.valueOf()) at the top of deep[Strict]Equal would change behaviour in these cases, but the new behaviour would be much less surprising IMO. I guess changing behaviour like this is probably out of the question for now, though.

@Fishrock123
Copy link
Contributor

is there any actual use case for someone to want deepEqual to think those types are equal?

That is why we have deepStrictEqual. Changing the behavior is probably not viable due to existing potential ecosystem dependency.

@joyeecheung
Copy link
Member

joyeecheung commented Dec 15, 2016

These look correct to me. deepEqual is not strict (only ==) so Number and Boolean Objects (not to be confused with Number(num) and Boolean(bool), the new makes a big difference!) just coerce to being "objects".

@Fishrock123 I am 50/50 on this, given it's a 50/50 result among the user land modules that I've tested :). I am currently inclined to leave deepEqual as-is and perform a more strict type check for deepStrictEqual.

Also, I think we could stress that deepEqual is in fact, a "loose"(non-strict) comparison in the docs, in case people miss the docs of deepStrictEqual.(EDIT: the docs are fine, they make a pretty good example there). Some user land modules put the name "deep equal" on a more deepStrictEqual-ish functionality, so their user can be surprised when transitioning to use assert in the core. Given that power-assert is gaining popularity I think there will be more people doing this(I know a few of my colleges are evangelizing it).

Trying to make a PR now! :)

joyeecheung added a commit to joyeecheung/node that referenced this issue Feb 25, 2017
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)
joyeecheung added a commit that referenced this issue Feb 27, 2017
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this issue Mar 11, 2017
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
jungx098 pushed a commit to jungx098/node that referenced this issue Mar 21, 2017
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants