Skip to content

Commit

Permalink
Use shortest possible constant name for filtering completions (#1074)
Browse files Browse the repository at this point in the history
  • Loading branch information
vinistock authored Oct 13, 2023
1 parent da6725d commit 87be915
Show file tree
Hide file tree
Showing 2 changed files with 70 additions and 16 deletions.
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

0 comments on commit 87be915

Please sign in to comment.