From 46b0411eabe1dbfc37bc14bc7dcbd1dde5b18850 Mon Sep 17 00:00:00 2001 From: Andy Waite Date: Fri, 4 Oct 2024 11:04:15 -0400 Subject: [PATCH] Add method support to References request (#2650) * Add find references support for methods * Format on one line * Delete commented-out code * Explain matching * Fix tests * Count matches in test * Add test for matching writers * PR feedback * Mark Target as abstract * More tests --- .../lib/ruby_indexer/reference_finder.rb | 58 +++++- .../test/reference_finder_test.rb | 166 +++++++++++++++++- lib/ruby_lsp/requests/references.rb | 63 +++++-- lib/ruby_lsp/requests/rename.rb | 22 ++- 4 files changed, 276 insertions(+), 33 deletions(-) diff --git a/lib/ruby_indexer/lib/ruby_indexer/reference_finder.rb b/lib/ruby_indexer/lib/ruby_indexer/reference_finder.rb index 72dc9be9c..956b166b2 100644 --- a/lib/ruby_indexer/lib/ruby_indexer/reference_finder.rb +++ b/lib/ruby_indexer/lib/ruby_indexer/reference_finder.rb @@ -5,6 +5,38 @@ module RubyIndexer class ReferenceFinder extend T::Sig + class Target + extend T::Helpers + + abstract! + end + + class ConstTarget < Target + extend T::Sig + + sig { returns(String) } + attr_reader :fully_qualified_name + + sig { params(fully_qualified_name: String).void } + def initialize(fully_qualified_name) + super() + @fully_qualified_name = fully_qualified_name + end + end + + class MethodTarget < Target + extend T::Sig + + sig { returns(String) } + attr_reader :method_name + + sig { params(method_name: String).void } + def initialize(method_name) + super() + @method_name = method_name + end + end + class Reference extend T::Sig @@ -27,14 +59,14 @@ def initialize(name, location, declaration:) sig do params( - fully_qualified_name: String, + target: Target, index: RubyIndexer::Index, dispatcher: Prism::Dispatcher, include_declarations: T::Boolean, ).void end - def initialize(fully_qualified_name, index, dispatcher, include_declarations: true) - @fully_qualified_name = fully_qualified_name + def initialize(target, index, dispatcher, include_declarations: true) + @target = target @index = index @include_declarations = include_declarations @stack = T.let([], T::Array[String]) @@ -62,6 +94,7 @@ def initialize(fully_qualified_name, index, dispatcher, include_declarations: tr :on_constant_or_write_node_enter, :on_constant_and_write_node_enter, :on_constant_operator_write_node_enter, + :on_call_node_enter, ) end @@ -78,7 +111,7 @@ def on_class_node_enter(node) name = constant_path.slice nesting = actual_nesting(name) - if nesting.join("::") == @fully_qualified_name + if @target.is_a?(ConstTarget) && nesting.join("::") == @target.fully_qualified_name @references << Reference.new(name, constant_path.location, declaration: true) end @@ -96,7 +129,7 @@ def on_module_node_enter(node) name = constant_path.slice nesting = actual_nesting(name) - if nesting.join("::") == @fully_qualified_name + if @target.is_a?(ConstTarget) && nesting.join("::") == @target.fully_qualified_name @references << Reference.new(name, constant_path.location, declaration: true) end @@ -213,6 +246,10 @@ def on_constant_operator_write_node_enter(node) sig { params(node: Prism::DefNode).void } def on_def_node_enter(node) + if @target.is_a?(MethodTarget) && (name = node.name.to_s) == @target.method_name + @references << Reference.new(name, node.name_loc, declaration: true) + end + if node.receiver.is_a?(Prism::SelfNode) @stack << "" end @@ -225,6 +262,13 @@ def on_def_node_leave(node) end end + sig { params(node: Prism::CallNode).void } + def on_call_node_enter(node) + if @target.is_a?(MethodTarget) && (name = node.name.to_s) == @target.method_name + @references << Reference.new(name, T.must(node.message_loc), declaration: false) + end + end + private sig { params(name: String).returns(T::Array[String]) } @@ -243,13 +287,15 @@ def actual_nesting(name) sig { params(name: String, location: Prism::Location).void } def collect_constant_references(name, location) + return unless @target.is_a?(ConstTarget) + entries = @index.resolve(name, @stack) return unless entries previous_reference = @references.last entries.each do |entry| - next unless entry.name == @fully_qualified_name + next unless entry.name == @target.fully_qualified_name # When processing a class/module declaration, we eagerly handle the constant reference. To avoid duplicates, # when we find the constant node defining the namespace, then we have to check if it wasn't already added diff --git a/lib/ruby_indexer/test/reference_finder_test.rb b/lib/ruby_indexer/test/reference_finder_test.rb index d1f9c190a..0d4627a8f 100644 --- a/lib/ruby_indexer/test/reference_finder_test.rb +++ b/lib/ruby_indexer/test/reference_finder_test.rb @@ -6,7 +6,7 @@ module RubyIndexer class ReferenceFinderTest < Minitest::Test def test_finds_constant_references - refs = find_references("Foo::Bar", <<~RUBY) + refs = find_const_references("Foo::Bar", <<~RUBY) module Foo class Bar end @@ -28,7 +28,7 @@ class Bar end def test_finds_constant_references_inside_singleton_contexts - refs = find_references("Foo::::Bar", <<~RUBY) + refs = find_const_references("Foo::::Bar", <<~RUBY) class Foo class << self class Bar @@ -47,7 +47,7 @@ class Bar end def test_finds_top_level_constant_references - refs = find_references("Bar", <<~RUBY) + refs = find_const_references("Bar", <<~RUBY) class Bar end @@ -70,15 +70,171 @@ class << self assert_equal(8, refs[2].location.start_line) end + def test_finds_method_references + refs = find_method_references("foo", <<~RUBY) + class Bar + def foo + end + + def baz + foo + end + end + RUBY + + assert_equal(2, refs.size) + + assert_equal("foo", refs[0].name) + assert_equal(2, refs[0].location.start_line) + + assert_equal("foo", refs[1].name) + assert_equal(6, refs[1].location.start_line) + end + + def test_does_not_mismatch_on_readers_and_writers + refs = find_method_references("foo", <<~RUBY) + class Bar + def foo + end + + def foo=(value) + end + + def baz + self.foo = 1 + self.foo + end + end + RUBY + + # We want to match `foo` but not `foo=` + assert_equal(2, refs.size) + + assert_equal("foo", refs[0].name) + assert_equal(2, refs[0].location.start_line) + + assert_equal("foo", refs[1].name) + assert_equal(10, refs[1].location.start_line) + end + + def test_matches_writers + refs = find_method_references("foo=", <<~RUBY) + class Bar + def foo + end + + def foo=(value) + end + + def baz + self.foo = 1 + self.foo + end + end + RUBY + + # We want to match `foo=` but not `foo` + assert_equal(2, refs.size) + + assert_equal("foo=", refs[0].name) + assert_equal(5, refs[0].location.start_line) + + assert_equal("foo=", refs[1].name) + assert_equal(9, refs[1].location.start_line) + end + + def test_find_inherited_methods + refs = find_method_references("foo", <<~RUBY) + class Bar + def foo + end + end + + class Baz < Bar + super.foo + end + RUBY + + assert_equal(2, refs.size) + + assert_equal("foo", refs[0].name) + assert_equal(2, refs[0].location.start_line) + + assert_equal("foo", refs[1].name) + assert_equal(7, refs[1].location.start_line) + end + + def test_finds_methods_created_in_mixins + refs = find_method_references("foo", <<~RUBY) + module Mixin + def foo + end + end + + class Bar + include Mixin + end + + Bar.foo + RUBY + + assert_equal(2, refs.size) + + assert_equal("foo", refs[0].name) + assert_equal(2, refs[0].location.start_line) + + assert_equal("foo", refs[1].name) + assert_equal(10, refs[1].location.start_line) + end + + def test_finds_singleton_methods + # The current implementation matches on both `Bar.foo` and `Bar#foo` even though they are different + + refs = find_method_references("foo", <<~RUBY) + class Bar + class << self + def foo + end + end + + def foo + end + end + + Bar.foo + RUBY + + assert_equal(3, refs.size) + + assert_equal("foo", refs[0].name) + assert_equal(3, refs[0].location.start_line) + + assert_equal("foo", refs[1].name) + assert_equal(7, refs[1].location.start_line) + + assert_equal("foo", refs[2].name) + assert_equal(11, refs[2].location.start_line) + end + private - def find_references(fully_qualified_name, source) + def find_const_references(const_name, source) + target = ReferenceFinder::ConstTarget.new(const_name) + find_references(target, source) + end + + def find_method_references(method_name, source) + target = ReferenceFinder::MethodTarget.new(method_name) + find_references(target, source) + end + + def find_references(target, source) file_path = "/fake.rb" index = Index.new index.index_single(IndexablePath.new(nil, file_path), source) parse_result = Prism.parse(source) dispatcher = Prism::Dispatcher.new - finder = ReferenceFinder.new(fully_qualified_name, index, dispatcher) + finder = ReferenceFinder.new(target, index, dispatcher) dispatcher.visit(parse_result.value) finder.references end diff --git a/lib/ruby_lsp/requests/references.rb b/lib/ruby_lsp/requests/references.rb index 89ce56fd5..cc4198bee 100644 --- a/lib/ruby_lsp/requests/references.rb +++ b/lib/ruby_lsp/requests/references.rb @@ -35,7 +35,13 @@ def perform node_context = RubyDocument.locate( @document.parse_result.value, char_position, - node_types: [Prism::ConstantReadNode, Prism::ConstantPathNode, Prism::ConstantPathTargetNode], + node_types: [ + Prism::ConstantReadNode, + Prism::ConstantPathNode, + Prism::ConstantPathTargetNode, + Prism::CallNode, + Prism::DefNode, + ], ) target = node_context.node parent = node_context.parent @@ -51,16 +57,17 @@ def perform target = T.cast( target, - T.any(Prism::ConstantReadNode, Prism::ConstantPathNode, Prism::ConstantPathTargetNode), + T.any( + Prism::ConstantReadNode, + Prism::ConstantPathNode, + Prism::ConstantPathTargetNode, + Prism::CallNode, + Prism::DefNode, + ), ) - name = constant_name(target) - return @locations unless name - - entries = @global_state.index.resolve(name, node_context.nesting) - return @locations unless entries - - fully_qualified_name = T.must(entries.first).name + reference_target = create_reference_target(target, node_context) + return @locations unless reference_target Dir.glob(File.join(@global_state.workspace_path, "**/*.rb")).each do |path| uri = URI::Generic.from_path(path: path) @@ -69,11 +76,11 @@ def perform next if @store.key?(uri) parse_result = Prism.parse_file(path) - collect_references(fully_qualified_name, parse_result, uri) + collect_references(reference_target, parse_result, uri) end @store.each do |_uri, document| - collect_references(fully_qualified_name, document.parse_result, document.uri) + collect_references(reference_target, document.parse_result, document.uri) end @locations @@ -83,15 +90,43 @@ def perform sig do params( - fully_qualified_name: String, + target_node: T.any( + Prism::ConstantReadNode, + Prism::ConstantPathNode, + Prism::ConstantPathTargetNode, + Prism::CallNode, + Prism::DefNode, + ), + node_context: NodeContext, + ).returns(T.nilable(RubyIndexer::ReferenceFinder::Target)) + end + def create_reference_target(target_node, node_context) + case target_node + when Prism::ConstantReadNode, Prism::ConstantPathNode, Prism::ConstantPathTargetNode + name = constant_name(target_node) + return unless name + + entries = @global_state.index.resolve(name, node_context.nesting) + return unless entries + + fully_qualified_name = T.must(entries.first).name + RubyIndexer::ReferenceFinder::ConstTarget.new(fully_qualified_name) + when Prism::CallNode, Prism::DefNode + RubyIndexer::ReferenceFinder::MethodTarget.new(target_node.name.to_s) + end + end + + sig do + params( + target: RubyIndexer::ReferenceFinder::Target, parse_result: Prism::ParseResult, uri: URI::Generic, ).void end - def collect_references(fully_qualified_name, parse_result, uri) + def collect_references(target, parse_result, uri) dispatcher = Prism::Dispatcher.new finder = RubyIndexer::ReferenceFinder.new( - fully_qualified_name, + target, @global_state.index, dispatcher, include_declarations: @params.dig(:context, :includeDeclaration) || true, diff --git a/lib/ruby_lsp/requests/rename.rb b/lib/ruby_lsp/requests/rename.rb index c33bc8922..58ac63759 100644 --- a/lib/ruby_lsp/requests/rename.rb +++ b/lib/ruby_lsp/requests/rename.rb @@ -66,7 +66,8 @@ def perform end fully_qualified_name = T.must(entries.first).name - changes = collect_text_edits(fully_qualified_name, name) + reference_target = RubyIndexer::ReferenceFinder::ConstTarget.new(fully_qualified_name) + changes = collect_text_edits(reference_target, name) # If the client doesn't support resource operations, such as renaming files, then we can only return the basic # text changes @@ -127,8 +128,13 @@ def collect_file_renames(fully_qualified_name, document_changes) end end - sig { params(fully_qualified_name: String, name: String).returns(T::Hash[String, T::Array[Interface::TextEdit]]) } - def collect_text_edits(fully_qualified_name, name) + sig do + params( + target: RubyIndexer::ReferenceFinder::Target, + name: String, + ).returns(T::Hash[String, T::Array[Interface::TextEdit]]) + end + def collect_text_edits(target, name) changes = {} Dir.glob(File.join(@global_state.workspace_path, "**/*.rb")).each do |path| @@ -138,12 +144,12 @@ def collect_text_edits(fully_qualified_name, name) next if @store.key?(uri) parse_result = Prism.parse_file(path) - edits = collect_changes(fully_qualified_name, parse_result, name, uri) + edits = collect_changes(target, parse_result, name, uri) changes[uri.to_s] = edits unless edits.empty? end @store.each do |uri, document| - edits = collect_changes(fully_qualified_name, document.parse_result, name, document.uri) + edits = collect_changes(target, document.parse_result, name, document.uri) changes[uri] = edits unless edits.empty? end @@ -152,15 +158,15 @@ def collect_text_edits(fully_qualified_name, name) sig do params( - fully_qualified_name: String, + target: RubyIndexer::ReferenceFinder::Target, parse_result: Prism::ParseResult, name: String, uri: URI::Generic, ).returns(T::Array[Interface::TextEdit]) end - def collect_changes(fully_qualified_name, parse_result, name, uri) + def collect_changes(target, parse_result, name, uri) dispatcher = Prism::Dispatcher.new - finder = RubyIndexer::ReferenceFinder.new(fully_qualified_name, @global_state.index, dispatcher) + finder = RubyIndexer::ReferenceFinder.new(target, @global_state.index, dispatcher) dispatcher.visit(parse_result.value) finder.references.map do |reference|