Index: clang-tools-extra/trunk/clangd/TUScheduler.cpp =================================================================== --- clang-tools-extra/trunk/clangd/TUScheduler.cpp +++ clang-tools-extra/trunk/clangd/TUScheduler.cpp @@ -228,6 +228,9 @@ 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 @@ 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 @@ 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> AST = IdleASTs.take(this); + if (!AST) { + llvm::Optional NewAST = + buildAST(FileName, std::move(Invocation), Inputs, NewPreamble, PCHs); + AST = NewAST ? llvm::make_unique(std::move(*NewAST)) : nullptr; } - // Remove the old AST if it's still in cache. - IdleASTs.take(this); - // Build the AST for diagnostics. - llvm::Optional 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(std::move(*AST)) : nullptr); + IdleASTs.put(this, std::move(*AST)); }; startTask("Update", Bind(Task, std::move(OnUpdated)), WantDiags); Index: clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp =================================================================== --- clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp +++ clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp @@ -390,5 +390,39 @@ 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) { ADD_FAILURE() << "Should not be called."; }); + S.runWithAST("touchAST", FooCpp, [](llvm::Expected 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 SeenDiags(false); + S.update(FooCpp, getInputs(FooCpp, Contents), WantDiagnostics::Auto, + [&](std::vector) { 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) { ADD_FAILURE() << "Should not be called."; }); + ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1))); +} + } // namespace clangd } // namespace clang