[clangd] Don't build diagnostics when preparing AST for a normal action.

[sammccall]

Summary

This saves something like 20% latency on the AST build with a typical load of
clang-tidy checks.
The tradeoff is the AST is never cached for diagnostics, but this is less
latency sensitive and rarely happens anyway.

[kadircet]

clang-tools-extra/clangd/ParsedAST.cpp

369

while here, maybe introduce another span for include fixer initialization ?

as getFormatStyle can do IO

374

umm, actually this is wrong in presence of stale preambles ... we should perform patching before initializing include fixer and make use of patched includes rather than stale ones.

no action needing, just taking a note for myself.

434

maybe

if (ProduceDiagnostics) {
 assert(CTFinder.hasValue());
...
}

to keep it similar to other usages.

clang-tools-extra/clangd/TUScheduler.h

113

why don't we just drop this ? I believe this patch is doing 2 things:

• Don't build diagnostics for read operations
• Don't reuse cached asts for diagnostics

and I think getting rid of ReuseAST bit would belong to second one. (unless we decide to emit status while building asts for read operations)

clang-tools-extra/clangd/unittests/ParsedASTTests.cpp

470

nit: initialize to false

clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp

540

WantDiags::Auto should be enough here, as we are blocking afterwards. I've heard someone is tryin to get rid of those :P

546

again WantDiags::Auto should do for both

796

is it worth asserting here (or in a new test) that AST.diags() is empty, whereas it is non-empty below ? (ofc, after introducing some diags to the code)

[sammccall]

rarely happens anyway

citation needed.

While this is true today, it seems fairly likely that async preambles is going to flip the dominant AST reuse from diagnostics -> read to read -> diagnostics.

This change would make the builds 20% faster for the cache misses on AST read (~30%), at the cost of rebuilding when diagnostics would hit (~0%).
If those probabilities flip, this becomes a really bad trade.

Let's wait a couple of weeks until we have data from production.

clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp

796

I don't think we want to *guarantee* when they're present/absent with runWithAST, this is an implementation choice. Added a comment to make this contract explicit.

[sammccall]

address comments

clang-tools-extra/clangd/ParsedAST.h

 /// Stores and provides access to parsed AST.
 class ParsedAST {
 public:
   /// Attempts to run Clang and store the parsed AST.
   /// If \p Preamble is non-null it is reused during parsing.
   /// This function does not check if preamble is valid to reuse.
   static llvm::Optional<ParsedAST>
   build(llvm::StringRef Filename, const ParseInputs &Inputs,
-        std::unique_ptr<clang::CompilerInvocation> CI,
+        std::unique_ptr<clang::CompilerInvocation> CI, bool ProduceDiagnostics,
         llvm::ArrayRef<Diag> CompilerInvocationDiags,
         std::shared_ptr<const PreambleData> Preamble);
 
   ParsedAST(ParsedAST &&Other);
   ParsedAST &operator=(ParsedAST &&Other);
 
   ~ParsedAST();
 

clang-tools-extra/clangd/ParsedAST.cpp

 #include "Preamble.h"
 #include "SourceCode.h"
 #include "index/CanonicalIncludes.h"
 #include "index/Index.h"
 #include "support/Logger.h"
 #include "support/Trace.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
+#include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TokenKinds.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Frontend/Utils.h"

 
 void dumpAST(ParsedAST &AST, llvm::raw_ostream &OS) {
   AST.getASTContext().getTranslationUnitDecl()->dump(OS, true);
 }
 
 llvm::Optional<ParsedAST>
 ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
                  std::unique_ptr<clang::CompilerInvocation> CI,
+                 bool ProduceDiagnostics,
                  llvm::ArrayRef<Diag> CompilerInvocationDiags,
                  std::shared_ptr<const PreambleData> Preamble) {
   trace::Span Tracer("BuildAST");
   SPAN_ATTACH(Tracer, "File", Filename);
 
   auto VFS = Inputs.FS;
   if (Preamble && Preamble->StatCache)
     VFS = Preamble->StatCache->getConsumingFS(std::move(VFS));
   if (VFS->setCurrentWorkingDirectory(Inputs.CompileCommand.Directory)) {
     log("Couldn't set working directory when building the preamble.");
     // We proceed anyway, our lit-tests rely on results for non-existing working
     // dirs.
   }
 
   assert(CI);
   // Command-line parsing sets DisableFree to true by default, but we don't want
   // to leak memory in clangd.
   CI->getFrontendOpts().DisableFree = false;
   const PrecompiledPreamble *PreamblePCH =
       Preamble ? &Preamble->Preamble : nullptr;
 
   // This is on-by-default in windows to allow parsing SDK headers, but it
   // breaks many features. Disable it for the main-file (not preamble).
   CI->getLangOpts()->DelayedTemplateParsing = false;
+  if (!ProduceDiagnostics)
+    CI->getDiagnosticOpts().IgnoreWarnings = true;
 
   StoreDiags ASTDiags;
+  IgnoringDiagConsumer IgnoreDiags;
 
   llvm::Optional<PreamblePatch> Patch;
   if (Preamble) {
     Patch = PreamblePatch::create(Filename, Inputs, *Preamble);
     Patch->apply(*CI);
   }
   auto Clang = prepareCompilerInstance(
       std::move(CI), PreamblePCH,
       llvm::MemoryBuffer::getMemBufferCopy(Inputs.Contents, Filename), VFS,
-      ASTDiags);
+      ProduceDiagnostics ? llvm::cast<DiagnosticConsumer>(ASTDiags)
+                         : llvm::cast<DiagnosticConsumer>(IgnoreDiags));
   if (!Clang)
     return None;
 
   auto Action = std::make_unique<ClangdFrontendAction>();
   const FrontendInputFile &MainInput = Clang->getFrontendOpts().Inputs[0];
   if (!Action->BeginSourceFile(*Clang, MainInput)) {
     log("BeginSourceFile() failed when building AST for {0}",
         MainInput.getFile());
     return None;
   }
 
   // Set up ClangTidy. Must happen after BeginSourceFile() so ASTContext exists.
   // Clang-tidy has some limitations to ensure reasonable performance:
   //  - checks don't see all preprocessor events in the preamble
   //  - matchers run only over the main-file top-level decls (and can't see
   //    ancestors outside this scope).
   // In practice almost all checks work well without modifications.
   std::vector<std::unique_ptr<tidy::ClangTidyCheck>> CTChecks;
