Support Hover, Completion, Go to definition for Class Variables

[rogancodes]

Motivation

closes #2897

Implementation

I’ve made separate commits for each feature, hoping it makes the review process easier.

Automated Tests

Added new tests

Manual Tests

Screencast.from.01-12-24.07.46.01.PM.IST.webm

[vinistock]

This is looking great. Just a small gotcha about class variables that we need to be careful about

[vinistock]

I believe we actually need to add the class variables to both the attached and the first level singleton class. For example:

# Inside singleton context
class Foo
  class << self
    @@hello = 123
  end
end

Foo.class_variables # => [:@@hello]
Foo.singleton_class.class_variables # => [:@@hello]
Foo.singleton_class.singleton_class.class_variables # => []

# Outside singleton context
class Bar
  @@hello = 123
end

Bar.class_variables # => [:@@hello]
Bar.singleton_class.class_variables # => [:@@hello]
Bar.singleton_class.singleton_class.class_variables # => []

So it seems that we need to always add to both. Can you please add a test for that?

[rogancodes]

Ok, should we push two entries: one for the singleton class and another for the base class?

[vinistock]

Yes, correct. That will make the features properly find the variable both in a singleton context or in the attached class context, since it is available for both.

[rogancodes]

I have updated the logic to add class variable entries to both the singleton class and the base class.

    def handle_class_variable(node, loc)
      name = node.name.to_s
      return if name == "@@"

      comments = collect_comments(node)
      owner_stack = @owner_stack.reverse

      owner = owner_stack.shift
      @index.add(Entry::ClassVariable.new(
        name,
        @uri,
        Location.from_prism_location(loc, @code_units_cache),
        comments,
        owner,
      ))

      case owner
      when Entry::SingletonClass
        owner_stack.each do |owner|
          @index.add(Entry::ClassVariable.new(
            name,
            @uri,
            Location.from_prism_location(loc, @code_units_cache),
            comments,
            owner,
          ))

          break unless owner.is_a?(Entry::SingletonClass)
        end
      when Entry::Class
        @index.add(Entry::ClassVariable.new(
          name,
          @uri,
          Location.from_prism_location(loc, @code_units_cache),
          comments,
          @index.existing_or_new_singleton_class(owner.name),
        ))
      end
    end

If the owner is a class node, I add a class variable entry to both its class and its singleton class. If the node is a singleton class, I reverse the owner stack and continue adding the class variable to the owners in the stack, stopping once I reach the second owner that is not a singleton class. Bubbling up works as expected, but I am uncertain how to implement bubbling in.

Screencast.from.15-12-24.12.23.09.AM.IST.webm

[vinistock]

I don't understand why we need to go through the owner stack in reverse. Can we not do something like this?

owner = current_owner
return unless owner

if owner.is_a?(Entry::SingletonClass)
  # If we're inside a singleton context of a class or module, add the
  # variable to the attached namespace
  @index.add(Entry::ClassVariable.new(
    name,
    @uri, 
    Location.from_prism_location(loc, @code_units_cache), 
    comments,
    @index.attached_class(owner.name),
   ))
else
  # If we're inside a class or a module, add the variable to the singleton
  # class of that namespace
  @index.add(Entry::ClassVariable.new(
    name,
    @uri, 
    Location.from_prism_location(loc, @code_units_cache), 
    comments,
    @index.existing_or_new_singleton_class(owner.name),
   ))
end

And then you essentially need to implement Index#attached_class to do the reverse of what existing_or_new_singleton_class. Something like this

def attached_class(name)
  # Build the attached name excluding the last portion, which for singletons
  # is always `<Class:Foo>`
  parts = name.split("::")
  attached_name = parts[...-1].join("::")

  # Return the existing namespaces for the attached name
  self[attached_name]&.grep(Entry::Namespace)
end

[rogancodes]

I'm not entirely confident about the singleton context and feel a bit confused regarding how our DeclarartionListener and TypeInferrer interpret it. Consider the example below.

class Foo
  # The declaration listener interprets the owner here as "Foo".
  # The type inferrer interprets the owner here as "Foo::<Class:Foo>".
  @@aa = 1

  class << self
    # The declaration listener interprets the owner here as "Foo::<Class:Foo>".
    # The type inferrer interprets the owner here as "Foo::<Class:Foo>::<Class:<Class:Foo>>".
    @@ab = 2

    def bar
      # The declaration listener interprets the owner here as "Foo::<Class:Foo>".
      # The type inferrer interprets the owner here as "Foo::<Class:Foo>".
      @@ac = 3
    end
  end

  def foobar
    # The declaration listener interprets the owner here as "Foo".
    # The type inferrer interprets the owner here as "Foo".
    @@ad = 5
  end
end

I reversed the owner stack based on the owner names inferred by the TypeInferrer. For instance, at the line where @@ab is defined, the owner stack would appear as [Foo, Foo::Class:Foo, Foo::Class:Foo::<Class:Class:Foo>]. Given that class variables are shared across these contexts, I reversed the stack and added a class variable entry for @@ab to each owner until encountering a class entry, using an each loop in a do-while manner.

However, after reviewing your comments, I debugged the DeclarationListener and found that its inferred context differs from the TypeInferrer's inferred context. This discrepancy might be the source of confusion. This discrepancy has left me wondering, am I missing something or misunderstanding how these contexts works?

[vinistock]

I understand the confusion now. The changes in the type inferrer are assuming that class variables will behave exactly like instance variables, which is likely the source of the issue.

I think we may be able to do this:

1. In the declaration listener during indexing, only add an entry for the attached class (forget my previous comment about storing a copy in the singleton version)
2. Then in the type inferrer, when detecting the self type for class variables, we essentially always return whatever the attached class of the current context is, since the class variables will always be associated to them. Something like this

class Foo
  # The inferred `self` here is `Foo` for class variables
  @@var = 123

  class << self
    # The inferred `self` here is also `Foo` for class variables
    @@other = 123
  end
end

I think that should lead to all of the features working as expected and it will simplify the declaration listener. Let me know if you find that this is not the right way.

[rogancodes]

I have made the changes as per your suggestion.

[vinistock]

I think we can narrow the type with a grep, so that even if someone passes a name that doesn't belong to a class variable, it still works.

Suggested change

	entries = T.cast(self[variable_name], T.nilable(T::Array[Entry::ClassVariable]))
	return unless entries
	entries = self[variable_name]&.grep(Entry::ClassVariable)
	return unless entries&.any?

[andyw8]

Let's add a comment to explain that this isn't valid Ruby, but the code could be temporarily in this state.

[andyw8]

Suggested change

	def test_completion_for_class_variables_show_only_uniq_entries
	def test_completion_for_class_variables_show_only_unique_entries