[LSP] Add support for providing definitions from the SymbolTable #1530

glatosinski · 2022-11-15T17:18:12Z

This PR is Work in Progress.

It adds support for creating a SymbolTable in the Language Server and utilizing it for finding definitions.

The current status is:

Currently, the workspace directory provided by the editor is used as VerilogProject root (however, emacs uses git repository root directory as the workspace directory, if available).

hzeller

Some first comments.

verilog/tools/ls/verilog_ls.cc

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

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

codecov-commenter · 2022-11-17T12:33:08Z

Codecov Report

Base: 93.00% // Head: 92.94% // Decreases project coverage by -0.07% ⚠️

Coverage data is based on head (cf04bf5) compared to base (0a1cf7e).
Patch coverage: 88.73% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1530      +/-   ##
==========================================
- Coverage   93.00%   92.94%   -0.07%     
==========================================
  Files         348      350       +2     
  Lines       24760    25030     +270     
==========================================
+ Hits        23028    23263     +235     
- Misses       1732     1767      +35

Impacted Files	Coverage Δ
verilog/analysis/symbol_table.cc	`98.24% <62.50%> (-0.31%)`	⬇️
verilog/tools/ls/verilog-language-server.cc	`94.59% <87.75%> (-3.96%)`	⬇️
verilog/tools/ls/symbol-table-handler.cc	`88.02% <88.02%> (ø)`
verilog/analysis/verilog_project.cc	`87.86% <100.00%> (+1.40%)`	⬆️
verilog/analysis/verilog_project.h	`95.74% <100.00%> (+1.00%)`	⬆️
verilog/tools/ls/lsp-parse-buffer.cc	`95.45% <100.00%> (+0.33%)`	⬆️
verilog/tools/ls/lsp-parse-buffer.h	`100.00% <100.00%> (ø)`
verilog/tools/ls/symbol-table-handler.h	`100.00% <100.00%> (ø)`
common/text/text_structure.cc	`97.18% <0.00%> (-1.06%)`	⬇️
verilog/formatting/align.cc	`94.47% <0.00%> (-0.14%)`	⬇️
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

verilog/analysis/verilog_project.cc

verilog/tools/ls/lsp-parse-buffer.h

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

verilog/tools/ls/verilog_ls.cc

hzeller

Some comments, mostly with focus on some style. It might look like a lot, but it is mostly minor.

Avoiding global complex destructing objects (e.g. use absl::string_view instead of std::string for global constant strings)
In general prefer absl::string_view to std::string if possible in particular for parameters but often in return values as well. This avoids unnecessary allocations and also provides a better way to trace where a particular string originates from.
Prefer insert() and find() to operator[] in maps. The latter does a lot more (e.g. creates a temporary object) that we usually not want (It is ok in containers such as vectors that only really return a reference to a particular pre-determined object; but map-like containers do more than we want).

An independent style-guide that we often use as reference is; it might not cover the above parts specifically, but in general it is worth-while reading
https://google.github.io/styleguide/cppguide.html

verilog/analysis/verilog_project.cc

verilog/tools/ls/lsp-parse-buffer.h

verilog/tools/ls/lsp-parse-buffer.cc

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

verilog/analysis/verilog_project.cc

verilog/analysis/verilog_project.h

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

verilog/analysis/verilog_project.cc

hzeller · 2022-12-13T10:21:01Z

What is the status here ? If you have finished a comment, is ok to Resolve conversation that particular comment, so that it is easier to see what is remaining.

You said that you'd like to put everything comprising the language server into an object. Are you working on that in a separate change ? It would be nice to get this in before this, as it then might simplify the integration here.

glatosinski · 2022-12-13T11:54:37Z

I am currently working on utilizing the FileList object to obtain the list of include files and incdirs using the verilog::FileList struct.

I have added initial lookup for the file with file list (currently I called it verible.filelist but if you have a better name I can quickly change it) up in the directory hierarchy (starting from workspace directory up to root directory).