-  ast_matchers::MatchFinder CTFinder;
+  llvm::Optional<ast_matchers::MatchFinder> CTFinder;
   llvm::Optional<tidy::ClangTidyContext> CTContext;
-  {
+  if (ProduceDiagnostics) {
     trace::Span Tracer("ClangTidyInit");
     dlog("ClangTidy configuration for file {0}: {1}", Filename,
          tidy::configurationAsText(Inputs.Opts.ClangTidyOpts));
     tidy::ClangTidyCheckFactories CTFactories;
     for (const auto &E : tidy::ClangTidyModuleRegistry::entries())
       E.instantiate()->addCheckFactories(CTFactories);
     CTContext.emplace(std::make_unique<tidy::DefaultOptionsProvider>(
         tidy::ClangTidyGlobalOptions(), Inputs.Opts.ClangTidyOpts));
ASTDiags.setLevelAdjuster([&CTContext](DiagnosticsEngine::Level DiagLevel,
               CTContext->treatAsError(CheckName)) {
             return DiagnosticsEngine::Error;
           }
         }
       }
       return DiagLevel;
     });
     Preprocessor *PP = &Clang->getPreprocessor();
+    CTFinder.emplace();
     for (const auto &Check : CTChecks) {
       if (!Check->isLanguageVersionSupported(CTContext->getLangOpts()))
         continue;
       // FIXME: the PP callbacks skip the entire preamble.
       // Checks that want to see #includes in the main file do not see them.
       Check->registerPPCallbacks(Clang->getSourceManager(), PP, PP);
-      Check->registerMatchers(&CTFinder);
+      Check->registerMatchers(CTFinder.getPointer());
     }
   }
 
   // Add IncludeFixer which can recover diagnostics caused by missing includes
   // (e.g. incomplete type) and attach include insertion fixes to diagnostics.
   llvm::Optional<IncludeFixer> FixIncludes;
   auto BuildDir = VFS->getCurrentWorkingDirectory();
