From b74c807aa3b0b1669996e8de32ba04e2d351f432 Mon Sep 17 00:00:00 2001 From: Jan Bylicki Date: Mon, 15 May 2023 13:42:08 +0200 Subject: [PATCH 01/15] ls: Added prepareRename support for language server 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 --- common/lsp/lsp-protocol.yaml | 7 ++++++ verilog/tools/ls/symbol-table-handler.cc | 28 +++++++++++++++++++++ verilog/tools/ls/symbol-table-handler.h | 3 +++ verilog/tools/ls/verilog-language-server.cc | 6 +++++ 4 files changed, 44 insertions(+) diff --git a/common/lsp/lsp-protocol.yaml b/common/lsp/lsp-protocol.yaml index a843c7933..94e504baf 100644 --- a/common/lsp/lsp-protocol.yaml +++ b/common/lsp/lsp-protocol.yaml @@ -202,3 +202,10 @@ DocumentLinkParams: DocumentLink: range: Range target?: string # DocumentUri + +# -- textDocument/prepareRename (TODO(jbylicki): check that requires project + active symbol table #1189) +PrepareRenameParams: + <: TextDocumentPositionParams + +# Response: Range[] + diff --git a/verilog/tools/ls/symbol-table-handler.cc b/verilog/tools/ls/symbol-table-handler.cc index 7bb81f179..2700296dc 100644 --- a/verilog/tools/ls/symbol-table-handler.cc +++ b/verilog/tools/ls/symbol-table-handler.cc @@ -317,6 +317,34 @@ std::vector SymbolTableHandler::FindReferencesLocations( return locations; } +std::vector SymbolTableHandler::FindRenameLocations( + const verible::lsp::PrepareRenameParams ¶ms, + const verilog::BufferTrackerContainer &parsed_buffers) { + Prepare(); + absl::string_view symbol = + GetTokenAtTextDocumentPosition(params, parsed_buffers); + const SymbolTableNode &root = symbol_table_->Root(); + const SymbolTableNode *node = ScanSymbolTreeForDefinition(&root, symbol); + if (!node) { + LOG(WARNING) << "NODE empty: " << symbol << "\n"; + return {}; + } + std::vector locations; + std::vector ranges; + CollectReferences(&root, node, &locations); + if (locations.empty()) { + LOG(WARNING) << "locations empty\n"; + return {}; + } + for (auto &loc : locations) { + ranges.push_back(loc.range); + LOG(INFO) << "range: " << loc.range.start.line << " " + << loc.range.start.character << " " << loc.range.end.line << " " + << loc.range.end.character << "\n"; + } + return ranges; +} + void SymbolTableHandler::CollectReferencesReferenceComponents( const ReferenceComponentNode *ref, const SymbolTableNode *ref_origin, const SymbolTableNode *definition_node, diff --git a/verilog/tools/ls/symbol-table-handler.h b/verilog/tools/ls/symbol-table-handler.h index b80889a2b..0c3ae0dd9 100644 --- a/verilog/tools/ls/symbol-table-handler.h +++ b/verilog/tools/ls/symbol-table-handler.h @@ -65,6 +65,9 @@ class SymbolTableHandler { const verible::lsp::ReferenceParams ¶ms, const verilog::BufferTrackerContainer &parsed_buffers); + std::vector FindRenameLocations( + const verible::lsp::PrepareRenameParams ¶ms, + const verilog::BufferTrackerContainer &parsed_buffers); // Provide new parsed content for the given path. If "content" is nullptr, // opens the given file instead. void UpdateFileContent(absl::string_view path, diff --git a/verilog/tools/ls/verilog-language-server.cc b/verilog/tools/ls/verilog-language-server.cc index 0af2d0f08..f5d29f83e 100644 --- a/verilog/tools/ls/verilog-language-server.cc +++ b/verilog/tools/ls/verilog-language-server.cc @@ -69,6 +69,7 @@ verible::lsp::InitializeResult VerilogLanguageServer::GetCapabilities() { {"documentHighlightProvider", true}, // Highlight same symbol {"definitionProvider", true}, // Provide going to definition {"referencesProvider", true}, // Provide going to references + {"renameProvider", true}, // Provide symbol renaming {"diagnosticProvider", // Pull model of diagnostics. { {"interFileDependencies", false}, @@ -138,6 +139,11 @@ void VerilogLanguageServer::SetRequestHandlers() { return symbol_table_handler_.FindReferencesLocations(p, parsed_buffers_); }); + dispatcher_.AddRequestHandler( + "textDocument/prepareRename", + [this](const verible::lsp::PrepareRenameParams &p) { + return symbol_table_handler_.FindRenameLocations(p, parsed_buffers_); + }); // The client sends a request to shut down. Use that to exit our loop. dispatcher_.AddRequestHandler("shutdown", [this](const nlohmann::json &) { shutdown_requested_ = true; From 2c756c52ffba9414c3153f3aca7297e18d09f1da Mon Sep 17 00:00:00 2001 From: Jan Bylicki Date: Mon, 15 May 2023 14:02:08 +0200 Subject: [PATCH 02/15] lsp: Added barebones rename capability 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 --- common/lsp/lsp-protocol.yaml | 7 +++++++ verilog/tools/ls/verilog-language-server.cc | 7 +++++++ 2 files changed, 14 insertions(+) diff --git a/common/lsp/lsp-protocol.yaml b/common/lsp/lsp-protocol.yaml index 94e504baf..aec19c413 100644 --- a/common/lsp/lsp-protocol.yaml +++ b/common/lsp/lsp-protocol.yaml @@ -209,3 +209,10 @@ PrepareRenameParams: # Response: Range[] +# -- textDocument/rename (TODO(jbylicki): check that requires project + active symbol table #1189) +RenameParams: + <: TextDocumentPositionParams + newName: string + +# Response: Range[] + diff --git a/verilog/tools/ls/verilog-language-server.cc b/verilog/tools/ls/verilog-language-server.cc index f5d29f83e..b1b701c40 100644 --- a/verilog/tools/ls/verilog-language-server.cc +++ b/verilog/tools/ls/verilog-language-server.cc @@ -144,6 +144,13 @@ void VerilogLanguageServer::SetRequestHandlers() { [this](const verible::lsp::PrepareRenameParams &p) { return symbol_table_handler_.FindRenameLocations(p, parsed_buffers_); }); + dispatcher_.AddRequestHandler( + "textDocument/rename", + [this](const verible::lsp::RenameParams &p) { + //TODO(jbylicki): implement rename based on locations from FindRenameLocations + // return symbol_table_handler_.FindRenameLocations(p, parsed_buffers_); + return std::vector(); + }); // The client sends a request to shut down. Use that to exit our loop. dispatcher_.AddRequestHandler("shutdown", [this](const nlohmann::json &) { shutdown_requested_ = true; From 132ed2db217fa35bad48384f15cbf68647e1d61e Mon Sep 17 00:00:00 2001 From: Jan Bylicki Date: Tue, 16 May 2023 12:31:40 +0200 Subject: [PATCH 03/15] lsp: Added basic rename functionality 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 --- verilog/tools/ls/symbol-table-handler.cc | 42 +++++++++++++++------ verilog/tools/ls/symbol-table-handler.h | 13 ++++--- verilog/tools/ls/verilog-language-server.cc | 8 ++-- 3 files changed, 42 insertions(+), 21 deletions(-) diff --git a/verilog/tools/ls/symbol-table-handler.cc b/verilog/tools/ls/symbol-table-handler.cc index 2700296dc..ad872cb0f 100644 --- a/verilog/tools/ls/symbol-table-handler.cc +++ b/verilog/tools/ls/symbol-table-handler.cc @@ -21,6 +21,7 @@ #include "absl/time/clock.h" #include "absl/time/time.h" #include "common/lsp/lsp-file-utils.h" +#include "common/lsp/lsp-protocol.h" #include "common/strings/line_column_map.h" #include "common/util/file_util.h" #include "common/util/range.h" @@ -325,26 +326,45 @@ std::vector SymbolTableHandler::FindRenameLocations( GetTokenAtTextDocumentPosition(params, parsed_buffers); const SymbolTableNode &root = symbol_table_->Root(); const SymbolTableNode *node = ScanSymbolTreeForDefinition(&root, symbol); - if (!node) { - LOG(WARNING) << "NODE empty: " << symbol << "\n"; - return {}; - } + if (!node) return {}; std::vector locations; std::vector ranges; CollectReferences(&root, node, &locations); - if (locations.empty()) { - LOG(WARNING) << "locations empty\n"; - return {}; - } + if (locations.empty()) return {}; + ranges.reserve(locations.size()); for (auto &loc : locations) { ranges.push_back(loc.range); - LOG(INFO) << "range: " << loc.range.start.line << " " - << loc.range.start.character << " " << loc.range.end.line << " " - << loc.range.end.character << "\n"; } return ranges; } +verible::lsp::WorkspaceEdit +SymbolTableHandler::FindRenameLocationsAndCreateEdits( + const verible::lsp::RenameParams ¶ms, + const verilog::BufferTrackerContainer &parsed_buffers) { + Prepare(); + absl::string_view symbol = + GetTokenAtTextDocumentPosition(params, parsed_buffers); + const SymbolTableNode &root = symbol_table_->Root(); + const SymbolTableNode *node = ScanSymbolTreeForDefinition(&root, symbol); + if (!node) return {}; + std::vector locations; + std::vector textedits; + CollectReferences(&root, node, &locations); + if (locations.empty()) return {}; + textedits.reserve(locations.size()); + for (auto &loc : locations) { + textedits.push_back(verible::lsp::TextEdit({ + .range = loc.range, + .newText = params.newName, + })); + } + files_dirty_ = true; + return verible::lsp::WorkspaceEdit{ + .changes = {{locations[0].uri, textedits}}, + }; +} + void SymbolTableHandler::CollectReferencesReferenceComponents( const ReferenceComponentNode *ref, const SymbolTableNode *ref_origin, const SymbolTableNode *definition_node, diff --git a/verilog/tools/ls/symbol-table-handler.h b/verilog/tools/ls/symbol-table-handler.h index 0c3ae0dd9..c5a2a817e 100644 --- a/verilog/tools/ls/symbol-table-handler.h +++ b/verilog/tools/ls/symbol-table-handler.h @@ -73,6 +73,10 @@ class SymbolTableHandler { void UpdateFileContent(absl::string_view path, const verible::TextStructureView *content); + verible::lsp::WorkspaceEdit FindRenameLocationsAndCreateEdits( + const verible::lsp::RenameParams ¶ms, + const verilog::BufferTrackerContainer &parsed_buffers); + // Creates a symbol table for entire project (public: needed in unit-test) std::vector BuildProjectSymbolTable(); @@ -115,9 +119,8 @@ class SymbolTableHandler { const SymbolTableNode *definition_node, std::vector *references); - // Looks for verible.filelist file down in directory structure and loads data - // to project. - // It is meant to be executed once per VerilogProject setup + // Looks for verible.filelist file down in directory structure and loads + // data to project. It is meant to be executed once per VerilogProject setup bool LoadProjectFileList(absl::string_view current_dir); // Parse all the files in the project. @@ -126,8 +129,8 @@ class SymbolTableHandler { // Path to the filelist file for the project std::string filelist_path_; - // Last timestamp of filelist file - used to check whether SymbolTable should - // be updated + // Last timestamp of filelist file - used to check whether SymbolTable + // should be updated absl::optional last_filelist_update_; // tells that symbol table should be rebuilt due to changes in files diff --git a/verilog/tools/ls/verilog-language-server.cc b/verilog/tools/ls/verilog-language-server.cc index b1b701c40..3617c5c05 100644 --- a/verilog/tools/ls/verilog-language-server.cc +++ b/verilog/tools/ls/verilog-language-server.cc @@ -145,11 +145,9 @@ void VerilogLanguageServer::SetRequestHandlers() { return symbol_table_handler_.FindRenameLocations(p, parsed_buffers_); }); dispatcher_.AddRequestHandler( - "textDocument/rename", - [this](const verible::lsp::RenameParams &p) { - //TODO(jbylicki): implement rename based on locations from FindRenameLocations - // return symbol_table_handler_.FindRenameLocations(p, parsed_buffers_); - return std::vector(); + "textDocument/rename", [this](const verible::lsp::RenameParams &p) { + return symbol_table_handler_.FindRenameLocationsAndCreateEdits( + p, parsed_buffers_); }); // The client sends a request to shut down. Use that to exit our loop. dispatcher_.AddRequestHandler("shutdown", [this](const nlohmann::json &) { From f42138f83d509ddcccce1230dd604416173e6e64 Mon Sep 17 00:00:00 2001 From: Jan Bylicki Date: Tue, 6 Jun 2023 12:45:48 +0200 Subject: [PATCH 04/15] symbol-table-handler: Added multi-file capabilities to rename Signed-off-by: Jan Bylicki --- verilog/tools/ls/symbol-table-handler.cc | 26 +++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/verilog/tools/ls/symbol-table-handler.cc b/verilog/tools/ls/symbol-table-handler.cc index ad872cb0f..4fef83deb 100644 --- a/verilog/tools/ls/symbol-table-handler.cc +++ b/verilog/tools/ls/symbol-table-handler.cc @@ -16,6 +16,7 @@ #include "verilog/tools/ls/symbol-table-handler.h" #include +#include #include "absl/flags/flag.h" #include "absl/time/clock.h" @@ -327,7 +328,11 @@ std::vector SymbolTableHandler::FindRenameLocations( const SymbolTableNode &root = symbol_table_->Root(); const SymbolTableNode *node = ScanSymbolTreeForDefinition(&root, symbol); if (!node) return {}; + std::optional location = + GetLocationFromSymbolName(*node->Key(), node->Value().file_origin); + if (!location) return {}; std::vector locations; + locations.push_back(location.value()); std::vector ranges; CollectReferences(&root, node, &locations); if (locations.empty()) return {}; @@ -342,29 +347,40 @@ verible::lsp::WorkspaceEdit SymbolTableHandler::FindRenameLocationsAndCreateEdits( const verible::lsp::RenameParams ¶ms, const verilog::BufferTrackerContainer &parsed_buffers) { + if (files_dirty_) { + BuildProjectSymbolTable(); + } Prepare(); absl::string_view symbol = GetTokenAtTextDocumentPosition(params, parsed_buffers); const SymbolTableNode &root = symbol_table_->Root(); const SymbolTableNode *node = ScanSymbolTreeForDefinition(&root, symbol); if (!node) return {}; + std::optional location = + GetLocationFromSymbolName(*node->Key(), node->Value().file_origin); + if (!location) return {}; std::vector locations; + locations.push_back(location.value()); std::vector textedits; CollectReferences(&root, node, &locations); if (locations.empty()) return {}; - textedits.reserve(locations.size()); + std::map> file_edit_pairs; + for(auto& loc: locations) { + file_edit_pairs[loc.uri].reserve(locations.size()); + } for (auto &loc : locations) { - textedits.push_back(verible::lsp::TextEdit({ + file_edit_pairs[loc.uri].push_back(verible::lsp::TextEdit({ .range = loc.range, .newText = params.newName, })); } files_dirty_ = true; - return verible::lsp::WorkspaceEdit{ - .changes = {{locations[0].uri, textedits}}, + verible::lsp::WorkspaceEdit edit = verible::lsp::WorkspaceEdit{ + .changes = {}, }; + edit.changes = file_edit_pairs; + return edit; } - void SymbolTableHandler::CollectReferencesReferenceComponents( const ReferenceComponentNode *ref, const SymbolTableNode *ref_origin, const SymbolTableNode *definition_node, From 90fff3e1aa6db51242b5fb3ec5bb316d409fa27f Mon Sep 17 00:00:00 2001 From: Jan Bylicki Date: Tue, 6 Jun 2023 12:46:14 +0200 Subject: [PATCH 05/15] verilog-language-server_test: Added rename test Signed-off-by: Jan Bylicki --- verilog/tools/ls/symbol-table-handler.cc | 2 +- .../tools/ls/verilog-language-server_test.cc | 106 ++++++++++++++++++ 2 files changed, 107 insertions(+), 1 deletion(-) diff --git a/verilog/tools/ls/symbol-table-handler.cc b/verilog/tools/ls/symbol-table-handler.cc index 4fef83deb..5fb02e35d 100644 --- a/verilog/tools/ls/symbol-table-handler.cc +++ b/verilog/tools/ls/symbol-table-handler.cc @@ -338,7 +338,7 @@ std::vector SymbolTableHandler::FindRenameLocations( if (locations.empty()) return {}; ranges.reserve(locations.size()); for (auto &loc : locations) { - ranges.push_back(loc.range); + if(loc.uri == params.textDocument.uri) ranges.push_back(loc.range); } return ranges; } diff --git a/verilog/tools/ls/verilog-language-server_test.cc b/verilog/tools/ls/verilog-language-server_test.cc index 39a77bd26..5c87bc19e 100644 --- a/verilog/tools/ls/verilog-language-server_test.cc +++ b/verilog/tools/ls/verilog-language-server_test.cc @@ -1459,6 +1459,112 @@ TEST_F(VerilogLanguageServerSymbolTableTest, CheckReferenceUnknownSymbol) { ASSERT_EQ(response_b["result"].size(), 0); } +struct RenameRequestParams { + int id; + std::string file; + std::string newName; + int line; + int character; +}; + +std::string RenameRequest(RenameRequestParams params) { + json renamerq = { + + {"jsonrpc", "2.0"}, + {"id", params.id}, + {"method", "textDocument/rename"}, + {"params", + { + {"textDocument", + { + {"uri", params.file}, + }}, + {"position",{{"line", params.line}, + {"character", params.character}}, + }, + {"newName",params.newName}, + }}}; + return renamerq.dump(); +} +std::string PrepareRenameRequest(RenameRequestParams params) { + json renamerq = { + + {"jsonrpc", "2.0"}, + {"id", params.id}, + {"method", "textDocument/prepareRename"}, + {"params", + { + {"textDocument", + { + {"uri", params.file}, + }}, + {"position",{{"line", params.line}, + {"character", params.character}}, + }, + {"newName",params.newName}, + }}}; + return renamerq.dump(); +} + +// Runs tests for textDocument/rangeFormatting requests +TEST_F(VerilogLanguageServerSymbolTableTest, PrepareRenameTest) { + // Create sample file and make sure diagnostics do not have errors + RenameRequestParams params; + params.line = 2; + params.character = 1; + params.file = "file://"+root_dir+"/fmt.sv"; + params.newName = "foo"; + params.id = 1; + const std::string mini_module = DidOpenRequest( + params.file, "module fmt();\nfunction automatic bar();\nbar();\nbar();\nendfunction;\nendmodule\n"); + ASSERT_OK(SendRequest(mini_module)); + + const json diagnostics = json::parse(GetResponse()); + EXPECT_EQ(diagnostics["method"], "textDocument/publishDiagnostics") + << "textDocument/publishDiagnostics not received"; + EXPECT_EQ(diagnostics["params"]["uri"], params.file) + << "Diagnostics for invalid file"; + + EXPECT_EQ(diagnostics["params"]["diagnostics"].size(), 0) + << "The test file has errors"; + std::string request = PrepareRenameRequest(params); + ASSERT_OK(SendRequest(request)); + + const json response = json::parse(GetResponse()); + EXPECT_EQ(response["result"].size(), 3) + << "Invalid result size for id: "; +} + +TEST_F(VerilogLanguageServerSymbolTableTest, RenameTest) { + // Create sample file and make sure diagnostics do not have errors + RenameRequestParams params; + params.line = 2; + params.character = 1; + params.file = "file://"+root_dir+"/fmt.sv"; + params.newName = "foo"; + params.id = 2; + const std::string mini_module = DidOpenRequest( + params.file, "module fmt();\nfunction automatic bar();\nbar();\nbar();\nendfunction;\nendmodule\n"); + ASSERT_OK(SendRequest(mini_module)); + + const json diagnostics = json::parse(GetResponse()); + EXPECT_EQ(diagnostics["method"], "textDocument/publishDiagnostics") + << "textDocument/publishDiagnostics not received"; + EXPECT_EQ(diagnostics["params"]["uri"], params.file) + << "Diagnostics for invalid file"; + + EXPECT_EQ(diagnostics["params"]["diagnostics"].size(), 0) + << "The test file has errors"; + std::string request = RenameRequest(params); + ASSERT_OK(SendRequest(request)); + + const json response = json::parse(GetResponse()); + EXPECT_EQ(response["result"]["changes"].size(), 1) + << "Invalid result size for id: "; + EXPECT_EQ(response["result"]["changes"][params.file].size(), 3) + << "Invalid result size for id: "; +} + // Tests correctness of Language Server shutdown request TEST_F(VerilogLanguageServerTest, ShutdownTest) { const absl::string_view shutdown_request = From 0e16ca7efaf8e46b70445162f1fb029822d410f3 Mon Sep 17 00:00:00 2001 From: Jan Bylicki Date: Mon, 12 Jun 2023 14:01:03 +0200 Subject: [PATCH 06/15] symbol-table-handler: Fixed FindReferenceLocations to match prepareRename spec Signed-off-by: Jan Bylicki --- verilog/tools/ls/symbol-table-handler.cc | 56 ++++++++++------ verilog/tools/ls/symbol-table-handler.h | 7 +- .../tools/ls/verilog-language-server_test.cc | 66 +++++++++++-------- 3 files changed, 80 insertions(+), 49 deletions(-) diff --git a/verilog/tools/ls/symbol-table-handler.cc b/verilog/tools/ls/symbol-table-handler.cc index 5fb02e35d..c261c8bd0 100644 --- a/verilog/tools/ls/symbol-table-handler.cc +++ b/verilog/tools/ls/symbol-table-handler.cc @@ -246,6 +246,29 @@ absl::string_view SymbolTableHandler::GetTokenAtTextDocumentPosition( return cursor_token.text(); } +verible::LineColumnRange +SymbolTableHandler::GetTokenRangeAtTextDocumentPosition( + const verible::lsp::TextDocumentPositionParams ¶ms, + const verilog::BufferTrackerContainer &parsed_buffers) { + const verilog::BufferTracker *tracker = + parsed_buffers.FindBufferTrackerOrNull(params.textDocument.uri); + if (!tracker) { + VLOG(1) << "Could not find buffer with URI " << params.textDocument.uri; + return {}; + } + std::shared_ptr parsedbuffer = tracker->current(); + if (!parsedbuffer) { + VLOG(1) << "Buffer not found among opened buffers: " + << params.textDocument.uri; + return {}; + } + const verible::LineColumn cursor{params.position.line, + params.position.character}; + const verible::TextStructureView &text = parsedbuffer->parser().Data(); + + const verible::TokenInfo cursor_token = text.FindTokenAt(cursor); + return text.GetRangeForToken(cursor_token); +} std::optional SymbolTableHandler::GetLocationFromSymbolName( absl::string_view symbol_name, const VerilogSourceFile *file_origin) { @@ -319,28 +342,18 @@ std::vector SymbolTableHandler::FindReferencesLocations( return locations; } -std::vector SymbolTableHandler::FindRenameLocations( +verible::lsp::Range SymbolTableHandler::FindRenameLocations( const verible::lsp::PrepareRenameParams ¶ms, const verilog::BufferTrackerContainer &parsed_buffers) { Prepare(); - absl::string_view symbol = - GetTokenAtTextDocumentPosition(params, parsed_buffers); - const SymbolTableNode &root = symbol_table_->Root(); - const SymbolTableNode *node = ScanSymbolTreeForDefinition(&root, symbol); - if (!node) return {}; - std::optional location = - GetLocationFromSymbolName(*node->Key(), node->Value().file_origin); - if (!location) return {}; - std::vector locations; - locations.push_back(location.value()); - std::vector ranges; - CollectReferences(&root, node, &locations); - if (locations.empty()) return {}; - ranges.reserve(locations.size()); - for (auto &loc : locations) { - if(loc.uri == params.textDocument.uri) ranges.push_back(loc.range); - } - return ranges; + verible::LineColumnRange symbol = + GetTokenRangeAtTextDocumentPosition(params, parsed_buffers); + verible::lsp::Range range; + range.start.line = symbol.start.line; + range.start.character = symbol.start.column; + range.end.line = symbol.end.line; + range.end.character = symbol.end.column; + return range; } verible::lsp::WorkspaceEdit @@ -364,8 +377,9 @@ SymbolTableHandler::FindRenameLocationsAndCreateEdits( std::vector textedits; CollectReferences(&root, node, &locations); if (locations.empty()) return {}; - std::map> file_edit_pairs; - for(auto& loc: locations) { + std::map> + file_edit_pairs; + for (auto &loc : locations) { file_edit_pairs[loc.uri].reserve(locations.size()); } for (auto &loc : locations) { diff --git a/verilog/tools/ls/symbol-table-handler.h b/verilog/tools/ls/symbol-table-handler.h index c5a2a817e..9a8d0ef4f 100644 --- a/verilog/tools/ls/symbol-table-handler.h +++ b/verilog/tools/ls/symbol-table-handler.h @@ -65,7 +65,7 @@ class SymbolTableHandler { const verible::lsp::ReferenceParams ¶ms, const verilog::BufferTrackerContainer &parsed_buffers); - std::vector FindRenameLocations( + verible::lsp::Range FindRenameLocations( const verible::lsp::PrepareRenameParams ¶ms, const verilog::BufferTrackerContainer &parsed_buffers); // Provide new parsed content for the given path. If "content" is nullptr, @@ -95,6 +95,11 @@ class SymbolTableHandler { const verible::lsp::TextDocumentPositionParams ¶ms, const verilog::BufferTrackerContainer &parsed_buffers); + // TODO(jbylicki): Add docstring + verible::LineColumnRange GetTokenRangeAtTextDocumentPosition( + const verible::lsp::TextDocumentPositionParams ¶ms, + const verilog::BufferTrackerContainer &parsed_buffers); + // Returns the Location of the symbol name in source file // pointed by the file_origin. // If given symbol name is not found, std::nullopt is returned. diff --git a/verilog/tools/ls/verilog-language-server_test.cc b/verilog/tools/ls/verilog-language-server_test.cc index 5c87bc19e..90aa2887d 100644 --- a/verilog/tools/ls/verilog-language-server_test.cc +++ b/verilog/tools/ls/verilog-language-server_test.cc @@ -1466,7 +1466,7 @@ struct RenameRequestParams { int line; int character; }; - + std::string RenameRequest(RenameRequestParams params) { json renamerq = { @@ -1475,15 +1475,16 @@ std::string RenameRequest(RenameRequestParams params) { {"method", "textDocument/rename"}, {"params", { - {"textDocument", - { - {"uri", params.file}, - }}, - {"position",{{"line", params.line}, - {"character", params.character}}, - }, - {"newName",params.newName}, - }}}; + {"textDocument", + { + {"uri", params.file}, + }}, + { + "position", + {{"line", params.line}, {"character", params.character}}, + }, + {"newName", params.newName}, + }}}; return renamerq.dump(); } std::string PrepareRenameRequest(RenameRequestParams params) { @@ -1494,15 +1495,16 @@ std::string PrepareRenameRequest(RenameRequestParams params) { {"method", "textDocument/prepareRename"}, {"params", { - {"textDocument", - { - {"uri", params.file}, - }}, - {"position",{{"line", params.line}, - {"character", params.character}}, - }, - {"newName",params.newName}, - }}}; + {"textDocument", + { + {"uri", params.file}, + }}, + { + "position", + {{"line", params.line}, {"character", params.character}}, + }, + {"newName", params.newName}, + }}}; return renamerq.dump(); } @@ -1512,11 +1514,13 @@ TEST_F(VerilogLanguageServerSymbolTableTest, PrepareRenameTest) { RenameRequestParams params; params.line = 2; params.character = 1; - params.file = "file://"+root_dir+"/fmt.sv"; + params.file = "file://" + root_dir + "/fmt.sv"; params.newName = "foo"; params.id = 1; - const std::string mini_module = DidOpenRequest( - params.file, "module fmt();\nfunction automatic bar();\nbar();\nbar();\nendfunction;\nendmodule\n"); + const std::string mini_module = + DidOpenRequest(params.file, + "module fmt();\nfunction automatic " + "bar();\nbar();\nbar();\nendfunction;\nendmodule\n"); ASSERT_OK(SendRequest(mini_module)); const json diagnostics = json::parse(GetResponse()); @@ -1531,8 +1535,13 @@ TEST_F(VerilogLanguageServerSymbolTableTest, PrepareRenameTest) { ASSERT_OK(SendRequest(request)); const json response = json::parse(GetResponse()); - EXPECT_EQ(response["result"].size(), 3) - << "Invalid result size for id: "; + EXPECT_EQ(response["result"]["start"]["line"], 2) + << "Invalid result for id: "; + EXPECT_EQ(response["result"]["start"]["character"], 0) + << "Invalid result for id: "; + EXPECT_EQ(response["result"]["end"]["line"], 2) << "Invalid result for id: "; + EXPECT_EQ(response["result"]["end"]["character"], 3) + << "Invalid result for id: "; } TEST_F(VerilogLanguageServerSymbolTableTest, RenameTest) { @@ -1540,11 +1549,13 @@ TEST_F(VerilogLanguageServerSymbolTableTest, RenameTest) { RenameRequestParams params; params.line = 2; params.character = 1; - params.file = "file://"+root_dir+"/fmt.sv"; + params.file = "file://" + root_dir + "/fmt.sv"; params.newName = "foo"; params.id = 2; - const std::string mini_module = DidOpenRequest( - params.file, "module fmt();\nfunction automatic bar();\nbar();\nbar();\nendfunction;\nendmodule\n"); + const std::string mini_module = + DidOpenRequest(params.file, + "module fmt();\nfunction automatic " + "bar();\nbar();\nbar();\nendfunction;\nendmodule\n"); ASSERT_OK(SendRequest(mini_module)); const json diagnostics = json::parse(GetResponse()); @@ -1559,6 +1570,7 @@ TEST_F(VerilogLanguageServerSymbolTableTest, RenameTest) { ASSERT_OK(SendRequest(request)); const json response = json::parse(GetResponse()); + std::cout << response << std::endl; EXPECT_EQ(response["result"]["changes"].size(), 1) << "Invalid result size for id: "; EXPECT_EQ(response["result"]["changes"][params.file].size(), 3) From 681bd6464ec7298d9737f4936263ca46815c7e60 Mon Sep 17 00:00:00 2001 From: Jan Bylicki Date: Thu, 15 Jun 2023 12:51:26 +0200 Subject: [PATCH 07/15] verilog-language-server_test: Changed pathing to fix windows matching issues Signed-off-by: Jan Bylicki --- .../tools/ls/verilog-language-server_test.cc | 24 +++++++++++++++---- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/verilog/tools/ls/verilog-language-server_test.cc b/verilog/tools/ls/verilog-language-server_test.cc index 90aa2887d..27cb39a89 100644 --- a/verilog/tools/ls/verilog-language-server_test.cc +++ b/verilog/tools/ls/verilog-language-server_test.cc @@ -1549,16 +1549,30 @@ TEST_F(VerilogLanguageServerSymbolTableTest, RenameTest) { RenameRequestParams params; params.line = 2; params.character = 1; - params.file = "file://" + root_dir + "/fmt.sv"; - params.newName = "foo"; - params.id = 2; + + absl::string_view filelist_content = "rename.sv\n"; + + const verible::file::testing::ScopedTestFile filelist( + root_dir, filelist_content, "verible.filelist"); + const verible::file::testing::ScopedTestFile module_foo( + root_dir, + "module rename();\nfunction automatic " + "bar();\nbar();\nbar();\nendfunction;\nendmodule\n", + "rename.sv"); + const std::string mini_module = - DidOpenRequest(params.file, - "module fmt();\nfunction automatic " + DidOpenRequest("file://" + module_foo.filename(), + "module rename();\nfunction automatic " "bar();\nbar();\nbar();\nendfunction;\nendmodule\n"); + + params.file = "file://" + module_foo.filename(); + params.newName = "foo"; + params.id = 2; + ASSERT_OK(SendRequest(mini_module)); const json diagnostics = json::parse(GetResponse()); + std::cout << diagnostics << std::endl; EXPECT_EQ(diagnostics["method"], "textDocument/publishDiagnostics") << "textDocument/publishDiagnostics not received"; EXPECT_EQ(diagnostics["params"]["uri"], params.file) From b4e16f4a736ed0ded4a5e3f95bed82cb34fb4541 Mon Sep 17 00:00:00 2001 From: Jan Bylicki Date: Mon, 19 Jun 2023 12:51:59 +0200 Subject: [PATCH 08/15] verilog-language-server_test: Moved request creation to LSP structs Signed-off-by: Jan Bylicki --- .../tools/ls/verilog-language-server_test.cc | 98 ++++++------------- 1 file changed, 31 insertions(+), 67 deletions(-) diff --git a/verilog/tools/ls/verilog-language-server_test.cc b/verilog/tools/ls/verilog-language-server_test.cc index 27cb39a89..95206b560 100644 --- a/verilog/tools/ls/verilog-language-server_test.cc +++ b/verilog/tools/ls/verilog-language-server_test.cc @@ -33,6 +33,7 @@ else \ EXPECT_TRUE(status__.ok()) << status__ +using verible::lsp::PathToLSPUri; namespace verilog { namespace { @@ -1459,66 +1460,31 @@ TEST_F(VerilogLanguageServerSymbolTableTest, CheckReferenceUnknownSymbol) { ASSERT_EQ(response_b["result"].size(), 0); } -struct RenameRequestParams { - int id; - std::string file; - std::string newName; - int line; - int character; -}; - -std::string RenameRequest(RenameRequestParams params) { - json renamerq = { - - {"jsonrpc", "2.0"}, - {"id", params.id}, - {"method", "textDocument/rename"}, - {"params", - { - {"textDocument", - { - {"uri", params.file}, - }}, - { - "position", - {{"line", params.line}, {"character", params.character}}, - }, - {"newName", params.newName}, - }}}; - return renamerq.dump(); +std::string RenameRequest(verible::lsp::RenameParams params) { + json request = {{"jsonrpc", "2.0"}, + {"id", 2}, + {"method", "textDocument/rename"}, + {"params", params}}; + return request.dump(); } -std::string PrepareRenameRequest(RenameRequestParams params) { - json renamerq = { - - {"jsonrpc", "2.0"}, - {"id", params.id}, - {"method", "textDocument/prepareRename"}, - {"params", - { - {"textDocument", - { - {"uri", params.file}, - }}, - { - "position", - {{"line", params.line}, {"character", params.character}}, - }, - {"newName", params.newName}, - }}}; - return renamerq.dump(); +std::string PrepareRenameRequest(verible::lsp::PrepareRenameParams params) { + json request = {{"jsonrpc", "2.0"}, + {"id", 2}, + {"method", "textDocument/prepareRename"}, + {"params", params}}; + return request.dump(); } - // Runs tests for textDocument/rangeFormatting requests TEST_F(VerilogLanguageServerSymbolTableTest, PrepareRenameTest) { // Create sample file and make sure diagnostics do not have errors - RenameRequestParams params; - params.line = 2; - params.character = 1; - params.file = "file://" + root_dir + "/fmt.sv"; - params.newName = "foo"; - params.id = 1; + std::string file_uri = PathToLSPUri(absl::string_view(root_dir + "/fmt.sv")); + verible::lsp::PrepareRenameParams params; + params.position.line = 2; + params.position.character = 1; + params.textDocument.uri = file_uri; + const std::string mini_module = - DidOpenRequest(params.file, + DidOpenRequest(file_uri, "module fmt();\nfunction automatic " "bar();\nbar();\nbar();\nendfunction;\nendmodule\n"); ASSERT_OK(SendRequest(mini_module)); @@ -1526,13 +1492,12 @@ TEST_F(VerilogLanguageServerSymbolTableTest, PrepareRenameTest) { const json diagnostics = json::parse(GetResponse()); EXPECT_EQ(diagnostics["method"], "textDocument/publishDiagnostics") << "textDocument/publishDiagnostics not received"; - EXPECT_EQ(diagnostics["params"]["uri"], params.file) + EXPECT_EQ(diagnostics["params"]["uri"], file_uri) << "Diagnostics for invalid file"; EXPECT_EQ(diagnostics["params"]["diagnostics"].size(), 0) << "The test file has errors"; - std::string request = PrepareRenameRequest(params); - ASSERT_OK(SendRequest(request)); + ASSERT_OK(SendRequest(PrepareRenameRequest(params))); const json response = json::parse(GetResponse()); EXPECT_EQ(response["result"]["start"]["line"], 2) @@ -1546,9 +1511,13 @@ TEST_F(VerilogLanguageServerSymbolTableTest, PrepareRenameTest) { TEST_F(VerilogLanguageServerSymbolTableTest, RenameTest) { // Create sample file and make sure diagnostics do not have errors - RenameRequestParams params; - params.line = 2; - params.character = 1; + std::string file_uri = + PathToLSPUri(absl::string_view(root_dir + "/rename.sv")); + verible::lsp::RenameParams params; + params.position.line = 2; + params.position.character = 1; + params.textDocument.uri = file_uri; + params.newName = "foo"; absl::string_view filelist_content = "rename.sv\n"; @@ -1565,17 +1534,13 @@ TEST_F(VerilogLanguageServerSymbolTableTest, RenameTest) { "module rename();\nfunction automatic " "bar();\nbar();\nbar();\nendfunction;\nendmodule\n"); - params.file = "file://" + module_foo.filename(); - params.newName = "foo"; - params.id = 2; - ASSERT_OK(SendRequest(mini_module)); const json diagnostics = json::parse(GetResponse()); std::cout << diagnostics << std::endl; EXPECT_EQ(diagnostics["method"], "textDocument/publishDiagnostics") << "textDocument/publishDiagnostics not received"; - EXPECT_EQ(diagnostics["params"]["uri"], params.file) + EXPECT_EQ(diagnostics["params"]["uri"], verible::lsp::LSPUriToPath(file_uri)) << "Diagnostics for invalid file"; EXPECT_EQ(diagnostics["params"]["diagnostics"].size(), 0) @@ -1584,10 +1549,9 @@ TEST_F(VerilogLanguageServerSymbolTableTest, RenameTest) { ASSERT_OK(SendRequest(request)); const json response = json::parse(GetResponse()); - std::cout << response << std::endl; EXPECT_EQ(response["result"]["changes"].size(), 1) << "Invalid result size for id: "; - EXPECT_EQ(response["result"]["changes"][params.file].size(), 3) + EXPECT_EQ(response["result"]["changes"][file_uri].size(), 3) << "Invalid result size for id: "; } From 0c64272f332ad2437992225708f0bf3afb530e8a Mon Sep 17 00:00:00 2001 From: Jan Bylicki Date: Fri, 16 Jun 2023 15:47:22 +0200 Subject: [PATCH 09/15] lsp: Fixed module instantiation renaming 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 --- common/lsp/lsp-protocol.yaml | 6 +-- verilog/tools/ls/symbol-table-handler.cc | 45 ++++++++++++------- verilog/tools/ls/symbol-table-handler.h | 4 +- verilog/tools/ls/verilog-language-server.cc | 3 +- .../tools/ls/verilog-language-server_test.cc | 7 ++- 5 files changed, 38 insertions(+), 27 deletions(-) diff --git a/common/lsp/lsp-protocol.yaml b/common/lsp/lsp-protocol.yaml index aec19c413..24735bca4 100644 --- a/common/lsp/lsp-protocol.yaml +++ b/common/lsp/lsp-protocol.yaml @@ -203,13 +203,13 @@ DocumentLink: range: Range target?: string # DocumentUri -# -- textDocument/prepareRename (TODO(jbylicki): check that requires project + active symbol table #1189) +# -- textDocument/prepareRename PrepareRenameParams: <: TextDocumentPositionParams -# Response: Range[] +# Response: Range -# -- textDocument/rename (TODO(jbylicki): check that requires project + active symbol table #1189) +# -- textDocument/rename RenameParams: <: TextDocumentPositionParams newName: string diff --git a/verilog/tools/ls/symbol-table-handler.cc b/verilog/tools/ls/symbol-table-handler.cc index c261c8bd0..9150c8be1 100644 --- a/verilog/tools/ls/symbol-table-handler.cc +++ b/verilog/tools/ls/symbol-table-handler.cc @@ -248,22 +248,23 @@ absl::string_view SymbolTableHandler::GetTokenAtTextDocumentPosition( verible::LineColumnRange SymbolTableHandler::GetTokenRangeAtTextDocumentPosition( - const verible::lsp::TextDocumentPositionParams ¶ms, + const verible::lsp::TextDocumentPositionParams &document_cursor, const verilog::BufferTrackerContainer &parsed_buffers) { const verilog::BufferTracker *tracker = - parsed_buffers.FindBufferTrackerOrNull(params.textDocument.uri); + parsed_buffers.FindBufferTrackerOrNull(document_cursor.textDocument.uri); if (!tracker) { - VLOG(1) << "Could not find buffer with URI " << params.textDocument.uri; + VLOG(1) << "Could not find buffer with URI " + << document_cursor.textDocument.uri; return {}; } std::shared_ptr parsedbuffer = tracker->current(); if (!parsedbuffer) { VLOG(1) << "Buffer not found among opened buffers: " - << params.textDocument.uri; + << document_cursor.textDocument.uri; return {}; } - const verible::LineColumn cursor{params.position.line, - params.position.character}; + const verible::LineColumn cursor{document_cursor.position.line, + document_cursor.position.character}; const verible::TextStructureView &text = parsedbuffer->parser().Data(); const verible::TokenInfo cursor_token = text.FindTokenAt(cursor); @@ -342,18 +343,16 @@ std::vector SymbolTableHandler::FindReferencesLocations( return locations; } -verible::lsp::Range SymbolTableHandler::FindRenameLocations( +verible::lsp::Range SymbolTableHandler::FindRenameableRangeAtCursor( const verible::lsp::PrepareRenameParams ¶ms, const verilog::BufferTrackerContainer &parsed_buffers) { Prepare(); + if (files_dirty_) { + BuildProjectSymbolTable(); + } verible::LineColumnRange symbol = GetTokenRangeAtTextDocumentPosition(params, parsed_buffers); - verible::lsp::Range range; - range.start.line = symbol.start.line; - range.start.character = symbol.start.column; - range.end.line = symbol.end.line; - range.end.character = symbol.end.column; - return range; + return RangeFromLineColumn(symbol); } verible::lsp::WorkspaceEdit @@ -383,16 +382,28 @@ SymbolTableHandler::FindRenameLocationsAndCreateEdits( file_edit_pairs[loc.uri].reserve(locations.size()); } for (auto &loc : locations) { - file_edit_pairs[loc.uri].push_back(verible::lsp::TextEdit({ - .range = loc.range, - .newText = params.newName, - })); + // TODO(jbylicki): Remove this band-aid fix once #1678 is merged - it should + // fix + // duplicate definition/references appending in modules and remove the need + // for adding the definition location above. + if (std::none_of( + file_edit_pairs[loc.uri].begin(), file_edit_pairs[loc.uri].end(), + [&loc](const verible::lsp::TextEdit &it) { + return loc.range.start.character == it.range.start.character && + loc.range.start.line == it.range.end.line; + })) { + file_edit_pairs[loc.uri].push_back(verible::lsp::TextEdit({ + .range = loc.range, + .newText = params.newName, + })); + } } files_dirty_ = true; verible::lsp::WorkspaceEdit edit = verible::lsp::WorkspaceEdit{ .changes = {}, }; edit.changes = file_edit_pairs; + std::cerr << "SIZE: " << edit.changes[locations[0].uri].size() << std::endl; return edit; } void SymbolTableHandler::CollectReferencesReferenceComponents( diff --git a/verilog/tools/ls/symbol-table-handler.h b/verilog/tools/ls/symbol-table-handler.h index 9a8d0ef4f..6a97e6033 100644 --- a/verilog/tools/ls/symbol-table-handler.h +++ b/verilog/tools/ls/symbol-table-handler.h @@ -65,7 +65,7 @@ class SymbolTableHandler { const verible::lsp::ReferenceParams ¶ms, const verilog::BufferTrackerContainer &parsed_buffers); - verible::lsp::Range FindRenameLocations( + verible::lsp::Range FindRenameableRangeAtCursor( const verible::lsp::PrepareRenameParams ¶ms, const verilog::BufferTrackerContainer &parsed_buffers); // Provide new parsed content for the given path. If "content" is nullptr, @@ -97,7 +97,7 @@ class SymbolTableHandler { // TODO(jbylicki): Add docstring verible::LineColumnRange GetTokenRangeAtTextDocumentPosition( - const verible::lsp::TextDocumentPositionParams ¶ms, + const verible::lsp::TextDocumentPositionParams &document_cursor, const verilog::BufferTrackerContainer &parsed_buffers); // Returns the Location of the symbol name in source file diff --git a/verilog/tools/ls/verilog-language-server.cc b/verilog/tools/ls/verilog-language-server.cc index 3617c5c05..371504245 100644 --- a/verilog/tools/ls/verilog-language-server.cc +++ b/verilog/tools/ls/verilog-language-server.cc @@ -142,7 +142,8 @@ void VerilogLanguageServer::SetRequestHandlers() { dispatcher_.AddRequestHandler( "textDocument/prepareRename", [this](const verible::lsp::PrepareRenameParams &p) { - return symbol_table_handler_.FindRenameLocations(p, parsed_buffers_); + return symbol_table_handler_.FindRenameableRangeAtCursor( + p, parsed_buffers_); }); dispatcher_.AddRequestHandler( "textDocument/rename", [this](const verible::lsp::RenameParams &p) { diff --git a/verilog/tools/ls/verilog-language-server_test.cc b/verilog/tools/ls/verilog-language-server_test.cc index 95206b560..2b4972e80 100644 --- a/verilog/tools/ls/verilog-language-server_test.cc +++ b/verilog/tools/ls/verilog-language-server_test.cc @@ -33,7 +33,6 @@ else \ EXPECT_TRUE(status__.ok()) << status__ -using verible::lsp::PathToLSPUri; namespace verilog { namespace { @@ -1475,7 +1474,7 @@ std::string PrepareRenameRequest(verible::lsp::PrepareRenameParams params) { return request.dump(); } // Runs tests for textDocument/rangeFormatting requests -TEST_F(VerilogLanguageServerSymbolTableTest, PrepareRenameTest) { +TEST_F(VerilogLanguageServerSymbolTableTest, PrepareRenameReturnsRangeOfEditableSymbol) { // Create sample file and make sure diagnostics do not have errors std::string file_uri = PathToLSPUri(absl::string_view(root_dir + "/fmt.sv")); verible::lsp::PrepareRenameParams params; @@ -1509,7 +1508,7 @@ TEST_F(VerilogLanguageServerSymbolTableTest, PrepareRenameTest) { << "Invalid result for id: "; } -TEST_F(VerilogLanguageServerSymbolTableTest, RenameTest) { +TEST_F(VerilogLanguageServerSymbolTableTest, RenameTestSymbolSingleFile) { // Create sample file and make sure diagnostics do not have errors std::string file_uri = PathToLSPUri(absl::string_view(root_dir + "/rename.sv")); @@ -1540,7 +1539,7 @@ TEST_F(VerilogLanguageServerSymbolTableTest, RenameTest) { std::cout << diagnostics << std::endl; EXPECT_EQ(diagnostics["method"], "textDocument/publishDiagnostics") << "textDocument/publishDiagnostics not received"; - EXPECT_EQ(diagnostics["params"]["uri"], verible::lsp::LSPUriToPath(file_uri)) + EXPECT_EQ(diagnostics["params"]["uri"], PathToLSPUri(verible::lsp::LSPUriToPath(file_uri))) << "Diagnostics for invalid file"; EXPECT_EQ(diagnostics["params"]["diagnostics"].size(), 0) From 667ed7eacaae70f8d177ff178ed6a7c32dc8d239 Mon Sep 17 00:00:00 2001 From: Jan Bylicki Date: Mon, 19 Jun 2023 15:57:57 +0200 Subject: [PATCH 10/15] verilog-language-server_test: Added cross-package renaming test Signed-off-by: Jan Bylicki --- .../tools/ls/verilog-language-server_test.cc | 52 +++++++++++++++++-- 1 file changed, 49 insertions(+), 3 deletions(-) diff --git a/verilog/tools/ls/verilog-language-server_test.cc b/verilog/tools/ls/verilog-language-server_test.cc index 2b4972e80..ecaee1274 100644 --- a/verilog/tools/ls/verilog-language-server_test.cc +++ b/verilog/tools/ls/verilog-language-server_test.cc @@ -1474,7 +1474,8 @@ std::string PrepareRenameRequest(verible::lsp::PrepareRenameParams params) { return request.dump(); } // Runs tests for textDocument/rangeFormatting requests -TEST_F(VerilogLanguageServerSymbolTableTest, PrepareRenameReturnsRangeOfEditableSymbol) { +TEST_F(VerilogLanguageServerSymbolTableTest, + PrepareRenameReturnsRangeOfEditableSymbol) { // Create sample file and make sure diagnostics do not have errors std::string file_uri = PathToLSPUri(absl::string_view(root_dir + "/fmt.sv")); verible::lsp::PrepareRenameParams params; @@ -1539,8 +1540,6 @@ TEST_F(VerilogLanguageServerSymbolTableTest, RenameTestSymbolSingleFile) { std::cout << diagnostics << std::endl; EXPECT_EQ(diagnostics["method"], "textDocument/publishDiagnostics") << "textDocument/publishDiagnostics not received"; - EXPECT_EQ(diagnostics["params"]["uri"], PathToLSPUri(verible::lsp::LSPUriToPath(file_uri))) - << "Diagnostics for invalid file"; EXPECT_EQ(diagnostics["params"]["diagnostics"].size(), 0) << "The test file has errors"; @@ -1554,6 +1553,53 @@ TEST_F(VerilogLanguageServerSymbolTableTest, RenameTestSymbolSingleFile) { << "Invalid result size for id: "; } +TEST_F(VerilogLanguageServerSymbolTableTest, RenameTestPackageDistinction) { + // Create sample file and make sure diagnostics do not have errors + std::string file_uri = + PathToLSPUri(absl::string_view(root_dir + "/rename.sv")); + verible::lsp::RenameParams params; + params.position.line = 7; + params.position.character = 15; + params.textDocument.uri = file_uri; + params.newName = "foobaz"; + std::string renamesv = + "package foo;\n" + " class foobar;\n" + " bar::foobar baz;\n" + " endclass;\n" + "endpackage;\n" + "package bar;\n" + " class foobar;\n" + " foo::foobar baz;\n" + " endclass;\n" + "endpackage;\n"; + absl::string_view filelist_content = "rename.sv\n"; + + const verible::file::testing::ScopedTestFile filelist( + root_dir, filelist_content, "verible.filelist"); + const verible::file::testing::ScopedTestFile module_foo(root_dir, renamesv, + "rename.sv"); + + const std::string mini_module = + DidOpenRequest("file://" + module_foo.filename(), renamesv); + ASSERT_OK(SendRequest(mini_module)); + + const json diagnostics = json::parse(GetResponse()); + EXPECT_EQ(diagnostics["method"], "textDocument/publishDiagnostics") + << "textDocument/publishDiagnostics not received"; + + // Complaints about package and file names + EXPECT_EQ(diagnostics["params"]["diagnostics"].size(), 2) + << "The test file has errors"; + std::string request = RenameRequest(params); + ASSERT_OK(SendRequest(request)); + + const json response = json::parse(GetResponse()); + EXPECT_EQ(response["result"]["changes"].size(), 1) + << "Invalid result size for id: "; + EXPECT_EQ(response["result"]["changes"][file_uri].size(), 2) + << "Invalid result size for id: "; +} // Tests correctness of Language Server shutdown request TEST_F(VerilogLanguageServerTest, ShutdownTest) { const absl::string_view shutdown_request = From 90a0cb7b8d025f6c7d67a194748b4cb2d4e6b1e4 Mon Sep 17 00:00:00 2001 From: Jan Bylicki Date: Mon, 19 Jun 2023 16:47:00 +0200 Subject: [PATCH 11/15] verilog-language-server_test: Added multi-file rename test Added a rename test that checks whether renaming across files works Signed-off-by: Jan Bylicki --- .../tools/ls/verilog-language-server_test.cc | 72 ++++++++++++++++++- 1 file changed, 71 insertions(+), 1 deletion(-) diff --git a/verilog/tools/ls/verilog-language-server_test.cc b/verilog/tools/ls/verilog-language-server_test.cc index ecaee1274..9ab584d2f 100644 --- a/verilog/tools/ls/verilog-language-server_test.cc +++ b/verilog/tools/ls/verilog-language-server_test.cc @@ -1530,7 +1530,7 @@ TEST_F(VerilogLanguageServerSymbolTableTest, RenameTestSymbolSingleFile) { "rename.sv"); const std::string mini_module = - DidOpenRequest("file://" + module_foo.filename(), + DidOpenRequest(file_uri, "module rename();\nfunction automatic " "bar();\nbar();\nbar();\nendfunction;\nendmodule\n"); @@ -1541,6 +1541,9 @@ TEST_F(VerilogLanguageServerSymbolTableTest, RenameTestSymbolSingleFile) { EXPECT_EQ(diagnostics["method"], "textDocument/publishDiagnostics") << "textDocument/publishDiagnostics not received"; + EXPECT_EQ(diagnostics["params"]["uri"], + PathToLSPUri(verible::lsp::LSPUriToPath(file_uri))) + << "Diagnostics for invalid file"; EXPECT_EQ(diagnostics["params"]["diagnostics"].size(), 0) << "The test file has errors"; std::string request = RenameRequest(params); @@ -1553,6 +1556,70 @@ TEST_F(VerilogLanguageServerSymbolTableTest, RenameTestSymbolSingleFile) { << "Invalid result size for id: "; } +TEST_F(VerilogLanguageServerSymbolTableTest, RenameTestSymbolMultipleFiles) { + // Create sample file and make sure diagnostics do not have errors + std::string top_uri = PathToLSPUri(absl::string_view(root_dir + "/top.sv")); + std::string foo_uri = PathToLSPUri(absl::string_view(root_dir + "/foo.sv")); + verible::lsp::RenameParams params; + params.position.line = 2; + params.position.character = 9; + params.textDocument.uri = top_uri; + params.newName = "foobaz"; + std::string foosv = + "package foo;\n" + " class foobar;\n" + " endclass;\n" + "endpackage;\n"; + std::string topsv = + "import foo::*;\n" + "module top;\n" + " foo::foobar bar;\n" + "endmodule;\n"; + absl::string_view filelist_content = "./foo.sv\n./top.sv\n"; + + const verible::file::testing::ScopedTestFile filelist( + root_dir, filelist_content, "verible.filelist"); + const verible::file::testing::ScopedTestFile module_foo(root_dir, foosv, + "foo.sv"); + + const verible::file::testing::ScopedTestFile module_top(root_dir, topsv, + "top.sv"); + const std::string top_request = DidOpenRequest(top_uri, topsv); + ASSERT_OK(SendRequest(top_request)); + + const json diagnostics = json::parse(GetResponse()); + EXPECT_EQ(diagnostics["method"], "textDocument/publishDiagnostics") + << "textDocument/publishDiagnostics not received"; + EXPECT_EQ(diagnostics["params"]["uri"], + PathToLSPUri(verible::lsp::LSPUriToPath(top_uri))) + << "Diagnostics for invalid file"; + + const std::string foo_request = DidOpenRequest(foo_uri, foosv); + ASSERT_OK(SendRequest(foo_request)); + + const json diagnostics_foo = json::parse(GetResponse()); + EXPECT_EQ(diagnostics_foo["method"], "textDocument/publishDiagnostics") + << "textDocument/publishDiagnostics not received"; + EXPECT_EQ(diagnostics_foo["params"]["uri"], + PathToLSPUri(verible::lsp::LSPUriToPath(foo_uri))) + << "Diagnostics for invalid file"; + + // Complaints about package and file names + EXPECT_EQ(diagnostics["params"]["diagnostics"].size(), 0) + << "The test file has errors"; + std::string request = RenameRequest(params); + ASSERT_OK(SendRequest(request)); + + const json response = json::parse(GetResponse()); + std::cout << response << std::endl; + EXPECT_EQ(response["result"]["changes"].size(), 2) + << "Invalid result size for id: "; + EXPECT_EQ(response["result"]["changes"][top_uri].size(), 1) + << "Invalid result size for id: "; + EXPECT_EQ(response["result"]["changes"][foo_uri].size(), 1) + << "Invalid result size for id: "; +} + TEST_F(VerilogLanguageServerSymbolTableTest, RenameTestPackageDistinction) { // Create sample file and make sure diagnostics do not have errors std::string file_uri = @@ -1587,6 +1654,9 @@ TEST_F(VerilogLanguageServerSymbolTableTest, RenameTestPackageDistinction) { const json diagnostics = json::parse(GetResponse()); EXPECT_EQ(diagnostics["method"], "textDocument/publishDiagnostics") << "textDocument/publishDiagnostics not received"; + EXPECT_EQ(diagnostics["params"]["uri"], + PathToLSPUri(verible::lsp::LSPUriToPath(file_uri))) + << "Diagnostics for invalid file"; // Complaints about package and file names EXPECT_EQ(diagnostics["params"]["diagnostics"].size(), 2) From 68ec5058bdd64692ff40ef8346a0e170af37a347 Mon Sep 17 00:00:00 2001 From: Jan Bylicki Date: Tue, 20 Jun 2023 12:22:35 +0200 Subject: [PATCH 12/15] symbol-table-handler: Added optional nullptr return from prepareRename Signed-off-by: Jan Bylicki --- verilog/tools/ls/symbol-table-handler.cc | 41 +++++++++++++++++-- verilog/tools/ls/symbol-table-handler.h | 6 ++- verilog/tools/ls/verilog-language-server.cc | 6 ++- .../tools/ls/verilog-language-server_test.cc | 33 +++++++++++++-- 4 files changed, 75 insertions(+), 11 deletions(-) diff --git a/verilog/tools/ls/symbol-table-handler.cc b/verilog/tools/ls/symbol-table-handler.cc index 9150c8be1..f19160364 100644 --- a/verilog/tools/ls/symbol-table-handler.cc +++ b/verilog/tools/ls/symbol-table-handler.cc @@ -223,6 +223,29 @@ void SymbolTableHandler::Prepare() { } } +std::optional +SymbolTableHandler::GetTokenInfoAtTextDocumentPosition( + const verible::lsp::TextDocumentPositionParams ¶ms, + const verilog::BufferTrackerContainer &parsed_buffers) { + const verilog::BufferTracker *tracker = + parsed_buffers.FindBufferTrackerOrNull(params.textDocument.uri); + if (!tracker) { + VLOG(1) << "Could not find buffer with URI " << params.textDocument.uri; + return {}; + } + std::shared_ptr parsedbuffer = tracker->current(); + if (!parsedbuffer) { + VLOG(1) << "Buffer not found among opened buffers: " + << params.textDocument.uri; + return {}; + } + const verible::LineColumn cursor{params.position.line, + params.position.character}; + const verible::TextStructureView &text = parsedbuffer->parser().Data(); + const verible::TokenInfo cursor_token = text.FindTokenAt(cursor); + return cursor_token; +} + absl::string_view SymbolTableHandler::GetTokenAtTextDocumentPosition( const verible::lsp::TextDocumentPositionParams ¶ms, const verilog::BufferTrackerContainer &parsed_buffers) { @@ -343,16 +366,26 @@ std::vector SymbolTableHandler::FindReferencesLocations( return locations; } -verible::lsp::Range SymbolTableHandler::FindRenameableRangeAtCursor( +std::optional +SymbolTableHandler::FindRenameableRangeAtCursor( const verible::lsp::PrepareRenameParams ¶ms, const verilog::BufferTrackerContainer &parsed_buffers) { Prepare(); if (files_dirty_) { BuildProjectSymbolTable(); } - verible::LineColumnRange symbol = - GetTokenRangeAtTextDocumentPosition(params, parsed_buffers); - return RangeFromLineColumn(symbol); + std::optional symbol = + GetTokenInfoAtTextDocumentPosition(params, parsed_buffers); + if (symbol.has_value()) { + verible::TokenInfo token = symbol.value(); + const SymbolTableNode &root = symbol_table_->Root(); + const SymbolTableNode *node = + ScanSymbolTreeForDefinition(&root, token.text()); + if (!node) return {}; + return RangeFromLineColumn( + GetTokenRangeAtTextDocumentPosition(params, parsed_buffers)); + } + return {}; } verible::lsp::WorkspaceEdit diff --git a/verilog/tools/ls/symbol-table-handler.h b/verilog/tools/ls/symbol-table-handler.h index 6a97e6033..630f02ce3 100644 --- a/verilog/tools/ls/symbol-table-handler.h +++ b/verilog/tools/ls/symbol-table-handler.h @@ -65,7 +65,7 @@ class SymbolTableHandler { const verible::lsp::ReferenceParams ¶ms, const verilog::BufferTrackerContainer &parsed_buffers); - verible::lsp::Range FindRenameableRangeAtCursor( + std::optional FindRenameableRangeAtCursor( const verible::lsp::PrepareRenameParams ¶ms, const verilog::BufferTrackerContainer &parsed_buffers); // Provide new parsed content for the given path. If "content" is nullptr, @@ -100,6 +100,10 @@ class SymbolTableHandler { const verible::lsp::TextDocumentPositionParams &document_cursor, const verilog::BufferTrackerContainer &parsed_buffers); + std::optional GetTokenInfoAtTextDocumentPosition( + const verible::lsp::TextDocumentPositionParams ¶ms, + const verilog::BufferTrackerContainer &parsed_buffers); + // Returns the Location of the symbol name in source file // pointed by the file_origin. // If given symbol name is not found, std::nullopt is returned. diff --git a/verilog/tools/ls/verilog-language-server.cc b/verilog/tools/ls/verilog-language-server.cc index 371504245..d725de192 100644 --- a/verilog/tools/ls/verilog-language-server.cc +++ b/verilog/tools/ls/verilog-language-server.cc @@ -141,9 +141,11 @@ void VerilogLanguageServer::SetRequestHandlers() { }); dispatcher_.AddRequestHandler( "textDocument/prepareRename", - [this](const verible::lsp::PrepareRenameParams &p) { - return symbol_table_handler_.FindRenameableRangeAtCursor( + [this](const verible::lsp::PrepareRenameParams &p) -> nlohmann::json { + auto range = symbol_table_handler_.FindRenameableRangeAtCursor( p, parsed_buffers_); + if (range.has_value()) return range.value(); + return nullptr; }); dispatcher_.AddRequestHandler( "textDocument/rename", [this](const verible::lsp::RenameParams &p) { diff --git a/verilog/tools/ls/verilog-language-server_test.cc b/verilog/tools/ls/verilog-language-server_test.cc index 9ab584d2f..24efe8271 100644 --- a/verilog/tools/ls/verilog-language-server_test.cc +++ b/verilog/tools/ls/verilog-language-server_test.cc @@ -1509,6 +1509,34 @@ TEST_F(VerilogLanguageServerSymbolTableTest, << "Invalid result for id: "; } +TEST_F(VerilogLanguageServerSymbolTableTest, PrepareRenameReturnsNull) { + // Create sample file and make sure diagnostics do not have errors + std::string file_uri = PathToLSPUri(absl::string_view(root_dir + "/fmt.sv")); + verible::lsp::PrepareRenameParams params; + params.position.line = 1; + params.position.character = 1; + params.textDocument.uri = file_uri; + + const std::string mini_module = + DidOpenRequest(file_uri, + "module fmt();\nfunction automatic " + "bar();\nbar();\nbar();\nendfunction;\nendmodule\n"); + ASSERT_OK(SendRequest(mini_module)); + + const json diagnostics = json::parse(GetResponse()); + EXPECT_EQ(diagnostics["method"], "textDocument/publishDiagnostics") + << "textDocument/publishDiagnostics not received"; + EXPECT_EQ(diagnostics["params"]["uri"], file_uri) + << "Diagnostics for invalid file"; + + EXPECT_EQ(diagnostics["params"]["diagnostics"].size(), 0) + << "The test file has errors"; + ASSERT_OK(SendRequest(PrepareRenameRequest(params))); + + const json response = json::parse(GetResponse()); + EXPECT_EQ(response["result"], nullptr) << "Invalid result for id: "; +} + TEST_F(VerilogLanguageServerSymbolTableTest, RenameTestSymbolSingleFile) { // Create sample file and make sure diagnostics do not have errors std::string file_uri = @@ -1537,7 +1565,6 @@ TEST_F(VerilogLanguageServerSymbolTableTest, RenameTestSymbolSingleFile) { ASSERT_OK(SendRequest(mini_module)); const json diagnostics = json::parse(GetResponse()); - std::cout << diagnostics << std::endl; EXPECT_EQ(diagnostics["method"], "textDocument/publishDiagnostics") << "textDocument/publishDiagnostics not received"; @@ -1611,7 +1638,6 @@ TEST_F(VerilogLanguageServerSymbolTableTest, RenameTestSymbolMultipleFiles) { ASSERT_OK(SendRequest(request)); const json response = json::parse(GetResponse()); - std::cout << response << std::endl; EXPECT_EQ(response["result"]["changes"].size(), 2) << "Invalid result size for id: "; EXPECT_EQ(response["result"]["changes"][top_uri].size(), 1) @@ -1647,8 +1673,7 @@ TEST_F(VerilogLanguageServerSymbolTableTest, RenameTestPackageDistinction) { const verible::file::testing::ScopedTestFile module_foo(root_dir, renamesv, "rename.sv"); - const std::string mini_module = - DidOpenRequest("file://" + module_foo.filename(), renamesv); + const std::string mini_module = DidOpenRequest(file_uri, renamesv); ASSERT_OK(SendRequest(mini_module)); const json diagnostics = json::parse(GetResponse()); From 2a1ec3e1256bf14c0943acba6c6afe1794e94d50 Mon Sep 17 00:00:00 2001 From: Jan Bylicki Date: Tue, 20 Jun 2023 17:48:20 +0200 Subject: [PATCH 13/15] symbol-table-handler_test: Added handler tests for Find* methods Signed-off-by: Jan Bylicki --- verilog/tools/ls/symbol-table-handler_test.cc | 312 ++++++++++++++++++ 1 file changed, 312 insertions(+) diff --git a/verilog/tools/ls/symbol-table-handler_test.cc b/verilog/tools/ls/symbol-table-handler_test.cc index 2a40026d4..18b5fd978 100644 --- a/verilog/tools/ls/symbol-table-handler_test.cc +++ b/verilog/tools/ls/symbol-table-handler_test.cc @@ -17,6 +17,7 @@ #include #include "absl/strings/string_view.h" +#include "common/lsp/lsp-file-utils.h" #include "common/util/file_util.h" #include "gtest/gtest.h" #include "verilog/analysis/verilog_project.h" @@ -210,6 +211,317 @@ TEST(SymbolTableHandlerTest, DefinitionNotTrackedFile) { EXPECT_EQ(location.size(), 0); } +TEST(SymbolTableHandlerTest, + FindRenamableRangeAtCursorReturnsNullUntrackedFile) { + const auto tempdir = ::testing::TempDir(); + std::string sources_dir = verible::file::JoinPath(tempdir, __FUNCTION__); + ASSERT_TRUE(verible::file::CreateDir(sources_dir).ok()); + + absl::string_view filelist_content = "b.sv\n"; + + const verible::file::testing::ScopedTestFile filelist( + sources_dir, filelist_content, "verible.filelist"); + const verible::file::testing::ScopedTestFile module_a(sources_dir, + kSampleModuleA, "a.sv"); + const verible::file::testing::ScopedTestFile module_b(sources_dir, + kSampleModuleB, "b.sv"); + verible::lsp::PrepareRenameParams parameters; + parameters.textDocument.uri = + verible::lsp::PathToLSPUri(sources_dir + "/c.sv"); + parameters.position.line = 1; + parameters.position.character = 11; + + std::filesystem::absolute({sources_dir.begin(), sources_dir.end()}).string(); + std::shared_ptr project = std::make_shared( + sources_dir, std::vector(), ""); + SymbolTableHandler symbol_table_handler; + symbol_table_handler.SetProject(project); + + verilog::BufferTrackerContainer parsed_buffers; + parsed_buffers.AddChangeListener( + [&symbol_table_handler, parameters]( + const std::string &uri, + const verilog::BufferTracker *buffer_tracker) { + if (!buffer_tracker) { + symbol_table_handler.UpdateFileContent( + verible::lsp::LSPUriToPath(parameters.textDocument.uri), nullptr); + return; + } + if (!buffer_tracker->last_good()) return; + symbol_table_handler.UpdateFileContent( + verible::lsp::LSPUriToPath(parameters.textDocument.uri), + &buffer_tracker->last_good()->parser().Data()); + }); + + // Add trackers for the files we're going to process - normally done by the + // LSP but we don't have one + auto a_buffer = verible::lsp::EditTextBuffer(kSampleModuleA); + parsed_buffers.GetSubscriptionCallback()( + verible::lsp::PathToLSPUri(sources_dir + "/a.sv"), &a_buffer); + symbol_table_handler.BuildProjectSymbolTable(); + ASSERT_FALSE(parsed_buffers.FindBufferTrackerOrNull( + parameters.textDocument.uri) != nullptr); + + std::optional edit_range = + symbol_table_handler.FindRenameableRangeAtCursor(parameters, + parsed_buffers); + ASSERT_FALSE(edit_range.has_value()); +} + +TEST(SymbolTableHandlerTest, + FindRenamableRangeAtCursorReturnsNullDefinitionUnknown) { + const auto tempdir = ::testing::TempDir(); + std::string sources_dir = verible::file::JoinPath(tempdir, __FUNCTION__); + ASSERT_TRUE(verible::file::CreateDir(sources_dir).ok()); + + absl::string_view filelist_content = "b.sv\n"; + + const verible::file::testing::ScopedTestFile filelist( + sources_dir, filelist_content, "verible.filelist"); + const verible::file::testing::ScopedTestFile module_a(sources_dir, + kSampleModuleA, "a.sv"); + const verible::file::testing::ScopedTestFile module_b(sources_dir, + kSampleModuleB, "b.sv"); + verible::lsp::PrepareRenameParams parameters; + parameters.textDocument.uri = + verible::lsp::PathToLSPUri(sources_dir + "/b.sv"); + parameters.position.line = 3; + parameters.position.character = 5; + + std::filesystem::absolute({sources_dir.begin(), sources_dir.end()}).string(); + std::shared_ptr project = std::make_shared( + sources_dir, std::vector(), ""); + SymbolTableHandler symbol_table_handler; + symbol_table_handler.SetProject(project); + + verilog::BufferTrackerContainer parsed_buffers; + parsed_buffers.AddChangeListener( + [&symbol_table_handler, parameters]( + const std::string &uri, + const verilog::BufferTracker *buffer_tracker) { + if (!buffer_tracker) { + symbol_table_handler.UpdateFileContent( + verible::lsp::LSPUriToPath(parameters.textDocument.uri), nullptr); + return; + } + if (!buffer_tracker->last_good()) return; + symbol_table_handler.UpdateFileContent( + verible::lsp::LSPUriToPath(parameters.textDocument.uri), + &buffer_tracker->last_good()->parser().Data()); + }); + + // Add trackers for the files we're going to process - normally done by the + // LSP but we don't have one + auto b_buffer = verible::lsp::EditTextBuffer(kSampleModuleB); + parsed_buffers.GetSubscriptionCallback()(parameters.textDocument.uri, + &b_buffer); + symbol_table_handler.BuildProjectSymbolTable(); + ASSERT_TRUE(parsed_buffers.FindBufferTrackerOrNull( + parameters.textDocument.uri) != nullptr); + + std::optional edit_range = + symbol_table_handler.FindRenameableRangeAtCursor(parameters, + parsed_buffers); + ASSERT_FALSE(edit_range.has_value()); +} + +TEST(SymbolTableHandlerTest, FindRenamableRangeAtCursorReturnsLocation) { + const auto tempdir = ::testing::TempDir(); + std::string sources_dir = verible::file::JoinPath(tempdir, __FUNCTION__); + ASSERT_TRUE(verible::file::CreateDir(sources_dir).ok()); + + absl::string_view filelist_content = + "a.sv\n" + "b.sv\n"; + + const verible::file::testing::ScopedTestFile filelist( + sources_dir, filelist_content, "verible.filelist"); + const verible::file::testing::ScopedTestFile module_a(sources_dir, + kSampleModuleA, "a.sv"); + const verible::file::testing::ScopedTestFile module_b(sources_dir, + kSampleModuleB, "b.sv"); + verible::lsp::PrepareRenameParams parameters; + parameters.textDocument.uri = + verible::lsp::PathToLSPUri(sources_dir + "/a.sv"); + parameters.position.line = 1; + parameters.position.character = 11; + + std::filesystem::absolute({sources_dir.begin(), sources_dir.end()}).string(); + std::shared_ptr project = std::make_shared( + sources_dir, std::vector(), ""); + SymbolTableHandler symbol_table_handler; + symbol_table_handler.SetProject(project); + + verilog::BufferTrackerContainer parsed_buffers; + parsed_buffers.AddChangeListener( + [&symbol_table_handler, parameters]( + const std::string &uri, + const verilog::BufferTracker *buffer_tracker) { + if (!buffer_tracker) { + symbol_table_handler.UpdateFileContent( + verible::lsp::LSPUriToPath(parameters.textDocument.uri), nullptr); + return; + } + if (!buffer_tracker->last_good()) return; + symbol_table_handler.UpdateFileContent( + verible::lsp::LSPUriToPath(parameters.textDocument.uri), + &buffer_tracker->last_good()->parser().Data()); + }); + + // Add trackers for the files we're going to process - normally done by the + // LSP but we don't have one + auto a_buffer = verible::lsp::EditTextBuffer(kSampleModuleA); + parsed_buffers.GetSubscriptionCallback()(parameters.textDocument.uri, + &a_buffer); + symbol_table_handler.BuildProjectSymbolTable(); + ASSERT_TRUE(parsed_buffers.FindBufferTrackerOrNull( + parameters.textDocument.uri) != nullptr); + + std::optional edit_range = + symbol_table_handler.FindRenameableRangeAtCursor(parameters, + parsed_buffers); + ASSERT_TRUE(edit_range.has_value()); + EXPECT_EQ(edit_range.value().start.line, 1); + EXPECT_EQ(edit_range.value().start.character, 9); +} +TEST(SymbolTableHandlerTest, + FindRenameLocationsAndCreateEditsReturnsLocationsTest) { + const auto tempdir = ::testing::TempDir(); + std::string sources_dir = verible::file::JoinPath(tempdir, __FUNCTION__); + ASSERT_TRUE(verible::file::CreateDir(sources_dir).ok()); + + absl::string_view filelist_content = + "a.sv\n" + "b.sv\n"; + + const verible::file::testing::ScopedTestFile filelist( + sources_dir, filelist_content, "verible.filelist"); + const verible::file::testing::ScopedTestFile module_a(sources_dir, + kSampleModuleA, "a.sv"); + const verible::file::testing::ScopedTestFile module_b(sources_dir, + kSampleModuleB, "b.sv"); + verible::lsp::RenameParams parameters; + parameters.textDocument.uri = + verible::lsp::PathToLSPUri(sources_dir + "/a.sv"); + parameters.position.line = 1; + parameters.position.character = 11; + parameters.newName = "aaa"; + + std::filesystem::absolute({sources_dir.begin(), sources_dir.end()}).string(); + std::shared_ptr project = std::make_shared( + sources_dir, std::vector(), ""); + SymbolTableHandler symbol_table_handler; + symbol_table_handler.SetProject(project); + + verilog::BufferTrackerContainer parsed_buffers; + parsed_buffers.AddChangeListener( + [&symbol_table_handler, parameters]( + const std::string &uri, + const verilog::BufferTracker *buffer_tracker) { + if (!buffer_tracker) { + symbol_table_handler.UpdateFileContent( + verible::lsp::LSPUriToPath(parameters.textDocument.uri), nullptr); + return; + } + if (!buffer_tracker->last_good()) return; + symbol_table_handler.UpdateFileContent( + verible::lsp::LSPUriToPath(parameters.textDocument.uri), + &buffer_tracker->last_good()->parser().Data()); + }); + + // Add trackers for the files we're going to process - normally done by the + // LSP but we don't have one + auto a_buffer = verible::lsp::EditTextBuffer(kSampleModuleA); + parsed_buffers.GetSubscriptionCallback()(parameters.textDocument.uri, + &a_buffer); + symbol_table_handler.BuildProjectSymbolTable(); + ASSERT_TRUE(parsed_buffers.FindBufferTrackerOrNull( + parameters.textDocument.uri) != nullptr); + + verible::lsp::WorkspaceEdit edit_range = + symbol_table_handler.FindRenameLocationsAndCreateEdits(parameters, + parsed_buffers); + EXPECT_EQ(edit_range.changes[parameters.textDocument.uri].size(), 2); + EXPECT_EQ( + edit_range.changes[verible::lsp::PathToLSPUri(sources_dir + "/b.sv")] + .size(), + 1); +} + +TEST(SymbolTableHandlerTest, + FindRenameLocationsAndCreateEditsReturnsLocationsOnDirtyFilesTest) { + const auto tempdir = ::testing::TempDir(); + std::string sources_dir = verible::file::JoinPath(tempdir, __FUNCTION__); + ASSERT_TRUE(verible::file::CreateDir(sources_dir).ok()); + + absl::string_view filelist_content = + "a.sv\n" + "b.sv\n"; + + const verible::file::testing::ScopedTestFile filelist( + sources_dir, filelist_content, "verible.filelist"); + const verible::file::testing::ScopedTestFile module_a(sources_dir, + kSampleModuleA, "a.sv"); + const verible::file::testing::ScopedTestFile module_b(sources_dir, + kSampleModuleB, "b.sv"); + verible::lsp::RenameParams parameters; + parameters.textDocument.uri = + verible::lsp::PathToLSPUri(sources_dir + "/a.sv"); + parameters.position.line = 1; + parameters.position.character = 11; + parameters.newName = "aaa"; + + std::filesystem::absolute({sources_dir.begin(), sources_dir.end()}).string(); + std::shared_ptr project = std::make_shared( + sources_dir, std::vector(), ""); + SymbolTableHandler symbol_table_handler; + symbol_table_handler.SetProject(project); + + verilog::BufferTrackerContainer parsed_buffers; + parsed_buffers.AddChangeListener( + [&symbol_table_handler, parameters]( + const std::string &uri, + const verilog::BufferTracker *buffer_tracker) { + if (!buffer_tracker) { + symbol_table_handler.UpdateFileContent( + verible::lsp::LSPUriToPath(parameters.textDocument.uri), nullptr); + return; + } + if (!buffer_tracker->last_good()) return; + symbol_table_handler.UpdateFileContent( + verible::lsp::LSPUriToPath(parameters.textDocument.uri), + &buffer_tracker->last_good()->parser().Data()); + }); + + // Add trackers for the files we're going to process - normally done by the + // LSP but we don't have one + auto a_buffer = verible::lsp::EditTextBuffer(kSampleModuleA); + parsed_buffers.GetSubscriptionCallback()(parameters.textDocument.uri, + &a_buffer); + symbol_table_handler.BuildProjectSymbolTable(); + ASSERT_TRUE(parsed_buffers.FindBufferTrackerOrNull( + parameters.textDocument.uri) != nullptr); + + verible::lsp::WorkspaceEdit edit_range = + symbol_table_handler.FindRenameLocationsAndCreateEdits(parameters, + parsed_buffers); + EXPECT_EQ(edit_range.changes[parameters.textDocument.uri].size(), 2); + EXPECT_EQ( + edit_range.changes[verible::lsp::PathToLSPUri(sources_dir + "/b.sv")] + .size(), + 1); + + parameters.newName = "bbb"; + + edit_range = symbol_table_handler.FindRenameLocationsAndCreateEdits( + parameters, parsed_buffers); + EXPECT_EQ(edit_range.changes[parameters.textDocument.uri].size(), 2); + EXPECT_EQ( + edit_range.changes[verible::lsp::PathToLSPUri(sources_dir + "/b.sv")] + .size(), + 1); +} + TEST(SymbolTableHandlerTest, MissingVerilogProject) { SymbolTableHandler symbol_table_handler; std::vector diagnostics = From 2391058ff696f25c367c2e76ffa9356bb5d388a7 Mon Sep 17 00:00:00 2001 From: Jan Bylicki Date: Mon, 3 Jul 2023 17:16:34 +0200 Subject: [PATCH 14/15] symbol-table-handler: Added docstrings and changed location typing Signed-off-by: Jan Bylicki --- verilog/tools/ls/symbol-table-handler.cc | 6 +++--- verilog/tools/ls/symbol-table-handler.h | 7 ++++++- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/verilog/tools/ls/symbol-table-handler.cc b/verilog/tools/ls/symbol-table-handler.cc index f19160364..b34b48724 100644 --- a/verilog/tools/ls/symbol-table-handler.cc +++ b/verilog/tools/ls/symbol-table-handler.cc @@ -376,7 +376,7 @@ SymbolTableHandler::FindRenameableRangeAtCursor( } std::optional symbol = GetTokenInfoAtTextDocumentPosition(params, parsed_buffers); - if (symbol.has_value()) { + if (symbol) { verible::TokenInfo token = symbol.value(); const SymbolTableNode &root = symbol_table_->Root(); const SymbolTableNode *node = @@ -411,10 +411,10 @@ SymbolTableHandler::FindRenameLocationsAndCreateEdits( if (locations.empty()) return {}; std::map> file_edit_pairs; - for (auto &loc : locations) { + for (const auto &loc : locations) { file_edit_pairs[loc.uri].reserve(locations.size()); } - for (auto &loc : locations) { + for (const auto &loc : locations) { // TODO(jbylicki): Remove this band-aid fix once #1678 is merged - it should // fix // duplicate definition/references appending in modules and remove the need diff --git a/verilog/tools/ls/symbol-table-handler.h b/verilog/tools/ls/symbol-table-handler.h index 630f02ce3..d010d171a 100644 --- a/verilog/tools/ls/symbol-table-handler.h +++ b/verilog/tools/ls/symbol-table-handler.h @@ -95,11 +95,16 @@ class SymbolTableHandler { const verible::lsp::TextDocumentPositionParams ¶ms, const verilog::BufferTrackerContainer &parsed_buffers); - // TODO(jbylicki): Add docstring + // Returns a range in which a token exists in the file by the LSP request + // based on TextDocumentPositionParams. If text is not found, + // empty-initialized LineColumnRange is returned. verible::LineColumnRange GetTokenRangeAtTextDocumentPosition( const verible::lsp::TextDocumentPositionParams &document_cursor, const verilog::BufferTrackerContainer &parsed_buffers); + // Returns a TokenInfo of a token pointed at by the cursor in the file by the + // LSP request based on TextDocumentPositionParams. If text is not found, + // nullptr is returned. std::optional GetTokenInfoAtTextDocumentPosition( const verible::lsp::TextDocumentPositionParams ¶ms, const verilog::BufferTrackerContainer &parsed_buffers); From 8e909d90ce8b92b4bcff76661fc475e2678e2fe7 Mon Sep 17 00:00:00 2001 From: Jan Bylicki Date: Mon, 3 Jul 2023 17:22:37 +0200 Subject: [PATCH 15/15] symbol-table-handler_test: Added const to sources_dir declarations Signed-off-by: Jan Bylicki --- verilog/tools/ls/symbol-table-handler_test.cc | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/verilog/tools/ls/symbol-table-handler_test.cc b/verilog/tools/ls/symbol-table-handler_test.cc index 18b5fd978..cb01b8d05 100644 --- a/verilog/tools/ls/symbol-table-handler_test.cc +++ b/verilog/tools/ls/symbol-table-handler_test.cc @@ -214,7 +214,8 @@ TEST(SymbolTableHandlerTest, DefinitionNotTrackedFile) { TEST(SymbolTableHandlerTest, FindRenamableRangeAtCursorReturnsNullUntrackedFile) { const auto tempdir = ::testing::TempDir(); - std::string sources_dir = verible::file::JoinPath(tempdir, __FUNCTION__); + const std::string sources_dir = + verible::file::JoinPath(tempdir, __FUNCTION__); ASSERT_TRUE(verible::file::CreateDir(sources_dir).ok()); absl::string_view filelist_content = "b.sv\n"; @@ -271,7 +272,8 @@ TEST(SymbolTableHandlerTest, TEST(SymbolTableHandlerTest, FindRenamableRangeAtCursorReturnsNullDefinitionUnknown) { const auto tempdir = ::testing::TempDir(); - std::string sources_dir = verible::file::JoinPath(tempdir, __FUNCTION__); + const std::string sources_dir = + verible::file::JoinPath(tempdir, __FUNCTION__); ASSERT_TRUE(verible::file::CreateDir(sources_dir).ok()); absl::string_view filelist_content = "b.sv\n"; @@ -327,7 +329,8 @@ TEST(SymbolTableHandlerTest, TEST(SymbolTableHandlerTest, FindRenamableRangeAtCursorReturnsLocation) { const auto tempdir = ::testing::TempDir(); - std::string sources_dir = verible::file::JoinPath(tempdir, __FUNCTION__); + const std::string sources_dir = + verible::file::JoinPath(tempdir, __FUNCTION__); ASSERT_TRUE(verible::file::CreateDir(sources_dir).ok()); absl::string_view filelist_content = @@ -387,7 +390,8 @@ TEST(SymbolTableHandlerTest, FindRenamableRangeAtCursorReturnsLocation) { TEST(SymbolTableHandlerTest, FindRenameLocationsAndCreateEditsReturnsLocationsTest) { const auto tempdir = ::testing::TempDir(); - std::string sources_dir = verible::file::JoinPath(tempdir, __FUNCTION__); + const std::string sources_dir = + verible::file::JoinPath(tempdir, __FUNCTION__); ASSERT_TRUE(verible::file::CreateDir(sources_dir).ok()); absl::string_view filelist_content = @@ -451,7 +455,8 @@ TEST(SymbolTableHandlerTest, TEST(SymbolTableHandlerTest, FindRenameLocationsAndCreateEditsReturnsLocationsOnDirtyFilesTest) { const auto tempdir = ::testing::TempDir(); - std::string sources_dir = verible::file::JoinPath(tempdir, __FUNCTION__); + const std::string sources_dir = + verible::file::JoinPath(tempdir, __FUNCTION__); ASSERT_TRUE(verible::file::CreateDir(sources_dir).ok()); absl::string_view filelist_content =