Skip to content

Commit

Permalink
LSP parse buffer: keep old parses alive whlie calling change listener.
Browse files Browse the repository at this point in the history
When updating the parse buffers, make sure the old ones don't disappear
while we still call the callbacks for everyone interested in the changes.

They might've hold on to some data and expect the old one to be still
viable when they get the update call.

Add a unit test.

Issues: chipsalliance#2078
While at it: update and refine documentation.
  • Loading branch information
hzeller committed Jan 28, 2024
1 parent a4d61b1 commit dd8ee8a
Show file tree
Hide file tree
Showing 4 changed files with 163 additions and 18 deletions.
11 changes: 11 additions & 0 deletions verilog/tools/ls/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,17 @@ cc_library(
],
)

cc_test(
name = "lsp-parse-buffer_test",
srcs = ["lsp-parse-buffer_test.cc"],
deps = [
":lsp-parse-buffer",
"//common/lsp:lsp-text-buffer",
"@com_google_absl//absl/strings",
"@com_google_googletest//:gtest_main",
],
)

cc_library(
name = "verible-lsp-adapter",
srcs = ["verible-lsp-adapter.cc"],
Expand Down
21 changes: 15 additions & 6 deletions verilog/tools/ls/lsp-parse-buffer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -50,20 +50,19 @@ ParsedBuffer::ParsedBuffer(int64_t version, absl::string_view uri,
content, uri)) {
VLOG(1) << "Analyzed " << uri << " lex:" << parser_->LexStatus()
<< "; parser:" << parser_->ParseStatus() << std::endl;
// TODO(hzeller): we should use a filename not URI; strip prefix.
// TODO(hzeller): should we use a filename not URI ?
if (auto lint_result = RunLinter(uri, *parser_); lint_result.ok()) {
lint_statuses_ = std::move(lint_result.value());
}
}

