Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support module_function in the indexer #2653

Closed
vinistock opened this issue Oct 2, 2024 · 8 comments · Fixed by #2733
Closed

Support module_function in the indexer #2653

vinistock opened this issue Oct 2, 2024 · 8 comments · Fixed by #2733
Assignees
Labels
good-first-issue Good for newcomers help-wanted Extra attention is needed pinned This issue or pull request is pinned and won't be marked as stale

Comments

@vinistock
Copy link
Member

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
@vinistock vinistock added good-first-issue Good for newcomers help-wanted Extra attention is needed pinned This issue or pull request is pinned and won't be marked as stale labels Oct 2, 2024
@IgnacioFan
Copy link
Contributor

Hi team,

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

@vinistock
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

IgnacioFan commented Oct 16, 2024

@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
Copy link
Member Author

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
Copy link
Contributor

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

@vinistock
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good-first-issue Good for newcomers help-wanted Extra attention is needed pinned This issue or pull request is pinned and won't be marked as stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants