feat(rome_analyze): add a fix argument to `rome check` command CLI and LSP #2615

leops · 2022-05-25T15:38:49Z

Summary

This PR adds a new auto-fix workflow to the analysis toolchain, with the following changes:

The callback injected into the analyze entrypoint function for the analyzer now returns an std::ops::ControlFlow enum. This lets the callback interrupt the analysis process by returning ControlFlow::Break(value), with the value being returned by the analyze function as an Option
The Workspace now has a fix_file method that applies all safe fixes to a given file and returns the resulting code. This is implemented by repeatedly calling into analyze and breaking for each emitted action signal with Applicability::Always to mutate the syntax tree with the code action, then restart the analysis until no more code actions are emitted
The check command of the CLI now has an --apply argument reusing the same traversal infrastructure, and calling fix_file on each processed file then writing the resulting code back to the disk
The Language Server can now emit a "Fix all auto-fixable issues" code action when requested by the language client (for instance when using the "Fix all" command in VS Code), the TextEdit backing this action is also generated from the result of fix_file
The code action emitted by the useSingleVarDeclarator rule has been marked as "always applicable", and the implementation of the fix has been changed to support variable statements in JsModuleItemList as well as JsStatementList

Test Plan

New tests have been added to the CLI test suite to check the rome check --apply command

github-actions · 2022-05-25T15:43:05Z

Playground for commit fdb8af7

cloudflare-pages · 2022-05-25T15:51:49Z

Deploying with Cloudflare Pages

Latest commit:	`3bc3389`
Status:	✅ Deploy successful!
Preview URL:	https://499532aa.tools-8rn.pages.dev
Branch Preview URL:	https://feature-auto-fix.tools-8rn.pages.dev

View logs

xunilrj · 2022-05-26T08:27:50Z

crates/rome_analyze/src/analyzers/use_single_var_declarator.rs

+            root: JsAnyRoot::unwrap_cast(
+                root.into_syntax()
+                    .replace_child(prev_parent.into(), next_parent.into())?,
+            ),
        })
    }


Would be possible to write some unit tests for each rule? Sometimes is not clear what the rule is flagging or doing.

There are snapshots tests for each rules under crates/rome_analyze/tests/specs. I went with snapshots since it captures the diagnostic or code action diff in a readable form close to what the end user will actually see on the CLI, rather than running individual asserts on things like the syntax tree resulting from a refactor since I found it to be easily unclear.

xunilrj · 2022-05-26T08:30:29Z

crates/rome_analyze/src/lib.rs

@@ -17,7 +19,7 @@ pub use crate::signals::{AnalyzerAction, AnalyzerSignal};

 /// Allows filtering the list of rules that will be executed in a run of the analyzer,
 /// and at what source code range signals (diagnostics or actions) may be raised
-#[derive(Default)]
+#[derive(Default, Clone, Copy)]


Why does it need to be Copy? It means we cannot filter using a string, like "filter by name".

It is currently possible to filter rules by name (that's used for the snapshot tests to enable only the rule being tested for instance), but since the name of each rules are statically known they can be represented with &'static str string slices that implement Copy

crates/rome_analyze/src/lib.rs

ematipico

Sorry to interject your PR but I am a little confused about where we are headed with the commands.

I would like to understand the requirements first (is there been a discussion?). Could you please create an issue or a discussion first?

The reason why I am concerned is because I don't have any idea of how the final commands will look like. I don't know if this will be final design, or temporary design because of how things are (with a plan to reach the final design).

In Rome classic we had:

rome ci, in line with the rewrite
rome format, in line with the rewrite
rome check, not in line with the rewrite. The former command applied linting and formatting. --apply was the argument to have safe fixed and formatting applied. Like --write in the current rome format --write

We now adding a rome fix command, which is "different" from the most popular tools I am aware about. For example, eslint uses the --fix argument, not a command. Classic rome had the --apply argument. Should we follow the same trend?

ematipico · 2022-05-26T14:54:21Z

crates/rome_analyze/src/lib.rs

+/// This should eventually get replaced with the `!` type when it gets stabilized
+pub enum Never {}
+
+/// Type alias of [ops::ControlFlow] with the `B` generic type defaulting to [Never]


Why do we default to Never?. It might make sense to explain it in the doc

Today the return type of loops that doesn't produce anything is (). Which is different from loops that never returns. Doesn't make sense to do the same here and use () as the default case?

By default the analysis loop never breaks, so it behaves mostly like let b = loop {}; and has a "break type" of ! (the ! type isn't stable yet so I'm using an empty enum instead but they're identical for this purpose)
In practice it's not really a loop but a for because it's iterating on all nodes in the syntax tree, so when it reaches the end of the iterator the loop will exit but without producing a value of type B: for this reason the analyze function returns an Option<B> that's set to Some(B) if the callback did break, and None if the analysis reached the end of the file.
Most consumers of the analyzer will want to analyze the entire file at once and never break, so using Never as the type of B in this case lets the compiler know the ControlFlow::Break branch will never be taken and can be optimized out, as well as completely remove the return Some case (Option<Never> has a size of 0 and can be elided, while Option<()> has a size of 1 as it still need to store a discriminant)

ematipico · 2022-05-26T14:55:48Z

crates/rome_analyze/src/lib.rs

+    file_id: FileId,
+    root: &JsAnyRoot,
+    filter: AnalysisFilter,
+    mut callback: impl FnMut(&dyn AnalyzerSignal) -> ControlFlow<B>,


Should we move the first three arguments into a struct? (I presume it's not possible to have the callback inside the struct)

Most of the arguments for the analysis have already been pushed into the AnalysisFilter struct, and I don't expect to add many more arguments to the analyze function at this stage except an eventual &mut self to let the analyzer keep some state / cache between runs, along some global-level settings

It may make sense to define a type alias for the Callback type and copy some of the explanations you gave in the other comments into its documentation so that future me will know what I need to return in the control flow.

ematipico · 2022-05-26T15:00:28Z

crates/rome_cli/src/commands/fix.rs

+    CliSession, Termination,
+};
+
+/// Handler for the "fix" command of the Rome CLI


Why the fix is a command and not an argument of check command?

In rome classic we called it --apply and it was an argument of the command rome check. That's because check wasn't just about "fixing" the code, but also apply formatting.

Having two different commands was mostly because the check and fix commands had mostly different backing implementations, but I could as well change it to have the fix behavior be implemented by the check command with an --apply flag instead to be consistent with how the CLI worked in Rome JS

MichaReiser · 2022-06-03T15:45:26Z

crates/rome_analyze/src/lib.rs

+    file_id: FileId,
+    root: &JsAnyRoot,
+    filter: AnalysisFilter,
+    mut callback: impl FnMut(&dyn AnalyzerSignal) -> ControlFlow<B>,


It may make sense to define a type alias for the Callback type and copy some of the explanations you gave in the other comments into its documentation so that future me will know what I need to return in the control flow.

MichaReiser · 2022-06-03T15:46:10Z

crates/rome_analyze/src/registry.rs

        &self,
        file_id: FileId,
        root: &JsAnyRoot,
        node: JsSyntaxNode,
-        callback: &mut impl FnMut(&dyn AnalyzerSignal),
-    ) {
+        callback: &mut impl FnMut(&dyn AnalyzerSignal) -> ControlFlow<B>,


Nit: Use the type alias here too?

MichaReiser · 2022-06-03T15:46:10Z

crates/rome_analyze/src/registry.rs

        &self,
        file_id: FileId,
        root: &JsAnyRoot,
        node: JsSyntaxNode,
-        callback: &mut impl FnMut(&dyn AnalyzerSignal),
-    ) {
+        callback: &mut impl FnMut(&dyn AnalyzerSignal) -> ControlFlow<B>,


Nit: Use the type alias here too?

leops · 2022-06-03T16:03:54Z

It may make sense to define a type alias for the Callback type and copy some of the explanations you gave in the other comments into its documentation so that future me will know what I need to return in the control flow.

Unfortunately it's not possible to move that declaration in a type alias since impl Trait is not stable in type aliases (and it's not possible to alias the FnMut(&dyn AnalyzerSignal) -> ControlFlow<B> trait under another name either)

ematipico · 2022-06-03T16:38:07Z

Maybe we should wait to merge this PR until we do the release next week?

github-actions · 2022-06-09T15:38:55Z

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	5595	5595	0
Panics	1	1	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	12391	12391	0
Failed	3866	3866	0
Panics	0	0	0
Coverage	76.22%	76.22%	0.00%

leops temporarily deployed to aws May 25, 2022 15:38 Inactive

leops temporarily deployed to aws May 25, 2022 15:45 Inactive

leops mentioned this pull request May 25, 2022

☂️ Analyzer / Linter Infrastructure #2561

Closed

9 tasks

xunilrj reviewed May 26, 2022

View reviewed changes

crates/rome_analyze/src/lib.rs Outdated Show resolved Hide resolved

ematipico suggested changes May 26, 2022

View reviewed changes

leops temporarily deployed to aws May 30, 2022 14:09 Inactive

MichaReiser approved these changes Jun 3, 2022

View reviewed changes

leops temporarily deployed to aws June 3, 2022 16:03 Inactive

ematipico changed the title ~~feat(rome_analyze): add a fix file command to the CLI and LSP~~ feat(rome_analyze): add a fix argument to rome check command CLI and LSP Jun 3, 2022

ematipico approved these changes Jun 3, 2022

View reviewed changes

leops added 4 commits June 9, 2022 17:34

feat(rome_analyze): add a fix file command to the CLI and LSP

308cf4b

add a help page for the fix command + document the no-colors option

3b35fe6

merge the fix command into check

305f8b3

add more documentation to the ControlFlow type alias

3bc3389

leops force-pushed the feature/auto-fix branch from 60e7198 to 3bc3389 Compare June 9, 2022 15:35

leops temporarily deployed to aws June 9, 2022 15:35 Inactive

leops merged commit a4815e4 into main Jun 9, 2022

leops deleted the feature/auto-fix branch June 9, 2022 15:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rome_analyze): add a fix argument to `rome check` command CLI and LSP #2615

