refactor(rome_js_parser): Extract shared parser infrastructure #3833

MichaReiser · 2022-11-23T11:14:55Z

Summary

Part of #2351

This PR extracts the shared parser infrastructure into a new rome_parser crate.

This PR introduces a new Parser trait that implements the common parser methods. The internal data used by the trait is wrapped in a ParserContext struct that ensures that access to events and diagnostics is locked down (only few operations are permitted by outside crates because manipulating events risks creating a corrupted events stream).

This PR renames the Parser struct in rome_js_parser to JsParser to avoid name clashes.

This PR introduces the new TokenSource trait and renames the JavaScript token source struct to JsTokenSource. The PR further introduces the new BumpWithContext and NthToken traits that TokenSource's can implement that support different lexing contexts or support lookahead (neither is the case for JSON).

Design Decision

I decided to remove the kind of the CompletedMarker and instead read it from the events vector to avoid that CompletedMarker, and in turn, ParsedSyntax must become generic over SyntaxKind. The downside of this decision is that it is possible to call completed_marker.kind(parser) with different parsers, yielding incorrect results if the parser uses a different kind.

I consider this a compromise worth taking as this is only a problem if both parsers happen to have the exact same events because indexing into the events stream would otherwise fail (or return rubbish).

Alternative Designs

My first approach used a Parser struct with a generic LanguageParser type parameter.

struct Parser<'source, L> where L: LanguageParser {
  source: L::Source<'source>,
  events: Vec<Event<L::Kind>>,
  diagnostics: Vec<ParseDiagnostic>,
  ...
}

Where LanguageParser would store any language-specific state. It worked reasonably but has the downsides that

it requires an extension trait to implement additional methods on Parser<JsLanguageParser> because Rust doesn't allow implementing methods for a struct unowned by the crate.
Implementing methods on JsLanguageParser directly was awkward because it required access to the Parser for almost all operations.

Test Plan

cargo test

netlify · 2022-11-23T11:15:00Z

✅ Deploy Preview for docs-rometools canceled.

Name	Link
🔨 Latest commit	`e6f9e6a`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-rometools/deploys/637f496c344001000928fa3c

MichaReiser · 2022-11-23T11:17:54Z

!bench_parser

github-actions · 2022-11-23T11:23:48Z

Parser conformance results on ubuntu-latest

js/262

Test result	`main` count	This PR count	Difference
Total	45879	45879	0
Passed	44936	44936	0
Failed	943	943	0
Panics	0	0	0
Coverage	97.94%	97.94%	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	1757	1757	0
Failed	4189	4189	0
Panics	0	0	0
Coverage	29.55%	29.55%	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	12397	12397	0
Failed	3860	3860	0
Panics	0	0	0
Coverage	76.26%	76.26%	0.00%

crates/rome_parser/src/parse_lists.rs

calibre-analytics · 2022-11-23T11:33:56Z

Comparing refactor(rome_js_parser): Extract shared parser infrastructure Snapshot #5 to median since last deploy of rome.tools.

	LCP^?	CLS^?	TBT^?
Overall ^{Median across all pages and test profiles}	2.39s ^from 258ms	0.0 ^no change	138ms ^no change
Chrome Desktop ^{Chrome Desktop • Cable}	2.39s ^from 258ms	0.0 ^no change	325ms ^from 22ms
iPhone, 4G LTE ^{iPhone 12 • 4G LTE}	1.07s ^from 238ms	0.0 ^no change	13ms ^no change
Motorola Moto G Power, 3G connection ^{Motorola Moto G Power • Regular 3G}	16.4s ^from 1.07s	0.0 ^no change	138ms ^no change

1 page tested

Home

Browser previews

Chrome Desktop	iPhone, 4G LTE	Motorola Moto G Power, 3G connection

Most significant changes

	Value	Budget
Total JavaScript Size in Bytes Chrome Desktop	5.36 MB ^{from 86.8 KB}
Total JavaScript Size in Bytes iPhone, 4G LTE	5.36 MB ^{from 86.8 KB}
Total JavaScript Size in Bytes Motorola Moto G Power, 3G connection	5.36 MB ^{from 86.8 KB}
JS Parse & Compile Motorola Moto G Power, 3G connection	1.61s ^from 27ms
JS Parse & Compile iPhone, 4G LTE	469ms ^from 10ms

