Editorial: remove useless step of ReturnIfAbrupt expansion

[michaelficarra]

Since "hygienicTemp is ephemeral and visible only in the steps pertaining to ReturnIfAbrupt", there's no reason to set it in the final step.

[michaelficarra]

This whole thread has got me thinking that it'd probably be worth the effort to look for instances of ReturnIfAbrupt that don't pattern match to one of the three cases in 5.2.3.3 or instances of prefix ? or ! that don't pattern match to one of the cases in 5.2.3.4.

[ljharb]

Per discussion, this change looks good. Further refinements can happen in this or a future PR.

[jmdyck]

instances of ReturnIfAbrupt that don't pattern match to one of the three cases in 5.2.3.3

It looks like all occurrences (outside 5.2.3.3 and 5.2.3.4) match

1. ReturnIfAbrupt(_argument_).

(The argument is always an alias/metavariable, never an operation-invocation, and the ReturnIfAbrupt call is never embedded within some other expression.)

Minor quibble: there's one occurrence that technically doesn't match, because it's in a one-line Else:

1. Else, ReturnIfAbrupt(_result_).

but of course it would match if we made it an indented sub-block.

---

or instances of prefix ? or ! that don't pattern match to one of the cases in 5.2.3.4.

For ?, it depends on how strictly you want to interpret the cases: both have ? at the start of the step, which (as stated above) doesn't occur in the spec. However, if you interpret them as talking about those forms occurring anywhere in the step, then it depends on what you think matches OperationName() and _someValue_.OperationName().

E.g., There are ~100 occurrences of the form:

? _X_.[[InternalMethod]](_args_)

So it depends whether you think [[InternalMethod]] counts as OperationName.

---

For !, note that 5.2.3.4 doesn't explicitly say where it's allowed, it just says "Similarly, prefix ! is used..." and then gives one example of its use. Presumably it means that ! is allowed in the same contexts where ? is allowed.

If so, then there are 2 occurrences of the form:

! _X_.[[InternalMethod]](_args_)

So again, it depends.

[jmdyck]

Actually, I'm no longer sure now whether ReturnIfAbrupt pattern matches full or partial algorithm steps. The wording in that section reads to me like only full algorithm steps apply, but I think the usage throughout the spec is consistent with partial algorithm steps, though I'm not sure exactly how that would work.

Yeah, 5.2.3.{3,4} seem a bit too vague for my taste. I'd prefer, for each construct, a single precise definition that covers all possible usages, rather than a few patterns whose coverage is at best questionable. However, I don't think we have the notation that would allow precise definitions, and I'm doubtful that it'd be worth introducing it.

[michaelficarra]

@jmdyck Thanks for doing the initial research! I've confirmed that all of the current usages of ReturnIfAbrupt without the shorthand will match the first definition. And many of them are for syntax-directed operations that could use the shorthand as described at the end of 5.2.3.4. I'm betting the authors just didn't know the shorthand was permitted to be used that way.

In my opinion, the way the shorthand notation is defined right now is unacceptably bad. I'd like to see if we could define rewrite rules for the shorthand notation that operate only on a whole step. Something like

• [words before] ? AbstractOperation([parameters]) [words after]

gets rewritten to

• Let hygienicTemp be AbstractOperation([parameters]).
• If hygienicTemp is an abrupt completion, return hygienicTemp.
• Else if hygienicTemp is a Completion Record, set hygienicTemp to hygienicTemp.[[Value]].
• [words before] hygienicTemp [words after]

@ljharb @bterlson thoughts?

[ljharb]

That sounds great to me - essentially, turning ? AbstractOperation(…) into a do expression :-)

[jmdyck]

I was just about to suggest something like that. Except I was going to suggest it for the expansion of ReturnIfAbrupt, and retain the idea of ? being a simple shorthand for ReturnIfAbrupt. You're suggesting to sever that connection?

[michaelficarra]

Either way works for me.

[ljharb]

Seems nice to keep the connection and add better shorthand for ReturnIfAbrupt.

[michaelficarra]

I opened a PR for better ReturnIfAbrupt definition in #1573.

[michaelficarra]

I think we should merge this without waiting for a resolution on #1573. It's an objective improvement for the time being.

[michaelficarra]

Ping @syg @bakkot for a very quick review.

[bakkot]

I'm a little hesitant to remove this step when so many of the existing usages of ReturnIfAbrupt (via the shorthand, specifically) do not fall into one of the enumerated cases and therefore must currently be understood by trying to figure out what the intent would be based on the cases which do exist. That is to say, while this step is useless in theory, in practice it serves to help a reader understand how ReturnIfAbrupt is used more generally, which is an act of interpretation which is necessary because we have not actually given a complete definition.

But sure, I guess.

[syg]

I actually feel like this should be blocked on #1573. Right now, even though that step is just wrong, you can kinda get at the intention through the wrongness: when expanded via ? Foo(), the intention is to return the unwrapped CR.[[Value]], even though that's not what the scoping of hygenicTemp says. If we remove this strictly-speaking useless line, I think the interim state of the spec is worse where folks will start asking if ? Foo() in fact unwraps normal completions.

[michaelficarra]

Okay that's fair, thanks for the reviews. We'll hold off until #1573 can fix this (literal) nonsense.