feat(rome_analyze): allow rules to dynamically emit local suppressions #2909
Conversation
|
!bench_analyzer |
Analyzer Benchmark Results |
crates/rome_analyze/src/registry.rs
Outdated
| /// Internal state for a given rule | ||
| #[derive(Clone, Default)] | ||
| struct RuleState<L: Language> { | ||
| suppressions: Rc<RefCell<RuleSuppressions<L>>>, |
There was a problem hiding this comment.
Would it be an alternative to using an Rc + RefCell to split node.rules into
node.rules: Storing the rule + an index into the state arraystates: AVecstoring the states of all Rules.
I don't understand the code well enough to tell if this isn't leading to any other borrow checker issues but may be an alternative to express that the state is shared and that node.rules is just a data structure to get faster rule lookup.
crates/rome_analyze/src/registry.rs
Outdated
| /// Internal state for a given rule | ||
| #[derive(Clone, Default)] | ||
| struct RuleState<L: Language> { | ||
| suppressions: Rc<RefCell<RuleSuppressions<L>>>, |
There was a problem hiding this comment.
Could you explain me what Rc and RefCell are used for here?
There was a problem hiding this comment.
In order to improve query matching performance, a given Rule may actually be represented as multiple RegistryRule withing a registry instance: for instance the NoEmptyPattern will have an entry matching JsArrayBindingPattern and another one matching JsObjectBindingPattern nodes.
Now although these are two distinct entries into the registry, they should share the same underlying RuleState since they belong to the same rule, and Rc allows that by implementing a reference-counted smart pointer managing a single instance of RuleSuppressions shared between multiple entries.
However since the state is now shared and the reference count is a dynamic runtime property, Rust cannot statically check how many outstanding references to the state exists and will not allow it to be mutated, so RefCell gets around that limitation by pushing this verification to the runtime, keeping a counter of outstanding mutable and immutable references and panicking if the code tries to do both at the same time.
Now as @MichaReiser mentioned here it would also be possible to manually manage how the state is shared by storing a vector of RuleState in PhaseRules, and having each RegistryRule simply store an index into that vector instead of an explicit reference-counted pointer. This would allow the Rust compiler to statically check the mutable access to the state and may actually be easier to understand so I'll try implementing this approach instead
| /// matching the `Query`. This is useful for rules that implement a code | ||
| /// action that recursively modifies multiple nodes at once, this hook | ||
| /// allows these rules to avoid matching on those nodes again. | ||
| fn suppressed_nodes( |
There was a problem hiding this comment.
Could you add an example - code snippet - of how to use this new function into the documentation?
Deploying with
|
| Latest commit: |
124084b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4fb64414.tools-8rn.pages.dev |
| Branch Preview URL: | https://feature-rule-suppression.tools-8rn.pages.dev |
|
I am wondering what would be the performance impact of not putting this in the "rule framework", but making it easy for rules to use We would be avoiding 99% of |
I expected this cost to be quite low since the implementation of |
Summary
This PR adds a new
suppressed_nodesmethod to theRuletrait. This hook allows lint rules to emit dynamic node suppressions for their own query, for instance to prevent the rule from matching again on a node that would be removed by a previously-emitted code action.Test Plan
I've modified the
UseTemplaterule to use this new method instead of recursively checking for nested binary expressions, this rule has tests checking the rule does not trigger again on recursive string concatenation expressions