Tools: 📎 Add autofix commands to LSP

[sebmck]

[yassere]

I can work on this.

Is there a good way to support suggestions autofixes through LSP, or do we just want a setting that enables our recommended --apply autofixes with codeActionsOnSave?

[sebmck]

@yassere Yeah you can reuse the way rome check --review works using action diagnostic advice. (Explanation is fairly elaborate, you don't need to know all of it to use but wanted to be comprehensive)

What is action diagnostic advice?

action advice references an operation that could be performed on the diagnostic. It contains information needed to query the server to perform. By default, we will print these actions as a CLI command suggestion.

• DiagnosticAdviceItem type
• cli-diagnostics printer

For example:

// (there are other optional props)
const item: DiagnosticAdviceAction = {
  type: "action",
  command: "test",
  args: ["foo"],
  commandFlags: {
    seb: "astian",
  },
}

would be printed in a diagnostic as:

$ rome test foo --seb astian

Except when the hidden property is set we hide it when printing, but it still exists in advice for extraction. You can however force them to be visible by running rome check --verbose-diagnostics.

How does the linter use action?

In CompilerContext#addFixableDiagnostic, when we are given the suggestions property, we push on a bunch of diagnostic advice that will show the different suggestions when printing the diagnostic.

We use the buildLintDecisionAdviceAction method in packages/@romefrontend/compiler/lint/decisions.ts to construct the suggestion advice, which sets hidden: true since it's extremely verbose to print.

How does the action indicate specific lint autofixes?

It's easier to demonstrate so I'll use the following code as an example:

let foo;
foo == "bar";

when linted with rome check test.ts --verbose-diagnostics will output:

 test.ts:2 lint/js/doubleEquals  FIXABLE  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ✖ Use === instead of ==.

    1 │ let foo;
  > 2 │ foo == "bar";
      │ ^^^^^^^^^^^^

  ℹ == is only allowed when comparing against null.

  Suggested fix: Use ===

  - foo·==·"bar"
  + foo·===·"bar"

  ℹ This may be unsafe if you are relying on type coercion

  ℹ To apply this fix run
  $ rome check test.ts --decisions fix-lint/js/doubleEquals-test.ts-2:0-0

  ℹ To suppress this error run
  $ rome check test.ts --decisions suppress-lint/js/doubleEquals-test.ts-2

  ℹ To add suppression comments for ALL files with this category run
  $ rome check --decisions global-suppress-lint/js/doubleEquals

Note the commands are now only visible because of the previously mentioned --verbose-diagnostics argument. The advice format isn't specific to the CLI command we output, we just convert it during printing. It's structured enough that we can use it verbatim internally as a server query.

You can see there's a structured --decisions argument that contains details on what fixes/actions should only be applied. You'll note that it also includes the ability to add suppressions so it's fairly generic and we can add other capabilities to the linter later if we wanted.

It also indicates the filename so you can chain them together.

This decisions argument is provided by the buildLintDecisionAdviceAction method, where we set a decisions property in commandFlags. packages/@romefrontend/compiler/lint/decisions.ts also contains the relevant code/methods we use to construct the decisions strings, and then parse them.

How does compiler get decisions from the CLI?

The check command in packages/@romefrontend/core/server/commands/check.ts declares the flag here. It's declared as an "implicit array" meaning that there could be multiple. ie. rome check --decisions foo --decisions bar. You need to be explicit about accepting this in flags when you're consuming them in defineFlags.

Then in the command callback is where we do the actual parsing here. We call parseDecisionStrings which is from the decisions.ts that's exported from the compiler module.

The parsed result is separated into lintCompilerOptionsPerFile and globalDecisions. globalDecisions contains decisions that we want to pass to all lint calls, while lintCompilerOptionsPerFile is a map where we only pass what that file actually needs. We pass both of these as LinterOptions to the Linter class.

In Linter#runLint we extract the options we're going to pass to compiler.lint().

packages/@romefrontend/compiler/lint/index.ts is where compiler.lint is located. Here we do two special things:

• Pass the options straight into CompilerContext to tell it what autofixes/suggestions should be chosen (explanation in the next section)
• Call addSuppressions which is where we perform suppression comment insertion

How does the compiler selectively apply decisions?

The logic for all of this is in CompilerContext. The notable methods used are:

• hasLintDecisions(): boolean - We use this to indicate if we should block all fixes that aren't in the decisions array. This uses the hasDecisions compiler option that was passed into us by Linter. We can't use decisions.length > 0 as an indicator because there could have been decisions selected for other files.
• getLintDecisions(key): Array<LintCompilerOptionsDecision> - We take in a "decision key" that was derived from the source location (it's just a string in the format "LINE:COLUMN"). We return a structured array of the decisions for that specific position.
• addFixableDiagnostic(opts, description): TransformExitResult - Checks hasLintDecisions, if true then only returns the fixed node if there was a fix decision with the expected diagnostic category.

Ok... how would that be used by the LSP server?

I know nothing about how LSP code actions work, but to implement this:

1. When sending diagnostics to the LSP, extract the actions and store them somewhere, and emit whatever messages/structure to tell the LSP client you need to have them visible.
2. When receiving an autofix signal from the LSP client, figure out what diagnostic action it referred to.
3. Build a server query and from inside LSPServer call this.server.handleRequest(this.client, query)
4. We should automatically pick up the file buffer so there isn't anything more to do there.

However, we will write the updated files to disk. Not sure how that interacts with code actions. ie. do we need to send back a text patch of the updated file, is the file automatically saved before running, will it load the saved file etc.

If we needed to we could add another flag to the check command called --no-save that wouldn't write files to disk and would instead return an object of files that need to be saved.

Commands have the ability to return structured data. You can see an example of this with the status command. The client command calls the server command that returns an object.

Other potentially useful references

• --review argument logic: packages/@romefrontend/core/client/review.ts

[sebmck]

However, we will write the updated files to disk. Not sure how that interacts with code actions. ie. do we need to send back a text patch of the updated file, is the file automatically saved before running, will it load the saved file etc.

If we needed to we could add another flag to the check command called --no-save that wouldn't write files to disk and would instead return an object of files that need to be saved.

Commands have the ability to return structured data. You can see an example of this with the status command. The client command calls the server command that returns an object.

Actually, there's the potential for a new abstraction here. Saving/updating files is common amongst a bunch of the commands, notably test snapshots and lint autofixes.

We can have a method in ServerRequest to push a file to update/save when the request has completed. This is useful since we can consolidate logic to verify the accuracy of our saved files. ie. we don't want to update a file on disk that's changed after we originally read the file.

We can then by default, save files at the end of the request, with an optional flag to disable it. We can also populate it as a field in the ServerRequestQueryResponse, which would allow the LSP, or any other "client", to handle how they want to save files however they want.

Opened #878 to track and I'll hopefully get time to work on that specific piece today.

[yassere]

@sebmck Thanks for the detailed explanation! I'll let you know if I have any questions once I've gone through the code in more depth.