feat(rome_analyze): add a fix argument to `rome check` command CLI and LSP #2615

leops commented May 25, 2022 •

edited

github-actions bot commented May 25, 2022 •

edited

cloudflare-pages bot commented May 25, 2022 •

edited

xunilrj May 26, 2022

leops May 30, 2022

xunilrj May 26, 2022

leops May 30, 2022

ematipico left a comment •

edited

ematipico May 26, 2022

xunilrj May 26, 2022

leops May 30, 2022

ematipico May 26, 2022

leops May 30, 2022

MichaReiser Jun 3, 2022

ematipico May 26, 2022

leops May 26, 2022

MichaReiser Jun 3, 2022

MichaReiser Jun 3, 2022

MichaReiser Jun 3, 2022

leops commented Jun 3, 2022

ematipico commented Jun 3, 2022

github-actions bot commented Jun 9, 2022

feat(rome_analyze): add a fix argument to rome check command CLI and LSP #2615

feat(rome_analyze): add a fix argument to rome check command CLI and LSP #2615

Conversation

leops commented May 25, 2022 • edited

Summary

Test Plan

github-actions bot commented May 25, 2022 • edited

cloudflare-pages bot commented May 25, 2022 • edited

Deploying with Cloudflare Pages

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leops commented Jun 3, 2022

ematipico commented Jun 3, 2022

github-actions bot commented Jun 9, 2022

Parser conformance results on ubuntu-latest

js/262

jsx/babel

symbols/microsoft

ts/babel

ts/microsoft

feat(rome_analyze): add a fix argument to `rome check` command CLI and LSP #2615

feat(rome_analyze): add a fix argument to `rome check` command CLI and LSP #2615

leops commented May 25, 2022 •

edited

github-actions bot commented May 25, 2022 •

edited

cloudflare-pages bot commented May 25, 2022 •

edited

ematipico left a comment •

edited