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

Normative: Permit Symbols as WeakMap keys #2777

Merged
merged 1 commit into from
Mar 18, 2023

Conversation

acutmore
Copy link
Contributor

@acutmore acutmore commented May 17, 2022

rendered preview here

This PR represents the Stage 3 specification text for Symbols as WeakMap keys.

The proposed spec has been written as a PR against the full spec due to the high number of small tweaks to descriptions and notes in sections such as liveness.

A separate minimal proposal specification with only the core changes is available at http://tc39.es/proposal-symbols-as-weakmap-keys

Closes #1194

@acutmore
Copy link
Contributor Author

I'm getting a 403 error when it tries to upload the preview: https://github.com/tc39/ecma262/runs/6472329978?check_suite_focus=true

Is there something I need to do?

@ljharb ljharb added normative change Affects behavior required to correctly evaluate some ECMAScript source text needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 pending stage 4 This proposal has not yet achieved stage 4, but may otherwise be ready to merge. proposal This is related to a specific proposal, and will be closed/merged when the proposal reaches stage 4. labels May 17, 2022
@ljharb
Copy link
Member

ljharb commented May 17, 2022

@acutmore i'll take a look at it when i have a moment.

spec.html Outdated
<emu-note type="editor">
<p>This abstract operation determines if _v_ is considered to have an identity suitable for a weak reference. i.e. Is _v_ a valid candidate key of a WeakMap, or entry in a WeakSet, or target of a WeakRef or FinalizationRegistry.</p>
<p>'Registered symbols' are not considered to have a suitable identity because their identity is similar to strings, and implementations may attempt to collect them when they are unreachable.</p>
<p>'Well-Known symbols' <emu-xref href="#sec-well-known-symbols"></emu-xref> are considered to have a suitable identity because there are a small finite number of them and implementations do not attempt to unobservably collect them.</p>
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look right to me. I think well-known symbols should behave like registered symbols (cannot be used as WeakMap keys).

delete globalThis.Symbol
delete Array.prototype.concat
// Now at this point, there is NO reference accessible from
// the user land to @@isConcatSpreadable
const realm = new ShadowRealm()
const newConcatSpreadable = realm.evaluate('Symbol.isConcatSpreadable')
// But I can get it back from another Realm.

In the code above, well-known symbols behave like a "registered symbol". They're thrown away (not accessible from the user's land) and can be re-construct from the ground up.

It's totally reasonable if an engine wants to implement it as a registered symbol. We should not allow it to be the WeakMap key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @Jack-Works , thanks for reviewing the proposal.

The champion group have been discussing this at the record&tuple monthly meeting. The reason we landed on only disallowing registered symbols is that: engines like spidermonkey do use an internal WeakMap to store registered symbols but we did not find evidence of an engine applying this ‘optimisation’ to well known symbols. As there are a small finite number of well known symbols it is arguably more efficient to allocate them once and share these references across all realms (as engines currently do) than to try and collect and re-materialize them.

The optimisation has greater benefit for registered symbols as there is effectively no limit to how many a program could create.

In your example you would also need delete Symbol.prototype.constructor. The examples of code demonstrating how well known symbols could be made unreachable do not seem to be realistic use cases for the majority of programs, we find this to be further evidence that engines would be unlikely to invest in applying this 'optimisation'.

I hope that helps clarify. If not please do raise an issue https://github.com/tc39/proposal-symbols-as-weakmap-keys/issues so we can discuss it there. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I agree to make well-known symbols inaccessible is a very edge case.

I just want to use this example to show the difference in how we categorize well-known symbols. I think well-known symbols are more like registered symbols than normal symbols, if my example doesn't convince you, I don't have stronger objections.

Copy link
Member

@hax hax May 18, 2022

Choose a reason for hiding this comment

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

I assume the spec is not only a spec of current known implementations, but for all implementors/programmers, so the problem here might be, current draft text only say "implementations do not attempt to unobservably collect well-known symbols" in the editor note, but if we allow well-known symbols, I think the text should clearly say "well-known symbols must never be collected observably" (or it will break SES requirements if i understand correctly).

On the other side, I also very dislike allowing well-known symbols, because if they must never be collected, it seems very contradict the "CanBeHeldWeakly" semantic and the purpose of using weakmap/weakset/weakref in the first place, and I can't imagine any valid use case. Allowing them just add confusion and footgun to developers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so the problem here might be, current draft text only say "implementations do not attempt to unobservably collect well-known symbols" in the editor note, but if we allow well-known symbols, I think the text should clearly say "well-known symbols must never be collected observably"

Good point. I agree that I worded this note poorly. It should not reference current state of engines. I will re-word.

On the other side, I also very dislike allowing well-known symbols, because if they must never be collected, it seems very contradict the "CanBeHeldWeakly" semantic

One reason the champion group is ok with this is because for a single realm application this is no different to Object.prototype - while it can be held weakly, this does not imply that it is useful to do so. Hope that helps explain our reasoning.

Copy link
Member

Choose a reason for hiding this comment

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

Delegates have raised concern that having an API that treats unique and well-known symbols differently adds more complexity to users, more than helps to avoid bugs.

Treats well-known symbols and registered symbols differently also have similar problems, and even worse, because in most aspects ("undeniable", GC behavior), well-known symbols are much close to registered symbols.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's exactly right.

Copy link
Member

Choose a reason for hiding this comment

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

Well-known symbols aren't part of the symbol registry. They are different, in a way that well-known and normal symbols aren't different.

Copy link
Member

@hax hax May 23, 2022

Choose a reason for hiding this comment

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

WeakMap does not attempt to block invalid objects that can't be collected

I didn't talk about the occasional uncollectable cases (for example, unwritable and unconfigurable properties on globalThis will never be collected, wm.set(wm) and Object.prototype are also such cases), but the inherent non-GC/unobservable-GC cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WeakMap does not attempt to block invalid objects that can't be collected

I didn't talk about the occasional uncollectable cases (for example, unwritable and unconfigurable properties on globalThis will never be collected, wm.set(wm) and Object.prototype are also such cases), but the inherent non-GC/unobservable-GC cases.

I was not clear. I did not mean to suggest that well-known symbols are the same as these objects. What I mean is that the existing API does not attempt to throw a fatal exception for values that are likely to be programmer errors.

The main reason for throwing an exception on well-known symbols wouldn’t be because they don’t have observable GC but because we think that this level of protection is required to prevent users of the API making a mistake. The use cases for this API do not suggest that attempting to use well-known symbols as weak map keys will be a common mistake - so it is perhaps simpler to allow them rather than special case them.

@acutmore
Copy link
Contributor Author

@acutmore i'll take a look at it when i have a moment.

The generated preview seems to have sorted itself out now - thanks anyway!

Copy link
Contributor

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

A few nits, but the spec text looks pretty good modulo final decisions regarding registered and well-known symbols.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated
</li>
</ul>
<emu-note>
The second condition above intends to capture the intuition that an object is live if its identity is observable via non-WeakRef means. An object's identity may be observed by observing a strict equality comparison between objects or observing the object being used as key in a Map.
The second condition above intends to capture the intuition that a referent is live if its identity is observable via non-WeakRef means. A referent's identity may be observed by observing a strict equality comparison or observing the value being used as key in a Map.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The second condition above intends to capture the intuition that a referent is live if its identity is observable via non-WeakRef means. A referent's identity may be observed by observing a strict equality comparison or observing the value being used as key in a Map.
The second condition above intends to capture the intuition that a referent is live if its identity is observable via non-WeakRef means. A referent's identity may be observed by observing a strict equality comparison or observing the value being used as key in a Map. Note that some values, such as well-known symbols, can become observable <em>after</em> an interval in which they are unreachable.

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 more than happy to have this change, though curious what made you think to add it? In my mind's pseudo-mark-sweep-gc of the spec, there is always at least one live realm during execution and the AOs and intrinsic functions are like a static roots, so because they reference the well-known symbols then the symbols are reachable from the GC - even if not reachable from userland.

@rricard and I did briefly talk about if we should add a concrete addition to the definition of liveness that says well-known symbols are always considered live. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would be in favor. It is non-obvious that well-known and registered symbols are always live as that term is defined by the spec, and that detail should be highlighted now that this proposal makes it observably relevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR updated to make the liveness of well-known symbols explicit.

Copy link
Member

Choose a reason for hiding this comment

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

Can we move or duplicate the editor's note about well-known symbol under this section.

It's not immediately clear why well-known symbols are not covered by the second bullet point. While some well-known symbols are syntactically undeniable, technically an execution where the deniable well-known symbols were removed from the Symbol global and where no new realm could be created wouldn't allow their observation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW well-known symbols can't be removed from the global Symbol, they are non-writable and non-configurable. To deny access the whole Symbol object needs to be denied and any other operation that could expose the symbol.

// first run before all other code:
delete Symbol.prototype.constructor;
delete gbloalThis.Symbol;
delete Array.prototype.concat;
delete globalThis.ShadowRealm; // and all other ways to construct a new realm
// No longer an execution that could observe the identity of `Symbol.isConcatSpreadable`

There is the question of how much flexibility to give engines here. In the above situation should they be allowed to GC @@isConcatSpreadable. The idea behind the 3rd bullet point (saying that well-known symbols are always considered live) is to deny that ability to ensure consistency across engines and avoid ambiguity in the spec. Though I can appreciate if engines would rather have the freedom.

cc: @syg @codehag

Copy link
Member

Choose a reason for hiding this comment

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

I'd be fine with adding a note here, but I don't think we need any more normative text to indicate that well-known symbols stay alive. There are plenty of values related to each Realm that implementations keep alive (e.g., certain built-in functions, and the exact set may differ between engines). Engines are always allowed to keep objects alive for longer or indefinitely, so these cases are clearly covered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @littledan , that's a good point. If we start to specifically mention well-known symbol here it starts to raise the question why other thing are/are-not also specified.

I've backed out the normative change. I'll re-add later as an editors note.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@acutmore
Copy link
Contributor Author

1 week till plenary. If reviewers could take a look at the latest draft this week that would be great. Thanks!
If happy with proposed text then we can check off on the tracking issue tc39/proposal-symbols-as-weakmap-keys#17

cc: @brad4d @ljharb @mhofman @gibson042

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Before this lands, I think 402 will need to be updated so the IntlFallbackSymbol is appended to the well-known symbols table (such that the symbol is automatically allowed to be held weakly)

Looks good overall though!

spec.html Show resolved Hide resolved
spec.html Outdated
Comment on lines 40471 to 40497
1. If Type(_key_) is not Object, throw a *TypeError* exception.
1. If CanBeHeldWeakly(_key_) is *false*, throw a *TypeError* exception.
Copy link
Member

Choose a reason for hiding this comment

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

personally i'd find it nice if the refactor of "type is object" to "canbeheldweakly" was done in a separate commit (or PR) - ie, that one first, and the "allow symbols" second. I think it would make the changes much easier to review.

not sure what @tc39/ecma262-editors think about that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

During stage 2 I've been prioritizing keeping the git history intact, so hopefully easier for an individual to see what has changed since the last time they reviewed.

If this gets to stage 3 we could definitely look at splitting apart the editorial and normative changes into separate PRs as you suggest.

@acutmore
Copy link
Contributor Author

acutmore commented Jun 1, 2022

Before this lands, I think 402 will need to be updated so the IntlFallbackSymbol is appended to the well-known symbols table (such that the symbol is automatically allowed to be held weakly)

Looks good overall though!

The AO for determining if a symbol can be held weakly is done by exclusion, so IntlFallbackSymbol will be allowed implicitly. It would only be disallowed if an implementation had incorrectly implemented it using Symbol.for, which does not appear to be the case for the implementations I've checked.

https://github.com/v8/v8/blob/5fe0aa3bc79c0a9d3ad546b79211f07105f09585/src/init/heap-symbols.h#L457

https://searchfox.org/mozilla-central/rev/3419858c997f422e3e70020a46baae7f0ec6dacc/js/src/builtin/intl/CommonFunctions.js#735

https://github.com/WebKit/WebKit/blob/84cfeb18efa90b3f67962f66c701f753633c618a/Source/JavaScriptCore/builtins/BuiltinNames.cpp#L44

@acutmore acutmore requested a review from ljharb June 1, 2022 11:29
@ljharb
Copy link
Member

ljharb commented Jun 1, 2022

@acutmore thats a good callout, but a cross-realm, non-registry symbol still belongs in that table, and imo it’s an editorial error in 402 that it does not.

Copy link
Contributor

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

The spec text is still coherent for disallowing registered symbols while allowing unique symbols, well-known symbols, and the ECMA-402 [[FallbackSymbol]] (which is effectively a unique symbol because per spec it is not shared across realms).

spec.html Outdated
@@ -12349,33 +12349,36 @@ <h1>Objectives</h1>
<emu-clause id="sec-liveness">
<h1>Liveness</h1>

<p>For some set of objects _S_, a <dfn>hypothetical WeakRef-oblivious</dfn> execution with respect to _S_ is an execution whereby the abstract operation WeakRefDeref of a WeakRef whose referent is an element of _S_ always returns *undefined*.</p>
<p>For some set of objects and/or symbols, _S_, a <dfn>hypothetical WeakRef-oblivious</dfn> execution with respect to _S_ is an execution whereby the abstract operation WeakRefDeref of a WeakRef whose referent is an element of _S_ always returns *undefined*.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Future-proofing suggestion:

Suggested change
<p>For some set of objects and/or symbols, _S_, a <dfn>hypothetical WeakRef-oblivious</dfn> execution with respect to _S_ is an execution whereby the abstract operation WeakRefDeref of a WeakRef whose referent is an element of _S_ always returns *undefined*.</p>
<p>For some set of ECMAScript language values, _S_, a <dfn>hypothetical WeakRef-oblivious</dfn> execution with respect to _S_ is an execution whereby the abstract operation WeakRefDeref of a WeakRef whose referent is an element of _S_ always returns *undefined*.</p>

spec.html Outdated
</emu-note>
<emu-note>
WeakRef-obliviousness is defined on sets of objects instead of individual objects to account for cycles. If it were defined on individual objects, then an object in a cycle will be considered live even though its Object value is only observed via WeakRefs of other objects in the cycle.
WeakRef-obliviousness is defined on sets of objects or symbols instead of individual referents to account for cycles. If it were defined on individual values, then a referent in a cycle will be considered live even though its value is only observed via WeakRefs of other referents in the cycle.
Copy link
Contributor

Choose a reason for hiding this comment

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

Future-proofing suggestion (with bonus grammar corrections):

Suggested change
WeakRef-obliviousness is defined on sets of objects or symbols instead of individual referents to account for cycles. If it were defined on individual values, then a referent in a cycle will be considered live even though its value is only observed via WeakRefs of other referents in the cycle.
WeakRef-obliviousness is defined on sets rather than individual referents to account for cycles. If it were defined on individual values, then a referent in a cycle would be considered live even if its value were only observed via WeakRefs of other referents in the cycle.

spec.html Outdated
</emu-note>

<p>At any point during evaluation, a set of objects _S_ is considered <dfn>live</dfn> if either of the following conditions is met:</p>
<p>At any point during evaluation, a set of objects and/or symbols, _S_, is considered <dfn>live</dfn> if any of the following conditions are met:</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Future-proofing suggestion:

Suggested change
<p>At any point during evaluation, a set of objects and/or symbols, _S_, is considered <dfn>live</dfn> if any of the following conditions are met:</p>
<p>At any point during evaluation, a set of language values, _S_, is considered <dfn>live</dfn> if any of the following conditions are met:</p>

spec.html Outdated
@@ -12389,20 +12392,20 @@ <h1>Liveness</h1>
<emu-clause id="sec-weakref-execution">
<h1>Execution</h1>

<p>At any time, if a set of objects _S_ is not live, an ECMAScript implementation may perform the following steps atomically:</p>
<p>At any time, if a set of objects and/or symbols _S_ is not live, an ECMAScript implementation may perform the following steps atomically:</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Future-proofing suggestion:

Suggested change
<p>At any time, if a set of objects and/or symbols _S_ is not live, an ECMAScript implementation may perform the following steps atomically:</p>
<p>At any time, if a set of language values _S_ is not live, an ECMAScript implementation may perform the following steps atomically:</p>

spec.html Outdated
</h1>
<dl class="header">
<dt>description</dt>
<dd>It returns *true* if and only if _v_ is considered to have an identity suitable for a weak reference. i.e. Is _v_ a valid candidate key of a WeakMap, or entry in a WeakSet, or target of a WeakRef or FinalizationRegistry.</dd>
Copy link
Contributor

Choose a reason for hiding this comment

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

This explanation is definitely appreciated, but a bit clunky. I'm not sure I can improve it significantly, though.

Suggested change
<dd>It returns *true* if and only if _v_ is considered to have an identity suitable for a weak reference. i.e. Is _v_ a valid candidate key of a WeakMap, or entry in a WeakSet, or target of a WeakRef or FinalizationRegistry.</dd>
<dd>It returns *true* if and only if _v_ is considered to have an identity suitable for weak reference (i.e., a key in a WeakMap or entry in a WeakSet or target of a WeakRef or FinalizationRegistry).</dd>

@mhofman
Copy link
Member

mhofman commented Jun 7, 2022

@acutmore thats a good callout, but a cross-realm, non-registry symbol still belongs in that table, and imo it’s an editorial error in 402 that it does not.

I don't believe it's an error. The fallback symbol is not meant to be cross-realm, 402 says explicitly it isn't. However there is no impact1 to this proposal since in both cases it could be held weakly.

Footnotes

  1. In practice, making Intl unreachable in the current realm would potentially make the symbol to itself become unreachable, which could be observed through WeakRef / FinalizationRegistry if implementations did the work to collect these intrinsics.

Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

This all looks good to me, minus a confusing bit about well-known symbols liveness.

Given we increased the usage of the word "referent", I'm wondering if we should have a definition somewhere that would be automatically linked to explaining it's any object or symbol value used as a WeakRef or FinalizationRegistry target.

I wish we had a more generic word to use that also applied to WeakMap keys / WeakSet values to avoid this "object/symbol" dance. Unfortunately @gibson042 "language value" future-proofing is only safe to do where the context already restricts these values in the first place.

spec.html Outdated
</li>
</ul>
<emu-note>
The second condition above intends to capture the intuition that an object is live if its identity is observable via non-WeakRef means. An object's identity may be observed by observing a strict equality comparison between objects or observing the object being used as key in a Map.
The second condition above intends to capture the intuition that a referent is live if its identity is observable via non-WeakRef means. A referent's identity may be observed by observing a strict equality comparison or observing the value being used as key in a Map.
Copy link
Member

Choose a reason for hiding this comment

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

Can we move or duplicate the editor's note about well-known symbol under this section.

It's not immediately clear why well-known symbols are not covered by the second bullet point. While some well-known symbols are syntactically undeniable, technically an execution where the deniable well-known symbols were removed from the Symbol global and where no new realm could be created wouldn't allow their observation.

spec.html Outdated Show resolved Hide resolved
@mhofman
Copy link
Member

mhofman commented Jun 7, 2022

To capture @erights's comment during the presentation, we would like to have well-known symbols be normatively described as an invariant along the lines of:

The string properties of the global %Symbol% that have a value with a symbol type.

Let me know if you want me to open a dedicated issue for this.

@acutmore
Copy link
Contributor Author

acutmore commented Jun 8, 2022

To capture @erights's comment during the presentation, we would like to have well-known symbols be normatively described as an invariant along the lines of:

The string properties of the global %Symbol% that have a value with a symbol type.

Let me know if you want me to open a dedicated issue for this.

Opening an issue on the ecma262 repo sounds like the best place to capture that.

spec.html Outdated
1. Return *false*.
</emu-alg>
<emu-note type="editor">
<p>Symbols in the GlobalSymbolRegistry (<emu-xref href="#sec-symbol.for"></emu-xref>) are not considered to have a suitable identity because they are permanently live. Well-known symbols (<emu-xref href="#sec-well-known-symbols"></emu-xref>) are likely to never be collected, but are nonetheless treated as suitable for weak reference because they are limited to elements of a fixed size list and therefore manageable by a variety of implementation approaches. However, any values associated to a well-known symbol in a live WeakMap is unlikely to be collected and could &ldquo;leak&rdquo; memory resources in many implementations.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry to keep harping on this, but I think it's important. What sense of "permanently live" applies to symbols in the GlobalSymbolRegistry but not also to well-known symbols? Members of the former set can technically be collected if they are disconnected and any means of creating a new realm is removed, and so can most (but not all!) members of the latter set—and it's the aspect relating to new realms that is subtle and therefore motivating special mention here.

Suggested change
<p>Symbols in the GlobalSymbolRegistry (<emu-xref href="#sec-symbol.for"></emu-xref>) are not considered to have a suitable identity because they are permanently live. Well-known symbols (<emu-xref href="#sec-well-known-symbols"></emu-xref>) are likely to never be collected, but are nonetheless treated as suitable for weak reference because they are limited to elements of a fixed size list and therefore manageable by a variety of implementation approaches. However, any values associated to a well-known symbol in a live WeakMap is unlikely to be collected and could &ldquo;leak&rdquo; memory resources in many implementations.</p>
<p>Symbols in the GlobalSymbolRegistry (<emu-xref href="#sec-symbol.for"></emu-xref>) are not considered to have a suitable identity because they are effectively permanently live (in the sense that their identity can still be observed even after no references to them remain). Well-known symbols (<emu-xref href="#sec-well-known-symbols"></emu-xref>) are also effectively permanently live, but are nonetheless treated as suitable for weak reference because they are limited to elements of a fixed size list and therefore manageable by a variety of implementation approaches. However, any values associated to a well-known symbol in a live WeakMap is unlikely to be collected and could &ldquo;leak&rdquo; memory resources in many implementations.</p>

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 sorry to keep harping on this, but I think it's important. What sense of "permanently live" applies to symbols in the GlobalSymbolRegistry but not also to well-known symbols

No worries! It's good to get this text right. I think a key thing the text is not capturing is that registered-symbols are designed so that engines are free to implement the GlobalSymbolRegistry as a weakMap.

// no other code has run
let s = Symbol.for('hello');
s = null;
// engines can now collect s from the table too, as nothing would observe that happening
s = Symbol.for('hello'); // this can actually be a fresh new symbol, no one would know

(This same optimisation technically applies to the well-known ones, but is less appetising to implement as there are a fixed number of them. Rather than one for each possible string value.)

The description of a WeakMap says that it doesn't extend the lifetime of it's key. But that wouldn't be true for registered symbols where the engine uses a weak GlobalSymbolTable. If Userland tries to hold it weakly, the engine can't also invisibly do it. So instead whenever Userland tries to hold it weakly the engine must ensure it is actually held strongly. That is the core important detail of why registered symbols are not allowed, but well-known ones are. That gives engines the most flexibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but those are implementation choices rather than normative specification constraints. From the perspective of ECMAScript code running in a conforming implementation, well-known and registered symbols have equivalent special behavior with respect to resurrection, and this proposal makes that special behavior relevant in a way that it was not previously (as the last paragraph of your comment describes).

Copy link
Member

Choose a reason for hiding this comment

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