I am currently testing the solution and once I get everything to work I will push the update.

I will also work today on wrapping the language server logic into an object - I will create a separate PR for it.

hzeller · 2023-01-06T23:45:02Z

Ready for another review, or are you still working on the comments ?

glatosinski · 2023-01-10T15:05:08Z

I changed the way symbol is searched in the SymbolTable - now the symbol is searched for in the reference components' tree, which is created during table building and resolving. It takes scope into consideration. I also added measuring textDocument/definition request handling time. I think we can do an another review.

…or textDocument/definition requests Signed-off-by: Grzegorz Latosinski <glatosinski@antmicro.com>

hzeller · 2023-01-25T19:20:43Z

verilog/tools/ls/lsp-parse-buffer.cc

-          if (change_listener_) change_listener_(uri, *tracker);
+          for (const auto &change_listener : change_listeners_) {
+            change_listener(uri, *tracker);
+          }
        } else {
          Remove(uri);


On called back with this callback, you store references to the text structure stored in tracker data.

But if the file gets removed, there is no change listener that informs about a URI being removed (the Remove(uri) below does not call any listeners)...

Which means in case the URI is deleted (someone closes the buffer in the editor) you'll store a dangeling reference to a removed text buffer in VerilogLanguageServer::UpdateEditedFileInProject() -> SymbolTableHandler::UpdateFileContent().

I suggest to have the change listener to take a pointer, which can inform about a nullptr buffer (i.e. inform about a removed URI), which you then can also use to properly clean up references.

Also, longer term it is probably better to have BufferTrackerown the ParsedVerilogSourceFile which we then pass to VerilogProject::UpdateFileContents(). Right now, due to the chain of change listeners, the ownership is quite murky and will result in issues.

verilog/analysis/verilog_project.cc

hzeller · 2023-01-25T21:07:59Z

The problem with the ./foo in the comment above can probably be fixed by canonicalizing the file coming from the filelist in symbol-table-handler.cc

 for (const auto &file_in_project : filelist.file_paths) {
    const std::string canoncialized = std::filesystem::path(file_in_project).lexically_normal().string();
   //... and use canonicalized instead of file_in_project below

hzeller · 2023-01-25T21:10:37Z

For your testing: have a file where going to definition opens another file. Then close the other file (which internally should remove it, but if the remove listener from above is not there will point dangeling data).
Attempt to go to definition again - that will now go to the wrong place and possibly crash things.

hzeller · 2023-01-25T21:22:41Z

The reacting to remove also would need to take into account that now we have to read the file from the filesystem again.

hzeller · 2023-01-26T02:17:38Z

Robustness testing: if there is one file in the filelist that has issues resolving symbols in the symbol table, no symbol can be resolved, even though other symbols might still be there, in particular in the file.

But in that case, the symbol resolving bails out with Could not find definition due to symbol table errors and then does not symbols that might be there.

This is important, because of course if you have 5000 files in the file-list (typical scale project), there will be one or more that have some issues parsing or producing symbols. That should not prevent us from the rest of the 'go to definition' working.

…for filelist file Signed-off-by: Grzegorz Latosinski <glatosinski@antmicro.com>

…erilogProject When "initialize" request does not send rootUri or rootPath, let's use either the current directory as project root or the first directory up in hierarchy that has filelist file Signed-off-by: Grzegorz Latosinski <glatosinski@antmicro.com>

…from filelist Signed-off-by: Grzegorz Latosinski <glatosinski@antmicro.com>

Signed-off-by: Grzegorz Latosinski <glatosinski@antmicro.com>

…he file in Language Server Signed-off-by: Grzegorz Latosinski <glatosinski@antmicro.com>

…osed/unopened files Signed-off-by: Grzegorz Latosinski <glatosinski@antmicro.com>

hzeller · 2023-01-26T16:29:42Z

With my VSCode that does not set the root path and URI the new logic that derives things from the file-list is kicking in (i really like that btw. I think that is a good fallback).

It does seem that it got confused in my testcase and attempts to resolve files relative to the filelist, not the filelist's directory - probably due to the dot in the filelist directory ? (/foo/bar/./verible.fileliist). Maybe some round of lexically_normal() needed somewhere (and some test that covers this use-case).

Verible Verilog Language Server built at v0.0-2644-g287db0e6
W0126 08:24:01.943099   18703 verilog-language-server.cc:178] Could not configure Verilog Project root
I0126 08:24:01.943285   18703 symbol-table-handler.cc:53] Found file list under /home/hzeller/tt/./verible.filelist
I0126 08:24:01.943310   18703 symbol-table-handler.cc:96] LoadProjectFileList
I0126 08:24:01.943339   18703 symbol-table-handler.cc:53] Found file list under /home/hzeller/tt/./verible.filelist
I0126 08:24:01.943343   18703 symbol-table-handler.cc:106] Found file list under /home/hzeller/tt/./verible.filelist
I0126 08:24:01.943350   18703 symbol-table-handler.cc:116] Updating the filelist
I0126 08:24:01.943464   18703 symbol-table-handler.cc:132] Adding "/home/hzeller/tt/." to include directories
I0126 08:24:01.943469   18703 symbol-table-handler.cc:135] Adding include path:  .
W0126 08:24:01.943493   18703 symbol-table-handler.cc:145] File included in /home/hzeller/tt/./verible.filelist not found:  bar.sv:  NOT_FOUND: /home/hzeller/tt/verible.filelist/bar.sv: Not a directory
W0126 08:24:01.943512   18703 symbol-table-handler.cc:145] File included in /home/hzeller/tt/./verible.filelist not found:  foo.sv:  NOT_FOUND: /home/hzeller/tt/verible.filelist/foo.sv: Not a directory
E0126 08:24:01.974014   18703 json-rpc-dispatcher.cc:58] Unhandled notification 'initialized'

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

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

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

