Provide code navigation features to erb files #2235
Conversation
st0012
added
enhancement
|
Worth adding an ERB fixture just for ease of tophatting? |
This commit allows Ruby LSP to start handling requests for ERB files, such as definition, completion, hover...etc., which will give users the same level of code navigation features in ERB and Ruby. However, certain requests are not supported: - formatting - on_type_formatting - diagnostics - code_actions Co-authored-by: Vinicius Stock <[email protected]>
Currently their receivers will be assumed as `Object`, which means most helper methods like `link_to` won't have completion even though we do have it in the index and are sure that `Object` doesn't define it. By avoiding setting the receiver as `Object`, we can greatly increase the number methods that can have definition support.
| @response_builder = response_builder | ||
| @global_state = global_state | ||
| @index = T.let(global_state.index, RubyIndexer::Index) | ||
| @type_inferrer = T.let(global_state.type_inferrer, TypeInferrer) | ||
| @uri = uri | ||
| @document = document |
There was a problem hiding this comment.
I would really like to avoid giving any listeners (even the LSP's own listeners) access to document. We designed the listener approach exactly for this reason, to guarantee that any AST traversal was always controlled by the LSP and not feature implementations.
Can we instead pass the document language ID as an enum?
There was a problem hiding this comment.
I thought the idea is to NOT expose language_id as an attribute but instead use the class to check the document type? If we use that approach in server.rb, we should use it in other places as well?
There was a problem hiding this comment.
Yes, for the server I believe that's a better idea because Sorbet will narrow types and help us get more out of the type checker.
But in this specific scenario, I really want to avoid passing document down. Maybe we can pass the document's class rather than the instance? Then we can differentiate the behaviour based on that.
| @@ -40,6 +40,8 @@ def perform | |||
| return unless @active_formatter | |||
| return if @document.syntax_error? | |||
|
|
|||
| # We don't format erb documents yet | |||
| document = @store.get(uri) | ||
|
|
||
| return send_empty_response(message[:id]) if document.is_a?(ERBDocument) | ||
|
|
||
| response = document.cache_fetch("textDocument/diagnostic") do |document| |
There was a problem hiding this comment.
Ideally, we should keep @store.cache_fetch as the "public" API for interacting with the cache. How about we use this?
response = @store.cache_fetch(uri, "textDocument/diagnostic") do |document|
case document
when RubyDocument
# ...
else
# non supported documents will just return `nil`, which is the same
# that send_empty_response is doing
nil
end
endThere was a problem hiding this comment.
Consider @store.cache_fetch is literally just get(uri).cache_fetch(request_name, &block), I don't feel we're losing anything by calling document.cache_fetch.
In the contrary, I think the fact that we will need a comment for the condition shows that the proposed implementation is potentially confusing?
I also feel the current implementation fits better with our current convention: if we don't need to go further, we return early.
There was a problem hiding this comment.
I also feel the current implementation fits better with our current convention: if we don't need to go further, we return early.
It depends on which convention you are prioritizing. We also don't manipulate the document's cache without going through the store anywhere, which means we'd be breaking a different convention.
In my head, in one case you are changing the implementation of a request, which should be flexible anyway since requests may operate differently and have different requirements. In the other, you are reaching into a lower level API.
It's not a strong opinion and if you feel like the case statement is confusing, feel free to move forward this way.
| @@ -37,13 +37,18 @@ def get(uri) | |||
| return document unless document.nil? | |||
|
|
|||
| path = T.must(uri.to_standardized_path) | |||
| set(uri: uri, source: File.binread(path), version: 0) | |||
| language_id = path.end_with?(".erb") ? "erb" : "ruby" | |||
There was a problem hiding this comment.
We should include rhtml as a possible ERB extension too, since that's a part of our grammar.
There was a problem hiding this comment.
TIL rhtml. I think we'll need to add it to the extension's documentSelector too?
There was a problem hiding this comment.
Not necessary because the grammar already assigns the erb language ID to that file type. Similar to how we assign the ruby ID to several different files, like Rakefile, Gemfile, etc.
Motivation
This commit allows Ruby LSP to start handling requests for ERB files, such as definition, completion, hover...etc., which will give users the same level of code navigation features in ERB and Ruby.
However, certain requests are not supported:
Implementation
ERBDocument(written by @vinistock) with a scanner that returns extracted Ruby and remaining html code (not used yet)Objectas view helper calls receiver type as it'll stop Ruby LSP from suggesting any definition candidatesDemo
erb-support-demo-1.mp4
Automated Tests
Manual Tests