refactor(rome_diagnostics): refactor diagnostics to use static categories #3270
Conversation
✅ Deploy Preview for rometools canceled.
|
f40c9f2 to
10d3272
Compare
Parser conformance results on ubuntu-latestjs/262
jsx/babel
symbols/microsoft
ts/babel
ts/microsoft
|
10d3272 to
c41a0d8
Compare
| "io/skippedFile", | ||
| "io/unhandledFile", | ||
| "parse", | ||
| "suppressions/unknownGroup", |
There was a problem hiding this comment.
Is io beginner friendly? (it's also an acronym right?) Would it make sense to call it fileSystem?
Also (sorry I caught this only now), I think args is not beginner friendly. A junior developer might not undestand what it means, it's really technical. To be honest, I also struggle to understand what it means, because there's fileNotFound later... which something related to the file system. I would suggest an alternative but I would need a clarification of the category first.
There was a problem hiding this comment.
I've tried to reword some of these codes, but I/O errors are not necessarily caused by the filesystem as they can also be related to the domain socket or named pipe used by the daemon process to communicate for instance. So I think we still need a generic internalError/io category when a low-level error happens.
For the args/fileNotFound category it's something I reused from Rome JS along with flags/invalid, but I haven't put much thought into it since at the moment those two are only used in examples and tests to re-implement the equivalent diagnostic from the JS version.
| "internalError/panic", | ||
| "io/skippedFile", | ||
| "io/unhandledFile", | ||
| "parse", |
There was a problem hiding this comment.
Is the parse category shared between all parsers?
There was a problem hiding this comment.
The parse category is highly generic and is shared between all syntax error diagnostics in the lexer, parser and potentially future rule in the syntax category in the analyzer
c41a0d8 to
059453e
Compare
|
Comparing refactor(rome_diagnostics): refactor diagnostics to use static categories Snapshot #6 to median since last deploy of rome.tools.
1 page tested
|
| Chrome Desktop | iPhone, 4G LTE | Motorola Moto G Power, 3G connection |
|---|---|---|
 |
 |
 |
Most significant changes
 |
Value | Budget |
|---|---|---|
| JS Parse & Compile Motorola Moto G Power, 3G connection |
1.81s  from 29ms |
 |
| Total JavaScript Size in Bytes Chrome Desktop |
4.29 MB  from 86.8 KB |
|
| Total JavaScript Size in Bytes iPhone, 4G LTE |
4.29 MB from 86.8 KB |
|
| Total JavaScript Size in Bytes Motorola Moto G Power, 3G connection |
4.29 MB from 86.8 KB |
|
| JS Parse & Compile iPhone, 4G LTE |
381ms  from 8ms |
|
28 other significant changes: JS Parse & Compile on Chrome Desktop, First Contentful Paint on Motorola Moto G Power, 3G connection, Largest Contentful Paint on Motorola Moto G Power, 3G connection, Speed Index 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, Number of Requests on Motorola Moto G Power, 3G connection, Number of Requests on Chrome Desktop, Number of Requests on iPhone, 4G LTE, Time to Interactive on Motorola Moto G Power, 3G connection, First Contentful Paint on Chrome Desktop, First Contentful Paint on iPhone, 4G LTE, Time to Interactive on Chrome Desktop, Total Blocking Time on Motorola Moto G Power, 3G connection, Speed Index on iPhone, 4G LTE, Speed Index on Chrome Desktop, Largest Contentful Paint on iPhone, 4G LTE, Time to Interactive on iPhone, 4G LTE, Largest Contentful Paint on Chrome Desktop, Total Image Size in Bytes on Chrome Desktop, Total Image Size in Bytes on iPhone, 4G LTE, Total Image Size in Bytes on Motorola Moto G Power, 3G connection, 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
059453e to
320d33b
Compare
320d33b to
b479602
Compare
Summary
This PR changes the type of diagnostics code / categories from plain strings to the new
Categorytype. I tried to make use of statically checked categories (using thecategory!macro) as much as possible, and one particular area of interest for these are the analyzer rules:Since some rules never emit any diagnostic (mainly assist rules such as
flipBinExporinlineVariable) this check needs to happen only for rules that actually end up creating a diagnostic. The solution I came up with is to have thedeclare_rule!macro emit a newrule_category!macro in the module context that expands to the category for the rule if it is correctly registered (or a compilation error if it's not). This macro is used by lint rules to create diagnostics with the correct category, and most importantly assist rules never use the macro and consequently never cause the compile error to be emitted in the expanded code.One downside of this approach is that the analyzer infrastructure is no longer responsible for injecting the category corresponding to a rule into the diagnostics it emits, and individual rules could possible emit diagnostics with a different category, but they would still be restricted to existing categories, and this may in fact turn out to be useful to implement syntax rules that would emit diagnostics with the "parse" error category instead of "lint"
Test Plan
I changed a number of error categories to re-use the name they had in Rome JS, this has caused some parser and CLI snapshots to change again. For the rest as I mentioned earlier I used static verification as much as possible, so this guarantees that the categories used in the code are valid if it builds correctly.