-  if (Inputs.Opts.SuggestMissingIncludes && Inputs.Index &&
-      !BuildDir.getError()) {
+  if (ProduceDiagnostics && Inputs.Opts.SuggestMissingIncludes &&
+      Inputs.Index && !BuildDir.getError()) {
     auto Style = getFormatStyleForFile(Filename, Inputs.Contents, VFS.get());
     auto Inserter = std::make_shared<IncludeInserter>(
         Filename, Inputs.Contents, Style, BuildDir.get(),
         &Clang->getPreprocessor().getHeaderSearchInfo());
     if (Preamble) {
       for (const auto &Inc : Preamble->Includes.MainFileIncludes)
         Inserter->addExisting(Inc);
     }
     FixIncludes.emplace(Filename, Inserter, *Inputs.Index,
                         /*IndexRequestLimit=*/5);
     ASTDiags.contributeFixes([&FixIncludes](DiagnosticsEngine::Level DiagLevl,
                                             const clang::Diagnostic &Info) {
       return FixIncludes->fix(DiagLevl, Info);
     });
ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
 
   // We have to consume the tokens before running clang-tidy to avoid collecting
   // tokens from running the preprocessor inside the checks (only
   // modernize-use-trailing-return-type does that today).
   syntax::TokenBuffer Tokens = std::move(CollectTokens).consume();
   std::vector<Decl *> ParsedDecls = Action->takeTopLevelDecls();
   // AST traversals should exclude the preamble, to avoid performance cliffs.
   Clang->getASTContext().setTraversalScope(ParsedDecls);
-  {
+  if (CTFinder.hasValue()) {
     // Run the AST-dependent part of the clang-tidy checks.
     // (The preprocessor part ran already, via PPCallbacks).
     trace::Span Tracer("ClangTidyMatch");
-    CTFinder.matchAST(Clang->getASTContext());
+    CTFinder->matchAST(Clang->getASTContext());
   }
 
-  // UnitDiagsConsumer is local, we can not store it in CompilerInstance that
-  // has a longer lifetime.
+  // Our diagnostic consumer is local, CompilerInstance must outlive it.
   Clang->getDiagnostics().setClient(new IgnoreDiagnostics);
   // CompilerInstance won't run this callback, do it directly.
   ASTDiags.EndSourceFile();
   // XXX: This is messy: clang-tidy checks flush some diagnostics at EOF.
   // However Action->EndSourceFile() would destroy the ASTContext!
   // So just inform the preprocessor of EOF, while keeping everything alive.
   Clang->getPreprocessor().EndSourceFile();
 
-  std::vector<Diag> Diags = CompilerInvocationDiags;
+  std::vector<Diag> Diags;
+  if (ProduceDiagnostics) {
+    Diags = CompilerInvocationDiags;
     // Add diagnostics from the preamble, if any.
     if (Preamble)
       Diags.insert(Diags.end(), Preamble->Diags.begin(), Preamble->Diags.end());
     // Finally, add diagnostics coming from the AST.
     {
       std::vector<Diag> D = ASTDiags.take(CTContext.getPointer());
       Diags.insert(Diags.end(), D.begin(), D.end());
     }
+  }
   return ParsedAST(Inputs.Version, std::move(Preamble), std::move(Clang),
                    std::move(Action), std::move(Tokens), std::move(Macros),
                    std::move(ParsedDecls), std::move(Diags),
                    std::move(Includes), std::move(CanonIncludes));
 }
 
 ParsedAST::ParsedAST(ParsedAST &&Other) = default;
 

clang-tools-extra/clangd/TUScheduler.h

 };
 
 // Internal status of the TU in TUScheduler.
 struct TUStatus {
   struct BuildDetails {
     /// Indicates whether clang failed to build the TU.
     bool BuildFailed = false;
     /// Indicates whether we reused the prebuilt AST.
+    /// FIXME: always false, diagnostics are always built from fresh ASTs.
     bool ReuseAST = false;
   };
   /// Serialize this to an LSP file status item.
   FileStatus render(PathRef File) const;
 
   PreambleAction PreambleActivity = PreambleAction::Idle;
   ASTAction ASTActivity;
   /// Stores status of the last build for the translation unit.

clang-tools-extra/clangd/TUScheduler.cpp

void ASTWorker::runWithAST(
   static constexpr trace::Metric ASTAccessForRead(
       "ast_access_read", trace::Metric::Counter, "result");
   auto Task = [=, Action = std::move(Action)]() mutable {
     if (auto Reason = isCancelled())
       return Action(llvm::make_error<CancelledError>(Reason));
     llvm::Optional<std::unique_ptr<ParsedAST>> AST =
         IdleASTs.take(this, &ASTAccessForRead);
     if (!AST) {
-      StoreDiags CompilerInvocationDiagConsumer;
+      IgnoreDiagnostics IgnoreDiags;
       std::unique_ptr<CompilerInvocation> Invocation =
-          buildCompilerInvocation(FileInputs, CompilerInvocationDiagConsumer);
+          buildCompilerInvocation(FileInputs, IgnoreDiags);
       // Try rebuilding the AST.
       vlog("ASTWorker rebuilding evicted AST to run {0}: {1} version {2}", Name,
            FileName, FileInputs.Version);
       // FIXME: We might need to build a patched ast once preamble thread starts
       // running async. Currently getPossiblyStalePreamble below will always
       // return a compatible preamble as ASTWorker::update blocks.
       llvm::Optional<ParsedAST> NewAST;
       if (Invocation) {
         NewAST = ParsedAST::build(FileName, FileInputs, std::move(Invocation),
-                                  CompilerInvocationDiagConsumer.take(),
+                                  /*ProduceDiagnostics=*/false, {},
                                   getPossiblyStalePreamble());
         ++ASTBuildCount;
       }
       AST = NewAST ? std::make_unique<ParsedAST>(std::move(*NewAST)) : nullptr;
     }
     // Make sure we put the AST back into the LRU cache.
     auto _ = llvm::make_scope_exit(
         [&AST, this]() { IdleASTs.put(this, std::move(*AST)); });
void ASTWorker::updatePreamble(std::unique_ptr<CompilerInvocation> CI,
   }
   ReceivedPreamble.notify();
   RequestsCV.notify_all();
 }
 
 void ASTWorker::generateDiagnostics(
     std::unique_ptr<CompilerInvocation> Invocation, ParseInputs Inputs,
     std::vector<Diag> CIDiags) {
-  // Tracks ast cache accesses for publishing diags.
-  static constexpr trace::Metric ASTAccessForDiag(
-      "ast_access_diag", trace::Metric::Counter, "result");
   assert(Invocation);
   // No need to rebuild the AST if we won't send the diagnostics.
   {
     std::lock_guard<std::mutex> Lock(PublishMu);
     if (!CanPublishResults)
       return;
   }
   // Used to check whether we can update AST cache.
if (InputsAreLatest && RanASTCallback)
     return;
 
   // Get the AST for diagnostics, either build it or use the cached one.
   std::string TaskName = llvm::formatv("Build AST ({0})", Inputs.Version);
   Status.update([&](TUStatus &Status) {
     Status.ASTActivity.K = ASTAction::Building;
     Status.ASTActivity.Name = std::move(TaskName);
   });
-  // We might be able to reuse the last we've built for a read request.
-  // FIXME: It might be better to not reuse this AST. That way queued AST builds
-  // won't be required for diags.
-  llvm::Optional<std::unique_ptr<ParsedAST>> AST =
-      IdleASTs.take(this, &ASTAccessForDiag);
-  if (!AST || !InputsAreLatest) {
-    auto RebuildStartTime = DebouncePolicy::clock::now();
-    llvm::Optional<ParsedAST> NewAST = ParsedAST::build(
-        FileName, Inputs, std::move(Invocation), CIDiags, LatestPreamble);
-    auto RebuildDuration = DebouncePolicy::clock::now() - RebuildStartTime;
-    ++ASTBuildCount;
-    // Try to record the AST-build time, to inform future update debouncing.
-    // This is best-effort only: if the lock is held, don't bother.
-    std::unique_lock<std::mutex> Lock(Mutex, std::try_to_lock);
-    if (Lock.owns_lock()) {
-      // Do not let RebuildTimes grow beyond its small-size (i.e.
-      // capacity).
-      if (RebuildTimes.size() == RebuildTimes.capacity())
-        RebuildTimes.erase(RebuildTimes.begin());
-      RebuildTimes.push_back(RebuildDuration);
-      Lock.unlock();
-    }
-    Status.update([&](TUStatus &Status) {
-      Status.Details.ReuseAST = false;
-      Status.Details.BuildFailed = !NewAST.hasValue();
-    });
-    AST = NewAST ? std::make_unique<ParsedAST>(std::move(*NewAST)) : nullptr;
-  } else {
-    log("Skipping rebuild of the AST for {0}, inputs are the same.", FileName);
-    Status.update([](TUStatus &Status) {
-      Status.Details.ReuseAST = true;
-      Status.Details.BuildFailed = false;
-    });
-  }
+  llvm::Optional<ParsedAST> AST =
+      ParsedAST::build(FileName, Inputs, std::move(Invocation),
+                       /*ProduceDiagnostics=*/true, CIDiags, LatestPreamble);
+    Status.Details.BuildFailed = !AST.hasValue();
 
   // Publish diagnostics.
   auto RunPublish = [&](llvm::function_ref<void()> Publish) {
     // Ensure we only publish results from the worker if the file was not
     // removed, making sure there are not race conditions.
     std::lock_guard<std::mutex> Lock(PublishMu);
     if (CanPublishResults)
       Publish();
   };
-  if (*AST) {
+  if (AST) {
     trace::Span Span("Running main AST callback");
-    Callbacks.onMainAST(FileName, **AST, RunPublish);
+    Callbacks.onMainAST(FileName, *AST, RunPublish);
   } else {
     // Failed to build the AST, at least report diagnostics from the
     // command line if there were any.
     // FIXME: we might have got more errors while trying to build the
     // AST, surface them too.
     Callbacks.onFailedAST(FileName, Inputs.Version, CIDiags, RunPublish);
   }
 
   // AST might've been built for an older version of the source, as ASTWorker
   // queue raced ahead while we were waiting on the preamble. In that case the
   // queue can't reuse the AST.
   if (InputsAreLatest) {
-    RanASTCallback = *AST != nullptr;
-    IdleASTs.put(this, std::move(*AST));
+    RanASTCallback = AST.hasValue();
+    IdleASTs.take(this); // Put requires the entry doesn't exist.
+    IdleASTs.put(this,
+                 AST ? std::make_unique<ParsedAST>(std::move(*AST)) : nullptr);
   }
 }
 
 std::shared_ptr<const PreambleData>
 ASTWorker::getPossiblyStalePreamble() const {
   std::lock_guard<std::mutex> Lock(Mutex);
   return LatestPreamble;
 }

clang-tools-extra/clangd/test/metrics.test

 # RUN: env CLANGD_METRICS=%t clangd -lit-test < %s && FileCheck %s < %t
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
 ---
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","version":1,"text":"void main() {}"}}}
 # Don't verify value, timestamp, or order.
 # CHECK-DAG: d,lsp_latency,textDocument/didOpen,
-# CHECK-DAG: c,ast_access_diag,miss,
+# CHECK-DAG: d,span_latency,BuildAST,
 ---
 {"jsonrpc":"2.0","id":5,"method":"shutdown"}
 ---
 {"jsonrpc":"2.0","method":"exit"}

clang-tools-extra/clangd/unittests/ParsedASTTests.cpp

 #include "Diagnostics.h"
 #include "Headers.h"
 #include "ParsedAST.h"
 #include "Preamble.h"
 #include "SourceCode.h"
 #include "TestFS.h"
 #include "TestTU.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TokenKinds.h"
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Token.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "gmock/gmock-matchers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
 using ::testing::AllOf;
 using ::testing::ElementsAre;
 using ::testing::ElementsAreArray;
+using ::testing::IsEmpty;
 
 MATCHER_P(DeclNamed, Name, "") {
   if (NamedDecl *ND = dyn_cast<NamedDecl>(arg))
     if (ND->getName() == Name)
       return true;
   if (auto *Stream = result_listener->stream()) {
     llvm::raw_os_ostream OS(*Stream);
     arg->dump(OS);
   }
   return false;
 }
 
+MATCHER_P(Diag, Message, "") { return arg.Message == Message; }
+
 // Matches if the Decl has template args equal to ArgName. If the decl is a
 // NamedDecl and ArgName is an empty string it also matches.
 MATCHER_P(WithTemplateArgs, ArgName, "") {
   if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(arg)) {
     if (const auto *Args = FD->getTemplateSpecializationArgs()) {
       std::string SpecializationArgs;
       // Without the PrintingPolicy "bool" will be printed as "_Bool".
       LangOptions LO;
TEST(ParsedASTTest, ReplayPreambleForTidyCheckers) {
   StoreDiags Diags;
   auto Inputs = TU.inputs();
   auto CI = buildCompilerInvocation(Inputs, Diags);
   auto EmptyPreamble =
       buildPreamble(testPath(TU.Filename), *CI, Inputs, true, nullptr);
   ASSERT_TRUE(EmptyPreamble);
   TU.Code = "#include <a.h>";
   Includes.clear();
-  auto PatchedAST = ParsedAST::build(testPath(TU.Filename), TU.inputs(),
-                                     std::move(CI), {}, EmptyPreamble);
+  auto PatchedAST =
+      ParsedAST::build(testPath(TU.Filename), TU.inputs(), std::move(CI),
+                       /*ProduceDiagnostics=*/true, {}, EmptyPreamble);
   ASSERT_TRUE(PatchedAST);
   // Make sure includes were seen only once.
   EXPECT_THAT(Includes,
               ElementsAre(WithFileName(testPath("__preamble_patch__.h")),
                           WithFileName("a.h")));
 }
 
+TEST(ParsedASTTest, NoDiagnostics) {
+  static bool TidyCheckIsError;
+  // A simple clang-tidy check, which we can verify:
+  //  - is used when ProduceDiagnostics is on
+  //  - does no work (isn't instantiated) when ProduceDiagnostics is off.
+  struct NoVars : public tidy::ClangTidyCheck {
+    NoVars(StringRef N, tidy::ClangTidyContext *C) : ClangTidyCheck(N, C) {
+      EXPECT_FALSE(TidyCheckIsError) << "Shouldn't even be instantiated!";
+    }
+    void registerMatchers(ast_matchers::MatchFinder *Finder) override {
+      Finder->addMatcher(ast_matchers::varDecl().bind("var"), this);
+    }
+    void check(const ast_matchers::MatchFinder::MatchResult &Result) override {
+      diag(Result.Nodes.getNodeAs<Decl>("var")->getLocation(), "tidy");
+    }
+  };
+  struct NoVarsModule : public tidy::ClangTidyModule {
+    void addCheckFactories(tidy::ClangTidyCheckFactories &CF) override {
+      CF.registerCheck<NoVars>("no-vars");
+    }
+  };
+  static tidy::ClangTidyModuleRegistry::Add<NoVarsModule> X("no-vars-mod", "");
+
+  StoreDiags StoreDriverDiags;
+  TestTU TU;
+  TU.ExtraArgs.push_back("-flag");
+  TU.AdditionalFiles["header.h"] = "#error header";
+  TU.Code = R"cpp(
+    #include "header.h"
+    #error preamble
+    int foo;
+    #error body
+    )cpp";
+  TU.ClangTidyChecks = "no-vars";
+
+  auto CI = *buildCompilerInvocation(TU.inputs(), StoreDriverDiags);
+  auto Preamble = TU.preamble();
+  auto Inputs = TU.inputs();
+  auto DriverDiags = StoreDriverDiags.take();
+
+  // Build with diagnostics, to sanity-check the inputs.
+  TidyCheckIsError = false;
+  auto AST = ParsedAST::build(
+      testPath(TU.Filename), Inputs, std::make_unique<CompilerInvocation>(CI),
+      /*ProduceDiagnostics=*/true, DriverDiags, Preamble);
+  EXPECT_THAT(AST->getDiagnostics(),
+              ElementsAre(Diag("unknown argument: '-flag'"),
+                          Diag("in included file: header"), Diag("preamble"),
+                          Diag("body"), Diag("tidy")));
+  EXPECT_EQ(Decl::Var, findDecl(*AST, "foo").getKind());
+
+  // Build with no diagnostics.
+  TidyCheckIsError = true; // Verify we don't do *any* clang-tidy work.
+  AST = ParsedAST::build(testPath(TU.Filename), Inputs,
+                         std::make_unique<CompilerInvocation>(CI),
+                         /*ProduceDiagnostics=*/false, DriverDiags, Preamble);
+  EXPECT_THAT(AST->getDiagnostics(), IsEmpty()) << "ProduceDiagnostics = false";
+  EXPECT_EQ(Decl::Var, findDecl(*AST, "foo").getKind()) << "AST still good";
+}
+
 TEST(ParsedASTTest, PatchesAdditionalIncludes) {
   llvm::StringLiteral ModifiedContents = R"cpp(
     #include "baz.h"
     #include "foo.h"
     #include "sub/aux.h"
     void bar() {
       foo();
       baz();
TEST(ParsedASTTest, PatchesAdditionalIncludes) {
   // Build preamble with no includes.
   TU.Code = "";
   StoreDiags Diags;
   auto Inputs = TU.inputs();
   auto CI = buildCompilerInvocation(Inputs, Diags);
   auto EmptyPreamble =
       buildPreamble(testPath("foo.cpp"), *CI, Inputs, true, nullptr);
   ASSERT_TRUE(EmptyPreamble);
-  EXPECT_THAT(EmptyPreamble->Includes.MainFileIncludes, testing::IsEmpty());
+  EXPECT_THAT(EmptyPreamble->Includes.MainFileIncludes, IsEmpty());
 
   // Now build an AST using empty preamble and ensure patched includes worked.
   TU.Code = ModifiedContents.str();
   Inputs = TU.inputs();
-  auto PatchedAST = ParsedAST::build(testPath("foo.cpp"), Inputs, std::move(CI),
-                                     {}, EmptyPreamble);
+  auto PatchedAST =
+      ParsedAST::build(testPath("foo.cpp"), Inputs, std::move(CI),
+                       /*ProduceDiagnostics=*/false, {}, EmptyPreamble);
   ASSERT_TRUE(PatchedAST);
   ASSERT_TRUE(PatchedAST->getDiagnostics().empty());
 
   // Ensure source location information is correct, including resolved paths.
   EXPECT_THAT(PatchedAST->getIncludeStructure().MainFileIncludes,
               testing::Pointwise(
                   EqInc(), ExpectedAST.getIncludeStructure().MainFileIncludes));
   auto StringMapToVector = [](const llvm::StringMap<unsigned> SM) {
TEST(ParsedASTTest, PatchesDeletedIncludes) {
   ASSERT_TRUE(BaselinePreamble);
   EXPECT_THAT(BaselinePreamble->Includes.MainFileIncludes,
               ElementsAre(testing::Field(&Inclusion::Written, "<foo.h>")));
 
   // Now build an AST using additional includes and check that locations are
   // correctly parsed.
   TU.Code = "";
   Inputs = TU.inputs();
-  auto PatchedAST = ParsedAST::build(testPath("foo.cpp"), Inputs, std::move(CI),
-                                     {}, BaselinePreamble);
+  auto PatchedAST =
+      ParsedAST::build(testPath("foo.cpp"), Inputs, std::move(CI),
+                       /*ProduceDiagnostics=*/false, {}, BaselinePreamble);
   ASSERT_TRUE(PatchedAST);
 
   // Ensure source location information is correct.
   EXPECT_THAT(PatchedAST->getIncludeStructure().MainFileIncludes,
               testing::Pointwise(
                   EqInc(), ExpectedAST.getIncludeStructure().MainFileIncludes));
   auto StringMapToVector = [](const llvm::StringMap<unsigned> SM) {
     std::vector<std::pair<std::string, unsigned>> Res;

clang-tools-extra/clangd/unittests/PreambleTests.cpp

llvm::Optional<ParsedAST> createPatchedAST(llvm::StringRef Baseline,
 
   IgnoreDiagnostics Diags;
   auto TU = TestTU::withCode(Modified);
   auto CI = buildCompilerInvocation(TU.inputs(), Diags);
   if (!CI) {
     ADD_FAILURE() << "Failed to build compiler invocation";
     return llvm::None;
   }
-  return ParsedAST::build(testPath(TU.Filename), TU.inputs(), std::move(CI), {},
-                          BaselinePreamble);
+  return ParsedAST::build(testPath(TU.Filename), TU.inputs(), std::move(CI),
+                          /*ProduceDiagnostics=*/false, {}, BaselinePreamble);
 }
 
 std::string getPreamblePatch(llvm::StringRef Baseline,
                              llvm::StringRef Modified) {
   auto BaselinePreamble = TestTU::withCode(Baseline).preamble();
   if (!BaselinePreamble) {
     ADD_FAILURE() << "Failed to build baseline preamble";
     return "";

clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp

TEST_F(TUSchedulerTests, ManyUpdates) {
   // We should receive diags for last update.
   for (const auto &Entry : LatestDiagVersion)
     EXPECT_EQ(Entry.second, UpdatesPerFile - 1);
   EXPECT_EQ(TotalASTReads, FilesCount * UpdatesPerFile);
   EXPECT_EQ(TotalPreambleReads, FilesCount * UpdatesPerFile);
 }
 
 TEST_F(TUSchedulerTests, EvictedAST) {
-  std::atomic<int> BuiltASTCounter(0);
   auto Opts = optsForTest();
   Opts.AsyncThreadsCount = 1;
   Opts.RetentionPolicy.MaxRetainedASTs = 2;
   trace::TestTracer Tracer;
   TUScheduler S(CDB, Opts);
 
   llvm::StringLiteral SourceContents = R"cpp(
     int* a;
     double* b = a;
   )cpp";
-  llvm::StringLiteral OtherSourceContents = R"cpp(
-    int* a;
-    double* b = a + 0;
-  )cpp";
 
   auto Foo = testPath("foo.cpp");
   auto Bar = testPath("bar.cpp");
   auto Baz = testPath("baz.cpp");
 
-  EXPECT_THAT(Tracer.takeMetric("ast_access_diag", "hit"), SizeIs(0));
-  EXPECT_THAT(Tracer.takeMetric("ast_access_diag", "miss"), SizeIs(0));
-  // Build one file in advance. We will not access it later, so it will be the
-  // one that the cache will evict.
-  updateWithCallback(S, Foo, SourceContents, WantDiagnostics::Yes,
-                     [&BuiltASTCounter]() { ++BuiltASTCounter; });
-  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
-  ASSERT_EQ(BuiltASTCounter.load(), 1);
-  EXPECT_THAT(Tracer.takeMetric("ast_access_diag", "hit"), SizeIs(0));
-  EXPECT_THAT(Tracer.takeMetric("ast_access_diag", "miss"), SizeIs(1));
-
-  // Build two more files. Since we can retain only 2 ASTs, these should be
-  // the ones we see in the cache later.
-  updateWithCallback(S, Bar, SourceContents, WantDiagnostics::Yes,
-                     [&BuiltASTCounter]() { ++BuiltASTCounter; });
-  updateWithCallback(S, Baz, SourceContents, WantDiagnostics::Yes,
-                     [&BuiltASTCounter]() { ++BuiltASTCounter; });
-  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
-  ASSERT_EQ(BuiltASTCounter.load(), 3);
-  EXPECT_THAT(Tracer.takeMetric("ast_access_diag", "hit"), SizeIs(0));
-  EXPECT_THAT(Tracer.takeMetric("ast_access_diag", "miss"), SizeIs(2));
+  // Build one file in advance.
+  S.update(Foo, getInputs(Foo, SourceContents.str()), WantDiagnostics::Yes);
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+  // AST should be cached after diagnostics are produced.
+  ASSERT_THAT(S.getFilesWithCachedAST(), UnorderedElementsAre(Foo));
 
-  // Check only the last two ASTs are retained.
+  // Build two more files.
+  S.update(Bar, getInputs(Foo, SourceContents.str()), WantDiagnostics::Yes);
+  S.update(Baz, getInputs(Foo, SourceContents.str()), WantDiagnostics::Yes);
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+  // We can retain only 2 ASTs, Foo should have been evicted.
   ASSERT_THAT(S.getFilesWithCachedAST(), UnorderedElementsAre(Bar, Baz));
 
+  // No reads yet.
+  EXPECT_THAT(Tracer.takeMetric("ast_access_read", "hit"), SizeIs(0));
+  EXPECT_THAT(Tracer.takeMetric("ast_access_read", "miss"), SizeIs(0));
+
+  // Run action on file in AST cache.
+  S.runWithAST("Cached", Bar,
+               [](llvm::Expected<InputsAndAST> E) { EXPECT_TRUE(bool(E)); });
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+  // Should have hit the cache, same files still present.
+  ASSERT_THAT(S.getFilesWithCachedAST(), UnorderedElementsAre(Bar, Baz));
+  EXPECT_THAT(Tracer.takeMetric("ast_access_read", "hit"), SizeIs(1));
+  EXPECT_THAT(Tracer.takeMetric("ast_access_read", "miss"), SizeIs(0));
+
   // Access the old file again.
-  updateWithCallback(S, Foo, OtherSourceContents, WantDiagnostics::Yes,
-                     [&BuiltASTCounter]() { ++BuiltASTCounter; });
+  S.runWithAST("Uncached", Foo,
+               [](llvm::Expected<InputsAndAST> E) { EXPECT_TRUE(bool(E)); });
   ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
-  ASSERT_EQ(BuiltASTCounter.load(), 4);
-  EXPECT_THAT(Tracer.takeMetric("ast_access_diag", "hit"), SizeIs(0));
-  EXPECT_THAT(Tracer.takeMetric("ast_access_diag", "miss"), SizeIs(1));
-
-  // Check the AST for foo.cpp is retained now and one of the others got
-  // evicted.
-  EXPECT_THAT(S.getFilesWithCachedAST(),
-              UnorderedElementsAre(Foo, AnyOf(Bar, Baz)));
+  // We missed the cache. Now Foo is retained and Baz got evicted (LRU).
+  EXPECT_THAT(Tracer.takeMetric("ast_access_read", "hit"), SizeIs(0));
+  EXPECT_THAT(Tracer.takeMetric("ast_access_read", "miss"), SizeIs(1));
+  EXPECT_THAT(S.getFilesWithCachedAST(), UnorderedElementsAre(Foo, Bar));
 }
 
 // We send "empty" changes to TUScheduler when we think some external event
 // *might* have invalidated current state (e.g. a header was edited).
 // Verify that this doesn't evict our cache entries.
 TEST_F(TUSchedulerTests, NoopChangesDontThrashCache) {
   auto Opts = optsForTest();
   Opts.RetentionPolicy.MaxRetainedASTs = 1;
TEST_F(TUSchedulerTests, NoChangeDiags) {
   trace::TestTracer Tracer;
   TUScheduler S(CDB, optsForTest(), captureDiags());
 
   auto FooCpp = testPath("foo.cpp");
   const auto *Contents = "int a; int b;";
 
   EXPECT_THAT(Tracer.takeMetric("ast_access_read", "hit"), SizeIs(0));
   EXPECT_THAT(Tracer.takeMetric("ast_access_read", "miss"), SizeIs(0));
-  EXPECT_THAT(Tracer.takeMetric("ast_access_diag", "hit"), SizeIs(0));
-  EXPECT_THAT(Tracer.takeMetric("ast_access_diag", "miss"), SizeIs(0));
   updateWithDiags(
       S, FooCpp, Contents, WantDiagnostics::No,
       [](std::vector<Diag>) { ADD_FAILURE() << "Should not be called."; });
   S.runWithAST("touchAST", FooCpp, [](Expected<InputsAndAST> IA) {
     // Make sure the AST was actually built.
     cantFail(std::move(IA));
   });
   ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
   EXPECT_THAT(Tracer.takeMetric("ast_access_read", "hit"), SizeIs(0));
   EXPECT_THAT(Tracer.takeMetric("ast_access_read", "miss"), SizeIs(1));
 
-  // Even though the inputs didn't change and AST can be reused, we need to
+  // Even though the inputs didn't change, we need to
   // report the diagnostics, as they were not reported previously.
   std::atomic<bool> SeenDiags(false);
   updateWithDiags(S, FooCpp, Contents, WantDiagnostics::Auto,
                   [&](std::vector<Diag>) { SeenDiags = true; });
   ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
   ASSERT_TRUE(SeenDiags);
-  EXPECT_THAT(Tracer.takeMetric("ast_access_diag", "hit"), SizeIs(1));
-  EXPECT_THAT(Tracer.takeMetric("ast_access_diag", "miss"), SizeIs(0));
 
   // Subsequent request does not get any diagnostics callback because the same
   // diags have previously been reported and the inputs didn't change.
   updateWithDiags(
       S, FooCpp, Contents, WantDiagnostics::Auto,
       [&](std::vector<Diag>) { ADD_FAILURE() << "Should not be called."; });
   ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
 }

clang-tools-extra/clangd/unittests/TestTU.cpp

 ParsedAST TestTU::build() const {
   auto Inputs = inputs();
   StoreDiags Diags;
   auto CI = buildCompilerInvocation(Inputs, Diags);
   assert(CI && "Failed to build compilation invocation.");
   auto Preamble = clang::clangd::buildPreamble(testPath(Filename), *CI, Inputs,
                                                /*StoreInMemory=*/true,
                                                /*PreambleCallback=*/nullptr);
-  auto AST = ParsedAST::build(testPath(Filename), Inputs, std::move(CI),
-                              Diags.take(), Preamble);
+  auto AST =
+      ParsedAST::build(testPath(Filename), Inputs, std::move(CI),
+                       /*ProduceDiagnostics=*/true, Diags.take(), Preamble);
   if (!AST.hasValue()) {
     ADD_FAILURE() << "Failed to build code:\n" << Code;
     llvm_unreachable("Failed to build TestTU!");
   }
   // Check for error diagnostics and report gtest failures (unless expected).
   // This guards against accidental syntax errors silently subverting tests.
   // error-ok is awfully primitive - using clang -verify would be nicer.
   // Ownership and layering makes it pretty hard.