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

doc: document asserts Weak(Map|Set) behavior #18248

Closed
wants to merge 5 commits into from

Conversation

BridgeAR
Copy link
Member

Right now it is not documentated that WeakMap entries are not
compared. This might result in some confusion. This adds a note
about the behavior in assert.deepStrictEqual. This documentation
is also references in util.isDeepStrictEqual, so we do not have
to document it again for that function as the underlying algorithm
is the same.

Refs: #18228
Closes: #18228

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
Affected core subsystem(s)

doc

Right now it is not documentated that WeakMap entries are not
compared. This might result in some confusion. This adds a note
about the behavior in `assert.deepStrictEqual`. This documentation
is also references in `util.isDeepStrictEqual`, so we do not have
to document it again for that function as the underlying algorithm
is the same.
@BridgeAR BridgeAR added the doc Issues and PRs related to the documentations. label Jan 19, 2018
@nodejs-github-bot nodejs-github-bot added assert Issues and PRs related to the assert subsystem. doc Issues and PRs related to the documentations. labels Jan 19, 2018
@BridgeAR
Copy link
Member Author

@@ -254,6 +255,16 @@ assert.deepStrictEqual({ [symbol1]: 1 }, { [symbol1]: 1 });
// OK, because it is the same symbol on both objects.
assert.deepStrictEqual({ [symbol1]: 1 }, { [symbol2]: 1 });
// Fails because symbol1 !== symbol2!

const weakMap1 = new WeakMap();
const weakMap2 = new WeakMap([1, 1]);
Copy link
Member

Choose a reason for hiding this comment

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

WeakMap throws an error in this code. I guess this example is WeakSet.

@@ -870,6 +881,8 @@ second argument. This might lead to difficult-to-spot errors.
[`Set`]: https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Set
[`Symbol`]: https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Symbol
[`TypeError`]: errors.html#errors_class_typeerror
[`WeakMap`]: https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/WeapMap
Copy link
Member

Choose a reason for hiding this comment

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

linter errors:
doc/api/assert.md
884:1-884:99 warning Found unused definition no-unused-definitions remark-lint
885:1-885:99 warning Found unused definition no-unused-definitions remark-lint

Copy link
Contributor

Choose a reason for hiding this comment

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

/WeapMap -> /WeakMap
-----^

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@BridgeAR
Copy link
Member Author

🙈 comments addressed

@BridgeAR
Copy link
Member Author

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

I would prefer we fix the actual behavior. If Map  are compared by entries WeakMap should be compared by entries too.

@@ -213,6 +213,8 @@ are recursively evaluated also by the following rules.
* Map keys and Set items are compared unordered.
* Recursion stops when both sides differ or both sides encounter a circular
reference.
* [`WeakMap`][] and [`WeakSet`][] will return true, no matter what values they
contain.
Copy link
Member

Choose a reason for hiding this comment

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

This sentence is not clear. Also can you use the present instead of the future?

Copy link
Member Author

@BridgeAR BridgeAR Jan 19, 2018

Choose a reason for hiding this comment

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

['WeakMap'][] and ['WeakSet'][] comparison does not rely on their values. See below for further details.?

@BridgeAR
Copy link
Member Author

@mcollina it is not possible to compare WeakMap and WeakSet entries. That is the whole point about this PR.

You can not look into the WeakMap. You can only check if a entry exists or not but you have to know what entry that is.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 19, 2018

I'd prefer if the behavior was changed to throw as suggested in #18228. In that case, this should not land on master.

@BridgeAR
Copy link
Member Author

Mini poll:

👍 document the current behavior (this PR)
🎉 always fail for WeakMap / WeakSet as in #18228
❤️ throw an error in case WeakMap / WeakSet is compared

Please note that this applies to both: assert.deepStrictEqual and util.isDeepStrictEqual (plus assert.deepEqual but documenting all deficiencies about that function is not possible anyway...).

@mcollina
Copy link
Member

The only thing I'm not ok with is to document the current behavior.

@BridgeAR
Copy link
Member Author

BridgeAR commented Jan 19, 2018

@mcollina in #18228 (comment) I mentioned why I do not like to always fail (as in return false). That was the reason for this PR. I guess I am fine with throwing. It might be a bit confusing for some users but that is the case one way or the other.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 19, 2018

I'm fine with documenting this behavior as a warning to users. I'd just rather not land that warning on master at all.

@TimothyGu
Copy link
Member

TimothyGu commented Jan 20, 2018

I don't see why WeakMap and WeakSet should be treated differently from regular objects. Thus, documentation only is the best option IMO.

@BridgeAR
Copy link
Member Author

I personally also think that WeakMap and WeakSet should be treated as any other regular object. The only reason why I would be OK with throwing is that many people seem to be confused about the behavior and that might clarify it right away. It might of course also lead to some trouble.

@BridgeAR BridgeAR requested a review from a team January 20, 2018 09:00
@targos
Copy link
Member

targos commented Jan 20, 2018

I'm for documenting the current behavior.

@mcollina
Copy link
Member

Documenting is definitely something we should land in 8 and 9.

I personally also think that WeakMap and WeakSet should be treated as any other regular object. The only reason why I would be OK with throwing is that many people seem to be confused about the behavior and that might clarify it right away. It might of course also lead to some trouble.

I disagree, people look at the docs when they see an exception or undocumented behavior. However this will be mostly silent. I compare two things that I expect to contain the same keys, but they are not and the test will not fail. Comparing the attached properties of a WeakMap is definitely not the behavior a user is looking for.
I would largely prefer that comparing two WeakMap always throwed "you can't compare two WeakMaps".

@yosuke-furukawa
Copy link
Member

this situation is similar to NaN .
NaN is uncomparable, NaN === NaN always returns false. So that I firstly think WeakMap and WeakSet would be better to return false on comparison.

However, util.isDeepStrictEqual returns true on util.isDeepStrictEqual(NaN, NaN).
Thus WeakMap/WeakSet also returns true cause they are uncomparable.

If we define util.isDeepStrictEqual returns true for uncomparable object, current behavior is reasonable.

but i feel it is not """strict equal""" ...

According to the api docs, util.isDeepStrictEqual explains Returns true if there is deep strict equality between val1 and val2. Otherwise, returns false..

If we follow the sentence, I think we would be better to return false cause that is not deep "strict" equal.

If we could change this function behavior drastically, throw an Error on uncomparable object like (NaN, WeakMap, WeakSet).

Anyway, we need to add some documentation for this function.

@jasnell
Copy link
Member

jasnell commented Jan 22, 2018

My preference would be:

  1. document the limitations in < 9.x
  2. throw when comparing uncomparables in >= 10.x

@BridgeAR
Copy link
Member Author

@yosuke-furukawa I am not sure in what way NaN shall not be comparable. It is very well comparable, just not by using ===. And that is done right now and it is the reason why the function returns true.

And I already tried to point out that using strict in the util function name is a bad idea, just similar to the assert functions. It is not about strict comparison because we test for a lot of different things that have nothing to do with "real" strict comparison.

@BridgeAR
Copy link
Member Author

Ok, it still seems highly controversial what to do and what not. The only relatively clear thing is that always returning false / always failing the check in case WeakMap or WeakSet is used is not the favorite way of handling it.

So either we should document the behavior or throw.

@nodejs/tsc I think this is something for your agenda. Please read the arguments closely.

@BridgeAR BridgeAR added tsc-review tsc-agenda Issues and PRs to discuss during the meetings of the TSC. labels Jan 22, 2018
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 30, 2018
BridgeAR added a commit to BridgeAR/node that referenced this pull request Feb 1, 2018
Right now it is not documentated that WeakMap entries are not
compared. This might result in some confusion. This adds a note
about the behavior in `assert.deepStrictEqual`. This documentation
is also references in `util.isDeepStrictEqual`, so we do not have
to document it again for that function as the underlying algorithm
is the same.

PR-URL: nodejs#18248
Fixes: nodejs#18228
Refs: nodejs#18228
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BridgeAR
Copy link
Member Author