…terals Signed-off-by: Grzegorz Latosinski <glatosinski@antmicro.com>

…r can be nullptr in modification callback Signed-off-by: Grzegorz Latosinski <glatosinski@antmicro.com>

…importing Signed-off-by: Grzegorz Latosinski <glatosinski@antmicro.com>

…irectory Signed-off-by: Grzegorz Latosinski <glatosinski@antmicro.com>

hzeller · 2023-01-26T21:06:33Z

Maybe to reduce the impact of 'there is a single issue somewhere in building the symbol table and therefore we can't resolve any symbols'-issue there could be a quick fix for now: always build the symbol table for a single file, the current one, and try to resolve a symbol there (I suspect that is very common).
Only if the symbol is not defined there, start building a symbol table for the whole project.

(which also will help performance: in my case, building the symbol table for a single file takes a few 100 microseconds, while building for the whole project takes about 40 seconds).

Longer term we then can refine things of course: ignore issues in some files but at least build partial symbol table, incremental update etc.

…te errors in symbol table Since there can be files that have errors in them, it is not a good idea to stop go-to definition request due to errors in the symbol table. This commit removes errors and halting of textDocument/definition request on Build and Resolve errors. Instead, it prints warnings and allow functions to continue Signed-off-by: Grzegorz Latosinski <glatosinski@antmicro.com>

…or textDocument/definition requests Signed-off-by: Grzegorz Latosinski <glatosinski@antmicro.com>

hzeller

LGTM

hzeller · 2023-01-27T00:15:13Z

verilog/analysis/verilog_project.cc

