feat(rome_js_parser): instantiation expressions #3152

IWANABETHATGUY · 2022-09-03T10:52:11Z

Summary

Part of 📎 Tracking tasks for implementing Instantiation expressions #3147
Finishing the first task, align to the implementation of Typescript Instantiation expressions microsoft/TypeScript#47607
Adding this extra SyntaxKind TsExpressionWithTypeArguments to align typescript AST shape.
Modifying the shape of TsTypeOfType,adding optional type_arguments,
now, the TsTypeOfType could be written like:
```
type T21 = typeof Array<string>;
```
The newly added SyntaxKind is used to support Typescript instantiation expression, also it would be sometimes
ambiguous when try to parse

template_literal inside member_expression_rest https://github.com/rome/tools/pull/3152/files#diff-1ca6d1ff770e933910c550807255d1bf03cdebbf60bf469579b528c0c28c1537R706-R709
new_expr https://github.com/rome/tools/pull/3152/files#diff-1ca6d1ff770e933910c550807255d1bf03cdebbf60bf469579b528c0c28c1537R735-R769
call_expr_rest https://github.com/rome/tools/pull/3152/files#diff-1ca6d1ff770e933910c550807255d1bf03cdebbf60bf469579b528c0c28c1537R1653-R1673

Newly Supported Syntax

let f1 = fx<string>;
let f2 = fx<string, number>;
let f3 = fx['test']<string>;
const a2 = f.g<number>;  // () => number
const a3 = f<number>.g;  // <U>() => U
const a4 = f<number>.g<number>;  // () => number
const a5 = f['g']<number>;  // () => number
const a7 = (f<number>)['g'];
const a6 = f<number>['g'];  // type Error
const b2 = f?.<number>();
const b3 = f<number>?.();
const b4 = f<number>?.<number>();  // Type Error, expected no type arguments
const x1 = f<true>
(true);
// Parsed as relational expression
const x2 = f<true>
true;
// Parsed as instantiation expression
const x3 = f<true>;
true;

test ts ts_type_instantiation_expression
type StringBox = Box<string>;

test_err ts ts_instantiation_expressions1
const a8 = f<number><number>;  // Relational operator error
const b1 = f?.<number>;  // Error, `(` expected

Test Plan

CI should pass
Checking the newly added snapshot

Prettier Compatibility

Main

Average compatibility: 85.67
Compatible lines: 84.09

PR

Average compatibility: 85.81
Compatible lines: 84.10

netlify · 2022-09-03T10:52:16Z

✅ Deploy Preview for docs-rometools ready!

Name	Link
🔨 Latest commit	`487ba17`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-rometools/deploys/631fed3d7082f70009e41b54
😎 Deploy Preview	https://deploy-preview-3152--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-09-05T08:34:55Z

!bench_parser

github-actions · 2022-09-05T08:53:37Z

Parser Benchmark Results

group                                 main                                   pr
-----                                 ----                                   --
parser/checker.ts                     1.02    134.3±4.69ms    19.4 MB/sec    1.00    132.0±6.21ms    19.7 MB/sec
parser/compiler.js                    1.00     79.8±3.98ms    13.1 MB/sec    1.06     84.7±2.63ms    12.4 MB/sec
parser/d3.min.js                      1.00     45.3±1.96ms     5.8 MB/sec    1.04     47.1±1.65ms     5.6 MB/sec
parser/dojo.js                        1.00      4.0±0.01ms    17.4 MB/sec    1.00      4.0±0.02ms    17.3 MB/sec
parser/ios.d.ts                       1.01    110.8±2.46ms    16.8 MB/sec    1.00    109.9±3.01ms    17.0 MB/sec
parser/jquery.min.js                  1.00     11.8±0.12ms     7.0 MB/sec    1.01     11.9±0.09ms     6.9 MB/sec
parser/math.js                        1.00     90.4±2.96ms     7.2 MB/sec    1.00     90.8±2.44ms     7.1 MB/sec
parser/parser.ts                      1.00      2.8±0.00ms    17.7 MB/sec    1.01      2.8±0.01ms    17.5 MB/sec
parser/pixi.min.js                    1.00     55.5±1.09ms     7.9 MB/sec    1.02     56.6±1.39ms     7.8 MB/sec
parser/react-dom.production.min.js    1.00     16.5±0.44ms     7.0 MB/sec    1.00     16.5±0.21ms     7.0 MB/sec
parser/react.production.min.js        1.00    882.7±3.32µs     7.0 MB/sec    1.00    880.7±0.53µs     7.0 MB/sec
parser/router.ts                      1.00      2.4±0.01ms    26.0 MB/sec    1.00      2.4±0.00ms    25.9 MB/sec
parser/tex-chtml-full.js              1.00    119.4±2.68ms     7.6 MB/sec    1.03    123.1±3.58ms     7.4 MB/sec
parser/three.min.js                   1.00     62.0±2.38ms     9.5 MB/sec    1.02     63.2±1.92ms     9.3 MB/sec
parser/typescript.js                  1.00   543.9±16.68ms    17.5 MB/sec    1.02   555.1±18.29ms    17.1 MB/sec
parser/vue.global.prod.js             1.00     20.3±0.32ms     5.9 MB/sec    1.02     20.6±0.46ms     5.8 MB/sec

IWANABETHATGUY · 2022-09-08T14:50:22Z

xtask/codegen/js.ungram

@@ -316,6 +316,7 @@ JsAnyExpression =
 	| TsTypeAssertionExpression


Adding this extra SyntaxKind to align typescript AST shape. Also, this is necessary for instantiation expression due to instantiation expression parsing sometimes needs an intermediate AST which may reparse in different contexts.

https://www.typescriptlang.org/play?#code/PTAEAEGcBcCcEsDG0Bco4FcCmAoHATLRAGwENYtREB7AOxlGoEY0NbCAzeWrfUAH1ABvUACM0MBLQDmoAL4BuPDXrRQADwDMoALygOAHkxYAfAqA

IWANABETHATGUY · 2022-09-08T14:58:20Z

!bench_parser

MichaReiser · 2022-09-08T15:12:46Z

Would you mind extening the PR summary by:

Adding some examples of the syntax that is supported now
New AST nodes added and where they are being used.
Mention of breaking AST changes (ExpressionWithArguments was used in other contexts before)

github-actions · 2022-09-08T15:16:33Z

Parser Benchmark Results

group                                 main                                   pr
-----                                 ----                                   --
parser/checker.ts                     1.00    118.6±5.58ms    21.9 MB/sec    1.02    120.8±7.32ms    21.5 MB/sec
parser/compiler.js                    1.00     74.3±3.73ms    14.1 MB/sec    1.03     76.8±3.77ms    13.7 MB/sec
parser/d3.min.js                      1.00     43.2±2.07ms     6.1 MB/sec    1.00     43.3±2.12ms     6.0 MB/sec
parser/dojo.js                        1.00      3.9±0.15ms    17.4 MB/sec    1.00      3.9±0.16ms    17.4 MB/sec
parser/ios.d.ts                       1.02    104.8±4.69ms    17.8 MB/sec    1.00    103.0±5.08ms    18.1 MB/sec
parser/jquery.min.js                  1.00     11.7±0.55ms     7.0 MB/sec    1.01     11.9±0.50ms     6.9 MB/sec
parser/math.js                        1.00     82.5±4.12ms     7.9 MB/sec    1.02     84.4±4.72ms     7.7 MB/sec
parser/parser.ts                      1.00      2.8±0.09ms    17.2 MB/sec    1.02      2.9±0.12ms    16.9 MB/sec
parser/pixi.min.js                    1.01     51.4±2.99ms     8.5 MB/sec    1.00     51.0±2.07ms     8.6 MB/sec
parser/react-dom.production.min.js    1.01     16.6±0.76ms     6.9 MB/sec    1.00     16.4±0.63ms     7.0 MB/sec
parser/react.production.min.js        1.02   905.2±44.27µs     6.8 MB/sec    1.00   890.8±50.91µs     6.9 MB/sec
parser/router.ts                      1.03      2.4±0.09ms    25.1 MB/sec    1.00      2.4±0.10ms    26.0 MB/sec
parser/tex-chtml-full.js              1.00    111.6±6.68ms     8.2 MB/sec    1.02    113.5±7.13ms     8.0 MB/sec
parser/three.min.js                   1.00     58.1±3.22ms    10.1 MB/sec    1.00     58.1±3.28ms    10.1 MB/sec
parser/typescript.js                  1.02   517.1±17.42ms    18.4 MB/sec    1.00   504.8±20.90ms    18.8 MB/sec
parser/vue.global.prod.js             1.00     19.7±0.88ms     6.1 MB/sec    1.03     20.3±1.18ms     5.9 MB/sec

crates/rome_js_parser/src/syntax/expr.rs

IWANABETHATGUY · 2022-09-08T15:21:40Z

crates/rome_js_parser/src/syntax/expr.rs

+        .or_add_diagnostic(p, expected_expression)
+        .map(|expr| parse_member_expression_rest(p, expr, context, false, &mut false))
+    {
+        if let TS_EXPRESSION_WITH_TYPE_ARGUMENTS = lhs.kind() {


The reason are similar with https://github.com/rome/tools/pull/3152/files#r966104100

IWANABETHATGUY · 2022-09-08T15:23:44Z

crates/rome_js_parser/src/syntax/expr.rs

        if !matches!(p.cur(), T![?.] | T![<] | T![<<] | T!['(']) {
            break lhs;
        }

        // Cloning here is necessary because parsing out the type arguments may rewind in which
        // case we want to return the `lhs`.
-        let m = lhs.clone().precede(p);
+        let m = match lhs.kind() {


The corresponding logic of Typescript you could reference https://github.dev/microsoft/TypeScript/blob/be4e9bac8ff66174d4e856dae06f69aa4ef7e479/src/compiler/parser.ts#L5956-L5958

crates/rome_js_parser/src/syntax/expr.rs

IWANABETHATGUY · 2022-09-08T15:33:03Z

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

@@ -1255,6 +1289,38 @@ pub(crate) fn parse_ts_type_arguments_in_expression(p: &mut Parser) -> ParsedSyn
    .unwrap_or(Absent)
 }

+#[inline]
+pub fn can_follow_type_arguments_in_expr(cur_kind: JsSyntaxKind) -> bool {


Corresponding to https://github.com/microsoft/TypeScript/pull/47607/files#diff-12a6724be007eb1a19d80018c7a63bbc73525ca793a9b3e5da49a4e86bbf457cR5550-R5584

IWANABETHATGUY · 2022-09-08T15:41:15Z

ExpressionWithArguments

You said that

ExpressionWithArguments was used in other contexts before

But I can't search it through the codebase.

IWANABETHATGUY · 2022-09-08T15:48:35Z

Would you mind extening the PR summary by:

Adding some examples of the syntax that is supported now

New AST nodes added and where they are being used.

Mention of breaking AST changes (ExpressionWithArguments was used in other contexts before)

Done

MichaReiser

The PR overall is looking good to me. But I do have a few follow up questions:

ExpressionWithTypeArguments: My understanding is that this is a new expression type and represents instantiation expressions. What's your thinking behind calling it ExpressionWithTypeArguments instead of TsInstantiationExpression? Are there other places where it should be used that aren't instantiation expressions?
ExpressionWithTypeArguments should end in Expression to align with our naming convention that all expressions are named ...Expression: JsCallExpression, JsIdentifierExpression, ...

crates/rome_js_formatter/src/ts/types/typeof_type.rs

crates/rome_js_parser/src/syntax/expr.rs

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

xtask/codegen/js.ungram

IWANABETHATGUY · 2022-09-10T07:57:00Z

ExpressionWithTypeArguments: My understanding is that this is a new expression type and represents instantiation expressions. What's your thinking behind calling it ExpressionWithTypeArguments instead of TsInstantiationExpression? Are there other places where it should be used that aren't instantiation expressions?

I do agree with you, but ExpressionWithTypeArguments is not the newly added SyntaxKind in Typescript parser, we don't have this syntaxKind because our parser is not a 100% port of Typescript. I add this new SyntaxKind because ExpressionWithTypeArguments are widely used in parsing TsInstantiationExpression (mostly as a middle SyntaxKind, maybe split in different contexts). I could give two solution
- Aligning to typescript, keep the ExpressionWithTypeArguments as is, (cons: violate our naming convention)
- Rename it to TsInstantiationExpression, this may increase the burden of mind of the maintainer when porting typescript in the future
  (maybe we could add some comments.)

MichaReiser · 2022-09-10T08:55:00Z

ExpressionWithTypeArguments: My understanding is that this is a new expression type and represents instantiation expressions. What's your thinking behind calling it ExpressionWithTypeArguments instead of TsInstantiationExpression? Are there other places where it should be used that aren't instantiation expressions?
1. I do agree with you, but `ExpressionWithTypeArguments` is not the newly added `SyntaxKind` in `Typescript` parser, we don't have this `syntaxKind` because our parser is not a 100% port of Typescript. I add this new `SyntaxKind` because `ExpressionWithTypeArguments` are widely used in parsing `TsInstantiationExpression` (mostly as a middle SyntaxKind, maybe split in different contexts). I could give two solution
   
   * Aligning to typescript, keep the `ExpressionWithTypeArguments` as is, (cons: violate our naming convention)
   * Rename it to `TsInstantiationExpression`, this may increase the burden of mind of the maintainer when porting typescript in  the future
     (maybe we could add some comments.)

I'm in favour of renaming the node to TsInstantiationExpression for the following reasons:

It aligns with our *Expression naming convention
I can google for typescript instantiation expression and find resources about the syntax. Searching for expressions with type arguments doesn't yield any useful search results.
TypeScript may mainly use the ExpressionWithTypeArguments name for historical reasons. It used to be an existing node for them. We don't have this historical context and can, therefore, more freely pick a new.
Our AST already differs in many places from TypeScript's because of our decision to use a CST and explicitly model required children. Being compatible with existing AST's hasn't been a goal yet, which is why I consider it OK to diverge from TypeScript.

We can ease the transition for people familiar with TypeScript's AST by adding a comment to TsInstantiationExpression in js.ungram that this is the same as TypeScript's ExpressionWithTypeArguments

IWANABETHATGUY · 2022-09-10T09:18:24Z

Done d2d9e60

MichaReiser

Thank you for the updates.

I think we're good to merge this PR after you included the TsTypeofType changes in the PR summary, add a new test for a TsTypeofType with a line break before the following <, and once the NeedsParentheses implementation has been double checked in which cases parentheses are necessary.

crates/rome_js_formatter/src/ts/types/typeof_type.rs

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

IWANABETHATGUY · 2022-09-12T15:40:12Z

Let me explain these two commits:
f45c26f
79d2af3

IWANABETHATGUY · 2022-09-12T16:04:28Z

crates/rome_js_formatter/src/ts/expressions/instantiation_expression.rs

+
+impl NeedsParentheses for TsInstantiationExpression {
+    fn needs_parentheses_with_parent(&self, parent: &rome_js_syntax::JsSyntaxNode) -> bool {
+        unary_like_expression_needs_parentheses(self.syntax(), parent)


Why I write the `needs_parenthesis_with_parent like this?

here is how typescript parse PostUpdateExpression https://github.dev/microsoft/TypeScript/blob/77374732df82c9d5c1319677dc595868bbc648b5/src/compiler/parser.ts#L5345-L5354, almost the same as how they parse ExpressionWithTypeArguments https://github.dev/microsoft/TypeScript/blob/77374732df82c9d5c1319677dc595868bbc648b5/src/compiler/parser.ts#L7504-L7514.

So I think the needs_parentheses_with_parent should be the same as JsPostUpdateExpression

tools/crates/rome_js_formatter/src/js/expressions/post_update_expression.rs

Lines 26 to 30 in 1d9d7a0

impl NeedsParentheses for JsPostUpdateExpression {

fn needs_parentheses_with_parent(&self, parent: &JsSyntaxNode) -> bool {

unary_like_expression_needs_parentheses(self.syntax(), parent)

}

}

IWANABETHATGUY · 2022-09-12T16:05:37Z

@MichaReiser , All issues have been addressed.

IWANABETHATGUY force-pushed the feat/instantiation_expressions branch from 4db4867 to 7968e4a Compare September 8, 2022 14:33

IWANABETHATGUY marked this pull request as ready for review September 8, 2022 14:36

IWANABETHATGUY requested review from MichaReiser, xunilrj and ematipico as code owners September 8, 2022 14:36

IWANABETHATGUY requested a review from a team September 8, 2022 14:36

IWANABETHATGUY commented Sep 8, 2022

View reviewed changes

crates/rome_js_parser/src/syntax/expr.rs Show resolved Hide resolved

IWANABETHATGUY commented Sep 8, 2022

View reviewed changes

crates/rome_js_parser/src/syntax/expr.rs Outdated Show resolved Hide resolved

IWANABETHATGUY commented Sep 8, 2022

View reviewed changes

MichaReiser reviewed Sep 9, 2022

View reviewed changes

IWANABETHATGUY force-pushed the feat/instantiation_expressions branch from 5279313 to d2d9e60 Compare September 10, 2022 09:18

IWANABETHATGUY requested review from MichaReiser and removed request for xunilrj and ematipico September 10, 2022 10:02

MichaReiser reviewed Sep 10, 2022

View reviewed changes

crates/rome_js_formatter/src/ts/types/typeof_type.rs Show resolved Hide resolved

crates/rome_js_parser/src/syntax/typescript/types.rs Show resolved Hide resolved

IWANABETHATGUY added 20 commits September 12, 2022 23:49

chore: 🤖 update snapshot

4301c23

chore: 🤖 update snapshot

b4818b7

chore: 🤖 update snapshot

db31173

chore: 🤖 update snapshot

76fa9b2

chore: 🤖 update snapshot

ff7c729

chore: 🤖 udpate snapshot

2d96604

test: 💍 new operator

c4ceae4

test: 💍 update test snapshot

6a6f46a

feat: 🎸 js format

5817eb9

fix: 🐛 lint

9818ceb

chore: 🤖 lint

706466a

chore: 🤖 fmt

2184ba1

chore: 🤖 clean up

e9acc5b

fix: 🐛 change expected_node to expected_token

a1a4196

chore: 🤖 cr issues

4ce75ae

chore: 🤖 rename ExpressionWithArguments to TsInstantiationExpr

10b49e5

fix: 🐛 cr issues

013532e

feat: 🎸 checkpoint

08f01a4

feat: 🎸 more strict constraint

3508048

chore: 🤖 fmt

2645de4

IWANABETHATGUY force-pushed the feat/instantiation_expressions branch from 46c4a8b to 2645de4 Compare September 12, 2022 15:50

chore: 🤖 compile error

daf8695

IWANABETHATGUY commented Sep 12, 2022

View reviewed changes

fix: 🐛 remove lefthandsideexpression

487ba17

MichaReiser approved these changes Sep 13, 2022

View reviewed changes

MichaReiser merged commit 5654fae into rome:main Sep 13, 2022

MichaReiser added the A-Parser Area: parser label Sep 13, 2022

MichaReiser added this to the 0.10.0 milestone Sep 13, 2022

IWANABETHATGUY deleted the feat/instantiation_expressions branch September 13, 2022 05:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rome_js_parser): instantiation expressions #3152

feat(rome_js_parser): instantiation expressions #3152

IWANABETHATGUY commented Sep 3, 2022 •

edited

Loading

netlify bot commented Sep 3, 2022 •

edited

Loading

IWANABETHATGUY commented Sep 5, 2022

github-actions bot commented Sep 5, 2022

IWANABETHATGUY Sep 8, 2022

IWANABETHATGUY Sep 8, 2022

IWANABETHATGUY commented Sep 8, 2022

MichaReiser commented Sep 8, 2022

github-actions bot commented Sep 8, 2022

IWANABETHATGUY Sep 8, 2022

IWANABETHATGUY Sep 8, 2022

IWANABETHATGUY Sep 8, 2022

IWANABETHATGUY commented Sep 8, 2022

IWANABETHATGUY commented Sep 8, 2022

MichaReiser left a comment •

edited

Loading

IWANABETHATGUY commented Sep 10, 2022

MichaReiser commented Sep 10, 2022 •

edited

Loading

IWANABETHATGUY commented Sep 10, 2022

MichaReiser left a comment

IWANABETHATGUY commented Sep 12, 2022

IWANABETHATGUY Sep 12, 2022

IWANABETHATGUY commented Sep 12, 2022 •

edited

Loading

		@@ -316,6 +316,7 @@ JsAnyExpression =
		\| TsTypeAssertionExpression

	impl NeedsParentheses for JsPostUpdateExpression {
	fn needs_parentheses_with_parent(&self, parent: &JsSyntaxNode) -> bool {
	unary_like_expression_needs_parentheses(self.syntax(), parent)
	}
	}

feat(rome_js_parser): instantiation expressions #3152

feat(rome_js_parser): instantiation expressions #3152

Conversation

IWANABETHATGUY commented Sep 3, 2022 • edited Loading

Summary

Newly Supported Syntax

Test Plan

Prettier Compatibility

Main

PR

netlify bot commented Sep 3, 2022 • edited Loading

✅ Deploy Preview for docs-rometools ready!

IWANABETHATGUY commented Sep 5, 2022

github-actions bot commented Sep 5, 2022

Parser Benchmark Results

IWANABETHATGUY Sep 8, 2022

Choose a reason for hiding this comment

IWANABETHATGUY Sep 8, 2022

Choose a reason for hiding this comment

IWANABETHATGUY commented Sep 8, 2022

MichaReiser commented Sep 8, 2022

github-actions bot commented Sep 8, 2022

Parser Benchmark Results

IWANABETHATGUY Sep 8, 2022

Choose a reason for hiding this comment

IWANABETHATGUY Sep 8, 2022

Choose a reason for hiding this comment

IWANABETHATGUY Sep 8, 2022

Choose a reason for hiding this comment

IWANABETHATGUY commented Sep 8, 2022

IWANABETHATGUY commented Sep 8, 2022

MichaReiser left a comment • edited Loading

Choose a reason for hiding this comment

IWANABETHATGUY commented Sep 10, 2022

MichaReiser commented Sep 10, 2022 • edited Loading

IWANABETHATGUY commented Sep 10, 2022

MichaReiser left a comment

Choose a reason for hiding this comment

IWANABETHATGUY commented Sep 12, 2022

IWANABETHATGUY Sep 12, 2022

Choose a reason for hiding this comment

IWANABETHATGUY commented Sep 12, 2022 • edited Loading

IWANABETHATGUY commented Sep 3, 2022 •

edited

Loading

netlify bot commented Sep 3, 2022 •

edited

Loading

MichaReiser left a comment •

edited

Loading

MichaReiser commented Sep 10, 2022 •

edited

Loading

IWANABETHATGUY commented Sep 12, 2022 •

edited

Loading