From 75463fff739668dd75500b0bff9be4aacf747c06 Mon Sep 17 00:00:00 2001 From: Henner Zeller Date: Wed, 31 Jan 2024 23:41:05 -0800 Subject: [PATCH 1/2] Refactor callback handling for symbol-table update. Instead of in the language server, define the callback that updates the symbol table in the symbol table itself (CreateBufferTrackerListener()). Use that in various places, including the unit tests that it simplifies. --- verilog/tools/ls/symbol-table-handler.cc | 43 ++++++++---- verilog/tools/ls/symbol-table-handler.h | 13 ++-- verilog/tools/ls/symbol-table-handler_test.cc | 70 ++----------------- verilog/tools/ls/verilog-language-server.cc | 23 +----- verilog/tools/ls/verilog-language-server.h | 4 -- 5 files changed, 47 insertions(+), 106 deletions(-) diff --git a/verilog/tools/ls/symbol-table-handler.cc b/verilog/tools/ls/symbol-table-handler.cc index dfd8fbbf8..630320a41 100644 --- a/verilog/tools/ls/symbol-table-handler.cc +++ b/verilog/tools/ls/symbol-table-handler.cc @@ -155,6 +155,7 @@ 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_ == @@ -162,6 +163,7 @@ bool SymbolTableHandler::LoadProjectFileList(absl::string_view current_dir) { // 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 @@ -383,9 +390,7 @@ std::vector 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 ¶ms, const verilog::BufferTrackerContainer &parsed_buffers) { Prepare(); - if (files_dirty_) { - BuildProjectSymbolTable(); - } + std::optional symbol = GetTokenInfoAtTextDocumentPosition(params, parsed_buffers); if (symbol) { @@ -434,10 +437,8 @@ 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(); @@ -515,4 +516,22 @@ 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; + } + if (!buffer_tracker) { + UpdateFileContent(path, nullptr); + return; + } + if (!buffer_tracker->last_good()) return; + UpdateFileContent(path, &buffer_tracker->last_good()->parser()); + }; +} + }; // namespace verilog diff --git a/verilog/tools/ls/symbol-table-handler.h b/verilog/tools/ls/symbol-table-handler.h index 587a2a076..4f2d57ed2 100644 --- a/verilog/tools/ls/symbol-table-handler.h +++ b/verilog/tools/ls/symbol-table-handler.h @@ -71,10 +71,6 @@ class SymbolTableHandler { 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, - // opens the given file instead. - void UpdateFileContent(absl::string_view path, - const verilog::VerilogAnalyzer *parsed); verible::lsp::WorkspaceEdit FindRenameLocationsAndCreateEdits( const verible::lsp::RenameParams ¶ms, @@ -83,6 +79,15 @@ class SymbolTableHandler { // Creates a symbol table for entire project (public: needed in unit-test) std::vector 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 index 46aab8309..76be6bdb8 100644 --- 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 diff --git a/verilog/tools/ls/verilog-language-server.cc b/verilog/tools/ls/verilog-language-server.cc index 95071320f..b43134824 100644 --- 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(), ""); 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 index f63b6a27a..eb67f78c3 100644 --- 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_; From fd195a5460ac4c9abd00e161b6b011336849ba30 Mon Sep 17 00:00:00 2001 From: Henner Zeller Date: Thu, 1 Feb 2024 00:33:25 -0800 Subject: [PATCH 2/2] Fix project-file issue: Admit all changes not just the parseable ones. Explained in the unit test. TL;DR: if we only update with last_good() content, we might update with the same good parse multiple times, which the string range index will detect and CHECK()-complain. --- verilog/tools/ls/lsp-parse-buffer.cc | 1 + verilog/tools/ls/lsp-parse-buffer_test.cc | 3 ++ verilog/tools/ls/symbol-table-handler.cc | 11 ++-- verilog/tools/ls/symbol-table-handler_test.cc | 50 +++++++++++++++++++ 4 files changed, 59 insertions(+), 6 deletions(-) diff --git a/verilog/tools/ls/lsp-parse-buffer.cc b/verilog/tools/ls/lsp-parse-buffer.cc index d93c387bf..abcd4a15f 100644 --- 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 index 743fe6835..f314e9893 100644 --- 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 index 630320a41..5e56d59a3 100644 --- a/verilog/tools/ls/symbol-table-handler.cc +++ b/verilog/tools/ls/symbol-table-handler.cc @@ -525,12 +525,11 @@ SymbolTableHandler::CreateBufferTrackerListener() { LOG(ERROR) << "Could not convert LS URI to path: " << uri; return; } - if (!buffer_tracker) { - UpdateFileContent(path, nullptr); - return; - } - if (!buffer_tracker->last_good()) return; - UpdateFileContent(path, &buffer_tracker->last_good()->parser()); + // 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); }; } diff --git a/verilog/tools/ls/symbol-table-handler_test.cc b/verilog/tools/ls/symbol-table-handler_test.cc index 76be6bdb8..0996b7b87 100644 --- a/verilog/tools/ls/symbol-table-handler_test.cc +++ b/verilog/tools/ls/symbol-table-handler_test.cc @@ -478,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 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.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 diagnostics =