fix(rome_js_parser): permisive instantiation expression #3359

IWANABETHATGUY · 2022-10-07T11:06:35Z

Summary

Part of 📎 Tracking tasks for implementing Instantiation expressions #3147
Related to More permissive parsing of instantiation expressions microsoft/TypeScript#48659

Test Plan

CI should pass.
New snapshot.

netlify · 2022-10-07T11:06:39Z

✅ Deploy Preview for docs-rometools ready!

Name	Link
🔨 Latest commit	`f3b3447`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-rometools/deploys/63442ba346a0eb00071d848a
😎 Deploy Preview	https://deploy-preview-3359--docs-rometools.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

IWANABETHATGUY · 2022-10-07T11:22:02Z

!bench_parser

github-actions · 2022-10-07T11:43:24Z

Parser Benchmark Results

group                                 main                                   pr
-----                                 ----                                   --
parser/checker.ts                     1.00    145.3±2.69ms    17.9 MB/sec    1.00    145.8±2.70ms    17.8 MB/sec
parser/compiler.js                    1.00     83.3±1.83ms    12.6 MB/sec    1.00     83.5±1.67ms    12.5 MB/sec
parser/d3.min.js                      1.00     48.7±1.17ms     5.4 MB/sec    1.00     48.6±1.07ms     5.4 MB/sec
parser/dojo.js                        1.00      4.1±0.05ms    16.6 MB/sec    1.00      4.1±0.04ms    16.5 MB/sec
parser/ios.d.ts                       1.00    126.8±2.30ms    14.7 MB/sec    1.00    127.2±2.39ms    14.7 MB/sec
parser/jquery.min.js                  1.00     12.7±0.20ms     6.5 MB/sec    1.01     12.9±0.25ms     6.4 MB/sec
parser/math.js                        1.00     99.6±1.79ms     6.5 MB/sec    1.01    100.5±1.75ms     6.4 MB/sec
parser/parser.ts                      1.03      3.0±0.05ms    16.4 MB/sec    1.00      2.9±0.04ms    16.8 MB/sec
parser/pixi.min.js                    1.01     63.5±1.85ms     6.9 MB/sec    1.00     62.7±1.77ms     7.0 MB/sec
parser/react-dom.production.min.js    1.00     17.4±0.27ms     6.6 MB/sec    1.00     17.4±0.26ms     6.6 MB/sec
parser/react.production.min.js        1.00   899.5±14.00µs     6.8 MB/sec    1.01   905.3±13.74µs     6.8 MB/sec
parser/router.ts                      1.00      2.4±0.04ms    25.1 MB/sec    1.01      2.5±0.05ms    25.0 MB/sec
parser/tex-chtml-full.js              1.00    136.7±1.85ms     6.7 MB/sec    1.00    137.2±2.28ms     6.6 MB/sec
parser/three.min.js                   1.00     70.4±1.39ms     8.3 MB/sec    1.01     71.1±1.65ms     8.3 MB/sec
parser/typescript.js                  1.00    593.4±8.25ms    16.0 MB/sec    1.01    596.8±8.12ms    15.9 MB/sec
parser/vue.global.prod.js             1.01     21.4±0.38ms     5.6 MB/sec    1.00     21.3±0.36ms     5.6 MB/sec

IWANABETHATGUY · 2022-10-10T13:09:26Z

@MichaReiser, Would mind helping with reviewing the code?

MichaReiser · 2022-10-10T09:34:36Z

crates/rome_js_parser/src/syntax/typescript/types.rs

+    }
+}
+
+/// You could refer to https://github.com/microsoft/TypeScript/blob/42b1049aee8c655631cb4f0065de86ec1023d20a/src/compiler/parser.ts#L4475


I don't think it makes much sense for us to link to typescript repository inside of the documentation. Instead, we should explain what this method is doing.

I will add some doc about the implementation , but then add the reference link in Typescrip for people who want to see more details, what do you think?

MichaReiser · 2022-10-10T09:34:52Z

crates/rome_js_parser/src/syntax/typescript/types.rs

+}
+
+/// You could refer to https://github.com/microsoft/TypeScript/blob/42b1049aee8c655631cb4f0065de86ec1023d20a/src/compiler/parser.ts#L4475
+fn is_start_of_expr(p: &mut Parser) -> bool {


How's this method different from is_at_expression?

is_start_of_expr has JS_BIG_INT_LITERAL, but is_at_expression don't. is_start_of_expr don't have T![super]

Have we checked if is_at_expression should have JS_BIG_INT_LITERAL and T![super]. Maybe that's a bug.

is_at_expression, Also is_start_of_expr need is_binary_operator for better error recover. I am not sure if we need it too, becuase our error recover maybe different from Typescript

MichaReiser · 2022-10-10T09:37:04Z

crates/rome_js_parser/src/syntax/typescript/types.rs

+        | T![await]
+        | T![yield] => true,
+        // TODO: how to represent private identifier
+        _ => is_binary_operator(p) || is_at_identifier(p),


What's the reason that this method returns true for binary operators. I'm asking because binary operators don't mark the start of an expression and instead appear in the middle of an expresison.

This function is just porting from Typescript, I keep the same behavior as Typescript because this will not bring too much maintenance burden when Typescript changes next time.

The issue I see here is that the method tests if it is at the start of an expression which may not be true for a binary operator. It also introduces a lot of duplication with is_at_expression.

I think it's ok to get inspiration from TypeScript but we should be careful with copying the implementation 1:1. Our parser works similar to TypeScript but isn't an exact rewrite in Rust. Thus, we should only inherit what we explicitly know is necessary.

I have added the reason why we need to add binary_operator. We could merge this function but, it may be not good for reuse, what do you think? Other than that, the pattern match is just against JS_SYNTAX_KIND which should be only u8 under the hood, and it is pretty trivial according to the benchmark.

My issue isn't with performance but with maintainability. As an author, how do I know is I should use is_at_expression or is_start_of_expr. So far, is_at_expression should always return true for anything that parse_expression returns Present which I think is an understandable definition. But I know wouldn't know if I have to use this new method or the old one.

That's why I think it's important to clearly document when to use which method and why. If there's no good reason, then the two should be merged together.

Just chiming in.

The decision here is to whether parse a plain && rhs to a missing lhs logical expression or not in Rome:

JsLogicalExpression { left: missing (required), operator_token: AMP2@2..5 "&&" [] [Whitespace(" ")], right: ... }

But regardless of the final decision, it shouldn't be introduced in a feature PR because it needs separate tests.

Also note, almost every kind of expression is a binary expression in TypeScript, so copying exact implementations from TypeScript will end up with a very different outcome.
(I just noticed the other day, assignments and sequence expressions are all binary expressions in TypeScript.)

@MichaReiser What do you think ? Do we need to rewrite the code, make it fixes this test case, and ignore the rest logic in Typescript, fix them one by one when we suffer them?

I think we need to understand what this logic accomplishes and if we need both is_*_expression method and if so why, document it and explain when to use which.

My preferred solution would be to only have one (unparametrized) method in the end.

MichaReiser

I think it's necessary to either make a very clear distinction between is_start_of_expr and is_at_expression but ideally, only have one.

I think we should also be more explicit in the PR description explaining which new cases this PR handles that the parser currently does not and add positive and negative tests.

github-actions · 2022-10-25T12:05:08Z

This PR is stale because it has been open 14 days with no activity.

chore: 🤖 finish

022f67f

IWANABETHATGUY requested review from MichaReiser and xunilrj as code owners October 7, 2022 11:06

chore: 🤖 refactor

e9ba015

MichaReiser reviewed Oct 10, 2022

View reviewed changes

IWANABETHATGUY added 2 commits October 10, 2022 22:16

chore: 🤖 add some doc

90b759b

chore: 🤖 more doc

f3b3447

MichaReiser suggested changes Oct 11, 2022

View reviewed changes

github-actions bot added the S-Stale label Oct 25, 2022

MichaReiser closed this Nov 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rome_js_parser): permisive instantiation expression #3359

fix(rome_js_parser): permisive instantiation expression #3359

IWANABETHATGUY commented Oct 7, 2022 •

edited

Loading

netlify bot commented Oct 7, 2022 •

edited

Loading

IWANABETHATGUY commented Oct 7, 2022

github-actions bot commented Oct 7, 2022

IWANABETHATGUY commented Oct 10, 2022

MichaReiser Oct 10, 2022

IWANABETHATGUY Oct 10, 2022

MichaReiser Oct 10, 2022

IWANABETHATGUY Oct 10, 2022

MichaReiser Oct 10, 2022

IWANABETHATGUY Oct 10, 2022

MichaReiser Oct 10, 2022

IWANABETHATGUY Oct 10, 2022

MichaReiser Oct 10, 2022

IWANABETHATGUY Oct 10, 2022

MichaReiser Oct 10, 2022

Boshen Oct 10, 2022 •

edited

Loading

IWANABETHATGUY Oct 11, 2022

MichaReiser Oct 11, 2022

MichaReiser left a comment

github-actions bot commented Oct 25, 2022

fix(rome_js_parser): permisive instantiation expression #3359

fix(rome_js_parser): permisive instantiation expression #3359

Conversation

IWANABETHATGUY commented Oct 7, 2022 • edited Loading

Summary

Test Plan

netlify bot commented Oct 7, 2022 • edited Loading

✅ Deploy Preview for docs-rometools ready!

IWANABETHATGUY commented Oct 7, 2022

github-actions bot commented Oct 7, 2022

Parser Benchmark Results

IWANABETHATGUY commented Oct 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Boshen Oct 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaReiser left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 25, 2022

IWANABETHATGUY commented Oct 7, 2022 •

edited

Loading

netlify bot commented Oct 7, 2022 •

edited

Loading

Boshen Oct 10, 2022 •

edited

Loading