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: Add Weakrefs and FinalizationRegistry #2089

Merged
merged 1 commit into from
Aug 5, 2020

Conversation

codehag
Copy link
Contributor

@codehag codehag commented Jul 10, 2020

Specification text for https://github.com/tc39/proposal-weakrefs

This version of the specification is without cleanupSome (see https://github.com/codehag/proposal-cleanup-some), in anticipation of the discussion of the upcoming meeting. Adding this back in would be trivial if we decide against it.

Note: we're adding inline normative optional spec text.

@codehag codehag added normative change Affects behavior required to correctly evaluate some ECMAScript source text 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 Jul 10, 2020
<h1>HostCleanupFinalizationRegistry(_finalizationRegistry_)</h1>

<p>
HostCleanupFinalizationRegistry is an implementation-defined abstract
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@syg This might be important for the incumbent realm tracking.

Copy link
Contributor

Choose a reason for hiding this comment

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

For incumbents, see #2086 and whatwg/html#5722

As both an editor and a delegate, given the reality with Promises, I think it'd be fine to fix up the callbacks in this PR to use JobCallback after #2086 is merged, even for Stage 4. There is no need to gate on that.

@codehag codehag force-pushed the weakrefs-sans-cleanupsome branch from 00ce5e9 to 53fe666 Compare July 10, 2020 08:02
@codehag codehag requested review from a team, syg, ljharb, michaelficarra and bakkot July 10, 2020 08:06
@codehag codehag force-pushed the weakrefs-sans-cleanupsome branch from 53fe666 to 78fbc63 Compare July 10, 2020 08:42
@codehag
Copy link
Contributor Author

codehag commented Jul 10, 2020

I am not sure what the linter wants from me, any recommendations?

@codehag codehag force-pushed the weakrefs-sans-cleanupsome branch from 78fbc63 to e7bf912 Compare July 10, 2020 09:02
@jmdyck
Copy link
Collaborator

jmdyck commented Jul 10, 2020

I am not sure what the linter wants from me, any recommendations?

There should be a space on either side of each square bracket in that heading, I believe.

@codehag codehag force-pushed the weakrefs-sans-cleanupsome branch from e7bf912 to e6fd679 Compare July 10, 2020 17:28
@bakkot
Copy link
Contributor

bakkot commented Jul 10, 2020

Ugh, my apologies for the terrible error messages from ecmarkup. I'll work on that.

I think the error you're seeing in the most recent commit is because of the inclusion of blank lines within <p>s and an <emu-note>. That shouldn't be a hard error (just a lint failure), but the parser is pretty rigid right now.

@codehag codehag force-pushed the weakrefs-sans-cleanupsome branch from e6fd679 to 7600747 Compare July 10, 2020 18:07
@codehag
Copy link
Contributor Author

codehag commented Jul 10, 2020

I think I fixed it, should be good now

Edit: narrator: she forgot to commit the patch

@codehag codehag force-pushed the weakrefs-sans-cleanupsome branch from 7600747 to a65af73 Compare July 10, 2020 18:37
@ljharb
Copy link
Member

ljharb commented Jul 10, 2020

Tests: tc39/test262#2192

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.

I started to do the suggestions, but it seems like every paragraph is hard-wrapped; it's probably easier to halt my review until those are updated :-)

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
1. Set _ref_.[[WeakRefTarget]] to ~empty~.
1. For each FinalizationRegistry _fg_ such that _fg_.[[Cells]] contains _cell_, such that _cell_.[[WeakRefTarget]] is _obj_, do
1. Set _cell_.[[WeakRefTarget]] to ~empty~.
1. Optionally, perform ! HostCleanupFinalizationRegistry(_fg_).
Copy link
Member

Choose a reason for hiding this comment

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

The implementation of HostCleanupFinalizationRegistry is already free to not clean things up so I don't think this needs to be optional.

Suggested change
1. Optionally, perform ! HostCleanupFinalizationRegistry(_fg_).
1. Perform ! HostCleanupFinalizationRegistry(_fg_).

Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree with this suggestion. While it is true that the host hook reserves the right to not do anything, calling out the optionality means hosts don't need to remember to explicitly allow optionality.

Copy link
Member

Choose a reason for hiding this comment

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

Why don't we just explicitly call out the optionality in the hook then? Isn't the entire point of host hooks to abstract this kind of thing?

Copy link
Member

Choose a reason for hiding this comment

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

It's important to call out optionality here to avoid a misunderstanding that it would be mandatory to trigger FinalizationRegistry. The word "optionally" makes the intended interpretation unambiguous, and it does not sacrifice precision/correctness. Remember that specifications are communication devices, not just mathematical artifacts. Because @devsnek's suggestion is an editorial one, we can continue considering it after this PR lands and possibly make the change later.

Copy link
Contributor

Choose a reason for hiding this comment

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

As an editor I also prefer the "Optionally" phrasing.

</p>

<p>
Because calling HostCleanupFinalizationRegistry is optional, registered objects
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Because calling HostCleanupFinalizationRegistry is optional, registered objects
Because calling CleanupFinalizationRegistry is optional, registered objects

Copy link
Contributor

Choose a reason for hiding this comment

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

I also disagree with this suggestion for the same reason as above.

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 Show resolved Hide resolved
1. Set _ref_.[[WeakRefTarget]] to ~empty~.
1. For each FinalizationRegistry _fg_ such that _fg_.[[Cells]] contains _cell_, such that _cell_.[[WeakRefTarget]] is _obj_, do
1. Set _cell_.[[WeakRefTarget]] to ~empty~.
1. Optionally, perform ! HostCleanupFinalizationRegistry(_fg_).
Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree with this suggestion. While it is true that the host hook reserves the right to not do anything, calling out the optionality means hosts don't need to remember to explicitly allow optionality.

</p>

<p>
Because calling HostCleanupFinalizationRegistry is optional, registered objects
Copy link
Contributor

Choose a reason for hiding this comment

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

I also disagree with this suggestion for the same reason as above.

spec.html Show resolved Hide resolved
spec.html 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 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
@codehag codehag force-pushed the weakrefs-sans-cleanupsome branch from 63bb481 to 913646b Compare July 13, 2020 13:14
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.

LGTM pending comments

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

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

Looking very good already, mostly small wording changes now.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Show resolved Hide resolved
@codehag codehag force-pushed the weakrefs-sans-cleanupsome branch from 913646b to fcead87 Compare July 14, 2020 11:57
@codehag
Copy link
Contributor Author

codehag commented Jul 14, 2020

Branch has also been rebased

@ljharb ljharb added has stage 4 This PR represents a proposal that has achieved stage 4, and is ready to merge. and removed pending stage 4 This proposal has not yet achieved stage 4, but may otherwise be ready to merge. labels Jul 21, 2020
Copy link
Member

@littledan littledan left a comment

Choose a reason for hiding this comment

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

This recent commit seems to fix the Realm-specificity bug.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
Colloquially, we say that an individual object is live if every set of objects containing it is live.
</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>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is "live" sufficiently specific to serve as a linkable definition id? It's not used elsewhere now, but I could see it cropping up in prose. Perhaps something like "liveness" (which is already used) would be better?

Copy link
Member

@littledan littledan Jul 27, 2020

Choose a reason for hiding this comment

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

I think it's reasonable to define "live" this way within the JavaScript specification. I don't think "liveness" is really any more specific. This proposal successfully links with the term "live" where relevant in normative text ("liveness" is just used in notes) and the rest of ecma262 does not seem to use "live" in a conflicting way.

Copy link
Contributor

Choose a reason for hiding this comment

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

"live" is also the spelling of a common English word (ranked somewhere between 100 and 300 AFAICT) that could easily find a place in the spec, as opposed to "liveness" which will only ever have this meaning.

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be fine to iterate on this in an editorial PR after landing this one.


<emu-clause id="sec-clear-kept-objects" aoid=ClearKeptObjects>
<h1>ClearKeptObjects ( )</h1>
<p>The abstract operation ClearKeptObjects takes no arguments. ECMAScript implementations are expected to call ClearKeptObjects when a synchronous sequence of ECMAScript executions completes. It performs the following steps when called:</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the plausible future addition of Symbols, it would be nice to give these operations initial names that do not directly reference the Object type. "KeptObject" could be replaced with something like "KeptAlive" or "KeptList".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A similar comment was brought up above, and it was agreed that we can change this after the Symbols in WeakMap proposal moved forward. I would rather do this is a group change, as there will be other edits, rather than anticipating an unrelated proposal in this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Renaming operations requires introducing oldids, which is a bit of an ugly hack that shouldn't be necessary if the initial names do not include "Object". I'm not suggesting that you make the definitions generic right now, just that the names theirselves are not overly specific.

Copy link
Member

Choose a reason for hiding this comment

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

As the champion of the Symbols as WeakMap keys proposal, I would prefer to not generalize this prematurely. oldids are fine.

@devsnek
Copy link
Member

devsnek commented Jul 27, 2020

I think these need to be added to the intrinsics table and to the "constructor properties of the global object" section.

@codehag
Copy link
Contributor Author

codehag commented Aug 3, 2020

I think I still need one more editor to approve this, so pinging @bakkot and @michaelficarra for one last look before I rebase and squash

@ljharb ljharb self-assigned this Aug 3, 2020
@ljharb ljharb requested review from bakkot and a team August 3, 2020 16:28
spec.html Outdated
<emu-clause id="sec-weakref-invariants">
<h1>Objectives</h1>

<p> This specification does not make any guarantees that any object will be garbage collected. Objects which are not live may be released after long periods of time, or never at all. For this reason, this specification uses the term "may" when describing behaviour triggered by garbage collection.</p>
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
<p> This specification does not make any guarantees that any object will be garbage collected. Objects which are not live may be released after long periods of time, or never at all. For this reason, this specification uses the term "may" when describing behaviour triggered by garbage collection.</p>
<p>This specification does not make any guarantees that any object will be garbage collected. Objects which are not live may be released after long periods of time, or never at all. For this reason, this specification uses the term "may" when describing behaviour triggered by garbage collection.</p>

Copy link
Contributor

@bakkot bakkot left a comment

Choose a reason for hiding this comment

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

LGTM other than the stray space above.

@bakkot
Copy link
Contributor

bakkot commented Aug 5, 2020

@codehag We've been letting @ljharb handle the merging process because he has idiosyncratic preferences for how that should happen.

@ljharb ljharb force-pushed the weakrefs-sans-cleanupsome branch from fe67804 to 71c0897 Compare August 5, 2020 21:34
@ljharb ljharb merged commit 71c0897 into tc39:master Aug 5, 2020
ljharb added a commit to ljharb/ecma262 that referenced this pull request Aug 5, 2020
…table (tc39#2126)

Unintentionally omitted from tc39#2089.

Also added the <dfn>s for easier linking.
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Aug 9, 2020
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Aug 11, 2020
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Aug 11, 2020
- Put 2 emu-clause attributes in quotes
  (We always quote element attributes -- see PR tc39#1868)

- Use title-case in clause heading

- Tweak some algorithm syntax

- Change "a finalization registry object" to "a FinalizationRegistry"
  (The former term isn't defined.)
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Aug 11, 2020
- Use title-case in clause heading

- Tweak some algorithm syntax

- Put 2 emu-clause attributes in quotes
  (We always quote element attributes -- see PR tc39#1868)

- Change "a finalization registry object" to "a FinalizationRegistry"
  (The former term isn't defined.)
ljharb pushed a commit to jmdyck/ecma262 that referenced this pull request Aug 11, 2020
ljharb pushed a commit to jmdyck/ecma262 that referenced this pull request Aug 11, 2020
- Use title-case in clause heading

- Tweak some algorithm syntax

- Put 2 emu-clause attributes in quotes
  (We always quote element attributes -- see PR tc39#1868)

- Change "a finalization registry object" to "a FinalizationRegistry"
  (The former term isn't defined.)
h2oche pushed a commit to h2oche/ecma262 that referenced this pull request Aug 16, 2020
h2oche pushed a commit to h2oche/ecma262 that referenced this pull request Aug 16, 2020
- Use title-case in clause heading

- Tweak some algorithm syntax

- Put 2 emu-clause attributes in quotes
  (We always quote element attributes -- see PR tc39#1868)

- Change "a finalization registry object" to "a FinalizationRegistry"
  (The former term isn't defined.)
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants