[LSP] Added Symbol Renaming to the language server #1948

jbylicki · 2023-06-15T08:14:30Z

This PR implements functionality for textDocument/prepareRename and textDocument/rename capabilities.

codecov-commenter · 2023-06-15T11:13:31Z

Codecov Report

Patch coverage: 86.74% and project coverage change: -0.04 ⚠️

Comparison is base (ddcea37) 92.84% compared to head (8e909d9) 92.80%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1948      +/-   ##
==========================================
- Coverage   92.84%   92.80%   -0.04%     
==========================================
  Files         355      355              
  Lines       26113    26206      +93     
==========================================
+ Hits        24245    24321      +76     
- Misses       1868     1885      +17

Impacted Files	Coverage Δ
verilog/tools/ls/symbol-table-handler.h	`100.00% <ø> (ø)`
verilog/tools/ls/symbol-table-handler.cc	`88.64% <85.33%> (-1.26%)`	⬇️
verilog/tools/ls/verilog-language-server.cc	`94.30% <100.00%> (+0.39%)`	⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

hzeller

Exciting, this will be a very cool feature!
A few first comments.

hzeller · 2023-06-15T22:35:34Z

verilog/tools/ls/verilog-language-server_test.cc

+  int character;
+};
+
+std::string RenameRequest(RenameRequestParams params) {


Instead of manually converting things to json, can you use the generated verible::lsp:: structs to fill and then just use the to_json() to convert it ?

Also we wouldn't need this RenameRequestParams struct here, we can directly use verible::lsp::PrepareRenameParams and verible::lsp::RenameParams.

The PrepareRenameRequest() and RenameRequest() functions then can take a verible::lsp::TextDocumentPositionParams.

I hope that longer term, we can mostly use these structs here and have our EXPECT_EQ() and stuff also look into these instead of manually poking in json arrays. This will be a cleanup PR at some point, but if we can already have new code with that in mind would be good.

I have changed the methods such that they place the (Prepare)RenameParams into a prepared snippet of JSON. Using to_json() didn't make sense here, as it dumps the contents of the struct only, however the request itself also requres an ID, method and other boilerplate. Methods built into the tests already use this approach (such as ReferencesRequest() and I have found no logical way to construct a full request in any other manner.

common/lsp/lsp-protocol.yaml

verilog/tools/ls/symbol-table-handler.cc

verilog/tools/ls/verilog-language-server_test.cc

hzeller · 2023-06-15T23:42:39Z

verilog/tools/ls/verilog-language-server_test.cc

+  EXPECT_EQ(response["result"]["changes"][params.file].size(), 3)
+      << "Invalid result size for id:  ";
+}
+


Additional possible tests: put the cursor over the definition of a symbol in one test and over one reference of a symbol. In both cases, the definition and all references should be updated.

Also other corner cases

symbol defined in a package ? Making sure that the same symbolname ('foo') defined in two different packages is not cross-renaming the other package symbol PackageA::foo and PackageB::foo

The first test: this is known to be broken and expected to be fixed by #1678 (as well as the duplication of positions with module names); I will add the test and remove duplicate finding once that PR gets merged, if that is okay.

As for the second one: added it and it works as expected.

verilog/tools/ls/verilog-language-server_test.cc

verilog/tools/ls/symbol-table-handler.h

verilog/tools/ls/symbol-table-handler.cc

Added the support to respond to prepareRename requests for the LSP, such that it provides locations for the naming changes that will be made once a proper rename will be sent Signed-off-by: Jan Bylicki <[email protected]>

Added protocol implementation for rename capability. No actual action will be taken but in testing there are no errors editor-side by now. Signed-off-by: Jan Bylicki <[email protected]>

Added an option to do a textDocument/rename LSP operation that will locate and send ranges for the editor to change. Signed-off-by: Jan Bylicki <[email protected]>

Signed-off-by: Jan Bylicki <[email protected]>

…name spec Signed-off-by: Jan Bylicki <[email protected]>

… issues Signed-off-by: Jan Bylicki <[email protected]>

Signed-off-by: Jan Bylicki <[email protected]>

I have added a fix that checks for duplicates in replace positions. That fix should be redundant once a PR fixing go-to-definition works, as stated in a comment in this commit Signed-off-by: Jan Bylicki <[email protected]>

Signed-off-by: Jan Bylicki <[email protected]>

Added a rename test that checks whether renaming across files works Signed-off-by: Jan Bylicki <[email protected]>

Signed-off-by: Jan Bylicki <[email protected]>

verilog/tools/ls/symbol-table-handler.cc

verilog/tools/ls/symbol-table-handler.h

verilog/tools/ls/symbol-table-handler.cc

verilog/tools/ls/symbol-table-handler_test.cc

Signed-off-by: Jan Bylicki <[email protected]>

hzeller · 2023-07-11T00:12:11Z

Looks pretty good. If you rebase, the symbol table handler has a little failure, that would probably need to be addressed.

The above coverage tool claims that only 86% of the changes have coverage, are there missed test opportunities ?

hzeller

Ready to submit after rebase.

hzeller · 2024-01-30T07:49:13Z

Rebased, then merged as #2081

jbylicki force-pushed the lsp-add-symbol-rename branch 3 times, most recently from 3a47eda to f523773 Compare June 15, 2023 10:54

jbylicki requested a review from hzeller June 15, 2023 11:49

hzeller reviewed Jun 15, 2023

View reviewed changes

jbylicki force-pushed the lsp-add-symbol-rename branch from feb3e8e to b20a9c3 Compare June 16, 2023 15:21

hzeller reviewed Jun 16, 2023

View reviewed changes

verilog/tools/ls/symbol-table-handler.cc Outdated Show resolved Hide resolved

jbylicki force-pushed the lsp-add-symbol-rename branch 3 times, most recently from 0c5ebc9 to 188c90e Compare June 19, 2023 11:40

jbylicki added 8 commits June 19, 2023 13:44

ls: Added prepareRename support for language server

b74c807

Added the support to respond to prepareRename requests for the LSP, such that it provides locations for the naming changes that will be made once a proper rename will be sent Signed-off-by: Jan Bylicki <[email protected]>

lsp: Added barebones rename capability

2c756c5

Added protocol implementation for rename capability. No actual action will be taken but in testing there are no errors editor-side by now. Signed-off-by: Jan Bylicki <[email protected]>

lsp: Added basic rename functionality

132ed2d

Added an option to do a textDocument/rename LSP operation that will locate and send ranges for the editor to change. Signed-off-by: Jan Bylicki <[email protected]>

symbol-table-handler: Added multi-file capabilities to rename

f42138f

Signed-off-by: Jan Bylicki <[email protected]>

verilog-language-server_test: Added rename test

90fff3e

Signed-off-by: Jan Bylicki <[email protected]>

symbol-table-handler: Fixed FindReferenceLocations to match prepareRe…

0e16ca7

…name spec Signed-off-by: Jan Bylicki <[email protected]>

verilog-language-server_test: Changed pathing to fix windows matching…

681bd64

… issues Signed-off-by: Jan Bylicki <[email protected]>

verilog-language-server_test: Moved request creation to LSP structs

b4e16f4

Signed-off-by: Jan Bylicki <[email protected]>

jbylicki force-pushed the lsp-add-symbol-rename branch 6 times, most recently from c795f14 to dbdcbba Compare June 19, 2023 14:04

jbylicki marked this pull request as draft June 19, 2023 14:47

jbylicki force-pushed the lsp-add-symbol-rename branch 4 times, most recently from f04e748 to 66fe077 Compare June 20, 2023 10:03

jbylicki force-pushed the lsp-add-symbol-rename branch from 8936233 to 5161a26 Compare June 20, 2023 12:45

jbylicki added 4 commits June 20, 2023 14:50

lsp: Fixed module instantiation renaming

0c64272

I have added a fix that checks for duplicates in replace positions. That fix should be redundant once a PR fixing go-to-definition works, as stated in a comment in this commit Signed-off-by: Jan Bylicki <[email protected]>

verilog-language-server_test: Added cross-package renaming test

667ed7e

Signed-off-by: Jan Bylicki <[email protected]>

verilog-language-server_test: Added multi-file rename test

90a0cb7

Added a rename test that checks whether renaming across files works Signed-off-by: Jan Bylicki <[email protected]>

symbol-table-handler: Added optional nullptr return from prepareRename

68ec505

Signed-off-by: Jan Bylicki <[email protected]>

jbylicki force-pushed the lsp-add-symbol-rename branch 5 times, most recently from b74fcdf to 861087e Compare June 23, 2023 09:23

symbol-table-handler_test: Added handler tests for Find* methods

2a1ec3e

Signed-off-by: Jan Bylicki <[email protected]>

jbylicki force-pushed the lsp-add-symbol-rename branch from 861087e to 2a1ec3e Compare June 23, 2023 09:54

jbylicki marked this pull request as ready for review June 23, 2023 11:12

jbylicki requested a review from hzeller June 23, 2023 11:12

mglb suggested changes Jul 3, 2023

View reviewed changes

jbylicki added 2 commits July 3, 2023 17:16

symbol-table-handler: Added docstrings and changed location typing

2391058

Signed-off-by: Jan Bylicki <[email protected]>

symbol-table-handler_test: Added const to sources_dir declarations

8e909d9

Signed-off-by: Jan Bylicki <[email protected]>

jbylicki force-pushed the lsp-add-symbol-rename branch from cb9e063 to 8e909d9 Compare July 3, 2023 15:26

mglb self-requested a review July 4, 2023 13:11

mglb approved these changes Jul 4, 2023

View reviewed changes

hzeller approved these changes Jul 11, 2023

View reviewed changes

hzeller mentioned this pull request Jan 28, 2024

Add Symbol Renaming to the language server #2081

Merged

hzeller closed this Jan 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LSP] Added Symbol Renaming to the language server #1948

[LSP] Added Symbol Renaming to the language server #1948

jbylicki commented Jun 15, 2023

codecov-commenter commented Jun 15, 2023 •

edited

Loading

hzeller left a comment

hzeller Jun 15, 2023

jbylicki Jun 19, 2023

hzeller Jun 15, 2023

jbylicki Jun 19, 2023

hzeller commented Jul 11, 2023

hzeller left a comment

hzeller commented Jan 30, 2024

[LSP] Added Symbol Renaming to the language server #1948

[LSP] Added Symbol Renaming to the language server #1948

Conversation

jbylicki commented Jun 15, 2023

codecov-commenter commented Jun 15, 2023 • edited Loading

Codecov Report

hzeller left a comment

Choose a reason for hiding this comment

hzeller Jun 15, 2023

Choose a reason for hiding this comment

jbylicki Jun 19, 2023

Choose a reason for hiding this comment

hzeller Jun 15, 2023

Choose a reason for hiding this comment

jbylicki Jun 19, 2023

Choose a reason for hiding this comment

hzeller commented Jul 11, 2023

hzeller left a comment

Choose a reason for hiding this comment

hzeller commented Jan 30, 2024

codecov-commenter commented Jun 15, 2023 •

edited

Loading