refactor(rome_analyze): refactor how Visitors, Queryables and QueryMatches are related #4063

leops · 2022-12-16T15:00:24Z

Summary

This PR modifies how various types related to the Queryable trait are injected into the analyzer infrastructure. This includes:

Allowing Queryable types to dynamically register the Visitor instances that emits them to the analyzer
Replacing the QueryMatch enum with TypeId and Box<dyn Any> to allow any arbitrary type to be emitted as a visitor event

The main goal of this refactor is to allow individual rules to declare custom syntax tree visitors to emit the event they rely upon as an optimization opportunity instead of having to traverse the syntax tree in their run method. For instance the useYield rule could be implemented like this:

Code

impl Rule for UseYield {
    type Query = MissingYield;
}

struct MissingYield(AnyFunctionLike);

impl Queryable for MissingYield {
    type Input = Self;
    type Output = AnyFunctionLike;
    
    fn build_visitor(
        analyzer: &mut impl AddVisitor<Self::Language>,
        _: &<Self::Language as Language>::Root,
    ) {
        analyzer.add_visitor(Phases::Syntax, MissingYieldVisitor::default());
    }

    fn unwrap_match(services: &ServiceBag, query: &Self::Input) -> Self::Output {
        query.0.clone()
    }
}

#[derive(Default)]
struct MissingYieldVisitor {
    stack: Vec<(AnyFunctionLike, bool)>,
}

impl Visitor for MissingYieldVisitor {
    fn visit(
        &mut self,
        event: &WalkEvent<SyntaxNode<Self::Language>>,
        ctx: VisitorContext<Self::Language>,
    ) {
        match event {
            WalkEvent::Enter(node) => {
                if let Some(node) = AnyFunctionLike::cast_ref(node) {
                    self.stack.push((node, false));
                }
                
                if let Some((_, has_yield)) = self.stack.last_mut() {
                    if JsYieldExpression::can_cast(node.kind()) {
                        *has_yield = true;
                    }
                }
            }
            WalkEvent::Leave(node) => {
                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));
                        }
                    }
                }
            }
        }
    }
}

Test Plan

This is an infrastructure-only change, the code should continue to build and existing tests should continue to pass correctly.

Documentation

The PR requires documentation
I will create a new PR to update the documentation

netlify · 2022-12-16T15:01:51Z

✅ Deploy Preview for docs-rometools canceled.

Name	Link
🔨 Latest commit	`d3bc27a`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-rometools/deploys/63a1ee0fef5dd2000853c181

xunilrj · 2022-12-16T16:05:57Z

In the yield example, the visitor will be called for all nodes?
Why not use something like the dispatch we have for rules that only answer Ast<...> queries that runs on the first pass?
We can give this new trait &mut to the service bag.

struct MissingYields(Vec<JsFunctionDeclaration>);

impl RuleService for MissingYields {
    type Query = Ast<JsFunctionDeclaration | JsYieldExpression>; // just an example

    fn visit(...) {
         match event {
            WalkEvent::Enter(node) => {
              ...
            }
            WalkEvent::Leave(node) => {
                ...
            }
        }
    }

    fn finish(bag: &mut ServiceBag) {
         bag.insert(MissingYields(...));
    .}
}

The main difference is that we introduce a hook to services. We can have later: FindReactCallsService, FunctionsWithJSX etc...
We can expand the

type Query = SemanticServices

To actually support requesting any services:

type Query = Services<SemanticModel, FindReactCallsService>

What do you think?

leops · 2022-12-16T16:43:57Z

For now this is mostly implemented as a simple extension to the existing Visitor API that allows individual rules to run custom specialized visitors on the syntax tree in parallel to existing visitors like the semantic model builder. Being able to specify query / filters on visitors seems interesting but would probably require adding another layer of indirection between the syntax tree preorder iterator and the visitors.

MichaReiser · 2022-12-17T12:59:30Z

!bench_analyzer

github-actions · 2022-12-17T13:13:30Z

Analyzer Benchmark Results

