-
Notifications
You must be signed in to change notification settings - Fork 659
feat(rome_js_parser): instantiation expressions #3152
feat(rome_js_parser): instantiation expressions #3152
Conversation
✅ Deploy Preview for docs-rometools ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
!bench_parser |
Parser Benchmark Results
|
4db4867
to
7968e4a
Compare
@@ -316,6 +316,7 @@ JsAnyExpression = | |||
| TsTypeAssertionExpression |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!bench_parser |
Would you mind extening the PR summary by:
|
Parser Benchmark Results
|
.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() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason are similar with https://github.com/rome/tools/pull/3152/files#r966104100
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() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The corresponding logic of Typescript you could reference https://github.dev/microsoft/TypeScript/blob/be4e9bac8ff66174d4e856dae06f69aa4ef7e479/src/compiler/parser.ts#L5956-L5958
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You said that
But I can't search it through the codebase. |
Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 itExpressionWithTypeArguments
instead ofTsInstantiationExpression
? Are there other places where it should be used that aren't instantiation expressions?ExpressionWithTypeArguments
should end inExpression
to align with our naming convention that all expressions are named...Expression
:JsCallExpression
,JsIdentifierExpression
, ...
|
I'm in favour of renaming the node to
We can ease the transition for people familiar with TypeScript's AST by adding a comment to |
5279313
to
d2d9e60
Compare
Done d2d9e60 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
46c4a8b
to
2645de4
Compare
|
||
impl NeedsParentheses for TsInstantiationExpression { | ||
fn needs_parentheses_with_parent(&self, parent: &rome_js_syntax::JsSyntaxNode) -> bool { | ||
unary_like_expression_needs_parentheses(self.syntax(), parent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 parseExpressionWithTypeArguments
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 asJsPostUpdateExpression
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) } }
@MichaReiser , All issues have been addressed. |
Summary
TsExpressionWithTypeArguments
to align typescript AST shape.TsTypeOfType
,adding optionaltype_arguments
,now, the
TsTypeOfType
could be written like:SyntaxKind
is used to support Typescript instantiation expression, also it would be sometimesambiguous when try to parse
template_literal
insidemember_expression_rest
https://github.com/rome/tools/pull/3152/files#diff-1ca6d1ff770e933910c550807255d1bf03cdebbf60bf469579b528c0c28c1537R706-R709new_expr
https://github.com/rome/tools/pull/3152/files#diff-1ca6d1ff770e933910c550807255d1bf03cdebbf60bf469579b528c0c28c1537R735-R769call_expr_rest
https://github.com/rome/tools/pull/3152/files#diff-1ca6d1ff770e933910c550807255d1bf03cdebbf60bf469579b528c0c28c1537R1653-R1673Newly Supported Syntax
Test Plan
Prettier Compatibility
Main
Average compatibility: 85.67
Compatible lines: 84.09
PR
Average compatibility: 85.81
Compatible lines: 84.10