Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

fix(rome_js_parser): Adding syntax error for new A?.b() #3118

Merged
merged 13 commits into from
Sep 21, 2022

Conversation

IWANABETHATGUY
Copy link
Contributor

@IWANABETHATGUY IWANABETHATGUY commented Aug 27, 2022

Summary

  1. Adding syntax error for new A?.b()
  2. Fixes 🐛 Missing syntax error for new A?.b() #3115

Test Plan

  1. Ci should pass
  2. Checking the newly added snapshot

if p.at(T!['(']) {
parse_call_arguments(p).unwrap();
} else if p.at(T![?.]) && is_identifier_expression {
let error = p
.err_builder("Invalid optional chain from new expression.")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to add detailed error descriptions like Typescript?That would introduce extra overhead, I think Invalid optional chain from new expression. would be enough.

@MichaReiser
Copy link
Contributor

MichaReiser commented Aug 27, 2022

This seems to be a new error introduced with TypeScript 4.8.

Please add a few more complicated tests where the optional chain isn't the first member

new A.b?.c()
new (A.b)?.c()
new (A.b?.()).c()
new A.b?.()()

I'm not sure that it will be feasible to catch all those inside of the parser or if it would be better to implement them as a rule.

@IWANABETHATGUY
Copy link
Contributor Author

@IWANABETHATGUY
Copy link
Contributor Author

The testing has been updated.

@IWANABETHATGUY
Copy link
Contributor Author

!bench_parser

@github-actions
Copy link

Parser Benchmark Results

group                                 main                                   pr
-----                                 ----                                   --
parser/checker.ts                     1.00    151.7±3.62ms    17.1 MB/sec    1.03    156.2±4.05ms    16.6 MB/sec
parser/compiler.js                    1.00    100.0±1.52ms    10.5 MB/sec    1.02    101.9±1.49ms    10.3 MB/sec
parser/d3.min.js                      1.00     55.4±0.73ms     4.7 MB/sec    1.00     55.6±0.47ms     4.7 MB/sec
parser/dojo.js                        1.00      4.7±0.01ms    14.6 MB/sec    1.01      4.7±0.01ms    14.5 MB/sec
parser/ios.d.ts                       1.00    130.3±2.95ms    14.3 MB/sec    1.02    133.5±2.83ms    14.0 MB/sec
parser/jquery.min.js                  1.01     14.4±0.64ms     5.7 MB/sec    1.00     14.2±0.13ms     5.8 MB/sec
parser/math.js                        1.00    106.8±2.29ms     6.1 MB/sec    1.01    107.4±2.01ms     6.0 MB/sec
parser/parser.ts                      1.00      3.3±0.00ms    14.8 MB/sec    1.01      3.3±0.01ms    14.7 MB/sec
parser/pixi.min.js                    1.00     64.2±1.48ms     6.8 MB/sec    1.03     66.1±0.76ms     6.6 MB/sec
parser/react-dom.production.min.js    1.00     19.6±0.24ms     5.9 MB/sec    1.02     20.0±0.20ms     5.8 MB/sec
parser/react.production.min.js        1.00   1042.1±1.01µs     5.9 MB/sec    1.01   1052.7±0.72µs     5.8 MB/sec
parser/router.ts                      1.00      2.8±0.06ms    21.7 MB/sec    1.00      2.8±0.00ms    21.6 MB/sec
parser/tex-chtml-full.js              1.00    143.0±2.42ms     6.4 MB/sec    1.00    143.5±1.59ms     6.4 MB/sec
parser/three.min.js                   1.00     75.0±1.28ms     7.8 MB/sec    1.01     76.0±0.79ms     7.7 MB/sec
parser/typescript.js                  1.00   628.0±16.17ms    15.1 MB/sec    1.00   630.0±14.47ms    15.1 MB/sec
parser/vue.global.prod.js             1.00     24.5±0.26ms     4.9 MB/sec    1.01     24.8±0.24ms     4.8 MB/sec

@IWANABETHATGUY IWANABETHATGUY requested review from MichaReiser and removed request for xunilrj August 28, 2022 05:10
@@ -1246,7 +1246,7 @@ pub(crate) fn parse_ts_type_arguments_in_expression(p: &mut Parser) -> ParsedSyn
p.re_lex(ReLexContext::TypeArgumentLessThan);
let arguments = parse_ts_type_arguments_impl(p, false);

if p.last() == Some(T![>]) && matches!(p.cur(), T!['('] | BACKTICK) {
if p.last() == Some(T![>]) && matches!(p.cur(), T!['('] | BACKTICK | T![?.]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind adding a new test case showing when this is valid?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is related to instantiation expressions that our parser currently doesn't support. I recommend tackling this in a separate PR as it is more involved:

microsoft/TypeScript@e6808c4

@ematipico ematipico added A-Parser Area: parser L-JavaScript Langauge: JavaScript labels Sep 2, 2022
@ematipico ematipico added this to the 0.10.0 milestone Sep 5, 2022
@ematipico
Copy link
Contributor

Adding 0.10.0 milestone to only signal to not merge this PR before 0.9.0 release

@github-actions
Copy link

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

@netlify
Copy link

netlify bot commented Sep 20, 2022

Deploy Preview for docs-rometools ready!

Name Link
🔨 Latest commit 7278b0b
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/632b2a336a6bde0008ca1ed1
😎 Deploy Preview https://deploy-preview-3118--docs-rometools.netlify.app
📱 Preview on mobile
Toggle QR Code...

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
Copy link
Contributor Author

@MichaReiser, Since the main logic of InstantiationExpression have been finished, we could push forward this pull request,

@IWANABETHATGUY
Copy link
Contributor Author

You could refer to this patch pr of https://github.com/microsoft/TypeScript/pull/48656/files, pretty much just porting.

@IWANABETHATGUY
Copy link
Contributor Author

The rest of two patch pr of instantiationExpression only modified https://github.com/microsoft/TypeScript/pull/48659/files#diff-12a6724be007eb1a19d80018c7a63bbc73525ca793a9b3e5da49a4e86bbf457cL5732-L5765 canFollowTypeArgumentsInExpression, we could skip now.

@IWANABETHATGUY
Copy link
Contributor Author

Why I want to push forward because this pr has been marked as stale, maybe closed a few days later.

@@ -147,6 +147,6 @@ if ((this ?? {}).#bar) { foo.bar; }

(undefined && this ?? {}).#bar;
(((typeof this) as string) || {}).#bar;
(new foo || {}).bar;
// (new foo || {}).bar;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why I disabled this test case , please refer to #3257

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could add // disabled because of https://github.com/rome/tools/issues/3257 to the comment, so we know the reason :)

@IWANABETHATGUY
Copy link
Contributor Author

!bench_parser

@github-actions
Copy link

Parser Benchmark Results

group                                 main                                   pr
-----                                 ----                                   --
parser/checker.ts                     1.00   150.4±10.67ms    17.3 MB/sec    1.02   152.7±11.27ms    17.0 MB/sec
parser/compiler.js                    1.09     95.7±5.84ms    10.9 MB/sec    1.00     87.8±3.98ms    11.9 MB/sec
parser/d3.min.js                      1.00     52.1±2.96ms     5.0 MB/sec    1.05     54.9±3.26ms     4.8 MB/sec
parser/dojo.js                        1.05      4.9±0.19ms    13.9 MB/sec    1.00      4.7±0.22ms    14.6 MB/sec
parser/ios.d.ts                       1.02    123.9±6.48ms    15.1 MB/sec    1.00    121.9±7.13ms    15.3 MB/sec
parser/jquery.min.js                  1.02     14.3±0.57ms     5.8 MB/sec    1.00     14.0±0.53ms     5.9 MB/sec
parser/math.js                        1.00    101.4±5.60ms     6.4 MB/sec    1.01    102.5±6.09ms     6.3 MB/sec
parser/parser.ts                      1.00      3.4±0.15ms    14.4 MB/sec    1.01      3.4±0.19ms    14.2 MB/sec
parser/pixi.min.js                    1.00     60.9±2.72ms     7.2 MB/sec    1.03     62.7±4.16ms     7.0 MB/sec
parser/react-dom.production.min.js    1.06     19.9±0.77ms     5.8 MB/sec    1.00     18.7±0.55ms     6.1 MB/sec
parser/react.production.min.js        1.04  1098.6±50.66µs     5.6 MB/sec    1.00  1056.3±62.72µs     5.8 MB/sec
parser/router.ts                      1.01      2.9±0.17ms    20.9 MB/sec    1.00      2.9±0.15ms    21.1 MB/sec
parser/tex-chtml-full.js              1.00    134.5±6.38ms     6.8 MB/sec    1.02    137.5±8.97ms     6.6 MB/sec
parser/three.min.js                   1.00     68.6±3.34ms     8.6 MB/sec    1.00     68.3±3.74ms     8.6 MB/sec
parser/vue.global.prod.js             1.04     24.2±0.86ms     5.0 MB/sec    1.00     23.2±1.04ms     5.2 MB/sec

@github-actions github-actions bot removed the S-Stale label Sep 21, 2022
@@ -768,6 +768,16 @@ fn parse_new_expr(p: &mut Parser, context: ExpressionContext) -> ParsedSyntax {
.or_add_diagnostic(p, expected_expression)
.map(|expr| parse_member_expression_rest(p, expr, context, false, &mut false))
{
if p.at(T![?.]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the error checking here because I don't want to clone the lhs.text(p). If put this branch here, https://github.com/rome/tools/pull/3118/files#diff-1ca6d1ff770e933910c550807255d1bf03cdebbf60bf469579b528c0c28c1537R797, you can't return a &str of lhs.text(p) because lhs is moved via lhs.undo_completion(p)

@MichaReiser MichaReiser merged commit c1fd437 into rome:main Sep 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Parser Area: parser L-JavaScript Langauge: JavaScript
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

🐛 Missing syntax error for new A?.b()
3 participants