Editorial: new algorithm convention for return-if-abrupt

[bterlson]

This replaces occurrences of the pattern:

1. Some call is made Here().
2. ReturnIfAbrupt() is called on the next line.

with

1. Some call is made ?Here().

There is also some new prose in Algorithm Conventions I imagine we can nitpick significantly :)

[ljharb]

Quick first pass:

• Is it better for readability to have ?Foo, or ? Foo? I feel like the space will make it easier to see the question mark.
• Perhaps the ?s can be auto-linked to the appropriate section describing the syntax, to ensure that any reader can easily figure out what it means?
• You have a couple places with this?NumberValue where I assume you mean ?thisNumberValue, and the casing broke your regex?
• These steps (and a number of others that now have multiple abrupt-returnable steps on one line) are missing the ? before both ToNumbers, which also could return an abrupt completion:

1. If _month_ is not specified, let _m_ be ?MonthFromTime(_t_); otherwise, let _m_ be ToNumber(_month_).
1. If _date_ is not specified, let _dt_ be ?DateFromTime(_t_); otherwise, let _dt_ be ToNumber(_date_).

• Same as above, with ToString - ie, 1. Let _elementK_ be ?Get(_O_, ToString(_k_)). should be 1. Let _elementK_ be ?Get(_O_, ?ToString(_k_)). - ie, the Get must not occur if the ToString returns an abrupt completion.

[bterlson]

Thank you! Will fix these issues!

On the syntax, I went with no space as its the style for unary operators, but I think Andreas prefers the space as well so maybe others do too?

I don't know about auto-linking ? to its definition... there are so many of them. Just read your algorithm conventions :-P

[bterlson]

Also, fwiw, the nested cases today don't RIA for intermediate values. In some cases, the inner operation can't fail. In other cases, the outer function passes through aburpt completion values. In still other cases, the implicit conversion of completion records to values is used.

I think it's ok to move these all to explicit RIA though.

[anba]

A couple of step notes probably need to be updated, too.

[bterlson]

@anba you are probably right :/ Any idea of a good way to find these other than manually?

FWIW, here's my list of ops that can't throw and so will never be prefixed with ?:

['ReturnIfAbrupt', 'NormalCompletion', 'Completion', 'Type',
'GetBase', 'GetReferencedName', 'IsStrictReference',
'HasPrimitiveBase', 'IsPropertyReference', 'IsUnresolvableReference',
'IsSuperReference', 'GetThisValue']

Adding to this list will likely result in a better conversion.

[bterlson]

Pushed a new commit that should address all @ljharb's issues. Now to figure out how to update step references :-P. Maybe step references should be generated somehow similar to other cross-refs.

[anba]

@anba you are probably right :/ Any idea of a good way to find these other than manually?

I only count 35 <emu-note> which contain "step", so manual updates should be doable.

Pattern <emu-note>.*?</emu-note> and then checked for "step"/"Step": https://gist.github.com/anba/7920c6d755706ff7ce68

[anba]

Maybe step references should be generated somehow similar to other cross-refs.

Yes, please!

[bterlson]

Anba, there are normative step references as well. Also some steps use the "NOTE: " convention to add a note to steps. But I think manually is the only way sadly :( Will work on updating step references tonight or tomorrow.

The problem with generating them is you have nothing to anchor the reference to without a number. Which means you might require some kind of tag at the referenced site, eg:

1. <a id="somealg-do-things"></a>Do some things.
2. Do them again.
3. Go to <emu-xref href="#somealg-do-things"></emu-xref>.

But I never could bring myself to do that because it's quite a bit of effort for authors. Perhaps the cost of using this convention is greater than just applying manual fixups occasionally?

[jmdyck]

The problem with generating them is you have nothing to anchor the reference to without a number.

You could maybe use the text of the referenced step (or some distinct prefix of it) as the anchor. E.g.:

   3. Go to <emu-xref aoid="foo" steptext="Do some things"></emu-xref>.

Then the referenced step doesn't need to have an id attached to it.

[bterlson]

That's a pretty good idea @jmdyck. I'll investigate. As long as steptext is unique it could be one character long even. Pretty nice :)

[bterlson]

Fixed step references.

Now concerned about the following pattern:

1. If something...
  1. let _foo_ be Call(...).
1. Else,
  1. let _foo_ be OtherCall(...).
1. ReturnIfAbrupt(_foo_).

Suspect I'd fix this incorrectly to

1. If something...
  1. let _foo_ be Call(...).
1. Else,
  1. let _foo_ be ?OtherCall(...).

[bterlson]

There were a few cases like that that I've fixed (for now by just not doing the conversion). Now back to wanting another review! :-D

[anba]

Issues:

• The new syntax needs to be explained in 6.2.2
• The inner ?Get in Let _enum_ be ?ToBoolean(?Get(_Obj_, "enumerable")). should be a simple Get. (This issue is present multiple times.)
• Invalid conversion in 9.4.4.5 (? before parens)
• Invalid conversion in 9.4.5.1 ("IntegerIndexedElementGet?")
• 12.7.3.1, first emu-note, step refs not updated
• Invalid conversion in 12.9.3 (? before parens)
• Invalid conversion in 20.1.3.3 (? before parens)
• Date.prototype.*: YearFromTime, MonthFromTime, DateFromTime, HourFromTime, MinFromTime, SecFromTime, msFromTime are all infallible, no '?' needed here.
• 20.3.4.21: '?' not needed for LocalTime
• Invalid conversions in String.prototype.includes, indexOf, lastIndexOf, startsWith, match, search (? before parens)
• Reverts the changes in 21.2.2.1
• Array.prototype.concat,filter,map: '?' misplaced for CreateDataPropertyOrThrow
• Array.prototype., %TypedArray%.prototype.: unnecessary ?ToString(n) where n is number
• %TypedArray%.prototype.set (2x): 'ToInteger?' instead of '?ToInteger'

Other questions/notes:

• Are there any plans to update evaluation of parser productions to use new the short form?
• Do you plan to perform a second pass to update ReturnIfAbrupt calls like in 20.1.3.2, 24.3.1.1, etc.?
• Do you think it makes sense to move the abrupt completion check from the callee to the callers in operations like "Abstract Relational Comparison"? Cf. ?GetValue(lref) vs GetValue(rref) in 12.9.3, abrupt completions for rref are later handled in "Abstract Relational Comparison".
• We may need a new notation to call an fallible operation to reduce the number of unused variables. 1. Let status be Compute(). 2. ReturnIfAbrupt(status). is now just 1. Let status be ?Compute(). and the status variable is never read.

[bterlson]

The new syntax needs to be explained in 6.2.2

Hmm, I had text in 5.2 earlier but it seems to have been dropped somehow. But I think 5.2 is more appropriate than 6.2.2. Maybe both places could be argued :-P.

The inner ?Get in Let enum be ?ToBoolean(?Get(Obj, "enumerable")). should be a simple Get . (This issue is present multiple times.)

I'm not sure... I did this intentionally. In general, for these nested forms to work, the outer function has to pass through abrupt completions. IMO it is more clear to instead make the inner call RIA. Is this bad for reasons I'm not seeing?

20.3.4.21: '?' not needed for LocalTime

Current spec text has RIA, so I'll leave it for now.

Reverts the changes in 21.2.2.1

I think it's because this wasn't rebased?

Array.prototype.concat,filter,map: '?' misplaced for CreateDataPropertyOrThrow

For some reason the source text has a space here. This will have to be fixed manually later. Same is true for the typed array cases.

Are there any plans to update evaluation of parser productions to use new the short form?

Sure :) Incremental improvements are the name of this game!

Do you plan to perform a second pass to update ReturnIfAbrupt calls like in 20.1.3.2, 24.3.1.1, etc.?

I was planning on going through every explicit RIA call and fixing it if necessary...

Do you think it makes sense to move the abrupt completion check from the callee to the callers in operations like "Abstract Relational Comparison"? Cf. ?GetValue(lref) vs GetValue(rref) in 12.9.3, abrupt completions for rref are later handled in "Abstract Relational Comparison".

Probably. It's kind of confusing as-is.

We may need a new notation to call an fallible operation to reduce the number of unused variables

I think we can use the calling convention "Perform ?Compute()" which is used in a few places (minus the ? of course).

[anba]

The inner ?Get in Let enum be ?ToBoolean(?Get(Obj, "enumerable")). should be a simple Get . (This issue is present multiple times.)

I'm not sure... I did this intentionally. In general, for these nested forms to work, the outer function has to pass through abrupt completions. IMO it is more clear to instead make the inner call RIA. Is this bad for reasons I'm not seeing?

I guess this depends on how ? gets defined. If it is just expanded to a return instruction like in ReturnIfAbrupt, it's probably more difficult to explain the inner ?. (Also: If ?Get already stops the algorithm when an abrupt completion is encountered, the ? in ?ToBoolean is not required.)

[bterlson]

See how you feel after reading the copy I added to 5.2? :)

[ljharb]

So far looking at the diff everything looks great to me.

fwiw, I still think it looks better with ? Foo instead of ?Foo - unary operators notwithstanding, this isn't code, it's spec text, and readability matters. Note that if "?" was in bold, I wouldn't feel the space is needed for readability - perhaps that would be a good compromise?

[UltCombo]

Regarding readability, If all ? were linked to the RIA semantics, they would have a distinctive blue color as an added bonus. 😸

[ljharb]

Sure - I'd be fine with linking, bolding, or adding the space - I prefer linking but anything imo is better than visually running the ? into the abstract operation name.

[bterlson]

I believe I have addressed all outstanding feedback in this thread (including adding the space!). I'm going to pull this in today. Any further issues can be raised of course :)

[bterlson]

Committed as 9c1e076! 🎆