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

fix(rome_js_parser): Allow arguments in d.ts files #3389

Merged
merged 1 commit into from
Oct 11, 2022

Conversation

MichaReiser
Copy link
Contributor

Ambient context has slightly different rules than parsing in a normal context. One such difference is that arguments and future reserved keywords are valid identifiers.

Our parser already correctly handled this case by setting the strict_mode to None when entering an ambient context. However, we initialized strict_mode with Module for d.ts files.

This PR correctly initializes the parser state with strict = None if the file is a type script definition file.

Tests

I added a new parser test and verified that the jquery.d.ts can now be formatted

cargo run --bin rome format ../vscode/extensions/html-language-features/server/lib/jquery.d.ts --write

@MichaReiser MichaReiser added the A-Parser Area: parser label Oct 11, 2022
@MichaReiser MichaReiser added this to the 10.0.0 milestone Oct 11, 2022
@MichaReiser MichaReiser requested a review from a team October 11, 2022 07:12
@netlify
Copy link

netlify bot commented Oct 11, 2022

Deploy Preview for rometools canceled.

Name Link
🔨 Latest commit 882d593
🔍 Latest deploy log https://app.netlify.com/sites/rometools/deploys/6345181abfe8d70008a45264

@MichaReiser MichaReiser temporarily deployed to netlify-playground October 11, 2022 07:12 Inactive
@MichaReiser MichaReiser linked an issue Oct 11, 2022 that may be closed by this pull request
1 task
@MichaReiser MichaReiser temporarily deployed to netlify-playground October 11, 2022 07:13 Inactive
Ambient context has slightly different rules than parsing in a normal context. One such difference is that `arguments` and future reserved keywords are valid identifiers.

Our parser already correctly handled this case by setting the `strict_mode` to `None` when entering an ambient context. However, we initialized `strict_mode` with `Module` for `d.ts` files.

This PR correctly initializes the parser state with `strict = None` if the file is a type script definition file.

## Tests

I added a new parser test and verified that the `jquery.d.ts` can now be formatted

```bash
cargo run --bin rome format ../vscode/extensions/html-language-features/server/lib/jquery.d.ts --write
```
@MichaReiser MichaReiser temporarily deployed to netlify-playground October 11, 2022 07:15 Inactive
@github-actions
Copy link

github-actions bot commented Oct 11, 2022

@github-actions
Copy link

github-actions bot commented Oct 11, 2022

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%

Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

I wonder if we should add an option to our playground, so we can test these cases 🤔

@MichaReiser MichaReiser merged commit ac43cf0 into main Oct 11, 2022
@MichaReiser MichaReiser deleted the fix/arguments-ambient-context branch October 11, 2022 10:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Parser Area: parser
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

🐛 Allow arguments in d.ts files
2 participants