group                     main                                   pr
-----                     ----                                   --
analyzer/css.js           1.00      2.6±0.01ms     4.5 MB/sec    1.30      3.3±0.03ms     3.5 MB/sec
analyzer/index.js         1.00      6.4±0.01ms     5.1 MB/sec    1.32      8.5±0.01ms     3.9 MB/sec
analyzer/lint.ts          1.00      2.8±0.03ms    14.7 MB/sec    1.53      4.3±0.00ms     9.6 MB/sec
analyzer/parser.ts        1.00      7.5±0.05ms     6.5 MB/sec    1.44     10.8±0.02ms     4.5 MB/sec
analyzer/router.ts        1.00      2.7±0.00ms    11.6 MB/sec    1.43      3.8±0.01ms     8.1 MB/sec
analyzer/statement.ts     1.00      7.3±0.01ms     4.9 MB/sec    1.43     10.4±0.02ms     3.4 MB/sec
analyzer/typescript.ts    1.00     10.6±0.02ms     5.1 MB/sec    1.43     15.2±0.17ms     3.6 MB/sec

leops · 2022-12-19T08:46:45Z

!bench_analyzer

ematipico · 2022-12-19T09:41:04Z

@leops it seems that the action didn't finish

…matches

leops · 2022-12-19T13:05:34Z

!bench_analyzer

github-actions · 2022-12-19T13:19:37Z

Analyzer Benchmark Results

group                     main                                   pr
-----                     ----                                   --
analyzer/css.js           1.00      2.5±0.01ms     4.6 MB/sec    1.02      2.6±0.01ms     4.5 MB/sec
analyzer/index.js         1.00      6.5±0.01ms     5.1 MB/sec    1.02      6.6±0.07ms     5.0 MB/sec
analyzer/lint.ts          1.00      2.8±0.00ms    14.8 MB/sec    1.07      3.0±0.01ms    13.9 MB/sec
analyzer/parser.ts        1.00      7.4±0.02ms     6.6 MB/sec    1.05      7.8±0.04ms     6.3 MB/sec
analyzer/router.ts        1.00      2.6±0.01ms    11.6 MB/sec    1.04      2.8±0.00ms    11.1 MB/sec
analyzer/statement.ts     1.00      7.2±0.01ms     4.9 MB/sec    1.05      7.5±0.01ms     4.7 MB/sec
analyzer/typescript.ts    1.00     10.6±0.12ms     5.1 MB/sec    1.04     11.1±0.04ms     4.9 MB/sec

MichaReiser · 2022-12-20T12:36:15Z

crates/rome_analyze/src/registry.rs

