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

! shorthand for completion record [[Value]] extraction is unnecessary #1572

Closed
michaelficarra opened this issue Jun 7, 2019 · 11 comments · Fixed by #2547
Closed

! shorthand for completion record [[Value]] extraction is unnecessary #1572

michaelficarra opened this issue Jun 7, 2019 · 11 comments · Fixed by #2547
Labels
completion records Relates to completion records, and ? / ! notation.

Comments

@michaelficarra
Copy link
Member

In §5.2.3.4 ReturnIfAbrupt Shorthands, we define a shorthand ! that's not really related to ReturnIfAbrupt. This shorthand asserts that the completion record is a normal completion and extracts its [[Value]]. But at the end of §5.2.3.1 Implicit Completion Values, we say

Any reference to a Completion Record value that is in a context that does not explicitly require a complete Completion Record value is equivalent to an explicit reference to the [[Value]] field of the Completion Record value unless the Completion Record is an abrupt completion.

I think we should either

  1. remove this clause and use the ! shorthand or explicit [[Value]] extraction throughout the spec, or
  2. remove the ! shorthand, its usages, and any explicit [[Value]] extraction
@claudepache
Copy link
Contributor

The primary purpose of ! is to assert that the operation didn’t end abruptly. Without !, you don’t know whether an abrupt completion is returned.

Also, some people would love to make obsolete the paragraph at the end of §5.2.3.1 Implicit Completion Values you cited, through the systematic use of the ?/! pair.

@ljharb
Copy link
Member

ljharb commented Jun 7, 2019

I'm a bit unclear what the issue is - indeed, the ! is critical to make it clear that a completion record is not abrupt. Perhaps the implicit unwrapping overall, means we don't need an explicit unwrap step for ! - or perhaps we should remove the implicit overall unwrapping, and use explicit unwrapping everywhere - is that what you're suggesting?

@jmdyck
Copy link
Collaborator

jmdyck commented Jun 7, 2019

Also, some people would love to make obsolete the paragraph at the end of §5.2.3.1 Implicit Completion Values you cited, through the systematic use of the ?/! pair.

(And some people would like to make it obsolete via somewhat different means.)

@michaelficarra
Copy link
Member Author

@ljharb Yes, we should either fully buy into that implicit unwrapping everywhere or (my preference) drop that paragraph and fix up anywhere that is relying on it. If the latter, we should probably move the ! shorthand section to the end of the completion values section instead of being lumped in with ReturnIfAbrupt.

@jmdyck Care to elaborate?

@ljharb
Copy link
Member

ljharb commented Jun 7, 2019

I’m fine with the implicit unwrapping in general, but i like it being explicit in the desugaring of ? and !

@TimothyGu
Copy link
Member

I got the feeling that newly written spec text has fairly consistently used !. I’m in support of marking all implicit unwrappings with !.

@bakkot
Copy link
Contributor

bakkot commented Jun 7, 2019

I think there is a useful distinction between operations which never return abrupt completions and those which do sometimes but don't under the particular conditions under which they are invoked in some algorithm. Perhaps we could distinguish the two somehow? For example, we could do away with the first paragraph of 5.2.3.1 so that the first type of algorithm could return unwrapped values and would not need !.

@domenic
Copy link
Member

domenic commented Jun 7, 2019

I think distinguishing those should be done at the declaration site, not the call site. See #253.

@jmdyck
Copy link
Collaborator

jmdyck commented Jun 17, 2019

@michaelficarra: "@jmdyck Care to elaborate?"

The implicit conversion clause only applies to normal completions, so one way to eliminate that clause is to eliminate normal completions. See issue #497.

@jmdyck
Copy link
Collaborator

jmdyck commented Jun 18, 2019

@bakkot:

I think there is a useful distinction between operations which never return abrupt completions and those which do sometimes ...

Yes, that's definitely a useful distinction, and I agree with @domenic that it should be made at the declaration site (see e.g. PR #545). I think it might be useful to also make the distinction at the call site (see #486), but I don't know that anyone else does.

@bakkot
Copy link
Contributor

bakkot commented Jun 18, 2019

@jmdyck I think it would be useful to make it at the call site as well as the declaration site.

The thing I am envisioning is:

  • Algorithms which never return abrupt completions are marked as such, and written so that they return unwrapped values, and are consumed as such.
  • Algorithms which sometimes return abrupt completions are marked as such, and written so that they always return completion records. To return a value they would need to explicitly return NormalCompletion(value).*
    • At a call site which knows that its particular usage will always result in a normal completion, the completion is unwrapped with !.
    • At a call site which does not know this,
      • If it would do what ReturnIfAbrupt does, it unwraps the completion with ?.
      • Otherwise, when it needs to handle abrupt completions itself (e.g. in promise logic), it does not use a macro but always handles the value returned by the call as a completion record.

And then get rid of all implicit conversions, both from and to completion records (except possibly as in the footnote below).

This is similar to #497, except without the mixing of completion records with non-completion values. I'm sure someone has proposed this before.

*: I'm flexible on this last part; we could instead have spec text which says that return value in an algorithm marked as returning completion values means that it is wrapped in NormalCompletion().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
completion records Relates to completion records, and ? / ! notation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants