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.

Sign up for GitHub

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

Account for private constants in features #1004

Merged
merged 4 commits into from
Sep 20, 2023
Merged

Account for private constants in features #1004

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
18866ca
Account for private constants when jumping to definition
vinistock Sep 14, 2023
3e285f2
Account for private constants when providing completion
vinistock Sep 14, 2023
57aa59a
Account for private constants when showing hover
vinistock Sep 14, 2023
fad6231
Account for private constants in workspace symbol
vinistock Sep 14, 2023
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
5 changes: 5 additions & 0 deletions lib/ruby_lsp/requests/completion.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,11 @@ def on_constant_path(node)

candidates = @index.prefix_search(name, top_level_reference ? [] : @nesting)
candidates.each do |entries|
# The only time we may have a private constant reference from outside of the namespace is if we're dealing
# with ConstantPath and the entry name doesn't start with the current nesting
first_entry = T.must(entries.first)
next if first_entry.visibility == :private && !first_entry.name.start_with?("#{@nesting}::")

@_response << build_entry_completion(
name,
node,
Expand Down
5 changes: 5 additions & 0 deletions lib/ruby_lsp/requests/definition.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,11 @@ def find_in_index(value)
entries = @index.resolve(value, @nesting)
return unless entries

# We should only allow jumping to the definition of private constants if the constant is defined in the same
# namespace as the reference
first_entry = T.must(entries.first)
return if first_entry.visibility == :private && first_entry.name != "#{@nesting.join("::")}::#{value}"

bundle_path = begin
Bundler.bundle_path.to_s
rescue Bundler::GemfileNotFound
Expand Down
5 changes: 5 additions & 0 deletions lib/ruby_lsp/requests/hover.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,11 @@ def generate_hover(name, location)
entries = @index.resolve(name, @nesting)
return unless entries

# We should only show hover for private constants if the constant is defined in the same namespace as the
# reference
first_entry = T.must(entries.first)
return if first_entry.visibility == :private && first_entry.name != "#{@nesting.join("::")}::#{name}"

@_response = Interface::Hover.new(
range: range_from_location(location),
contents: markdown_from_index_entries(name, entries),
Expand Down
3 changes: 3 additions & 0 deletions lib/ruby_lsp/requests/workspace_symbol.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ def run
next
end

# We should never show private symbols when searching the entire workspace
next if entry.visibility == :private

kind = kind_for_entry(entry)
loc = entry.location

Expand Down
52 changes: 52 additions & 0 deletions test/requests/completion_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,58 @@ module Foo
RubyLsp::DependencyDetector.const_set(:HAS_TYPECHECKER, true)
end

def test_completion_private_constants_inside_the_same_namespace
RubyLsp::DependencyDetector.const_set(:HAS_TYPECHECKER, false)
document = RubyLsp::Document.new(source: +<<~RUBY, version: 1, uri: @uri)
class A
CONST = 1
private_constant(:CONST)

C
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)

end_position = { line: 3, character: 4 }
result = run_request(
method: "textDocument/completion",
params: { textDocument: { uri: @uri.to_s }, position: end_position },
)
assert_equal(["CONST"], result.map { |completion| completion.text_edit.new_text })
ensure
RubyLsp::DependencyDetector.const_set(:HAS_TYPECHECKER, true)
end

def test_completion_private_constants_from_different_namespace
RubyLsp::DependencyDetector.const_set(:HAS_TYPECHECKER, false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in this PR, but since we use this a lot I wonder we should extract it out, e.g.:

def with_typechecker_disabled(&block)
  RubyLsp::DependencyDetector.const_set(:HAS_TYPECHECKER, false)
  yield
ensure
  RubyLsp::DependencyDetector.const_set(:HAS_TYPECHECKER, true)
end

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true. We can extract it in another PR.

document = RubyLsp::Document.new(source: +<<~RUBY, version: 1, uri: @uri)
class A
CONST = 1
private_constant(:CONST)
end

A::C
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)

end_position = { line: 4, character: 5 }
result = run_request(
method: "textDocument/completion",
params: { textDocument: { uri: @uri.to_s }, position: end_position },
)
assert_empty(result)
ensure
RubyLsp::DependencyDetector.const_set(:HAS_TYPECHECKER, true)
end

private

def run_request(method:, params: {})
Expand Down
74 changes: 74 additions & 0 deletions test/requests/definition_expectations_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,80 @@ def test_jumping_to_default_require_of_a_gem
T.must(message_queue).close
end

def test_jumping_to_private_constant_inside_the_same_namespace
message_queue = Thread::Queue.new
store = RubyLsp::Store.new

uri = URI("file:///folder/fake.rb")
source = <<~RUBY
class A
CONST = 123
private_constant(:CONST)

CONST
end
RUBY

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

executor = RubyLsp::Executor.new(store, message_queue)

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

begin
RubyLsp::DependencyDetector.const_set(:HAS_TYPECHECKER, false)
response = executor.execute({
method: "textDocument/definition",
params: { textDocument: { uri: "file:///folder/fake.rb" }, position: { character: 2, line: 4 } },
}).response
ensure
RubyLsp::DependencyDetector.const_set(:HAS_TYPECHECKER, true)
end

assert_equal(uri.to_s, response.first.attributes[:uri])
ensure
T.must(message_queue).close
end

def test_jumping_to_private_constant_from_different_namespace
message_queue = Thread::Queue.new
store = RubyLsp::Store.new

uri = URI("file:///folder/fake.rb")
source = <<~RUBY
class A
CONST = 123
private_constant(:CONST)
end

A::CONST # invalid private reference
RUBY

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

executor = RubyLsp::Executor.new(store, message_queue)

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

begin
RubyLsp::DependencyDetector.const_set(:HAS_TYPECHECKER, false)
response = executor.execute({
method: "textDocument/definition",
params: { textDocument: { uri: "file:///folder/fake.rb" }, position: { character: 0, line: 5 } },
}).response
ensure
RubyLsp::DependencyDetector.const_set(:HAS_TYPECHECKER, true)
end

assert_nil(response)
ensure
T.must(message_queue).close
end

def test_definition_extensions
source = <<~RUBY
RubyLsp
Expand Down
62 changes: 62 additions & 0 deletions test/requests/hover_expectations_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,68 @@ def run_expectations(source)
end
end

def test_hovering_over_private_constant_from_the_same_namespace
message_queue = Thread::Queue.new
store = RubyLsp::Store.new

uri = URI("file:///fake.rb")
source = <<~RUBY
class A
CONST = 123
private_constant(:CONST)

CONST
end
RUBY
store.set(uri: uri, source: source, version: 1)

executor = RubyLsp::Executor.new(store, message_queue)
index = executor.instance_variable_get(:@index)
index.index_single(RubyIndexer::IndexablePath.new(nil, T.must(uri.to_standardized_path)), source)

RubyLsp::DependencyDetector.const_set(:HAS_TYPECHECKER, false)
response = executor.execute({
method: "textDocument/hover",
params: { textDocument: { uri: uri }, position: { character: 2, line: 4 } },
}).response

assert_match("CONST", response.contents.value)
ensure
RubyLsp::DependencyDetector.const_set(:HAS_TYPECHECKER, true)
T.must(message_queue).close
end

def test_hovering_over_private_constant_from_different_namespace
message_queue = Thread::Queue.new
store = RubyLsp::Store.new

uri = URI("file:///fake.rb")
source = <<~RUBY
class A
CONST = 123
private_constant(:CONST)
end

A::CONST # invalid private reference
RUBY
store.set(uri: uri, source: source, version: 1)

executor = RubyLsp::Executor.new(store, message_queue)
index = executor.instance_variable_get(:@index)
index.index_single(RubyIndexer::IndexablePath.new(nil, T.must(uri.to_standardized_path)), source)

RubyLsp::DependencyDetector.const_set(:HAS_TYPECHECKER, false)
response = executor.execute({
method: "textDocument/hover",
params: { textDocument: { uri: uri }, position: { character: 0, line: 5 } },
}).response

assert_nil(response)
ensure
RubyLsp::DependencyDetector.const_set(:HAS_TYPECHECKER, true)
T.must(message_queue).close
end

def test_hover_extensions
source = <<~RUBY
# Hello
Expand Down
13 changes: 13 additions & 0 deletions test/requests/workspace_symbol_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,4 +95,17 @@ def test_finds_default_gem_symbols
result = RubyLsp::Requests::WorkspaceSymbol.new("Pathname", @index).run
refute_empty(result)
end

def test_does_not_include_private_constants
@index.index_single(RubyIndexer::IndexablePath.new(nil, "/fake.rb"), <<~RUBY)
class Foo
CONSTANT = 1
private_constant(:CONSTANT)
end
RUBY

result = RubyLsp::Requests::WorkspaceSymbol.new("Foo::CONSTANT", @index).run
assert_equal(1, result.length)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be misunderstanding the test, but if we're looking up Foo::CONSTANT then why is Foo in the results?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we fuzzy match the results for workspace symbol and the Jaro-Winkler similarity between Foo and Foo::CONSTANT is high. The reason is likely because Jaro-Winkler gives a higher importance to the prefix than it does to the rest of the word

ruby-lsp(main):001> DidYouMean::JaroWinkler.distance("Foo", "Foo::CONSTANT")
=> 0.8205128205128205

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. That may be worth a code comment.

assert_equal("Foo", T.must(result.first).name)
end
end