Enhance scope definition resolution and add tests for nested class constants

Author: kwerle

lib/ruby_language_server/project_manager.rb

@@ -160,7 +160,7 @@ def word_at_location(uri, position)
 
     def possible_definitions(uri, position)
       name = word_at_location(uri, position)
-      return {} if name.blank?
+      return [] if name.blank?
 
       # Special case: "new" should find "initialize" (instance method, not class method)
       name = 'initialize' if name == 'new'
@@ -230,60 +230,91 @@ def possible_definitions(uri, position)
       results = scope_definitions_for(name, scope, uri)
       return results unless results.empty?
 
-      project_definitions_for(name, current_scopes)
+      # Always return an array
+      project_definitions_for(name, current_scopes) || []
     end
 
     # Return variables found in the current scope.  After all, those are the important ones.
     # Should probably be private...
     def scope_definitions_for(name, scope, uri)
       check_scope = scope
-      return_array = []
+      matches = []
       while check_scope
         check_scope.variables.each do |variable|
-          return_array << Location.hash(uri, variable.line, 1) if variable.name == name
+          matches << Location.hash(uri, variable.line, 1) if variable.name == name
         end
         check_scope = check_scope.parent
       end
-      RubyLanguageServer.logger.debug("==============>> scope_definitions_for(#{name}, #{scope.to_json}, #{uri}: #{return_array.uniq})")
-      return_array.uniq
+      RubyLanguageServer.logger.debug("==============>> scope_definitions_for(#{name}, #{scope.to_json}, #{uri}: #{matches.inspect} )")
+      matches
     end
 
     # class_method_filter is for new -> initialize
     def project_definitions_for(name, parent_scopes = [], class_method_filter = nil)
-      results = []
-
+      # If no parent scope, just return all matches as before
       if parent_scopes.empty?
-        # No parent scopes provided - search all top-level scopes
         all_scopes = RubyLanguageServer::ScopeData::Scope.where(name: name)
         all_scopes = all_scopes.where(class_method: class_method_filter) unless class_method_filter.nil?
-        results.concat(all_scopes.to_a)
-
-        # Also search for constants at root level
         all_variables = RubyLanguageServer::ScopeData::Variable.where(name: name)
-        results.concat(all_variables.to_a)
-      else
-        # Start with the deepest (first) scope and search upward through parent chain
-        current_scope = parent_scopes.first
-        while current_scope
-          # Search for child scopes with matching name in current scope
-          child_scopes = current_scope.children.where(name: name)
-          child_scopes = child_scopes.where(class_method: class_method_filter) unless class_method_filter.nil?
-          results.concat(child_scopes.to_a)
-
-          # Search for variables with matching name in current scope
-          matching_variables = current_scope.variables.where(name: name)
-          results.concat(matching_variables.to_a)
-
-          # If we found results, stop searching (most specific scope wins)
-          break unless results.empty?
-
-          # Move up to parent scope
-          current_scope = current_scope.parent
+        all_matches = all_scopes.to_a + all_variables.to_a
+        return all_matches.reject { |item| item.code_file.nil? }.map do |item|
+          line = item.respond_to?(:top_line) ? item.top_line : item.line
+          Location.hash(item.code_file.uri, line, 1)
+        end || []
+      end
+
+      # First pass: Walk up the parent scope chain, looking ONLY for variables (closest wins)
+      # Variables in scope chain take priority over child scopes and project-wide definitions
+      supplied_scope = parent_scopes.first
+
+      current = supplied_scope
+      while current
+        # For constants, skip method scopes and keep searching up to class/module scopes
+        if current.class_type == RubyLanguageServer::ScopeData::Scope::TYPE_METHOD
+          current = current.parent
+          next
+        end
+
+        # Check variables (constants) in the current scope
+        matching_variable = current.variables.where(name: name).reject { |item| item.code_file.nil? }.first
+        if matching_variable
+          line = matching_variable.respond_to?(:top_line) ? matching_variable.top_line : matching_variable.line
+          return [Location.hash(matching_variable.code_file.uri, line, 1)]
         end
+
+        # Move up to parent scope and continue searching for variables
+        current = current.parent
+      end
+
+      # Second pass: If no variables found, check child scopes in the scope chain
+      current = supplied_scope
+      while current
+        # For constants, skip method scopes and keep searching up to class/module scopes
+        if current.class_type == RubyLanguageServer::ScopeData::Scope::TYPE_METHOD
+          current = current.parent
+          next
+        end
+
+        # Check child scopes (classes/modules/methods) with this name (for class/module/method lookup)
+        child_scopes = current.children.where(name: name)
+        child_scopes = child_scopes.where(class_method: class_method_filter) unless class_method_filter.nil?
+        unless child_scopes.empty?
+          return child_scopes.reject { |item| item.code_file.nil? }.map do |item|
+            line = item.respond_to?(:top_line) ? item.top_line : item.line
+            Location.hash(item.code_file.uri, line, 1)
+          end
+        end
+
+        # Move up to parent scope and continue searching
+        current = current.parent
       end
 
-      # Return locations for all matching scopes and variables
-      results.reject { |item| item.code_file.nil? }.map do |item|
+      # If nothing found in the scope chain, fall back to project-wide search
+      all_scopes = RubyLanguageServer::ScopeData::Scope.where(name: name)
+      all_scopes = all_scopes.where(class_method: class_method_filter) unless class_method_filter.nil?
+      all_variables = RubyLanguageServer::ScopeData::Variable.where(name: name)
+      all_matches = all_scopes.to_a + all_variables.to_a
+      all_matches.reject { |item| item.code_file.nil? }.map do |item|
         line = item.respond_to?(:top_line) ? item.top_line : item.line
         Location.hash(item.code_file.uri, line, 1)
       end

spec/lib/ruby_language_server/project_manager_spec.rb

@@ -4,6 +4,72 @@
 require 'minitest/autorun'
 
 describe RubyLanguageServer::ProjectManager do
+  describe 'scope prioritization for duplicate names' do
+    let(:file_with_nested_classes) do
+      <<~CODE_FILE
+        class Outer
+          VALUE = 'outer'
+          class Inner
+            VALUE = 'inner'
+            def show
+              puts VALUE
+            end
+          end
+          def after_inner
+            puts VALUE
+          end
+        end
+      CODE_FILE
+    end
+
+    before(:each) do
+      project_manager.update_document_content('dup_uri', file_with_nested_classes)
+      project_manager.tags_for_uri('dup_uri')
+    end
+
+    it 'returns the closest VALUE constant in the innermost scope' do
+      # Dynamically find the line and character offset of 'VALUE' in the line containing 'puts VALUE'
+      code_lines = file_with_nested_classes.lines
+      value_line_num = code_lines.find_index { |l| l.include?('puts VALUE') }
+      raise "No line with 'puts VALUE' found" if value_line_num.nil?
+
+      value_line = code_lines[value_line_num]
+      value_char = value_line.index('VALUE')
+      raise "'VALUE' not found in line: #{value_line.inspect}" if value_char.nil?
+
+      position = OpenStruct.new(line: value_line_num, character: value_char)
+      results = project_manager.possible_definitions('dup_uri', position)
+      assert_equal 2, results.length
+      assert_equal 'dup_uri', results.first[:uri]
+      assert_equal 'dup_uri', results.last[:uri]
+      # Should find VALUE defined on line 3 (Inner::VALUE) first, then Outer::VALUE on line 1
+      assert_equal 3, results.first[:range][:start][:line]
+      assert_equal 1, results.last[:range][:start][:line]
+    end
+
+    it 'returns the outer VALUE constant when in the outer scope' do
+      # Dynamically find the line and character offset of 'VALUE' in the after_inner method
+      project_manager.update_document_content('dup_uri', file_with_nested_classes)
+      code_lines = file_with_nested_classes.lines
+      value_line_num = code_lines.find_index { |l| l.include?('puts VALUE') && l.include?('after_inner').! }
+      # Find the second occurrence of 'puts VALUE' (after_inner)
+      occurrences = code_lines.each_with_index.select { |l, _i| l.include?('puts VALUE') }
+      raise "No line with 'puts VALUE' found" if occurrences.empty?
+
+      # The second occurrence is for after_inner
+      value_line_num = occurrences[1][1] if occurrences.size > 1
+      value_line = code_lines[value_line_num]
+      value_char = value_line.index('VALUE')
+      raise "'VALUE' not found in line: #{value_line.inspect}" if value_char.nil?
+
+      position = OpenStruct.new(line: value_line_num, character: value_char)
+      results = project_manager.possible_definitions('dup_uri', position)
+      assert_equal 1, results.length
+      assert_equal 'dup_uri', results.first[:uri]
+      # Should find VALUE defined on line 1 (Outer::VALUE)
+      assert_equal 1, results.first[:range][:start][:line]
+    end
+  end
   let(:rails_file_text) do
     <<~CODE_FILE
       class Foo < ActiveRecord::Base