+    contents = std::make_unique<VerilogSourceFile>(projectpath, path, "");
+  auto fileptr = files_.find(projectpath);
+  if (fileptr == files_.end()) {
+    files_.insert(std::make_pair(projectpath, std::move(contents)));


For later: Probably files_.emplace() might be easier here.

hzeller · 2023-01-27T00:23:36Z

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

+  return true;
+}
+
+const SymbolTableNode *ScanReferenceComponents(


For future consideration: if scanning takes a while, maybe we need to build an index in the symbol table.

glatosinski force-pushed the lsp-symbol-table-support branch 3 times, most recently from cd94b3d to 8546e27 Compare November 15, 2022 17:55

hzeller reviewed Nov 16, 2022

View reviewed changes

glatosinski force-pushed the lsp-symbol-table-support branch from 84bcca0 to 339bbb1 Compare November 17, 2022 11:47

glatosinski force-pushed the lsp-symbol-table-support branch 2 times, most recently from f6a2016 to 69439b1 Compare November 29, 2022 13:36

hzeller reviewed Dec 5, 2022

View reviewed changes

glatosinski force-pushed the lsp-symbol-table-support branch from 9f544ba to bd48c7d Compare December 5, 2022 17:32

hzeller reviewed Dec 6, 2022

View reviewed changes

glatosinski force-pushed the lsp-symbol-table-support branch from bd48c7d to 9c3f2a6 Compare December 6, 2022 14:58

hzeller reviewed Dec 6, 2022

View reviewed changes

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

hzeller reviewed Dec 7, 2022

View reviewed changes

verilog/analysis/verilog_project.cc Show resolved Hide resolved

glatosinski force-pushed the lsp-symbol-table-support branch 2 times, most recently from 3827733 to d9bdf2a Compare December 8, 2022 13:55

glatosinski mentioned this pull request Dec 13, 2022

[LSP] Wrap implementation of the Language Server in a class #1560

Merged

glatosinski changed the title ~~[LSP] Add support for providing definitions from the SymbolTable~~ [LSP] [WIP] Add support for providing definitions from the SymbolTable Dec 15, 2022

glatosinski force-pushed the lsp-symbol-table-support branch 4 times, most recently from 1c4bb75 to 42dd666 Compare December 21, 2022 18:11

glatosinski force-pushed the lsp-symbol-table-support branch from 42dd666 to ffd76df Compare January 10, 2023 14:39

glatosinski changed the title ~~[LSP] [WIP] Add support for providing definitions from the SymbolTable~~ [LSP] Add support for providing definitions from the SymbolTable Jan 10, 2023

glatosinski force-pushed the lsp-symbol-table-support branch from ffd76df to 916c28f Compare January 10, 2023 15:01

glatosinski marked this pull request as ready for review January 10, 2023 15:02

verilog: tools: ls: verilog-language-server: test: Added more tests f…

28ea6d3

…or textDocument/definition requests Signed-off-by: Grzegorz Latosinski <glatosinski@antmicro.com>

hzeller reviewed Jan 25, 2023

View reviewed changes

verilog/analysis/verilog_project.cc Outdated Show resolved Hide resolved

glatosinski added 6 commits January 26, 2023 13:44

verilog: tools: ls: symbol-table-handler: Separated function looking …

5c56ae9

…for filelist file Signed-off-by: Grzegorz Latosinski <glatosinski@antmicro.com>

verilog: tools: ls: symbol-table-handler: Added canonicalizing paths …

d449041

…from filelist Signed-off-by: Grzegorz Latosinski <glatosinski@antmicro.com>

verilog: analysis: verilog_project: Simplified file update functions

d7d980e

Signed-off-by: Grzegorz Latosinski <glatosinski@antmicro.com>

verilog: tools: ls: verilog-language-server: Added handling closing t…

0f4a18c

…he file in Language Server Signed-off-by: Grzegorz Latosinski <glatosinski@antmicro.com>

verilog: tools: ls: verilog-language-server: test: Added tests for cl…

e617e89

…osed/unopened files Signed-off-by: Grzegorz Latosinski <glatosinski@antmicro.com>

hzeller reviewed Jan 26, 2023

View reviewed changes

verilog/tools/ls/verilog-language-server_test.cc Outdated Show resolved Hide resolved

hzeller reviewed Jan 26, 2023

View reviewed changes

verilog/tools/ls/verilog-language-server_test.cc Outdated Show resolved Hide resolved

hzeller reviewed Jan 26, 2023

View reviewed changes

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

hzeller reviewed Jan 26, 2023

View reviewed changes

verilog/tools/ls/verilog-language-server.cc Outdated Show resolved Hide resolved

glatosinski added 4 commits January 26, 2023 21:44

verilog: tools: ls: tests: Replaced multi-line strings with string li…

9fafb08

…terals Signed-off-by: Grzegorz Latosinski <glatosinski@antmicro.com>

verilog: tools: ls: lsp-parse-buffer: Added mention that BufferTracke…

94c67b5

…r can be nullptr in modification callback Signed-off-by: Grzegorz Latosinski <glatosinski@antmicro.com>

verilog: tools: ls: verilog-language-server: test: Removed namespace …

c54b107

…importing Signed-off-by: Grzegorz Latosinski <glatosinski@antmicro.com>

verilog: tools: ls: verilog-language-server: Fixed getting FileList d…

252e81e

…irectory Signed-off-by: Grzegorz Latosinski <glatosinski@antmicro.com>

glatosinski added 2 commits January 27, 2023 00:42

verilog: tools: ls: verilog-language-server: test: Added more tests f…

cf04bf5

…or textDocument/definition requests Signed-off-by: Grzegorz Latosinski <glatosinski@antmicro.com>

glatosinski marked this pull request as ready for review January 27, 2023 00:07

glatosinski requested a review from hzeller January 27, 2023 00:08

hzeller approved these changes Jan 27, 2023

View reviewed changes

hzeller merged commit 286e27c into chipsalliance:master Jan 27, 2023

hzeller mentioned this pull request Jan 27, 2023

Language Server: implement textDocument/definition , textDocument/declaration, textDocument/references #1189

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LSP] Add support for providing definitions from the SymbolTable #1530

