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

fix(rome_diagnostics): fix unknown rule warning false positive #3412

Merged
merged 2 commits into from
Oct 13, 2022

Conversation

leops
Copy link
Contributor

@leops leops commented Oct 12, 2022

Summary

Fixes #3406

This PR changes how the "rule registry" concept of the analyzer works by splitting its various use-case into separate types implemented at different location in the codebase through a common RegistryVisitor trait. This trait makes it possible to differentiate between the "static registry" containing all the rules for a given language (implemented through the new visit_registry function replacing build_registry), and various "dynamic registries" that get built on demand throughout the codebase: the analyzer holds both a fast set of all the known rules and a RuleRegistry containing only the active rules, the lintdoc tools builds a list of the lint rules only, the LSP also needs to build a list of the code action rules, ...

Test Plan

I've added an additional test for suppression comments to ensure the analyzer does not emit an "unknown rule" diagnostic on a suppression comment for a rule whose category is disabled. The rest of the change should be covered by the existing tests for the corresponding area of the codebase.

@leops leops requested a review from a team October 12, 2022 15:17
@leops leops temporarily deployed to netlify-playground October 12, 2022 15:17 Inactive
@netlify
Copy link

netlify bot commented Oct 12, 2022

Deploy Preview for rometools canceled.

Name Link
🔨 Latest commit 223a81b
🔍 Latest deploy log https://app.netlify.com/sites/rometools/deploys/6346e9235906fa0008ae4058

@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%

@github-actions
Copy link

github-actions bot commented Oct 12, 2022

@MichaReiser
Copy link
Contributor

!bench_analyzer

@MichaReiser MichaReiser added the A-Linter Area: linter label Oct 12, 2022
@MichaReiser MichaReiser added this to the 10.0.0 milestone Oct 12, 2022
@github-actions
Copy link

Analyzer Benchmark Results

group                     main                                   pr
-----                     ----                                   --
analyzer/css.js           1.00      2.7±0.04ms     4.3 MB/sec    1.00      2.7±0.04ms     4.3 MB/sec
analyzer/index.js         1.00      7.7±0.10ms     4.2 MB/sec    1.01      7.8±0.29ms     4.2 MB/sec
analyzer/lint.ts          1.00      3.5±0.05ms    11.9 MB/sec    1.03      3.6±0.08ms    11.5 MB/sec
analyzer/parser.ts        1.00      8.8±0.12ms     5.5 MB/sec    1.01      8.9±0.16ms     5.5 MB/sec
analyzer/router.ts        1.00      6.4±0.11ms     9.6 MB/sec    1.02      6.5±0.13ms     9.4 MB/sec
analyzer/statement.ts     1.00      9.4±0.16ms     3.8 MB/sec    1.03      9.7±0.14ms     3.6 MB/sec
analyzer/typescript.ts    1.00     14.8±0.31ms     3.7 MB/sec    1.02     15.0±0.34ms     3.6 MB/sec

@leops leops temporarily deployed to netlify-playground October 12, 2022 16:19 Inactive
@leops leops merged commit 10cbc0e into main Oct 13, 2022
@leops leops deleted the fix/unknown-filtered-rule branch October 13, 2022 07:18
@ematipico
Copy link
Contributor

ematipico commented Oct 13, 2022

Since the error we were trying to fix originated via CLI, it would have been great to add a test in the CLI crate

@leops
Copy link
Contributor Author

leops commented Oct 13, 2022

Since the error we were trying to fix originated via CLI, it would have been great to add a test in the CLI crate

It's not really a "CLI-exclusive" error, since the root cause is in the analyzer it also happens in the LSP, the only reason it doesn't show up in the editor is that we don't report warnings that are emitted during formatting

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Linter Area: linter
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

🐛 npx rome format prints lots of Unknown lint rule _______ in suppression comment
3 participants