+        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 } => {
+                let node = params.query.downcast_ref::<SyntaxNode<L>>().unwrap();


Do you see a way that would avoid the need for the runtime assertion that rules and the query type (downcast) match?

ematipico

While this seems to be an infrastructure change only, this change unlocks new features that are not documented in this PR.

Other than showing an example in this PR, I think we should update the contribution guidelines that show how to create a rule with its own visitor system.

ematipico · 2022-12-20T11:52:09Z

crates/rome_analyze/src/lib.rs

-            .entry(phase)
-            .or_default()
-            .push(Box::new(visitor));
+    pub fn add_visitor(


I think it's time to document this public API. We started using it more and more... I suppose some description is due.

crates/rome_analyze/src/matcher.rs

ematipico · 2022-12-20T11:54:21Z

crates/rome_analyze/src/query.rs

+    fn build_visitor(
+        analyzer: &mut impl AddVisitor<Self::Language>,
+        root: &<Self::Language as Language>::Root,
+    );
+
+    fn key() -> QueryKey<Self::Language> {
+        QueryKey::TypeId(TypeId::of::<Self::Input>())
+    }


Documentation?

ematipico · 2022-12-20T11:54:42Z

crates/rome_analyze/src/query.rs

-    Syntax(SyntaxNode<L>),
-    SemanticModel(TextRange),
-    ControlFlowGraph(ControlFlowGraph<L>, TextRange),
+pub trait AddVisitor<L: Language> {


Documentation?

ematipico · 2022-12-20T11:57:18Z

crates/rome_analyze/src/registry.rs

-    ast_rules: Vec<SyntaxKindRules<L>>,
-    control_flow: Vec<RegistryRule<L>>,
-    semantic_model: Vec<RegistryRule<L>>,
+    type_rules: FxHashMap<TypeId, TypeRules<L>>,


Documentation?

ematipico · 2022-12-20T11:58:41Z

crates/rome_analyze/src/registry.rs

+                    .type_rules
+                    .entry(key)
+                    .or_insert_with(|| TypeRules::TypeRules { rules: Vec::new() })
+                    else { unreachable!() };


Can we insert a small description inside unreachable!() in case it is actually reached? The message should hint where to fix the issue.

ematipico · 2022-12-20T12:56:58Z

crates/rome_analyze/src/query.rs

-};
+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 {


I suppose this trait should have more documentation now

leops added 2 commits December 13, 2022 15:00

refactor(rome_analyze): allow queryables to register their own visitor

3d613a8

replace the QueryMatch enum with Box<dyn Any>

fa28a62

leops requested review from ematipico and xunilrj as code owners December 16, 2022 15:00

store visitors in a BTreeMap to register them in a stable order

e5e6a0f

fix CI checks

7615912

leops requested a review from a team as a code owner December 16, 2022 16:46

move calculatiung the text range of query matches to lazy evaluation

82f408d

introduce a Query type to enforce the correct downcasting of query …

12fc36b

…matches

MichaReiser approved these changes Dec 20, 2022

View reviewed changes

ematipico suggested changes Dec 20, 2022

View reviewed changes

add documentation

d3bc27a

ematipico approved these changes Dec 20, 2022

View reviewed changes

leops merged commit c502c2c into main Dec 21, 2022

leops deleted the refactor/analyzer-visitor branch December 21, 2022 08:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(rome_analyze): refactor how Visitors, Queryables and QueryMatches are related #4063

refactor(rome_analyze): refactor how Visitors, Queryables and QueryMatches are related #4063

leops commented Dec 16, 2022

netlify bot commented Dec 16, 2022 •

edited

Loading

xunilrj commented Dec 16, 2022

leops commented Dec 16, 2022

MichaReiser commented Dec 17, 2022

github-actions bot commented Dec 17, 2022

leops commented Dec 19, 2022

ematipico commented Dec 19, 2022

leops commented Dec 19, 2022

github-actions bot commented Dec 19, 2022

MichaReiser Dec 20, 2022

ematipico left a comment

ematipico Dec 20, 2022

ematipico Dec 20, 2022

ematipico Dec 20, 2022

ematipico Dec 20, 2022

ematipico Dec 20, 2022

ematipico Dec 20, 2022

refactor(rome_analyze): refactor how Visitors, Queryables and QueryMatches are related #4063

refactor(rome_analyze): refactor how Visitors, Queryables and QueryMatches are related #4063

Conversation

leops commented Dec 16, 2022

Summary

Test Plan

Documentation

netlify bot commented Dec 16, 2022 • edited Loading

✅ Deploy Preview for docs-rometools canceled.

xunilrj commented Dec 16, 2022

leops commented Dec 16, 2022

MichaReiser commented Dec 17, 2022

github-actions bot commented Dec 17, 2022

Analyzer Benchmark Results

leops commented Dec 19, 2022

ematipico commented Dec 19, 2022

leops commented Dec 19, 2022

github-actions bot commented Dec 19, 2022

Analyzer Benchmark Results

MichaReiser Dec 20, 2022

Choose a reason for hiding this comment

ematipico left a comment

Choose a reason for hiding this comment

ematipico Dec 20, 2022

Choose a reason for hiding this comment

ematipico Dec 20, 2022

Choose a reason for hiding this comment

ematipico Dec 20, 2022

Choose a reason for hiding this comment

ematipico Dec 20, 2022

Choose a reason for hiding this comment

ematipico Dec 20, 2022

Choose a reason for hiding this comment

ematipico Dec 20, 2022

Choose a reason for hiding this comment

netlify bot commented Dec 16, 2022 •

edited

Loading