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: stricter object comparison #27495

Open
wants to merge 1 commit into
base: master
from

Conversation

@BridgeAR
Copy link
Member

BridgeAR commented Apr 30, 2019

This changes the comparison for assert.equal() and
assert.notEqual() to compare objects by reference and not with the
abstract equality comparator. That prevents some mistakes that most
people would not anticipate as equal.

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

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@BridgeAR BridgeAR requested a review from Trott Apr 30, 2019
@nodejs-github-bot

This comment has been minimized.

generatedMessage: true
});
assert.throws(() => {
a.equal({ [Symbol.toPrimitive]() { return 5; } }, 5);

This comment has been minimized.

Copy link
@Trott

Trott Apr 30, 2019

Member

I guess this is the edge case that this is for? Is there a simpler/more common one? (It's OK if there's not. I'm just making sure I understand this change!)

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR May 1, 2019

Author Member

The above one is the more common one: 0 == []. This is IMHO quite bad. I can also add e.g., [1] == true or ['0'] == 0 (true, 1, '1', 1n, etc. are all interchangeable, just as false, 0, '', '0'...).

Both sides are stringified when comparing objects.

lib/assert.js Outdated Show resolved Hide resolved
@targos

This comment has been minimized.

Copy link
Member

targos commented May 1, 2019

I'm not convinced this is a good idea. I prefer the function to behave as it was always documented with a == b than having a different behavior for some types. IMO [1]==1 is not more confusing than true==1.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented May 1, 2019

I'm not convinced this is a good idea. I prefer the function to behave as it was always documented with a == b than having a different behavior for some types. IMO [1]==1 is not more confusing than true==1.

I agree. I might yet be convinced, but it would have to be some strongly compelling reason to have assert.equal() differ from ==.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented May 1, 2019

I consider objects to be the most surprising due to e.g., [[[[12345]]]] == 12345n and Buffer.from([10, 44, 45, 40, 50, 60, 70, 80, 90, 10]) == '\n,-(2<FPZ\n'. This can cause really weird bugs. I could switch to a deprecation message instead for these types first (CITGM shows some errors and I'll open PRs to fix those).

Ideally this functionality would not exist but since it does, I think it's best to minimize the surprises for users.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented May 2, 2019

Ideally this functionality would not exist but since it does, I think it's best to minimize the surprises for users.

Agreed. The question is which is more surprising: That the loose equal function gets it "wrong" in some weird ways? Or that the loose equal function differs from the corresponding == operator? There's legitimate room for debate on this, I think.

@targos

This comment has been minimized.

Copy link
Member

targos commented May 2, 2019

Let's take you second example: assert.equal(Buffer.from([10, 44, 45, 40, 50, 60, 70, 80, 90, 10]), '\n,-(2<FPZ\n').
If someone is doing that, it is unlikely to be a mistake. They are using the feature that Buffer is coerced to a string for comparison.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented May 2, 2019

@Trott

The question is which is more surprising: That the loose equal function gets it "wrong" in some weird ways? Or that the loose equal function differs from the corresponding == operator?

There is definitely no absolute right or wrong here. I think for new people this change will help. Most developers do not have such in depth experience that they know how the algorithm actually works. They know that primitives like 1 == '1' are true but other things are often unknown (in my experience when talking with people about this subject). I am also relatively sure that this might surface a few bugs that where hidden so far. In other cases it might displease people who are proficient with the language and know exactly how it works and they wanted this behavior. But even the latter should likely use the strict variation because someone else who reads their code might not know why things work as they do and we are all humans and make mistakes.

I am relatively certain that the failure in JSONStream is an actual error either in the test or in the program and it just was not detected by the author due to the very loose comparison: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1840/nodes=fedora-latest-x64/testReport/junit/(root)/citgm/JSONStream_v1_3_5/

@targos

If someone is doing that, it is unlikely to be a mistake. They are using the feature that Buffer is coerced to a string for comparison.

The behavior in iconv was intentional (and is pretty much my example above) but due to the reasons I outline above I still think it would be best to change it (https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1840/nodes=fedora-latest-x64/testReport/junit/(root)/citgm/iconv_v2_3_4/). @bnoordhuis since you wrote this, what do you think would be best?


I don't think this is an ideal approach but I think it'll uncover a few actual bugs while also making sure that the code is understood by more people even though it'll require some people to change their tests at times. The loose equality is already doc deprecated and should hopefully be used less over time in general.

It is nothing that we have to do and I will never use any of the loose comparisons. I just believe it'll actually help some people in a couple of cases.

@BridgeAR BridgeAR requested review from Trott, targos and bnoordhuis May 6, 2019
@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented May 10, 2019

since you wrote this, what do you think would be best?

This is about making it more intuitive to the average user? I'd be okay with that although I'm not sure if it's actually more intuitive.

assert.equal() behaves the way I expect it to but I'm not a typical user. Probably no maintainer is. :-)

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented May 13, 2019

@bnoordhuis

This is about making it more intuitive to the average user? I'd be okay with that although I'm not sure if it's actually more intuitive.

Not directly more intuitive but less error prone for the average user (quite a few developers I worked with before are not aware of the details or the implications of the abstract comparison).

@Trott Trott force-pushed the nodejs:master branch from 1ecc406 to 49cf67e Sep 17, 2019
This changes the comparison for `assert.equal()` and
`assert.notEqual()` to compare objects by reference and not with the
abstract equality comparator. That prevents some mistakes that most
people would not anticipate as equal.
@BridgeAR BridgeAR force-pushed the BridgeAR:prevent-bad-loose-comparison branch from 2191af9 to f6a7dee Dec 19, 2019
@devnexen devnexen force-pushed the nodejs:master branch from e8a4568 to 5289f80 Dec 26, 2019

```js
// WARNING: This does not throw an AssertionError!
assert.deepEqual(/a/gi, new Date());

This comment has been minimized.

Copy link
@ljharb

ljharb Jan 8, 2020

Member

i think it would be very surprising to change this. One of the whole points of this method is to compare distinct objects based on their shape/contents and not based on their identity.

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Jan 8, 2020

Author Member

That was just an somewhat unrelated update to the docs. It does throw an error by now and the example is outdated.

This comment has been minimized.

Copy link
@ljharb

ljharb Jan 8, 2020

Member

ok, fair on this line - but my comment still applies to “reference equality in objects”. It’s trivial to do that already with ===; what’s hard is loose equality, which is what the non-strict assert methods do.

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Jan 11, 2020

Author Member

The deep equal function is not changed at all and it still applies the loose equality check while comparing properties. This mainly aligns the loose equal check with the deep equal one by special handling objects. Comparing objects using the abstract equality calls toString and that definitely causes surprising behavior. This is just a step to limit the surprising parts a tiny bit. It's definitely not ideal but it might be some kind of compromise for the loose equality check.

This comment has been minimized.

Copy link
@ljharb

ljharb Jan 11, 2020

Member

That's not surprising, that's the purpose of loose equality checks when comparing an object and a non-object. It's a desired feature of deepEqual. (just like string == object works in the language, more or less)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.