Shopify / ruby-lsp

An opinionated language server for Ruby
https://shopify.github.io/ruby-lsp/
MIT License
1.63k stars 168 forks source link

Use the document link request to link to Rails documentation #258

Closed egiurleo closed 2 years ago

egiurleo commented 2 years ago

We implemented the DocumentLink request in #58 and used it to go to method definition from gem RBIs.

As a next step, let's use this request to link to the correct Rails documentation for methods that come from the rails gem.

When a user hovers over a method that's defined in rails, there should be a link that says "go to documentation." We can use information about the method, such as the class that defines it and its ancestor chain, as well as information about the rails gem (version number) to link to the correct documentation page for that method.

st0012 commented 2 years ago

I'm not sure if the implementation in my head is the best solution, so I want to discuss it here:

Link format

Class (or module)

Method

Information required for making the link

Implementation

  1. When the request comes in, extract the gem's name and version from it.
  2. Check if it's a Rails gem. Return if it's not.
  3. Create a namespace stack
  4. Parse the document and
    1. When we enter a class/module definition, create a link and push it to the stack
    2. When we visit a method definition, create a link with the class/module name on the stack + the method name
egiurleo commented 2 years ago

@st0012 This sounds like a really good plan to me! I have one question: would it be possible to eliminate the use of the stack by using things like the Object#const_source_location or the Method#owner to discover the namespaces of each const and method?

st0012 commented 2 years ago

I think it's not possible because we're not actually evaluating the code? We can only access names instead of the objects from syntax tree AFAIK.

vinistock commented 2 years ago

Oh, it's unfortunate that the classes and modules are a part of the URL. However, we don't need to add document links for every single method exported by Rails. Just providing this for the most common ones will probably already provide quite a bit of value.

What do you think about doing something like this

class DocumentLink < BaseRequest
  BASE_RAILS_DOC_URL = "https://api.rubyonrails.org/v#{RAILS_VERSION}" # read the rails version from the stubs
  METHOD_TO_DOC_URL = {
    "belongs_to" => "classes/ActiveRecord/Associations/ClassMethods.html"
  }

  def visit_command(node) # Not sure if it's command, just an example
    suffix = METHOD_TO_DOC_URL[node.value] # or node.message something like that
    return if suffix.nil? # We don't know the doc URL for this one

    target = "#{BASE_RAILS_DOC_URL}/#{suffix}#method-i-#{node.value}"
    # create link from target
  end
end

I suggest prioritizing the following. You can start by implementing only one of them in an initial PR (e.g.: only belongs_to) to get feedback on the logic. After we settled on that, it's just a matter of adding more entries to the hash in follow up PRs.

paracycle commented 2 years ago

Rails API guides have a search mechanism which relies on the data stored in the search_index.js file. The data is basically JSON, which we can use to match method names to discover the full doc URL.

For example, the info hash contains the following for run_callbacks:

  // ...
  [
    "run_callbacks",
    "ActiveSupport::Callbacks",
    "classes/ActiveSupport/Callbacks.html#method-i-run_callbacks",
    "(kind)",
    "<p>Runs the callbacks for the given event.\n<p>Calls the before and around callbacks in the order they were set, …\n"
  ],
  // ...
st0012 commented 2 years ago

@vinistock I think the white listing approach is not ideal for Rails documentation as there are many methods and components that could use this feature. So the list will end up very long and we'll need to manually select things to be added. I'd like to avoid this if possible.

@paracycle The search_index.js is very interesting and I didn't know it exists until now. For anyone who's curious, the file is generated by rdoc so it's not Rails-specific. I thought about using it as a database, but since most of the data are stored in arrays, we may need to transform it first (or bsearch on elements?).

I'll prototype the proposed flow to see if the current rbi files can provide reliable context for generating the urls. If not, I'll use search_index.js to do that.

Morriar commented 2 years ago

As another aspect to this issue, can we also consider adding comments to the generated RBIs for DSLs in Tapioca?

Thanks to the DSL compilers, a lot of the methods created through Rails meta-programming get reified in the RBI files and appear when the user overs them.

We could make the DSL compilers attach comments to the generated RBI nodes so it explains where the things come from and points to the documentation of the feature.

Morriar commented 2 years ago

We can use information about the method, such as the class that defines it and its ancestor chain, as well as information about the rails gem (version number) to link to the correct documentation page for that method.

Consider this:

# file1.rb
class CustomJob < ApplicationJob
  # ...
end
# file2.rb
class SomeJob < CustomJob
  # ...
end

Only considering file2.rb won't give you much information about what you could document inside SomeJob, you would need to resolve CustomJob and then find out that it inherits ApplicationJob.

If we want to rely on the model we will have to know about the whole code base and I fear that anything not based on Sorbet will be too slow.

In the meantime we can prototype with the whitelist approach but we may link things that are unrelated by accident.

st0012 commented 2 years ago

I thought this link will only be activated in rbi files? Because the current flow is like:

  1. Go to definition via Sorbet
  2. Land on a rbi file
  3. Use the source comment to visit the source

So I think the Rails document link will only happen in the rbi file too:

  1. Go to definition via Sorbet
  2. Land on a rbi file
  3. When on a method, say belongs_to, it'll show the user an option to visit the document

If we try to resolve the method source from application code directly, I think it'd be too inaccurate.

st0012 commented 2 years ago

After discussing with @vinistock we had these conclusion:

  1. The link will be shown in application code, e.g. app/models/post.rb. Not the rbi files I assumed.
  2. We'll use the search_index file mentioned by @paracycle as the data source.
    • The file will be fetched during LSP initialisation via net/http and be stored inside a constant.
    • The fetching should be fail-safe
  3. Because some methods could be common among libraries, like belongs_to is common in API serialisation tools. We may need to use the current file's path to decide if we should show the link. For example, if it's not under app/models/, we don't show links for anything under ActiveRecord's namespaces.
  4. We'll skip ActiveSupport and ActionView at the beginning because it could be hard to determine if the methods are actually from them.
joosnaak27 commented 2 years ago

Ok is cool