Merge pull request #2091 from hzeller/20240131-always-update-project

Fix potential double-update of a buffer in Verilog Projecdt
chipsalliance · Feb 1, 2024 · 6c218e7 · 6c218e7
2 parents 2d84159 + fd195a5
commit 6c218e7
Show file tree

Hide file tree

Showing 7 changed files with 100 additions and 106 deletions.
diff --git a/verilog/tools/ls/lsp-parse-buffer.cc b/verilog/tools/ls/lsp-parse-buffer.cc
@@ -59,6 +59,7 @@ ParsedBuffer::ParsedBuffer(int64_t version, absl::string_view uri,
 void BufferTracker::Update(const std::string &uri,
                            const verible::lsp::EditTextBuffer &txt) {
   if (current_ && current_->version() == txt.last_global_version()) {
+    LOG(DFATAL) << "Testing: Forgot to update version number ?";
     return;  // Nothing to do (we don't really expect this to happen)
   }
   txt.RequestContent([&txt, &uri, this](absl::string_view content) {

diff --git a/verilog/tools/ls/lsp-parse-buffer_test.cc b/verilog/tools/ls/lsp-parse-buffer_test.cc
@@ -98,9 +98,11 @@ TEST(BufferTrackerConatainer, ParseUpdateNotification) {
 
   EXPECT_EQ(update_remove_count, 0);
 
+  int64_t version_number = 0;
   auto feed_callback = container.GetSubscriptionCallback();
   // Put one document in there.
   verible::lsp::EditTextBuffer foo_doc("module foo(); endmodule");
+  foo_doc.set_last_global_version(++version_number);
   feed_callback("foo.sv", &foo_doc);
 
   EXPECT_EQ(update_remove_count, 1);
@@ -110,6 +112,7 @@ TEST(BufferTrackerConatainer, ParseUpdateNotification) {
   EXPECT_NE(tracker->last_good().get(), nullptr);
 
   verible::lsp::EditTextBuffer updated_foo_doc("module foobar(); endmodule");
+  updated_foo_doc.set_last_global_version(++version_number);
   feed_callback("foo.sv", &updated_foo_doc);
   EXPECT_EQ(update_remove_count, 2);
 

diff --git a/verilog/tools/ls/symbol-table-handler.cc b/verilog/tools/ls/symbol-table-handler.cc
@@ -155,13 +155,15 @@ bool SymbolTableHandler::LoadProjectFileList(absl::string_view current_dir) {
     }
     filelist_path_ = projectpath;
   }
+
   if (!last_filelist_update_) {
     last_filelist_update_ = std::filesystem::last_write_time(filelist_path_);
   } else if (*last_filelist_update_ ==
              std::filesystem::last_write_time(filelist_path_)) {
     // filelist file is unchanged, keeping it
     return true;
   }
+
   VLOG(1) << "Updating the filelist";
   // fill the FileList object
   FileList filelist;
@@ -174,6 +176,7 @@ bool SymbolTableHandler::LoadProjectFileList(absl::string_view current_dir) {
     last_filelist_update_ = {};
     return false;
   }
+
   // add directory containing filelist to includes
   // TODO (glatosinski): should we do this?
   const absl::string_view filelist_dir = verible::file::Dirname(filelist_path_);
@@ -184,6 +187,7 @@ bool SymbolTableHandler::LoadProjectFileList(absl::string_view current_dir) {
     VLOG(1) << "Adding include path:  " << incdir;
     curr_project_->AddIncludePath(incdir);
   }
+
   // Add files from file list to the project
   VLOG(1) << "Resolving " << filelist.file_paths.size() << " files.";
   int actually_opened = 0;
@@ -202,6 +206,11 @@ bool SymbolTableHandler::LoadProjectFileList(absl::string_view current_dir) {
     ++actually_opened;
   }
 
+  // It could be that we just (re-) opened all the exactly same files, so
+  // setting files_dirty_ here might overstate it. However, good conservative
+  // estimate.
+  files_dirty_ |= (actually_opened > 0);
+
   VLOG(1) << "Successfully opened " << actually_opened
           << " files from file-list: " << (absl::Now() - start);
   return true;
@@ -254,9 +263,7 @@ const SymbolTableNode *SymbolTableHandler::ScanSymbolTreeForDefinition(
 
 void SymbolTableHandler::Prepare() {
   LoadProjectFileList(curr_project_->TranslationUnitRoot());
-  if (files_dirty_) {
-    BuildProjectSymbolTable();
-  }
+  if (files_dirty_) BuildProjectSymbolTable();
 }
 
 std::optional<verible::TokenInfo>
@@ -383,9 +390,7 @@ std::vector<verible::lsp::Location> SymbolTableHandler::FindDefinitionLocation(
 
 const verible::Symbol *SymbolTableHandler::FindDefinitionSymbol(
     absl::string_view symbol) {
-  if (files_dirty_) {
-    BuildProjectSymbolTable();
-  }
+  Prepare();
   const SymbolTableNode *symbol_table_node =
       ScanSymbolTreeForDefinition(&symbol_table_->Root(), symbol);
   if (symbol_table_node) return symbol_table_node->Value().syntax_origin;
@@ -413,9 +418,7 @@ SymbolTableHandler::FindRenameableRangeAtCursor(
     const verible::lsp::PrepareRenameParams &params,
     const verilog::BufferTrackerContainer &parsed_buffers) {
   Prepare();
-  if (files_dirty_) {
-    BuildProjectSymbolTable();
-  }
+
   std::optional<verible::TokenInfo> symbol =
       GetTokenInfoAtTextDocumentPosition(params, parsed_buffers);
   if (symbol) {
@@ -434,10 +437,8 @@ verible::lsp::WorkspaceEdit
 SymbolTableHandler::FindRenameLocationsAndCreateEdits(
     const verible::lsp::RenameParams &params,
     const verilog::BufferTrackerContainer &parsed_buffers) {
-  if (files_dirty_) {
-    BuildProjectSymbolTable();
-  }
   Prepare();
+
   absl::string_view symbol =
       GetTokenAtTextDocumentPosition(params, parsed_buffers);
   const SymbolTableNode &root = symbol_table_->Root();
@@ -515,4 +516,21 @@ void SymbolTableHandler::UpdateFileContent(
   curr_project_->UpdateFileContents(path, parsed);
 }
 
+BufferTrackerContainer::ChangeCallback
+SymbolTableHandler::CreateBufferTrackerListener() {
+  return [this](const std::string &uri,
+                const verilog::BufferTracker *buffer_tracker) {
+    const std::string path = verible::lsp::LSPUriToPath(uri);
+    if (path.empty()) {
+      LOG(ERROR) << "Could not convert LS URI to path:  " << uri;
+      return;
+    }
+    // Note, if we actually got any result we must use it here to update
+    // the file content, as the old one will be deleted.
+    // So must use current() as last_good() might be nullptr.
+    UpdateFileContent(
+        path, buffer_tracker ? &buffer_tracker->current()->parser() : nullptr);
+  };
+}
+
 };  // namespace verilog
diff --git a/verilog/tools/ls/symbol-table-handler.h b/verilog/tools/ls/symbol-table-handler.h
@@ -71,10 +71,6 @@ class SymbolTableHandler {
   std::optional<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,
-  // opens the given file instead.
-  void UpdateFileContent(absl::string_view path,
-                         const verilog::VerilogAnalyzer *parsed);
 
   verible::lsp::WorkspaceEdit FindRenameLocationsAndCreateEdits(
       const verible::lsp::RenameParams &params,
@@ -83,6 +79,15 @@ class SymbolTableHandler {
   // Creates a symbol table for entire project (public: needed in unit-test)
   std::vector<absl::Status> BuildProjectSymbolTable();
 
+  // Provide new parsed content for the given path. If "content" is nullptr,
+  // opens the given file instead.
+  void UpdateFileContent(absl::string_view path,
+                         const verilog::VerilogAnalyzer *parsed);
+
+  // Create a listener to be wired up to a buffer tracker. Whenever we
+  // there is a change in the editor, this will update our internal project.
+  BufferTrackerContainer::ChangeCallback CreateBufferTrackerListener();
+
  private:
   // prepares structures for symbol-based requests
   void Prepare();

diff --git a/verilog/tools/ls/symbol-table-handler_test.cc b/verilog/tools/ls/symbol-table-handler_test.cc
@@ -242,19 +242,7 @@ TEST(SymbolTableHandlerTest,
 
   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());
-      });
+      symbol_table_handler.CreateBufferTrackerListener());
 
   // Add trackers for the files we're going to process - normally done by the
   // LSP but we don't have one
@@ -304,19 +292,7 @@ TEST(SymbolTableHandlerTest,
 
   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());
-      });
+      symbol_table_handler.CreateBufferTrackerListener());
 
   // Add trackers for the files we're going to process - normally done by the
   // LSP but we don't have one
@@ -366,19 +342,7 @@ TEST(SymbolTableHandlerTest, FindRenamableRangeAtCursorReturnsLocation) {
 
   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());
-      });
+      symbol_table_handler.CreateBufferTrackerListener());
 
   // Add trackers for the files we're going to process - normally done by the
   // LSP but we don't have one
@@ -430,19 +394,7 @@ TEST(SymbolTableHandlerTest,
 
   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());
-      });
+      symbol_table_handler.CreateBufferTrackerListener());
 
   // Add trackers for the files we're going to process - normally done by the
   // LSP but we don't have one
@@ -495,19 +447,7 @@ TEST(SymbolTableHandlerTest,
 
   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());
-      });
+      symbol_table_handler.CreateBufferTrackerListener());
 
   // Add trackers for the files we're going to process - normally done by the
   // LSP but we don't have one
