Index: clangd/ClangdLSPServer.h =================================================================== --- clangd/ClangdLSPServer.h +++ clangd/ClangdLSPServer.h @@ -49,7 +49,7 @@ private: // Implement DiagnosticsConsumer. - void onDiagnosticsReady(PathRef File, std::vector Diagnostics) override; + void onDiagnosticsReady(PathRef File, DiagnosticsResult Diagnostics) override; // LSP methods. Notifications have signature void(const Params&). // Calls have signature void(const Params&, Callback). Index: clangd/ClangdLSPServer.cpp =================================================================== --- clangd/ClangdLSPServer.cpp +++ clangd/ClangdLSPServer.cpp @@ -751,11 +751,14 @@ } void ClangdLSPServer::onDiagnosticsReady(PathRef File, - std::vector Diagnostics) { + DiagnosticsResult Diagnostics) { + if (Diagnostics.isSkipped()) + return; // no action needed, previous diags can be reused. + URIForFile URI(File); std::vector LSPDiagnostics; DiagnosticToReplacementMap LocalFixIts; // Temporary storage - for (auto &Diag : Diagnostics) { + for (auto &Diag : Diagnostics.diags()) { toLSPDiags(Diag, URI, DiagOpts, [&](clangd::Diagnostic Diag, ArrayRef Fixes) { auto &FixItsForDiagnostic = LocalFixIts[Diag]; Index: clangd/ClangdServer.h =================================================================== --- clangd/ClangdServer.h +++ clangd/ClangdServer.h @@ -40,9 +40,8 @@ public: virtual ~DiagnosticsConsumer() = default; - /// Called by ClangdServer when \p Diagnostics for \p File are ready. virtual void onDiagnosticsReady(PathRef File, - std::vector Diagnostics) = 0; + DiagnosticsResult Diagnostics) = 0; }; /// Manages a collection of source files and derived data (ASTs, indexes), @@ -229,7 +228,7 @@ typedef uint64_t DocVersion; void consumeDiagnostics(PathRef File, DocVersion Version, - std::vector Diags); + DiagnosticsResult Diags); tooling::CompileCommand getCompileCommand(PathRef File); Index: clangd/ClangdServer.cpp =================================================================== --- clangd/ClangdServer.cpp +++ clangd/ClangdServer.cpp @@ -140,7 +140,7 @@ Path FileStr = File.str(); WorkScheduler.update(File, std::move(Inputs), WantDiags, - [this, FileStr, Version](std::vector Diags) { + [this, FileStr, Version](DiagnosticsResult Diags) { consumeDiagnostics(FileStr, Version, std::move(Diags)); }); } @@ -446,7 +446,7 @@ } void ClangdServer::consumeDiagnostics(PathRef File, DocVersion Version, - std::vector Diags) { + DiagnosticsResult Diags) { // We need to serialize access to resulting diagnostics to avoid calling // `onDiagnosticsReady` in the wrong order. std::lock_guard DiagsLock(DiagnosticsMutex); Index: clangd/TUScheduler.h =================================================================== --- clangd/TUScheduler.h +++ clangd/TUScheduler.h @@ -74,6 +74,23 @@ virtual void onMainAST(PathRef Path, ParsedAST &AST) {} }; +class DiagnosticsResult { +public: + DiagnosticsResult() = default; + explicit DiagnosticsResult(std::vector Diags) + : Diags(std::move(Diags)) {} + + /// Indicates computation of diagnostics was skipped, because inputs did not + /// change. Previous diagnostics should be reused. + bool isSkipped() { return !Diags.hasValue(); } + + /// Cannot be called if isSkipped is false. + std::vector &diags() { return *Diags; } + +private: + llvm::Optional> Diags; +}; + /// Handles running tasks for ClangdServer and managing the resources (e.g., /// preambles and ASTs) for opened files. /// TUScheduler is not thread-safe, only one thread should be providing updates @@ -102,7 +119,7 @@ /// \p File was not part of it before. /// FIXME(ibiryukov): remove the callback from this function. void update(PathRef File, ParseInputs Inputs, WantDiagnostics WD, - llvm::unique_function)> OnUpdated); + llvm::unique_function OnUpdated); /// Remove \p File from the list of tracked files and schedule removal of its /// resources. Index: clangd/TUScheduler.cpp =================================================================== --- clangd/TUScheduler.cpp +++ clangd/TUScheduler.cpp @@ -176,7 +176,7 @@ ~ASTWorker(); void update(ParseInputs Inputs, WantDiagnostics, - llvm::unique_function)> OnUpdated); + llvm::unique_function OnUpdated); void runWithAST(StringRef Name, unique_function)> Action); bool blockUntilIdle(Deadline Timeout) const; @@ -333,7 +333,7 @@ } void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags, - unique_function)> OnUpdated) { + unique_function OnUpdated) { auto Task = [=](decltype(OnUpdated) OnUpdated) mutable { // Will be used to check if we can avoid rebuilding the AST. bool InputsAreTheSame = @@ -358,6 +358,9 @@ // Make sure anyone waiting for the preamble gets notified it could not // be built. PreambleWasBuilt.notify(); + if (WantDiags != WantDiagnostics::No) + OnUpdated(DiagnosticsResult()); // FIXME(ibiryukov): report an error to + // the user. return; } @@ -397,6 +400,9 @@ // current file at this point? log("Skipping rebuild of the AST for {0}, inputs are the same.", FileName); + if (WantDiags != WantDiagnostics::No) + OnUpdated(DiagnosticsResult()); // Signals prev diagnostics should be + // reused. return; } } @@ -417,7 +423,7 @@ // spam us with updates. // Note *AST can still be null if buildAST fails. if (*AST) { - OnUpdated((*AST)->getDiagnostics()); + OnUpdated(DiagnosticsResult((*AST)->getDiagnostics())); trace::Span Span("Running main AST callback"); Callbacks.onMainAST(FileName, **AST); DiagsWereReported = true; @@ -697,7 +703,7 @@ void TUScheduler::update(PathRef File, ParseInputs Inputs, WantDiagnostics WantDiags, - unique_function)> OnUpdated) { + unique_function OnUpdated) { std::unique_ptr &FD = Files[File]; if (!FD) { // Create a new worker to process the AST-related tasks. Index: unittests/clangd/ClangdTests.cpp =================================================================== --- unittests/clangd/ClangdTests.cpp +++ unittests/clangd/ClangdTests.cpp @@ -55,9 +55,10 @@ class ErrorCheckingDiagConsumer : public DiagnosticsConsumer { public: - void onDiagnosticsReady(PathRef File, - std::vector Diagnostics) override { - bool HadError = diagsContainErrors(Diagnostics); + void onDiagnosticsReady(PathRef File, DiagnosticsResult Result) override { + if (Result.isSkipped()) + return; + bool HadError = diagsContainErrors(Result.diags()); std::lock_guard Lock(Mutex); HadErrorInLastDiags = HadError; } @@ -76,9 +77,8 @@ /// least one error. class MultipleErrorCheckingDiagConsumer : public DiagnosticsConsumer { public: - void onDiagnosticsReady(PathRef File, - std::vector Diagnostics) override { - bool HadError = diagsContainErrors(Diagnostics); + void onDiagnosticsReady(PathRef File, DiagnosticsResult Result) override { + bool HadError = diagsContainErrors(Result.diags()); std::lock_guard Lock(Mutex); LastDiagsHadError[File] = HadError; @@ -272,10 +272,10 @@ mutable int Got; } FS; struct DiagConsumer : public DiagnosticsConsumer { - void onDiagnosticsReady(PathRef File, - std::vector Diagnostics) override { + void onDiagnosticsReady(PathRef, DiagnosticsResult) override { Got = Context::current().getExisting(Secret); } + int Got; } DiagConsumer; MockCompilationDatabase CDB; @@ -592,14 +592,16 @@ public: TestDiagConsumer() : Stats(FilesCount, FileStat()) {} - void onDiagnosticsReady(PathRef File, - std::vector Diagnostics) override { + void onDiagnosticsReady(PathRef File, DiagnosticsResult Result) override { + if (Result.isSkipped()) + return; + StringRef FileIndexStr = sys::path::stem(File); ASSERT_TRUE(FileIndexStr.consume_front("Foo")); unsigned long FileIndex = std::stoul(FileIndexStr.str()); - bool HadError = diagsContainErrors(Diagnostics); + bool HadError = diagsContainErrors(Result.diags()); std::lock_guard Lock(Mutex); if (HadError) @@ -847,7 +849,10 @@ NoConcurrentAccessDiagConsumer(std::promise StartSecondReparse) : StartSecondReparse(std::move(StartSecondReparse)) {} - void onDiagnosticsReady(PathRef, std::vector) override { + void onDiagnosticsReady(PathRef, DiagnosticsResult) override { onAccess(); } + + private: + void onAccess() { ++Count; std::unique_lock Lock(Mutex, std::try_to_lock_t()); ASSERT_TRUE(Lock.owns_lock()) @@ -863,7 +868,6 @@ } } - private: std::mutex Mutex; bool FirstRequest = true; std::promise StartSecondReparse; Index: unittests/clangd/CodeCompleteTests.cpp =================================================================== --- unittests/clangd/CodeCompleteTests.cpp +++ unittests/clangd/CodeCompleteTests.cpp @@ -41,7 +41,7 @@ class IgnoreDiagnostics : public DiagnosticsConsumer { void onDiagnosticsReady(PathRef File, - std::vector Diagnostics) override {} + DiagnosticsResult Diagnostics) override {} }; // GMock helpers for matching completion items. Index: unittests/clangd/FindSymbolsTests.cpp =================================================================== --- unittests/clangd/FindSymbolsTests.cpp +++ unittests/clangd/FindSymbolsTests.cpp @@ -28,7 +28,7 @@ class IgnoreDiagnostics : public DiagnosticsConsumer { void onDiagnosticsReady(PathRef File, - std::vector Diagnostics) override {} + DiagnosticsResult Diagnostics) override {} }; // GMock helpers for matching SymbolInfos items. Index: unittests/clangd/TUSchedulerTests.cpp =================================================================== --- unittests/clangd/TUSchedulerTests.cpp +++ unittests/clangd/TUSchedulerTests.cpp @@ -28,7 +28,7 @@ using ::testing::Pointee; using ::testing::UnorderedElementsAre; -void ignoreUpdate(Optional>) {} +void ignoreUpdate(DiagnosticsResult) {} void ignoreError(Error Err) { handleAllErrors(std::move(Err), [](const ErrorInfoBase &) {}); } @@ -109,20 +109,20 @@ ASTRetentionPolicy()); auto Path = testPath("foo.cpp"); S.update(Path, getInputs(Path, ""), WantDiagnostics::Yes, - [&](std::vector) { Ready.wait(); }); + [&](DiagnosticsResult) { Ready.wait(); }); S.update(Path, getInputs(Path, "request diags"), WantDiagnostics::Yes, - [&](std::vector Diags) { ++CallbackCount; }); + [&](DiagnosticsResult) { ++CallbackCount; }); S.update(Path, getInputs(Path, "auto (clobbered)"), WantDiagnostics::Auto, - [&](std::vector Diags) { + [&](DiagnosticsResult) { ADD_FAILURE() << "auto should have been cancelled by auto"; }); S.update(Path, getInputs(Path, "request no diags"), WantDiagnostics::No, - [&](std::vector Diags) { + [&](DiagnosticsResult) { ADD_FAILURE() << "no diags should not be called back"; }); S.update(Path, getInputs(Path, "auto (produces)"), WantDiagnostics::Auto, - [&](std::vector Diags) { ++CallbackCount; }); + [&](DiagnosticsResult) { ++CallbackCount; }); Ready.notify(); } EXPECT_EQ(2, CallbackCount); @@ -138,15 +138,15 @@ // FIXME: we could probably use timeouts lower than 1 second here. auto Path = testPath("foo.cpp"); S.update(Path, getInputs(Path, "auto (debounced)"), WantDiagnostics::Auto, - [&](std::vector Diags) { + [&](DiagnosticsResult) { ADD_FAILURE() << "auto should have been debounced and canceled"; }); std::this_thread::sleep_for(std::chrono::milliseconds(200)); S.update(Path, getInputs(Path, "auto (timed out)"), WantDiagnostics::Auto, - [&](std::vector Diags) { ++CallbackCount; }); + [&](DiagnosticsResult) { ++CallbackCount; }); std::this_thread::sleep_for(std::chrono::seconds(2)); S.update(Path, getInputs(Path, "auto (shut down)"), WantDiagnostics::Auto, - [&](std::vector Diags) { ++CallbackCount; }); + [&](DiagnosticsResult) { ++CallbackCount; }); } EXPECT_EQ(2, CallbackCount); } @@ -173,7 +173,7 @@ // The stale read should see A, and the consistent read should see B. // (We recognize the preambles by their included files). S.update(Path, getInputs(Path, "#include "), WantDiagnostics::Yes, - [&](std::vector Diags) { + [&](DiagnosticsResult Diags) { // This callback runs in between the two preamble updates. // This blocks update B, preventing it from winning the race @@ -187,7 +187,7 @@ std::this_thread::sleep_for(std::chrono::milliseconds(100)); }); S.update(Path, getInputs(Path, "#include "), WantDiagnostics::Yes, - [&](std::vector Diags) {}); + [&](DiagnosticsResult Diags) {}); S.runWithPreamble("StaleRead", Path, TUScheduler::Stale, [&](Expected Pre) { @@ -253,37 +253,33 @@ { WithContextValue WithNonce(NonceKey, ++Nonce); - S.update(File, Inputs, WantDiagnostics::Auto, - [File, Nonce, &Mut, - &TotalUpdates](Optional> Diags) { - EXPECT_THAT(Context::current().get(NonceKey), - Pointee(Nonce)); - - std::lock_guard Lock(Mut); - ++TotalUpdates; - EXPECT_EQ(File, - *TUScheduler::getFileBeingProcessedInContext()); - }); + S.update( + File, Inputs, WantDiagnostics::Auto, + [File, Nonce, &Mut, &TotalUpdates](DiagnosticsResult) { + EXPECT_THAT(Context::current().get(NonceKey), Pointee(Nonce)); + + std::lock_guard Lock(Mut); + ++TotalUpdates; + EXPECT_EQ(File, *TUScheduler::getFileBeingProcessedInContext()); + }); } { WithContextValue WithNonce(NonceKey, ++Nonce); - S.runWithAST("CheckAST", File, - [File, Inputs, Nonce, &Mut, - &TotalASTReads](Expected AST) { - EXPECT_THAT(Context::current().get(NonceKey), - Pointee(Nonce)); - - ASSERT_TRUE((bool)AST); - EXPECT_EQ(AST->Inputs.FS, Inputs.FS); - EXPECT_EQ(AST->Inputs.Contents, Inputs.Contents); - - std::lock_guard Lock(Mut); - ++TotalASTReads; - EXPECT_EQ( - File, - *TUScheduler::getFileBeingProcessedInContext()); - }); + S.runWithAST( + "CheckAST", File, + [File, Inputs, Nonce, &Mut, + &TotalASTReads](Expected AST) { + EXPECT_THAT(Context::current().get(NonceKey), Pointee(Nonce)); + + ASSERT_TRUE((bool)AST); + EXPECT_EQ(AST->Inputs.FS, Inputs.FS); + EXPECT_EQ(AST->Inputs.Contents, Inputs.Contents); + + std::lock_guard Lock(Mut); + ++TotalASTReads; + EXPECT_EQ(File, *TUScheduler::getFileBeingProcessedInContext()); + }); } { @@ -337,16 +333,25 @@ // Build one file in advance. We will not access it later, so it will be the // one that the cache will evict. S.update(Foo, getInputs(Foo, SourceContents), WantDiagnostics::Yes, - [&BuiltASTCounter](std::vector Diags) { ++BuiltASTCounter; }); + [&BuiltASTCounter](DiagnosticsResult R) { + if (!R.isSkipped()) + ++BuiltASTCounter; + }); ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); ASSERT_EQ(BuiltASTCounter.load(), 1); // Build two more files. Since we can retain only 2 ASTs, these should be the // ones we see in the cache later. S.update(Bar, getInputs(Bar, SourceContents), WantDiagnostics::Yes, - [&BuiltASTCounter](std::vector Diags) { ++BuiltASTCounter; }); + [&BuiltASTCounter](DiagnosticsResult R) { + if (!R.isSkipped()) + ++BuiltASTCounter; + }); S.update(Baz, getInputs(Baz, SourceContents), WantDiagnostics::Yes, - [&BuiltASTCounter](std::vector Diags) { ++BuiltASTCounter; }); + [&BuiltASTCounter](DiagnosticsResult R) { + if (!R.isSkipped()) + ++BuiltASTCounter; + }); ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); ASSERT_EQ(BuiltASTCounter.load(), 3); @@ -355,7 +360,10 @@ // Access the old file again. S.update(Foo, getInputs(Foo, OtherSourceContents), WantDiagnostics::Yes, - [&BuiltASTCounter](std::vector Diags) { ++BuiltASTCounter; }); + [&BuiltASTCounter](DiagnosticsResult R) { + if (!R.isSkipped()) + ++BuiltASTCounter; + }); ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); ASSERT_EQ(BuiltASTCounter.load(), 4); @@ -383,31 +391,31 @@ )cpp"; auto WithEmptyPreamble = R"cpp(int main() {})cpp"; S.update(Foo, getInputs(Foo, WithPreamble), WantDiagnostics::Auto, - [](std::vector) {}); - S.runWithPreamble("getNonEmptyPreamble", Foo, TUScheduler::Stale, - [&](Expected Preamble) { - // We expect to get a non-empty preamble. - EXPECT_GT(cantFail(std::move(Preamble)) - .Preamble->Preamble.getBounds() - .Size, - 0u); - }); + [](DiagnosticsResult) {}); + S.runWithPreamble( + "getNonEmptyPreamble", Foo, TUScheduler::Stale, + [&](Expected Preamble) { + // We expect to get a non-empty preamble. + EXPECT_GT( + cantFail(std::move(Preamble)).Preamble->Preamble.getBounds().Size, + 0u); + }); // Wait for the preamble is being built. ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); // Update the file which results in an empty preamble. S.update(Foo, getInputs(Foo, WithEmptyPreamble), WantDiagnostics::Auto, - [](std::vector) {}); + [](DiagnosticsResult) {}); // Wait for the preamble is being built. ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); - S.runWithPreamble("getEmptyPreamble", Foo, TUScheduler::Stale, - [&](Expected Preamble) { - // We expect to get an empty preamble. - EXPECT_EQ(cantFail(std::move(Preamble)) - .Preamble->Preamble.getBounds() - .Size, - 0u); - }); + S.runWithPreamble( + "getEmptyPreamble", Foo, TUScheduler::Stale, + [&](Expected Preamble) { + // We expect to get an empty preamble. + EXPECT_EQ( + cantFail(std::move(Preamble)).Preamble->Preamble.getBounds().Size, + 0u); + }); } TEST_F(TUSchedulerTests, RunWaitsForPreamble) { @@ -429,7 +437,7 @@ std::mutex PreamblesMut; std::vector Preambles(ReadsToSchedule, nullptr); S.update(Foo, getInputs(Foo, NonEmptyPreamble), WantDiagnostics::Auto, - [](std::vector) {}); + [](DiagnosticsResult) {}); for (int I = 0; I < ReadsToSchedule; ++I) { S.runWithPreamble( "test", Foo, TUScheduler::Stale, @@ -466,13 +474,18 @@ // Return value indicates if the updated callback was received. auto DoUpdate = [&](ParseInputs Inputs) -> bool { std::atomic Updated(false); - Updated = false; + std::atomic Skipped(false); S.update(Source, std::move(Inputs), WantDiagnostics::Yes, - [&Updated](std::vector) { Updated = true; }); + [&](DiagnosticsResult R) { + Updated = true; + Skipped = R.isSkipped(); + }); bool UpdateFinished = S.blockUntilIdle(timeoutSeconds(10)); if (!UpdateFinished) ADD_FAILURE() << "Updated has not finished in one second. Threading bug?"; - return Updated; + if (!Updated) + ADD_FAILURE() << "The callback was never called"; + return !Skipped; }; // Test that subsequent updates with the same inputs do not cause rebuilds. @@ -509,7 +522,7 @@ auto Contents = "int a; int b;"; S.update(FooCpp, getInputs(FooCpp, Contents), WantDiagnostics::No, - [](std::vector) { ADD_FAILURE() << "Should not be called."; }); + [](DiagnosticsResult) { ADD_FAILURE() << "Should not be called."; }); S.runWithAST("touchAST", FooCpp, [](Expected IA) { // Make sure the AST was actually built. cantFail(std::move(IA)); @@ -518,18 +531,25 @@ // 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 SeenDiags(false); + std::atomic SeenResult(false); S.update(FooCpp, getInputs(FooCpp, Contents), WantDiagnostics::Auto, - [&](std::vector) { SeenDiags = true; }); + [&](DiagnosticsResult R) { + EXPECT_FALSE(R.isSkipped()) << "New diagnostics were not produced"; + SeenResult = true; + }); ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); - ASSERT_TRUE(SeenDiags); + ASSERT_TRUE(SeenResult); // 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) { ADD_FAILURE() << "Should not be called."; }); + SeenResult = false; + S.update(FooCpp, getInputs(FooCpp, Contents), WantDiagnostics::Auto, + [&](DiagnosticsResult R) { + EXPECT_TRUE(R.isSkipped()) << "Diagnostics should not be produced"; + SeenResult = true; + }); ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); + ASSERT_TRUE(SeenResult); } TEST_F(TUSchedulerTests, Run) { Index: unittests/clangd/XRefsTests.cpp =================================================================== --- unittests/clangd/XRefsTests.cpp +++ unittests/clangd/XRefsTests.cpp @@ -34,7 +34,7 @@ class IgnoreDiagnostics : public DiagnosticsConsumer { void onDiagnosticsReady(PathRef File, - std::vector Diagnostics) override {} + DiagnosticsResult Diagnostics) override {} }; // Extracts ranges from an annotated example, and constructs a matcher for a