[LSP] Add support for providing definitions from the SymbolTable #1530

glatosinski commented Nov 15, 2022 •

edited

Loading

hzeller left a comment

codecov-commenter commented Nov 17, 2022 •

edited

Loading

hzeller left a comment

hzeller commented Dec 13, 2022

glatosinski commented Dec 13, 2022

hzeller commented Jan 6, 2023

glatosinski commented Jan 10, 2023

hzeller Jan 25, 2023

hzeller commented Jan 25, 2023

hzeller commented Jan 25, 2023

hzeller commented Jan 25, 2023

hzeller commented Jan 26, 2023

hzeller commented Jan 26, 2023

hzeller commented Jan 26, 2023

hzeller left a comment

hzeller Jan 27, 2023

hzeller Jan 27, 2023

[LSP] Add support for providing definitions from the SymbolTable #1530

[LSP] Add support for providing definitions from the SymbolTable #1530

Conversation

glatosinski commented Nov 15, 2022 • edited Loading

hzeller left a comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 17, 2022 • edited Loading

Codecov Report

hzeller left a comment

Choose a reason for hiding this comment

hzeller commented Dec 13, 2022

glatosinski commented Dec 13, 2022

hzeller commented Jan 6, 2023

glatosinski commented Jan 10, 2023

hzeller Jan 25, 2023

Choose a reason for hiding this comment

hzeller commented Jan 25, 2023

hzeller commented Jan 25, 2023

hzeller commented Jan 25, 2023

hzeller commented Jan 26, 2023

hzeller commented Jan 26, 2023

hzeller commented Jan 26, 2023

hzeller left a comment

Choose a reason for hiding this comment

hzeller Jan 27, 2023

Choose a reason for hiding this comment

hzeller Jan 27, 2023

Choose a reason for hiding this comment

glatosinski commented Nov 15, 2022 •

edited

Loading

codecov-commenter commented Nov 17, 2022 •

edited

Loading