Shopify / ruby-lsp

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

Support `module_function` in the indexer #2653

Open vinistock opened 2 weeks ago

vinistock commented 2 weeks ago

When module_function is invoked, the given method becomes available in the singleton class and it is turned into a private method on including namespaces. For example:

module A
  def foo; end
  module_function :foo
end

A.foo # okay!

class Something
  include A
end

Something.new.foo # not okay, instance method foo is private

We need to add processing for module_function here. Essentially, when we find an invocation to module_function, we want to:

  1. Get the method entry that was already inserted in the index. Update the visibility to private
  2. Add a new method entry for the same method, but in this case the owner is the singleton class
IgnacioFan commented 1 week ago

Hi team,

I am taking a look at this issue and will push something a few days later.

vinistock commented 1 week ago

Hello! Sounds good, I have assigned the issue to you. If you hit any blockers or have any doubts, please do not hesitate to ask for assistance here and the team can discuss the implementation.

IgnacioFan commented 1 week ago

Hi @vinistock,

Thank you for pointing out where the method entry should proceed. It's really helpful for me to start looking around the issue 😊.

For the second requirement, I am confused about what you mean by "the same method." Do you mean on_call_node_enter in declaration_listener.rb?

Also, I would like to double-check with you for the first requirement. I think the fix is to add a new entry point, module_function, to the on_call_node_enter and set the visibility as private (please see the following image). Please correct me if I am wrong! I am still attempting to understand what the visibility_stack is and how it works.

Image

vinistock commented 1 week ago

You're on the right track, but it's slightly different. When the indexing process finds the module_function invocation, we already finished processing the method declaration. So we don't want to change the visibility stack, we want to mutate the entry that was already inserted in the index.

For example

module Foo
  # We process this method declaration and add an `Entry::Method` for it
  def something; end

  # After adding it to the index, then we find the method call to `module_function`
  module_function :something
end

So we need to operate on the entry that we just inserted. Here's the pseudo code of how this should happen

case message
# ...
when :module_function
  # get the argument for module function from the node, which is the name of the method we are
  # turning into a module function

  # then find the existing entry for the method. It will be something like
  entries = @index.resolve_method(name_of_method, owner_name)

  entries.each do |e|
    # then turn the existing entries into private (this API may not exist yet)
    e.visibility = Entry::Visibility::PRIVATE

    # and add the singleton version of the method to the index (which is what module_function does)
    # here the important part is that this method's owner will not be the same owner, but the 
    # singleton version of it
    singleton_owner = @index.existing_or_new_singleton_class(name_of_owner)
    new_entry = Entry::Method.new(..., singleton_owner)
  end
end
IgnacioFan commented 1 day ago

@vinistock, thanks for the pseudo code. I am now writing a test, but I need to figure out how to test the singleton entry.

Right now, the following code is a piece of tests placed in method_test.rb.

def test_visibility_tracking_with_module_function
  index(<<~RUBY)
    module Foo
      def foo; end
      module_function :foo
    end
    class Bar
      include Foo
    end
    Bar.new.foo
  RUBY

  assert_entry("foo", Entry::Method, "/fake/path/foo.rb:1-2:1-14", visibility: Entry::Visibility::PRIVATE)
  # How can I make another assertion to test Bar.new.foo?
end
vinistock commented 1 day ago

You need to get a handle for the entry so that you can assert that the owner is correct (example).

When you get the entries for @index["foo"], there should be two entries:

  1. One for the original made, which was made private after invoking module_function. That one should be owned by the attached class (Foo)
  2. And the second for the public singleton method, which should be owned by Foo::<Class:Foo>
IgnacioFan commented 23 hours ago

Hi @vinistock, thank you! I think I am ready to push code for reviews, but I got permission denied 😅

vinistock commented 14 hours ago

You need to fork the repository in order to propose PRs in open source repositories you don't have permissions. This guide explains it in more detail https://docs.github.com/en/get-started/exploring-projects-on-github/contributing-to-a-project.