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 useless step of ReturnIfAbrupt expansion #1570

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@michaelficarra
Copy link
Member

commented Jun 4, 2019

Since "hygienicTemp is ephemeral and visible only in the steps pertaining to ReturnIfAbrupt", there's no reason to set it in the final step.

Show resolved Hide resolved spec.html
@michaelficarra

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

This whole thread has got me thinking that it'd probably be worth the effort to look for instances of ReturnIfAbrupt that don't pattern match to one of the three cases in 5.2.3.3 or instances of prefix ? or ! that don't pattern match to one of the cases in 5.2.3.4.

@ljharb

ljharb approved these changes Jun 5, 2019

Copy link
Member

left a comment

Per discussion, this change looks good. Further refinements can happen in this or a future PR.

@ljharb ljharb requested review from zenparsing and tc39/ecma262-editors Jun 5, 2019

@jmdyck

This comment has been minimized.

Copy link
Collaborator

commented Jun 5, 2019

instances of ReturnIfAbrupt that don't pattern match to one of the three cases in 5.2.3.3

It looks like all occurrences (outside 5.2.3.3 and 5.2.3.4) match

1. ReturnIfAbrupt(_argument_).

(The argument is always an alias/metavariable, never an operation-invocation, and the ReturnIfAbrupt call is never embedded within some other expression.)

Minor quibble: there's one occurrence that technically doesn't match, because it's in a one-line Else:

1. Else, ReturnIfAbrupt(_result_).

but of course it would match if we made it an indented sub-block.


or instances of prefix ? or ! that don't pattern match to one of the cases in 5.2.3.4.

For ?, it depends on how strictly you want to interpret the cases: both have ? at the start of the step, which (as stated above) doesn't occur in the spec. However, if you interpret them as talking about those forms occurring anywhere in the step, then it depends on what you think matches OperationName() and _someValue_.OperationName().

E.g., There are ~100 occurrences of the form:

? _X_.[[InternalMethod]](_args_)

So it depends whether you think [[InternalMethod]] counts as OperationName.


For !, note that 5.2.3.4 doesn't explicitly say where it's allowed, it just says "Similarly, prefix ! is used..." and then gives one example of its use. Presumably it means that ! is allowed in the same contexts where ? is allowed.

If so, then there are 2 occurrences of the form:

! _X_.[[InternalMethod]](_args_)

So again, it depends.

@jmdyck

This comment has been minimized.

Copy link
Collaborator

commented Jun 5, 2019

Actually, I'm no longer sure now whether ReturnIfAbrupt pattern matches full or partial algorithm steps. The wording in that section reads to me like only full algorithm steps apply, but I think the usage throughout the spec is consistent with partial algorithm steps, though I'm not sure exactly how that would work.

Yeah, 5.2.3.{3,4} seem a bit too vague for my taste. I'd prefer, for each construct, a single precise definition that covers all possible usages, rather than a few patterns whose coverage is at best questionable. However, I don't think we have the notation that would allow precise definitions, and I'm doubtful that it'd be worth introducing it.

@michaelficarra

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

@jmdyck Thanks for doing the initial research! I've confirmed that all of the current usages of ReturnIfAbrupt without the shorthand will match the first definition. And many of them are for syntax-directed operations that could use the shorthand as described at the end of 5.2.3.4. I'm betting the authors just didn't know the shorthand was permitted to be used that way.

In my opinion, the way the shorthand notation is defined right now is unacceptably bad. I'd like to see if we could define rewrite rules for the shorthand notation that operate only on a whole step. Something like

  • [words before] ? AbstractOperation([parameters]) [words after]

gets rewritten to

  • Let hygienicTemp be AbstractOperation([parameters]).
  • If hygienicTemp is an abrupt completion, return hygienicTemp.
  • Else if hygienicTemp is a Completion Record, set hygienicTemp to hygienicTemp.[[Value]].
  • [words before] hygienicTemp [words after]

@ljharb @bterlson thoughts?

@ljharb

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

That sounds great to me - essentially, turning ? AbstractOperation(…) into a do expression :-)

@jmdyck

This comment has been minimized.

Copy link
Collaborator

commented Jun 5, 2019

I was just about to suggest something like that. Except I was going to suggest it for the expansion of ReturnIfAbrupt, and retain the idea of ? being a simple shorthand for ReturnIfAbrupt. You're suggesting to sever that connection?

@michaelficarra

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

Either way works for me.

@ljharb

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

Seems nice to keep the connection and add better shorthand for ReturnIfAbrupt.

@michaelficarra

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

I opened a PR for better ReturnIfAbrupt definition in #1573.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.