From 7307e4944b138dc4bf6ab486b6d25c0fd72fe28e Mon Sep 17 00:00:00 2001 From: Andy Waite Date: Mon, 18 Sep 2023 12:46:47 -0400 Subject: [PATCH 1/6] Migrate DocumentLink to YARP --- Gemfile.lock | 2 +- lib/ruby_lsp/document.rb | 1 + lib/ruby_lsp/requests/document_link.rb | 43 ++++++++++-- ruby-lsp.gemspec | 2 +- .../document_link/source_comment.exp.json | 70 +++++++++++++++++++ test/expectations/expectations_test_runner.rb | 3 - test/fixtures/source_comment.rb | 18 +++++ 7 files changed, 127 insertions(+), 12 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 442089c75c..3222442309 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -5,7 +5,7 @@ PATH language_server-protocol (~> 3.17.0) sorbet-runtime syntax_tree (>= 6.1.1, < 7) - yarp (>= 0.11, < 0.13) + yarp (>= 0.12, < 0.13) GEM remote: https://rubygems.org/ diff --git a/lib/ruby_lsp/document.rb b/lib/ruby_lsp/document.rb index 55d86b5a7d..c3f20c3296 100644 --- a/lib/ruby_lsp/document.rb +++ b/lib/ruby_lsp/document.rb @@ -30,6 +30,7 @@ def initialize(source:, version:, uri:, encoding: Constant::PositionEncodingKind @uri = T.let(uri, URI::Generic) @unparsed_edits = T.let([], T::Array[EditShape]) @parse_result = T.let(YARP.parse(@source), YARP::ParseResult) + @parse_result.attach_comments! end sig { returns(YARP::ProgramNode) } diff --git a/lib/ruby_lsp/requests/document_link.rb b/lib/ruby_lsp/requests/document_link.rb index 211b2c0241..8ba3de49b8 100644 --- a/lib/ruby_lsp/requests/document_link.rb +++ b/lib/ruby_lsp/requests/document_link.rb @@ -86,12 +86,43 @@ def initialize(uri, emitter, message_queue) @gem_version = T.let(version_match && version_match[0], T.nilable(String)) @_response = T.let([], T::Array[Interface::DocumentLink]) - emitter.register(self, :on_comment) + emitter.register(self, :on_def, :on_class, :on_module, :on_constant_write, :on_constant_path_write) end - sig { params(node: SyntaxTree::Comment).void } - def on_comment(node) - match = node.value.match(%r{source://.*#\d+$}) + sig { params(node: YARP::DefNode).void } + def on_def(node) + extract_document_link(node) + end + + sig { params(node: YARP::ClassNode).void } + def on_class(node) + extract_document_link(node) + end + + sig { params(node: YARP::ModuleNode).void } + def on_module(node) + extract_document_link(node) + end + + sig { params(node: YARP::ConstantWriteNode).void } + def on_constant_write(node) + extract_document_link(node) + end + + sig { params(node: YARP::ConstantPathWriteNode).void } + def on_constant_path_write(node) + extract_document_link(node) + end + + private + + sig { params(node: YARP::Node).void } + def extract_document_link(node) + comments = node.location.comments + return if comments.none? + + first_comment_location = comments.first.location + match = first_comment_location.slice.match(%r{source://.*#\d+$}) return unless match uri = T.cast(URI(T.must(match[0])), URI::Source) @@ -102,14 +133,12 @@ def on_comment(node) return if file_path.nil? @_response << Interface::DocumentLink.new( - range: range_from_node(node), + range: range_from_location(first_comment_location), target: "file://#{file_path}##{uri.line_number}", tooltip: "Jump to #{file_path}##{uri.line_number}", ) end - private - # Try to figure out the gem version for a source:// link. The order of precedence is: # 1. The version in the URI # 2. The version in the RBI file name diff --git a/ruby-lsp.gemspec b/ruby-lsp.gemspec index 533fae8e19..35eb8065b7 100644 --- a/ruby-lsp.gemspec +++ b/ruby-lsp.gemspec @@ -20,7 +20,7 @@ Gem::Specification.new do |s| s.add_dependency("language_server-protocol", "~> 3.17.0") s.add_dependency("sorbet-runtime") s.add_dependency("syntax_tree", ">= 6.1.1", "< 7") - s.add_dependency("yarp", ">= 0.11", "< 0.13") + s.add_dependency("yarp", ">= 0.12", "< 0.13") s.required_ruby_version = ">= 3.0" end diff --git a/test/expectations/document_link/source_comment.exp.json b/test/expectations/document_link/source_comment.exp.json index 5196d89375..a33e6aba58 100644 --- a/test/expectations/document_link/source_comment.exp.json +++ b/test/expectations/document_link/source_comment.exp.json @@ -41,6 +41,76 @@ }, "target": "file://BUNDLER_PATH/gems/syntax_tree-SYNTAX_TREE_VERSION/lib/syntax_tree.rb#39", "tooltip": "Jump to BUNDLER_PATH/gems/syntax_tree-SYNTAX_TREE_VERSION/lib/syntax_tree.rb#39" + }, + { + "range": { + "start": { + "line": 12, + "character": 0 + }, + "end": { + "line": 12, + "character": 44 + } + }, + "target": "file://BUNDLER_PATH/gems/syntax_tree-SYNTAX_TREE_VERSION/lib/syntax_tree.rb#1", + "tooltip": "Jump to BUNDLER_PATH/gems/syntax_tree-SYNTAX_TREE_VERSION/lib/syntax_tree.rb#1" + }, + { + "range": { + "start": { + "line": 16, + "character": 0 + }, + "end": { + "line": 16, + "character": 44 + } + }, + "target": "file://BUNDLER_PATH/gems/syntax_tree-SYNTAX_TREE_VERSION/lib/syntax_tree.rb#2", + "tooltip": "Jump to BUNDLER_PATH/gems/syntax_tree-SYNTAX_TREE_VERSION/lib/syntax_tree.rb#2" + }, + { + "range": { + "start": { + "line": 20, + "character": 0 + }, + "end": { + "line": 20, + "character": 44 + } + }, + "target": "file://BUNDLER_PATH/gems/syntax_tree-SYNTAX_TREE_VERSION/lib/syntax_tree.rb#3", + "tooltip": "Jump to BUNDLER_PATH/gems/syntax_tree-SYNTAX_TREE_VERSION/lib/syntax_tree.rb#3" + }, + { + "range": { + "start": { + "line": 24, + "character": 0 + }, + "end": { + "line": 24, + "character": 44 + } + }, + "target": "file://BUNDLER_PATH/gems/syntax_tree-SYNTAX_TREE_VERSION/lib/syntax_tree.rb#4", + "tooltip": "Jump to BUNDLER_PATH/gems/syntax_tree-SYNTAX_TREE_VERSION/lib/syntax_tree.rb#4" + }, + { + "range": { + "start": { + "line": 27, + "character": 0 + }, + "end": { + "line": 27, + "character": 44 + } + }, + "target": "file://BUNDLER_PATH/gems/syntax_tree-SYNTAX_TREE_VERSION/lib/syntax_tree.rb#5", + "tooltip": "Jump to BUNDLER_PATH/gems/syntax_tree-SYNTAX_TREE_VERSION/lib/syntax_tree.rb#5" } ] } diff --git a/test/expectations/expectations_test_runner.rb b/test/expectations/expectations_test_runner.rb index f6f71d89ea..c05b9550d0 100644 --- a/test/expectations/expectations_test_runner.rb +++ b/test/expectations/expectations_test_runner.rb @@ -50,9 +50,6 @@ def default_args RB Dir.glob(TEST_FIXTURES_GLOB).each do |path| - # temporarily skip until we figure out comment handling - next if handler_class == RubyLsp::Requests::DocumentLink && path == "test/fixtures/source_comment.rb" - test_name = File.basename(path, ".rb") expectations_dir = File.join(TEST_EXP_DIR, expectation_suffix) diff --git a/test/fixtures/source_comment.rb b/test/fixtures/source_comment.rb index e8b062a7c6..170425b208 100644 --- a/test/fixtures/source_comment.rb +++ b/test/fixtures/source_comment.rb @@ -10,6 +10,24 @@ def bar def baz end +# source://syntax_tree//lib/syntax_tree.rb#1 +class Foo +end + +# source://syntax_tree//lib/syntax_tree.rb#2 +class Foo::Bar +end + +# source://syntax_tree//lib/syntax_tree.rb#3 +module Foo +end + +# source://syntax_tree//lib/syntax_tree.rb#4 +FOO = 1 + +# source://syntax_tree//lib/syntax_tree.rb#5 +FOO::BAR = 1 + # source://deleted//lib/foo.rb.rb#1 def baz end From e142d4977ebf8d14db3cb8676cd8d6d1a2620081 Mon Sep 17 00:00:00 2001 From: Andy Waite Date: Mon, 18 Sep 2023 15:31:39 -0400 Subject: [PATCH 2/6] Only parse comments where needed --- lib/ruby_lsp/document.rb | 6 ++++- lib/ruby_lsp/executor.rb | 2 +- lib/ruby_lsp/requests/document_link.rb | 26 ++++++++++++++----- .../document_link_expectations_test.rb | 2 +- 4 files changed, 26 insertions(+), 10 deletions(-) diff --git a/lib/ruby_lsp/document.rb b/lib/ruby_lsp/document.rb index c3f20c3296..f253f15d25 100644 --- a/lib/ruby_lsp/document.rb +++ b/lib/ruby_lsp/document.rb @@ -30,7 +30,6 @@ def initialize(source:, version:, uri:, encoding: Constant::PositionEncodingKind @uri = T.let(uri, URI::Generic) @unparsed_edits = T.let([], T::Array[EditShape]) @parse_result = T.let(YARP.parse(@source), YARP::ParseResult) - @parse_result.attach_comments! end sig { returns(YARP::ProgramNode) } @@ -38,6 +37,11 @@ def tree @parse_result.value end + # sig { returns(YARP::ProgramNode) } + def comments + @parse_result.comments + end + sig { params(other: Document).returns(T::Boolean) } def ==(other) @source == other.source diff --git a/lib/ruby_lsp/executor.rb b/lib/ruby_lsp/executor.rb index 8e1c193e8c..c354a04188 100644 --- a/lib/ruby_lsp/executor.rb +++ b/lib/ruby_lsp/executor.rb @@ -97,7 +97,7 @@ def run(request) # Run listeners for the document emitter = EventEmitter.new document_symbol = Requests::DocumentSymbol.new(emitter, @message_queue) - document_link = Requests::DocumentLink.new(uri, emitter, @message_queue) + document_link = Requests::DocumentLink.new(uri, emitter, @message_queue, document.comments) code_lens = Requests::CodeLens.new(uri, emitter, @message_queue, @test_library) semantic_highlighting = Requests::SemanticHighlighting.new(emitter, @message_queue) diff --git a/lib/ruby_lsp/requests/document_link.rb b/lib/ruby_lsp/requests/document_link.rb index 8ba3de49b8..934c3d6c02 100644 --- a/lib/ruby_lsp/requests/document_link.rb +++ b/lib/ruby_lsp/requests/document_link.rb @@ -75,8 +75,15 @@ def gem_paths sig { override.returns(ResponseType) } attr_reader :_response - sig { params(uri: URI::Generic, emitter: EventEmitter, message_queue: Thread::Queue).void } - def initialize(uri, emitter, message_queue) + sig do + params( + uri: URI::Generic, + emitter: EventEmitter, + message_queue: Thread::Queue, + comments: T::Array[YARP::Comment], + ).void + end + def initialize(uri, emitter, message_queue, comments) super(emitter, message_queue) # Match the version based on the version in the RBI file name. Notice that the `@` symbol is sanitized to `%40` @@ -85,6 +92,12 @@ def initialize(uri, emitter, message_queue) version_match = path ? /(?<=%40)[\d.]+(?=\.rbi$)/.match(path) : nil @gem_version = T.let(version_match && version_match[0], T.nilable(String)) @_response = T.let([], T::Array[Interface::DocumentLink]) + @comments = T.let( + comments.map do |comment| + [comment.location.end_line, comment] + end.to_h, + T::Hash[Integer, YARP::Comment], + ) emitter.register(self, :on_def, :on_class, :on_module, :on_constant_write, :on_constant_path_write) end @@ -118,11 +131,10 @@ def on_constant_path_write(node) sig { params(node: YARP::Node).void } def extract_document_link(node) - comments = node.location.comments - return if comments.none? + comment = @comments[node.location.start_line - 1] + return unless comment - first_comment_location = comments.first.location - match = first_comment_location.slice.match(%r{source://.*#\d+$}) + match = comment.location.slice.match(%r{source://.*#\d+$}) return unless match uri = T.cast(URI(T.must(match[0])), URI::Source) @@ -133,7 +145,7 @@ def extract_document_link(node) return if file_path.nil? @_response << Interface::DocumentLink.new( - range: range_from_location(first_comment_location), + range: range_from_location(comment.location), target: "file://#{file_path}##{uri.line_number}", tooltip: "Jump to #{file_path}##{uri.line_number}", ) diff --git a/test/requests/document_link_expectations_test.rb b/test/requests/document_link_expectations_test.rb index 5b1ab655f0..b18319b6fc 100644 --- a/test/requests/document_link_expectations_test.rb +++ b/test/requests/document_link_expectations_test.rb @@ -26,7 +26,7 @@ def run_expectations(source) document = RubyLsp::Document.new(source: source, version: 1, uri: uri) emitter = RubyLsp::EventEmitter.new - listener = RubyLsp::Requests::DocumentLink.new(uri, emitter, message_queue) + listener = RubyLsp::Requests::DocumentLink.new(uri, emitter, message_queue, document.comments) emitter.visit(document.tree) listener.response ensure From 6744a8d5ee2b8f3bca9181bfe81f7263fdbe4ba8 Mon Sep 17 00:00:00 2001 From: Andy Waite Date: Mon, 18 Sep 2023 15:33:16 -0400 Subject: [PATCH 3/6] Fix type --- lib/ruby_lsp/document.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ruby_lsp/document.rb b/lib/ruby_lsp/document.rb index f253f15d25..4bd9e91208 100644 --- a/lib/ruby_lsp/document.rb +++ b/lib/ruby_lsp/document.rb @@ -37,7 +37,7 @@ def tree @parse_result.value end - # sig { returns(YARP::ProgramNode) } + sig { returns(T::Array[YARP::Comment]) } def comments @parse_result.comments end From 5e6da7b0ff3b5ba89e28a427b799a7bd4af5c76c Mon Sep 17 00:00:00 2001 From: Andy Waite Date: Mon, 18 Sep 2023 15:53:28 -0400 Subject: [PATCH 4/6] PR feedback --- lib/ruby_lsp/executor.rb | 2 +- lib/ruby_lsp/requests/document_link.rb | 12 ++++++------ test/requests/document_link_expectations_test.rb | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/ruby_lsp/executor.rb b/lib/ruby_lsp/executor.rb index c354a04188..85cf18a0d1 100644 --- a/lib/ruby_lsp/executor.rb +++ b/lib/ruby_lsp/executor.rb @@ -97,7 +97,7 @@ def run(request) # Run listeners for the document emitter = EventEmitter.new document_symbol = Requests::DocumentSymbol.new(emitter, @message_queue) - document_link = Requests::DocumentLink.new(uri, emitter, @message_queue, document.comments) + document_link = Requests::DocumentLink.new(uri, emitter, document.comments, @message_queue) code_lens = Requests::CodeLens.new(uri, emitter, @message_queue, @test_library) semantic_highlighting = Requests::SemanticHighlighting.new(emitter, @message_queue) diff --git a/lib/ruby_lsp/requests/document_link.rb b/lib/ruby_lsp/requests/document_link.rb index 934c3d6c02..c75b16ae60 100644 --- a/lib/ruby_lsp/requests/document_link.rb +++ b/lib/ruby_lsp/requests/document_link.rb @@ -78,12 +78,12 @@ def gem_paths sig do params( uri: URI::Generic, + comments: T::Array[YARP::Comment], emitter: EventEmitter, message_queue: Thread::Queue, - comments: T::Array[YARP::Comment], ).void end - def initialize(uri, emitter, message_queue, comments) + def initialize(uri, comments, emitter, message_queue) super(emitter, message_queue) # Match the version based on the version in the RBI file name. Notice that the `@` symbol is sanitized to `%40` @@ -92,10 +92,10 @@ def initialize(uri, emitter, message_queue, comments) version_match = path ? /(?<=%40)[\d.]+(?=\.rbi$)/.match(path) : nil @gem_version = T.let(version_match && version_match[0], T.nilable(String)) @_response = T.let([], T::Array[Interface::DocumentLink]) - @comments = T.let( - comments.map do |comment| + @lines_to_comments = T.let( + comments.to_h do |comment| [comment.location.end_line, comment] - end.to_h, + end, T::Hash[Integer, YARP::Comment], ) @@ -131,7 +131,7 @@ def on_constant_path_write(node) sig { params(node: YARP::Node).void } def extract_document_link(node) - comment = @comments[node.location.start_line - 1] + comment = @lines_to_comments[node.location.start_line - 1] return unless comment match = comment.location.slice.match(%r{source://.*#\d+$}) diff --git a/test/requests/document_link_expectations_test.rb b/test/requests/document_link_expectations_test.rb index b18319b6fc..0309325f55 100644 --- a/test/requests/document_link_expectations_test.rb +++ b/test/requests/document_link_expectations_test.rb @@ -26,7 +26,7 @@ def run_expectations(source) document = RubyLsp::Document.new(source: source, version: 1, uri: uri) emitter = RubyLsp::EventEmitter.new - listener = RubyLsp::Requests::DocumentLink.new(uri, emitter, message_queue, document.comments) + listener = RubyLsp::Requests::DocumentLink.new(uri, document.comments, emitter, message_queue) emitter.visit(document.tree) listener.response ensure From 0370d5decb5e64b4b957994132cf0d16746dcd41 Mon Sep 17 00:00:00 2001 From: Andy Waite Date: Mon, 18 Sep 2023 16:14:49 -0400 Subject: [PATCH 5/6] Fix argument order --- lib/ruby_lsp/executor.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ruby_lsp/executor.rb b/lib/ruby_lsp/executor.rb index 85cf18a0d1..81370072c5 100644 --- a/lib/ruby_lsp/executor.rb +++ b/lib/ruby_lsp/executor.rb @@ -97,7 +97,7 @@ def run(request) # Run listeners for the document emitter = EventEmitter.new document_symbol = Requests::DocumentSymbol.new(emitter, @message_queue) - document_link = Requests::DocumentLink.new(uri, emitter, document.comments, @message_queue) + document_link = Requests::DocumentLink.new(uri, document.comments, emitter, @message_queue) code_lens = Requests::CodeLens.new(uri, emitter, @message_queue, @test_library) semantic_highlighting = Requests::SemanticHighlighting.new(emitter, @message_queue) From a661b18b674faa61ba84b512e7a7cdeed530b0e6 Mon Sep 17 00:00:00 2001 From: Andy Waite Date: Mon, 18 Sep 2023 16:14:59 -0400 Subject: [PATCH 6/6] Re-enable DocumentLink integration test --- test/integration_test.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/test/integration_test.rb b/test/integration_test.rb index 9d0e8993b2..4487f7309d 100644 --- a/test/integration_test.rb +++ b/test/integration_test.rb @@ -149,7 +149,6 @@ def test_semantic_highlighting end def test_document_link - skip initialize_lsp(["documentLink"]) open_file_with(<<~DOC) # source://syntax_tree/#{Gem::Specification.find_by_name("syntax_tree").version}/lib/syntax_tree.rb#39