Skip to content
This repository has been archived by the owner on Jun 15, 2023. It is now read-only.

await / pipe first precedence #686

Closed
cknitt opened this issue Oct 16, 2022 · 30 comments
Closed

await / pipe first precedence #686

cknitt opened this issue Oct 16, 2022 · 30 comments

Comments

@cknitt
Copy link
Member

cknitt commented Oct 16, 2022

await server->start

is parsed/printed as

(await server)->start

I would have expected it to mean

await (server->start)
@cknitt cknitt added this to the v10.1 "The New Features" milestone Oct 16, 2022
@cknitt
Copy link
Member Author

cknitt commented Oct 16, 2022

Also,

(await (server->start))->ignore

loses the await on reformat:

server->start->ignore

@cristianoc
Copy link
Contributor

This use parseUnaryExpr like other unary expressions.
Some exploration needs to be done about the implications of changing that.

@cknitt
Copy link
Member Author

cknitt commented Oct 16, 2022

My second comment (losing await) is clearly a bug though - shall I put that into a separate issue?

@cristianoc
Copy link
Contributor

My second comment (losing await) is clearly a bug though - shall I put that into a separate issue?

Yes please

@mununki
Copy link
Member

mununki commented Oct 16, 2022

So, what is the precedence between await and pipe-first? await (server->start) is the expected one, pipe-first is greater than await?

@cristianoc
Copy link
Contributor

What about - server->start

@cristianoc
Copy link
Contributor

From googling JS: "An AwaitExpression is a UnaryExpression and has the same precedence as delete, void, typeof, +, -, ~, and !, binding stronger than any binary operator."

We should check that this is true and conform to that.

@cristianoc
Copy link
Contributor

So the real question seems to be about pipe, not about await.

@mununki
Copy link
Member

mununki commented Oct 16, 2022

What about - server->start

Exactly. If we can tell await is a unary operator and pipe-first is a binary operator, negation, await and binary operators are just functions. So, we can say associativity as (-> (await server) start) or (await (-> server start)), both are possible.

@mununki
Copy link
Member

mununki commented Oct 16, 2022

From googling JS: "An AwaitExpression is a UnaryExpression and has the same precedence as delete, void, typeof, +, -, ~, and !, binding stronger than any binary operator."

We should check that this is true and conform to that.

It seems correct.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Operator_Precedence

@cknitt
Copy link
Member Author

cknitt commented Oct 16, 2022

My thinking was that

await server->start

corresponds to a method call in JS:

await server.start();

which is the same as

await (server.start());

@cristianoc
Copy link
Contributor

My thinking was that

await server->start

corresponds to a method call in JS:

await server.start();

which is the same as

await (server.start());

This is a comment about pipe not await.
Someone should check how the priority for pipe was set, and what are the trade-offs.

@IwanKaramazow
Copy link
Contributor

Someone should check how the priority for pipe was set, and what are the trade-offs.

"Organically through some trial and error while trying to fix some Reason problems and it still isn't perfect for all use cases"

@cristianoc
Copy link
Contributor

Anything specifically about unary operators? And in fact binary operators.
As if pipe has a higher priority than unary then it must have higher priority than binary too.

@mununki
Copy link
Member

mununki commented Oct 17, 2022

I didn't look into the parseUnary and parseBinaray in detail yet. But, the precedence of pipe-first is greater than await.

let precedence = function

That means await e1->e2 should be parsed to (await (e1->e2)) if the precedence logic is affected correctly.

@mununki
Copy link
Member

mununki commented Oct 18, 2022

Today I grab some time to look at the parse await, unary, binary expression.
IMHO, there are two strange things to me.

  1. parseAwaitExpression use parseUnaryExpr. Why not use just parseExpr?
  2. await is not affected by precedence rules

@cristianoc
Copy link
Contributor

Today I grab some time to look at the parse await, unary, binary expression. IMHO, there are two strange things to me.

  1. parseAwaitExpression use parseUnaryExpr. Why not use just parseExpr?
  2. await is not affected by precedence rules

It's a unary operator that behaves like other unary operators (- e or lazy e), and the JS standard says it's unary operator.
What's strange about it?

@mununki
Copy link
Member

mununki commented Oct 18, 2022

