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
Reduce unnecessary Awaits for nullish values in blocks containing await using
#219
base: main
Are you sure you want to change the base?
Conversation
A preview of this PR can be found at https://tc39.es/proposal-explicit-resource-management/pr/219. |
In example 2, if Did you mean to say that in step 3 there is only an |
I agree, in principle. My concern is this: should
Example 3, steps 2 and 3:
Step 3 performs an Essentially, this ensures that the following code will Await at least once, but up to twice depending on whether {
using A = syncNonNull;
await using B = null, C = null, D = undefined, E = asyncNonNull;
} As we unwind disposal, we do the following:
|
Consensus in plenary was to collapse all Awaits for |
@nicolo-ribaudo, you asked to review the proposed changes here before this could reach consensus. I'd appreciate if you could review these changes as of the most recent commits. The intent behind these changes is as follows:
This has the following effects: Given, {
await using x = null, y = null;
} we record an Await is needed when disposing Given, {
await using x = null, y = asyncDisposable, z = null;
} we record an Await is needed when disposing Given, {
await using x = null, y = syncThrowInAsyncDispose, z = null;
} we record an Await is needed when disposing Given, {
await using x = null, y = null, z = asyncDisposable;
} we record an Await is needed when disposing Given, {
using a = syncDisposable;
await using x = null, y = null;
} we record an Await is needed when disposing Given, {
using a = syncDisposable;
await using x = null, y = asyncDisposable, z = null;
} we record an Await is needed when disposing Given, {
using a = syncDisposable;
await using x = null, y = syncThrowInAsyncDispose, z = null;
} we record an Await is needed when disposing Given, {
using a = syncDisposable;
await using x = null, y = null, z = asyncDisposable;
} we record an Await is needed when disposing |
I'd also appreciate additional reviews from @syg, @waldemarhorwat, @michaelficarra, and @bakkot. |
How can I see a rendered form of this? |
The first comment on this PR contains a link to the rendered spec text. The specific section in question is here: https://tc39.es/proposal-explicit-resource-management/pr/219/#sec-disposeresources |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me.
It's not explicit from the examples, but awaits are also collapsed across subsequent separate await using
declarations right?
await using x = null;
await using y = null;
Keeps being the same as
await using x = null, y = null;
Correct. I called this out:
|
I keep wondering if it wouldn't be simpler to always |
That would not simplify the algorithm, it would make it more complex. I also see no reason to break with how |
This is an alternative to #216 and is based on another fix in #218.
Per #196 (comment), this reduces the number of
Await
s in blocks containingawait using
and inAsyncDisposableStack
by collapsing theAwait
s introduced fornull
orundefined
resources to a singleAwait
.As currently specified, the following code results in three
Await
s when only one is necessary:This was also the case in an
AsyncDisposableStack
containing synchronous disposables:This PR changes this behavior such that we only need an implicit
Await
in the following cases:null
orundefined
async-from-sync resource to an async resource, non-nullish async-from-sync resource, or sync resource3.null
orundeifned
async-from-sync resource is the last disposal performed for a scope.Example 1
C
isundefined
so no disposal happens, but we track that anAwait
must occur. [+0 turns]B
isnull
so no disposal happens, but we track that anAwait
must occur. [+0 turns]A
is neithernull
norundefined
and we have tracked that anAwait
must occur, so weAwait(undefined)
. [+1 turn]A[@@dispose]()
is invoked in the following turn. [+0 turns]HAPPENS_AFTER
happens in the same turn asA[@@dispose]()
, but a turn afterHAPPENS_BEFORE
.Example 2
C
is neithernull
norundefined
so we invoke its@@asyncDispose
method. If it completes normally weAwait
the result [+1 turns], but if it throws we continue on the same turn [+0 turns].B
isnull
so no disposal happens, but we track that anAwait
must occur. [+0 turns]A
is neithernull
norundefined
and we have tracked that anAwait
must occur, so weAwait(undefined)
. [+1 turns]A[@@dispose]()
is invoked in the following turn. [+0 turns]HAPPENS_AFTER
happens in the same turn asA[@@dispose]()
, but between 1-2 turns afterHAPPENS_BEFORE
.Example 3
C
isnull
so no disposal happens, but we track that anAwait
must occur. [+0 turns]B
is neithernull
norundefined
and we have tracked that anAwait
must occur, so weAwait(undefined)
. [+1 turns]B
's@@asyncDispose
method. If it completes normally weAwait
the result [+1 turns], but if it throws we continue on the same turn [+0 turns].A
is neithernull
norundefined
so we invokeA[@@dispose]()
either on the turn afterB
's disposal (if it was normal), or on the same turn (if it threw). [+0 turns]HAPPENS_AFTER
happens in the same turn asA[@@dispose]()
, but between 1-2 turns afterHAPPENS_BEFORE
.It is important to note that a synchronous throw completion from a call to
@@asyncDispose
will result in theAwait
being passed over, but this is consistent withfor await
andAsyncIterator.next()
behavior. Following #218, this is unlikely to occur for a@@dispose
method as it will be automatically wrapped in a newPromiseCapability
, though it is still possible to throw synchronously ifPromise.prototype.then
is patched to throw an exception synchronously.This PR also removes unnecessary
Await
s fornull
/undefined
resources inAsyncDisposableStack.prototype.disposeAsync()
.The PR against ecma262 is being tracked in rbuckton/ecma262#10
Fixes #196
Fixes #208
Footnotes
An async resource is an object with an
@@asyncDispose
method initialized in anawait using
declaration. ↩An sync-from-sync resource is either
null
,undefined
, or an object with a@@dispose
method (but not an@@asyncDispose
method) initialized in anawait using
declaration. ↩A sync resource is either
null
,undefined
, or an object with a@@dispose
method initialized in ausing
declaration. ↩