@@ -538,6 +478,56 @@ TEST(SymbolTableHandlerTest,
       1);
 }
 
+TEST(SymbolTableHandlerTest, UpdateWithUnparseableEditorContentRegression) {
+  const auto tempdir = ::testing::TempDir();
+  const std::string sources_dir =
+      verible::file::JoinPath(tempdir, __FUNCTION__);
+  ASSERT_TRUE(verible::file::CreateDir(sources_dir).ok());
+
+  absl::string_view filelist_content = "";
+  const verible::file::testing::ScopedTestFile filelist(
+      sources_dir, filelist_content, "verible.filelist");
+  const std::string uri = verible::lsp::PathToLSPUri(sources_dir + "/a.sv");
+  std::filesystem::absolute({sources_dir.begin(), sources_dir.end()}).string();
+  std::shared_ptr<VerilogProject> project = std::make_shared<VerilogProject>(
+      sources_dir, std::vector<std::string>(), "");
+  SymbolTableHandler symbol_table_handler;
+  symbol_table_handler.SetProject(project);
+
+  verilog::BufferTrackerContainer parsed_buffers;
+  parsed_buffers.AddChangeListener(
+      symbol_table_handler.CreateBufferTrackerListener());
+
+  // We want to make sure that every change updates the project file with
+  // the latest content.
+  //
+  // The verilog_project would react badly if it gets the exact same content
+  // twice, as it would want to register its string_view locations in its
+  // reverse map.
+  //
+  // If we'd filter to only send 'last_good()' content, which stays the same
+  // while we have bad content, we'd attempt to register the same range.
+  // multiple times with the project.
+  //
+  // So walking through the sequence good content (sets current() and
+  // last_good()), parse error content (sets only current(), but leaves
+  // last_good() as-is), and good content again (replaces current() as well
+  // as last_good()), we make sure that this sequence will work.
+  // Give our file list a valid content
+  auto a_buffer = verible::lsp::EditTextBuffer(kSampleModuleA);
+  a_buffer.set_last_global_version(1);
+  parsed_buffers.GetSubscriptionCallback()(uri, &a_buffer);
+
+  // Now, the content in the editor becomes invalid.
+  auto broken_buffer = verible::lsp::EditTextBuffer("invalid-file");
+  broken_buffer.set_last_global_version(2);
+  parsed_buffers.GetSubscriptionCallback()(uri, &broken_buffer);
+
+  // ... back to a valid parsed file. This replaces the previously broken file.
+  a_buffer.set_last_global_version(3);
+  parsed_buffers.GetSubscriptionCallback()(uri, &a_buffer);
+}
+
 TEST(SymbolTableHandlerTest, MissingVerilogProject) {
   SymbolTableHandler symbol_table_handler;
   std::vector<absl::Status> diagnostics =

diff --git a/verilog/tools/ls/verilog-language-server.cc b/verilog/tools/ls/verilog-language-server.cc
@@ -233,11 +233,9 @@ void VerilogLanguageServer::ConfigureProject(absl::string_view project_root) {
       proj_root, std::vector<std::string>(), "");
   symbol_table_handler_.SetProject(proj);
 
+  // Whenever an updated buffer in editor is available, update symbol table.
   parsed_buffers_.AddChangeListener(
-      [this](const std::string &uri,
-             const verilog::BufferTracker *buffer_tracker) {
-        UpdateEditedFileInProject(uri, buffer_tracker);
-      });
+      symbol_table_handler_.CreateBufferTrackerListener());
 }
 
 void VerilogLanguageServer::SendDiagnostics(
@@ -258,21 +256,4 @@ void VerilogLanguageServer::SendDiagnostics(
   dispatcher_.SendNotification("textDocument/publishDiagnostics", params);
 }
 
-void VerilogLanguageServer::UpdateEditedFileInProject(
-    const std::string &uri, const verilog::BufferTracker *buffer_tracker) {
-  const std::string path = verible::lsp::LSPUriToPath(uri);
-  if (path.empty()) {
-    LOG(ERROR) << "Could not convert LS URI to path:  " << uri;
-    return;
-  }
-  if (!buffer_tracker) {
-    symbol_table_handler_.UpdateFileContent(path, nullptr);
-    return;
-  }
-  if (!buffer_tracker->last_good()) return;
-  symbol_table_handler_.UpdateFileContent(
-      path, &buffer_tracker->last_good()->parser());
-  VLOG(1) << "Updated file:  " << uri << " (" << path << ")";
-}
-
 };  // namespace verilog
diff --git a/verilog/tools/ls/verilog-language-server.h b/verilog/tools/ls/verilog-language-server.h
@@ -73,10 +73,6 @@ class VerilogLanguageServer {
   void SendDiagnostics(const std::string &uri,
                        const verilog::BufferTracker &buffer_tracker);
 
-  // Updates file contents in the project on change in Language Server Client
-  void UpdateEditedFileInProject(const std::string &uri,
-                                 const verilog::BufferTracker *buffer_tracker);
-
   // Stream splitter splits the input stream into messages (header/body).
   verible::lsp::MessageStreamSplitter stream_splitter_;