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

Editorial: Change many "abrupt completion" to "throw completion" #2676

Merged
merged 1 commit into from Apr 21, 2022

Conversation

jmdyck
Copy link
Collaborator

@jmdyck jmdyck commented Mar 1, 2022

This is a follow-up to PR #2547, which tagged many operations as returning a normal completion or an abrupt completion. But in most cases, the only abrupt completion possible is a throw completion. So this PR changes lots of "abrupt completion" to the more precise "throw completion".

The first two commits deal with internal methods and host hooks, which the spec explicitly constrains to return normal+throw completions. The third commit deals with ~200 other operations, which I found via static analysis.

Of the ~50 remaining operations that still say "abrupt completion", I think quite a few should likewise change to "throw completion", but it would take more sophisticated analysis to be sure.

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

bakkot commented Mar 2, 2022

LGTM other than comments. It's always a bit surprising to me that PerformEval can't break or return, but that's how it works.

Of the remaining abrupt-completion-returning AOs, I believe only

  • NamedEvaluation
  • EvaluateBody
  • OrdinaryCallEvaluateBody
  • LoopEvaluation
  • DoWhileLoopEvaluation
  • WhileLoopEvaluation
  • ForLoopEvaluation
  • ForBodyEvaluation
  • ForInOfLoopEvaluation
  • ForIn/OfBodyEvaluation
  • CaseBlockEvaluation
  • LabelledEvaluation
  • CatchClauseEvaluation
  • EvaluateFunctionBody

(i.e. the various "Evaluation" AOs used for statements) can actually can produce non-throw abrupt completions. If you don't want to include the remaining cases in this PR I can do it in a followup.

Gonna be fun to revert most of this if do expressions ever advances.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Mar 2, 2022

Of the remaining abrupt-completion-returning AOs, I believe only [list] (i.e. the various "Evaluation" AOs used for statements) can actually can produce non-throw abrupt completions.

I agree with all of those except NamedEvaluation. It looks like its only sources of abrupt completions are recursive calls to NamedEvaluation, and an invocation of ClassDefinitionEvaluation, which can only throw, right?

But there are also a few missing from your list:

  • ForIn/OfHeadEvaluation (explicit ~break~)
  • Evaluate{Generator,AsyncFunction,AsyncConcise,AsyncGenerator}Body (explicit ~return~)
  • UpdateEmpty (accepts all kinds of abrupt completions and returns a completion of the same [[Type]])
  • Yield (via AsyncGeneratorYield) [which seems odd: if a function executes a YieldExpression, that could cause the function to return?]

And maybe some others. E.g. G.p.return passes a return completion to GeneratorResumeAbrupt, and I haven't tracked down all the places that it can reach.

If you don't want to include the remaining cases in this PR I can do it in a followup.

Yeah, I might opt for that, as this PR is playing it safe.

Gonna be fun to revert most of this if do expressions ever advances.

To be clear, you mean most of the "extras", not most of what this PR proposes. (Offhand, I don't think anything in the PR would be affected by do-expressions.)

@bakkot
Copy link
Contributor

bakkot commented Mar 2, 2022

Oh geeze, I forgot yield could stick return completions in the middle of expressions. I'll need to review again for that.

spec.html Outdated
@@ -20788,7 +20788,7 @@ <h1>
Runtime Semantics: RestDestructuringAssignmentEvaluation (
_value_: unknown,
_excludedNames_: unknown,
): either a normal completion containing ~unused~ or an abrupt completion
): either a normal completion containing ~unused~ or a throw completion
Copy link
Contributor

@bakkot bakkot Mar 4, 2022

Choose a reason for hiding this comment

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

This can produce a return completion, as in

let g = (function* (){ ({ ...{}[yield] } = {}) })();
g.next();
g.return(42);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed. (I was avoiding "Evaluation" ops on principle, regardless of what my analysis said, but that one slipped through.)

@bakkot
Copy link
Contributor

bakkot commented Mar 4, 2022

I agree with all of those except NamedEvaluation. It looks like its only sources of abrupt completions are recursive calls to NamedEvaluation, and an invocation of ClassDefinitionEvaluation, which can only throw, right?

Alas, ClassDefinitionEvaluation can also produce return completions, because of generators:

let g = (function* (){ var x = class { [yield 1](){} }; })();
g.next();
g.return(42);

But there are also a few missing from your list:

Yup, agreed with all of those.

[which seems odd: if a function executes a YieldExpression, that could cause the function to return?]

That's how it works, yup. This is used for mainly for cleanup: if you do

function* g() {
  let resource = lock(thing);
  try {
    yield 0;
    yield 1;
    yield 2;
  } finally {
    unlock(thing);
  }
}

then the finally gets executed (via a call to IteratorClose) even if you stop iteration early, as in

for (let x of g()) {
  if (condition) {
    break; // executes the `finally` of `g`, by injecting a return completion at whatever `yield` it's paused on
  }
}

To be clear, you mean most of the "extras", not most of what this PR proposes. (Offhand, I don't think anything in the PR would be affected by do-expressions.)

No, I just didn't initialize realize how few of these were actually evaluating expression.

@bakkot
Copy link
Contributor

bakkot commented Mar 4, 2022

And maybe some others.

Indeed, that list was completely wrong. Reviewing again after remembering how yield works, almost all of the things which (as of this PR) return "an abrupt completion" actually do so; those that don't, by my reading, are

  • InitializeReferencedBinding
  • InitializeBoundName
  • FunctionDeclarationInstantiation (because an early error prevents use of yield in parameter lists)
  • RestBindingInitialization (because the only form allowed is ...x for an identifier x)
  • EvaluateClassStaticBlockBody (because an early error prevents use of yield)

There's also GetValue and PutValue, which are gross in that they take a completion record as an argument and immediately return it if it's abrupt. If they were refactored to not do that, they'd also not be able to return non-throw abrupt completions.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Mar 10, 2022

(force-pushed to resolve merge conflicts and squash 2 fixups)

@bakkot
Copy link
Contributor

bakkot commented Apr 20, 2022

Needs a rebase but otherwise is ready to land.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Apr 21, 2022

(force-pushed to resolve merge conflict from 2719)

jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Apr 21, 2022
…c39#2676)

This is a follow-up to PR tc39#2547, which tagged many operations
as returning a normal completion or an abrupt completion.
But in most cases, the only abrupt completion possible is a throw completion.
So this PR changes lots of "abrupt completion" to the more precise "throw completion".
@jmdyck
Copy link
Collaborator Author

jmdyck commented Apr 21, 2022

(force-pushed again, to squash to a single commit)

@bakkot bakkot added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Apr 21, 2022
…9#2676)

This is a follow-up to PR tc39#2547, which tagged many operations
as returning a normal completion or an abrupt completion.
But in most cases, the only abrupt completion possible is a throw completion.
So this PR changes lots of "abrupt completion" to the more precise "throw completion".
@ljharb ljharb merged commit ad096b3 into tc39:main Apr 21, 2022
@jmdyck jmdyck deleted the abrupt_to_throw branch April 21, 2022 21:21
@jmdyck jmdyck mentioned this pull request May 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial change 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.

None yet

3 participants