Migrate DocumentLink request to YARP #982

andyw8 · 2023-09-07T13:44:12Z

Motivation

Partially addresses #449

Implementation

The previous approach was based on parsing the document for comments, but with YARP we don't have a Comment node, so instead we visit the critical nodes and then check for attached comments.

Automated Tests

I added extra examples to the existing fixtures.

Manual Tests

lib/ruby_lsp/requests/document_link.rb

-      sig { params(uri: URI::Generic, emitter: EventEmitter, message_queue: Thread::Queue).void }
-      def initialize(uri, emitter, message_queue)
+      sig { params(uri: URI::Generic, emitter: EventEmitter, message_queue: Thread::Queue, document: Document).void }
+      def initialize(uri, emitter, message_queue, document)


andyw8 · 2023-09-13T14:20:50Z

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!


@vinistock do you think we should always attach the comments, or make it conditional for requests that need it?

Do we know what performance cost it could bring to large files? If it's not too significant, having comments always available could enable more extensions (e.g. go-to-definition for yard comments).

It could indeed have a performance cost because it basically analyzes the entire AST to attach the comments.

Given that the only request that needs comments is document link, which we plan on removing in the future anyway, I'm not sure we want to do it.

Tapioca always adds the magic source comment immediately above the class/module/constant/method, so we always know where to find it without paying this cost.

What do you think about avoiding it for now to get the most performance we can out of YARP?

In that case, we can pass the comments array to document link's initializer, turn the array into a hash of line_number => comment and then just access it directly when finding nodes.

I'll push a commit to show how that would look, then we can decide.

lib/ruby_lsp/requests/document_link.rb

andyw8 · 2023-09-18T19:34:21Z

lib/ruby_lsp/requests/document_link.rb

@@ -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(


Not sure what to name the instance variable, @lines_to_comments?

Sounds like a good name 👍

vinistock

Looks good! Just some minor adjustments

vinistock · 2023-09-18T19:43:13Z

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)


We've been putting extra arguments other than emitter and message_queue first. Can we put this after uri please?

vinistock · 2023-09-18T19:43:30Z

lib/ruby_lsp/requests/document_link.rb

@@ -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(


Sounds like a good name 👍

vinistock · 2023-09-18T19:44:02Z

lib/ruby_lsp/requests/document_link.rb

+          comments.map do |comment|
+            [comment.location.end_line, comment]
+          end.to_h,


I think we don't need the intermediate array.

Suggested change

comments.map do |comment|

[comment.location.end_line, comment]

end.to_h,

comments.to_h do |comment|

[comment.location.end_line, comment]

end,

Ah, nice, I forgot to_h can accept a block.

vinistock · 2023-09-18T19:58:11Z

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, document.comments, @message_queue)


The order doesn't look right here.

Fixed. And this highlighted that the integration test was being skipped, which I've re-enabled.

st0012 · 2023-09-18T19:58:40Z

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, document.comments, @message_queue)


I think the order of arguments is not right?

* Temporarily disable .github/workflows/lsp_check.yml * Temporarily disable some tests * Preparation for YARP migration * Migrate DocumentSymbol request to YARP * Migrate Hover request to YARP * Migrate PathCompletion request to YARP * Migrate Sorbet request to YARP * Migrate InlayHints request to YARP * Migrate ShowSyntaxTree request to YARP * Migrate SelectionRange request to YARP * Migrate Definition request to YARP * Migrate Diagnostics request to YARP * Migrate CodeActionResolve request to YARP * Update selection ranges expectations * Fix rebase discrepancies * Add YARP RBI annotations (#978) * Migrate code lens to yarp (#979) Migrate CodeLens to YARP * Update `SERVER_EXTENSIONS.md` for YARP (#984) Co-authored-by: Andy Waite <andyw8@users.noreply.github.com> * Update text references from Syntax Tree to YARP (#986) Co-authored-by: Andy Waite <andyw8@users.noreply.github.com> * Fix rebase discrepancies * Migrate folding range to YARP (#980) * Fix rebase discrepancies 3 * Migrate semantic highlighting to YARP (#1000) Co-authored-by: Alexandre Terrasa <Morriar@users.noreply.github.com> * Migrate document highlight to YARP (#1005) * Simplify block locals handling in semantic highlighting (#1011) * Fix latest breaking changes * Make folding range a listener (#1013) * Migrate DocumentLink request to YARP (#982) * Migrate DocumentLink to YARP * Only parse comments where needed * Fix type * PR feedback * Fix argument order * Re-enable DocumentLink integration test --------- Co-authored-by: Andy Waite <andyw8@users.noreply.github.com> * Re-enable CI (#1022) * Re-generate YARP RBIs * Re-enable tests * Fix typechecking errors * Use locations for creating document symbol entries --------- Co-authored-by: Andy Waite <andyw8@users.noreply.github.com> Co-authored-by: Andy Waite <13400+andyw8@users.noreply.github.com> Co-authored-by: Alexandre Terrasa <Morriar@users.noreply.github.com>

andyw8 changed the title ~~Migrate DocumentLink request to YARP~~ WIP: Migrate DocumentLink request to YARP Sep 7, 2023

andyw8 force-pushed the andyw8/migrate-document-link-to-yarp branch from 3933178 to 8090d76 Compare September 7, 2023 15:05

andyw8 mentioned this pull request Sep 7, 2023

Implement early adoption of YARP #449

Closed

29 tasks

andyw8 commented Sep 7, 2023

View reviewed changes

andyw8 force-pushed the andyw8/migrate-document-link-to-yarp branch 2 times, most recently from d7a6fdb to 9702676 Compare September 7, 2023 16:02

vinistock force-pushed the yarp-clean branch from 3c49936 to d7cf5a3 Compare September 8, 2023 19:50

andyw8 force-pushed the andyw8/migrate-document-link-to-yarp branch 2 times, most recently from dc4c184 to ed9bde7 Compare September 13, 2023 14:19

andyw8 commented Sep 13, 2023

View reviewed changes

andyw8 force-pushed the andyw8/migrate-document-link-to-yarp branch from ed9bde7 to d0e4379 Compare September 13, 2023 14:22

vinistock force-pushed the yarp-clean branch from 49b835e to dab68c0 Compare September 13, 2023 18:13

andyw8 force-pushed the andyw8/migrate-document-link-to-yarp branch from d0e4379 to 75178a4 Compare September 13, 2023 18:59

vinistock force-pushed the yarp-clean branch 2 times, most recently from c01556e to e4912b0 Compare September 18, 2023 15:00

andyw8 force-pushed the andyw8/migrate-document-link-to-yarp branch 2 times, most recently from 1cd1a00 to 933b7a1 Compare September 18, 2023 16:42

andyw8 changed the title ~~WIP: Migrate DocumentLink request to YARP~~ Migrate DocumentLink request to YARP Sep 18, 2023

andyw8 force-pushed the andyw8/migrate-document-link-to-yarp branch from 933b7a1 to 2002a4d Compare September 18, 2023 16:44

Migrate DocumentLink to YARP

7307e49

andyw8 force-pushed the andyw8/migrate-document-link-to-yarp branch from 2002a4d to 7307e49 Compare September 18, 2023 16:46

andyw8 marked this pull request as ready for review September 18, 2023 16:46

andyw8 requested a review from a team as a code owner September 18, 2023 16:46

andyw8 requested review from Morriar, KaanOzkan and vinistock September 18, 2023 16:46

st0012 reviewed Sep 18, 2023

View reviewed changes

lib/ruby_lsp/requests/document_link.rb Show resolved Hide resolved

andyw8 added 2 commits September 18, 2023 15:31

Only parse comments where needed

e142d49

Fix type

6744a8d

andyw8 commented Sep 18, 2023

View reviewed changes

vinistock approved these changes Sep 18, 2023

View reviewed changes

PR feedback

5e6da7b

vinistock reviewed Sep 18, 2023

View reviewed changes

st0012 reviewed Sep 18, 2023

View reviewed changes

andyw8 added 2 commits September 18, 2023 16:14

Fix argument order

0370d5d

Re-enable DocumentLink integration test

a661b18

andyw8 merged commit fb20c0b into yarp-clean Sep 18, 2023
21 checks passed

andyw8 deleted the andyw8/migrate-document-link-to-yarp branch September 18, 2023 20:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate DocumentLink request to YARP #982

Migrate DocumentLink request to YARP #982

andyw8 commented Sep 7, 2023 •

edited

This comment was marked as outdated.

andyw8 Sep 13, 2023

st0012 Sep 18, 2023

vinistock Sep 18, 2023

andyw8 Sep 18, 2023

andyw8 Sep 18, 2023

andyw8 Sep 18, 2023

vinistock Sep 18, 2023

vinistock left a comment

vinistock Sep 18, 2023

vinistock Sep 18, 2023

vinistock Sep 18, 2023

andyw8 Sep 18, 2023

vinistock Sep 18, 2023

andyw8 Sep 18, 2023

st0012 Sep 18, 2023

Migrate DocumentLink request to YARP #982

Migrate DocumentLink request to YARP #982

Conversation

andyw8 commented Sep 7, 2023 • edited

Motivation

Implementation

Automated Tests

Manual Tests

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinistock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyw8 commented Sep 7, 2023 •

edited