[wip] lsp: Fixed module instantiation renaming

In the current state of the commit it is NOT acutally fixed, but it is a WIP towards a solution Signed-off-by: Jan Bylicki <[email protected]>
chipsalliance · Jun 19, 2023 · 43b0d16 · 43b0d16
1 parent b4e16f4
commit 43b0d16
Show file tree

Hide file tree

Showing 5 changed files with 36 additions and 25 deletions.
diff --git 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
@@ -248,22 +248,23 @@ absl::string_view SymbolTableHandler::GetTokenAtTextDocumentPosition(
 
 verible::LineColumnRange
 SymbolTableHandler::GetTokenRangeAtTextDocumentPosition(
-    const verible::lsp::TextDocumentPositionParams &params,
+    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<const ParsedBuffer> 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<verible::lsp::Location> SymbolTableHandler::FindReferencesLocations(
   return locations;
 }
 
-verible::lsp::Range SymbolTableHandler::FindRenameLocations(
+verible::lsp::Range SymbolTableHandler::FindRenameableRangeAtCursor(
     const verible::lsp::PrepareRenameParams &params,
     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
@@ -65,7 +65,7 @@ class SymbolTableHandler {
       const verible::lsp::ReferenceParams &params,
       const verilog::BufferTrackerContainer &parsed_buffers);
 
-  verible::lsp::Range FindRenameLocations(
+  verible::lsp::Range FindRenameableRangeAtCursor(
       const verible::lsp::PrepareRenameParams &params,
       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 &params,
+      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
@@ -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
@@ -33,7 +33,6 @@
   else                                               \
     EXPECT_TRUE(status__.ok()) << status__
 
-using verible::lsp::PathToLSPUri;
 namespace verilog {
 namespace {
 
@@ -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(verible::lsp::LSPUriToPath(diagnostics["params"]["uri"]), verible::lsp::LSPUriToPath(file_uri))
       << "Diagnostics for invalid file";
 
   EXPECT_EQ(diagnostics["params"]["diagnostics"].size(), 0)