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

Remove await from current minimal proposal spec #108

Merged
merged 1 commit into from
Mar 18, 2018

Conversation

mAAdhaTTah
Copy link
Collaborator

I just removed what enabled it. I think this is all thats needed on the spec side, unless there should be some explicit indication of a SyntaxError here?

@js-choi
Copy link
Collaborator

js-choi commented Mar 17, 2018

Promises can resolve into functions. We’re changing the original/minimal proposal such that value |> await promiseThatResolvesToFunction is a syntax error, right? (Also, what about value |> (await promiseThatResolvesToFunction), value |> (await promiseThatResolvesToFunction, ), etc.?)

@ljharb
Copy link
Member

ljharb commented Mar 17, 2018

Parenthesizing it should work fine, provided that the promise resolves to a function.

@js-choi
Copy link
Collaborator

js-choi commented Mar 17, 2018

@ljharb: But without parentheses, it would be an error, under the current plan (babel/proposals#29 (comment), TC39 presentation), right?

If so, then a supplemental grammar (covering unparenthesized awaited expressions immediately after |>) and an early error (which uses the supplemental grammar) need to also be added, if I understand the situation correctly.

@zenparsing
Copy link
Member

We need to be very careful about requiring parenthesis for the purpose of disambiguating semantics (as opposed to merely disambiguating precedence). It might be a smell.

@js-choi
Copy link
Collaborator

js-choi commented Mar 17, 2018

@zenparsing: It is a smell, but it’s a fundamental part of how Proposal 1 currently addresses await. That’s why smart pipelines accommodate arbitrary expressions—it doesn’t have any silent footguns with required parentheses, though of course it may its own disadvantages.

See also the bottom paragraphs of @tabatkins’ comment at tc39/proposal-smart-pipelines#3 (comment) and also tc39/proposal-smart-pipelines#3 (comment). There may be ways to ameliorate this smell for Proposal 1, as we keep developing all the options. We’re doing it this way for now to compare everything.

@mAAdhaTTah
Copy link
Collaborator Author

mAAdhaTTah commented Mar 17, 2018

Just to be clear, this isn't really intended as the final semantics of the pipeline proposal, but we can be certain that this behavior is not final. This makes await an early error so we can bring the babel plugin's default behavior into alignment with this spec and allows each proposal to solve it as best for them.

@js-choi
Copy link
Collaborator

js-choi commented Mar 17, 2018

@mAAdhaTTah: Making sure sure: Only unparenthesized await right after |> is an early error, right?

We still need to add that supplemental grammar and early error to this pull request. If you are uncertain how to do that, ping me on the Babel slack; or I could try to take care of it later, after I handle some stuff with smart pipelines’ explainer/spec and the presentation. Thanks again for handling this.

@mAAdhaTTah
Copy link
Collaborator Author

To provide some additional context, here is the conversation where @littledan made this suggestion. I'm thinking about the "minimal proposal" as a strawman we can use to compare the tradeoffs of the various proposals.


@js-choi That's the intention, yes.

@js-choi
Copy link
Collaborator

js-choi commented Mar 17, 2018

I think that we should consider making await forbidden at all in the original/minimal proposal’s pipeline bodies.

That is, for the production |PipelineExpression| : |LogicalORExpression|, “It is an early Syntax Error if |LogicalORExpression| Contains await.” (Note that this still allows await inside inner async functions because those hide their inner awaits from the Contains static semantic rule.)

This would make a supplemental covering production unnecessary. It would also prevent the inconsistency of x |> await being invalid in the original/minimal proposal but x |> (await) being valid.

@mAAdhaTTah mAAdhaTTah force-pushed the ban-await branch 2 times, most recently from 8127eb7 to 5e936b4 Compare March 18, 2018 00:14
@mAAdhaTTah
Copy link
Collaborator Author

@js-choi x |> (await) desugars to (await)(x), which is a SyntaxError.

I updated to include the early error verbage.

@js-choi
Copy link
Collaborator

js-choi commented Mar 18, 2018

@mAAdhaTTah: Great, though you’ll want to use the Contains static semantic rule, rather than “appears”, and it should go in its own Early Errors subsection. Though this is probably good enough for now for a sketch, so we’ll deal with that later.

@littledan
Copy link
Member

If we wanted to be literally subset of the intersection both proposals, we would have to prohibit several other constructs as well. Given that we are not doing that, I don't see why to prohibit any more await cases.

spec.html Outdated
@@ -35,10 +35,10 @@ <h1>Syntax</h1>
PipelineExpression[In, Yield, Await] :
LogicalORExpression[?In, ?Yield, ?Await]
[~Await] PipelineExpression[?In, ?Yield, ?Await] `|>` LogicalORExpression[?In, ?Yield, ?Await]
[+Await] PipelineExpression[?In, ?Yield, ?Await] `|>` [lookahead &lt;! {`await`}] LogicalORExpression[?In, ?Yield, ?Await]
[+Await] PipelineExpression[?In, ?Yield, ?Await] `|>` `await` LogicalORExpression[?In, ?Yield, ?Await]
Copy link
Member

Choose a reason for hiding this comment

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

Deleting the second line looks good, but deleting the first line makes pipeline not work in async functions at all.

Copy link
Collaborator Author

@mAAdhaTTah mAAdhaTTah Mar 18, 2018

Choose a reason for hiding this comment

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

I'll re-add it, but could you elaborate as to why that's the case? I think I'm misunderstanding the purpose of the lookahead here.

spec.html Outdated
</ins>
</emu-grammar>

It is a Syntax Error if `await` appears in the `LogicalORExpression` the RHS of a `PipelineExpression`.
Copy link
Member

Choose a reason for hiding this comment

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

If you keep the lookahead restriction, it would ban literal await following a pipeline. With that, I don't see why we'd need a syntax error.

Nit: syntax errors tend to be presented in Static Semantics: Early Error blocks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is kinda what I thought but I wasn't sure. I'll remove this, since the wording isn't quite right.

This will now result in an early error when using `|> await f`.
Updates the README as well.
@littledan littledan merged commit da9a5ff into tc39:master Mar 18, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants