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

Add using and await using Declarations, SuppressedError, DisposableStack, and AsyncDisposableStack #3000

Draft
wants to merge 57 commits into
base: main
Choose a base branch
from

Conversation

rbuckton
Copy link
Contributor

@rbuckton rbuckton commented Jan 23, 2023

This PR contains the Stage 3 specification text for Explicit Resource Management.

This version of the specification text includes several changes from the version at https://tc39.es/proposal-explicit-resource-management:

NOTE: The three PRs above will be discussed in the January, 2023 plenary. I've already made the edits here as I believe they will be non-controversial.

@rbuckton rbuckton force-pushed the explicit-resource-management branch from b7b82d5 to 17e0bc0 Compare January 23, 2023 23:33
@rbuckton
Copy link
Contributor Author

I'm not quite sure how to address the remaining esmeta issues.

@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 Jan 24, 2023
@jmdyck
Copy link
Collaborator

jmdyck commented Jan 24, 2023

Rather than suggest changes here, I've made a PR against this PR's branch: rbuckton#2

@rbuckton
Copy link
Contributor Author

This last commit adds a few UpdateEmpty calls that aren't strictly necessary, but esmeta doesn't quite understand what's happening without it. I also had to switch AO parameters typed : a function object to : an ECMAScript language value since IsCallable doesn't seem to act as a type guard.

@ljharb ljharb marked this pull request as draft January 25, 2023 22:30
@bakkot
Copy link
Contributor

bakkot commented Jan 25, 2023

@rbuckton Sorry about that - esmeta is still a little imprecise. You should feel free to write the spec text you want and not worry about esmeta's errors, and we can update esmeta or the ignore file.

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

@rbuckton Sorry about that - esmeta is still a little imprecise. You should feel free to write the spec text you want and not worry about esmeta's errors, and we can update esmeta or the ignore file.

I replaced the UpdateEmpty calls with a few asserts that seem to be accurate. For example, in ForIn/OfBodyEvaluation, if status is abrupt going in to DisposeResources, it should still be abrupt on the way out. Likewise, ForBodyEvaluation won't return a normal completion for empty, so DisposeResources shouldn't produce one either.

@rbuckton
Copy link
Contributor Author

Please note, I've created rbuckton#3 as a separate PR against this one containing the additional specification changes that would be necessary to support https://github.com/tc39/proposal-async-explicit-resource-management, should it advance to Stage 3.

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

I found an issue with the change in e4f2d05 to use an EE to restrict binding patterns, in that it results in an ambiguous parse of using[x] before the EE can be applied, which was not intended.

I've filed tc39/proposal-explicit-resource-management#152 against the proposal spec, and will update this PR in due time.

@rbuckton
Copy link
Contributor Author

I found an issue with the change in e4f2d05 to use an EE to restrict binding patterns, in that it results in an ambiguous parse of using[x] before the EE can be applied, which was not intended.

I've filed tc39/proposal-explicit-resource-management#152 against the proposal spec, and will update this PR in due time.

tc39/proposal-explicit-resource-management#152 has merged, and I've amended this PR to match as part of 3044fd9.

@rbalicki2

This comment was marked as duplicate.

spec.html Outdated
It is a Syntax Error if the BoundNames of |BindingList| contains any duplicate entries.
</li>
<li>
It is a Syntax Error if the goal symbol is |Script| and |UsingDeclaration| is not contained, either directly or indirectly, within a |Block|, |CaseBlock|, |ForStatement|, |ForInOfStatement|, |FunctionBody|, |GeneratorBody|, |AsyncGeneratorBody|, |AsyncFunctionBody|, |ClassStaticBlockBody|, or |ClassBody|.
Copy link
Contributor

Choose a reason for hiding this comment

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

@rbuckton What is an example of a using contained within a ClassBody? It can't be directly contained within one, and if indirectly contained, then it would already be directly contained within another production on the list (like ClassStaticBlockBody, FunctionBody, etc). During implementation we found the inclusion of ClassBody in this list confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct, ClassBody here is superfluous and should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rbuckton
Copy link
Contributor Author

This now includes the await using declaration and AsyncDisposableStack, per prior consensus (incl. the March 2023 plenary).

@rbuckton
Copy link
Contributor Author

Note to implementers: There is a typo in GetDisposeMethod that will be addressed by tc39/proposal-explicit-resource-management#217. The typo was introduced as part of a normative change that reached consensus in July 2023, but fixing it is also a potentially a normative change and may require additional consensus.

@rbuckton
Copy link
Contributor Author

This is also missing the consensus change to use await using as the syntactic head for the asynchronous form of the using declaration. That will be up shortly.

@rbuckton rbuckton force-pushed the explicit-resource-management branch from d9d189a to 4914a91 Compare May 23, 2024 23:59
@rbuckton
Copy link
Contributor Author

Rebased (twice, to make GitHub happy). Hopefully that fixes the PR in the diff review website.

@debadree25
Copy link

Hey! now its significantly easier to read from https://arai-a.github.io/ecma262-compare/?pr=3000 thank you so much!!

@jmdyck
Copy link
Collaborator

jmdyck commented May 27, 2024

More editorial suggestions at rbuckton#11

rbuckton added 2 commits June 15, 2024 15:00
…nullish

Reduce unnecessary Awaits for nullish values in blocks containing `await using`
rbuckton added a commit to tc39/proposal-explicit-resource-management that referenced this pull request Jun 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants