From a554660cc80da606be2abad3c03b0bf970b591a2 Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Fri, 21 Jun 2024 13:31:06 -0400 Subject: [PATCH] Avoid showing duplicate completions (#2215) * Avoid showing duplicate completions for overidden methods * PR Feedback --- lib/ruby_indexer/lib/ruby_indexer/entry.rb | 12 ++++-- lib/ruby_indexer/lib/ruby_indexer/index.rb | 43 +++++++++++++++++----- lib/ruby_indexer/test/index_test.rb | 32 ++++++++++++++++ 3 files changed, 74 insertions(+), 13 deletions(-) diff --git a/lib/ruby_indexer/lib/ruby_indexer/entry.rb b/lib/ruby_indexer/lib/ruby_indexer/entry.rb index 34c27e14b..6f4a8483a 100644 --- a/lib/ruby_indexer/lib/ruby_indexer/entry.rb +++ b/lib/ruby_indexer/lib/ruby_indexer/entry.rb @@ -483,6 +483,9 @@ class MethodAlias < Entry sig { returns(T.any(Member, MethodAlias)) } attr_reader :target + sig { returns(T.nilable(Entry::Namespace)) } + attr_reader :owner + sig { params(target: T.any(Member, MethodAlias), unresolved_alias: UnresolvedMethodAlias).void } def initialize(target, unresolved_alias) full_comments = ["Alias for #{target.name}\n"] @@ -498,12 +501,13 @@ def initialize(target, unresolved_alias) ) @target = target + @owner = T.let(unresolved_alias.owner, T.nilable(Entry::Namespace)) end - sig { returns(T.nilable(Entry::Namespace)) } - def owner - @target.owner - end + # sig { returns(T.nilable(Entry::Namespace)) } + # def owner + # @target.owner + # end sig { returns(T::Array[Parameter]) } def parameters diff --git a/lib/ruby_indexer/lib/ruby_indexer/index.rb b/lib/ruby_indexer/lib/ruby_indexer/index.rb index 0345a3baa..19e7abf75 100644 --- a/lib/ruby_indexer/lib/ruby_indexer/index.rb +++ b/lib/ruby_indexer/lib/ruby_indexer/index.rb @@ -150,17 +150,42 @@ def method_completion_candidates(name, receiver_name) ancestors = linearized_ancestors_of(receiver_name) candidates = name ? prefix_search(name).flatten : @entries.values.flatten - candidates.filter_map do |entry| - case entry - when Entry::Member, Entry::MethodAlias - entry if ancestors.any?(entry.owner&.name) - when Entry::UnresolvedMethodAlias - if ancestors.any?(entry.owner&.name) - resolved_alias = resolve_method_alias(entry, receiver_name) - resolved_alias if resolved_alias.is_a?(Entry::MethodAlias) - end + completion_items = candidates.each_with_object({}) do |entry, hash| + unless entry.is_a?(Entry::Member) || entry.is_a?(Entry::MethodAlias) || + entry.is_a?(Entry::UnresolvedMethodAlias) + next + end + + entry_name = entry.name + ancestor_index = ancestors.index(entry.owner&.name) + existing_entry, existing_entry_index = hash[entry_name] + + # Conditions for matching a method completion candidate: + # 1. If an ancestor_index was found, it means that this method is owned by the receiver. The exact index is + # where in the ancestor chain the method was found. For example, if the ancestors are ["A", "B", "C"] and we + # found the method declared in `B`, then the ancestors index is 1 + # + # 2. We already established that this method is owned by the receiver. Now, check if we already added a + # completion candidate for this method name. If not, then we just go and add it (the left hand side of the or) + # + # 3. If we had already found a method entry for the same name, then we need to check if the current entry that + # we are comparing appears first in the hierarchy or not. For example, imagine we have the method `open` defined + # in both `File` and its parent `IO`. If we first find the method `open` in `IO`, it will be inserted into the + # hash. Then, when we find the entry for `open` owned by `File`, we need to replace `IO.open` by `File.open`, + # since `File.open` appears first in the hierarchy chain and is therefore the correct method being invoked. The + # last part of the conditional checks if the current entry was found earlier in the hierarchy chain, in which + # case we must update the existing entry to avoid showing the wrong method declaration for overridden methods + next unless ancestor_index && (!existing_entry || ancestor_index < existing_entry_index) + + if entry.is_a?(Entry::UnresolvedMethodAlias) + resolved_alias = resolve_method_alias(entry, receiver_name) + hash[entry_name] = [resolved_alias, ancestor_index] if resolved_alias.is_a?(Entry::MethodAlias) + else + hash[entry_name] = [entry, ancestor_index] end end + + completion_items.values.map!(&:first) end # Resolve a constant to its declaration based on its name and the nesting where the reference was found. Parameter diff --git a/lib/ruby_indexer/test/index_test.rb b/lib/ruby_indexer/test/index_test.rb index ed4b1166d..28b96e66f 100644 --- a/lib/ruby_indexer/test/index_test.rb +++ b/lib/ruby_indexer/test/index_test.rb @@ -1509,5 +1509,37 @@ class Foo assert_empty(@index.method_completion_candidates("bar", "Foo")) end + + def test_completion_does_not_duplicate_overridden_methods + index(<<~RUBY) + class Foo + def bar; end + end + + class Baz < Foo + def bar; end + end + RUBY + + entries = @index.method_completion_candidates("bar", "Baz") + assert_equal(["bar"], entries.map(&:name)) + assert_equal("Baz", T.must(entries.first.owner).name) + end + + def test_completion_does_not_duplicate_methods_overridden_by_aliases + index(<<~RUBY) + class Foo + def bar; end + end + + class Baz < Foo + alias bar to_s + end + RUBY + + entries = @index.method_completion_candidates("bar", "Baz") + assert_equal(["bar"], entries.map(&:name)) + assert_equal("Baz", T.must(entries.first.owner).name) + end end end