What I’m thinking strange about is the precendence.

op1 op2 a op3 b

op1, 2, 3 needs to be parsed by the precedence, no matter it is unary or binary. But await is not affected by precedence rule now. It has just parsed expression right next to it.

@cristianoc
Copy link
Contributor

cristianoc commented Oct 18, 2022

What I’m thinking strange about is the precendence.

op1 op2 a op3 b

op1, 2, 3 needs to be parsed by the precedence, no matter it is unary or binary. But await is not affected by precedence rule now. It has just parsed expression right next to it.

If it's parsed differently (with other priority) competed to other unary operators, it's interesting to know.
What are examples?

@cristianoc
Copy link
Contributor

Assuming the discussion ends up on: what's the precedence of pipe.
Here's some TC39 proposal:
https://github.com/tc39/proposal-pipeline-operator

The pipe operator’s precedence is the same as:

the function arrow =>;
the assignment operators =, +=, etc.;
the generator operators yield and yield *;
It is tighter than only the comma operator ,.
It is looser than all other operators.

@mununki
Copy link
Member

mununki commented Oct 19, 2022

Thanks for the clarified reference.
Maybe I have to open another issue for my question. Don't we have to put await in the precedence rule and parse await expression in different way?

@cristianoc
Copy link
Contributor

There's no "precedence rule". The precedences are: whatever the parser gives you.
So the level of precedence is implicit in the code.
I believe await has the same precedence as other unary operators, and higher than for example binary operators.

@cristianoc
Copy link
Contributor

cristianoc commented Oct 19, 2022

Don't we have to put await in the precedence rule and parse await expression in different way?

It's difficult to answer a question like that.
A more concrete comment has this form: here's one expression, here's how it is parsed (the AST), I think it should instead be parsed as ... (another AST).

@mununki
Copy link
Member

mununki commented Oct 19, 2022

There's no "precedence rule". The precedences are: whatever the parser gives you. So the level of precedence is implicit in the code. I believe await has the same precedence as other unary operators, and higher than for example binary operators.

Indeed. I think my expression rule was misread.

A more concrete comment has this form: here's one expression, here's how it is parsed (the AST), I think it should instead be parsed as ... (another AST).

Good! I'll clean up my thought and leave an issue later.

@mununki
Copy link
Member

mununki commented Oct 19, 2022

If the pipe has the same precedence as the arrow =>, the current parsed AST looks correct. await has higher precedence than the pipe and arrow.

await a->b

// to

(await a)->b

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Operator_Precedence

@IwanKaramazow
Copy link
Contributor

Hmm, I'm reading the above as await (a->b).
-> is currently being parsed as a normal binary operator in parseBinaryExpr. We might want to change this behavior.
First step would be gathering all the possible examples.

@cristianoc
Copy link
Contributor

Basically, if one wants to try to make -> behave the same way as application, one can start from parseCallExpr, see how it's triggered by (, and implement a similar behaviour triggered by ->.

This is going to interpret await a->b as await (a->b) just as -f("3") is currently interpreted as -(f("3")).

Then see what the consequences of the change are. (And the printer needs similar changes).

@d4h0
Copy link

d4h0 commented Oct 29, 2022

I don't really understand the binary/unary-stuff, but I believe this should be optimized for what people expect, what the most common use cases are, and for what results in the clearest and easiest to understand code.

To me let result = await object->async_method(arg) looks like an extremely common use case, and requiring await (object->async_method(arg)) seems a bit awful, to be honest.

I'm currently working on a program that uses Playwright pretty heavily, and probably will have hundreds of lines of await object->async_method(arg) (almost every action is triggered via an async method).

On the other hand, await async_function(arg)->handle_result() also doesn't seem great.

(await async_function(arg))->handle_result() would make it clearer and easier to visually parse what is going on, I believe.

Besides that, I would expect that a keyword has a lower precedence than an operator. Is there an example, where a ReScript keyword has higher precedence than an operator?

@cristianoc
Copy link
Contributor

This can be used for testing #711

@cknitt
Copy link
Member Author

cknitt commented Oct 30, 2022

Tested, seems to work fine, except that the printer adds unnecessary parentheses in this case:

(await server->start)->ignore

->

(await (server->start))->ignore

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants