Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Editorial: remove implicit wrapping/unwrapping of completion records #2547

Merged
merged 1 commit into from
Feb 24, 2022

Conversation

michaelficarra
Copy link
Member

@michaelficarra michaelficarra commented Oct 8, 2021

Ready for review. Based on #2546. Fixes #1796. Fixes #497. Fixes #496. Closes #1916. Fixes #253. Fixes #1572. Closes #486. Closes #2612. Fixes #2645.

  1. remove the provision for implicit wrapping in normal completion
  2. remove the provision for treating normal completions as their [[Value]]
  3. annotate return types
  4. make sure every AO has a return type specified; figure out what to do with any remaining explicit "unknown" returns
  5. see if we can replace any ~empty~ returns with ~unused~
  6. make sure all internal methods with the same name have matching return types
  7. make sure all concrete methods with the same name have matching return types
  8. call sites for AOs that return completion records should use ?/!/Completion(); call sites for AOs that do not return completion records should not use !/?/Completion()
  9. make sure that each AO that doesn't return a completion record also doesn't contain a step that uses ReturnIfAbrupt/?/throw
  10. make sure that each AO that does return a completion record might plausibly return an abrupt completion, i.e. it uses ? or throw or ReturnIfAbrupt or returns a completion explicitly
  11. make sure that all AOs that return ~unused~ are always invoked using "Perform". if it's a completion record that normally contains ~unused~, make sure only its [[Type]] is relied upon.
  12. If an AO is only ever invoked using "Perform", it should return ~unused~ or a completion record that normally contains ~unused~
  13. find any AOs that return completion records but are always called using ! and make them not return completion records
  14. add notational convention for wrapping of returns in NormalCompletion in AOs that return completion records; except Completion or ThrowCompletion or NormalCompletion
  15. replace return Completion(X) with return ? X in AOs that return a completion record
  16. make sure that all uses of Completion() are on the call of an AO that returns a completion, field accesses, or some other opaque source
  17. go through each AO that returns a completion record with unknown normal type and see if we can further refine it
  18. make sure we're not returning Completion(...) or Completion Record { ... } or NormalCompletion(...) or ThrowCompletion(...) in algorithms that don't return completion records
  19. check as many of the above as possible in ecmarkup and bump ecmarkup
  20. go through effect suppressions and make sure they are still valid and necessary
  21. go through issues mentioned in Editorial: Can-call-user-code annotation #2548 and make sure they are addressed here
  22. add visual indicator to AO headers when the AO returns a completion record
  23. explain what "a Completion Record normally containing an X" means; maybe think of better phrasing?

@arhadthedev
Copy link
Contributor

The PR introduces a term a Completion Record normally containing [a Type]. I think it would be useful to explicitly define it in https://tc39.es/ecma262/#sec-completion-record-specification-type via a normal completion (probably with no <dfn> to avoid visual clutter). Otherwise it may seem that "normally" means "sometimes it can be violated and another type may appear".

spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
@michaelficarra michaelficarra marked this pull request as ready for review February 4, 2022 14:57
@jmdyck
Copy link
Collaborator

jmdyck commented Feb 4, 2022

(You should probably also update the PR description where it says "Not ready for review".)

@jmdyck
Copy link
Collaborator

jmdyck commented Feb 4, 2022

replace return Completion(X) with return ? X in AOs that return a completion record

Why?

@michaelficarra
Copy link
Member Author

@jmdyck No reason, we just thought it was stylistically neater and semantically it's equivalent.

@bakkot
Copy link
Contributor

bakkot commented Feb 4, 2022

In particular, we already have lots of Return ? Foo(), which is equivalent to Return Completion(Foo()); we might as well pick one of those two forms and stick to it. And the former is less noisy (and, to my mind, clearer).

@jmdyck
Copy link
Collaborator

jmdyck commented Feb 5, 2022

In particular, we already have lots of Return ? Foo(), which is equivalent to Return Completion(Foo()); we might as well pick one of those two forms and stick to it.

Okay. Given those choices, I'd vote for Return Completion(Foo()).

And the former is less noisy (and, to my mind, clearer).

I might agree that Return ? X is less noisy than Return Completion(X), but

  • not much, and
  • if noise is a concern, you could invent a less noisy notation for Completion(X).

As for "clearer", I strongly disagree. To me,

  • Return Completion(X) says: X is a CR, which is returned.
  • Return ? X says: X is a CR. If it's an abrupt CR, then it's [implicitly] returned (via the semantics of ?). Otherwise (i.e., if it's a normal CR), the CR's [[Value]] is extracted and becomes the argument of the Return, at which point that value is implicitly passed to NormalCompletion(), and the resulting normal CR is [explicitly] returned.

Also, in a lot of cases of Return ? _cr_, the _cr_ is never a normal CR, so the abrupt 'path' is always taken and the Return is never actually executed, which feels odd. But there's no corresponding problem for Return Completion(_cr_).

@bakkot
Copy link
Contributor

bakkot commented Feb 5, 2022

It's true that if you think about what the spec looks like after applying the rewrite rules, that thing ends up being a fairly awkward sequence of steps.

But most readers are not thinking about what the spec looks like after applying rewrite rules, and they already encounter ? AO() all the time. Return ? AO() looks like Let _a_ be ? AO() and requires no further thought. Using a different construct than the usual one here obscures that fundamental similarity.

I stand by "clearer" here.

@ljharb ljharb deleted the GH-1796 branch February 24, 2022 18:16
domenic pushed a commit to whatwg/html that referenced this pull request Feb 27, 2022
As part of the tc39/ecma262#2547 the JavaScript specification has reformed how it deals with Completion Records, and thus what abstract operations can return. See tc39/ecma262#253 (comment) for a brief description of when to use ? vs. ! vs. nothing at all.

Additional minor fixes:

* Replace uses of ObjectCreate (which no longer exists) with OrdinaryObjectCreate.
* Replace "Rethrow any exceptions" with the ? syntax.
* Use ? with GetFunctionRealm, which can throw due to revoked proxies.
domenic pushed a commit to whatwg/webidl that referenced this pull request Mar 7, 2022
As part of the tc39/ecma262#2547 ecma262 has reformed how it deals with Completion Records. See tc39/ecma262#253 (comment) for a brief description of when to use ? vs. ! vs. nothing at all.
ljharb added a commit to ljharb/ecma262 that referenced this pull request Mar 11, 2022
ljharb added a commit to ljharb/ecma262 that referenced this pull request Mar 11, 2022
ljharb added a commit to ljharb/ecma262 that referenced this pull request Mar 11, 2022
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Apr 18, 2022
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 tc39#2547,
these cases would have caused an implicit unwrapping
from the Completion Record to its [[Value]],
but tc39#2547 removed the idea of implicit unwrapping,
so these references have been editorial bugs since then.
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Apr 21, 2022
…c39#2676)

This is a follow-up to PR tc39#2547, which tagged many operations
as returning a normal completion or an abrupt completion.
But in most cases, the only abrupt completion possible is a throw completion.
So this PR changes lots of "abrupt completion" to the more precise "throw completion".
ljharb pushed a commit to jmdyck/ecma262 that referenced this pull request Apr 21, 2022
…9#2676)

This is a follow-up to PR tc39#2547, which tagged many operations
as returning a normal completion or an abrupt completion.
But in most cases, the only abrupt completion possible is a throw completion.
So this PR changes lots of "abrupt completion" to the more precise "throw completion".
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Apr 28, 2022
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 tc39#2547,
these cases would have caused an implicit unwrapping
from the Completion Record to its [[Value]],
but tc39#2547 removed the idea of implicit unwrapping,
so these references have been editorial bugs since then.
mfreed7 pushed a commit to mfreed7/html that referenced this pull request Jun 3, 2022
As part of the tc39/ecma262#2547 the JavaScript specification has reformed how it deals with Completion Records, and thus what abstract operations can return. See tc39/ecma262#253 (comment) for a brief description of when to use ? vs. ! vs. nothing at all.

Additional minor fixes:

* Replace uses of ObjectCreate (which no longer exists) with OrdinaryObjectCreate.
* Replace "Rethrow any exceptions" with the ? syntax.
* Use ? with GetFunctionRealm, which can throw due to revoked proxies.
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Jun 16, 2022
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 tc39#2547,
these cases would have caused an implicit unwrapping
from the Completion Record to its [[Value]],
but tc39#2547 removed the idea of implicit unwrapping,
so these references have been editorial bugs since then.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial change ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
6 participants