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

refactor(rome_analyze): move the JS-specific code to rome_js_analyze #2700

Merged
merged 3 commits into from
Jun 14, 2022

Conversation

leops
Copy link
Contributor

@leops leops commented Jun 13, 2022

Summary

This PR splits out the analyzer codebase between the generic analysis infrastructure in rome_analyze, and the implementation of JS-specific rules in rome_js_analyze

Test Plan

This change is mostly renaming files or moving items between crates with little to no actual code changes, so the code relying on the analyzer should continue building and passing the same tests as before.

Base automatically changed from feature/analyzer-diagnostic-builder to main June 13, 2022 12:23
@cloudflare-pages
Copy link

cloudflare-pages bot commented Jun 13, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: f91f5de
Status: ✅  Deploy successful!
Preview URL: https://67c5aab9.tools-8rn.pages.dev
Branch Preview URL: https://refactor-analyzer-js.tools-8rn.pages.dev

View logs

@leops leops temporarily deployed to aws June 13, 2022 12:24 Inactive
@leops leops marked this pull request as ready for review June 13, 2022 12:24
@github-actions
Copy link

github-actions bot commented Jun 13, 2022

@github-actions
Copy link

github-actions bot commented Jun 13, 2022

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 350 350 0
Failed 5596 5596 0
Panics 0 0 0
Coverage 5.89% 5.89% 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%

@ematipico
Copy link
Contributor

How would this infrastructure work when/if we will have rules that touch different languages at the same time? E.g. JSX and HTML

@leops
Copy link
Contributor Author

leops commented Jun 13, 2022

How would this infrastructure work when/if we will have rules that touch different languages at the same time? E.g. JSX and HTML

At the root level analysis is always performed on a syntax tree of a known language, using a registry of rules that support analyzing this specific language. In order to write rules that work in multiple languages, they would need to be written once in a way that keeps them "injectable" into instances of the registry specialized for different languages. Currently rules aren't directly tied to a specific language at the type level, this relation only exists indirectly through the Language associated type of the Query AST node, and support cross-language analysis is basically about erasing this relation.

One possibility I see is either making it possible to declare "higher-level unions" across languages, or directly allowing unions to implement AstNode for multiple language types. This would completely sever the rule-language relationship and make it possible to run any rule on any language (the query would simply never match when running CSS-only rules on a JS-only syntax tree for instance)

Another possibility is turning the trait Rule into Rule<L: Language>, allowing rules to be "implemented once" but on a generic language. Here the query type would be more complex to express (as it would need to rely on additional traits and associated types) but this would statically ensure a rule can never be used on a syntax tree using a language it doesn't support

@leops leops temporarily deployed to aws June 13, 2022 14:12 Inactive
@leops leops temporarily deployed to aws June 13, 2022 14:16 Inactive
@MichaReiser MichaReiser requested a review from xunilrj June 14, 2022 06:47
crates/rome_analyze/src/lib.rs Outdated Show resolved Hide resolved
crates/rome_js_analyze/src/registry.rs Show resolved Hide resolved
@leops leops temporarily deployed to aws June 14, 2022 07:30 Inactive
@leops leops temporarily deployed to aws June 14, 2022 07:52 Inactive
crates/rome_analyze/src/lib.rs Show resolved Hide resolved
crates/rome_analyze/src/lib.rs Show resolved Hide resolved
@leops leops temporarily deployed to aws June 14, 2022 14:59 Inactive
@leops leops merged commit 579fc97 into main Jun 14, 2022
@leops leops deleted the refactor/analyzer-js branch June 14, 2022 15:04
IWANABETHATGUY pushed a commit to IWANABETHATGUY/tools that referenced this pull request Jun 15, 2022
…ome#2700)

* refactor(rome_analyze): move the JS-specific code to rome_js_analyze

* add a constructor function to the analyzer

* add documentation
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants