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

commented Jan 19, 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.

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

doc: document asserts Weak(Map|Set) behavior
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

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2018

@@ -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]);

This comment has been minimized.

Copy link
@yosuke-furukawa

yosuke-furukawa Jan 19, 2018

Member

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

This comment has been minimized.

Copy link
@yosuke-furukawa

yosuke-furukawa Jan 19, 2018

Member

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

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt Jan 19, 2018

Contributor

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

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Jan 24, 2018

Author Member

Fixed

@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2018

🙈 comments addressed

@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2018

@mcollina
Copy link
Member

left a comment

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.

This comment has been minimized.

Copy link
@mcollina

mcollina Jan 19, 2018

Member

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

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Jan 19, 2018

Author Member

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

@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2018

@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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2018

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

This comment has been minimized.

Copy link
Member

commented Jan 19, 2018

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

@BridgeAR

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Contributor

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.

@odykyi

odykyi approved these changes Jan 19, 2018

@TimothyGu

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

commented Jan 20, 2018

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 nodejs/tsc Jan 20, 2018

@targos

This comment has been minimized.

Copy link
Member

commented Jan 20, 2018

I'm for documenting the current behavior.

@mcollina

This comment has been minimized.

Copy link
Member

commented Jan 20, 2018

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

This comment has been minimized.

Copy link
Member

commented Jan 21, 2018

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

This comment has been minimized.

Copy link
Member

commented Jan 22, 2018

My preference would be:

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

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2018

@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

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2018

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 added a commit to BridgeAR/node that referenced this pull request Feb 1, 2018

doc: document asserts Weak(Map|Set) behavior
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

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2018

Landed in 936eea5

The question to the TSC still stands nevertheless.

@BridgeAR BridgeAR closed this Feb 1, 2018

@yosuke-furukawa

This comment has been minimized.

Copy link
Member

commented Feb 1, 2018

@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.

kcaulfield94 added a commit to kcaulfield94/node that referenced this pull request Feb 2, 2018

doc: document asserts Weak(Map|Set) behavior
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>

msoechting added a commit to hpicgs/node that referenced this pull request Feb 5, 2018

doc: document asserts Weak(Map|Set) behavior
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>

msoechting added a commit to hpicgs/node that referenced this pull request Feb 7, 2018

doc: document asserts Weak(Map|Set) behavior
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

This comment has been minimized.

Copy link
Member

commented Feb 20, 2018

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

doc: document asserts Weak(Map|Set) behavior
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 added a commit to BridgeAR/node that referenced this pull request Mar 8, 2018

doc: document asserts Weak(Map|Set) behavior
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 added a commit to BridgeAR/node that referenced this pull request Mar 8, 2018

doc: document asserts Weak(Map|Set) behavior
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 referenced this pull request Mar 8, 2018

Closed

[9.x backport] assert & util PRs #19230

4 of 4 tasks complete
@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Mar 8, 2018

Backport opened in #19230

MylesBorins added a commit that referenced this pull request Mar 15, 2018

doc: document asserts Weak(Map|Set) behavior
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 referenced this pull request Mar 18, 2018

Merged

v9.9.0 proposal #19428

MylesBorins added a commit that referenced this pull request Mar 20, 2018

doc: document asserts Weak(Map|Set) behavior
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

This comment has been minimized.

Copy link
Member

commented Mar 24, 2018

Can we remove tsc-* labels?

@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Mar 24, 2018

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.

MayaLekova added a commit to MayaLekova/node that referenced this pull request May 8, 2018

doc: document asserts Weak(Map|Set) behavior
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

This comment has been minimized.

Copy link
Member

commented Jul 31, 2018

Requested backport to 8.x in #19230

@BridgeAR BridgeAR deleted the BridgeAR:improve-weak-documentation branch Apr 1, 2019

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