await AST node for expressions #7368
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Syntax Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.05.
| Benchmark suite | Current: de28573 | Previous: 3f06468 | Ratio |
|---|---|---|---|
Parse RedBlackTree.res - time/run |
1.84985202 ms |
1.3461563799999998 ms |
1.37 |
Print RedBlackTree.res - time/run |
3.0524644133333334 ms |
1.8942094533333333 ms |
1.61 |
Print RedBlackTreeNoComments.res - time/run |
2.9472621 ms |
1.7506641333333335 ms |
1.68 |
Parse Napkinscript.res - time/run |
65.18005879333333 ms |
42.33869618 ms |
1.54 |
Print Napkinscript.res - time/run |
110.03848714 ms |
57.47105978 ms |
1.91 |
Parse HeroGraphic.res - time/run |
8.063686846666666 ms |
5.736426893333333 ms |
1.41 |
Print HeroGraphic.res - time/run |
14.140058206666666 ms |
7.840572506666666 ms |
1.80 |
This comment was automatically generated by workflow using github-action-benchmark.
| @@ -29,11 +29,11 @@ user.data = await fetch() | |||
|
|
|||
| <Navbar promise={await gc()}> {await weirdReactSuspenseApi} </Navbar> | |||
|
|
|||
| let inBinaryExpression = await x->Js.Promise.resolve + 1 | |||
| let inBinaryExpression = await x->Js.Promise.resolve + await y->Js.Promise.resolve | |||
| let inBinaryExpression = (await x->Js.Promise.resolve) + 1 | |||
There was a problem hiding this comment.
here in this file in general
There was a problem hiding this comment.
Hmm, I'm not entirely sure about this. I believe that no parentheses are needed if the top-level await expression is followed by a single expression (top-level from the expression point of view):
await xHowever, if it is nested within another expression, it makes sense to use parentheses:
let x = foo((await bar), y, z)
let a = (await b) - (await c)I think the changes in this file are reasonable so far. However, I'm no expert on this, so I would like to ask for input from others as well.
There was a problem hiding this comment.
Looks like the formatting for a simple binary operation is (await 1) + (await 2), so this example looks odd.
I think what's going on before this PR is that by adding await as an attribute, each sub-expressions is processed not as an await expression, but as a -> expression, as the checks go straight through the ast.
In general (await e1) + (await e2) should probably not look inside e1 and e2 for deciding how to parenthesise. So it seems the formatting before this PR might not have been intentional.
There was a problem hiding this comment.
However, if it is nested within another expression, it makes sense to use parentheses:
Agreed, this makes things clearer. The formatting changes are fine with me.
| @@ -88,7 +88,7 @@ let _ = await (assert(x)) | |||
| let _ = await promises[0] | |||
| let _ = await promises["resolved"] | |||
| let _ = await promises["resolved"] = sideEffect() | |||
| let _ = @attr await (expr) | |||
| let _ = @outer await (@inner expr) | |||
There was a problem hiding this comment.
Update test to showcase the difference between attributes outside and inside the await expression.
This distinction was not possible before adding an ast node for await.
tests/tools_tests/test.sh
Outdated
| @@ -9,7 +9,7 @@ done | |||
|
|
|||
| for file in ppx/*.res; do | |||
| output="src/expected/$(basename $file).jsout" | |||
| ../../cli/bsc -ppx "../../_build/install/default/bin/rescript-tools ppx" $file > $output | |||
| node ../../cli/bsc.js -ppx "../../_build/install/default/bin/rescript-tools ppx" $file > $output | |||
There was a problem hiding this comment.
@cometkim is this the way to call bsc now?
| @@ -29,11 +29,11 @@ user.data = await fetch() | |||
|
|
|||
| <Navbar promise={await gc()}> {await weirdReactSuspenseApi} </Navbar> | |||
|
|
|||
| let inBinaryExpression = await x->Js.Promise.resolve + 1 | |||
| let inBinaryExpression = await x->Js.Promise.resolve + await y->Js.Promise.resolve | |||
| let inBinaryExpression = (await x->Js.Promise.resolve) + 1 | |||
There was a problem hiding this comment.
However, if it is nested within another expression, it makes sense to use parentheses:
Agreed, this makes things clearer. The formatting changes are fine with me.
No description provided.