From a0b74ed409000d8c1ef93ecd30e25ea201a57f44 Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Wed, 11 Sep 2024 10:56:04 -0400 Subject: [PATCH] Lazily fetch entry comments --- .../lib/ruby_indexer/declaration_listener.rb | 15 +++-- lib/ruby_indexer/lib/ruby_indexer/entry.rb | 61 +++++++++++++------ lib/ruby_indexer/lib/ruby_indexer/index.rb | 9 +-- .../lib/ruby_indexer/rbs_indexer.rb | 4 +- .../test/classes_and_modules_test.rb | 20 +++--- lib/ruby_indexer/test/constant_test.rb | 8 +-- lib/ruby_indexer/test/enhancements_test.rb | 4 +- lib/ruby_indexer/test/method_test.rb | 4 +- lib/ruby_indexer/test/rbs_indexer_test.rb | 2 +- lib/ruby_lsp/requests/support/common.rb | 2 +- 10 files changed, 81 insertions(+), 48 deletions(-) diff --git a/lib/ruby_indexer/lib/ruby_indexer/declaration_listener.rb b/lib/ruby_indexer/lib/ruby_indexer/declaration_listener.rb index 1fc625568..88c0bcca8 100644 --- a/lib/ruby_indexer/lib/ruby_indexer/declaration_listener.rb +++ b/lib/ruby_indexer/lib/ruby_indexer/declaration_listener.rb @@ -17,10 +17,11 @@ class DeclarationListener dispatcher: Prism::Dispatcher, parse_result: Prism::ParseResult, file_path: String, + collect_comments: T::Boolean, enhancements: T::Array[Enhancement], ).void end - def initialize(index, dispatcher, parse_result, file_path, enhancements: []) + def initialize(index, dispatcher, parse_result, file_path, collect_comments: false, enhancements: []) @index = index @file_path = file_path @enhancements = enhancements @@ -40,6 +41,7 @@ def initialize(index, dispatcher, parse_result, file_path, enhancements: []) # A stack of namespace entries that represent where we currently are. Used to properly assign methods to an owner @owner_stack = T.let([], T::Array[Entry::Namespace]) @indexing_errors = T.let([], T::Array[String]) + @collect_comments = collect_comments dispatcher.register( self, @@ -540,9 +542,11 @@ def add_constant(node, name, value = nil) ) end - sig { params(node: Prism::Node).returns(T::Array[String]) } + sig { params(node: Prism::Node).returns(T.nilable(String)) } def collect_comments(node) - comments = [] + return unless @collect_comments + + comments = +"" start_line = node.location.start_line - 1 start_line -= 1 unless @comments_by_line.key?(start_line) @@ -551,7 +555,7 @@ def collect_comments(node) comment = @comments_by_line[line] break unless comment - comment_content = comment.location.slice.chomp + comment_content = comment.location.slice # invalid encodings would raise an "invalid byte sequence" exception if !comment_content.valid_encoding? || comment_content.match?(@index.configuration.magic_comment_regex) @@ -560,9 +564,10 @@ def collect_comments(node) comment_content.delete_prefix!("#") comment_content.delete_prefix!(" ") - comments.prepend(comment_content) + comments.prepend("#{comment_content}\n") end + comments.chomp! comments end diff --git a/lib/ruby_indexer/lib/ruby_indexer/entry.rb b/lib/ruby_indexer/lib/ruby_indexer/entry.rb index e68170ee8..28bba256f 100644 --- a/lib/ruby_indexer/lib/ruby_indexer/entry.rb +++ b/lib/ruby_indexer/lib/ruby_indexer/entry.rb @@ -24,9 +24,6 @@ class Visibility < T::Enum alias_method :name_location, :location - sig { returns(T::Array[String]) } - attr_reader :comments - sig { returns(Visibility) } attr_accessor :visibility @@ -35,7 +32,7 @@ class Visibility < T::Enum name: String, file_path: String, location: T.any(Prism::Location, RubyIndexer::Location), - comments: T::Array[String], + comments: T.nilable(String), ).void end def initialize(name, file_path, location, comments) @@ -79,6 +76,37 @@ def file_name File.basename(@file_path) end + sig { returns(String) } + def comments + @comments ||= begin + # Parse only the comments based on the file path, which is much faster than parsing the entire file + parsed_comments = Prism.parse_file_comments(@file_path) + + # Group comments based on whether they belong to a single block of comments + grouped = parsed_comments.slice_when do |left, right| + left.location.start_line + 1 != right.location.start_line + end + + # Find the group that is either immediately or two lines above the current entry + correct_group = grouped.find do |group| + comment_end_line = group.last.location.start_line + (comment_end_line - 1..comment_end_line).cover?(@location.start_line - 1) + end + + # If we found something, we join the comments together. Otherwise, the entry has no documentation and we don't + # want to accidentally re-parse it, so we set it to an empty string. If an entry is updated, the entire entry + # object is dropped, so this will not prevent updates + if correct_group + correct_group.filter_map do |comment| + content = comment.slice + content if content.valid_encoding? + end.join("\n") + else + "" + end + end + end + class ModuleOperation extend T::Sig extend T::Helpers @@ -116,7 +144,7 @@ class Namespace < Entry file_path: String, location: T.any(Prism::Location, RubyIndexer::Location), name_location: T.any(Prism::Location, Location), - comments: T::Array[String], + comments: T.nilable(String), ).void end def initialize(nesting, file_path, location, name_location, comments) @@ -177,7 +205,7 @@ class Class < Namespace file_path: String, location: T.any(Prism::Location, RubyIndexer::Location), name_location: T.any(Prism::Location, Location), - comments: T::Array[String], + comments: T.nilable(String), parent_class: T.nilable(String), ).void end @@ -195,7 +223,7 @@ def ancestor_hash class SingletonClass < Class extend T::Sig - sig { params(location: Prism::Location, name_location: Prism::Location, comments: T::Array[String]).void } + sig { params(location: Prism::Location, name_location: Prism::Location, comments: T.nilable(String)).void } def update_singleton_information(location, name_location, comments) # Create a new RubyIndexer::Location object from the Prism location @location = Location.new( @@ -210,7 +238,7 @@ def update_singleton_information(location, name_location, comments) name_location.start_column, name_location.end_column, ) - @comments.concat(comments) + (@comments ||= +"") << comments if comments end end @@ -332,7 +360,7 @@ def parameters name: String, file_path: String, location: T.any(Prism::Location, RubyIndexer::Location), - comments: T::Array[String], + comments: T.nilable(String), visibility: Visibility, owner: T.nilable(Entry::Namespace), ).void @@ -400,7 +428,7 @@ class Method < Member file_path: String, location: T.any(Prism::Location, RubyIndexer::Location), name_location: T.any(Prism::Location, Location), - comments: T::Array[String], + comments: T.nilable(String), signatures: T::Array[Signature], visibility: Visibility, owner: T.nilable(Entry::Namespace), @@ -451,7 +479,7 @@ class UnresolvedConstantAlias < Entry name: String, file_path: String, location: T.any(Prism::Location, RubyIndexer::Location), - comments: T::Array[String], + comments: T.nilable(String), ).void end def initialize(target, nesting, name, file_path, location, comments) # rubocop:disable Metrics/ParameterLists @@ -488,7 +516,7 @@ class InstanceVariable < Entry name: String, file_path: String, location: T.any(Prism::Location, RubyIndexer::Location), - comments: T::Array[String], + comments: T.nilable(String), owner: T.nilable(Entry::Namespace), ).void end @@ -517,7 +545,7 @@ class UnresolvedMethodAlias < Entry owner: T.nilable(Entry::Namespace), file_path: String, location: T.any(Prism::Location, RubyIndexer::Location), - comments: T::Array[String], + comments: T.nilable(String), ).void end def initialize(new_name, old_name, owner, file_path, location, comments) # rubocop:disable Metrics/ParameterLists @@ -541,10 +569,9 @@ class MethodAlias < Entry sig { params(target: T.any(Member, MethodAlias), unresolved_alias: UnresolvedMethodAlias).void } def initialize(target, unresolved_alias) - full_comments = ["Alias for #{target.name}\n"] - full_comments.concat(unresolved_alias.comments) - full_comments << "\n" - full_comments.concat(target.comments) + full_comments = +"Alias for #{target.name}\n" + full_comments << "#{unresolved_alias.comments}\n" + full_comments << target.comments super( unresolved_alias.new_name, diff --git a/lib/ruby_indexer/lib/ruby_indexer/index.rb b/lib/ruby_indexer/lib/ruby_indexer/index.rb index dc2353408..5acf3ccfe 100644 --- a/lib/ruby_indexer/lib/ruby_indexer/index.rb +++ b/lib/ruby_indexer/lib/ruby_indexer/index.rb @@ -312,12 +312,12 @@ def index_all(indexable_paths: @configuration.indexables, &block) break unless block.call(progress) end - index_single(path) + index_single(path, collect_comments: false) end end - sig { params(indexable_path: IndexablePath, source: T.nilable(String)).void } - def index_single(indexable_path, source = nil) + sig { params(indexable_path: IndexablePath, source: T.nilable(String), collect_comments: T::Boolean).void } + def index_single(indexable_path, source = nil, collect_comments: true) content = source || File.read(indexable_path.full_path) dispatcher = Prism::Dispatcher.new @@ -327,6 +327,7 @@ def index_single(indexable_path, source = nil) dispatcher, result, indexable_path.full_path, + collect_comments: collect_comments, enhancements: @enhancements, ) dispatcher.dispatch(result.value) @@ -607,7 +608,7 @@ def existing_or_new_singleton_class(name) attached_ancestor.file_path, attached_ancestor.location, attached_ancestor.name_location, - [], + nil, nil, ) add(singleton, skip_prefix_tree: true) diff --git a/lib/ruby_indexer/lib/ruby_indexer/rbs_indexer.rb b/lib/ruby_indexer/lib/ruby_indexer/rbs_indexer.rb index 89cca8642..5b845c60f 100644 --- a/lib/ruby_indexer/lib/ruby_indexer/rbs_indexer.rb +++ b/lib/ruby_indexer/lib/ruby_indexer/rbs_indexer.rb @@ -272,10 +272,10 @@ def handle_signature_alias(member, owner_entry) RBS::AST::Declarations::Constant, RBS::AST::Members::MethodDefinition, RBS::AST::Members::Alias, - )).returns(T::Array[String]) + )).returns(T.nilable(String)) end def comments_to_string(declaration) - Array(declaration.comment&.string) + declaration.comment&.string end 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 eae6d6a45..fe201dc4d 100644 --- a/lib/ruby_indexer/test/classes_and_modules_test.rb +++ b/lib/ruby_indexer/test/classes_and_modules_test.rb @@ -213,10 +213,10 @@ class Bar; end RUBY foo_entry = @index["Foo"].first - assert_equal("This is a Foo comment\nThis is another Foo comment", foo_entry.comments.join("\n")) + assert_equal("This is a Foo comment\nThis is another Foo comment", foo_entry.comments) bar_entry = @index["Bar"].first - assert_equal("This Bar comment has 1 line padding", bar_entry.comments.join("\n")) + assert_equal("This Bar comment has 1 line padding", bar_entry.comments) end def test_skips_comments_containing_invalid_encodings @@ -239,10 +239,10 @@ class Bar; end RUBY foo_entry = @index["Foo"].first - assert_equal("This is a Foo comment\nThis is another Foo comment", foo_entry.comments.join("\n")) + assert_equal("This is a Foo comment\nThis is another Foo comment", foo_entry.comments) bar_entry = @index["Foo::Bar"].first - assert_equal("This is a Bar comment", bar_entry.comments.join("\n")) + assert_equal("This is a Bar comment", bar_entry.comments) end def test_comments_can_be_attached_to_a_reopened_class @@ -255,10 +255,10 @@ class Foo; end RUBY first_foo_entry = @index["Foo"][0] - assert_equal("This is a Foo comment", first_foo_entry.comments.join("\n")) + assert_equal("This is a Foo comment", first_foo_entry.comments) second_foo_entry = @index["Foo"][1] - assert_equal("This is another Foo comment", second_foo_entry.comments.join("\n")) + assert_equal("This is another Foo comment", second_foo_entry.comments) end def test_comments_removes_the_leading_pound_and_space @@ -271,10 +271,10 @@ class Bar; end RUBY first_foo_entry = @index["Foo"][0] - assert_equal("This is a Foo comment", first_foo_entry.comments.join("\n")) + assert_equal("This is a Foo comment", first_foo_entry.comments) second_foo_entry = @index["Bar"][0] - assert_equal("This is a Bar comment", second_foo_entry.comments.join("\n")) + assert_equal("This is a Bar comment", second_foo_entry.comments) end def test_private_class_and_module_indexing @@ -483,7 +483,7 @@ class << self foo = T.must(@index["Foo::"].first) assert_equal(4, foo.location.start_line) - assert_equal("Some extra comments", foo.comments.join("\n")) + assert_equal("Some extra comments", foo.comments) end def test_dynamic_singleton_class_blocks @@ -501,7 +501,7 @@ class << bar # That pattern cannot be properly analyzed statically and assuming that it's always a regular singleton simplifies # the implementation considerably. assert_equal(3, singleton.location.start_line) - assert_equal("Some extra comments", singleton.comments.join("\n")) + assert_equal("Some extra comments", singleton.comments) end def test_namespaces_inside_singleton_blocks diff --git a/lib/ruby_indexer/test/constant_test.rb b/lib/ruby_indexer/test/constant_test.rb index 7f64e2aa7..7e95d640b 100644 --- a/lib/ruby_indexer/test/constant_test.rb +++ b/lib/ruby_indexer/test/constant_test.rb @@ -86,16 +86,16 @@ class A A::BAZ = 1 RUBY - foo_comment = @index["FOO"].first.comments.join("\n") + foo_comment = @index["FOO"].first.comments assert_equal("FOO comment", foo_comment) - a_foo_comment = @index["A::FOO"].first.comments.join("\n") + a_foo_comment = @index["A::FOO"].first.comments assert_equal("A::FOO comment", a_foo_comment) - bar_comment = @index["BAR"].first.comments.join("\n") + bar_comment = @index["BAR"].first.comments assert_equal("::BAR comment", bar_comment) - a_baz_comment = @index["A::BAZ"].first.comments.join("\n") + a_baz_comment = @index["A::BAZ"].first.comments assert_equal("A::BAZ comment", a_baz_comment) end diff --git a/lib/ruby_indexer/test/enhancements_test.rb b/lib/ruby_indexer/test/enhancements_test.rb index df33ff568..cf23a6adf 100644 --- a/lib/ruby_indexer/test/enhancements_test.rb +++ b/lib/ruby_indexer/test/enhancements_test.rb @@ -38,7 +38,7 @@ def on_call_node(index, owner, node, file_path) file_path, location, location, - [], + nil, [Entry::Signature.new([Entry::RequiredParameter.new(name: :a)])], Entry::Visibility::PUBLIC, owner, @@ -120,7 +120,7 @@ def on_call_node(index, owner, node, file_path) file_path, location, location, - [], + nil, [], Entry::Visibility::PUBLIC, owner, diff --git a/lib/ruby_indexer/test/method_test.rb b/lib/ruby_indexer/test/method_test.rb index a443b3655..425020edf 100644 --- a/lib/ruby_indexer/test/method_test.rb +++ b/lib/ruby_indexer/test/method_test.rb @@ -382,9 +382,9 @@ class Foo RUBY assert_entry("bar", Entry::Accessor, "/fake/path/foo.rb:2-15:2-18") - assert_equal("Hello there", @index["bar"].first.comments.join("\n")) + assert_equal("Hello there", @index["bar"].first.comments) assert_entry("other", Entry::Accessor, "/fake/path/foo.rb:2-21:2-26") - assert_equal("Hello there", @index["other"].first.comments.join("\n")) + assert_equal("Hello there", @index["other"].first.comments) assert_entry("baz=", Entry::Accessor, "/fake/path/foo.rb:3-15:3-18") assert_entry("qux", Entry::Accessor, "/fake/path/foo.rb:4-17:4-20") assert_entry("qux=", Entry::Accessor, "/fake/path/foo.rb:4-17:4-20") diff --git a/lib/ruby_indexer/test/rbs_indexer_test.rb b/lib/ruby_indexer/test/rbs_indexer_test.rb index bba982c5c..5a42d4fc9 100644 --- a/lib/ruby_indexer/test/rbs_indexer_test.rb +++ b/lib/ruby_indexer/test/rbs_indexer_test.rb @@ -345,7 +345,7 @@ def test_signature_alias assert_equal("all?", entry.old_name) assert_equal("Array", entry.owner.name) assert(entry.file_path.end_with?("core/array.rbs")) - assert_includes(entry.comments[0], "Returns `true` if any element of `self` meets a given criterion.") + assert_includes(entry.comments, "Returns `true` if any element of `self` meets a given criterion.") end private diff --git a/lib/ruby_lsp/requests/support/common.rb b/lib/ruby_lsp/requests/support/common.rb index 7a86b91fd..8f6b00d5f 100644 --- a/lib/ruby_lsp/requests/support/common.rb +++ b/lib/ruby_lsp/requests/support/common.rb @@ -99,7 +99,7 @@ def categorized_markdown_from_index_entries(title, entries, max_entries = nil) ) definitions << "[#{entry.file_name}](#{uri})" - content << "\n\n#{entry.comments.join("\n")}" unless entry.comments.empty? + content << "\n\n#{entry.comments}" unless entry.comments.empty? end additional_entries_text = if max_entries && entries.length > max_entries