From b8f17bf8bcc302eb27e2e48b5ccbf2ca0c87d219 Mon Sep 17 00:00:00 2001 From: Stan Lo Date: Thu, 30 May 2024 17:06:49 +0100 Subject: [PATCH] Track method's visibility during indexing (#2093) * Track method visibility in the index This commit supports visibility tracking for methods in the index by: - Adding a visibility field to the Member entry, which is a superclass of Accessor, Method, and Alias. - In the DeclarationListener, create a nested stack structure to track the visibility of the current scope. * Use T::Enum to type visibility values * Simplify visibility stack implementation --- .../lib/ruby_indexer/declaration_listener.rb | 45 +++++++++++++++++-- lib/ruby_indexer/lib/ruby_indexer/entry.rb | 22 ++++++--- .../test/classes_and_modules_test.rb | 6 +-- lib/ruby_indexer/test/constant_test.rb | 16 +++---- lib/ruby_indexer/test/method_test.rb | 37 +++++++++++++++ lib/ruby_indexer/test/test_case.rb | 4 +- lib/ruby_lsp/listeners/completion.rb | 3 +- lib/ruby_lsp/listeners/definition.rb | 3 +- lib/ruby_lsp/listeners/hover.rb | 3 +- lib/ruby_lsp/requests/workspace_symbol.rb | 2 +- 10 files changed, 117 insertions(+), 24 deletions(-) diff --git a/lib/ruby_indexer/lib/ruby_indexer/declaration_listener.rb b/lib/ruby_indexer/lib/ruby_indexer/declaration_listener.rb index 8e22800d8..ab7cbed46 100644 --- a/lib/ruby_indexer/lib/ruby_indexer/declaration_listener.rb +++ b/lib/ruby_indexer/lib/ruby_indexer/declaration_listener.rb @@ -11,6 +11,7 @@ class DeclarationListener def initialize(index, dispatcher, parse_result, file_path) @index = index @file_path = file_path + @visibility_stack = T.let([Entry::Visibility::PUBLIC], T::Array[Entry::Visibility]) @comments_by_line = T.let( parse_result.comments.to_h do |c| [c.location.start_line, c] @@ -35,6 +36,7 @@ def initialize(index, dispatcher, parse_result, file_path) :on_def_node_enter, :on_def_node_leave, :on_call_node_enter, + :on_call_node_leave, :on_multi_write_node_enter, :on_constant_path_write_node_enter, :on_constant_path_or_write_node_enter, @@ -55,6 +57,7 @@ def initialize(index, dispatcher, parse_result, file_path) sig { params(node: Prism::ClassNode).void } def on_class_node_enter(node) + @visibility_stack.push(Entry::Visibility::PUBLIC) name = node.constant_path.location.slice comments = collect_comments(node) @@ -82,10 +85,12 @@ def on_class_node_enter(node) def on_class_node_leave(node) @stack.pop @owner_stack.pop + @visibility_stack.pop end sig { params(node: Prism::ModuleNode).void } def on_module_node_enter(node) + @visibility_stack.push(Entry::Visibility::PUBLIC) name = node.constant_path.location.slice comments = collect_comments(node) @@ -100,6 +105,7 @@ def on_module_node_enter(node) def on_module_node_leave(node) @stack.pop @owner_stack.pop + @visibility_stack.pop end sig { params(node: Prism::MultiWriteNode).void } @@ -203,6 +209,25 @@ def on_call_node_enter(node) handle_module_operation(node, :included_modules) when :prepend handle_module_operation(node, :prepended_modules) + when :public + @visibility_stack.push(Entry::Visibility::PUBLIC) + when :protected + @visibility_stack.push(Entry::Visibility::PROTECTED) + when :private + @visibility_stack.push(Entry::Visibility::PRIVATE) + end + end + + sig { params(node: Prism::CallNode).void } + def on_call_node_leave(node) + message = node.name + case message + when :public, :protected, :private + # We want to restore the visibility stack when we leave a method definition with a visibility modifier + # e.g. `private def foo; end` + if node.arguments&.arguments&.first&.is_a?(Prism::DefNode) + @visibility_stack.pop + end end end @@ -220,6 +245,7 @@ def on_def_node_enter(node) node.location, comments, node.parameters, + current_visibility, @owner_stack.last, ) when Prism::SelfNode @@ -229,6 +255,7 @@ def on_def_node_enter(node) node.location, comments, node.parameters, + current_visibility, @owner_stack.last, ) end @@ -333,7 +360,7 @@ def handle_private_constant(node) # The private_constant method does not resolve the constant name. It always points to a constant that needs to # exist in the current namespace entries = @index[fully_qualify_name(name)] - entries&.each { |entry| entry.visibility = :private } + entries&.each { |entry| entry.visibility = Entry::Visibility::PRIVATE } end sig do @@ -430,8 +457,15 @@ def handle_attribute(node, reader:, writer:) next unless name && loc - @index << Entry::Accessor.new(name, @file_path, loc, comments, @owner_stack.last) if reader - @index << Entry::Accessor.new("#{name}=", @file_path, loc, comments, @owner_stack.last) if writer + @index << Entry::Accessor.new(name, @file_path, loc, comments, current_visibility, @owner_stack.last) if reader + @index << Entry::Accessor.new( + "#{name}=", + @file_path, + loc, + comments, + current_visibility, + @owner_stack.last, + ) if writer end end @@ -456,5 +490,10 @@ def handle_module_operation(node, operation) collection = operation == :included_modules ? owner.included_modules : owner.prepended_modules collection.concat(names) end + + sig { returns(Entry::Visibility) } + def current_visibility + T.must(@visibility_stack.last) + end end end diff --git a/lib/ruby_indexer/lib/ruby_indexer/entry.rb b/lib/ruby_indexer/lib/ruby_indexer/entry.rb index ceaa36983..8fac069ed 100644 --- a/lib/ruby_indexer/lib/ruby_indexer/entry.rb +++ b/lib/ruby_indexer/lib/ruby_indexer/entry.rb @@ -3,6 +3,14 @@ module RubyIndexer class Entry + class Visibility < T::Enum + enums do + PUBLIC = new(:public) + PROTECTED = new(:protected) + PRIVATE = new(:private) + end + end + extend T::Sig sig { returns(String) } @@ -17,7 +25,7 @@ class Entry sig { returns(T::Array[String]) } attr_reader :comments - sig { returns(Symbol) } + sig { returns(Visibility) } attr_accessor :visibility sig do @@ -32,7 +40,7 @@ def initialize(name, file_path, location, comments) @name = name @file_path = file_path @comments = comments - @visibility = T.let(:public, Symbol) + @visibility = T.let(Visibility::PUBLIC, Visibility) @location = T.let( if location.is_a?(Prism::Location) @@ -188,11 +196,13 @@ class Member < Entry file_path: String, location: T.any(Prism::Location, RubyIndexer::Location), comments: T::Array[String], + visibility: Visibility, owner: T.nilable(Entry::Namespace), ).void end - def initialize(name, file_path, location, comments, owner) + def initialize(name, file_path, location, comments, visibility, owner) # rubocop:disable Metrics/ParameterLists super(name, file_path, location, comments) + @visibility = visibility @owner = owner end @@ -227,11 +237,12 @@ class Method < Member location: T.any(Prism::Location, RubyIndexer::Location), comments: T::Array[String], parameters_node: T.nilable(Prism::ParametersNode), + visibility: Visibility, owner: T.nilable(Entry::Namespace), ).void end - def initialize(name, file_path, location, comments, parameters_node, owner) # rubocop:disable Metrics/ParameterLists - super(name, file_path, location, comments, owner) + def initialize(name, file_path, location, comments, parameters_node, visibility, owner) # rubocop:disable Metrics/ParameterLists + super(name, file_path, location, comments, visibility, owner) @parameters = T.let(list_params(parameters_node), T::Array[Parameter]) end @@ -377,6 +388,7 @@ class Alias < Entry def initialize(target, unresolved_alias) super(unresolved_alias.name, unresolved_alias.file_path, unresolved_alias.location, unresolved_alias.comments) + @visibility = unresolved_alias.visibility @target = target end end diff --git a/lib/ruby_indexer/test/classes_and_modules_test.rb b/lib/ruby_indexer/test/classes_and_modules_test.rb index d18cb1b02..d76c25e17 100644 --- a/lib/ruby_indexer/test/classes_and_modules_test.rb +++ b/lib/ruby_indexer/test/classes_and_modules_test.rb @@ -290,13 +290,13 @@ class D; end RUBY b_const = @index["A::B"].first - assert_equal(:private, b_const.visibility) + assert_equal(Entry::Visibility::PRIVATE, b_const.visibility) c_const = @index["A::C"].first - assert_equal(:private, c_const.visibility) + assert_equal(Entry::Visibility::PRIVATE, c_const.visibility) d_const = @index["A::D"].first - assert_equal(:public, d_const.visibility) + assert_equal(Entry::Visibility::PUBLIC, d_const.visibility) end def test_keeping_track_of_super_classes diff --git a/lib/ruby_indexer/test/constant_test.rb b/lib/ruby_indexer/test/constant_test.rb index 44839ffa3..9c6e77915 100644 --- a/lib/ruby_indexer/test/constant_test.rb +++ b/lib/ruby_indexer/test/constant_test.rb @@ -122,13 +122,13 @@ class A RUBY b_const = @index["A::B"].first - assert_equal(:private, b_const.visibility) + assert_equal(Entry::Visibility::PRIVATE, b_const.visibility) c_const = @index["A::C"].first - assert_equal(:private, c_const.visibility) + assert_equal(Entry::Visibility::PRIVATE, c_const.visibility) d_const = @index["A::D"].first - assert_equal(:public, d_const.visibility) + assert_equal(Entry::Visibility::PUBLIC, d_const.visibility) end def test_marking_constants_as_private_reopening_namespaces @@ -155,13 +155,13 @@ module B RUBY a_const = @index["A::B::CONST_A"].first - assert_equal(:private, a_const.visibility) + assert_equal(Entry::Visibility::PRIVATE, a_const.visibility) b_const = @index["A::B::CONST_B"].first - assert_equal(:private, b_const.visibility) + assert_equal(Entry::Visibility::PRIVATE, b_const.visibility) c_const = @index["A::B::CONST_C"].first - assert_equal(:private, c_const.visibility) + assert_equal(Entry::Visibility::PRIVATE, c_const.visibility) end def test_marking_constants_as_private_with_receiver @@ -179,10 +179,10 @@ module B RUBY a_const = @index["A::B::CONST_A"].first - assert_equal(:private, a_const.visibility) + assert_equal(Entry::Visibility::PRIVATE, a_const.visibility) b_const = @index["A::B::CONST_B"].first - assert_equal(:private, b_const.visibility) + assert_equal(Entry::Visibility::PRIVATE, b_const.visibility) end def test_indexing_constant_aliases diff --git a/lib/ruby_indexer/test/method_test.rb b/lib/ruby_indexer/test/method_test.rb index ff1245fa6..4565bcb2a 100644 --- a/lib/ruby_indexer/test/method_test.rb +++ b/lib/ruby_indexer/test/method_test.rb @@ -71,6 +71,43 @@ def bar assert_equal("Bar", second_entry.owner.name) end + def test_visibility_tracking + index(<<~RUBY) + private def foo + end + + def bar; end + + protected + + def baz; end + RUBY + + assert_entry("foo", Entry::InstanceMethod, "/fake/path/foo.rb:0-8:1-3", visibility: Entry::Visibility::PRIVATE) + assert_entry("bar", Entry::InstanceMethod, "/fake/path/foo.rb:3-0:3-12", visibility: Entry::Visibility::PUBLIC) + assert_entry("baz", Entry::InstanceMethod, "/fake/path/foo.rb:7-0:7-12", visibility: Entry::Visibility::PROTECTED) + end + + def test_visibility_tracking_with_nested_class_or_modules + index(<<~RUBY) + class Foo + private + + def foo; end + + class Bar + def bar; end + end + + def baz; end + end + RUBY + + assert_entry("foo", Entry::InstanceMethod, "/fake/path/foo.rb:3-2:3-14", visibility: Entry::Visibility::PRIVATE) + assert_entry("bar", Entry::InstanceMethod, "/fake/path/foo.rb:6-4:6-16", visibility: Entry::Visibility::PUBLIC) + assert_entry("baz", Entry::InstanceMethod, "/fake/path/foo.rb:9-2:9-14", visibility: Entry::Visibility::PRIVATE) + end + def test_method_with_parameters index(<<~RUBY) class Foo diff --git a/lib/ruby_indexer/test/test_case.rb b/lib/ruby_indexer/test/test_case.rb index cf666cb4e..61708495d 100644 --- a/lib/ruby_indexer/test/test_case.rb +++ b/lib/ruby_indexer/test/test_case.rb @@ -15,7 +15,7 @@ def index(source) @index.index_single(IndexablePath.new(nil, "/fake/path/foo.rb"), source) end - def assert_entry(expected_name, type, expected_location) + def assert_entry(expected_name, type, expected_location, visibility: nil) entries = @index[expected_name] refute_empty(entries, "Expected #{expected_name} to be indexed") @@ -28,6 +28,8 @@ def assert_entry(expected_name, type, expected_location) ":#{location.end_line - 1}-#{location.end_column}" assert_equal(expected_location, location_string) + + assert_equal(visibility, entry.visibility) if visibility end def refute_entry(expected_name) diff --git a/lib/ruby_lsp/listeners/completion.rb b/lib/ruby_lsp/listeners/completion.rb index 0d678b0f8..70a54cf80 100644 --- a/lib/ruby_lsp/listeners/completion.rb +++ b/lib/ruby_lsp/listeners/completion.rb @@ -174,7 +174,8 @@ def constant_path_completion(name, range) # 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}::") + next if first_entry.visibility == RubyIndexer::Entry::Visibility::PRIVATE && + !first_entry.name.start_with?("#{@nesting}::") constant_name = first_entry.name.delete_prefix("#{real_namespace}::") full_name = aliased_namespace.empty? ? constant_name : "#{aliased_namespace}::#{constant_name}" diff --git a/lib/ruby_lsp/listeners/definition.rb b/lib/ruby_lsp/listeners/definition.rb index dab51fb6a..5fc1a551a 100644 --- a/lib/ruby_lsp/listeners/definition.rb +++ b/lib/ruby_lsp/listeners/definition.rb @@ -211,7 +211,8 @@ def find_in_index(value) # 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}" + return if first_entry.visibility == RubyIndexer::Entry::Visibility::PRIVATE && + first_entry.name != "#{@nesting.join("::")}::#{value}" entries.each do |entry| location = entry.location diff --git a/lib/ruby_lsp/listeners/hover.rb b/lib/ruby_lsp/listeners/hover.rb index c580a8d98..f3c661b33 100644 --- a/lib/ruby_lsp/listeners/hover.rb +++ b/lib/ruby_lsp/listeners/hover.rb @@ -166,7 +166,8 @@ def generate_hover(name, location) # 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}" + return if first_entry.visibility == RubyIndexer::Entry::Visibility::PRIVATE && + first_entry.name != "#{@nesting.join("::")}::#{name}" categorized_markdown_from_index_entries(name, entries).each do |category, content| @response_builder.push(content, category: category) diff --git a/lib/ruby_lsp/requests/workspace_symbol.rb b/lib/ruby_lsp/requests/workspace_symbol.rb index c30cf02ad..e3f60a49f 100644 --- a/lib/ruby_lsp/requests/workspace_symbol.rb +++ b/lib/ruby_lsp/requests/workspace_symbol.rb @@ -39,7 +39,7 @@ def perform next if @global_state.typechecker && not_in_dependencies?(file_path) # We should never show private symbols when searching the entire workspace - next if entry.visibility == :private + next if entry.visibility == RubyIndexer::Entry::Visibility::PRIVATE kind = kind_for_entry(entry) loc = entry.location