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: Fix return value logic for async arrow functions #1406

Merged
merged 1 commit into from
Jan 23, 2020

Conversation

TimothyGu
Copy link
Member

@TimothyGu TimothyGu commented Jan 13, 2019

Introduce a new production ExpressionBody for both plain and async arrow functions, and allow correctly handling return value logic for both. Currently, AsyncFunctionStart does not correctly handle the return value of async arrow functions correctly.

Copy link
Member

@zenparsing zenparsing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bug with the current specification, correct?

@TimothyGu
Copy link
Member Author

@zenparsing Yes I believe so

@ljharb ljharb requested review from bterlson and a team January 16, 2019 22:53
@anba
Copy link
Contributor

anba commented Jan 21, 2019

I wonder if a better approach to fix this issue is to add a new runtime semantics to evaluate AssignmentExpressions for arrow-function like contexts.

14.2.15 Runtime Semantics: EvaluateBody has:

  1. ...
  2. Let exprRef be the result of evaluating AssignmentExpression.
  3. Let exprValue be ? GetValue(exprRef).
  4. Return Completion { [[Type]]: return, [[Value]]: exprValue, [[Target]]: empty }.

which is exactly what we need here for async arrow functions, i.e. evaluate an AssignmentExpression and then convert the result into a Return completion. (Plus it also handles GetValue which also seems to be missing for async arrow functions.).

The same steps are executed in the class fields proposal at https://tc39.github.io/proposal-class-fields/#runtime-semantics-class-public-field-definition-evaluation, so we soon have three places which could use a single shared definition.


I haven't verified if this works, but the quick'n dirty approach to get this working may be to add a wrapper around AssignmentExpression, so we get:

FunkyAssignmentExpression[In, Yield, Await]:
  AssignmentExpression[In?, Yield?, Await?]

ConciseBody[In]:
  [lookahead ≠ {] FunkyAssignmentExpression[?In, ~Yield, ~Await]
  { FunctionBody[~Yield, ~Await] }

AsyncConciseBody[In]:
  [lookahead ≠ {] FunkyAssignmentExpression[?In, ~Yield, +Await]
  { AsyncFunctionBody }

FieldDefinition[Yield, Await]:
  ClassElementName[?Yield, ?Await] FieldInitializer opt

FieldInitializer:
  = FunkyAssignmentExpression[In, ~Yield, ~Await]

with:

Runtime Semantics: Evaluation

FunkyAssignmentExpression: AssignmentExpression
1. Let exprRef be the result of evaluating AssignmentExpression.
2. Let exprValue be ? GetValue(exprRef).
3. Return Completion { [[Type]]: return, [[Value]]: exprValue, [[Target]]: empty }. 

@TimothyGu TimothyGu changed the title Editorial: Fix return value logic in AsyncFunctionStart Editorial: Fix return value logic for async arrow functions Feb 9, 2019
@TimothyGu
Copy link
Member Author

@anba I went with your approach. @tc39/ecma262-editors please take a look.

TimothyGu added a commit to engine262/engine262 that referenced this pull request Feb 9, 2019
@ljharb ljharb requested a review from waldemarhorwat February 9, 2019 05:54
@ljharb ljharb added the es2019 label Feb 10, 2019
@bterlson bterlson removed the es2019 label Feb 28, 2019
@ljharb ljharb requested review from zenparsing and ljharb and removed request for bterlson March 8, 2019 08:25
@ljharb ljharb self-assigned this May 1, 2019
@ljharb
Copy link
Member

ljharb commented May 2, 2019

@TimothyGu can you rebase this and resolve the conflicts?

@TimothyGu TimothyGu force-pushed the asyncfunctionstart branch from 2022cb9 to 1feb3d8 Compare May 2, 2019 22:05
@TimothyGu
Copy link
Member Author

@ljharb Rebased

@ljharb ljharb removed their assignment May 2, 2019
targos pushed a commit to engine262/engine262 that referenced this pull request Jun 26, 2019
@ljharb
Copy link
Member

ljharb commented Aug 29, 2019

ping @waldemarhorwat

@ljharb ljharb requested a review from syg October 21, 2019 23:27
@ljharb ljharb requested a review from bakkot November 9, 2019 04:25
spec.html Outdated
</emu-grammar>
<emu-alg>
1. Return the result of EvaluateBody of |AsyncFunctionBody| passing _functionObject_ and _argumentsList_ as the arguments.
</emu-alg>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Musing: I wonder if we should have ecmarkup auto-generate chain production expansions somehow so the reader can get a clickable link for the RHS.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theoretically possible, but I think it would go substantially beyond ecmarkup's current capabilities just to deduce the chain rules that have been left implicit.

@ljharb ljharb self-assigned this Jan 23, 2020
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rubberstamp based on existing approvals.

Introduce a new production ExpressionBody for both plain and async arrow
functions, and allow correctly handling return value logic for both.
Currently, AsyncFunctionStart does not correctly handle the return value
of async arrow functions correctly.
@ljharb ljharb force-pushed the asyncfunctionstart branch from 1feb3d8 to a879fd5 Compare January 23, 2020 07:19
@ljharb ljharb merged commit a879fd5 into tc39:master Jan 23, 2020
@TimothyGu TimothyGu deleted the asyncfunctionstart branch January 23, 2020 07:48
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Jan 23, 2020
- Fix a 'use' production for AsyncConciseBody
  to match the new defining production.

- Add an <emu-prodref> for ExpressionBody to Annex A.
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Feb 4, 2020
- Fix a 'use' production for AsyncConciseBody
  to match the new defining production.

- Add an <emu-prodref> for ExpressionBody to Annex A.
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Feb 5, 2020
- Fix a 'use' production for AsyncConciseBody
  to match the new defining production.

- Add an <emu-prodref> for ExpressionBody to Annex A.
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Feb 13, 2020
- Fix a 'use' production for AsyncConciseBody
  to match the new defining production.

- Add an <emu-prodref> for ExpressionBody to Annex A.
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Feb 13, 2020
- Fix a 'use' production for AsyncConciseBody
  to match the new defining production.

- Add an <emu-prodref> for ExpressionBody to Annex A.
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Feb 15, 2020
- Fix a 'use' production for AsyncConciseBody
  to match the new defining production.

- Add an <emu-prodref> for ExpressionBody to Annex A.
ljharb pushed a commit to jmdyck/ecma262 that referenced this pull request Feb 19, 2020
 - insert space before paren in clause heading
 - eliminate grammatical parameters from non-defining production
 - Add 4 <emu-prodref> elements to Annex A
 - tweak syntax in algorithms
 - eliminate accidental ambiguity in Punctuator
      PR tc39#1646 introduced an ambiguity to the lexical grammar:
      Punctuator derives OtherPunctuator in two different ways --
      directly, and indirectly via OptionalChainingPunctuator.

      It doesn't make sense for OptionalChainingPunctuator to derive OtherPunctuator,
      so I've eliminated that alternative.
 - minor grammar refactoring re Punctuator
      Move the Punctuator production back to its former position.
      (Grammars are generally written top-down.)
 - Fix a 'use' production for AsyncConciseBody to match the new defining production
 - Add an <emu-prodref> for ExpressionBody to Annex A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants