From 3d613a87a02daddfbac1900c6108f87e2fe488b4 Mon Sep 17 00:00:00 2001 From: l3ops Date: Mon, 12 Dec 2022 17:05:18 +0100 Subject: [PATCH 1/7] refactor(rome_analyze): allow queryables to register their own visitor --- crates/rome_analyze/src/lib.rs | 16 ++-- crates/rome_analyze/src/matcher.rs | 2 +- crates/rome_analyze/src/query.rs | 35 ++++---- crates/rome_analyze/src/registry.rs | 85 ++++++++++++++++--- crates/rome_analyze/src/syntax.rs | 9 +- .../analyzers/correctness/no_unreachable.rs | 12 +-- crates/rome_js_analyze/src/aria_services.rs | 10 ++- crates/rome_js_analyze/src/control_flow.rs | 25 +++++- crates/rome_js_analyze/src/lib.rs | 72 +++------------- .../rome_js_analyze/src/semantic_services.rs | 20 +++-- 10 files changed, 170 insertions(+), 116 deletions(-) diff --git a/crates/rome_analyze/src/lib.rs b/crates/rome_analyze/src/lib.rs index bd714e72097..a92e170e072 100644 --- a/crates/rome_analyze/src/lib.rs +++ b/crates/rome_analyze/src/lib.rs @@ -28,7 +28,7 @@ pub use crate::diagnostics::AnalyzerDiagnostic; use crate::diagnostics::SuppressionDiagnostic; pub use crate::matcher::{InspectMatcher, MatchQueryParams, QueryMatcher, RuleKey, SignalEntry}; pub use crate::options::{AnalyzerConfiguration, AnalyzerOptions, AnalyzerRules}; -pub use crate::query::{Ast, QueryKey, QueryMatch, Queryable}; +pub use crate::query::{AddVisitor, Ast, QueryKey, QueryMatch, Queryable}; pub use crate::registry::{ LanguageRoot, MetadataRegistry, Phase, Phases, RegistryRuleMetadata, RegistryVisitor, RuleRegistry, RuleRegistryBuilder, RuleSuppressions, @@ -107,14 +107,12 @@ where } } - pub fn add_visitor(&mut self, phase: Phases, visitor: V) - where - V: Visitor + 'analyzer, - { - self.phases - .entry(phase) - .or_default() - .push(Box::new(visitor)); + pub fn add_visitor( + &mut self, + phase: Phases, + visitor: Box + 'analyzer>, + ) { + self.phases.entry(phase).or_default().push(visitor); } pub fn run(self, mut ctx: AnalyzerContext) -> Option { diff --git a/crates/rome_analyze/src/matcher.rs b/crates/rome_analyze/src/matcher.rs index de3da497db1..5ad7cf34a47 100644 --- a/crates/rome_analyze/src/matcher.rs +++ b/crates/rome_analyze/src/matcher.rs @@ -338,7 +338,7 @@ mod tests { &mut emit_signal, ); - analyzer.add_visitor(Phases::Syntax, SyntaxVisitor::default()); + analyzer.add_visitor(Phases::Syntax, Box::new(SyntaxVisitor::default())); let ctx: AnalyzerContext = AnalyzerContext { file_id: FileId::zero(), diff --git a/crates/rome_analyze/src/query.rs b/crates/rome_analyze/src/query.rs index 2d490feaa64..7846a3169b4 100644 --- a/crates/rome_analyze/src/query.rs +++ b/crates/rome_analyze/src/query.rs @@ -4,7 +4,7 @@ use rome_rowan::{AstNode, Language, SyntaxKindSet, SyntaxNode, TextRange}; use crate::{ registry::{NodeLanguage, Phase}, services::FromServices, - ServiceBag, + Phases, ServiceBag, SyntaxVisitor, Visitor, }; /// Trait implemented for all types, for example lint rules can query them to emit diagnostics or code actions. @@ -13,6 +13,11 @@ pub trait Queryable: Sized { type Language: Language; type Services: FromServices + Phase; + fn build_visitor( + analyzer: &mut impl AddVisitor, + root: &::Root, + ); + /// Statically declares which [QueryMatch] variant is matched by this /// [Queryable] type. For instance the [Ast] queryable matches on /// [QueryMatch::Syntax], so its key is defined as [QueryKey::Syntax] @@ -26,6 +31,12 @@ pub trait Queryable: Sized { fn unwrap_match(services: &ServiceBag, query: &QueryMatch) -> Self::Output; } +pub trait AddVisitor { + fn add_visitor(&mut self, phase: Phases, visitor: V) + where + V: Visitor + 'static; +} + /// Enumerate all the types of [Queryable] analyzer visitors may emit #[derive(Clone, Debug)] pub enum QueryMatch { @@ -63,6 +74,13 @@ where type Language = NodeLanguage; type Services = (); + fn build_visitor( + analyzer: &mut impl AddVisitor, + _: &::Root, + ) { + analyzer.add_visitor(Phases::Syntax, SyntaxVisitor::default()); + } + /// Match on [QueryMatch::Syntax] if the kind of the syntax node matches /// the kind set of `N` const KEY: QueryKey = QueryKey::Syntax(N::KIND_SET); @@ -74,18 +92,3 @@ where } } } - -impl Queryable for ControlFlowGraph { - type Output = Self; - type Language = L; - type Services = (); - - const KEY: QueryKey = QueryKey::ControlFlowGraph; - - fn unwrap_match(_: &ServiceBag, query: &QueryMatch) -> Self::Output { - match query { - QueryMatch::ControlFlowGraph(cfg, _) => cfg.clone(), - _ => panic!("tried to unwrap unsupported QueryMatch kind, expected Syntax"), - } - } -} diff --git a/crates/rome_analyze/src/registry.rs b/crates/rome_analyze/src/registry.rs index add4b38ada4..2a2f191d5f3 100644 --- a/crates/rome_analyze/src/registry.rs +++ b/crates/rome_analyze/src/registry.rs @@ -1,20 +1,21 @@ -use std::{borrow, collections::BTreeSet}; +use std::{any::TypeId, borrow, collections::BTreeSet}; use crate::{ - context::RuleContext, + context::{RuleContext, ServiceBagRuleOptionsWrapper}, matcher::{GroupKey, MatchQueryParams}, + options::OptionsDeserializationDiagnostic, query::{QueryKey, QueryMatch, Queryable}, signals::RuleSignal, - AnalysisFilter, GroupCategory, QueryMatcher, Rule, RuleGroup, RuleKey, RuleMetadata, - SignalEntry, + AddVisitor, AnalysisFilter, AnalyzerOptions, DeserializableRuleOptions, GroupCategory, + QueryMatcher, Rule, RuleGroup, RuleKey, RuleMetadata, ServiceBag, SignalEntry, Visitor, }; use rome_diagnostics::Error; use rome_rowan::{AstNode, Language, RawSyntaxKind, SyntaxKind, SyntaxNode}; -use rustc_hash::FxHashSet; +use rustc_hash::{FxHashMap, FxHashSet}; /// Defines all the phases that the [RuleRegistry] supports. #[repr(usize)] -#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] pub enum Phases { Syntax = 0, Semantic = 1, @@ -103,12 +104,21 @@ pub struct RuleRegistry { } impl RuleRegistry { - pub fn builder<'a>(filter: &'a AnalysisFilter<'a>) -> RuleRegistryBuilder<'a, L> { + pub fn builder<'a>( + filter: &'a AnalysisFilter<'a>, + options: &'a AnalyzerOptions, + root: &'a L::Root, + ) -> RuleRegistryBuilder<'a, L> { RuleRegistryBuilder { + filter, + options, + root, registry: RuleRegistry { phase_rules: Default::default(), }, - filter, + visitors: FxHashMap::default(), + services: ServiceBag::default(), + diagnostics: Vec::new(), } } } @@ -126,8 +136,16 @@ struct PhaseRules { } pub struct RuleRegistryBuilder<'a, L: Language> { - registry: RuleRegistry, filter: &'a AnalysisFilter<'a>, + options: &'a AnalyzerOptions, + root: &'a L::Root, + // Rule Registry + registry: RuleRegistry, + // Analyzer Visitors + visitors: FxHashMap<(Phases, TypeId), Box>>, + // Service Bag + services: ServiceBag, + diagnostics: Vec, } impl RegistryVisitor for RuleRegistryBuilder<'_, L> { @@ -189,12 +207,57 @@ impl RegistryVisitor for RuleRegistryBuilder<'_, L> { } phase.rule_states.push(RuleState::default()); + + let rule_key = RuleKey::rule::(); + let options = if let Some(options) = self.options.configuration.rules.get_rule(&rule_key) { + let value = options.value(); + match ::try_from(value.clone()) { + Ok(result) => Ok(result), + Err(error) => Err(OptionsDeserializationDiagnostic::new( + rule_key.rule_name(), + value.to_string(), + error, + )), + } + } else { + Ok(::default()) + }; + + match options { + Ok(options) => self + .services + .insert_service(ServiceBagRuleOptionsWrapper::(options)), + Err(err) => self.diagnostics.push(err), + } + + ::build_visitor(&mut self.visitors, self.root); + } +} + +impl AddVisitor for FxHashMap<(Phases, TypeId), Box>> { + fn add_visitor(&mut self, phase: Phases, visitor: V) + where + V: Visitor + 'static, + { + self.insert((phase, TypeId::of::()), Box::new(visitor)); } } +type BuilderResult = ( + RuleRegistry, + ServiceBag, + Vec, + FxHashMap<(Phases, TypeId), Box>>, +); + impl RuleRegistryBuilder<'_, L> { - pub fn build(self) -> RuleRegistry { - self.registry + pub fn build(self) -> BuilderResult { + ( + self.registry, + self.services, + self.diagnostics, + self.visitors, + ) } } diff --git a/crates/rome_analyze/src/syntax.rs b/crates/rome_analyze/src/syntax.rs index 838dae8641b..2e16cd95e08 100644 --- a/crates/rome_analyze/src/syntax.rs +++ b/crates/rome_analyze/src/syntax.rs @@ -2,7 +2,6 @@ use rome_rowan::{Language, SyntaxNode, WalkEvent}; use crate::{QueryMatch, Visitor, VisitorContext}; -#[derive(Default)] /// The [SyntaxVisitor] is the simplest form of visitor implemented for the /// analyzer, it simply broadcast each [WalkEvent::Enter] as a query match /// event for the [SyntaxNode] being entered @@ -14,6 +13,12 @@ pub struct SyntaxVisitor { skip_subtree: Option>, } +impl Default for SyntaxVisitor { + fn default() -> Self { + Self { skip_subtree: None } + } +} + impl Visitor for SyntaxVisitor { type Language = L; @@ -116,7 +121,7 @@ mod tests { &mut emit_signal, ); - analyzer.add_visitor(Phases::Syntax, SyntaxVisitor::default()); + analyzer.add_visitor(Phases::Syntax, Box::new(SyntaxVisitor::default())); let ctx: AnalyzerContext = AnalyzerContext { file_id: FileId::zero(), diff --git a/crates/rome_js_analyze/src/analyzers/correctness/no_unreachable.rs b/crates/rome_js_analyze/src/analyzers/correctness/no_unreachable.rs index bf40d2cf482..fc3f7ae8d5f 100644 --- a/crates/rome_js_analyze/src/analyzers/correctness/no_unreachable.rs +++ b/crates/rome_js_analyze/src/analyzers/correctness/no_unreachable.rs @@ -14,7 +14,7 @@ use rome_js_syntax::{ use rome_rowan::{declare_node_union, AstNode}; use rustc_hash::FxHashMap; -use crate::control_flow::ControlFlowGraph; +use crate::control_flow::{ControlFlowGraph, JsControlFlowGraph}; declare_rule! { /// Disallow unreachable code @@ -202,13 +202,13 @@ impl Rule for NoUnreachable { /// simple reachability analysis instead of the fine analysis const COMPLEXITY_THRESHOLD: u32 = 20; -/// Returns true if the "complexity score" for the [ControlFlowGraph] is higher +/// Returns true if the "complexity score" for the [JsControlFlowGraph] is higher /// than [COMPLEXITY_THRESHOLD]. This score is an arbritrary value (the formula /// is similar to the cyclomatic complexity of the function but this is only /// approximative) used to determine whether the NoDeadCode rule should perform /// a fine reachability analysis or fall back to a simpler algorithm to avoid /// spending too much time analyzing exceedingly complex functions -fn exceeds_complexity_threshold(cfg: &ControlFlowGraph) -> bool { +fn exceeds_complexity_threshold(cfg: &JsControlFlowGraph) -> bool { let nodes = cfg.blocks.len() as u32; let mut edges: u32 = 0; @@ -256,7 +256,7 @@ fn exceeds_complexity_threshold(cfg: &ControlFlowGraph) -> bool { /// Perform a simple reachability analysis, does not attempt to determine a /// terminator instruction for unreachable ranges allowing blocks to be visited /// at most once and ensuring the algorithm finishes in a bounded time -fn analyze_simple(cfg: &ControlFlowGraph, signals: &mut UnreachableRanges) { +fn analyze_simple(cfg: &JsControlFlowGraph, signals: &mut UnreachableRanges) { // Perform a simple reachability analysis on the control flow graph by // traversing the function starting at the entry point let mut reachable_blocks = RoaringBitmap::new(); @@ -361,7 +361,7 @@ fn analyze_simple(cfg: &ControlFlowGraph, signals: &mut UnreachableRanges) { /// the reachability of each block and instruction but also find one or more /// "terminator instructions" for each unreachable range of code that cause it /// to be impossible to reach -fn analyze_fine(cfg: &ControlFlowGraph, signals: &mut UnreachableRanges) { +fn analyze_fine(cfg: &JsControlFlowGraph, signals: &mut UnreachableRanges) { // Traverse the CFG and calculate block / instruction reachability let block_paths = traverse_cfg(cfg, signals); @@ -427,7 +427,7 @@ struct PathState<'cfg> { /// Perform a simple reachability analysis on the control flow graph by /// traversing the function starting at the entry points fn traverse_cfg( - cfg: &ControlFlowGraph, + cfg: &JsControlFlowGraph, signals: &mut UnreachableRanges, ) -> FxHashMap>>> { let mut queue = VecDeque::new(); diff --git a/crates/rome_js_analyze/src/aria_services.rs b/crates/rome_js_analyze/src/aria_services.rs index 049144a2b19..b3cbb4e1efe 100644 --- a/crates/rome_js_analyze/src/aria_services.rs +++ b/crates/rome_js_analyze/src/aria_services.rs @@ -1,9 +1,9 @@ use rome_analyze::{ - FromServices, MissingServicesDiagnostic, Phase, Phases, QueryKey, QueryMatch, Queryable, - RuleKey, ServiceBag, + AddVisitor, FromServices, MissingServicesDiagnostic, Phase, Phases, QueryKey, QueryMatch, + Queryable, RuleKey, ServiceBag, SyntaxVisitor, }; use rome_aria::{AriaProperties, AriaRoles}; -use rome_js_syntax::JsLanguage; +use rome_js_syntax::{AnyJsRoot, JsLanguage}; use rome_rowan::AstNode; use std::sync::Arc; @@ -59,6 +59,10 @@ where type Language = JsLanguage; type Services = AriaServices; + fn build_visitor(analyzer: &mut impl AddVisitor, _: &AnyJsRoot) { + analyzer.add_visitor(Phases::Syntax, SyntaxVisitor::default()); + } + /// Match on [QueryMatch::Syntax] if the kind of the syntax node matches /// the kind set of `N` const KEY: QueryKey = QueryKey::Syntax(N::KIND_SET); diff --git a/crates/rome_js_analyze/src/control_flow.rs b/crates/rome_js_analyze/src/control_flow.rs index 58236c04930..1e85953bf06 100644 --- a/crates/rome_js_analyze/src/control_flow.rs +++ b/crates/rome_js_analyze/src/control_flow.rs @@ -1,6 +1,8 @@ +use rome_analyze::{AddVisitor, Phases, QueryKey, QueryMatch, Queryable, ServiceBag}; +use rome_js_syntax::AnyJsRoot; use rome_js_syntax::JsLanguage; -pub(crate) type ControlFlowGraph = rome_control_flow::ControlFlowGraph; +pub(crate) type JsControlFlowGraph = rome_control_flow::ControlFlowGraph; pub(crate) type FunctionBuilder = rome_control_flow::builder::FunctionBuilder; mod nodes; @@ -8,3 +10,24 @@ mod visitor; pub(crate) use self::visitor::make_visitor; pub(crate) use self::visitor::AnyJsControlFlowRoot; + +pub(crate) struct ControlFlowGraph(JsControlFlowGraph); + +impl Queryable for ControlFlowGraph { + type Output = JsControlFlowGraph; + type Language = JsLanguage; + type Services = (); + + fn build_visitor(analyzer: &mut impl AddVisitor, _: &AnyJsRoot) { + analyzer.add_visitor(Phases::Syntax, make_visitor()); + } + + const KEY: QueryKey = QueryKey::ControlFlowGraph; + + fn unwrap_match(_: &ServiceBag, query: &QueryMatch) -> Self::Output { + match query { + QueryMatch::ControlFlowGraph(cfg, _) => cfg.clone(), + _ => panic!("tried to unwrap unsupported QueryMatch kind, expected Syntax"), + } + } +} diff --git a/crates/rome_js_analyze/src/lib.rs b/crates/rome_js_analyze/src/lib.rs index 5129656a82e..cebd498d672 100644 --- a/crates/rome_js_analyze/src/lib.rs +++ b/crates/rome_js_analyze/src/lib.rs @@ -1,13 +1,9 @@ pub use crate::registry::visit_registry; -use crate::semantic_services::{SemanticModelBuilderVisitor, SemanticModelVisitor}; use crate::suppression_action::apply_suppression_comment; -use control_flow::make_visitor; -use rome_analyze::context::ServiceBagRuleOptionsWrapper; -use rome_analyze::options::OptionsDeserializationDiagnostic; use rome_analyze::{ AnalysisFilter, Analyzer, AnalyzerContext, AnalyzerOptions, AnalyzerSignal, ControlFlow, - DeserializableRuleOptions, InspectMatcher, LanguageRoot, MatchQueryParams, MetadataRegistry, - Phases, RuleAction, RuleRegistry, ServiceBag, SuppressionKind, SyntaxVisitor, + InspectMatcher, LanguageRoot, MatchQueryParams, MetadataRegistry, RuleAction, RuleRegistry, + SuppressionKind, }; use rome_aria::{AriaProperties, AriaRoles}; use rome_diagnostics::{category, Diagnostic, FileId}; @@ -47,45 +43,6 @@ pub fn metadata() -> &'static MetadataRegistry { &METADATA } -pub struct RulesConfigurator<'a> { - options: &'a AnalyzerOptions, - services: &'a mut ServiceBag, - diagnostics: Vec, -} - -impl<'a, L: rome_rowan::Language + Default> rome_analyze::RegistryVisitor - for RulesConfigurator<'a> -{ - fn record_rule(&mut self) - where - R: rome_analyze::Rule + 'static, - R::Query: rome_analyze::Queryable, - ::Output: Clone, - { - let rule_key = rome_analyze::RuleKey::rule::(); - let options = if let Some(options) = self.options.configuration.rules.get_rule(&rule_key) { - let value = options.value(); - match ::try_from(value.clone()) { - Ok(result) => result, - Err(error) => { - let err = OptionsDeserializationDiagnostic::new( - rule_key.rule_name(), - value.to_string(), - error, - ); - self.diagnostics.push(err); - ::default() - } - } - } else { - ::default() - }; - - self.services - .insert_service(ServiceBagRuleOptionsWrapper::(options)); - } -} - /// Run the analyzer on the provided `root`: this process will use the given `filter` /// to selectively restrict analysis to specific rules / a specific source range, /// then call `emit_signal` when an analysis rule emits a diagnostic or action. @@ -138,21 +95,14 @@ where result } - let mut registry = RuleRegistry::builder(&filter); + let mut registry = RuleRegistry::builder(&filter, options, root); visit_registry(&mut registry); - // Parse rule options - let mut services = ServiceBag::default(); - let mut configurator = RulesConfigurator { - options, - services: &mut services, - diagnostics: vec![], - }; - visit_registry(&mut configurator); + let (registry, mut services, diagnostics, visitors) = registry.build(); // Bail if we can't parse a rule option - if !configurator.diagnostics.is_empty() { - for diagnostic in configurator.diagnostics { + if !diagnostics.is_empty() { + for diagnostic in diagnostics { emit_signal(&diagnostic); } return None; @@ -160,17 +110,15 @@ where let mut analyzer = Analyzer::new( metadata(), - InspectMatcher::new(registry.build(), inspect_matcher), + InspectMatcher::new(registry, inspect_matcher), parse_linter_suppression_comment, apply_suppression_comment, &mut emit_signal, ); - analyzer.add_visitor(Phases::Syntax, make_visitor()); - analyzer.add_visitor(Phases::Syntax, SyntaxVisitor::default()); - analyzer.add_visitor(Phases::Syntax, SemanticModelBuilderVisitor::new(root)); - analyzer.add_visitor(Phases::Semantic, SemanticModelVisitor); - analyzer.add_visitor(Phases::Semantic, SyntaxVisitor::default()); + for ((phase, _), visitor) in visitors { + analyzer.add_visitor(phase, visitor); + } services.insert_service(Arc::new(AriaRoles::default())); services.insert_service(Arc::new(AriaProperties::default())); diff --git a/crates/rome_js_analyze/src/semantic_services.rs b/crates/rome_js_analyze/src/semantic_services.rs index c1c7605537a..a60ff78b8c1 100644 --- a/crates/rome_js_analyze/src/semantic_services.rs +++ b/crates/rome_js_analyze/src/semantic_services.rs @@ -1,6 +1,6 @@ use rome_analyze::{ - FromServices, MissingServicesDiagnostic, Phase, Phases, QueryKey, QueryMatch, Queryable, - RuleKey, ServiceBag, Visitor, VisitorContext, VisitorFinishContext, + AddVisitor, FromServices, MissingServicesDiagnostic, Phase, Phases, QueryKey, QueryMatch, + Queryable, RuleKey, ServiceBag, SyntaxVisitor, Visitor, VisitorContext, VisitorFinishContext, }; use rome_js_semantic::{SemanticEventExtractor, SemanticModel, SemanticModelBuilder}; use rome_js_syntax::{AnyJsRoot, JsLanguage, WalkEvent}; @@ -43,6 +43,11 @@ impl Queryable for SemanticServices { type Language = JsLanguage; type Services = Self; + fn build_visitor(analyzer: &mut impl AddVisitor, root: &AnyJsRoot) { + analyzer.add_visitor(Phases::Syntax, SemanticModelBuilderVisitor::new(root)); + analyzer.add_visitor(Phases::Semantic, SemanticModelVisitor); + } + const KEY: QueryKey = QueryKey::SemanticModel; fn unwrap_match(services: &ServiceBag, query: &QueryMatch) -> Self::Output { @@ -68,6 +73,11 @@ where type Language = JsLanguage; type Services = SemanticServices; + fn build_visitor(analyzer: &mut impl AddVisitor, root: &AnyJsRoot) { + analyzer.add_visitor(Phases::Syntax, SemanticModelBuilderVisitor::new(root)); + analyzer.add_visitor(Phases::Semantic, SyntaxVisitor::default()); + } + /// Match on [QueryMatch::Syntax] if the kind of the syntax node matches /// the kind set of `N` const KEY: QueryKey = QueryKey::Syntax(N::KIND_SET); @@ -80,13 +90,13 @@ where } } -pub(crate) struct SemanticModelBuilderVisitor { +struct SemanticModelBuilderVisitor { extractor: SemanticEventExtractor, builder: SemanticModelBuilder, } impl SemanticModelBuilderVisitor { - pub(crate) fn new(root: &AnyJsRoot) -> Self { + fn new(root: &AnyJsRoot) -> Self { Self { extractor: SemanticEventExtractor::default(), builder: SemanticModelBuilder::new(root.clone()), @@ -123,7 +133,7 @@ impl Visitor for SemanticModelBuilderVisitor { } } -pub(crate) struct SemanticModelVisitor; +pub struct SemanticModelVisitor; impl Visitor for SemanticModelVisitor { type Language = JsLanguage; From fa28a627f22199bf1a6ad4d0feaffccaec1683f3 Mon Sep 17 00:00:00 2001 From: l3ops Date: Fri, 16 Dec 2022 15:35:17 +0100 Subject: [PATCH 2/7] replace the `QueryMatch` enum with `Box` --- crates/rome_analyze/src/lib.rs | 4 +- crates/rome_analyze/src/matcher.rs | 19 ++--- crates/rome_analyze/src/query.rs | 78 ++++--------------- crates/rome_analyze/src/registry.rs | 69 +++++++++------- crates/rome_analyze/src/syntax.rs | 64 ++++++++++++--- crates/rome_analyze/src/visitor.rs | 5 +- crates/rome_js_analyze/src/aria_services.rs | 21 +++-- crates/rome_js_analyze/src/control_flow.rs | 28 ++++--- .../src/control_flow/visitor.rs | 12 +-- crates/rome_js_analyze/src/lib.rs | 4 +- .../rome_js_analyze/src/semantic_services.rs | 44 ++++++----- .../src/file_handlers/javascript.rs | 20 ++--- 12 files changed, 195 insertions(+), 173 deletions(-) diff --git a/crates/rome_analyze/src/lib.rs b/crates/rome_analyze/src/lib.rs index a92e170e072..b00daaf1355 100644 --- a/crates/rome_analyze/src/lib.rs +++ b/crates/rome_analyze/src/lib.rs @@ -28,7 +28,7 @@ pub use crate::diagnostics::AnalyzerDiagnostic; use crate::diagnostics::SuppressionDiagnostic; pub use crate::matcher::{InspectMatcher, MatchQueryParams, QueryMatcher, RuleKey, SignalEntry}; pub use crate::options::{AnalyzerConfiguration, AnalyzerOptions, AnalyzerRules}; -pub use crate::query::{AddVisitor, Ast, QueryKey, QueryMatch, Queryable}; +pub use crate::query::{AddVisitor, QueryKey, QueryMatch, Queryable}; pub use crate::registry::{ LanguageRoot, MetadataRegistry, Phase, Phases, RegistryRuleMetadata, RegistryVisitor, RuleRegistry, RuleRegistryBuilder, RuleSuppressions, @@ -40,7 +40,7 @@ pub use crate::rule::{ pub use crate::services::{FromServices, MissingServicesDiagnostic, ServiceBag}; use crate::signals::DiagnosticSignal; pub use crate::signals::{AnalyzerAction, AnalyzerSignal}; -pub use crate::syntax::SyntaxVisitor; +pub use crate::syntax::{Ast, SyntaxVisitor}; pub use crate::visitor::{NodeVisitor, Visitor, VisitorContext, VisitorFinishContext}; pub use rule::DeserializableRuleOptions; diff --git a/crates/rome_analyze/src/matcher.rs b/crates/rome_analyze/src/matcher.rs index 5ad7cf34a47..eda568ce7e5 100644 --- a/crates/rome_analyze/src/matcher.rs +++ b/crates/rome_analyze/src/matcher.rs @@ -1,10 +1,9 @@ use crate::{ - AnalyzerSignal, Phases, QueryMatch, Rule, RuleFilter, RuleGroup, ServiceBag, - SuppressionCommentEmitter, + AnalyzerSignal, Phases, Rule, RuleFilter, RuleGroup, ServiceBag, SuppressionCommentEmitter, }; use rome_diagnostics::FileId; use rome_rowan::{Language, TextRange}; -use std::{cmp::Ordering, collections::BinaryHeap}; +use std::{any::Any, cmp::Ordering, collections::BinaryHeap}; /// The [QueryMatcher] trait is responsible of running lint rules on /// [QueryMatch] instances emitted by the various [Visitor](crate::Visitor) @@ -19,7 +18,8 @@ pub struct MatchQueryParams<'phase, 'query, L: Language> { pub phase: Phases, pub file_id: FileId, pub root: &'phase L::Root, - pub query: QueryMatch, + pub text_range: TextRange, + pub query: Box, pub services: &'phase ServiceBag, pub signal_queue: &'query mut BinaryHeap>, pub apply_suppression_comment: SuppressionCommentEmitter, @@ -158,14 +158,14 @@ mod tests { use rome_diagnostics::{Diagnostic, Severity}; use rome_rowan::{ raw_language::{RawLanguage, RawLanguageKind, RawLanguageRoot, RawSyntaxTreeBuilder}, - AstNode, TextRange, TextSize, TriviaPiece, TriviaPieceKind, + AstNode, SyntaxNode, TextRange, TextSize, TriviaPiece, TriviaPieceKind, }; use crate::SuppressionKind; use crate::{ signals::DiagnosticSignal, Analyzer, AnalyzerContext, AnalyzerOptions, AnalyzerSignal, - ControlFlow, MetadataRegistry, Never, Phases, QueryMatch, QueryMatcher, RuleKey, - ServiceBag, SignalEntry, SyntaxVisitor, + ControlFlow, MetadataRegistry, Never, Phases, QueryMatcher, RuleKey, ServiceBag, + SignalEntry, SyntaxVisitor, }; use super::MatchQueryParams; @@ -184,10 +184,7 @@ mod tests { impl QueryMatcher for SuppressionMatcher { /// Emits a warning diagnostic for all literal expressions fn match_query(&mut self, params: MatchQueryParams) { - let node = match params.query { - QueryMatch::Syntax(node) => node, - _ => unreachable!(), - }; + let node = params.query.downcast::>().unwrap(); if node.kind() != RawLanguageKind::LITERAL_EXPRESSION { return; diff --git a/crates/rome_analyze/src/query.rs b/crates/rome_analyze/src/query.rs index 7846a3169b4..3fa9ea43028 100644 --- a/crates/rome_analyze/src/query.rs +++ b/crates/rome_analyze/src/query.rs @@ -1,15 +1,14 @@ -use rome_control_flow::ControlFlowGraph; -use rome_rowan::{AstNode, Language, SyntaxKindSet, SyntaxNode, TextRange}; +use std::any::TypeId; -use crate::{ - registry::{NodeLanguage, Phase}, - services::FromServices, - Phases, ServiceBag, SyntaxVisitor, Visitor, -}; +use rome_rowan::{Language, SyntaxKindSet, TextRange}; + +use crate::{registry::Phase, services::FromServices, Phases, ServiceBag, Visitor}; /// Trait implemented for all types, for example lint rules can query them to emit diagnostics or code actions. pub trait Queryable: Sized { + type Input: QueryMatch; type Output; + type Language: Language; type Services: FromServices + Phase; @@ -18,17 +17,16 @@ pub trait Queryable: Sized { root: &::Root, ); - /// Statically declares which [QueryMatch] variant is matched by this - /// [Queryable] type. For instance the [Ast] queryable matches on - /// [QueryMatch::Syntax], so its key is defined as [QueryKey::Syntax] - const KEY: QueryKey; + fn key() -> QueryKey { + QueryKey::TypeId(TypeId::of::()) + } /// Unwrap an instance of `Self` from a [QueryMatch]. /// /// ## Panics /// /// If the [QueryMatch] variant of `query` doesn't match `Self::KEY` - fn unwrap_match(services: &ServiceBag, query: &QueryMatch) -> Self::Output; + fn unwrap_match(services: &ServiceBag, query: &Self::Input) -> Self::Output; } pub trait AddVisitor { @@ -37,58 +35,14 @@ pub trait AddVisitor { V: Visitor + 'static; } -/// Enumerate all the types of [Queryable] analyzer visitors may emit -#[derive(Clone, Debug)] -pub enum QueryMatch { - Syntax(SyntaxNode), - SemanticModel(TextRange), - ControlFlowGraph(ControlFlowGraph, TextRange), -} - -impl QueryMatch { - pub fn text_range(&self) -> TextRange { - match self { - QueryMatch::Syntax(node) => node.text_trimmed_range(), - QueryMatch::SemanticModel(range) | QueryMatch::ControlFlowGraph(_, range) => *range, - } - } +/// Marker trait implemented for all the types analyzer visitors may emit +pub trait QueryMatch: 'static { + fn text_range(&self) -> TextRange; } -/// Mirrors the variants of [QueryMatch] to statically compute which queries a -/// given [Queryable] type can match +/// Represents which type a given [Queryable] type can match, either a specific +/// subset of syntax node kinds or any generic type pub enum QueryKey { Syntax(SyntaxKindSet), - ControlFlowGraph, - SemanticModel, -} - -/// Query type usable by lint rules to match on specific [AstNode] types -#[derive(Clone)] -pub struct Ast(pub N); - -impl Queryable for Ast -where - N: AstNode + 'static, -{ - type Output = N; - type Language = NodeLanguage; - type Services = (); - - fn build_visitor( - analyzer: &mut impl AddVisitor, - _: &::Root, - ) { - analyzer.add_visitor(Phases::Syntax, SyntaxVisitor::default()); - } - - /// Match on [QueryMatch::Syntax] if the kind of the syntax node matches - /// the kind set of `N` - const KEY: QueryKey = QueryKey::Syntax(N::KIND_SET); - - fn unwrap_match(_: &ServiceBag, query: &QueryMatch) -> Self::Output { - match query { - QueryMatch::Syntax(node) => N::unwrap_cast(node.clone()), - _ => panic!("tried to unwrap unsupported QueryMatch kind, expected Syntax"), - } - } + TypeId(TypeId), } diff --git a/crates/rome_analyze/src/registry.rs b/crates/rome_analyze/src/registry.rs index 2a2f191d5f3..be3999eebe5 100644 --- a/crates/rome_analyze/src/registry.rs +++ b/crates/rome_analyze/src/registry.rs @@ -4,7 +4,7 @@ use crate::{ context::{RuleContext, ServiceBagRuleOptionsWrapper}, matcher::{GroupKey, MatchQueryParams}, options::OptionsDeserializationDiagnostic, - query::{QueryKey, QueryMatch, Queryable}, + query::{QueryKey, Queryable}, signals::RuleSignal, AddVisitor, AnalysisFilter, AnalyzerOptions, DeserializableRuleOptions, GroupCategory, QueryMatcher, Rule, RuleGroup, RuleKey, RuleMetadata, ServiceBag, SignalEntry, Visitor, @@ -126,15 +126,16 @@ impl RuleRegistry { /// Holds a collection of rules for each phase. #[derive(Default)] struct PhaseRules { - /// Holds a collection of rules for each [SyntaxKind] node type that has - /// lint rules associated with it - ast_rules: Vec>, - control_flow: Vec>, - semantic_model: Vec>, + type_rules: FxHashMap>, /// Holds a list of states for all the rules in this phase rule_states: Vec>, } +enum TypeRules { + SyntaxRules { rules: Vec> }, + TypeRules { rules: Vec> }, +} + pub struct RuleRegistryBuilder<'a, L: Language> { filter: &'a AnalysisFilter<'a>, options: &'a AnalyzerOptions, @@ -148,7 +149,7 @@ pub struct RuleRegistryBuilder<'a, L: Language> { diagnostics: Vec, } -impl RegistryVisitor for RuleRegistryBuilder<'_, L> { +impl RegistryVisitor for RuleRegistryBuilder<'_, L> { fn record_category>(&mut self) { if self.filter.match_category::() { C::record_groups(self); @@ -177,8 +178,14 @@ impl RegistryVisitor for RuleRegistryBuilder<'_, L> { let rule = RegistryRule::new::(phase.rule_states.len()); - match ::KEY { + match ::key() { QueryKey::Syntax(key) => { + let TypeRules::SyntaxRules { rules } = phase + .type_rules + .entry(TypeId::of::>()) + .or_insert_with(|| TypeRules::SyntaxRules { rules: Vec::new() }) + else { unreachable!() }; + // Iterate on all the SyntaxKind variants this node can match for kind in key.iter() { // Convert the numerical value of `kind` to an index in the @@ -188,21 +195,24 @@ impl RegistryVisitor for RuleRegistryBuilder<'_, L> { // Ensure the vector has enough capacity by inserting empty // `SyntaxKindRules` as required - if phase.ast_rules.len() <= index { - phase.ast_rules.resize_with(index + 1, SyntaxKindRules::new); + if rules.len() <= index { + rules.resize_with(index + 1, SyntaxKindRules::new); } // Insert a handle to the rule `R` into the `SyntaxKindRules` entry // corresponding to the SyntaxKind index - let node = &mut phase.ast_rules[index]; + let node = &mut rules[index]; node.rules.push(rule); } } - QueryKey::ControlFlowGraph => { - phase.control_flow.push(rule); - } - QueryKey::SemanticModel => { - phase.semantic_model.push(rule); + QueryKey::TypeId(key) => { + let TypeRules::TypeRules { rules } = phase + .type_rules + .entry(key) + .or_insert_with(|| TypeRules::TypeRules { rules: Vec::new() }) + else { unreachable!() }; + + rules.push(rule); } } @@ -261,25 +271,28 @@ impl RuleRegistryBuilder<'_, L> { } } -impl QueryMatcher for RuleRegistry { +impl QueryMatcher for RuleRegistry { fn match_query(&mut self, mut params: MatchQueryParams) { let phase = &mut self.phase_rules[params.phase as usize]; - let rules = match ¶ms.query { - QueryMatch::Syntax(node) => { + let Some(rules) = phase.type_rules.get(¶ms.query.type_id()) else { return }; + + let rules = match rules { + TypeRules::SyntaxRules { rules } => { + let node = params.query.downcast_ref::>().unwrap(); + // Convert the numerical value of the SyntaxKind to an index in the // `syntax` vector let RawSyntaxKind(kind) = node.kind().to_raw(); let kind = usize::from(kind); // Lookup the syntax entry corresponding to the SyntaxKind index - match phase.ast_rules.get_mut(kind) { - Some(entry) => &mut entry.rules, + match rules.get(kind) { + Some(entry) => &entry.rules, None => return, } } - QueryMatch::ControlFlowGraph(..) => &mut phase.control_flow, - QueryMatch::SemanticModel(..) => &mut phase.semantic_model, + TypeRules::TypeRules { rules } => rules, }; // Run all the rules registered to this QueryMatch @@ -397,7 +410,7 @@ impl RegistryRule { R::Query: 'static, ::Output: Clone, { - if let QueryMatch::Syntax(node) = ¶ms.query { + if let Some(node) = params.query.downcast_ref::>>() { if state.suppressions.inner.contains(node) { return Ok(()); } @@ -405,16 +418,16 @@ impl RegistryRule { // SAFETY: The rule should never get executed in the first place // if the query doesn't match - let query_result = - ::unwrap_match(params.services, ¶ms.query); + let query_result = params.query.downcast_ref().unwrap(); + let query_result = ::unwrap_match(params.services, query_result); + let ctx = match RuleContext::new(&query_result, params.root, params.services) { Ok(ctx) => ctx, Err(error) => return Err(error), }; for result in R::run(&ctx) { - let text_range = - R::text_range(&ctx, &result).unwrap_or_else(|| params.query.text_range()); + let text_range = R::text_range(&ctx, &result).unwrap_or(params.text_range); R::suppressed_nodes(&ctx, &result, &mut state.suppressions); diff --git a/crates/rome_analyze/src/syntax.rs b/crates/rome_analyze/src/syntax.rs index 2e16cd95e08..6ab87bf3739 100644 --- a/crates/rome_analyze/src/syntax.rs +++ b/crates/rome_analyze/src/syntax.rs @@ -1,6 +1,45 @@ -use rome_rowan::{Language, SyntaxNode, WalkEvent}; +use rome_rowan::{AstNode, Language, SyntaxNode, WalkEvent}; + +use crate::{ + registry::NodeLanguage, AddVisitor, Phases, QueryKey, QueryMatch, Queryable, ServiceBag, + Visitor, VisitorContext, +}; + +/// Query type usable by lint rules to match on specific [AstNode] types +#[derive(Clone)] +pub struct Ast(pub N); + +impl Queryable for Ast +where + N: AstNode + 'static, +{ + type Input = SyntaxNode>; + type Output = N; + + type Language = NodeLanguage; + type Services = (); + + fn build_visitor( + analyzer: &mut impl AddVisitor, + _: &::Root, + ) { + analyzer.add_visitor(Phases::Syntax, SyntaxVisitor::default()); + } + + fn key() -> QueryKey { + QueryKey::Syntax(N::KIND_SET) + } + + fn unwrap_match(_: &ServiceBag, node: &Self::Input) -> Self::Output { + N::unwrap_cast(node.clone()) + } +} -use crate::{QueryMatch, Visitor, VisitorContext}; +impl QueryMatch for SyntaxNode { + fn text_range(&self) -> rome_rowan::TextRange { + self.text_trimmed_range() + } +} /// The [SyntaxVisitor] is the simplest form of visitor implemented for the /// analyzer, it simply broadcast each [WalkEvent::Enter] as a query match @@ -19,7 +58,7 @@ impl Default for SyntaxVisitor { } } -impl Visitor for SyntaxVisitor { +impl Visitor for SyntaxVisitor { type Language = L; fn visit(&mut self, event: &WalkEvent>, mut ctx: VisitorContext) { @@ -47,7 +86,7 @@ impl Visitor for SyntaxVisitor { } } - ctx.match_query(QueryMatch::Syntax(node.clone())); + ctx.match_query(node.clone()); } } @@ -59,12 +98,12 @@ mod tests { use rome_diagnostics::location::FileId; use rome_rowan::{ raw_language::{RawLanguage, RawLanguageKind, RawLanguageRoot, RawSyntaxTreeBuilder}, - AstNode, + AstNode, SyntaxNode, }; use crate::{ matcher::MatchQueryParams, registry::Phases, Analyzer, AnalyzerContext, AnalyzerOptions, - AnalyzerSignal, ControlFlow, MetadataRegistry, Never, QueryMatch, QueryMatcher, ServiceBag, + AnalyzerSignal, ControlFlow, MetadataRegistry, Never, QueryMatcher, ServiceBag, SyntaxVisitor, }; @@ -75,12 +114,13 @@ mod tests { impl<'a> QueryMatcher for &'a mut BufferMatcher { fn match_query(&mut self, params: MatchQueryParams) { - match params.query { - QueryMatch::Syntax(node) => { - self.nodes.push(node.kind()); - } - _ => unreachable!(), - } + self.nodes.push( + params + .query + .downcast::>() + .unwrap() + .kind(), + ); } } diff --git a/crates/rome_analyze/src/visitor.rs b/crates/rome_analyze/src/visitor.rs index 2ef04c16838..bd749fd8d11 100644 --- a/crates/rome_analyze/src/visitor.rs +++ b/crates/rome_analyze/src/visitor.rs @@ -22,12 +22,13 @@ pub struct VisitorContext<'phase, 'query, L: Language> { } impl<'phase, 'query, L: Language> VisitorContext<'phase, 'query, L> { - pub fn match_query(&mut self, query: QueryMatch) { + pub fn match_query(&mut self, query: T) { self.query_matcher.match_query(MatchQueryParams { phase: self.phase, file_id: self.file_id, root: self.root, - query, + text_range: query.text_range(), + query: Box::new(query), services: self.services, signal_queue: self.signal_queue, apply_suppression_comment: self.apply_suppression_comment, diff --git a/crates/rome_js_analyze/src/aria_services.rs b/crates/rome_js_analyze/src/aria_services.rs index b3cbb4e1efe..9165dd63e52 100644 --- a/crates/rome_js_analyze/src/aria_services.rs +++ b/crates/rome_js_analyze/src/aria_services.rs @@ -1,9 +1,9 @@ use rome_analyze::{ - AddVisitor, FromServices, MissingServicesDiagnostic, Phase, Phases, QueryKey, QueryMatch, - Queryable, RuleKey, ServiceBag, SyntaxVisitor, + AddVisitor, FromServices, MissingServicesDiagnostic, Phase, Phases, QueryKey, Queryable, + RuleKey, ServiceBag, SyntaxVisitor, }; use rome_aria::{AriaProperties, AriaRoles}; -use rome_js_syntax::{AnyJsRoot, JsLanguage}; +use rome_js_syntax::{AnyJsRoot, JsLanguage, JsSyntaxNode}; use rome_rowan::AstNode; use std::sync::Arc; @@ -55,7 +55,9 @@ impl Queryable for Aria where N: AstNode + 'static, { + type Input = JsSyntaxNode; type Output = N; + type Language = JsLanguage; type Services = AriaServices; @@ -63,14 +65,11 @@ where analyzer.add_visitor(Phases::Syntax, SyntaxVisitor::default()); } - /// Match on [QueryMatch::Syntax] if the kind of the syntax node matches - /// the kind set of `N` - const KEY: QueryKey = QueryKey::Syntax(N::KIND_SET); + fn key() -> QueryKey { + QueryKey::Syntax(N::KIND_SET) + } - fn unwrap_match(_: &ServiceBag, query: &QueryMatch) -> Self::Output { - match query { - QueryMatch::Syntax(node) => N::unwrap_cast(node.clone()), - _ => panic!("tried to unwrap unsupported QueryMatch kind, expected Syntax"), - } + fn unwrap_match(_: &ServiceBag, node: &Self::Input) -> Self::Output { + N::unwrap_cast(node.clone()) } } diff --git a/crates/rome_js_analyze/src/control_flow.rs b/crates/rome_js_analyze/src/control_flow.rs index 1e85953bf06..0507bf2d90a 100644 --- a/crates/rome_js_analyze/src/control_flow.rs +++ b/crates/rome_js_analyze/src/control_flow.rs @@ -1,8 +1,10 @@ -use rome_analyze::{AddVisitor, Phases, QueryKey, QueryMatch, Queryable, ServiceBag}; +use rome_analyze::QueryMatch; +use rome_analyze::{AddVisitor, Phases, Queryable, ServiceBag}; use rome_js_syntax::AnyJsRoot; use rome_js_syntax::JsLanguage; +use rome_js_syntax::TextRange; -pub(crate) type JsControlFlowGraph = rome_control_flow::ControlFlowGraph; +pub type JsControlFlowGraph = rome_control_flow::ControlFlowGraph; pub(crate) type FunctionBuilder = rome_control_flow::builder::FunctionBuilder; mod nodes; @@ -11,10 +13,21 @@ mod visitor; pub(crate) use self::visitor::make_visitor; pub(crate) use self::visitor::AnyJsControlFlowRoot; -pub(crate) struct ControlFlowGraph(JsControlFlowGraph); +pub struct ControlFlowGraph { + pub graph: JsControlFlowGraph, + pub range: TextRange, +} + +impl QueryMatch for ControlFlowGraph { + fn text_range(&self) -> TextRange { + self.range + } +} impl Queryable for ControlFlowGraph { + type Input = ControlFlowGraph; type Output = JsControlFlowGraph; + type Language = JsLanguage; type Services = (); @@ -22,12 +35,7 @@ impl Queryable for ControlFlowGraph { analyzer.add_visitor(Phases::Syntax, make_visitor()); } - const KEY: QueryKey = QueryKey::ControlFlowGraph; - - fn unwrap_match(_: &ServiceBag, query: &QueryMatch) -> Self::Output { - match query { - QueryMatch::ControlFlowGraph(cfg, _) => cfg.clone(), - _ => panic!("tried to unwrap unsupported QueryMatch kind, expected Syntax"), - } + fn unwrap_match(_: &ServiceBag, query: &ControlFlowGraph) -> Self::Output { + query.graph.clone() } } diff --git a/crates/rome_js_analyze/src/control_flow/visitor.rs b/crates/rome_js_analyze/src/control_flow/visitor.rs index 683d3b7dcce..437370edf5f 100644 --- a/crates/rome_js_analyze/src/control_flow/visitor.rs +++ b/crates/rome_js_analyze/src/control_flow/visitor.rs @@ -1,6 +1,6 @@ use std::any::TypeId; -use rome_analyze::{merge_node_visitors, QueryMatch, Visitor, VisitorContext}; +use rome_analyze::{merge_node_visitors, Visitor, VisitorContext}; use rome_js_syntax::{ AnyJsFunction, JsConstructorClassMember, JsGetterClassMember, JsGetterObjectMember, JsLanguage, JsMethodClassMember, JsMethodObjectMember, JsModule, JsScript, JsSetterClassMember, @@ -8,6 +8,8 @@ use rome_js_syntax::{ }; use rome_rowan::{declare_node_union, AstNode, SyntaxError, SyntaxResult}; +use crate::ControlFlowGraph; + use super::{nodes::*, FunctionBuilder}; /// Return a new instance of the [ControlFlowVisitor] @@ -188,10 +190,10 @@ impl rome_analyze::NodeVisitor for FunctionVisitor { _: &mut ControlFlowVisitor, ) { if let Some(builder) = self.builder { - ctx.match_query(QueryMatch::ControlFlowGraph( - builder.finish(), - node.syntax().text_trimmed_range(), - )); + ctx.match_query(ControlFlowGraph { + graph: builder.finish(), + range: node.syntax().text_trimmed_range(), + }); } } } diff --git a/crates/rome_js_analyze/src/lib.rs b/crates/rome_js_analyze/src/lib.rs index cebd498d672..6a385c433b6 100644 --- a/crates/rome_js_analyze/src/lib.rs +++ b/crates/rome_js_analyze/src/lib.rs @@ -1,4 +1,3 @@ -pub use crate::registry::visit_registry; use crate::suppression_action::apply_suppression_comment; use rome_analyze::{ AnalysisFilter, Analyzer, AnalyzerContext, AnalyzerOptions, AnalyzerSignal, ControlFlow, @@ -28,6 +27,9 @@ mod suppression_action; mod syntax; pub mod utils; +pub use crate::control_flow::ControlFlowGraph; +pub use crate::registry::visit_registry; + pub(crate) type JsRuleAction = RuleAction; /// Return the static [MetadataRegistry] for the JS analyzer rules diff --git a/crates/rome_js_analyze/src/semantic_services.rs b/crates/rome_js_analyze/src/semantic_services.rs index a60ff78b8c1..0a67e25ce6e 100644 --- a/crates/rome_js_analyze/src/semantic_services.rs +++ b/crates/rome_js_analyze/src/semantic_services.rs @@ -3,7 +3,7 @@ use rome_analyze::{ Queryable, RuleKey, ServiceBag, SyntaxVisitor, Visitor, VisitorContext, VisitorFinishContext, }; use rome_js_semantic::{SemanticEventExtractor, SemanticModel, SemanticModelBuilder}; -use rome_js_syntax::{AnyJsRoot, JsLanguage, WalkEvent}; +use rome_js_syntax::{AnyJsRoot, JsLanguage, JsSyntaxNode, TextRange, WalkEvent}; use rome_rowan::{AstNode, SyntaxNode}; pub struct SemanticServices { @@ -39,7 +39,9 @@ impl Phase for SemanticServices { /// The [SemanticServices] types can be used as a queryable to get an instance /// of the whole [SemanticModel] without matching on a specific AST node impl Queryable for SemanticServices { + type Input = SemanticModelEvent; type Output = SemanticModel; + type Language = JsLanguage; type Services = Self; @@ -48,16 +50,11 @@ impl Queryable for SemanticServices { analyzer.add_visitor(Phases::Semantic, SemanticModelVisitor); } - const KEY: QueryKey = QueryKey::SemanticModel; - - fn unwrap_match(services: &ServiceBag, query: &QueryMatch) -> Self::Output { - match query { - QueryMatch::SemanticModel(..) => services - .get_service::() - .expect("SemanticModel service is not registered") - .clone(), - _ => panic!("tried to unwrap unsupported QueryMatch kind, expected SemanticModel"), - } + fn unwrap_match(services: &ServiceBag, _: &SemanticModelEvent) -> Self::Output { + services + .get_service::() + .expect("SemanticModel service is not registered") + .clone() } } @@ -69,7 +66,9 @@ impl Queryable for Semantic where N: AstNode + 'static, { + type Input = JsSyntaxNode; type Output = N; + type Language = JsLanguage; type Services = SemanticServices; @@ -78,15 +77,12 @@ where analyzer.add_visitor(Phases::Semantic, SyntaxVisitor::default()); } - /// Match on [QueryMatch::Syntax] if the kind of the syntax node matches - /// the kind set of `N` - const KEY: QueryKey = QueryKey::Syntax(N::KIND_SET); + fn key() -> QueryKey { + QueryKey::Syntax(N::KIND_SET) + } - fn unwrap_match(_: &ServiceBag, query: &QueryMatch) -> Self::Output { - match query { - QueryMatch::Syntax(node) => N::unwrap_cast(node.clone()), - _ => panic!("tried to unwrap unsupported QueryMatch kind, expected Syntax"), - } + fn unwrap_match(_: &ServiceBag, node: &Self::Input) -> Self::Output { + N::unwrap_cast(node.clone()) } } @@ -135,6 +131,14 @@ impl Visitor for SemanticModelBuilderVisitor { pub struct SemanticModelVisitor; +pub struct SemanticModelEvent(TextRange); + +impl QueryMatch for SemanticModelEvent { + fn text_range(&self) -> TextRange { + self.0 + } +} + impl Visitor for SemanticModelVisitor { type Language = JsLanguage; @@ -155,6 +159,6 @@ impl Visitor for SemanticModelVisitor { }; let text_range = root.text_range(); - ctx.match_query(QueryMatch::SemanticModel(text_range)); + ctx.match_query(SemanticModelEvent(text_range)); } } diff --git a/crates/rome_service/src/file_handlers/javascript.rs b/crates/rome_service/src/file_handlers/javascript.rs index 50b285d81a5..f30d5c6eb73 100644 --- a/crates/rome_service/src/file_handlers/javascript.rs +++ b/crates/rome_service/src/file_handlers/javascript.rs @@ -14,14 +14,16 @@ use crate::{ }; use indexmap::IndexSet; use rome_analyze::{ - AnalysisFilter, AnalyzerOptions, ControlFlow, GroupCategory, Never, QueryMatch, - RegistryVisitor, RuleCategories, RuleCategory, RuleFilter, RuleGroup, + AnalysisFilter, AnalyzerOptions, ControlFlow, GroupCategory, Never, RegistryVisitor, + RuleCategories, RuleCategory, RuleFilter, RuleGroup, }; use rome_diagnostics::{category, Applicability, Diagnostic, DiagnosticExt, Severity}; use rome_formatter::{FormatError, Printed}; use rome_fs::RomePath; use rome_js_analyze::utils::rename::{RenameError, RenameSymbolExtensions}; -use rome_js_analyze::{analyze, analyze_with_inspect_matcher, visit_registry, RuleError}; +use rome_js_analyze::{ + analyze, analyze_with_inspect_matcher, visit_registry, ControlFlowGraph, RuleError, +}; use rome_js_formatter::context::{ trailing_comma::TrailingComma, QuoteProperties, QuoteStyle, Semicolons, }; @@ -153,22 +155,22 @@ fn debug_control_flow(rome_path: &RomePath, parse: AnyParse, cursor: TextSize) - &parse.tree(), filter, |match_params| { - let (cfg, range) = match &match_params.query { - QueryMatch::ControlFlowGraph(cfg, node) => (cfg, node), + let cfg = match match_params.query.downcast_ref::() { + Some(cfg) => cfg, _ => return, }; - if !range.contains(cursor) { + if !cfg.range.contains(cursor) { return; } match &control_flow_graph { None => { - control_flow_graph = Some((cfg.to_string(), *range)); + control_flow_graph = Some((cfg.graph.to_string(), cfg.range)); } Some((_, prev_range)) => { - if range.len() < prev_range.len() { - control_flow_graph = Some((cfg.to_string(), *range)); + if cfg.range.len() < prev_range.len() { + control_flow_graph = Some((cfg.graph.to_string(), cfg.range)); } } } From e5e6a0f21a1580668c718478d67ddad5918416e9 Mon Sep 17 00:00:00 2001 From: l3ops Date: Fri, 16 Dec 2022 16:09:49 +0100 Subject: [PATCH 3/7] store visitors in a BTreeMap to register them in a stable order --- crates/rome_analyze/src/query.rs | 3 ++- crates/rome_analyze/src/registry.rs | 20 ++++++++++++------- crates/rome_analyze/src/syntax.rs | 2 +- crates/rome_js_analyze/src/aria_services.rs | 2 +- crates/rome_js_analyze/src/control_flow.rs | 2 +- .../rome_js_analyze/src/semantic_services.rs | 8 ++++---- 6 files changed, 22 insertions(+), 15 deletions(-) diff --git a/crates/rome_analyze/src/query.rs b/crates/rome_analyze/src/query.rs index 3fa9ea43028..7fa08e5b648 100644 --- a/crates/rome_analyze/src/query.rs +++ b/crates/rome_analyze/src/query.rs @@ -30,8 +30,9 @@ pub trait Queryable: Sized { } pub trait AddVisitor { - fn add_visitor(&mut self, phase: Phases, visitor: V) + fn add_visitor(&mut self, phase: Phases, visitor: F) where + F: FnOnce() -> V, V: Visitor + 'static; } diff --git a/crates/rome_analyze/src/registry.rs b/crates/rome_analyze/src/registry.rs index be3999eebe5..aa32f55ddaa 100644 --- a/crates/rome_analyze/src/registry.rs +++ b/crates/rome_analyze/src/registry.rs @@ -1,4 +1,8 @@ -use std::{any::TypeId, borrow, collections::BTreeSet}; +use std::{ + any::TypeId, + borrow, + collections::{BTreeMap, BTreeSet}, +}; use crate::{ context::{RuleContext, ServiceBagRuleOptionsWrapper}, @@ -116,7 +120,7 @@ impl RuleRegistry { registry: RuleRegistry { phase_rules: Default::default(), }, - visitors: FxHashMap::default(), + visitors: BTreeMap::default(), services: ServiceBag::default(), diagnostics: Vec::new(), } @@ -143,7 +147,7 @@ pub struct RuleRegistryBuilder<'a, L: Language> { // Rule Registry registry: RuleRegistry, // Analyzer Visitors - visitors: FxHashMap<(Phases, TypeId), Box>>, + visitors: BTreeMap<(Phases, TypeId), Box>>, // Service Bag services: ServiceBag, diagnostics: Vec, @@ -244,12 +248,14 @@ impl RegistryVisitor for RuleRegistryBuilder } } -impl AddVisitor for FxHashMap<(Phases, TypeId), Box>> { - fn add_visitor(&mut self, phase: Phases, visitor: V) +impl AddVisitor for BTreeMap<(Phases, TypeId), Box>> { + fn add_visitor(&mut self, phase: Phases, visitor: F) where + F: FnOnce() -> V, V: Visitor + 'static, { - self.insert((phase, TypeId::of::()), Box::new(visitor)); + self.entry((phase, TypeId::of::())) + .or_insert_with(move || Box::new((visitor)())); } } @@ -257,7 +263,7 @@ type BuilderResult = ( RuleRegistry, ServiceBag, Vec, - FxHashMap<(Phases, TypeId), Box>>, + BTreeMap<(Phases, TypeId), Box>>, ); impl RuleRegistryBuilder<'_, L> { diff --git a/crates/rome_analyze/src/syntax.rs b/crates/rome_analyze/src/syntax.rs index 6ab87bf3739..5b085a0b29e 100644 --- a/crates/rome_analyze/src/syntax.rs +++ b/crates/rome_analyze/src/syntax.rs @@ -23,7 +23,7 @@ where analyzer: &mut impl AddVisitor, _: &::Root, ) { - analyzer.add_visitor(Phases::Syntax, SyntaxVisitor::default()); + analyzer.add_visitor(Phases::Syntax, SyntaxVisitor::default); } fn key() -> QueryKey { diff --git a/crates/rome_js_analyze/src/aria_services.rs b/crates/rome_js_analyze/src/aria_services.rs index 9165dd63e52..bf6616594f3 100644 --- a/crates/rome_js_analyze/src/aria_services.rs +++ b/crates/rome_js_analyze/src/aria_services.rs @@ -62,7 +62,7 @@ where type Services = AriaServices; fn build_visitor(analyzer: &mut impl AddVisitor, _: &AnyJsRoot) { - analyzer.add_visitor(Phases::Syntax, SyntaxVisitor::default()); + analyzer.add_visitor(Phases::Syntax, SyntaxVisitor::default); } fn key() -> QueryKey { diff --git a/crates/rome_js_analyze/src/control_flow.rs b/crates/rome_js_analyze/src/control_flow.rs index 0507bf2d90a..3a110dca970 100644 --- a/crates/rome_js_analyze/src/control_flow.rs +++ b/crates/rome_js_analyze/src/control_flow.rs @@ -32,7 +32,7 @@ impl Queryable for ControlFlowGraph { type Services = (); fn build_visitor(analyzer: &mut impl AddVisitor, _: &AnyJsRoot) { - analyzer.add_visitor(Phases::Syntax, make_visitor()); + analyzer.add_visitor(Phases::Syntax, make_visitor); } fn unwrap_match(_: &ServiceBag, query: &ControlFlowGraph) -> Self::Output { diff --git a/crates/rome_js_analyze/src/semantic_services.rs b/crates/rome_js_analyze/src/semantic_services.rs index 0a67e25ce6e..3d586259fd4 100644 --- a/crates/rome_js_analyze/src/semantic_services.rs +++ b/crates/rome_js_analyze/src/semantic_services.rs @@ -46,8 +46,8 @@ impl Queryable for SemanticServices { type Services = Self; fn build_visitor(analyzer: &mut impl AddVisitor, root: &AnyJsRoot) { - analyzer.add_visitor(Phases::Syntax, SemanticModelBuilderVisitor::new(root)); - analyzer.add_visitor(Phases::Semantic, SemanticModelVisitor); + analyzer.add_visitor(Phases::Syntax, || SemanticModelBuilderVisitor::new(root)); + analyzer.add_visitor(Phases::Semantic, || SemanticModelVisitor); } fn unwrap_match(services: &ServiceBag, _: &SemanticModelEvent) -> Self::Output { @@ -73,8 +73,8 @@ where type Services = SemanticServices; fn build_visitor(analyzer: &mut impl AddVisitor, root: &AnyJsRoot) { - analyzer.add_visitor(Phases::Syntax, SemanticModelBuilderVisitor::new(root)); - analyzer.add_visitor(Phases::Semantic, SyntaxVisitor::default()); + analyzer.add_visitor(Phases::Syntax, || SemanticModelBuilderVisitor::new(root)); + analyzer.add_visitor(Phases::Semantic, SyntaxVisitor::default); } fn key() -> QueryKey { From 76159124cbeee23d4dcf7c74d40ffda82883c387 Mon Sep 17 00:00:00 2001 From: l3ops Date: Fri, 16 Dec 2022 17:46:06 +0100 Subject: [PATCH 4/7] fix CI checks --- Cargo.lock | 1 - crates/rome_analyze/Cargo.toml | 1 - crates/rome_analyze/src/matcher.rs | 5 +++-- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 920b176e5e9..0bf74fe2ae5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1501,7 +1501,6 @@ version = "0.0.0" dependencies = [ "bitflags", "rome_console", - "rome_control_flow", "rome_diagnostics", "rome_rowan", "rustc-hash", diff --git a/crates/rome_analyze/Cargo.toml b/crates/rome_analyze/Cargo.toml index e755e351ba4..ccec199501e 100644 --- a/crates/rome_analyze/Cargo.toml +++ b/crates/rome_analyze/Cargo.toml @@ -10,7 +10,6 @@ license = "MIT" [dependencies] rome_rowan = { path = "../rome_rowan" } -rome_control_flow = { path = "../rome_control_flow" } rome_console = { path = "../rome_console" } rome_diagnostics = { path = "../rome_diagnostics" } bitflags = "1.3.2" diff --git a/crates/rome_analyze/src/matcher.rs b/crates/rome_analyze/src/matcher.rs index eda568ce7e5..42531f18ba8 100644 --- a/crates/rome_analyze/src/matcher.rs +++ b/crates/rome_analyze/src/matcher.rs @@ -6,8 +6,9 @@ use rome_rowan::{Language, TextRange}; use std::{any::Any, cmp::Ordering, collections::BinaryHeap}; /// The [QueryMatcher] trait is responsible of running lint rules on -/// [QueryMatch] instances emitted by the various [Visitor](crate::Visitor) -/// and push signals wrapped in [SignalEntry] to the signal queue +/// [QueryMatch](crate::QueryMatch) instances emitted by the various +/// [Visitor](crate::Visitor) and push signals wrapped in [SignalEntry] +/// to the signal queue pub trait QueryMatcher { /// Execute a single query match fn match_query(&mut self, params: MatchQueryParams); From 82f408da504c69d75490e43b1f88a9b16ff10235 Mon Sep 17 00:00:00 2001 From: l3ops Date: Mon, 19 Dec 2022 09:43:44 +0100 Subject: [PATCH 5/7] move calculatiung the text range of query matches to lazy evaluation --- crates/rome_analyze/src/matcher.rs | 2 +- crates/rome_analyze/src/registry.rs | 3 ++- crates/rome_analyze/src/visitor.rs | 8 ++++++-- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/crates/rome_analyze/src/matcher.rs b/crates/rome_analyze/src/matcher.rs index 42531f18ba8..10fb5f7718f 100644 --- a/crates/rome_analyze/src/matcher.rs +++ b/crates/rome_analyze/src/matcher.rs @@ -19,7 +19,7 @@ pub struct MatchQueryParams<'phase, 'query, L: Language> { pub phase: Phases, pub file_id: FileId, pub root: &'phase L::Root, - pub text_range: TextRange, + pub text_range: fn(&dyn Any) -> TextRange, pub query: Box, pub services: &'phase ServiceBag, pub signal_queue: &'query mut BinaryHeap>, diff --git a/crates/rome_analyze/src/registry.rs b/crates/rome_analyze/src/registry.rs index aa32f55ddaa..04a887606b7 100644 --- a/crates/rome_analyze/src/registry.rs +++ b/crates/rome_analyze/src/registry.rs @@ -433,7 +433,8 @@ impl RegistryRule { }; for result in R::run(&ctx) { - let text_range = R::text_range(&ctx, &result).unwrap_or(params.text_range); + let text_range = R::text_range(&ctx, &result) + .unwrap_or_else(|| (params.text_range)(¶ms.query)); R::suppressed_nodes(&ctx, &result, &mut state.suppressions); diff --git a/crates/rome_analyze/src/visitor.rs b/crates/rome_analyze/src/visitor.rs index bd749fd8d11..da0ffa95282 100644 --- a/crates/rome_analyze/src/visitor.rs +++ b/crates/rome_analyze/src/visitor.rs @@ -1,4 +1,4 @@ -use std::collections::BinaryHeap; +use std::{any::Any, collections::BinaryHeap}; use rome_diagnostics::location::FileId; use rome_rowan::{AstNode, Language, SyntaxNode, TextRange, WalkEvent}; @@ -27,7 +27,7 @@ impl<'phase, 'query, L: Language> VisitorContext<'phase, 'query, L> { phase: self.phase, file_id: self.file_id, root: self.root, - text_range: query.text_range(), + text_range: read_text_range::, query: Box::new(query), services: self.services, signal_queue: self.signal_queue, @@ -36,6 +36,10 @@ impl<'phase, 'query, L: Language> VisitorContext<'phase, 'query, L> { } } +fn read_text_range(query: &dyn Any) -> TextRange { + query.downcast_ref::().unwrap().text_range() +} + /// Mutable context objects provided to the finish hook of visitors pub struct VisitorFinishContext<'a, L: Language> { pub root: &'a LanguageRoot, From 12fc36b9a936b90526632c3ca19c170c965554d1 Mon Sep 17 00:00:00 2001 From: l3ops Date: Mon, 19 Dec 2022 11:55:08 +0100 Subject: [PATCH 6/7] introduce a `Query` type to enforce the correct downcasting of query matches --- crates/rome_analyze/src/matcher.rs | 52 ++++++++++++++++++++++++++--- crates/rome_analyze/src/registry.rs | 7 ++-- crates/rome_analyze/src/visitor.rs | 11 ++---- 3 files changed, 55 insertions(+), 15 deletions(-) diff --git a/crates/rome_analyze/src/matcher.rs b/crates/rome_analyze/src/matcher.rs index 10fb5f7718f..17727ab51ce 100644 --- a/crates/rome_analyze/src/matcher.rs +++ b/crates/rome_analyze/src/matcher.rs @@ -1,9 +1,14 @@ use crate::{ - AnalyzerSignal, Phases, Rule, RuleFilter, RuleGroup, ServiceBag, SuppressionCommentEmitter, + AnalyzerSignal, Phases, QueryMatch, Rule, RuleFilter, RuleGroup, ServiceBag, + SuppressionCommentEmitter, }; use rome_diagnostics::FileId; use rome_rowan::{Language, TextRange}; -use std::{any::Any, cmp::Ordering, collections::BinaryHeap}; +use std::{ + any::{Any, TypeId}, + cmp::Ordering, + collections::BinaryHeap, +}; /// The [QueryMatcher] trait is responsible of running lint rules on /// [QueryMatch](crate::QueryMatch) instances emitted by the various @@ -19,13 +24,52 @@ pub struct MatchQueryParams<'phase, 'query, L: Language> { pub phase: Phases, pub file_id: FileId, pub root: &'phase L::Root, - pub text_range: fn(&dyn Any) -> TextRange, - pub query: Box, + pub query: Query, pub services: &'phase ServiceBag, pub signal_queue: &'query mut BinaryHeap>, pub apply_suppression_comment: SuppressionCommentEmitter, } +/// Wrapper type for a [QueryMatch] +/// +/// This type is functionally equivalent to `Box`, it +/// emulates dynamic dispatch for both traits and allows downcasting to a +/// reference or owned type. +pub struct Query { + data: Box, + read_text_range: fn(&dyn Any) -> TextRange, +} + +impl Query { + /// Construct a new [Query] instance from a [QueryMatch] + pub fn new(data: T) -> Self { + Self { + data: Box::new(data), + read_text_range: |query| query.downcast_ref::().unwrap().text_range(), + } + } + + /// Attempt to downcast the query to an owned type. + pub fn downcast(self) -> Option { + Some(*self.data.downcast::().ok()?) + } + + /// Attempt to downcast the query to a reference type. + pub fn downcast_ref(&self) -> Option<&T> { + self.data.downcast_ref::() + } + + /// Returns the [TypeId] of this query, equivalent to calling [Any::type_id]. + pub(crate) fn type_id(&self) -> TypeId { + self.data.as_ref().type_id() + } + + /// Returns the [TextRange] of this query, equivalent to calling [QueryMatch::text_range]. + pub(crate) fn text_range(&self) -> TextRange { + (self.read_text_range)(self.data.as_ref()) + } +} + /// Opaque identifier for a group of rule #[derive(Copy, Clone, Debug, PartialEq, Eq)] pub struct GroupKey { diff --git a/crates/rome_analyze/src/registry.rs b/crates/rome_analyze/src/registry.rs index 04a887606b7..adba677bad5 100644 --- a/crates/rome_analyze/src/registry.rs +++ b/crates/rome_analyze/src/registry.rs @@ -281,7 +281,8 @@ impl QueryMatcher for RuleRegistry { fn match_query(&mut self, mut params: MatchQueryParams) { let phase = &mut self.phase_rules[params.phase as usize]; - let Some(rules) = phase.type_rules.get(¶ms.query.type_id()) else { return }; + let query_type = params.query.type_id(); + let Some(rules) = phase.type_rules.get(&query_type) else { return }; let rules = match rules { TypeRules::SyntaxRules { rules } => { @@ -433,8 +434,8 @@ impl RegistryRule { }; for result in R::run(&ctx) { - let text_range = R::text_range(&ctx, &result) - .unwrap_or_else(|| (params.text_range)(¶ms.query)); + let text_range = + R::text_range(&ctx, &result).unwrap_or_else(|| params.query.text_range()); R::suppressed_nodes(&ctx, &result, &mut state.suppressions); diff --git a/crates/rome_analyze/src/visitor.rs b/crates/rome_analyze/src/visitor.rs index da0ffa95282..bb6b470a410 100644 --- a/crates/rome_analyze/src/visitor.rs +++ b/crates/rome_analyze/src/visitor.rs @@ -1,10 +1,10 @@ -use std::{any::Any, collections::BinaryHeap}; +use std::collections::BinaryHeap; use rome_diagnostics::location::FileId; use rome_rowan::{AstNode, Language, SyntaxNode, TextRange, WalkEvent}; use crate::{ - matcher::MatchQueryParams, + matcher::{MatchQueryParams, Query}, registry::{NodeLanguage, Phases}, LanguageRoot, QueryMatch, QueryMatcher, ServiceBag, SignalEntry, SuppressionCommentEmitter, }; @@ -27,8 +27,7 @@ impl<'phase, 'query, L: Language> VisitorContext<'phase, 'query, L> { phase: self.phase, file_id: self.file_id, root: self.root, - text_range: read_text_range::, - query: Box::new(query), + query: Query::new(query), services: self.services, signal_queue: self.signal_queue, apply_suppression_comment: self.apply_suppression_comment, @@ -36,10 +35,6 @@ impl<'phase, 'query, L: Language> VisitorContext<'phase, 'query, L> { } } -fn read_text_range(query: &dyn Any) -> TextRange { - query.downcast_ref::().unwrap().text_range() -} - /// Mutable context objects provided to the finish hook of visitors pub struct VisitorFinishContext<'a, L: Language> { pub root: &'a LanguageRoot, From d3bc27a6f50271132c9cb9566e09cb0c5231d4ac Mon Sep 17 00:00:00 2001 From: l3ops Date: Tue, 20 Dec 2022 18:16:52 +0100 Subject: [PATCH 7/7] add documentation --- crates/rome_analyze/CONTRIBUTING.md | 92 +++++++++++++++++++++++++++++ crates/rome_analyze/src/lib.rs | 1 + crates/rome_analyze/src/query.rs | 11 +++- crates/rome_analyze/src/registry.rs | 5 +- 4 files changed, 106 insertions(+), 3 deletions(-) diff --git a/crates/rome_analyze/CONTRIBUTING.md b/crates/rome_analyze/CONTRIBUTING.md index f96cd0e750d..58ecea18e04 100644 --- a/crates/rome_analyze/CONTRIBUTING.md +++ b/crates/rome_analyze/CONTRIBUTING.md @@ -387,6 +387,98 @@ If this the rule can retrieve its option with let options = ctx.options(); ``` +#### Custom Visitors + +Some lint rules may need to deeply inspect the child nodes of a query match +before deciding on whether they should emit a signal or not. These rules can be +inefficient to implement using the query system, as they will lead to redundant +traversal passes being executed over the same syntax tree. To make this more +efficient, you can implement a custom `Queryable` type and and associated +`Visitor` to emit it as part of the analyzer's main traversal pass. As an +example, here's how this could be done to implement the `useYield` rule: + +```rust,ignore +// First, create a visitor struct that holds a stack of function syntax nodes and booleans +#[derive(Default)] +struct MissingYieldVisitor { + stack: Vec<(AnyFunctionLike, bool)>, +} + +// Implement the `Visitor` trait for this struct +impl Visitor for MissingYieldVisitor { + fn visit( + &mut self, + event: &WalkEvent>, + ctx: VisitorContext, + ) { + match event { + WalkEvent::Enter(node) => { + // When the visitor enters a function node, push a new entry on the stack + if let Some(node) = AnyFunctionLike::cast_ref(node) { + self.stack.push((node, false)); + } + + if let Some((_, has_yield)) = self.stack.last_mut() { + // When the visitor enters a `yield` expression, set the + // `has_yield` flag for the top entry on the stack to `true` + if JsYieldExpression::can_cast(node.kind()) { + *has_yield = true; + } + } + } + WalkEvent::Leave(node) => { + // When the visitor exits a function, if it matches the node of the top-most + // entry of the stack and the `has_yield` flag is `false`, emit a query match + if let Some(exit_node) = AnyFunctionLike::cast_ref(node) { + if let Some((enter_node, has_yield)) = self.stack.pop() { + assert_eq!(enter_node, exit_node); + if !has_yield { + ctx.match_query(MissingYield(enter_node)); + } + } + } + } + } + } +} + +// Declare a query match struct type containing a JavaScript function node +struct MissingYield(AnyFunctionLike); + +// Implement the `Queryable` trait for this type +impl Queryable for MissingYield { + // `Input` is the type that `ctx.match_query()` is called with in the visitor + type Input = Self; + // `Output` if the type that `ctx.query()` will return in the rule + type Output = AnyFunctionLike; + + fn build_visitor( + analyzer: &mut impl AddVisitor, + _: &::Root, + ) { + // Register our custom visitor to run in the `Syntax` phase + analyzer.add_visitor(Phases::Syntax, MissingYieldVisitor::default()); + } + + // Extract the output object from the input type + fn unwrap_match(services: &ServiceBag, query: &Self::Input) -> Self::Output { + query.0.clone() + } +} + +impl Rule for UseYield { + // Declare the custom `MissingYield` queryable as the rule's query + type Query = MissingYield; + + fn run(ctx: &RuleContext) -> Self::Signals { + // Read the function's root node from the queryable output + let query: &AnyFunctionLike = ctx.query(); + + // ... + } +} +``` + # Using Just It is also possible to do all the steps above using our `Just` automation. For example, we can create diff --git a/crates/rome_analyze/src/lib.rs b/crates/rome_analyze/src/lib.rs index b00daaf1355..255e8cccafd 100644 --- a/crates/rome_analyze/src/lib.rs +++ b/crates/rome_analyze/src/lib.rs @@ -107,6 +107,7 @@ where } } + /// Registers a [Visitor] to be executed as part of a given `phase` of the analyzer run pub fn add_visitor( &mut self, phase: Phases, diff --git a/crates/rome_analyze/src/query.rs b/crates/rome_analyze/src/query.rs index 7fa08e5b648..7810208418a 100644 --- a/crates/rome_analyze/src/query.rs +++ b/crates/rome_analyze/src/query.rs @@ -4,7 +4,7 @@ use rome_rowan::{Language, SyntaxKindSet, TextRange}; use crate::{registry::Phase, services::FromServices, Phases, ServiceBag, Visitor}; -/// Trait implemented for all types, for example lint rules can query them to emit diagnostics or code actions. +/// Trait implemented for types that lint rules can query in order to emit diagnostics or code actions. pub trait Queryable: Sized { type Input: QueryMatch; type Output; @@ -12,11 +12,18 @@ pub trait Queryable: Sized { type Language: Language; type Services: FromServices + Phase; + /// Registers one or more [Visitor] that will emit `Self::Input` query + /// matches during the analyzer run fn build_visitor( analyzer: &mut impl AddVisitor, root: &::Root, ); + /// Returns the type of query matches this [Queryable] expects as inputs + /// + /// Unless your custom queryable needs to match on a specific + /// [SyntaxKindSet], you should not override the default implementation of + /// this method fn key() -> QueryKey { QueryKey::TypeId(TypeId::of::()) } @@ -29,7 +36,9 @@ pub trait Queryable: Sized { fn unwrap_match(services: &ServiceBag, query: &Self::Input) -> Self::Output; } +/// This trait is implemented on all types that supports the registration of [Visitor] pub trait AddVisitor { + /// Registers a [Visitor] for a given `phase` fn add_visitor(&mut self, phase: Phases, visitor: F) where F: FnOnce() -> V, diff --git a/crates/rome_analyze/src/registry.rs b/crates/rome_analyze/src/registry.rs index adba677bad5..64b86e47a29 100644 --- a/crates/rome_analyze/src/registry.rs +++ b/crates/rome_analyze/src/registry.rs @@ -130,6 +130,7 @@ impl RuleRegistry { /// Holds a collection of rules for each phase. #[derive(Default)] struct PhaseRules { + /// Maps the [TypeId] of known query matches types to the corresponding list of rules type_rules: FxHashMap>, /// Holds a list of states for all the rules in this phase rule_states: Vec>, @@ -188,7 +189,7 @@ impl RegistryVisitor for RuleRegistryBuilder .type_rules .entry(TypeId::of::>()) .or_insert_with(|| TypeRules::SyntaxRules { rules: Vec::new() }) - else { unreachable!() }; + else { unreachable!("the SyntaxNode type has already been registered as a TypeRules instead of a SyntaxRules, this is generally caused by an implementation of `Queryable::key` returning a `QueryKey::TypeId` with the type ID of `SyntaxNode`") }; // Iterate on all the SyntaxKind variants this node can match for kind in key.iter() { @@ -214,7 +215,7 @@ impl RegistryVisitor for RuleRegistryBuilder .type_rules .entry(key) .or_insert_with(|| TypeRules::TypeRules { rules: Vec::new() }) - else { unreachable!() }; + else { unreachable!("the query type has already been registered as a SyntaxRules instead of a TypeRules, this is generally ca used by an implementation of `Queryable::key` returning a `QueryKey::TypeId` with the type ID of `SyntaxNode`") }; rules.push(rule); }