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

Only allow 'unique' and 'well-known' symbols #23

Merged
merged 6 commits into from
May 4, 2022

Conversation

acutmore
Copy link
Contributor

@acutmore acutmore commented Mar 2, 2022

Made a start on what the spec text changes could be if registered and well-known symbols were not allowed in WeakMap, WeakSet, WeakRef and FinalizationRegistry

Closes #21

@acutmore acutmore changed the title only allow unique symbols Only allow 'unique' symbols Mar 2, 2022
spec.emu Outdated Show resolved Hide resolved
spec.emu Outdated
</tr>
<tr>
<td>
<ins>%CanBeHeldWeakly%</ins>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We create this as a new intrinsic so it can be shared across WeakMap.isValidKey, WeakSet.isValidValue, WeakRef.isValidTarget and FinalizationRegistry.isValidTarget.

spec.emu Outdated
1. If Type(_argument_) is Symbol, then
1. For each element _e_ of the GlobalSymbolRegistry List (see <emu-xref href="#sec-symbol.for"></emu-xref>), do
1. If SameValue(_e_.[[Symbol]], _argument_) is *true*, return *false*.
1. Let _wellKnownSymbols_ be the symbols listed in <emu-xref href="#sec-well-known-symbols"></emu-xref>.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm referencing the section rather than the table, because I got a ecmarkup error saying it couldn't find the xref when referencing the table.

Copy link
Member

@rricard rricard left a comment

Choose a reason for hiding this comment

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

It is my understanding that for forward web compatibility we would prefer to actually name the exposed predicate something like isUnforgeable.

The naming here is clear on what we are evaluating: does the thing passed have the necessary property (of being unforgeable) that then happens to be compatible with being a weakmap key or not?

If we end up evolving WeakMap key compatibility later to registered symbols as well for instance, isValidKey would have to be changed, thus breaking web compatibility, while isUnforgeable or similarly named alternatives would not.

spec.emu Outdated Show resolved Hide resolved
spec.emu Outdated
</emu-clause>
</emu-clause>
<emu-clause id="sec-%canbeheldweakly%">
<h1>%CanBeHeldWeakly%( _argument_ )</h1>
Copy link
Member

Choose a reason for hiding this comment

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

We need to explicitly define .name, otherwise it will probably be missing or "%CanBeHeldWeakly%" (I never read the part of the spec that automatically defines function names). Or you could define it as an "exposed function" directly, similarly to how Array.prototype.values is defined normally and then Array.prototype.@@iterator "copies" it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I wasn't sure about this, I do say "The <dfn>%CanBeHeldWeakly%</dfn> intrinsic is an anonymous built-in function object, which I based off how throwTypeError is defined.

The reason I didn't do it as an exposed function was because it has a different name depending on it's home, so not clear which home should win, isValidKey | isValidValue | isValidTarget. There is some precedent for one of them winning though as there is Set.prototype.keys.name === 'values'.

spec.emu Outdated
1. For each element _e_ of the GlobalSymbolRegistry List (see <emu-xref href="#sec-symbol.for"></emu-xref>), do
1. If SameValue(_e_.[[Symbol]], _argument_) is *true*, return *false*.
1. Let _wellKnownSymbols_ be the symbols listed in <emu-xref href="#sec-well-known-symbols"></emu-xref>.
1. If _wellKnownSymbols_ contains _argument_, return *false*.
Copy link

@bathos bathos Mar 2, 2022

Choose a reason for hiding this comment

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

This might be relevant here: tc39/test262#3420.

The %Intl%.[[FallbackSymbol]] symbol is specified to be a new unique symbol associated with each realm. The text here would consider it to be unforgeable, which is theoretically true and will be in some implementations, e.g. Spidermonkey.

V8 however implemented the fallback symbol with the single-identity-across-realms characteristic of a well-known symbol. Though technically incorrect, it seems this may have been a deliberate decision and the discussion about which behavior is desired did not appear conclusive. This proposal might force the issue of getting engines / the Intl spec to agree on the true nature of this abomination.

Copy link
Member

Choose a reason for hiding this comment

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

Thats something that needs to either be changed in the spec, or, v8 needs to change their implementation, and it should be discussed in plenary regardless of this PR/proposal.

Copy link
Contributor Author

@acutmore acutmore Mar 4, 2022

Choose a reason for hiding this comment

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

If I have understood that issue, this is not an issue for this proposal in practice. Because the symbol value itself is not exposed anywhere, so it wouldn't be possible to try and use it in a WeakSet for example. So the spec would say that it is not well-known (cross realm) so can be used in a WeakSet, V8 implement it cross-realm so it is well-known so if they didn't allow it in a WeakSet that would be incorrect, but that incorrect behaviour can't be triggered in any way.

Please do correct anything I have misunderstood.

EDIT: I did misunderstand.

Copy link
Contributor Author

@acutmore acutmore Mar 4, 2022

Choose a reason for hiding this comment

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

I re-read the Intl spec and realised I misunderstood, the symbol is exposed directly.

Reflect.ownKeys(
  Intl.DateTimeFormat.call(Object.create(Intl.DateTimeFormat.prototype))
)[0]; // Symbol(IntlLegacyConstructedSymbol)

Copy link

@bathos bathos Mar 4, 2022

Choose a reason for hiding this comment

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

Yes. Though as you mentioned in another thread, the status of well-knowns as “forgeable” is pretty iffy*. If it’s decided that well-known symbols aren’t considered forgeable, then this outlier-in-practice thing is moot anyway.

* It’s strictly impossible to “prove” either conclusion in all possible scenarios from an ES code runtime POV, so while the spec describes them in terms that consistently support the well-knowns-are-unforgeable conclusion, I understand that this isn’t cut and dry since if a tree falls in the forest...

@acutmore
Copy link
Contributor Author

acutmore commented Mar 3, 2022

It is my understanding that for forward web compatibility we would prefer to actually name the exposed predicate something like isUnforgeable.

Thanks @rricard - good point that this was still being discussed. I've created an issue #24

@acutmore acutmore changed the title Only allow 'unique' symbols Only allow 'unique' and 'well-known' symbols Apr 24, 2022
@acutmore
Copy link
Contributor Author

Based on the most recently conversations in the Record & Tuple monthly call - I have updated the PR:

  • removed the previously included static predicates (e.g. WeakMap.isValidKey)
  • widened the CanBeHeldWeakly AO to also return true for well-known symbols by only returning false for symbols that are registered.

spec.emu Outdated Show resolved Hide resolved
@bathos
Copy link

bathos commented Apr 26, 2022

also return true for well-known symbols by only returning false for symbols that are registered.

@acutmore I’d thought about this (way too much) and came to the same conclusion, but wasn’t confident about my ability to articulate the case coherently. It’s really cool to see that this is where you’ve landed!

@rricard rricard merged commit a84561a into tc39:main May 4, 2022
@acutmore acutmore deleted the only-allow-unique-symbols branch June 1, 2022 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use of registered symbols would allow observation that a WeakMap entry has been freed
5 participants