Use translated Prism AST to run RuboCop

[vinistock]

Motivation

Use the Prism AST translator to reuse our existing AST to run RuboCop.

Implementation

RuboCop doesn't have an API where we can simply ask "format this AST", so implementing this requires patching a handful of places so that we can pass the AST around.

The idea is that we instantiate the ProcessedSource ourselves and prevent the Prism translator from re-parsing the file if we already have an AST in hand.

Questions

1. Is this the best way to pass the existing AST around? Could we be doing this in a more elegant way?
2. What do we need to do to prevent this from failing in older RuboCop versions that do not support Prism as a backend?

Automated Tests

Will add tests.

Manual Tests

1. Start the LSP on this branch
2. Verify that diagnostics and formatting is working properly

[vinistock]

Tagging @koic and @kddnewton. I'd love to hear your thoughts on the approach and if there's something that can be upstreamed to Prism or RuboCop to make this integration more seamless.

[kddnewton]

Looks good. In general I think we should provide the ability to pass through a parsed AST to Prism::Translation::Parser in prism itself instead of patching it here. One way to do this would be change it in prism to do:

def parse(source_buffer)
  ...
  Prism.parse(...)
  ...
end

instead to:

def initialize(parser: Prism)
  @parser = parser
end

def parse(source_buffer)
  ....
  @parser.parse(...)
  ...
end

and then pass in a custom object in ruby-lsp for the parser that would return the parse result.

[vinistock]

We could avoid some patching if there was a way to instantiate the ProcessedSource with an existing AST and token list. @koic, would you be open to accepting that?

It would allow us to avoid some patching.

[koic]

As I'm not familiar with Ruby LSP, I cannot say much at this point. However, I'd suggest opening a PR to RuboCop AST. There might be adjustments needed regarding the API, but I think they could be acceptable depending on the use case. cc @bbatsov @marcandre
https://github.com/rubocop/rubocop-ast

[marcandre]

I'm curious about the context. In particular, why is @parse_result being passed around as an AST and not a ProcessedSource? I.e., would it be possible upstream in the code to generate a ProcessedSource instead of just an AST, and pass that information instead of just the AST?

[vinistock]

All features in the Ruby LSP depend only the Prim's parse result, so we keep that in our representation of documents.

For RuboCop, we just invoke run with the file contents, which will let it re-parse the file and do everything it needs to do.

The Ruby LSP supports multiple formatters. We hook into RuboCop if it is available, but it's not a dependency. So we can't depend on RuboCop internals for our basic document representations, like using a ProcessedSource. All RuboCop things are limited to the runner here.

Now that RuboCop supports the Prism backend, we're trying to find what the easiest way of reusing the existing AST is, so that we can stop parsing documents twice.

The ideal API would be something like what Syntax Tree supports, which allows you to format a given AST. That would not only simplify the code here a lot, it would enable RuboCop to support range formatting for LSPs, which is currently impossible.

That said, the ideal API may require significant refactors, which is why I suggested something a bit simpler that will still minimize the number of patches. If we can instantiate the ProcessedSource in a way that allows us to reuse the existing AST and prevents the second parse, that'll already simplify the code quite a bit.

[marcandre]

But a ProcessedDocument also holds the tokens and the comments

[vinistock]

Sorry, I don't understand what you mean. Are there two different classes that would require changing? Or are you saying we could be using something else?