change how ReturnIfAbrupt and its shorthands are specified

[michaelficarra]

In #1570, we discussed how the way ReturnIfAbrupt and its shorthands are specified is unclear. I've made it clear that the ReturnIfAbrupt equivalence is on sequences of algorithm steps and the shorthand is a simple replacement in any context.

I've updated the ! shorthand in this PR as well, but in #1572 I give the option that we remove it, so we should resolve that first.

I recommend reading the current and proposed spec text separately to review. The diff will not be very helpful.

This will obsolete #1570.

[michaelficarra]

@ljharb I've moved the ! shorthand up to the "Implicit Completion Values" section for now while we decide its fate in #1572.

[jmdyck]

Rather than having 2 definitions, discriminated by the form of the argument, you could have one definition with a generalized argument.

[michaelficarra]

Since it's a structural rewrite, I'd like to keep it as simple as I can. Though one could argue that 1 rewrite rule is simpler overall than 2 individually simpler rewrite rules. I'll leave that one up to @tc39/ecma262-editors.

[ljharb]

1 seems probably simpler, but totes fine to do that in a followup.

[michaelficarra]

I've reduced the ReturnIfAbrupt definition to a single macro expansion, though it now has the peculiarity that, when used on an alias, as in ReturnIfAbrupt(_someAlias_), we are left with a step at the end that just references the alias. I think that's probably fine, but it may not be clear to everyone that that is meant to be a no-op. Let me know if you want me to switch it back to 2 expansions, I'm not tied to a particular choice.

[bakkot]

I like this as a single expansion. I think the "step at the end that just references the alias" thing is unlikely to trip up readers, but if we're worried, we could say "the last line is omitted if *[before]* is not present" (or "is empty" or whatever).

[jmdyck]

As in the status quo, this is an illustrative example, not a real definition. You should maybe give it a proper definition as with 'ReturnIfAbupt'.

[michaelficarra]

Since the ! shorthand has no effect on the evaluation, I think it's a little less important to be as rigorous as we are with ReturnIfAbrupt expansion. If anything, the ? shorthand would deserve that level of rigour. I would do it for ? if the editors think that would help.

[michaelficarra]

@jmdyck I've addressed your comments and left some responses. Please do another review.

[michaelficarra]

Okay @jmdyck @ljharb ready for another round of reviews.

[ljharb]

not sure if we need to call out Perform (ie, Perform ? Op() or Perform ! Op()), or if that's a topic for a different PR.

[michaelficarra]

A potential issue with this change as-written: multiple ReturnIfAbrupt are permitted within a single algorithm step, but are always evaluated right-to-left. This works fine for instances like

... ?A( ?B( ?C() ) ) ...

which I had in mind when writing it, but causes undesirable ordering for instances like

... ?A( ?B(), ?C() ) ...

These both would evaluate C, then B, then A. I'm not sure how we'd go about writing a macro expansion that correctly orders the latter case. Is it necessary to support those?

[devsnek]

... ?A( ?B(), ?C() ) ...

At that point you should probably be using multiple lines just for the sake of clarity.

[michaelficarra]

Okay, then to be clear @devsnek, you think it would be okay forbidding that usage? If so, I should add a non-normative note to spec/proposal authors that describes this potentially erroneous usage.

[jmdyck]

A potential issue with this change as-written: multiple ReturnIfAbrupt are permitted within a single algorithm step, but are always evaluated right-to-left. This works fine for instances like

... ?A( ?B( ?C() ) ) ...

which I had in mind when writing it, but causes undesirable ordering for instances like

... ?A( ?B(), ?C() ) ...

These both would evaluate C, then B, then A. I'm not sure how we'd go about writing a macro expansion that correctly orders the latter case.

Tangent:

I'm guessing you mean that the correct ordering for the latter case is B then C then A, but I don't think the spec actually says that. 5.2.1 Abstract Operations says:

Abstract operations are typically referenced using a functional application style such as OperationName(arg1, arg2).

but it doesn't then go on to say anything about the order in which the args are 'evaluated'.

I'd be inclined to say that any case where the order is observable is a spec bug. (Detecting such bugs is another matter though.)

[devsnek]

You can detect the order by making them both throw

[jmdyck]

You can detect the order by making them both throw

I was talking about detecting (just by examining the spec) places where the order of 'evaluation' of operation arguments could be observed. I think you're saying that, once such a place has been identified, you can construct a test case that will reveal which order an implementation is using.

[michaelficarra]

This is blocked until completion record reform work has been completed.

[bakkot]

These both would evaluate C, then B, then A. I'm not sure how we'd go about writing a macro expansion that correctly orders the latter case. Is it necessary to support those?

My preference would be to define an evaluation order for spec steps more generally - not rigorously, just say something like "Evaluation of each algorithm step follows a left-to-right, inside-to-outside evaluation order, so that the step Let _x_ be A(B(), C()) + D(E()). evaluates B, then C, then A, then E, then D" or something like that.

And then in this macro you can say that if there are multiple occurrences of ReturnIfAbrupt within one step, they are expanded in evaluation order, with the first to be evaluated being expanded first.

[jugglinmike]

@michaelficarra was gh-2547 the completion reform which blocked this work?

[michaelficarra]

@jugglinmike Yes, this should be unblocked now.

[jugglinmike]

Thanks, Michael! I can't commit to moving this along, but it helps to understand the status.

[michaelficarra]

@jugglinmike That's okay, I plan on revisiting it soon(-ish). Let me know if you want to take it over, though. I don't remember what work it needs.

[bakkot]

What if we just do away with ReturnIfAbrupt entirely, and instead just define ? directly? That's probably simpler, since it doesn't need to rebind any aliases. And after #3268 there's only 4 uses of ReturnIfAbrupt left, so getting rid of it is straightforward.

(In es2015 there were closer to 100, but we've been chipping away at them in #1571, #2744, etc.)