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.

[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.

[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 https://github.com/tc39/ecma262/pull/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 https://github.com/tc39/ecma262/pull/1571, https://github.com/tc39/ecma262/pull/2744, etc.)