[clangd] Report diagnostics even if WantDiags::No AST was reused
Summary: After r338256, clangd stopped reporting diagnostics if WantDiags::No request is followed by a WantDiags::Yes request but the AST can be reused. Reviewers: ioeric Reviewed By: ioeric Subscribers: javed.absar, MaskRay, jkorous, arphaman, jfb, cfe-commits Differential Revision: https://reviews.llvm.org/D50045 llvm-svn: 338361
This commit is contained in:
parent
85673b083a
commit
442c283218
|
@ -228,6 +228,9 @@ private:
|
|||
Semaphore &Barrier;
|
||||
/// Inputs, corresponding to the current state of AST.
|
||||
ParseInputs FileInputs;
|
||||
/// Whether the diagnostics for the current FileInputs were reported to the
|
||||
/// users before.
|
||||
bool DiagsWereReported = false;
|
||||
/// Size of the last AST
|
||||
/// Guards members used by both TUScheduler and the worker thread.
|
||||
mutable std::mutex Mutex;
|
||||
|
@ -330,7 +333,9 @@ void ASTWorker::update(
|
|||
std::tie(Inputs.CompileCommand, Inputs.Contents);
|
||||
|
||||
tooling::CompileCommand OldCommand = std::move(FileInputs.CompileCommand);
|
||||
bool PrevDiagsWereReported = DiagsWereReported;
|
||||
FileInputs = Inputs;
|
||||
DiagsWereReported = false;
|
||||
|
||||
log("Updating file {0} with command [{1}] {2}", FileName,
|
||||
Inputs.CompileCommand.Directory,
|
||||
|
@ -366,34 +371,44 @@ void ASTWorker::update(
|
|||
OldPreamble.reset();
|
||||
PreambleWasBuilt.notify();
|
||||
|
||||
if (CanReuseAST) {
|
||||
// Take a shortcut and don't build the AST if neither the inputs nor the
|
||||
// preamble have changed.
|
||||
// Note that we do not report the diagnostics, since they should not have
|
||||
// changed either. All the clients should handle the lack of OnUpdated()
|
||||
// call anyway to handle empty result from buildAST.
|
||||
// FIXME(ibiryukov): the AST could actually change if non-preamble
|
||||
// includes changed, but we choose to ignore it.
|
||||
// FIXME(ibiryukov): should we refresh the cache in IdleASTs for the
|
||||
// current file at this point?
|
||||
log("Skipping rebuild of the AST for {0}, inputs are the same.",
|
||||
FileName);
|
||||
return;
|
||||
if (!CanReuseAST) {
|
||||
IdleASTs.take(this); // Remove the old AST if it's still in cache.
|
||||
} else {
|
||||
// Since we don't need to rebuild the AST, we might've already reported
|
||||
// the diagnostics for it.
|
||||
if (PrevDiagsWereReported) {
|
||||
DiagsWereReported = true;
|
||||
// Take a shortcut and don't report the diagnostics, since they should
|
||||
// not changed. All the clients should handle the lack of OnUpdated()
|
||||
// call anyway to handle empty result from buildAST.
|
||||
// FIXME(ibiryukov): the AST could actually change if non-preamble
|
||||
// includes changed, but we choose to ignore it.
|
||||
// FIXME(ibiryukov): should we refresh the cache in IdleASTs for the
|
||||
// current file at this point?
|
||||
log("Skipping rebuild of the AST for {0}, inputs are the same.",
|
||||
FileName);
|
||||
return;
|
||||
}
|
||||
}
|
||||
|
||||
// Get the AST for diagnostics.
|
||||
llvm::Optional<std::unique_ptr<ParsedAST>> AST = IdleASTs.take(this);
|
||||
if (!AST) {
|
||||
llvm::Optional<ParsedAST> NewAST =
|
||||
buildAST(FileName, std::move(Invocation), Inputs, NewPreamble, PCHs);
|
||||
AST = NewAST ? llvm::make_unique<ParsedAST>(std::move(*NewAST)) : nullptr;
|
||||
}
|
||||
// Remove the old AST if it's still in cache.
|
||||
IdleASTs.take(this);
|
||||
|
||||
// Build the AST for diagnostics.
|
||||
llvm::Optional<ParsedAST> AST =
|
||||
buildAST(FileName, std::move(Invocation), Inputs, NewPreamble, PCHs);
|
||||
// We want to report the diagnostics even if this update was cancelled.
|
||||
// It seems more useful than making the clients wait indefinitely if they
|
||||
// spam us with updates.
|
||||
if (WantDiags != WantDiagnostics::No && AST)
|
||||
OnUpdated(AST->getDiagnostics());
|
||||
// Note *AST can be still be null if buildAST fails.
|
||||
if (WantDiags != WantDiagnostics::No && *AST) {
|
||||
OnUpdated((*AST)->getDiagnostics());
|
||||
DiagsWereReported = true;
|
||||
}
|
||||
// Stash the AST in the cache for further use.
|
||||
IdleASTs.put(this,
|
||||
AST ? llvm::make_unique<ParsedAST>(std::move(*AST)) : nullptr);
|
||||
IdleASTs.put(this, std::move(*AST));
|
||||
};
|
||||
|
||||
startTask("Update", Bind(Task, std::move(OnUpdated)), WantDiags);
|
||||
|
|
|
@ -390,5 +390,39 @@ TEST_F(TUSchedulerTests, NoopOnEmptyChanges) {
|
|||
ASSERT_FALSE(DoUpdate(getInputs(Source, OtherSourceContents)));
|
||||
}
|
||||
|
||||
TEST_F(TUSchedulerTests, NoChangeDiags) {
|
||||
TUScheduler S(
|
||||
/*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
|
||||
/*StorePreambleInMemory=*/true, PreambleParsedCallback(),
|
||||
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
|
||||
ASTRetentionPolicy());
|
||||
|
||||
auto FooCpp = testPath("foo.cpp");
|
||||
auto Contents = "int a; int b;";
|
||||
|
||||
S.update(FooCpp, getInputs(FooCpp, Contents), WantDiagnostics::No,
|
||||
[](std::vector<Diag>) { ADD_FAILURE() << "Should not be called."; });
|
||||
S.runWithAST("touchAST", FooCpp, [](llvm::Expected<InputsAndAST> IA) {
|
||||
// Make sure the AST was actually built.
|
||||
cantFail(std::move(IA));
|
||||
});
|
||||
ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
|
||||
|
||||
// Even though the inputs didn't change and AST can be reused, we need to
|
||||
// report the diagnostics, as they were not reported previously.
|
||||
std::atomic<bool> SeenDiags(false);
|
||||
S.update(FooCpp, getInputs(FooCpp, Contents), WantDiagnostics::Auto,
|
||||
[&](std::vector<Diag>) { SeenDiags = true; });
|
||||
ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
|
||||
ASSERT_TRUE(SeenDiags);
|
||||
|
||||
// Subsequent request does not get any diagnostics callback because the same
|
||||
// diags have previously been reported and the inputs didn't change.
|
||||
S.update(
|
||||
FooCpp, getInputs(FooCpp, Contents), WantDiagnostics::Auto,
|
||||
[&](std::vector<Diag>) { ADD_FAILURE() << "Should not be called."; });
|
||||
ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
|
||||
}
|
||||
|
||||
} // namespace clangd
|
||||
} // namespace clang
|
||||
|
|
Loading…
Reference in New Issue