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

refactor(rome_analyze): add the visitor and queryable trait #2778

Merged
merged 5 commits into from
Jun 29, 2022

Conversation

leops
Copy link
Contributor

@leops leops commented Jun 24, 2022

Summary

This PR refactors the analysis infrastructure and introduces the concepts of Visitor and Queryable. The general goal of these changes is to let the analyzer support not just the linting use case, but also provide the infrastructure for all kind of analysis we may need to perform on top of the syntax tree to support formatting, compiling and various IDE features.

Visitor is the main new building block for the analyzer: in fact the analyzer is nothing more than an executor running a collection of visitors over a given syntax tree. All visitors are executed simultaneously and receive WalkEvent as the syntax tree is traversed, and may use the opportunity to mutate their own state, as well as mutably access the shared VisitorContext object.

This context holds a reference to the RuleRegistry through which the visitors may dispatch QueryMatch objects to trigger the rules associated with a certain Queryable type: for instance rome_analyze defines a SyntaxVisitor that dispatches an Ast<T> query match for each node it enters, which is the query type used by all currently existing lint rules as they all currently work on AST nodes.

The PR also introduces a new merge_node_visitors macro that creates a single struct implementing Visitor over a collection of type implementing the NodeVisitor helper trait. Unlike the global Visitor, node visitors are transient: they get instantiated when the traversal enters the corresponding node and destroyed when the node is exited. They are intended as a building blocks for creating and managing the state of complex visitors by allowing the implementation to be split over multiple smaller components.

Test Plan

TODO: Despite being more open-ended the components of this infrastructure are still tied together quite tightly, and this doesn't really allow writing tests for individual components. It should be possible to improve this by at least decoupling the emission of query matches by the visitors from the execution of rules matching on these queries

@cloudflare-pages
Copy link

cloudflare-pages bot commented Jun 24, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: fc4f882
Status: ✅  Deploy successful!
Preview URL: https://dbbdb1b0.tools-8rn.pages.dev
Branch Preview URL: https://refactor-analyze-visitor.tools-8rn.pages.dev

View logs

Base automatically changed from feature/astnode-static-kind to main June 28, 2022 07:03
@leops leops temporarily deployed to aws June 28, 2022 08:16 Inactive
@leops leops marked this pull request as ready for review June 28, 2022 08:17
@github-actions
Copy link

github-actions bot commented Jun 28, 2022

@github-actions
Copy link

Parser conformance results on ubuntu-latest

js/262

Test result main count This PR count Difference
Total 45878 45878 0
Passed 44938 44938 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 388 388 0
Failed 5558 5558 0
Panics 0 0 0
Coverage 6.53% 6.53% 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 12393 12393 0
Failed 3864 3864 0
Panics 0 0 0
Coverage 76.23% 76.23% 0.00%

crates/rome_analyze/src/query.rs Outdated Show resolved Hide resolved
crates/rome_analyze/src/query.rs Show resolved Hide resolved
crates/rome_analyze/src/query.rs Outdated Show resolved Hide resolved
crates/rome_analyze/src/query.rs Outdated Show resolved Hide resolved
crates/rome_analyze/src/query.rs Outdated Show resolved Hide resolved
crates/rome_analyze/src/syntax.rs Show resolved Hide resolved
crates/rome_analyze/src/visitor.rs Show resolved Hide resolved
crates/rome_analyze/src/visitor.rs Show resolved Hide resolved
crates/rome_analyze/src/visitor.rs Outdated Show resolved Hide resolved

/// Quary type usable bi lint rules to match on specific [AstNode] types
#[derive(Clone)]
pub struct Ast<N>(pub N);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is basically just a "newtype" struct that wraps an AstNode type N to implement Queryable on it, this is a common pattern in Rust to get around the orphan rule

@leops leops temporarily deployed to aws June 28, 2022 12:58 Inactive
@leops leops temporarily deployed to aws June 28, 2022 13:37 Inactive
@leops leops temporarily deployed to aws June 29, 2022 09:16 Inactive
@leops leops temporarily deployed to aws June 29, 2022 09:24 Inactive
@leops leops merged commit 7d74a4d into main Jun 29, 2022
@leops leops deleted the refactor/analyze-visitor branch June 29, 2022 13:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants