rescript-lang / syntax

ReScript's syntax as a standalone repo.
MIT License
254 stars 38 forks source link

await / pipe first precedence #686

Closed cknitt closed 2 years ago

cknitt commented 2 years ago
await server->start

is parsed/printed as

(await server)->start

I would have expected it to mean

await (server->start)
cknitt commented 2 years ago

Also,

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

loses the await on reformat:

server->start->ignore
cristianoc commented 2 years ago

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

cknitt commented 2 years ago

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

cristianoc commented 2 years ago

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

Yes please

mununki commented 2 years ago

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

cristianoc commented 2 years ago

What about - server->start

cristianoc commented 2 years ago

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 commented 2 years ago

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

mununki commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

I didn't look into the parseUnary and parseBinaray in detail yet. But, the precedence of pipe-first is greater than await. https://github.com/rescript-lang/syntax/blob/d64839e3deae9ecb9a5507b18981292dc6e3ec9f/src/res_token.ml#L100 That means await e1->e2 should be parsed to (await (e1->e2)) if the precedence logic is affected correctly.

mununki commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

This can be used for testing https://github.com/rescript-lang/syntax/pull/711

cknitt commented 2 years ago

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

(await server->start)->ignore

->

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