_{27 other significant changes: JS Parse & Compile on Chrome Desktop, Largest Contentful Paint on Motorola Moto G Power, 3G connection, First Contentful Paint on Motorola Moto G Power, 3G connection, Total Blocking Time on Chrome Desktop, Total CSS Size in Bytes on Chrome Desktop, Total CSS Size in Bytes on iPhone, 4G LTE, Total CSS Size in Bytes on Motorola Moto G Power, 3G connection, Time to Interactive on Motorola Moto G Power, 3G connection, Total Page Size in Bytes on Chrome Desktop, Total Page Size in Bytes on iPhone, 4G LTE, Total Page Size in Bytes on Motorola Moto G Power, 3G connection, Time to Interactive on Chrome Desktop, Largest Contentful Paint on Chrome Desktop, First Contentful Paint on Chrome Desktop, Number of Requests on Motorola Moto G Power, 3G connection, Number of Requests on Chrome Desktop, Number of Requests on iPhone, 4G LTE, Speed Index on Motorola Moto G Power, 3G connection, Time to Interactive on iPhone, 4G LTE, Largest Contentful Paint on iPhone, 4G LTE, First Contentful Paint on iPhone, 4G LTE, Speed Index on Chrome Desktop, Total HTML Size in Bytes on Chrome Desktop, Total HTML Size in Bytes on iPhone, 4G LTE, Total HTML Size in Bytes on Motorola Moto G Power, 3G connection, Lighthouse Performance Score on Motorola Moto G Power, 3G connection, Lighthouse Performance Score on Chrome Desktop}

_{Calibre: Site dashboard | View this PR | Edit settings | View documentation}

github-actions · 2022-11-23T11:36:10Z

Parser Benchmark Results

group                                 main                                   pr
-----                                 ----                                   --
parser/checker.ts                     1.09    148.2±8.77ms    17.5 MB/sec    1.00    136.3±9.66ms    19.1 MB/sec
parser/compiler.js                    1.07     81.7±8.10ms    12.8 MB/sec    1.00     76.3±6.70ms    13.7 MB/sec
parser/d3.min.js                      1.00     45.3±3.22ms     5.8 MB/sec    1.05     47.4±4.51ms     5.5 MB/sec
parser/dojo.js                        1.05      4.0±0.26ms    17.2 MB/sec    1.00      3.8±0.15ms    18.0 MB/sec
parser/ios.d.ts                       1.00    120.1±6.55ms    15.5 MB/sec    1.00    120.5±8.90ms    15.5 MB/sec
parser/jquery.min.js                  1.00     12.4±0.74ms     6.7 MB/sec    1.10     13.7±0.97ms     6.0 MB/sec
parser/math.js                        1.00     95.3±7.25ms     6.8 MB/sec    1.07    101.8±8.95ms     6.4 MB/sec
parser/parser.ts                      1.00      2.8±0.12ms    17.3 MB/sec    1.05      3.0±0.21ms    16.5 MB/sec
parser/pixi.min.js                    1.06     62.2±5.93ms     7.1 MB/sec    1.00     58.5±5.93ms     7.5 MB/sec
parser/react-dom.production.min.js    1.17     18.6±1.52ms     6.2 MB/sec    1.00     15.9±0.75ms     7.3 MB/sec
parser/react.production.min.js        1.00   893.0±70.78µs     6.9 MB/sec    1.00   890.1±63.49µs     6.9 MB/sec
parser/router.ts                      1.00      2.3±0.13ms    27.1 MB/sec    1.08      2.5±0.15ms    25.2 MB/sec
parser/tex-chtml-full.js              1.03    136.3±8.44ms     6.7 MB/sec    1.00    131.7±8.41ms     6.9 MB/sec
parser/three.min.js                   1.17     75.1±5.70ms     7.8 MB/sec    1.00     64.0±5.96ms     9.2 MB/sec
parser/typescript.js                  1.02   578.6±23.02ms    16.4 MB/sec    1.00   570.0±46.42ms    16.7 MB/sec
parser/vue.global.prod.js             1.00     20.1±1.39ms     6.0 MB/sec    1.14     22.8±1.85ms     5.3 MB/sec

ematipico

No particular concerns, happy to merge it!

ematipico · 2022-11-24T11:30:51Z

crates/rome_parser/src/diagnostic.rs

+    span: Option<TextRange>,
+    /// Reference to a file where the issue occurred
+    #[location(resource)]
+    pub(super) file_id: FileId,


Any chance we could remove FileId during this refactor? Not a blocker, it would be a nice to have! :)

I'll leave that to you or leo as I have no idea what must be done and I only moved the file around.

crates/rome_parser/src/lib.rs

MichaReiser added the A-Parser Area: parser label Nov 23, 2022

MichaReiser marked this pull request as ready for review November 23, 2022 11:32

MichaReiser requested review from leops, ematipico and xunilrj as code owners November 23, 2022 11:32

MichaReiser requested a review from a team November 23, 2022 11:32

MichaReiser commented Nov 23, 2022

View reviewed changes

crates/rome_parser/src/parse_lists.rs Show resolved Hide resolved

MichaReiser added 10 commits November 24, 2022 08:24

refactor(rome_js_parser): Rename Parser to JsParser

444ca43

refactor(rome_js_parser): Introduce TokenSource trait

7e1df4d

refactor(rome_js_parser): Parser struct, and ParseLanguage trait

9b8edf4

refactor(rome_js_parser): Introduce rome_parser crate

2d9772c

Make Parser a trait

4e65055

Re-use ParseDiagnostic in rome_json_parser

407a0d2

Remove 'source lifetime from Parser trait

05bb71e

Safety comment

a63944f

Remove commented out usings

bd9dafe

Extract lossless tree sink

df1d539

MichaReiser force-pushed the refactor/parser-trait branch from d596ea1 to df1d539 Compare November 24, 2022 07:30

Use implicit lifetimes

e6f9e6a

ematipico reviewed Nov 24, 2022

View reviewed changes

ematipico approved these changes Nov 24, 2022

View reviewed changes

MichaReiser merged commit 5c2928b into main Nov 25, 2022

MichaReiser deleted the refactor/parser-trait branch November 25, 2022 08:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(rome_js_parser): Extract shared parser infrastructure #3833

refactor(rome_js_parser): Extract shared parser infrastructure #3833

Uh oh!

MichaReiser commented Nov 23, 2022 •

edited

Loading

Uh oh!

netlify bot commented Nov 23, 2022 •

edited

Loading

Uh oh!

MichaReiser commented Nov 23, 2022

Uh oh!

github-actions bot commented Nov 23, 2022

Uh oh!

Uh oh!

calibre-analytics bot commented Nov 23, 2022 •

edited

Loading

Browser previews

Most significant changes

Uh oh!

github-actions bot commented Nov 23, 2022

Uh oh!

ematipico left a comment

Uh oh!

ematipico Nov 24, 2022

Uh oh!

MichaReiser Nov 24, 2022

Uh oh!

Uh oh!

Uh oh!

refactor(rome_js_parser): Extract shared parser infrastructure #3833

refactor(rome_js_parser): Extract shared parser infrastructure #3833

Uh oh!

Conversation

MichaReiser commented Nov 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Design Decision

Alternative Designs

Test Plan

Uh oh!

netlify bot commented Nov 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for docs-rometools canceled.

Uh oh!

MichaReiser commented Nov 23, 2022

Uh oh!

github-actions bot commented Nov 23, 2022

Parser conformance results on ubuntu-latest

js/262

jsx/babel

symbols/microsoft

ts/babel

ts/microsoft

Uh oh!

Uh oh!

calibre-analytics bot commented Nov 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1 page tested

Browser previews

Most significant changes

Uh oh!

github-actions bot commented Nov 23, 2022

Parser Benchmark Results

Uh oh!

ematipico left a comment

Choose a reason for hiding this comment

Uh oh!

ematipico Nov 24, 2022

Choose a reason for hiding this comment

Uh oh!

MichaReiser Nov 24, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MichaReiser commented Nov 23, 2022 •

edited

Loading

netlify bot commented Nov 23, 2022 •

edited

Loading

calibre-analytics bot commented Nov 23, 2022 •

edited

Loading