From 18866ca19fe5fde5458707ac440306d906e4a266 Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Thu, 14 Sep 2023 08:47:30 -0400 Subject: [PATCH 1/4] Account for private constants when jumping to definition --- lib/ruby_lsp/requests/definition.rb | 5 ++ test/requests/definition_expectations_test.rb | 74 +++++++++++++++++++ 2 files changed, 79 insertions(+) diff --git a/lib/ruby_lsp/requests/definition.rb b/lib/ruby_lsp/requests/definition.rb index dad604ff6..753f050d3 100644 --- a/lib/ruby_lsp/requests/definition.rb +++ b/lib/ruby_lsp/requests/definition.rb @@ -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 diff --git a/test/requests/definition_expectations_test.rb b/test/requests/definition_expectations_test.rb index 72933f5d7..8c7cdff21 100644 --- a/test/requests/definition_expectations_test.rb +++ b/test/requests/definition_expectations_test.rb @@ -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 From 3e285f2f5a931e91e43aa860dd99349e161e9a7a Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Thu, 14 Sep 2023 08:47:47 -0400 Subject: [PATCH 2/4] Account for private constants when providing completion --- lib/ruby_lsp/requests/completion.rb | 5 +++ test/requests/completion_test.rb | 52 +++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+) diff --git a/lib/ruby_lsp/requests/completion.rb b/lib/ruby_lsp/requests/completion.rb index e8525d546..d66718950 100644 --- a/lib/ruby_lsp/requests/completion.rb +++ b/lib/ruby_lsp/requests/completion.rb @@ -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, diff --git a/test/requests/completion_test.rb b/test/requests/completion_test.rb index 7b1408270..d2e4a2e71 100644 --- a/test/requests/completion_test.rb +++ b/test/requests/completion_test.rb @@ -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) + 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: {}) From 57aa59a22f05f7375b48c9b7269052f9c039f733 Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Thu, 14 Sep 2023 08:48:05 -0400 Subject: [PATCH 3/4] Account for private constants when showing hover --- lib/ruby_lsp/requests/hover.rb | 5 ++ test/requests/hover_expectations_test.rb | 62 ++++++++++++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/lib/ruby_lsp/requests/hover.rb b/lib/ruby_lsp/requests/hover.rb index 23eba552c..bb792d644 100644 --- a/lib/ruby_lsp/requests/hover.rb +++ b/lib/ruby_lsp/requests/hover.rb @@ -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), diff --git a/test/requests/hover_expectations_test.rb b/test/requests/hover_expectations_test.rb index 93fd6962e..bfa6a0624 100644 --- a/test/requests/hover_expectations_test.rb +++ b/test/requests/hover_expectations_test.rb @@ -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 From fad6231a79709740961ee1ad060bd1d77cb0167b Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Thu, 14 Sep 2023 08:48:20 -0400 Subject: [PATCH 4/4] Account for private constants in workspace symbol --- lib/ruby_lsp/requests/workspace_symbol.rb | 3 +++ test/requests/workspace_symbol_test.rb | 13 +++++++++++++ 2 files changed, 16 insertions(+) diff --git a/lib/ruby_lsp/requests/workspace_symbol.rb b/lib/ruby_lsp/requests/workspace_symbol.rb index 8b3822a3f..10505f8dd 100644 --- a/lib/ruby_lsp/requests/workspace_symbol.rb +++ b/lib/ruby_lsp/requests/workspace_symbol.rb @@ -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 diff --git a/test/requests/workspace_symbol_test.rb b/test/requests/workspace_symbol_test.rb index 818e130f8..cf5d8e07a 100644 --- a/test/requests/workspace_symbol_test.rb +++ b/test/requests/workspace_symbol_test.rb @@ -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) + assert_equal("Foo", T.must(result.first).name) + end end