Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use shortest possible constant name for filtering completions #1074

Merged
merged 1 commit into from
Oct 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 29 additions & 9 deletions lib/ruby_lsp/requests/completion.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,13 @@ def on_constant_read(node)
candidates = @index.prefix_search(name, @nesting)
candidates.each do |entries|
complete_name = T.must(entries.first).name
@_response << build_entry_completion(complete_name, node, entries, top_level?(complete_name, candidates))
@_response << build_entry_completion(
complete_name,
name,
node,
entries,
top_level?(complete_name, candidates),
)
end
end

Expand Down Expand Up @@ -99,6 +105,7 @@ def on_constant_path(node)

@_response << build_entry_completion(
full_name,
name,
node,
entries,
top_level_reference || top_level?(T.must(entries.first).name, candidates),
Expand All @@ -122,13 +129,14 @@ def build_completion(label, node)

sig do
params(
name: String,
real_name: String,
incomplete_name: String,
node: Prism::Node,
entries: T::Array[RubyIndexer::Index::Entry],
top_level: T::Boolean,
).returns(Interface::CompletionItem)
end
def build_entry_completion(name, node, entries, top_level)
def build_entry_completion(real_name, incomplete_name, node, entries, top_level)
first_entry = T.must(entries.first)
kind = case first_entry
when RubyIndexer::Index::Entry::Class
Expand All @@ -141,14 +149,18 @@ def build_entry_completion(name, node, entries, top_level)
Constant::CompletionItemKind::REFERENCE
end

insertion_text = name.dup
insertion_text = real_name.dup
filter_text = real_name.dup

# If we have two entries with the same name inside the current namespace and the user selects the top level
# option, we have to ensure it's prefixed with `::` or else we're completing the wrong constant. For example:
# If we have the index with ["Foo::Bar", "Bar"], and we're providing suggestions for `B` inside a `Foo` module,
# then selecting the `Foo::Bar` option needs to complete to `Bar` and selecting the top level `Bar` option needs
# to complete to `::Bar`.
insertion_text.prepend("::") if top_level
if top_level
insertion_text.prepend("::")
filter_text.prepend("::")
end

# If the user is searching for a constant inside the current namespace, then we prefer completing the short name
# of that constant. E.g.:
Expand All @@ -159,14 +171,22 @@ def build_entry_completion(name, node, entries, top_level)
#
# Foo::B # --> completion inserts `Bar` instead of `Foo::Bar`
# end
@nesting.each { |namespace| insertion_text.delete_prefix!("#{namespace}::") }
@nesting.each do |namespace|
prefix = "#{namespace}::"
insertion_text.delete_prefix!(prefix)

# If the user is typing a fully qualified name `Foo::Bar::Baz`, then we should not use the short name (e.g.:
# `Baz`) as filtering. So we only shorten the filter text if the user is not including the namespaces in their
# typing
filter_text.delete_prefix!(prefix) unless incomplete_name.start_with?(prefix)
end

# When using a top level constant reference (e.g.: `::Bar`), the editor includes the `::` as part of the filter.
# For these top level references, we need to include the `::` as part of the filter text or else it won't match
# the right entries in the index
Interface::CompletionItem.new(
label: name,
filter_text: top_level ? "::#{name}" : name,
label: real_name,
filter_text: filter_text,
text_edit: Interface::TextEdit.new(
range: range_from_node(node),
new_text: insertion_text,
Expand All @@ -175,7 +195,7 @@ def build_entry_completion(name, node, entries, top_level)
label_details: Interface::CompletionItemLabelDetails.new(
description: entries.map(&:file_name).join(","),
),
documentation: markdown_from_index_entries(name, entries),
documentation: markdown_from_index_entries(real_name, entries),
)
end

Expand Down
48 changes: 41 additions & 7 deletions test/requests/completion_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ module Foo
params: { textDocument: { uri: @uri.to_s }, position: end_position },
)
assert_equal(["Foo::Bar", "Bar"], result.map(&:label))
assert_equal(["Foo::Bar", "::Bar"], result.map(&:filter_text))
assert_equal(["Bar", "::Bar"], result.map(&:filter_text))
assert_equal(["Bar", "::Bar"], result.map { |completion| completion.text_edit.new_text })

end_position = { line: 10, character: 6 }
Expand Down Expand Up @@ -333,7 +333,7 @@ class A
end

def test_completion_for_aliased_constants
RubyLsp::DependencyDetector.const_set(:HAS_TYPECHECKER, false)
stub_no_typechecker
document = RubyLsp::Document.new(source: +<<~RUBY, version: 1, uri: @uri)
module A
module B
Expand Down Expand Up @@ -362,12 +362,10 @@ module Other
assert_equal(["ALIAS_NAME::B::CONST"], result.map(&:label))
assert_equal(["ALIAS_NAME::B::CONST"], result.map(&:filter_text))
assert_equal(["ALIAS_NAME::B::CONST"], result.map { |completion| completion.text_edit.new_text })
ensure
RubyLsp::DependencyDetector.const_set(:HAS_TYPECHECKER, true)
end

def test_completion_for_aliased_complex_constants
RubyLsp::DependencyDetector.const_set(:HAS_TYPECHECKER, false)
stub_no_typechecker
document = RubyLsp::Document.new(source: +<<~RUBY, version: 1, uri: @uri)
module A
module B
Expand Down Expand Up @@ -397,8 +395,44 @@ module Other
assert_equal(["FINAL_ALIAS::ALIAS_NAME::B::CONST"], result.map(&:label))
assert_equal(["FINAL_ALIAS::ALIAS_NAME::B::CONST"], result.map(&:filter_text))
assert_equal(["FINAL_ALIAS::ALIAS_NAME::B::CONST"], result.map { |completion| completion.text_edit.new_text })
ensure
RubyLsp::DependencyDetector.const_set(:HAS_TYPECHECKER, true)
end

def test_completion_uses_shortest_possible_name_for_filter_text
stub_no_typechecker
document = RubyLsp::Document.new(source: +<<~RUBY, version: 1, uri: @uri)
module A
module B
class Foo
end

F
A::B::F
end
end
RUBY

@store.set(uri: @uri, source: document.source, version: 1)

index = @executor.instance_variable_get(:@index)
index.index_single(RubyIndexer::IndexablePath.new(nil, @uri.to_standardized_path), document.source)

result = run_request(
method: "textDocument/completion",
params: { textDocument: { uri: @uri.to_s }, position: { line: 5, character: 5 } },
)

assert_equal(["A::B::Foo"], result.map(&:label))
assert_equal(["Foo"], result.map(&:filter_text))
assert_equal(["Foo"], result.map { |completion| completion.text_edit.new_text })

result = run_request(
method: "textDocument/completion",
params: { textDocument: { uri: @uri.to_s }, position: { line: 6, character: 11 } },
)

assert_equal(["A::B::Foo"], result.map(&:label))
assert_equal(["A::B::Foo"], result.map(&:filter_text))
assert_equal(["Foo"], result.map { |completion| completion.text_edit.new_text })
end

private
Expand Down
Loading