void BufferTracker::Update(const std::string &filename,
void BufferTracker::Update(const std::string &uri,
const verible::lsp::EditTextBuffer &txt) {
if (current_ && current_->version() == txt.last_global_version()) {
return; // Nothing to do (we don't really expect this to happen)
}
txt.RequestContent([&txt, &filename, this](absl::string_view content) {
current_ = std::make_shared<ParsedBuffer>(txt.last_global_version(),
filename, content);
txt.RequestContent([&txt, &uri, this](absl::string_view content) {
current_.reset(new ParsedBuffer(txt.last_global_version(), uri, content));
});
if (current_->parsed_successfully()) {
last_good_ = current_;
Expand All @@ -74,9 +73,19 @@ verible::lsp::BufferCollection::UriBufferCallback
BufferTrackerContainer::GetSubscriptionCallback() {
return
[this](const std::string &uri, const verible::lsp::EditTextBuffer *txt) {
// The Update() might replace, thus discard, old parsed buffers.
// However, the change listeners we're about to inform might
// expect them to be still alive while the update takes place.
// So hold on to them here until all updates are performed.
// (this copy is cheap as it is just reference counted pointers).
BufferTracker remember_previous;
if (const BufferTracker *tracker = FindBufferTrackerOrNull(uri)) {
remember_previous = *tracker;
}

if (txt) {
const BufferTracker *tracker = Update(uri, *txt);
// Now inform our listeners.
// Updated current() and last_good(); Now inform our listeners.
for (const auto &change_listener : change_listeners_) {
change_listener(uri, tracker);
}
Expand Down
29 changes: 17 additions & 12 deletions verilog/tools/ls/lsp-parse-buffer.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,16 @@ class ParsedBuffer {
std::vector<verible::LintRuleStatus> lint_statuses_;
};

// A buffer tracker tracks the EditTextBuffer content and keeps up to
// two versions of ParsedBuffers - the latest, that might have parse errors,
// and the last known good that parsed without errors (if available).
// A buffer tracker tracks of a single file EditTextBuffer content and stores
// a parsed version.
// It keeps up to two versions of ParsedBuffers - the latest, that might have
// parse errors, and the last known good that parsed without errors
// (if available).
class BufferTracker {
public:
void Update(const std::string &filename,
const verible::lsp::EditTextBuffer &txt);
// Update with a changed text buffer from the LSP subsystem. Triggers
// re-parsing and updating our current() and potentially last_good().
void Update(const std::string &uri, const verible::lsp::EditTextBuffer &txt);

// ---
// Thread guarantee for the following functions.
Expand All @@ -80,19 +83,20 @@ class BufferTracker {
// ---

// Get the current ParsedBuffer from the last text update we received
// from the editor. This can be nullptr if it could not be parsed.
// from the editor.
//
// Use in operations that only really makes sense on the latest view and
// only if it was parseable, e.g. suggesting edits.
// Use in operations that only really makes sense on the latest view,
// e.g. suggesting edits.
std::shared_ptr<const ParsedBuffer> current() const { return current_; }

// Get the ParsedBuffer that represents that last time we were able to
// parse the document from the editor correctly. This can be the same
// as current() if the last text update was fully parseable, or nullptr
// if we never received a buffer that was parseable.
//
// Use in operations that focus on returning something even it it is slightly
// outdated, e.g. finding a particular symbol.
// Use in operations that focus on returning something that requires a
// valid parsed file even it it is slightly outdated, e.g. finding
// a particular symbol.
std::shared_ptr<const ParsedBuffer> last_good() const { return last_good_; }

private:
Expand All @@ -107,9 +111,10 @@ class BufferTracker {
std::shared_ptr<const ParsedBuffer> last_good_;
};

// Container holding all buffer trackers keyed by file uri.
// Container holding a buffer tracker per file uri.
// This is the correspondent to verible::lsp::BufferCollection that
// internally stores
// internally stores file content by uri. Here we keep parsed files per uri,
// whenever we're informed of a change in the buffer collection.
class BufferTrackerContainer {
public:
// Return a callback that allows to subscribe to an lsp::BufferCollection
Expand Down
120 changes: 120 additions & 0 deletions verilog/tools/ls/lsp-parse-buffer_test.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
// Copyright 2024 The Verible Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#include "verilog/tools/ls/lsp-parse-buffer.h"

#include "common/lsp/lsp-text-buffer.h"
#include "gtest/gtest.h"

namespace verilog {
namespace {
TEST(BufferTraccker, ParseGoodDocument) {
verible::lsp::EditTextBuffer good_document("module foo(); endmodule");
BufferTracker tracker;
EXPECT_EQ(tracker.current().get(), nullptr);
tracker.Update("foo.sv", good_document);

ASSERT_NE(tracker.current().get(), nullptr);

// Successfully parsed, so last_good() == current()
EXPECT_EQ(tracker.last_good().get(), tracker.current().get());
}

TEST(BufferTraccker, ParseParseErrorDocument) {
verible::lsp::EditTextBuffer bad_document("moduleinvalid foo(); endmodule");
BufferTracker tracker;
tracker.Update("foo.sv", bad_document);

ASSERT_NE(tracker.current().get(), nullptr); // a current document, maybe bad

// Not successfully parsed, so last_good() is still null.
ASSERT_EQ(tracker.last_good().get(), nullptr);
}

TEST(BufferTrackerConatainer, PopulateBufferCollection) {
BufferTrackerContainer container;
auto feed_callback = container.GetSubscriptionCallback();

verible::lsp::EditTextBuffer foo_doc("module foo(); endmodule");
feed_callback("foo.sv", &foo_doc);
const BufferTracker *tracker = container.FindBufferTrackerOrNull("foo.sv");
ASSERT_NE(tracker, nullptr);
EXPECT_NE(tracker->last_good().get(), nullptr);

verible::lsp::EditTextBuffer bar_doc("modulebroken bar(); endmodule");
feed_callback("bar.sv", &bar_doc);
tracker = container.FindBufferTrackerOrNull("bar.sv");
ASSERT_NE(tracker, nullptr);
EXPECT_NE(tracker->current().get(), nullptr); // Current always there
EXPECT_EQ(tracker->last_good().get(), nullptr); // Was a bad parse

feed_callback("bar.sv", nullptr); // Remove.
tracker = container.FindBufferTrackerOrNull("bar.sv");
EXPECT_EQ(tracker, nullptr);
}

TEST(BufferTrackerConatainer, ParseUpdateNotification) {
BufferTrackerContainer container;

const verible::TextStructureView *last_text_structure = nullptr;
int update_remove_count = 0; // track these
container.AddChangeListener([&update_remove_count, &last_text_structure](
const std::string &,
const BufferTracker *tracker) {
if (tracker != nullptr) {
++update_remove_count;
} else {
--update_remove_count;
};

// attempt to access the previous text structure if it was not null to
// make sure it was not deleted. If it was, then this UB might only be
// detected in the ASAN test.
if (last_text_structure) {
// Accessing the content and see if it is alive and well, not corrupted.
EXPECT_TRUE(absl::StartsWith(last_text_structure->Contents(), "module"));
}

// Remember current text structure for last time.
if (tracker) {
last_text_structure = &tracker->current()->parser().Data();
} else {
last_text_structure = nullptr;
}
});

EXPECT_EQ(update_remove_count, 0);

auto feed_callback = container.GetSubscriptionCallback();
// Put one document in there.
verible::lsp::EditTextBuffer foo_doc("module foo(); endmodule");
feed_callback("foo.sv", &foo_doc);

EXPECT_EQ(update_remove_count, 1);

const BufferTracker *tracker = container.FindBufferTrackerOrNull("foo.sv");
ASSERT_NE(tracker, nullptr);
EXPECT_NE(tracker->last_good().get(), nullptr);

verible::lsp::EditTextBuffer updated_foo_doc("module foobar(); endmodule");
feed_callback("foo.sv", &updated_foo_doc);
EXPECT_EQ(update_remove_count, 2);

// Remove doc
feed_callback("foo.sv", nullptr);
EXPECT_EQ(update_remove_count, 1);
}

} // namespace
} // namespace verilog

0 comments on commit dd8ee8a

Please sign in to comment.