Add definition for all missing global variable nodes (#2757)

* feat: handle all kind of global variable nodes in definition request * feat: apply target correction in case of position not exactly on target * test: lines document start at 0 * fix: apply target correction on instance variable * chore: remove callNode target correction since StringNode and SymbolNode are now well targetted
Shopify · Oct 23, 2024 · 6803883 · 6803883
1 parent e0299e5
commit 6803883
Show file tree

Hide file tree

Showing 4 changed files with 134 additions and 25 deletions.
diff --git a/lib/ruby_lsp/listeners/definition.rb b/lib/ruby_lsp/listeners/definition.rb
@@ -39,7 +39,12 @@ def initialize(response_builder, global_state, language_id, uri, node_context, d
           :on_block_argument_node_enter,
           :on_constant_read_node_enter,
           :on_constant_path_node_enter,
+          :on_global_variable_and_write_node_enter,
+          :on_global_variable_operator_write_node_enter,
+          :on_global_variable_or_write_node_enter,
           :on_global_variable_read_node_enter,
+          :on_global_variable_target_node_enter,
+          :on_global_variable_write_node_enter,
           :on_instance_variable_read_node_enter,
           :on_instance_variable_write_node_enter,
           :on_instance_variable_and_write_node_enter,
@@ -121,23 +126,34 @@ def on_constant_read_node_enter(node)
         find_in_index(name)
       end
 
+      sig { params(node: Prism::GlobalVariableAndWriteNode).void }
+      def on_global_variable_and_write_node_enter(node)
+        handle_global_variable_definition(node.name.to_s)
+      end
+
+      sig { params(node: Prism::GlobalVariableOperatorWriteNode).void }
+      def on_global_variable_operator_write_node_enter(node)
+        handle_global_variable_definition(node.name.to_s)
+      end
+
+      sig { params(node: Prism::GlobalVariableOrWriteNode).void }
+      def on_global_variable_or_write_node_enter(node)
+        handle_global_variable_definition(node.name.to_s)
+      end
+
       sig { params(node: Prism::GlobalVariableReadNode).void }
       def on_global_variable_read_node_enter(node)
-        entries = @index[node.name.to_s]
-
-        return unless entries
+        handle_global_variable_definition(node.name.to_s)
+      end
 
-        entries.each do |entry|
-          location = entry.location
+      sig { params(node: Prism::GlobalVariableTargetNode).void }
+      def on_global_variable_target_node_enter(node)
+        handle_global_variable_definition(node.name.to_s)
+      end
 
-          @response_builder << Interface::Location.new(
-            uri: URI::Generic.from_path(path: entry.file_path).to_s,
-            range: Interface::Range.new(
-              start: Interface::Position.new(line: location.start_line - 1, character: location.start_column),
-              end: Interface::Position.new(line: location.end_line - 1, character: location.end_column),
-            ),
-          )
-        end
+      sig { params(node: Prism::GlobalVariableWriteNode).void }
+      def on_global_variable_write_node_enter(node)
+        handle_global_variable_definition(node.name.to_s)
       end
 
       sig { params(node: Prism::InstanceVariableReadNode).void }
@@ -197,6 +213,25 @@ def handle_super_node_definition
         )
       end
 
+      sig { params(name: String).void }
+      def handle_global_variable_definition(name)
+        entries = @index[name]
+
+        return unless entries
+
+        entries.each do |entry|
+          location = entry.location
+
+          @response_builder << Interface::Location.new(
+            uri: URI::Generic.from_path(path: entry.file_path).to_s,
+            range: Interface::Range.new(
+              start: Interface::Position.new(line: location.start_line - 1, character: location.start_column),
+              end: Interface::Position.new(line: location.end_line - 1, character: location.end_column),
+            ),
+          )
+        end
+      end
+
       sig { params(name: String).void }
       def handle_instance_variable_definition(name)
         # Sorbet enforces that all instance variables be declared on typed strict or higher, which means it will be able

diff --git a/lib/ruby_lsp/requests/definition.rb b/lib/ruby_lsp/requests/definition.rb
@@ -12,12 +12,6 @@ class Definition < Request
       extend T::Sig
       extend T::Generic
 
-      SPECIAL_METHOD_CALLS = [
-        :require,
-        :require_relative,
-        :autoload,
-      ].freeze
-
       sig do
         params(
           document: T.any(RubyDocument, ERBDocument),
@@ -46,7 +40,12 @@ def initialize(document, global_state, position, dispatcher, sorbet_level)
             Prism::ConstantReadNode,
             Prism::ConstantPathNode,
             Prism::BlockArgumentNode,
+            Prism::GlobalVariableAndWriteNode,
+            Prism::GlobalVariableOperatorWriteNode,
+            Prism::GlobalVariableOrWriteNode,
             Prism::GlobalVariableReadNode,
+            Prism::GlobalVariableTargetNode,
+            Prism::GlobalVariableWriteNode,
             Prism::InstanceVariableReadNode,
             Prism::InstanceVariableAndWriteNode,
             Prism::InstanceVariableOperatorWriteNode,
@@ -72,11 +71,7 @@ def initialize(document, global_state, position, dispatcher, sorbet_level)
             parent,
             position,
           )
-        elsif target.is_a?(Prism::CallNode) && !SPECIAL_METHOD_CALLS.include?(target.message) && !covers_position?(
-          target.message_loc, position
-        )
-          # If the target is a method call, we need to ensure that the requested position is exactly on top of the
-          # method identifier. Otherwise, we risk showing definitions for unrelated things
+        elsif position_outside_target?(position, target)
           target = nil
         # For methods with block arguments using symbol-to-proc
         elsif target.is_a?(Prism::SymbolNode) && parent.is_a?(Prism::BlockArgumentNode)
@@ -107,6 +102,26 @@ def perform
         @dispatcher.dispatch_once(@target) if @target
         @response_builder.response
       end
+
+      private
+
+      sig { params(position: T::Hash[Symbol, T.untyped], target: T.nilable(Prism::Node)).returns(T::Boolean) }
+      def position_outside_target?(position, target)
+        case target
+        when Prism::GlobalVariableAndWriteNode,
+          Prism::GlobalVariableOperatorWriteNode,
+          Prism::GlobalVariableOrWriteNode,
+          Prism::GlobalVariableWriteNode,
+          Prism::InstanceVariableAndWriteNode,
+          Prism::InstanceVariableOperatorWriteNode,
+          Prism::InstanceVariableOrWriteNode,
+          Prism::InstanceVariableWriteNode
+
+          !covers_position?(target.name_loc, position)
+        else
+          false
+        end
+      end
     end
   end
 end
diff --git a/test/requests/definition_expectations_test.rb b/test/requests/definition_expectations_test.rb
@@ -673,6 +673,11 @@ def baz
 
   def test_definition_for_global_variables
     source = <<~RUBY
+      $bar &&= 1
+      $bar += 1
+      $foo ||= 1
+      $bar, $foo = 1
+      $foo = 1
       $DEBUG
     RUBY
 
@@ -685,13 +690,67 @@ def test_definition_for_global_variables
         method: "textDocument/definition",
         params: { textDocument: { uri: uri }, position: { character: 1, line: 0 } },
       )
+
+      response = server.pop_response.response
+      assert_equal(3, response.size)
+      assert_equal(0, response[0].range.start.line)
+      assert_equal(1, response[1].range.start.line)
+      assert_equal(3, response[2].range.start.line)
+
+      server.process_message(
+        id: 1,
+        method: "textDocument/definition",
+        params: { textDocument: { uri: uri }, position: { character: 1, line: 2 } },
+      )
+
+      response = server.pop_response.response
+      assert_equal(3, response.size)
+      assert_equal(2, response[0].range.start.line)
+      assert_equal(3, response[1].range.start.line)
+      assert_equal(4, response[2].range.start.line)
+
+      server.process_message(
+        id: 1,
+        method: "textDocument/definition",
+        params: { textDocument: { uri: uri }, position: { character: 1, line: 5 } },
+      )
+
       response = server.pop_response.response.first
       assert_match(%r{/gems/rbs-.*/core/global_variables.rbs}, response.uri)
       assert_equal(response.range.start.line, response.range.end.line)
       assert_operator(response.range.start.character, :<, response.range.end.character)
     end
   end
 
+  def test_definition_apply_target_correction
+    source = <<~RUBY
+      $foo &&= 1
+      $foo += 1
+      $foo ||= 1
+      $foo = 1
+      class Foo
+        @foo &&= 1
+        @foo += 1
+        @foo ||= 1
+        @foo = 1
+      end
+    RUBY
+
+    lines_with_target_correction = [0, 1, 2, 3, 5, 6, 7, 8]
+
+    with_server(source) do |server, uri|
+      lines_with_target_correction.each do |line|
+        server.process_message(
+          id: 1,
+          method: "textDocument/definition",
+          params: { textDocument: { uri: uri }, position: { character: 7, line: line } },
+        )
+
+        assert_empty(server.pop_response.response)
+      end
+    end
+  end
+
   def test_definition_for_instance_variables
     source = <<~RUBY
       class Foo

diff --git a/test/requests/hover_expectations_test.rb b/test/requests/hover_expectations_test.rb
@@ -109,7 +109,7 @@ def test_hover_apply_target_correction
       $foo = 1
     RUBY
 
-    lines_with_target_correction = [1, 2, 3, 4]
+    lines_with_target_correction = [0, 1, 2, 3]
 
     with_server(source) do |server, uri|
       lines_with_target_correction.each do |line|