refactor(rome_js_parser): APIs to create diagnostics #3429
Conversation
86e41f6 to
b0f79ef
Compare
crates/rome_js_parser/src/parser.rs
Outdated
| #[must_use] | ||
| pub fn err_builder(&self, message: &str) -> ParseDiagnostic { | ||
| ParseDiagnostic::new(self.file_id, message) | ||
| pub fn err_builder(&self, message: &str, span: impl AsSpan) -> ParseDiagnostic { |
There was a problem hiding this comment.
The message parameter could be typed to impl Into<String> (or even impl Display by changing the definition of ParseDiagnostic::new too) to allow the code using this function to pass in a String instance directly instead of having to allocate twice (for instance when doing .err_build(&format!()))
bbad581 to
c21d763
Compare
|
Do we have an understanding why it regresses performance in such a significant way? Particularly because the benchmarks only test programs without ANY diagnostic. |
b0f79ef to
39c7264
Compare
39c7264 to
94dcedc
Compare
Personally I don't know 😞 |
Have you tried profiling a parser benchmark with the old diagnostic, then profile the same benchmark with the new diagnostics and compared the numbers? Are there any methods that now require significant more time? Have you tried running the benchmarks with |
|
This branch
|
| @@ -302,6 +302,8 @@ array_assignment_target_err.js:1:4 parse ━━━━━━━━━━━━━ | |||
| 2 │ [a, c, ...rest,] = test; | |||
| 3 │ [a = , = "test"] = test; | |||
|
|
|||
| i Remove a | |||
There was a problem hiding this comment.
Unrelated to your change but a should probably be put in quotes :D
There was a problem hiding this comment.
Now that we can support markup, we can enrich these messages :)
|
I probably did something wrong when I run the benchmarks locally. I re-ran them again, in release mode: |
|
!bench_parser |
Parser Benchmark Results |
Summary
Closes #3424
.primarydoesn't exist anymore.secondaryhas been renamed in.detail, an API used to show more details around the error encountered.footer_notehas been renamed in.hint, a small message that should explain how an error can be fixedTest Plan
cargo bench_parserand made sure that works;cargo coverageand made sure that works;