BridgeAR commented Feb 1, 2018

Landed in 936eea5

The question to the TSC still stands nevertheless.

@BridgeAR BridgeAR closed this Feb 1, 2018
@yosuke-furukawa
Copy link
Member

@BridgeAR

Changing the name now is not really possible anymore.

But we could change this function behavior to truly strict equal (I know that the change breaks api behavior, so we will update it on v10). and if we really need the SameValue comparison, we could add assert.deepSameValue and util.isDeepSameValue .

Nevertheless, I do not see any reasoning for any of the mentioned alternatives. It is just that you say you think it is neither of the mentioned comparisons and that is why it should be changed to one of the suggestions without saying why any of those are better than the current implementation.

  1. Returning false

firstly I think this is the better behavior than current behavior. Because most JS engineer using WeakMap and WeakSet know that they do not have a list API and those collections are depends on GC. WeakCollection has non-determinism. The strict compare is impossible, so false.

Returning false would be wrong out of my perspective and it is weird that these objects shall be special handles opposing to all other possible objects.

IMO It is not weird, cause WeakMap|Set are special collection about comparison, we need special handles.

  1. Throwing Error

This is a kind of type error. it means "we do not support WeakMap|WeakSet on this function".
My ideal solution is this.

  1. Returning undefined

Returning undefined is a compromised plan. It means "no answer (neither true nor false)".
I don't think this behavior is good. but throwing error changes current behavior drastically, so I just set the compromised plan.

@MylesBorins
Copy link
Contributor

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 8, 2018
Right now it is not documentated that WeakMap entries are not
compared. This might result in some confusion. This adds a note
about the behavior in `assert.deepStrictEqual`. This documentation
is also references in `util.isDeepStrictEqual`, so we do not have
to document it again for that function as the underlying algorithm
is the same.

PR-URL: nodejs#18248
Fixes: nodejs#18228
Refs: nodejs#18228
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BridgeAR BridgeAR mentioned this pull request Mar 8, 2018
4 tasks
@BridgeAR
Copy link
Member Author

BridgeAR commented Mar 8, 2018

Backport opened in #19230

MylesBorins pushed a commit that referenced this pull request Mar 15, 2018
Right now it is not documentated that WeakMap entries are not
compared. This might result in some confusion. This adds a note
about the behavior in `assert.deepStrictEqual`. This documentation
is also references in `util.isDeepStrictEqual`, so we do not have
to document it again for that function as the underlying algorithm
is the same.

Backport-PR-URL: #19230
PR-URL: #18248
Fixes: #18228
Refs: #18228
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@targos targos mentioned this pull request Mar 18, 2018
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
Right now it is not documentated that WeakMap entries are not
compared. This might result in some confusion. This adds a note
about the behavior in `assert.deepStrictEqual`. This documentation
is also references in `util.isDeepStrictEqual`, so we do not have
to document it again for that function as the underlying algorithm
is the same.

Backport-PR-URL: #19230
PR-URL: #18248
Fixes: #18228
Refs: #18228
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@targos targos added backported-to-v9.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v9.x labels Mar 24, 2018
@targos
Copy link
Member

targos commented Mar 24, 2018

Can we remove tsc-* labels?

@BridgeAR
Copy link
Member Author

I would say so. It is still a matter of finding the right way to continue but that should be done in the corresponding issue.

@BridgeAR BridgeAR removed tsc-agenda Issues and PRs to discuss during the meetings of the TSC. tsc-review labels Mar 24, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Right now it is not documentated that WeakMap entries are not
compared. This might result in some confusion. This adds a note
about the behavior in `assert.deepStrictEqual`. This documentation
is also references in `util.isDeepStrictEqual`, so we do not have
to document it again for that function as the underlying algorithm
is the same.

PR-URL: nodejs#18248
Fixes: nodejs#18228
Refs: nodejs#18228
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins
Copy link
Contributor

Requested backport to 8.x in #19230

@BridgeAR BridgeAR deleted the improve-weak-documentation branch April 1, 2019 23:38
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. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.