Let's consider the following hypothetical execution:

  • Create a registered symbol with Symbol.for
  • Remove global Symbol and ShadowRealm (and other realm creating host APIs), and drop all existing reference to them and Symbol.for (if any).
  • Create a WeakRef targeting earlier registered symbol (assuming it wasn't prevented by the spec) and drop reference to the symbol.

At this point the WeakRef would be allowed by the spec to be cleared because there is no WeakRef-oblivious execution where the identity of the symbol can be observed.

However, the important part here is that the GlobalSymbolRegistry is reified as Symbol.for() which I think makes it more tangible to programs and appears as a clear way of "resuscitating" symbols, where well-known symbols being plain properties feels less coordinated. While, both are subject to leak if used as WeakMap key, and technically both could be eligible for collection (minus some syntax undeniable well-known symbols), in my mind it's ok for only registered symbols to be explicitly prevented from usage a WMK because of this difference in tangibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not talking about whether it is acceptable for registered vs. well-known symbols to be supported as weakly-held entities, I'm just asking for explicit acknowledgement in the spec text that both have the same reference-independent liveness behavior that requires special care with respect to such support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I can try and capture this correctly, when you say "requires special care" would this note be aimed at engine implementers or end users?

My guess is end users? So for registered symbols code, not being allowed to be held weakly, code would need to either prevent trying or handle the throw exception. And for well-known symbols, while they are allowed, this carries risk.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say both. Implementers must be aware of the special behavior in order to conform with the specification, and practitioners should be aware of it before using well-known symbols in weak contexts (particularly as WeakMap keys).

@acutmore acutmore changed the title [Stage 2 Draft] - Symbols as WeakMap keys [Stage 3 Draft] - Symbols as WeakMap keys Jun 10, 2022
@ptomato
Copy link
Contributor

ptomato commented Jan 3, 2023

This proposal has test262 tests now.

@ljharb ljharb added the has stage 4 This PR represents a proposal that has achieved stage 4, and is ready to merge. label Feb 6, 2023
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
@michaelficarra michaelficarra removed the editor call to be discussed in the next editor call label Feb 8, 2023
jmdyck
jmdyck previously requested changes Feb 12, 2023
Copy link
Collaborator

@jmdyck jmdyck left a comment

Choose a reason for hiding this comment

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

Looks okay to me otherwise.

spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
@acutmore acutmore requested a review from jmdyck February 13, 2023 18:15
@jmdyck
Copy link
Collaborator

jmdyck commented Feb 25, 2023

@acutmore: This PR has merge conflicts (from PR #2947, it looks like). If you resolve those, I can take another look.

Copy link
Collaborator

@jmdyck jmdyck left a comment

Choose a reason for hiding this comment

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

Looks okay to me.

spec.html Show resolved Hide resolved
Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

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

I've opened acutmore#3 with some changes we'll discuss in the 262 editor call tomorrow, but otherwise LGTM.

@michaelficarra michaelficarra added editor call to be discussed in the next editor call and removed editor call to be discussed in the next editor call labels Feb 28, 2023
syg added a commit to syg/ecma262 that referenced this pull request Mar 9, 2023
This 2-level stratification of identity is needed rigor for Symbols as
WeakMap keys (tc39#2777) to capture the reason for why Symbol.for symbols
cannot be used as weak targets.
@syg
Copy link
Contributor

syg commented Mar 9, 2023

@michaelficarra and I have chatted about our disagreement of the use of "identity" for the concept of "can be used as a weak target", and we're agreed on stratifying the concept of identity into "specification identity" and "language identity", with "language identity" being the concept that this proposal should use.

This PR should block on #3027 which does the stratification.

syg added a commit to syg/ecma262 that referenced this pull request Mar 15, 2023
This 2-level stratification of identity is needed rigor for Symbols as
WeakMap keys (tc39#2777) to capture the reason for why Symbol.for symbols
cannot be used as weak targets.
ljharb pushed a commit to syg/ecma262 that referenced this pull request Mar 16, 2023
This 2-level stratification of identity is needed rigor for Symbols as
WeakMap keys (tc39#2777) to capture the reason for why Symbol.for symbols
cannot be used as weak targets.
ljharb pushed a commit to syg/ecma262 that referenced this pull request Mar 17, 2023
This 2-level stratification of identity is needed rigor for Symbols as
WeakMap keys (tc39#2777) to capture the reason for why Symbol.for symbols
cannot be used as weak targets.
@bakkot bakkot added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Mar 18, 2023
- Proposal: https://github.com/tc39/proposal-symbols-as-weakmap-keys
- Also allows Symbols in WeakSet, WeakRef, and FinalizationRegistry
- Adds new AOs `CanBeHeldWeakly`, `KeyForSymbol`
- Registered Symbols can not be held weakly

Closes tc39#1194

Co-authored-by: Ashley Claymore <aclaymore@bloomberg.net>
Co-authored-by: Daniel Ehrenberg <dehrenberg@bloomberg.net>
Co-authored-by: Leo Balter <leonardo.balter@gmail.com>
Co-authored-by: Mathieu Hofman <86499+mhofman@users.noreply.github.com>
Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
Co-authored-by: Shu-yu Guo <syg@chromium.org>
Co-authored-by: Michael Dyck <jmdyck@ibiblio.org>
Co-authored-by: Michael Ficarra <mficarra@shapesecurity.com>
@ljharb ljharb changed the title [Stage 4] - Symbols as WeakMap keys Normative: Permit Symbols as WeakMap keys Mar 18, 2023
@ljharb ljharb dismissed jmdyck’s stale review March 18, 2023 04:55

changes addressed

@ljharb ljharb merged commit 6e70bb6 into tc39:main Mar 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has stage 4 This PR represents a proposal that has achieved stage 4, and is ready to merge. has test262 tests normative change Affects behavior required to correctly evaluate some ECMAScript source text proposal This is related to a specific proposal, and will be closed/merged when the proposal reaches stage 4. ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Symbols can't be used as WeakMap keys