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

Better iterator close #3075

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

Better iterator close #3075

wants to merge 4 commits into from

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented May 25, 2023

EDIT: this is slightly wrong and will need to be tweaked. Don't review yet.

Fixes #3072.

Open to other names for the new AOs. IteratorCloseSuppressingExceptions would also work.

I didn't add an async version since we only touch async iterators in a couple places.

"abrupt completion" isn't synonymous with "throw completion", so the use in IfAbruptCloseIterator can't be replaced as currently written. As it happens, all of the callsites of IfAbruptCloseIterator currently in the spec can only pass it throw completions or normal completions, so we could change it to "IfThrowCloseIterator" and then change the implementation. But the iterator helpers proposal introduces some uses of IfAbruptCloseIterator which can get passed return completions, so we'd need to do something else there. I'm inclined to leave it alone.

I went a little further than #3072 suggests, in the third commit, which I think ends up in a cleaner place. I'm fine leaving it at the second commit though.

Note that this will require changes in 402, Temporal, and Iterator Helpers (if we include the third commit), which I'll do after we land this.

spec.html Show resolved Hide resolved
1. If _innerResult_.[[Type]] is ~throw~, return ? _innerResult_.
1. Let _return_ be ? GetMethod(_iterator_, *"return"*).
1. If _return_ is *undefined*, return ~unused~.
1. Let _innerResult_ be ? Call(_return_, _iterator_).
1. If _innerResult_.[[Value]] is not an Object, throw a *TypeError* exception.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Delete the .[[Value]] here, because the previous line unwrapped it.

@jmdyck
Copy link
Collaborator

jmdyck commented May 26, 2023

I don't think you need IteratorCloseForThrow. Anywhere that says
Perform IteratorCloseForThrow(...),
you can just say
Perform IteratorClose(...)
with the same semantics.

1. Return _result_.
</emu-alg>
<emu-grammar>ArrayAssignmentPattern : `[` AssignmentElementList `,` Elision? AssignmentRestElement? `]`</emu-grammar>
<emu-alg>
1. Let _iteratorRecord_ be ? GetIterator(_value_, ~sync~).
1. Let _status_ be Completion(IteratorDestructuringAssignmentEvaluation of |AssignmentElementList| with argument _iteratorRecord_).
1. If _status_ is an abrupt completion, then
1. If _iteratorRecord_.[[Done]] is *false*, return ? IteratorClose(_iteratorRecord_, _status_).
1. If _iteratorRecord_.[[Done]] is *false*, perform IteratorCloseForThrow(_iteratorRecord_).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is _status_ necessarily a throw completion here?

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: IteratorDestructuringAssignmentEvaluation can only return a throw completion or a normal completion, so guarding on abruptness of its result suffices.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes: IteratorDestructuringAssignmentEvaluation can only return a throw completion or a normal completion,

So we should change its declared return-type?

@@ -22028,7 +22058,8 @@ <h1>
1. Return ? _status_.
1. Else,
1. Assert: _iterationKind_ is ~iterate~.
1. Return ? IteratorClose(_iteratorRecord_, _status_).
1. Perform IteratorCloseForThrow(_iteratorRecord_).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is _status_ necessarily a throw completion here?

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: _status_ here can only arise from DestructuringAssignmentEvaluation, BindingInitialization, Evaluation of _lhs_ (which is either a LeftHandSideExpression, a ForBinding, a ForDeclaration, or a ForBinding; for each of these Evaluation to throw or normal completions), PutValue, ForDeclarationBindingInitialization, or InitializeReferencedBinding. And those all can only return a throw completion or a normal completion, so guarding on abruptness of `status suffices.

Copy link
Collaborator

Choose a reason for hiding this comment

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

DestructuringAssignmentEvaluation, BindingInitialization, PutValue, ForDeclarationBindingInitialization, and InitializeReferencedBinding are all declared to return normal or abrupt. Can we change abrupt to throw for them?

And for the Evaluation of _lhs_, maybe when _lhsRef_ is abrupt, there could be an Assert that it's a throw completion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(This reminded me of #2676 and #2676 (comment) in particular.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah jeez, you're right, I always forget about yield. These are wrong.

@bakkot
Copy link
Contributor Author

bakkot commented May 26, 2023

I don't think you need IteratorCloseForThrow. Anywhere that says Perform IteratorCloseForThrow(...), you can just say Perform IteratorClose(...) with the same semantics.

By convention (enforced by ecmarkup) you can't call any completion-returning AO without either explicitly mentioning the fact that it returns a completion record (by wrapping with Completion) or unwrapping with either ? or !.

We could say Perform Completion(IteratorClose(...)), of course. But that's less clear than an explicit AO, I think.

@jmdyck
Copy link
Collaborator

jmdyck commented May 26, 2023

By convention (enforced by ecmarkup) you can't call any completion-returning AO without either explicitly mentioning the fact that it returns a completion record (by wrapping with Completion) or unwrapping with either ? or !.

Okay.

We could say Perform Completion(IteratorClose(...)), of course. But that's less clear than an explicit AO, I think.

I think I'd prefer Perform Completion(IteratorClose(...)) over another AO.

@bakkot bakkot marked this pull request as draft May 26, 2023 07:42
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.

Clarify IteratorClose
2 participants