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

fix(rome_js_analyzer): precedence for useOptionalChain #3277

Merged
merged 2 commits into from
Sep 30, 2022
Merged

Conversation

ematipico
Copy link
Contributor

Summary

This PR fixes #3257

Test Plan

The PR add two new test cases

@netlify
Copy link

netlify bot commented Sep 26, 2022

Deploy Preview for rometools canceled.

Name Link
🔨 Latest commit c423b22
🔍 Latest deploy log https://app.netlify.com/sites/rometools/deploys/6336e509ecdac90008742223

@ematipico ematipico temporarily deployed to netlify-playground September 26, 2022 13:50 Inactive
@github-actions
Copy link

github-actions bot commented Sep 26, 2022

@ematipico ematipico added the A-Linter Area: linter label Sep 26, 2022
Copy link
Contributor

@leops leops left a comment

Choose a reason for hiding this comment

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

I'm not sure those are really false positive, it looks more like the fix is simply incorrectly removing the parenthesis: (new foo || {}).bar should be replaced with (new foo)?.bar, but the generated code action emits new foo?.bar instead which then gets re-parsed as new (foo?.bar) and results in a syntax error

@ematipico ematipico requested a review from a team September 28, 2022 07:25
@ematipico ematipico temporarily deployed to netlify-playground September 28, 2022 07:25 Inactive
@ematipico
Copy link
Contributor Author

ematipico commented Sep 28, 2022

Turns out that the issue was the precedence. If you check the MDN table the new keyword has two different precedence if there are or not arguments.

The absence of arguments yields a unique precedence just for this case, which was not tracked in our enum. I updated the code and it now works as @leops suggested.

@ematipico ematipico temporarily deployed to netlify-playground September 28, 2022 07:29 Inactive
@ematipico ematipico temporarily deployed to netlify-playground September 28, 2022 07:29 Inactive
@github-actions
Copy link

Parser conformance results on ubuntu-latest

js/262

Test result main count This PR count Difference
Total 45879 45879 0
Passed 44939 44939 0
Failed 940 940 0
Panics 0 0 0
Coverage 97.95% 97.95% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 39 39 0
Passed 36 36 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.31% 92.31% 0.00%

symbols/microsoft

Test result main count This PR count Difference
Total 5946 5946 0
Passed 1621 1621 0
Failed 4325 4325 0
Panics 0 0 0
Coverage 27.26% 27.26% 0.00%

ts/babel

Test result main count This PR count Difference
Total 588 588 0
Passed 519 519 0
Failed 69 69 0
Panics 0 0 0
Coverage 88.27% 88.27% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 16257 16257 0
Passed 12395 12395 0
Failed 3862 3862 0
Panics 0 0 0
Coverage 76.24% 76.24% 0.00%

@ematipico ematipico changed the title fix(rome_js_analyzer): fix false positive for useOptionalChain fix(rome_js_analyzer): fix wrong precedence for useOptionalChain Sep 28, 2022
@ematipico ematipico changed the title fix(rome_js_analyzer): fix wrong precedence for useOptionalChain fix(rome_js_analyzer): precedence for useOptionalChain Sep 30, 2022
Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Would you mind testing if the formatter handles the precedence correctly and, if not, fix the parentheses setting in its own PR?

crates/rome_js_syntax/src/lib.rs Outdated Show resolved Hide resolved
@ematipico ematipico temporarily deployed to netlify-playground September 30, 2022 12:46 Inactive
@ematipico ematipico merged commit d354294 into main Sep 30, 2022
@ematipico ematipico deleted the fix/issue-3257 branch September 30, 2022 12:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Linter Area: linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 invalid code action of useOptionalChain
3 participants