Editorial: Make Evaluation more like regular SDOs

[jmdyck]

The Evaluation SDO is different from most SDOs in several ways, which this PR tries to reduce.

Each commit is a logically separate refactoring, but they should probably be squashed before merging.

The "spec bug" label is not to imply that treating Evaluation differently was a bug, but rather that this PR fixes some bugs along the way.

---

(1) The definitions of Evaluation are distributed over the "ECMAScript Language" sections of the spec rather than being gathered into one place (as established in PR #2271).

This PR does not consolidate the definitions of Evaluation.

However, the distributed definition has also meant that this SDO doesn't have a "home" section, so the first commit of this PR adds such a section. Now there will be a place to link to for "Evaluation", and a place (in the rendered spec) where you can find links to invocations of it. There could also be some prose that talks about Evaluation in general; suggestions welcome.

(The placement of the section, making it the new 8.1, was suggested by bakkot. To me, it seems odd to put a Runtime Semantics section before a Static Semantics section. I'd be inclined to reorganize section 8 into SS and RS, but I don't care too much.)

Note: Since Evaluation is now declared to return a Completion Record, it doesn't need to be special-cased in the first para of Implicit Normal Completion.

(Another aspect of Evaluation's distributed definition is that the multiple headings for its definitions don't conform to the 'structured header' conventions of PRs #2512 and #2547. I didn't try to change this, as bakkot thought that might confuse ecmarkup.)

---

(2) Currently, the invocations of Evaluation don't refer to it by that name, but are instead of the form "the result of evaluating X".

The second commit of this PR changes all such to "Evaluation of X".

Note that I left a few occurrences of the old form:

• In [[Call]] + [[Construct]] for a built-in function object, the result of evaluating _F_ isn't a reference to the Evaluation SDO. (For one thing, the 'operand' is a function object, not a Parse Node.)
• Similarly in Property Accessors.
• In Annex F, they are references to Evaluation, and could reasonably be changed. However, it's prose (where "the result of evaluating X" reads better), and it's an Annex, so I'm not sure there's a benefit. Let me know.

---

(3) Although Evaluation returns Completion Records, its invocations aren't currently marked with !, ?, or Completion().

To remove this difference, commit 3 starts by surrounding every invocation of Evaluation with Completion(), and then commits 4-8 gradually replace most of those with ?.

(Commit 3 skips the two invocations of Evaluation in the chain rule example at the bottom of 5.2.2 Syntax-Directed Operations, because injecting "Completion()" would just confuse the reader. [Unfortunately, this omission causes ecmarkup to fail.] Perhaps the example should be rewritten to use an SDO that doesn't return a CR.)

Note: In commit 3, Implicit Normal Completion can drop the exception:

when directly returning with the phrase "Evaluation of",

because all of those cases are now covered by the exception:

when the result of applying Completion ... is directly returned.

Commits 4-8 have detailed commit messages; I'm not sure what I'll do with those if/when they get squashed.

---

Commit 9 is just a bonus bugfix.

[bakkot]

Thanks! I'll give this a detailed review soon.

(Commit 3 skips the two invocations of Evaluation in the chain rule example at the bottom of 5.2.2 Syntax-Directed Operations, because injecting "Completion()" would just confuse the reader. [Unfortunately, this omission causes ecmarkup to fail.] Perhaps the example should be rewritten to use an SDO that doesn't return a CR.)

That might be warranted, but either way ecmarkup probably shouldn't apply that rule to algorithms marked example; I'll fix it up.

[jmdyck]

Oh, I checked the downstream specs: they don't have any occurrences of "the result of evaluating".

[michaelficarra]

Can the new "home" section for Evaluation link to all the other piecewise definitions? I don't really care whether this is manually maintained or generated by ecmarkup (for now). Just a big ul with each li representing one definition site and including a comma-separated list of the nonterminals that are covered at that definition site would be sufficient.

[jmdyck]

Can the new "home" section for Evaluation link to all the other piecewise definitions?

@bakkot, let me know if ecmarkup might do this in the foreseeable future.

[bakkot]

I think probably not? It's too specific to this one SDO. Also I'm less convinced of the utility of that than Michael, vs just C-f'ing for "runtime semantics: evaluation".

[jmdyck]

just C-f'ing for "runtime semantics: evaluation".

If you're in single-page mode, that gets 70 matches. And when you land on a match, you might have to scroll a bit to establish whether this section has the definition you're looking for.

If you're in multi-page mode, there are 4 separate pages with matches.

---

It's probably faster to go to the defining occurrence of the nonterminal in question, then go to the next "Evaluation" section.

[jmdyck]

Can the new "home" section for Evaluation link to all the other piecewise definitions? [...] Just a big ul with each li representing one definition site and including a comma-separated list of the nonterminals that are covered at that definition site would be sufficient.

I've added a commit that adds that index.

There are 7 nonterminals whose Evaluation semantics are defined over more than one section:

• 2x AdditiveExpression
• 3x CallExpression
• 3x MemberExpression
• 3x PrimaryExpression
• 3x ShiftExpression
• 7x UnaryExpression
• 4x UpdateExpression

[bakkot]

Hm. Now that I see this rendered, I feel like it's actually inverted: the thing you want is to go from a nonterminal to its clause number, not from a clause number to its nonterminals. Would it be better to instead list, for each of the relevant nonterminals, the clause or clauses which define its Evaluation semantics? Possibly combining items where there's multiple nonterminals defined in a single clause and no others.

So like

• |IdentifierReference|: 13.1.3
• [...]
• |ComputedPropertyName|, |LiteralPropertyName|, |ObjectLiteral|: 13.2.5.4
• [...]
• |CallExpression|: 13.3.2.1, 13.3.6.1, 13.3.11.1
• |MemberExpression|: 13.3.2.1, 13.3.5.1, 13.3.11.1
• [...]
• |AdditiveExpression|: 13.8.1.1, 13.8.2.1

Though either way I guess it's probably going to be easier to navigate by search rather than actually reading, so perhaps it doesn't matter much. Thoughts?

[michaelficarra]

@bakkot I'm okay with either but I think I slightly prefer the current organisation.

[jmdyck]

Would it be better to instead list, for each of the relevant nonterminals, the clause or clauses which define its Evaluation semantics?

Yeah, I wondered about that too. With the current format, it isn't always obvious that there are multiple sections for a given nonterminal. So if you find one occurrence of the nonterminal in the index (either by control-F or visually), click on the section number, and don't find what you're looking for, it isn't clear that you need to go back, look for another occurrence of that nonterminal in the index, and repeat. With ones like UnaryExpression, where all the occurrences are together, you can see at a glance that there are multiples, but with ones like PrimaryExpression, CallExpression, and MemberExpression, you probably can't. Readers might find that frustrating.

If we do 'invert' the index, I'd suggest putting the nonterminals in alphabetical order (for people who want to scroll rather than control-F).

[syg]

It strikes me that the current formulation of the index is not very useful. As a reader I'd be using the index if I had a piece of code in mind and I want to jump to the definition of Evaluation for it. If I'm not already an expert in the grammar and know what non-terminal covers that case, I wouldn't know how to use the index anyway.

So, I'd prefer the index to list the productions themselves, as well as be organized by production instead of by section number.

[jmdyck]

So, I'd prefer the index to list the productions themselves, as well as be organized by production instead of by section number.

It's unclear what "organized by production" means. I'm guessing that the alternatives for a given nonterminal would appear together, but there are a couple ways that the nonterminals could be ordered:

• alphabetically, or
• in the order that the spec defines them.

(There's also Annex A order, but I think that's the same as spec-definition order for the nonterminals that appear in this index.)

---

I can imagine us iterating a few more times on the format of this index, so maybe it should be a separate PR (since it isn't part of the point of this PR).

[bakkot]

We discussed this at the editor call today. We're fine with landing without the index for now and doing it in a followup. We did discuss details of exactly how it should look, eventually, and came away with

• productions are ordered as in the grammar
• every individual production which has Evaluation semantics gets its own line in the index, followed by a link to the clause which defines the Evaluation semantics for that production (text of the link being the clause number, as generated by emu-xref)

so the first few lines would be like

IdentifierReference : Identifier
13.1.3

IdentifierReference : `yield`
13.1.3

IdentifierReference : `await`
13.1.3

BindingIdentifier : Identifier
14.7.5.8

BindingIdentifier : `yield`
14.7.5.8

BindingIdentifier : `await`
14.7.5.8

PrimaryExpression : this
13.2.1.1

[michaelficarra]

LGTM otherwise. This is great!

[ljharb]

@jmdyck want to clean up these commits, or should i?

[jmdyck]

@ljharb: I need to think about how to do it.

[jmdyck]

Commits 4-8 have detailed commit messages; I'm not sure what I'll do with those if/when they get squashed.

I decided to preserve them here:

commit 5fff77832417f2e6f2e10930f05a817bc3bd2c33

Change some `Completion()` to `?` (ReturnIfAbrupt)

Specifically, change occurrences of:
  1. Let _foo_ be Completion(Evaluation of X).
  1. ReturnIfAbrupt(_foo_).
to:
  1. Let _foo_ be ? Evaluation of X.

This is basically just applying the definition of `?` in reverse.

commit d45660a1b3a159f161e7fb9e5aa8fc91e8e928fc

Change more `Completion()` to `?` (Return)

Specifically, change occurrences of:
```
  1. Return Completion(Evaluation of X).
```
to:
```
  1. Return ? Evaluation of X.
```
These are basically equivalent but the latter is preferred.

That is, if Evaluation returns an abrupt completion,
then the `?` will cause the algorithm to return it.
And if Evaluation returns a normal completion,
the `?` will unwrap it,
and then the Implicit Normal Completion rules will re-wrap it,
and then the `Return` will return it.
So either way, the algorithm returns the Completion that Evaluation returns.

Except: if you can prove that, for a given call site,
all invocations will return a normal completion, then it should be '!', not '?'.
However, I don't think that's true for any of the cases here.

commit 0a75518a942b8be5398a33f67055edc0987fd078

Change more `Completion()` to `?` (GetValue tricky)

Specifically, change occurrences of:
```
  1. Let _ref_ be Completion(Evaluation of X).
  1. ... ? GetValue(_ref_) ...
  1. (later references to _ref_)
```
to:
```
  1. Let _ref_ be ? Evaluation of X.
  1. ... ? GetValue(_ref_) ...
  1. (later references to _ref_)
```

In the following case analysis, let "CR" denote the value
returned by `Evaluation of X` (always a Completion Record),
and, if it's a normal completion,
let "V" denote the value in its [[Value]] field.

Before the change:
  If CR is an abrupt completion:
  it is bound to _ref_,
  then passed to GetValue,
  which executes ReturnIfAbrupt on it,
  i.e. returns it from GetValue,
  and the `?` before GetValue returns it from the algorithm.

  If CR is a normal completion:
  _ref_ is bound to it,
  then passed to GetValue,
  which would execute ReturnIfAbrupt on it,
  which would unwrap it to V.
  The rest of GetValue then operates on V,
  and then, in any event, returns some completion record,
  not necessarily the same as CR.
  (Note that _ref_ remains bound to CR.)

After the change:
  If CR is an abrupt completion:
  the `?` before Evaluation returns it from the algorithm.
  (So, the same net effect as before.)

  If CR is a normal completion:
  the `?` before Evaluation unwraps it to V,
  _ref_ is bound to V,
  then passed to GetValue.
  GetValue's ReturnIfAbrupt has no effect on V,
  and the rest of GetValue operates on V,
  and returns some completion record,
  not necessaily the same as CR.
  (So, the same effect as before,
  *except* that _ref_ is bound to V, not CR.
  This difference is important because of
  the later references to _ref_.)

To summarize:
if the result of `Evaluation of X` is a normal completion,
then in the status quo, the later references to _ref_
are references to that Completion Record,
but with this commit, they would be references to the [[Value]]
of that Completion Record.

However, I believe that in every case,
that difference either doesn't matter,
or it fixes an editorial bug.

Example of "doesn't matter":
In some cases, the only later reference
is where _ref_ is passed to the first parameter of PutValue,
which immediately calls ReturnIfAbrupt on it,
so it doesn't matter if _ref_ refers to the Completion Record or its [[Value]].

Example of "fixes an editorial bug":
In the Evaluation rule for
`CallExpression : CoverCallExpressionAndAsyncArrowHead`
the next step says:
```If _ref_ is a Reference Record, ...```
In the "Before" world, where _ref_ is bound to a Completion Record,
this test is never true.

Another example:
In many cases, the value of _ref_ is passed (directly or indirectly)
to the second parameter of EvaluateCall,
which requires "an ECMAScript language value or a Reference Record".
In the "Before" world, _ref_ is bound to a Completion Record,
which is not what's required.

Prior to the merge of PR #2547,
these cases would have caused an implicit unwrapping
from the Completion Record to its [[Value]],
but #2547 removed the idea of implicit unwrapping,
so these references have been editorial bugs since then.

commit a604df0b1bb5a8c16d2bd6c530e2af020f4eac80

Change more `Completion()` to `?` (GetValue simple)

Specifically, change occurrences of:
```
  1. Let _ref_ be Completion(Evaluation of X).
  1. ... ? GetValue(_ref_) ...
  1. (no further references to _ref_)
```
to:
```
  1. Let _ref_ be ? Evaluation of X.
  1. ... ? GetValue(_ref_) ...
  1. (no further references to _ref_)
```
This is like the previous commit,
but without the complication of other references to `_ref_`.

Note that, in these cases, we could eliminate _ref_
by inlining its only use:
```... ? GetValue(? Evaluation of X) ...```

commit 7a9038b23bd13923e5283001845f2e98785b1552

Change more `Completion()` to `?` (bugfix)

In the Evaluation rule for
  UnaryExpression : `typeof` UnaryExpression
we have to unwrap the result of Evaluation
in order to make sense of step 2,
testing whether it's a Reference Record.

[jmdyck]

@ljharb: Okay, it's ready to go.

(I'm getting an "x" on requiring "Allow Edits" because somehow it got unchecked. I re-checked it, but that doesn't make the "x" go away.)

[bakkot]

Still LGTM, thanks!

I re-checked it, but that doesn't make the "x" go away.

Yeah, that job needs to be manually re-run, which I've done.