[clangd] Introduce pullDiags endpoint

[kadircet]

Summary

Implement initial support for pull-based diagnostics in ClangdServer.
This is planned for LSP 3.17, and initial proposal is in
https://github.com/microsoft/vscode-languageserver-node/blob/d15eb0671e0947d14e3548d13754104ee13aa56d/protocol/src/common/proposed.diagnostic.ts#L111.

We chose to serve the requests only when clangd has a fresh preamble
available. In case of a stale preamble we just drop the request on the
floor.

This patch doesn't plumb this to LSP layer yet, as pullDiags is still a
proposal with only an implementation in vscode.

[kadircet]

• Add fixme for the error code.

[sammccall]

Thanks, nice to see this isn't going to require deep changes, and looking forward to seeing the diagnostics performance improvements.
(I wonder if we'll end up in the perverse situation where clangd is faster for most operations when your preamble is stale :-D Probably not...)

This patch doesn't plumb this to LSP layer yet, as pullDiags is still a proposal with only an implementation in vscode.

Based on our experiences with LSP + VSCode, I don't think that should stop us - I think it's OK for us to make incompatible changes to a newly-added request that hasn't been in any major release yet.
That said, good to keep it out of this patch for size reasons alone :-)

clang-tools-extra/clangd/ClangdServer.cpp

85–87

Replace comment with assert(AST.getDiagnostics().hasValue() && "We issue callback only with fresh preambles")?

clang-tools-extra/clangd/ClangdServer.h

72

Describe the relationship with the pull diagnostic method? (e.g. opposite of comment suggested below)

353

nit: "busy" is very vague - server is busy (building a preamble)?

354

I assume you mean if retried here.
This isn't a great access pattern though, it creates a useless loop of creating ASTs (or fetching them from cache?) until the preamble is ready. We really want people to retry after onSemanticsMaybeChanged right? So we need to guarantee that will work.

Maybe "If it fails, clients should wait for onSemanticsMaybeChanged and then retry."

355

Naming:

• "diagnostics" rather than "diags" (clangd::Diag is an awkward name to avoid a collision with the LSP struct).
• the "pull" nature is already in the signature, and while the push/pull distinction is very important to us now, I'm not sure it's central enough to put in the name. Rather maybe explain the relationship between them in the doc comment: "These 'pulled' diagnostics do not interfere with the diagnostics 'pushed' to Callbacks::onDiagnosticsReady, and clients may use either or both."

clang-tools-extra/clangd/ParsedAST.cpp

270–271

This is a slightly confusing/strange structure, because the triviality is a property of the patch, and determined when creating, not when applying.

I'd consider instead something more explicit like:

Patch = PreamblePatch::create(...);
PatchedPreamble = Patch.preservesDiagnostics(); // and rename PatchedPreamble variable.
Patch->apply(*CI);

This conceptually separates the idea of "diagnostics are trusted" from "patch is a no-op". Maybe we want to keep these identical forever, but I think PreamblePatch is the right place to express that decision.

(This isn't a big deal though, up to you)

clang-tools-extra/clangd/ParsedAST.h

91–93

please don't declare API methods with auto if we can avoid it :-)

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

138

This assumption now appears in a bunch of places without comment.
Maybe add The result will always have getDiagnostics() populated to build() in TestTU.h, and assert it inside TestTU.cpp?

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

1

I think we want unit tests here or in PreambleTests.cpp to show that the diagnostics get not-set appropriately.

(I'm fine with not testing the ClangdServer layer as it's pretty trivial, as long as we plan to have at least a happy-path smoke lit test for the LSP layer)

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

282

this doesn't produce a useful failure message.
Maybe gather the ranges into a vector<Range> MacroRefRanges and ASSERT_THAT(MacroRefRanges, Contains(Modified.range())) ?

[kadircet]

clang-tools-extra/clangd/ParsedAST.h

91–93

oops forgot to fix after testing :D

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

1

i was hoping some indirect testing of EXPECT_FALSE(PatchedAST->getDiagnostics()) in arbitrary tests to be enough, but you are right. adding an explicit test.

[kadircet]

• Update comments/names
• Instead of using apply, have a separate preserveDiagnostics API in PreamblePatch.

[sammccall]

clang-tools-extra/clangd/ClangdServer.cpp

916

building diagnostics -> parsing includes?

920

nit: PullDiagnostics or Diagnostics

[kadircet]

• address comments

clang-tools-extra/clangd/ClangdServer.h

 //===--- ClangdServer.h - Main clangd server code ----------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_CLANGDSERVER_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CLANGDSERVER_H
 
 #include "../clang-tidy/ClangTidyOptions.h"
 #include "CodeComplete.h"
 #include "ConfigProvider.h"
+#include "Diagnostics.h"
 #include "DraftStore.h"
 #include "FeatureModule.h"
 #include "GlobalCompilationDatabase.h"
 #include "Hover.h"
 #include "Protocol.h"
 #include "SemanticHighlighting.h"
 #include "TUScheduler.h"
 #include "XRefs.h"

 #include "llvm/ADT/FunctionExtras.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include <functional>
 #include <string>
 #include <type_traits>
 #include <utility>
+#include <vector>
 
 namespace clang {
 namespace clangd {
 /// Manages a collection of source files and derived data (ASTs, indexes),
 /// and provides language-aware features such as code completion.
 ///
 /// The primary client is ClangdLSPServer which exposes these features via
 /// the Language Server protocol. ClangdServer may also be embedded directly,
 /// though its API is not stable over time.
 ///
 /// ClangdServer should be used from a single thread. Many potentially-slow
 /// operations have asynchronous APIs and deliver their results on another
 /// thread.
 /// Such operations support cancellation: if the caller sets up a cancelable
 /// context, many operations will notice cancellation and fail early.
 /// (ClangdLSPServer uses this to implement $/cancelRequest).
 class ClangdServer {
 public:
   /// Interface with hooks for users of ClangdServer to be notified of events.
   class Callbacks {
   public:
     virtual ~Callbacks() = default;
 
     /// Called by ClangdServer when \p Diagnostics for \p File are ready.
+    /// These pushed diagnostics might correspond to an older version of the
+    /// file, they do not interfere with "pull-based" ClangdServer::diagnostics.
     /// May be called concurrently for separate files, not for a single file.
     virtual void onDiagnosticsReady(PathRef File, llvm::StringRef Version,
                                     std::vector<Diag> Diagnostics) {}
     /// Called whenever the file status is updated.
     /// May be called concurrently for separate files, not for a single file.
     virtual void onFileUpdated(PathRef File, const TUStatus &Status) {}
 
     /// Called when background indexing tasks are enqueued/started/completed.
     /// Not called concurrently.
     virtual void
public:
 
   /// Runs an arbitrary action that has access to the AST of the specified file.
   /// The action will execute on one of ClangdServer's internal threads.
   /// The AST is only valid for the duration of the callback.
   /// As with other actions, the file must have been opened.
   void customAction(PathRef File, llvm::StringRef Name,
                     Callback<InputsAndAST> Action);
 
+  /// Fetches diagnostics for current version of the \p File. This might fail if
+  /// server is busy (building a preamble) and would require a long time to
+  /// prepare diagnostics. If it fails, clients should wait for
+  /// onSemanticsMaybeChanged and then retry.
+  /// These 'pulled' diagnostics do not interfere with the diagnostics 'pushed'
+  /// to Callbacks::onDiagnosticsReady, and clients may use either or both.
+  void diagnostics(PathRef File, Callback<std::vector<Diag>> CB);
+
   /// Returns estimated memory usage and other statistics for each of the
   /// currently open files.
   /// Overall memory usage of clangd may be significantly more than reported
   /// here, as this metric does not account (at least) for:
   ///   - memory occupied by static and dynamic index,
   ///   - memory required for in-flight requests,
   /// FIXME: those metrics might be useful too, we should add them.
   llvm::StringMap<TUScheduler::FileStats> fileStats() const;

clang-tools-extra/clangd/ClangdServer.cpp

 //===--- ClangdServer.cpp - Main clangd server code --------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===-------------------------------------------------------------------===//
 
 #include "ClangdServer.h"
 #include "CodeComplete.h"
 #include "Config.h"
+#include "Diagnostics.h"
 #include "DumpAST.h"
 #include "FindSymbols.h"
 #include "Format.h"
 #include "HeaderSourceSwitch.h"
 #include "Headers.h"
 #include "ParsedAST.h"
 #include "Preamble.h"
 #include "Protocol.h"
void onPreambleAST(PathRef Path, llvm::StringRef Version, ASTContext &Ctx,
     if (ServerCallbacks)
       ServerCallbacks->onSemanticsMaybeChanged(Path);
   }
 
   void onMainAST(PathRef Path, ParsedAST &AST, PublishFn Publish) override {
     if (FIndex)
       FIndex->updateMain(Path, AST);
 
-    std::vector<Diag> Diagnostics = AST.getDiagnostics();
+    assert(AST.getDiagnostics().hasValue() &&
+           "We issue callback only with fresh preambles");
+    std::vector<Diag> Diagnostics = *AST.getDiagnostics();
     if (ServerCallbacks)
       Publish([&]() {
         ServerCallbacks->onDiagnosticsReady(Path, AST.version(),
                                             std::move(Diagnostics));
       });
   }
 
   void onFailedAST(PathRef Path, llvm::StringRef Version,
void ClangdServer::getAST(PathRef File, Range R,
   WorkScheduler->runWithAST("GetAST", File, std::move(Action));
 }
 
 void ClangdServer::customAction(PathRef File, llvm::StringRef Name,
                                 Callback<InputsAndAST> Action) {
   WorkScheduler->runWithAST(Name, File, std::move(Action));
 }
 
+void ClangdServer::diagnostics(PathRef File, Callback<std::vector<Diag>> CB) {
+  auto Action =
+      [CB = std::move(CB)](llvm::Expected<InputsAndAST> InpAST) mutable {
+        if (!InpAST)
+          return CB(InpAST.takeError());
+        if (auto Diags = InpAST->AST.getDiagnostics())
+          return CB(*Diags);
+        // FIXME: Use ServerCancelled error once it is settled in LSP-3.17.
+        return CB(llvm::make_error<LSPError>("server is busy parsing includes",
+                                             ErrorCode::InternalError));
+      };
+
+  WorkScheduler->runWithAST("Diagnostics", File, std::move(Action));
+}
+
 llvm::StringMap<TUScheduler::FileStats> ClangdServer::fileStats() const {
   return WorkScheduler->fileStats();
 }
 
 LLVM_NODISCARD bool
 ClangdServer::blockUntilIdleForTest(llvm::Optional<double> TimeoutSeconds) {
   // Order is important here: we don't want to block on A and then B,
   // if B might schedule work on A.

clang-tools-extra/clangd/ParsedAST.h

const LangOptions &getLangOpts() const {
     return getASTContext().getLangOpts();
   }
 
   /// This function returns top-level decls present in the main file of the AST.
   /// The result does not include the decls that come from the preamble.
   /// (These should be const, but RecursiveASTVisitor requires Decl*).
   ArrayRef<Decl *> getLocalTopLevelDecls();
 
-  const std::vector<Diag> &getDiagnostics() const;
+  const llvm::Optional<std::vector<Diag>> &getDiagnostics() const {
+    return Diags;
+  }
 
   /// Returns the estimated size of the AST and the accessory structures, in
   /// bytes. Does not include the size of the preamble.
   std::size_t getUsedBytes() const;
   const IncludeStructure &getIncludeStructure() const;
   const CanonicalIncludes &getCanonicalIncludes() const;
 
   /// Gets all macro references (definition, expansions) present in the main
public:
   }
 
 private:
   ParsedAST(llvm::StringRef Version,
             std::shared_ptr<const PreambleData> Preamble,
             std::unique_ptr<CompilerInstance> Clang,
             std::unique_ptr<FrontendAction> Action, syntax::TokenBuffer Tokens,
             MainFileMacros Macros, std::vector<Decl *> LocalTopLevelDecls,
-            std::vector<Diag> Diags, IncludeStructure Includes,
+            llvm::Optional<std::vector<Diag>> Diags, IncludeStructure Includes,
             CanonicalIncludes CanonIncludes);
 
   std::string Version;
   // In-memory preambles must outlive the AST, it is important that this member
   // goes before Clang and Action.
   std::shared_ptr<const PreambleData> Preamble;
   // We store an "incomplete" FrontendAction (i.e. no EndSourceFile was called
   // on it) and CompilerInstance used to run it. That way we don't have to do
   // complex memory management of all Clang structures on our own. (They are
   // stored in CompilerInstance and cleaned up by
   // FrontendAction.EndSourceFile).
   std::unique_ptr<CompilerInstance> Clang;
   std::unique_ptr<FrontendAction> Action;
   /// Tokens recorded after the preamble finished.
   ///   - Includes all spelled tokens for the main file.
   ///   - Includes expanded tokens produced **after** preamble.
   ///   - Does not have spelled or expanded tokens for files from preamble.
   syntax::TokenBuffer Tokens;
 
   /// All macro definitions and expansions in the main file.
   MainFileMacros Macros;
-  // Data, stored after parsing.
-  std::vector<Diag> Diags;
+  // Data, stored after parsing. None if AST was built with a stale preamble.
+  llvm::Optional<std::vector<Diag>> Diags;
   // Top-level decls inside the current file. Not that this does not include
   // top-level decls from the preamble.
   std::vector<Decl *> LocalTopLevelDecls;
   IncludeStructure Includes;
   CanonicalIncludes CanonIncludes;
   std::unique_ptr<HeuristicResolver> Resolver;
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_PARSEDAST_H

clang-tools-extra/clangd/ParsedAST.cpp

ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
 
   // 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;
 
   StoreDiags ASTDiags;
 
   llvm::Optional<PreamblePatch> Patch;
+  bool PreserveDiags = true;
   if (Preamble) {
     Patch = PreamblePatch::create(Filename, Inputs, *Preamble);
     Patch->apply(*CI);
+    PreserveDiags = Patch->preserveDiagnostics();
   }
   auto Clang = prepareCompilerInstance(
       std::move(CI), PreamblePCH,
       llvm::MemoryBuffer::getMemBufferCopy(Inputs.Contents, Filename), VFS,
       ASTDiags);
   if (!Clang)
     return None;
 
ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
   // So just inform the preprocessor of EOF, while keeping everything alive.
   Clang->getPreprocessor().EndSourceFile();
   // UnitDiagsConsumer is local, we can not store it in CompilerInstance that
   // has a longer lifetime.
   Clang->getDiagnostics().setClient(new IgnoreDiagnostics);
   // CompilerInstance won't run this callback, do it directly.
   ASTDiags.EndSourceFile();
 
-  std::vector<Diag> Diags = CompilerInvocationDiags;
+  llvm::Optional<std::vector<Diag>> Diags;
+  // FIXME: Also skip generation of diagnostics alltogether to speed up ast
+  // builds when we are patching a stale preamble.
+  if (PreserveDiags) {
+    Diags = CompilerInvocationDiags;
     // Add diagnostics from the preamble, if any.
     if (Preamble)
-    Diags.insert(Diags.end(), Preamble->Diags.begin(), Preamble->Diags.end());
+      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());
+      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;

 }
 
 llvm::ArrayRef<Decl *> ParsedAST::getLocalTopLevelDecls() {
   return LocalTopLevelDecls;
 }
 
 const MainFileMacros &ParsedAST::getMacros() const { return Macros; }
 
-const std::vector<Diag> &ParsedAST::getDiagnostics() const { return Diags; }
-
 std::size_t ParsedAST::getUsedBytes() const {
   auto &AST = getASTContext();
   // FIXME(ibiryukov): we do not account for the dynamically allocated part of
   // Message and Fixes inside each diagnostic.
-  std::size_t Total =
-      clangd::getUsedBytes(LocalTopLevelDecls) + clangd::getUsedBytes(Diags);
+  std::size_t Total = clangd::getUsedBytes(LocalTopLevelDecls) +
+                      (Diags ? clangd::getUsedBytes(*Diags) : 0);
 
   // FIXME: the rest of the function is almost a direct copy-paste from
   // libclang's clang_getCXTUResourceUsage. We could share the implementation.
 
   // Sum up various allocators inside the ast context and the preprocessor.
   Total += AST.getASTAllocatedMemory();
   Total += AST.getSideTableAllocatedMemory();
   Total += AST.Idents.getAllocator().getTotalMemory();

 }
 
 ParsedAST::ParsedAST(llvm::StringRef Version,
                      std::shared_ptr<const PreambleData> Preamble,
                      std::unique_ptr<CompilerInstance> Clang,
                      std::unique_ptr<FrontendAction> Action,
                      syntax::TokenBuffer Tokens, MainFileMacros Macros,
                      std::vector<Decl *> LocalTopLevelDecls,
-                     std::vector<Diag> Diags, IncludeStructure Includes,
-                     CanonicalIncludes CanonIncludes)
+                     llvm::Optional<std::vector<Diag>> Diags,
+                     IncludeStructure Includes, CanonicalIncludes CanonIncludes)
     : Version(Version), Preamble(std::move(Preamble)), Clang(std::move(Clang)),
       Action(std::move(Action)), Tokens(std::move(Tokens)),
       Macros(std::move(Macros)), Diags(std::move(Diags)),
       LocalTopLevelDecls(std::move(LocalTopLevelDecls)),
       Includes(std::move(Includes)), CanonIncludes(std::move(CanonIncludes)) {
   Resolver = std::make_unique<HeuristicResolver>(getASTContext());
   assert(this->Clang);
   assert(this->Action);

clang-tools-extra/clangd/Preamble.h

public:
   std::vector<Inclusion> preambleIncludes() const;
 
   /// Returns preamble bounds for the Modified.
   PreambleBounds modifiedBounds() const { return ModifiedBounds; }
 
   /// Returns textual patch contents.
   llvm::StringRef text() const { return PatchContents; }
 
+  /// Whether diagnostics generated using this patch are trustable.
+  bool preserveDiagnostics() const { return PatchContents.empty(); }
+
 private:
   PreamblePatch() = default;
   std::string PatchContents;
   std::string PatchFileName;
   /// Includes that are present in both \p Baseline and \p Modified. Used for
   /// patching includes of baseline preamble.
   std::vector<Inclusion> PreambleIncludes;
   PreambleBounds ModifiedBounds = {0, false};

clang-tools-extra/clangd/tool/Check.cpp

bool buildAST() {
 
     log("Building AST...");
     AST = ParsedAST::build(File, Inputs, std::move(Invocation),
                            /*InvocationDiags=*/std::vector<Diag>{}, Preamble);
     if (!AST) {
       elog("Failed to build AST");
       return false;
     }
-    ErrCount += showErrors(llvm::makeArrayRef(AST->getDiagnostics())
+    ErrCount += showErrors(llvm::makeArrayRef(*AST->getDiagnostics())
                                .drop_front(Preamble->Diags.size()));
 
     if (Opts.BuildDynamicSymbolIndex) {
       log("Indexing AST...");
       Index.updateMain(File, *AST);
     }
     return true;
   }

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

o]]();
       a.$nomember[[y]];
       test::$nomembernamespace[[test]];
       $macro[[ID($macroarg[[fod]])]]();
     }
   )cpp");
   auto TU = TestTU::withCode(Test.code());
   TU.ClangTidyProvider = addTidyChecks("google-explicit-constructor");
   EXPECT_THAT(
-      TU.build().getDiagnostics(),
+      *TU.build().getDiagnostics(),
       ElementsAre(
           // This range spans lines.
           AllOf(Diag(Test.range("typo"),
                      "use of undeclared identifier 'goo'; did you mean 'foo'?"),
                 DiagSource(Diag::Clang), DiagName("undeclared_var_use_suggest"),
                 WithFix(
                     Fix(Test.range("typo"), "foo", "change 'go\\…' to 'foo'")),
                 // This is a pretty normal range.
EXPECT_THAT(
                      "avoid unintentional implicit conversions"),
                 WithFix(Fix(Test.range("explicit"), "explicit ",
                             "insert 'explicit '")))));
 }
 
 TEST(DiagnosticsTest, FlagsMatter) {
   Annotations Test("[[void]] main() {} // error-ok");
   auto TU = TestTU::withCode(Test.code());
-  EXPECT_THAT(TU.build().getDiagnostics(),
+  EXPECT_THAT(*TU.build().getDiagnostics(),
               ElementsAre(AllOf(Diag(Test.range(), "'main' must return 'int'"),
                                 WithFix(Fix(Test.range(), "int",
                                             "change 'void' to 'int'")))));
   // Same code built as C gets different diagnostics.
   TU.Filename = "Plain.c";
   EXPECT_THAT(
-      TU.build().getDiagnostics(),
+      *TU.build().getDiagnostics(),
       ElementsAre(AllOf(
           Diag(Test.range(), "return type of 'main' is not 'int'"),
           WithFix(Fix(Test.range(), "int", "change return type to 'int'")))));
 }
 
 TEST(DiagnosticsTest, DiagnosticPreamble) {
   Annotations Test(R"cpp(
     #include $[["not-found.h"]] // error-ok
   )cpp");
 
   auto TU = TestTU::withCode(Test.code());
-  EXPECT_THAT(TU.build().getDiagnostics(),
+  EXPECT_THAT(*TU.build().getDiagnostics(),
               ElementsAre(::testing::AllOf(
                   Diag(Test.range(), "'not-found.h' file not found"),
                   DiagSource(Diag::Clang), DiagName("pp_file_not_found"))));
 }
 
 TEST(DiagnosticsTest, DeduplicatedClangTidyDiagnostics) {
   Annotations Test(R"cpp(
     float foo = [[0.1f]];
   )cpp");
   auto TU = TestTU::withCode(Test.code());
   // Enable alias clang-tidy checks, these check emits the same diagnostics
   // (except the check name).
   TU.ClangTidyProvider = addTidyChecks("readability-uppercase-literal-suffix,"
                                        "hicpp-uppercase-literal-suffix");
   // Verify that we filter out the duplicated diagnostic message.
   EXPECT_THAT(
-      TU.build().getDiagnostics(),
+      *TU.build().getDiagnostics(),
       UnorderedElementsAre(::testing::AllOf(
           Diag(Test.range(),
                "floating point literal has suffix 'f', which is not uppercase"),
           DiagSource(Diag::ClangTidy))));
 
   Test = Annotations(R"cpp(
     template<typename T>
     void func(T) {
       float f = [[0.3f]];
     }
     void k() {
       func(123);
       func(2.0);
     }
   )cpp");
   TU.Code = std::string(Test.code());
   // The check doesn't handle template instantiations which ends up emitting
   // duplicated messages, verify that we deduplicate them.
   EXPECT_THAT(
-      TU.build().getDiagnostics(),
+      *TU.build().getDiagnostics(),
       UnorderedElementsAre(::testing::AllOf(
           Diag(Test.range(),
                "floating point literal has suffix 'f', which is not uppercase"),
           DiagSource(Diag::ClangTidy))));
 }
 
 TEST(DiagnosticsTest, ClangTidy) {
   Annotations Test(R"cpp(
     #include $deprecated[["assert.h"]]
 
     #define $macrodef[[SQUARE]](X) (X)*(X)
     int $main[[main]]() {
       int y = 4;
       return SQUARE($macroarg[[++]]y);
       return $doubled[[sizeof]](sizeof(int));
     }
   )cpp");
   auto TU = TestTU::withCode(Test.code());
   TU.HeaderFilename = "assert.h"; // Suppress "not found" error.
   TU.ClangTidyProvider = addTidyChecks("bugprone-sizeof-expression,"
                                        "bugprone-macro-repeated-side-effects,"
                                        "modernize-deprecated-headers,"
                                        "modernize-use-trailing-return-type");
   EXPECT_THAT(
-      TU.build().getDiagnostics(),
+      *TU.build().getDiagnostics(),
       UnorderedElementsAre(
           AllOf(Diag(Test.range("deprecated"),
                      "inclusion of deprecated C++ header 'assert.h'; consider "
                      "using 'cassert' instead"),
                 DiagSource(Diag::ClangTidy),
                 DiagName("modernize-deprecated-headers"),
                 WithFix(Fix(Test.range("deprecated"), "<cassert>",
                             "change '\"assert.h\"' to '<cassert>'"))),
TEST(DiagnosticsTest, ClangTidyEOF) {
   [[#]]include <b.h>
   #include "a.h")cpp");
   // clang-format on
   auto TU = TestTU::withCode(Test.code());
   TU.ExtraArgs = {"-isystem."};
   TU.AdditionalFiles["a.h"] = TU.AdditionalFiles["b.h"] = "";
   TU.ClangTidyProvider = addTidyChecks("llvm-include-order");
   EXPECT_THAT(
-      TU.build().getDiagnostics(),
+      *TU.build().getDiagnostics(),
       Contains(AllOf(Diag(Test.range(), "#includes are not sorted properly"),
                      DiagSource(Diag::ClangTidy),
                      DiagName("llvm-include-order"))));
 }
 
 TEST(DiagnosticTest, TemplatesInHeaders) {
   // Diagnostics from templates defined in headers are placed at the expansion.
   Annotations Main(R"cpp(
     Derived<int> [[y]]; // error-ok
   )cpp");
   Annotations Header(R"cpp(
     template <typename T>
     struct Derived : [[T]] {};
   )cpp");
   TestTU TU = TestTU::withCode(Main.code());
   TU.HeaderCode = Header.code().str();
   EXPECT_THAT(
-      TU.build().getDiagnostics(),
+      *TU.build().getDiagnostics(),
       ElementsAre(AllOf(
           Diag(Main.range(), "in template: base specifier must name a class"),
           WithNote(Diag(Header.range(), "error occurred here"),
                    Diag(Main.range(), "in instantiation of template class "
                                       "'Derived<int>' requested here")))));
 }
 
 TEST(DiagnosticTest, MakeUnique) {
TU.HeaderCode = R"cpp(
     // These mocks aren't quite right - we omit unique_ptr for simplicity.
     // forward is included to show its body is not needed to get the diagnostic.
     template <typename T> T&& forward(T& t) { return static_cast<T&&>(t); }
     template <typename T, typename... A> T* make_unique(A&&... args) {
        return new T(std::forward<A>(args)...);
     }
     }
   )cpp";
-  EXPECT_THAT(TU.build().getDiagnostics(),
+  EXPECT_THAT(*TU.build().getDiagnostics(),
               UnorderedElementsAre(
                   Diag(Main.range(),
                        "in template: "
                        "no matching constructor for initialization of 'S'")));
 }
 
 TEST(DiagnosticTest, NoMultipleDiagnosticInFlight) {
   Annotations Main(R"cpp(
void f() {
       [[for]] (auto it = label_info_map.begin(); it != label_info_map.end(); ++it) {
         auto S = *it;
       }
     }
   )cpp");
   TestTU TU = TestTU::withCode(Main.code());
   TU.ClangTidyProvider = addTidyChecks("modernize-loop-convert");
   EXPECT_THAT(
-      TU.build().getDiagnostics(),
+      *TU.build().getDiagnostics(),
       UnorderedElementsAre(::testing::AllOf(
           Diag(Main.range(), "use range-based for loop instead"),
           DiagSource(Diag::ClangTidy), DiagName("modernize-loop-convert"))));
 }
 
 TEST(DiagnosticTest, RespectsDiagnosticConfig) {
   Annotations Main(R"cpp(
     // error-ok
     void x() {
       [[unknown]]();
       $ret[[return]] 42;
     }
   )cpp");
   auto TU = TestTU::withCode(Main.code());
   EXPECT_THAT(
-      TU.build().getDiagnostics(),
+      *TU.build().getDiagnostics(),
       ElementsAre(Diag(Main.range(), "use of undeclared identifier 'unknown'"),
                   Diag(Main.range("ret"),
                        "void function 'x' should not return a value")));
   Config Cfg;
   Cfg.Diagnostics.Suppress.insert("return-type");
   WithContextValue WithCfg(Config::Key, std::move(Cfg));
-  EXPECT_THAT(TU.build().getDiagnostics(),
+  EXPECT_THAT(*TU.build().getDiagnostics(),
               ElementsAre(Diag(Main.range(),
                                "use of undeclared identifier 'unknown'")));
 }
 
 TEST(DiagnosticTest, ClangTidySuppressionComment) {
   Annotations Main(R"cpp(
     int main() {
       int i = 3;
       double d = 8 / i;  // NOLINT
       // NOLINTNEXTLINE
       double e = 8 / i;
       #define BAD 8 / i
       double f = BAD;  // NOLINT
       double g = [[8]] / i;
       #define BAD2 BAD
       double h = BAD2;  // NOLINT
     }
   )cpp");
   TestTU TU = TestTU::withCode(Main.code());
   TU.ClangTidyProvider = addTidyChecks("bugprone-integer-division");
   EXPECT_THAT(
-      TU.build().getDiagnostics(),
+      *TU.build().getDiagnostics(),
       UnorderedElementsAre(::testing::AllOf(
           Diag(Main.range(), "result of integer division used in a floating "
                              "point context; possible loss of precision"),
           DiagSource(Diag::ClangTidy), DiagName("bugprone-integer-division"))));
 }
 
 TEST(DiagnosticTest, ClangTidyWarningAsError) {
   Annotations Main(R"cpp(
     int main() {
       int i = 3;
       double f = [[8]] / i; // error-ok
     }
   )cpp");
   TestTU TU = TestTU::withCode(Main.code());
   TU.ClangTidyProvider =
       addTidyChecks("bugprone-integer-division", "bugprone-integer-division");
   EXPECT_THAT(
-      TU.build().getDiagnostics(),
+      *TU.build().getDiagnostics(),
       UnorderedElementsAre(::testing::AllOf(
           Diag(Main.range(), "result of integer division used in a floating "
                              "point context; possible loss of precision"),
           DiagSource(Diag::ClangTidy), DiagName("bugprone-integer-division"),
           DiagSeverity(DiagnosticsEngine::Error))));
 }
 
 TEST(DiagnosticTest, LongFixMessages) {
   // We limit the size of printed code.
   Annotations Source(R"cpp(
     int main() {
       // error-ok
       int somereallyreallyreallyreallyreallyreallyreallyreallylongidentifier;
       [[omereallyreallyreallyreallyreallyreallyreallyreallylongidentifier]]= 10;
     }
   )cpp");
   TestTU TU = TestTU::withCode(Source.code());
   EXPECT_THAT(
-      TU.build().getDiagnostics(),
+      *TU.build().getDiagnostics(),
       ElementsAre(WithFix(Fix(
           Source.range(),
           "somereallyreallyreallyreallyreallyreallyreallyreallylongidentifier",
           "change 'omereallyreallyreallyreallyreallyreallyreallyreall…' to "
           "'somereallyreallyreallyreallyreallyreallyreallyreal…'"))));
   // Only show changes up to a first newline.
   Source = Annotations(R"cpp(
     // error-ok
     int main() {
       int ident;
       [[ide\
 n]] = 10; // error-ok
     }
   )cpp");
   TU.Code = std::string(Source.code());
-  EXPECT_THAT(TU.build().getDiagnostics(),
+  EXPECT_THAT(*TU.build().getDiagnostics(),
               ElementsAre(WithFix(
                   Fix(Source.range(), "ident", "change 'ide\\…' to 'ident'"))));
 }
 
 TEST(DiagnosticTest, ClangTidySuppressionCommentTrumpsWarningAsError) {
   Annotations Main(R"cpp(
     int main() {
       int i = 3;
       double f = [[8]] / i;  // NOLINT
     }
   )cpp");
   TestTU TU = TestTU::withCode(Main.code());
   TU.ClangTidyProvider =
       addTidyChecks("bugprone-integer-division", "bugprone-integer-division");
-  EXPECT_THAT(TU.build().getDiagnostics(), UnorderedElementsAre());
+  EXPECT_THAT(*TU.build().getDiagnostics(), UnorderedElementsAre());
 }
 
 TEST(DiagnosticTest, ClangTidyNoLiteralDataInMacroToken) {
   Annotations Main(R"cpp(
     #define SIGTERM 15
     using pthread_t = int;
     int pthread_kill(pthread_t thread, int sig);
     int func() {
       pthread_t thread;
       return pthread_kill(thread, 0);
     }
   )cpp");
   TestTU TU = TestTU::withCode(Main.code());
   TU.ClangTidyProvider = addTidyChecks("bugprone-bad-signal-to-kill-thread");
-  EXPECT_THAT(TU.build().getDiagnostics(), UnorderedElementsAre()); // no-crash
+  EXPECT_THAT(*TU.build().getDiagnostics(), UnorderedElementsAre()); // no-crash
 }
 
 TEST(DiagnosticTest, ElseAfterReturnRange) {
   Annotations Main(R"cpp(
     int foo(int cond) {
     if (cond == 1) {
       return 42;
     } [[else]] if (cond == 2) {
       return 43;
     }
     return 44;
     }
   )cpp");
   TestTU TU = TestTU::withCode(Main.code());
   TU.ClangTidyProvider = addTidyChecks("llvm-else-after-return");
   EXPECT_THAT(
-      TU.build().getDiagnostics(),
+      *TU.build().getDiagnostics(),
       ElementsAre(Diag(Main.range(), "do not use 'else' after 'return'")));
 }
 
 TEST(DiagnosticsTest, Preprocessor) {
   // This looks like a preamble, but there's an #else in the middle!
   // Check that:
   //  - the #else doesn't generate diagnostics (we had this bug)
   //  - we get diagnostics from the taken branch
   //  - we get no diagnostics from the not taken branch
   Annotations Test(R"cpp(
     #ifndef FOO
     #define FOO
       int a = [[b]]; // error-ok
     #else
       int x = y;
     #endif
     )cpp");
   EXPECT_THAT(
-      TestTU::withCode(Test.code()).build().getDiagnostics(),
+      *TestTU::withCode(Test.code()).build().getDiagnostics(),
       ElementsAre(Diag(Test.range(), "use of undeclared identifier 'b'")));
 }
 
 TEST(DiagnosticsTest, IgnoreVerify) {
   auto TU = TestTU::withCode(R"cpp(
     int a; // expected-error {{}}
   )cpp");
   TU.ExtraArgs.push_back("-Xclang");
   TU.ExtraArgs.push_back("-verify");
-  EXPECT_THAT(TU.build().getDiagnostics(), IsEmpty());
+  EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
 }
 
 // Recursive main-file include is diagnosed, and doesn't crash.
 TEST(DiagnosticsTest, RecursivePreamble) {
   auto TU = TestTU::withCode(R"cpp(
     #include "foo.h" // error-ok
     int symbol;
   )cpp");
   TU.Filename = "foo.h";
-  EXPECT_THAT(TU.build().getDiagnostics(),
+  EXPECT_THAT(*TU.build().getDiagnostics(),
               ElementsAre(DiagName("pp_including_mainfile_in_preamble")));
   EXPECT_THAT(TU.build().getLocalTopLevelDecls(), SizeIs(1));
 }
 
 // Recursive main-file include with #pragma once guard is OK.
 TEST(DiagnosticsTest, RecursivePreamblePragmaOnce) {
   auto TU = TestTU::withCode(R"cpp(
     #pragma once
     #include "foo.h"
     int symbol;
   )cpp");
   TU.Filename = "foo.h";
-  EXPECT_THAT(TU.build().getDiagnostics(), IsEmpty());
+  EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
   EXPECT_THAT(TU.build().getLocalTopLevelDecls(), SizeIs(1));
 }
 
 // Recursive main-file include with #ifndef guard should be OK.
 // However, it's not yet recognized (incomplete at end of preamble).
 TEST(DiagnosticsTest, RecursivePreambleIfndefGuard) {
   auto TU = TestTU::withCode(R"cpp(
     #ifndef FOO
     #define FOO
     #include "foo.h" // error-ok
     int symbol;
     #endif
   )cpp");
   TU.Filename = "foo.h";
   // FIXME: should be no errors here.
-  EXPECT_THAT(TU.build().getDiagnostics(),
+  EXPECT_THAT(*TU.build().getDiagnostics(),
               ElementsAre(DiagName("pp_including_mainfile_in_preamble")));
   EXPECT_THAT(TU.build().getLocalTopLevelDecls(), SizeIs(1));
 }
 
 TEST(DiagnosticsTest, InsideMacros) {
   Annotations Test(R"cpp(
     #define TEN 10
     #define RET(x) return x + 10
 
     int* foo() {
       RET($foo[[0]]); // error-ok
     }
     int* bar() {
       return $bar[[TEN]];
     }
     )cpp");
-  EXPECT_THAT(TestTU::withCode(Test.code()).build().getDiagnostics(),
+  EXPECT_THAT(*TestTU::withCode(Test.code()).build().getDiagnostics(),
               ElementsAre(Diag(Test.range("foo"),
                                "cannot initialize return object of type "
                                "'int *' with an rvalue of type 'int'"),
                           Diag(Test.range("bar"),
                                "cannot initialize return object of type "
                                "'int *' with an rvalue of type 'int'")));
 }
 
 TEST(DiagnosticsTest, NoFixItInMacro) {
   Annotations Test(R"cpp(
     #define Define(name) void name() {}
 
     [[Define]](main) // error-ok
   )cpp");
   auto TU = TestTU::withCode(Test.code());
-  EXPECT_THAT(TU.build().getDiagnostics(),
+  EXPECT_THAT(*TU.build().getDiagnostics(),
               ElementsAre(AllOf(Diag(Test.range(), "'main' must return 'int'"),
                                 Not(WithFix(_)))));
 }
 
 TEST(ClangdTest, MSAsm) {
   // Parsing MS assembly tries to use the target MCAsmInfo, which we don't link.
   // We used to crash here. Now clang emits a diagnostic, which we filter out.
   llvm::InitializeAllTargetInfos(); // As in ClangdMain
   auto TU = TestTU::withCode("void fn() { __asm { cmp cl,64 } }");
   TU.ExtraArgs = {"-fms-extensions"};
-  EXPECT_THAT(TU.build().getDiagnostics(), IsEmpty());
+  EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
 }
 
 TEST(DiagnosticsTest, ToLSP) {
   URIForFile MainFile =
       URIForFile::canonicalize(testPath("foo/bar/main.cpp"), "");
   URIForFile HeaderFile =
       URIForFile::canonicalize(testPath("foo/bar/header.h"), "");
 
class T {
   )cpp");
   auto TU = TestTU::withCode(Test.code());
   TU.ExtraArgs.push_back("-std=c++17");
   auto Index = buildIndexWithSymbol(
       {SymbolWithHeader{"ns::X", "unittest:///x.h", "\"x.h\""}});
   TU.ExternalIndex = Index.get();
 
   EXPECT_THAT(
-      TU.build().getDiagnostics(),
+      *TU.build().getDiagnostics(),
       UnorderedElementsAreArray(
           {AllOf(Diag(Test.range("nested"),
                       "incomplete type 'ns::X' named in nested name specifier"),
                  DiagName("incomplete_nested_name_spec"),
                  WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
                              "Add include \"x.h\" for symbol ns::X"))),
            AllOf(Diag(Test.range("base"), "base class has incomplete type"),
                  DiagName("incomplete_base_class"),
int main() {
   Sym.IncludeHeaders.emplace_back("\"x.h\"", 1);
 
   SymbolSlab::Builder Slab;
   Slab.insert(Sym);
   auto Index =
       MemIndex::build(std::move(Slab).build(), RefSlab(), RelationSlab());
   TU.ExternalIndex = Index.get();
 
-  EXPECT_THAT(TU.build().getDiagnostics(),
+  EXPECT_THAT(*TU.build().getDiagnostics(),
               UnorderedElementsAre(
                   Diag(Test.range("base"), "base class has incomplete type"),
                   Diag(Test.range("access"),
                        "member access into incomplete type 'ns::X'")));
 }
 
 TEST(IncludeFixerTest, Typo) {
   Annotations Test(R"cpp(// error-ok
using Type = ns::$template[[Foo]]<int>;
   auto TU = TestTU::withCode(Test.code());
   auto Index = buildIndexWithSymbol(
       {SymbolWithHeader{"ns::X", "unittest:///x.h", "\"x.h\""},
        SymbolWithHeader{"Global", "unittest:///global.h", "\"global.h\""},
        SymbolWithHeader{"ns::Foo", "unittest:///foo.h", "\"foo.h\""}});
   TU.ExternalIndex = Index.get();
 
   EXPECT_THAT(
-      TU.build().getDiagnostics(),
+      *TU.build().getDiagnostics(),
       UnorderedElementsAre(
           AllOf(Diag(Test.range("unqualified1"), "unknown type name 'X'"),
                 DiagName("unknown_typename"),
                 WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
                             "Add include \"x.h\" for symbol ns::X"))),
           Diag(Test.range("unqualified2"), "use of undeclared identifier 'X'"),
           AllOf(Diag(Test.range("qualified1"),
                      "no type named 'X' in namespace 'ns'"),

 }
   )cpp");
   auto TU = TestTU::withCode(Test.code());
   auto Index = buildIndexWithSymbol(
       {SymbolWithHeader{"na::X", "unittest:///a.h", "\"a.h\""},
        SymbolWithHeader{"na::nb::X", "unittest:///b.h", "\"b.h\""}});
   TU.ExternalIndex = Index.get();
 
-  EXPECT_THAT(TU.build().getDiagnostics(),
+  EXPECT_THAT(*TU.build().getDiagnostics(),
               UnorderedElementsAre(AllOf(
                   Diag(Test.range("unqualified"), "unknown type name 'X'"),
                   DiagName("unknown_typename"),
                   WithFix(Fix(Test.range("insert"), "#include \"a.h\"\n",
                               "Add include \"a.h\" for symbol na::X"),
                           Fix(Test.range("insert"), "#include \"b.h\"\n",
                               "Add include \"b.h\" for symbol na::nb::X")))));
 }
 
 TEST(IncludeFixerTest, NoCrashMemebrAccess) {
   Annotations Test(R"cpp(// error-ok
     struct X { int  xyz; };
     void g() { X x; x.$[[xy]]; }
   )cpp");
   auto TU = TestTU::withCode(Test.code());
   auto Index = buildIndexWithSymbol(
       SymbolWithHeader{"na::X", "unittest:///a.h", "\"a.h\""});
   TU.ExternalIndex = Index.get();
 
   EXPECT_THAT(
-      TU.build().getDiagnostics(),
+      *TU.build().getDiagnostics(),
       UnorderedElementsAre(Diag(Test.range(), "no member named 'xy' in 'X'")));
 }
 
 TEST(IncludeFixerTest, UseCachedIndexResults) {
   // As index results for the identical request are cached, more than 5 fixes
   // are generated.
   Annotations Test(R"cpp(// error-ok
 $insert[[]]void foo() {

 }
   )cpp");
   auto TU = TestTU::withCode(Test.code());
   auto Index =
       buildIndexWithSymbol(SymbolWithHeader{"X", "unittest:///a.h", "\"a.h\""});
   TU.ExternalIndex = Index.get();
 
   auto Parsed = TU.build();
-  for (const auto &D : Parsed.getDiagnostics()) {
+  for (const auto &D : *Parsed.getDiagnostics()) {
     if (D.Fixes.size() != 1) {
       ADD_FAILURE() << "D.Fixes.size() != 1";
       continue;
     }
     EXPECT_EQ(D.Fixes[0].Message,
               std::string("Add include \"a.h\" for symbol X"));
   }
 }
 
 TEST(IncludeFixerTest, UnresolvedNameAsSpecifier) {
   Annotations Test(R"cpp(// error-ok
 $insert[[]]namespace ns {
 }
 void g() {  ns::$[[scope]]::X_Y();  }
   )cpp");
   TestTU TU;
   TU.Code = std::string(Test.code());
   // FIXME: Figure out why this is needed and remove it, PR43662.
   TU.ExtraArgs.push_back("-fno-ms-compatibility");
   auto Index = buildIndexWithSymbol(
       SymbolWithHeader{"ns::scope::X_Y", "unittest:///x.h", "\"x.h\""});
   TU.ExternalIndex = Index.get();
 
   EXPECT_THAT(
-      TU.build().getDiagnostics(),
+      *TU.build().getDiagnostics(),
       UnorderedElementsAre(AllOf(
           Diag(Test.range(), "no member named 'scope' in namespace 'ns'"),
           DiagName("no_member"),
           WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
                       "Add include \"x.h\" for symbol ns::scope::X_Y")))));
 }
 
 TEST(IncludeFixerTest, UnresolvedSpecifierWithSemaCorrection) {
void f() {
   // FIXME: Figure out why this is needed and remove it, PR43662.
   TU.ExtraArgs.push_back("-fno-ms-compatibility");
   auto Index = buildIndexWithSymbol(
       {SymbolWithHeader{"clang::clangd::X", "unittest:///x.h", "\"x.h\""},
        SymbolWithHeader{"clang::clangd::ns::Y", "unittest:///y.h", "\"y.h\""}});
   TU.ExternalIndex = Index.get();
 
   EXPECT_THAT(
-      TU.build().getDiagnostics(),
+      *TU.build().getDiagnostics(),
       UnorderedElementsAre(
           AllOf(
               Diag(Test.range("q1"), "use of undeclared identifier 'clangd'; "
                                      "did you mean 'clang'?"),
               DiagName("undeclared_var_use_suggest"),
               WithFix(_, // change clangd to clang
                       Fix(Test.range("insert"), "#include \"x.h\"\n",
                           "Add include \"x.h\" for symbol clang::clangd::X"))),
namespace c {
   b::$[[X]] x;
 }
   )cpp");
   auto TU = TestTU::withCode(Test.code());
   auto Index = buildIndexWithSymbol(
       SymbolWithHeader{"a::X", "unittest:///x.h", "\"x.h\""});
   TU.ExternalIndex = Index.get();
 
-  EXPECT_THAT(TU.build().getDiagnostics(),
+  EXPECT_THAT(*TU.build().getDiagnostics(),
               UnorderedElementsAre(AllOf(
                   Diag(Test.range(), "no type named 'X' in namespace 'a'"),
                   DiagName("typename_nested_not_found"),
                   WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
                               "Add include \"x.h\" for symbol a::X")))));
 }
 
 TEST(IncludeFixerTest, NoCrashOnTemplateInstantiations) {
Annotations Test(R"cpp(
     };
   )cpp");
 
   auto TU = TestTU::withCode(Test.code());
   auto Index = buildIndexWithSymbol({});
   TU.ExternalIndex = Index.get();
 
   EXPECT_THAT(
-      TU.build().getDiagnostics(),
+      *TU.build().getDiagnostics(),
       ElementsAre(Diag(Test.range(), "use of undeclared identifier 'a'")));
 }
 
 TEST(DiagsInHeaders, DiagInsideHeader) {
   Annotations Main(R"cpp(
     #include [["a.h"]]
     void foo() {})cpp");
   Annotations Header("[[no_type_spec]]; // error-ok");
   TestTU TU = TestTU::withCode(Main.code());
   TU.AdditionalFiles = {{"a.h", std::string(Header.code())}};
-  EXPECT_THAT(TU.build().getDiagnostics(),
+  EXPECT_THAT(*TU.build().getDiagnostics(),
               UnorderedElementsAre(AllOf(
                   Diag(Main.range(), "in included file: C++ requires a "
                                      "type specifier for all declarations"),
                   WithNote(Diag(Header.range(), "error occurred here")))));
 }
 
 TEST(DiagsInHeaders, DiagInTransitiveInclude) {
   Annotations Main(R"cpp(
     #include [["a.h"]]
     void foo() {})cpp");
   TestTU TU = TestTU::withCode(Main.code());
   TU.AdditionalFiles = {{"a.h", "#include \"b.h\""},
                         {"b.h", "no_type_spec; // error-ok"}};
-  EXPECT_THAT(TU.build().getDiagnostics(),
+  EXPECT_THAT(*TU.build().getDiagnostics(),
               UnorderedElementsAre(
                   Diag(Main.range(), "in included file: C++ requires a "
                                      "type specifier for all declarations")));
 }
 
 TEST(DiagsInHeaders, DiagInMultipleHeaders) {
   Annotations Main(R"cpp(
     #include $a[["a.h"]]
     #include $b[["b.h"]]
     void foo() {})cpp");
   TestTU TU = TestTU::withCode(Main.code());
   TU.AdditionalFiles = {{"a.h", "no_type_spec; // error-ok"},
                         {"b.h", "no_type_spec; // error-ok"}};
-  EXPECT_THAT(TU.build().getDiagnostics(),
+  EXPECT_THAT(*TU.build().getDiagnostics(),
               UnorderedElementsAre(
                   Diag(Main.range("a"), "in included file: C++ requires a type "
                                         "specifier for all declarations"),
                   Diag(Main.range("b"), "in included file: C++ requires a type "
                                         "specifier for all declarations")));
 }
 
 TEST(DiagsInHeaders, PreferExpansionLocation) {
   Annotations Main(R"cpp(
     #include [["a.h"]]
     #include "b.h"
     void foo() {})cpp");
   TestTU TU = TestTU::withCode(Main.code());
   TU.AdditionalFiles = {
       {"a.h", "#include \"b.h\"\n"},
       {"b.h", "#ifndef X\n#define X\nno_type_spec; // error-ok\n#endif"}};
-  EXPECT_THAT(TU.build().getDiagnostics(),
+  EXPECT_THAT(*TU.build().getDiagnostics(),
               UnorderedElementsAre(Diag(Main.range(),
                                         "in included file: C++ requires a type "
                                         "specifier for all declarations")));
 }
 
 TEST(DiagsInHeaders, PreferExpansionLocationMacros) {
   Annotations Main(R"cpp(
     #define X
     #include "a.h"
     #undef X
     #include [["b.h"]]
     void foo() {})cpp");
   TestTU TU = TestTU::withCode(Main.code());
   TU.AdditionalFiles = {
       {"a.h", "#include \"c.h\"\n"},
       {"b.h", "#include \"c.h\"\n"},
       {"c.h", "#ifndef X\n#define X\nno_type_spec; // error-ok\n#endif"}};
-  EXPECT_THAT(TU.build().getDiagnostics(),
+  EXPECT_THAT(*TU.build().getDiagnostics(),
               UnorderedElementsAre(
                   Diag(Main.range(), "in included file: C++ requires a "
                                      "type specifier for all declarations")));
 }
 
 TEST(DiagsInHeaders, LimitDiagsOutsideMainFile) {
   Annotations Main(R"cpp(
     #include [["a.h"]]
TU.AdditionalFiles = {{"a.h", "#include \"c.h\"\n"},
       no_type_spec_4;
       no_type_spec_5;
       no_type_spec_6;
       no_type_spec_7;
       no_type_spec_8;
       no_type_spec_9;
       no_type_spec_10;
       #endif)cpp"}};
-  EXPECT_THAT(TU.build().getDiagnostics(),
+  EXPECT_THAT(*TU.build().getDiagnostics(),
               UnorderedElementsAre(
                   Diag(Main.range(), "in included file: C++ requires a "
                                      "type specifier for all declarations")));
 }
 
 TEST(DiagsInHeaders, OnlyErrorOrFatal) {
   Annotations Main(R"cpp(
     #include [["a.h"]]
     void foo() {})cpp");
   Annotations Header(R"cpp(
     [[no_type_spec]]; // error-ok
     int x = 5/0;)cpp");
   TestTU TU = TestTU::withCode(Main.code());
   TU.AdditionalFiles = {{"a.h", std::string(Header.code())}};
-  EXPECT_THAT(TU.build().getDiagnostics(),
+  EXPECT_THAT(*TU.build().getDiagnostics(),
               UnorderedElementsAre(AllOf(
                   Diag(Main.range(), "in included file: C++ requires "
                                      "a type specifier for all declarations"),
                   WithNote(Diag(Header.range(), "error occurred here")))));
 }
 
 TEST(DiagsInHeaders, OnlyDefaultErrorOrFatal) {
   Annotations Main(R"cpp(
     #include [["a.h"]] // get unused "foo" warning when building preamble.
     )cpp");
   Annotations Header(R"cpp(
     namespace { void foo() {} }
     void func() {foo();} ;)cpp");
   TestTU TU = TestTU::withCode(Main.code());
   TU.AdditionalFiles = {{"a.h", std::string(Header.code())}};
   // promote warnings to errors.
   TU.ExtraArgs = {"-Werror", "-Wunused"};
-  EXPECT_THAT(TU.build().getDiagnostics(), IsEmpty());
+  EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
 }
 
 TEST(DiagsInHeaders, FromNonWrittenSources) {
   Annotations Main(R"cpp(
     #include [["a.h"]]
     void foo() {})cpp");
   Annotations Header(R"cpp(
     int x = 5/0;
     int b = [[FOO]]; // error-ok)cpp");
   TestTU TU = TestTU::withCode(Main.code());
   TU.AdditionalFiles = {{"a.h", std::string(Header.code())}};
   TU.ExtraArgs = {"-DFOO=NOOO"};
-  EXPECT_THAT(TU.build().getDiagnostics(),
+  EXPECT_THAT(*TU.build().getDiagnostics(),
               UnorderedElementsAre(AllOf(
                   Diag(Main.range(),
                        "in included file: use of undeclared identifier 'NOOO'"),
                   WithNote(Diag(Header.range(), "error occurred here")))));
 }
 
 TEST(DiagsInHeaders, ErrorFromMacroExpansion) {
   Annotations Main(R"cpp(
   void bar() {
     int fo; // error-ok
     #include [["a.h"]]
   })cpp");
   Annotations Header(R"cpp(
   #define X foo
   X;)cpp");
   TestTU TU = TestTU::withCode(Main.code());
   TU.AdditionalFiles = {{"a.h", std::string(Header.code())}};
-  EXPECT_THAT(TU.build().getDiagnostics(),
+  EXPECT_THAT(*TU.build().getDiagnostics(),
               UnorderedElementsAre(
                   Diag(Main.range(), "in included file: use of undeclared "
                                      "identifier 'foo'; did you mean 'fo'?")));
 }
 
 TEST(DiagsInHeaders, ErrorFromMacroArgument) {
   Annotations Main(R"cpp(
   void bar() {
     int fo; // error-ok
     #include [["a.h"]]
   })cpp");
   Annotations Header(R"cpp(
   #define X(arg) arg
   X(foo);)cpp");
   TestTU TU = TestTU::withCode(Main.code());
   TU.AdditionalFiles = {{"a.h", std::string(Header.code())}};
-  EXPECT_THAT(TU.build().getDiagnostics(),
+  EXPECT_THAT(*TU.build().getDiagnostics(),
               UnorderedElementsAre(
                   Diag(Main.range(), "in included file: use of undeclared "
                                      "identifier 'foo'; did you mean 'fo'?")));
 }
 
 TEST(IgnoreDiags, FromNonWrittenInclude) {
   TestTU TU;
   TU.ExtraArgs.push_back("--include=a.h");
   TU.AdditionalFiles = {{"a.h", "void main();"}};
   // The diagnostic "main must return int" is from the header, we don't attempt
   // to render it in the main file as there is no written location there.
-  EXPECT_THAT(TU.build().getDiagnostics(), UnorderedElementsAre());
+  EXPECT_THAT(*TU.build().getDiagnostics(), UnorderedElementsAre());
 }
 
 TEST(ToLSPDiag, RangeIsInMain) {
   ClangdDiagnosticOptions Opts;
   clangd::Diag D;
   D.Range = {pos(1, 2), pos(3, 4)};
   D.Notes.emplace_back();
   Note &N = D.Notes.back();

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

)cpp");
   TU.AdditionalFiles["module.h"] = R"cpp(
     typedef int foo;
 )cpp";
   TU.AdditionalFiles["m.modulemap"] = R"modulemap(
     module M {
       header "module.h"
     }
 )modulemap";
-  EXPECT_TRUE(TU.build().getDiagnostics().empty());
+  EXPECT_TRUE(TU.build().getDiagnostics()->empty());
 }
 
 TEST(Modules, Diagnostic) {
   // Produce a diagnostic while building an implicit module. Use
   // -fmodules-strict-decluse, but any non-silenced diagnostic will do.
   TestTU TU = TestTU::withCode(R"cpp(
     /*error-ok*/
     #include "modular.h"

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

 //===-- ParsedASTTests.cpp ------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // These tests cover clangd's logic to build a TU, which generally uses the APIs
TEST(ParsedASTTest, ReplayPreambleForTidyCheckers) {
   for (llvm::StringLiteral Case : Cases) {
     TU.Code = Case.str();
 
     IgnoreDiagnostics Diags;
     auto CI = buildCompilerInvocation(TU.inputs(FS), Diags);
     auto PatchedAST = ParsedAST::build(testPath(TU.Filename), TU.inputs(FS),
                                        std::move(CI), {}, BaselinePreamble);
     ASSERT_TRUE(PatchedAST);
-    EXPECT_TRUE(PatchedAST->getDiagnostics().empty());
+    EXPECT_FALSE(PatchedAST->getDiagnostics());
   }
 
   // Then ensure correctness by making sure includes were seen only once.
   // Note that we first see the includes from the patch, as preamble includes
   // are replayed after exiting the built-in file.
   Includes.clear();
   TU.Code = R"cpp(
     #include "a.h"
     #include "b.h")cpp";
   IgnoreDiagnostics Diags;
   auto CI = buildCompilerInvocation(TU.inputs(FS), Diags);
   auto PatchedAST = ParsedAST::build(testPath(TU.Filename), TU.inputs(FS),
                                      std::move(CI), {}, BaselinePreamble);
   ASSERT_TRUE(PatchedAST);
-  EXPECT_TRUE(PatchedAST->getDiagnostics().empty());
+  EXPECT_FALSE(PatchedAST->getDiagnostics());
   EXPECT_THAT(Includes,
               ElementsAre(WithFileName(testPath("__preamble_patch__.h")),
                           WithFileName("b.h"), WithFileName("a.h")));
 }
 
 TEST(ParsedASTTest, PatchesAdditionalIncludes) {
   llvm::StringLiteral ModifiedContents = R"cpp(
     #include "baz.h"
TEST(ParsedASTTest, PatchesAdditionalIncludes) {
   EXPECT_THAT(EmptyPreamble->Includes.MainFileIncludes, testing::IsEmpty());
 
   // Now build an AST using empty preamble and ensure patched includes worked.
   TU.Code = ModifiedContents.str();
   Inputs = TU.inputs(FS);
   auto PatchedAST = ParsedAST::build(testPath("foo.cpp"), Inputs, std::move(CI),
                                      {}, EmptyPreamble);
   ASSERT_TRUE(PatchedAST);
-  ASSERT_TRUE(PatchedAST->getDiagnostics().empty());
+  ASSERT_FALSE(PatchedAST->getDiagnostics());
 
   // 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) {
     std::vector<std::pair<std::string, unsigned>> Res;
     for (const auto &E : SM)

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

)cpp",
   for (const auto &Case : Cases) {
     SCOPED_TRACE(Case.Contents);
     Annotations Modified(Case.Contents);
     EXPECT_THAT(getPreamblePatch("", Modified.code()),
                 MatchesRegex(Case.ExpectedPatch));
 
     auto AST = createPatchedAST("", Modified.code());
     ASSERT_TRUE(AST);
-    EXPECT_THAT(AST->getDiagnostics(),
-                Not(Contains(Field(&Diag::Range, Modified.range()))));
+    std::vector<Range> MacroRefRanges;
+    for (auto &M : AST->getMacros().MacroRefs) {
+      for (auto &O : M.getSecond())
+        MacroRefRanges.push_back(O.Rng);
+    }
+    EXPECT_THAT(MacroRefRanges, Contains(Modified.range()));
   }
 }
 
 TEST(PreamblePatchTest, OrderingPreserved) {
   llvm::StringLiteral Baseline = "#define BAR(X) X";
   Annotations Modified(R"cpp(
     #define BAR(X, Y) X Y
     #define BAR(X) X
     [[BAR]](int y);
   )cpp");
 
   llvm::StringLiteral ExpectedPatch(R"cpp(#line 0 ".*main.cpp"
 #line 2
 #define     BAR\(X, Y\) X Y
 #line 3
 #define     BAR\(X\) X
 )cpp");
   EXPECT_THAT(getPreamblePatch(Baseline, Modified.code()),
               MatchesRegex(ExpectedPatch.str()));
 
   auto AST = createPatchedAST(Baseline, Modified.code());
   ASSERT_TRUE(AST);
-  EXPECT_THAT(AST->getDiagnostics(),
-              Not(Contains(Field(&Diag::Range, Modified.range()))));
 }
 
 TEST(PreamblePatchTest, LocateMacroAtWorks) {
   struct {
     const char *const Baseline;
     const char *const Modified;
   } Cases[] = {
       // Addition of new directive
for (const auto &Case : Cases) {
 
     const auto ExpectedBounds =
         Lexer::ComputePreamble(Case.Modified, *CI->getLangOpts());
     EXPECT_EQ(PP.modifiedBounds().Size, ExpectedBounds.Size);
     EXPECT_EQ(PP.modifiedBounds().PreambleEndsAtStartOfLine,
               ExpectedBounds.PreambleEndsAtStartOfLine);
   }
 }
+
+TEST(PreamblePatch, DropsDiagnostics) {
+  llvm::StringLiteral Code = "#define FOO\nx;/* error-ok */";
+  // First check that this code generates diagnostics.
+  EXPECT_THAT(*TestTU::withCode(Code).build().getDiagnostics(),
+              testing::Not(testing::IsEmpty()));
+  // Ensure they are dropeed when a patched preamble is used.
+  EXPECT_FALSE(createPatchedAST("", Code)->getDiagnostics());
+}
 } // namespace
 } // namespace clangd
 } // namespace clang

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

 #include "Expand.inc"
         br^eak;
     }
   )cpp";
   Annotations Test(Case);
   auto TU = TestTU::withCode(Test.code());
   TU.AdditionalFiles["Expand.inc"] = "MACRO\n";
   auto AST = TU.build();
-  EXPECT_THAT(AST.getDiagnostics(), ::testing::IsEmpty());
+  EXPECT_THAT(*AST.getDiagnostics(), ::testing::IsEmpty());
   auto T = makeSelectionTree(Case, AST);
 
   EXPECT_EQ("BreakStmt", T.commonAncestor()->kind());
   EXPECT_EQ("WhileStmt", T.commonAncestor()->Parent->kind());
 }
 
 TEST(SelectionTest, IncludedFile) {
   const char *Case = R"cpp(

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

static Key<llvm::unique_function<void(PathRef File, std::vector<Diag>)>>
       DiagsCallbackKey;
 
   /// A diagnostics callback that should be passed to TUScheduler when it's used
   /// in updateWithDiags.
   static std::unique_ptr<ParsingCallbacks> captureDiags() {
     class CaptureDiags : public ParsingCallbacks {
     public:
       void onMainAST(PathRef File, ParsedAST &AST, PublishFn Publish) override {
-        reportDiagnostics(File, AST.getDiagnostics(), Publish);
+        reportDiagnostics(File, *AST.getDiagnostics(), Publish);
       }
 
       void onFailedAST(PathRef File, llvm::StringRef Version,
                        std::vector<Diag> Diags, PublishFn Publish) override {
         reportDiagnostics(File, Diags, Publish);
       }
 
     private:

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

struct TestTU {
   // in memory across multiple clang invocations, at least in tests, to
   // eliminate the need for real file system here.
   // Please avoid using this for things other than implicit modules. The plan is
   // to eliminate this option some day.
   bool OverlayRealFileSystemForModules = false;
 
   // By default, build() will report Error diagnostics as GTest errors.
   // Suppress this behavior by adding an 'error-ok' comment to the code.
+  // The result will always have getDiagnostics() populated.
   ParsedAST build() const;
   std::shared_ptr<const PreambleData>
   preamble(PreambleParsedCallback PreambleCallback = nullptr) const;
   ParseInputs inputs(MockFS &FS) const;
   SymbolSlab headerSymbols() const;
   RefSlab headerRefs() const;
   std::unique_ptr<SymbolIndex> index() const;
 };

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

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);
   if (!AST.hasValue()) {
     ADD_FAILURE() << "Failed to build code:\n" << Code;
     llvm_unreachable("Failed to build TestTU!");
   }
+  if (!AST->getDiagnostics()) {
+    ADD_FAILURE() << "TestTU should always build an AST with a fresh Preamble"
+                  << Code;
+    return std::move(*AST);
+  }
   // 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.
   bool ErrorOk = [&, this] {
     llvm::StringLiteral Marker = "error-ok";
     if (llvm::StringRef(Code).contains(Marker) ||
         llvm::StringRef(HeaderCode).contains(Marker))
       return true;
     for (const auto &KV : this->AdditionalFiles)
       if (llvm::StringRef(KV.second).contains(Marker))
         return true;
     return false;
   }();
   if (!ErrorOk) {
-    for (const auto &D : AST->getDiagnostics())
+    // We always build AST with a fresh preamble in TestTU.
+    for (const auto &D : *AST->getDiagnostics())
       if (D.Severity >= DiagnosticsEngine::Error) {
         ADD_FAILURE()
             << "TestTU failed to build (suppress with /*error-ok*/): \n"
             << D << "\n\nFor code:\n"
             << Code;
         break; // Just report first error for simplicity.
       }
   }

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

TEST(TypeHierarchy, RecursiveHierarchyUnbounded) {
   )cpp");
 
   TestTU TU = TestTU::withCode(Source.code());
   TU.ExtraArgs.push_back("-ftemplate-depth=10");
   auto AST = TU.build();
 
   // The compiler should produce a diagnostic for hitting the
   // template instantiation depth.
-  ASSERT_TRUE(!AST.getDiagnostics().empty());
+  ASSERT_TRUE(!AST.getDiagnostics()->empty());
 
   // Make sure getTypeHierarchy() doesn't get into an infinite recursion.
   // The parent is reported as "S" because "S<0>" is an invalid instantiation.
   // We then iterate once more and find "S" again before detecting the
   // recursion.
   llvm::Optional<TypeHierarchyItem> Result = getTypeHierarchy(
       AST, Source.points()[0], 0, TypeHierarchyDirection::Parents);
   ASSERT_TRUE(bool(Result));