[clangd] Add config option for fast diagnostics mode

[sammccall]

clang-tools-extra/clangd/ConfigFragment.h

243

I think "Diagnostics.Mode" is too vague a name.

I expect this to be a rollout flag that we remove in the medium term (either deciding to stick to the old or new behavior), so maybe something ultra-specific like`AllowStalePreamble: yes/no`?
(Preamble is jargon, maybe there's something better, but again I don't think we should plan for this to be really "user-visible")

[kadircet]

Wire up the feature and add some test cases

[kadircet]

clang-tools-extra/clangd/ConfigFragment.h

243

i actually feel like there's some value in keeping this around. some users value correctness of their diagnostics too much, and option isn't really costly to implement. why do you think we should stick to one or the other?

[sammccall]

clang-tools-extra/clangd/ConfigFragment.h

243

some users value correctness of their diagnostics too much

First, citation needed! (People *claiming* that they value correctness when they actually prefer latency seems common based on our past optimizations).

Second, this is a fine-grained knob that probably doesn't yield a useful correctness/latency tradeoff. If it trades off a lot of correctness, we probably want to leave it off by default and at that point we should delete the feature. If it trades off only a little correctness, then most people will want it and *disabling it won't significantly improve diagnostics correctness* as most errors come from non-configurable tradeoffs made elsewhere.

There's a sweet spot where this trades off just the critical amount of correctness to make it a difficult call, I just think it's unlikely we'll hit that. (I'm optimistic about this being mostly unnoticeable).

---

Regardless, if this is a long-lived option, it's more important to have a good name. AllowStalePreamble seems clearly better than Mode but maybe we could come up with something better - I just don't think it matters that much if the option is short-lived.

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

555

comment doesn't seem to apply here (and below)

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

608

hmm, this convert-then-assert-eq is harder to debug when there are unexpected diagnostics than matchers, I think. Might be better to follow what DiagnosticTests does here?

652

nit: embedded newlines are hard to read and inconsistent with other tests. rawstrings?

658

it's unclear what the behavior is here: do we get a diagnostic with no range, or a bad range that's being filtered out, or no diagnostic at all?

(The presence/absence of the diagnostics is important to test, independent of the locations)

666

assert the behavior instead of describing it in a comment?

[kadircet]

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

555

oops, copy pasting is hard. just dropped them.

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

608

not sure what you mean by that. when things go wrong this prints the expected and actual ranges, which is pretty much all we care about from this logic's perspective.

658

well, sent out D143197. we were not generating any diagnositcs at all, because we couldn't figure out the headerid for patched include. i guess these improvements/fixes to the preamble patching deserves its own patch, whether or not we chose to improve things.

666

done. this test was checking so many things at once. made it simpler by getting rid of the macro def from the preamble and checking for it (the #undef issue) only in the next test.

[kadircet]

• Address comments

[kadircet]

• Update tests after discussions in D143096 to be line-oriented, rather than being directive-oriented.

[sammccall]

The code looks good, but I have a very hard time following the tests.

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

228

nit: seems clearer at this point to clone the TU and overwrite Code

608

I care about the diagnostic messages/codes too, otherwise I can't understand what the assertions are about (especially on failure, but even just reading the code I can't tell what is being tested)

627

nit: "preamble" vs "code" is a confusing distinction when we're using both as code.
Spelling out the inputs would be IMO clearer than pasting strings together, tests don't need to be DRY

658

what exactly is being tested here?
"removals" suggests we're dropping includes but think we're not.
Is the comment change significant, or the whitespace change in the directive, or both?
The code looks OK, what's the diagnostic being emitted?

659

raw strings

667

raw strings

676

raw strings

680

what is the warning for BARXYZ?

This comment says the diagnostic shouldn't be pointing at FOO inside baselinepreamble (which I assume means the first FOO) but it already isn't - did you mean "should"? This is probably just two FIXMEs.

683

named ranges?

698

nit: at the correct coordinates in NewCode? (the diagnostic already points at NewCode)

[kadircet]

• Use raw string literals
• Make tests more expressive by mentioning diagnostic names

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

627

Spelling out the inputs would be IMO clearer than pasting strings together, tests don't need to be DRY

Done.

658

what exactly is being tested here?

so this test is all about diagnostics that are emitted as part of the main file AST built, specifically the ones that are emitted on ranges inside the preamble.
the particular diagnostics on include directives are coming from include-cleaner.

"removals" suggests we're dropping includes but think we're not.

idea is checking that translation logic works as intended when the preamble bounds got smaller.

Is the comment change significant, or the whitespace change in the directive, or both?

comment change is insignificant, it's just to make sure we are making preamble bounds smaller, could as well be another include.
whitespace change is insignificant since we're moving the whole directive around (2nd line to 0th line). it was to make it more explicit. happy to hide if you think that's confusing.

The code looks OK, what's the diagnostic being emitted?

unused-include diag as mentioned above. couldn't really find any diagnostics that are emitted (or contain notes that are pointing) to an include directive.

680

what is the warning for BARXYZ?

redefinition of FOO, it's emitted at location of B then we turn it into a range inside clangd by lexing the token. hence it's expanded into the full token range.

This comment says the diagnostic shouldn't be pointing at FOO inside baselinepreamble (which I assume means the first FOO) but it already isn't - did you mean "should"? This is probably just two FIXMEs.

sorry that's my bad english. i was trying to say it shouldn't point at BARXYZ and should point at FOO instead.

[kadircet]

• Insert missing include

[sammccall]

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

227–228

now there's only one variable used for both - just rename it instead?

[vitalybuka]

One of your patches likely introduced UB https://lab.llvm.org/buildbot/#/builders/85/builds/14558
Can you please take a look?

FYI, @kstoimenov

[kadircet]

In D142890#4149181, @vitalybuka wrote:

One of your patches likely introduced UB https://lab.llvm.org/buildbot/#/builders/85/builds/14558
Can you please take a look?

Thanks a lot Vitaly and sorry for not noticing this myself :(
This sounds to me like a bad consequence between an empty stringref and memorybuffers. I've put together https://reviews.llvm.org/D144706 to make sure we can prevent this from happening in the library, but getting it reviewed will probably take time.
In the meanwhile I've sent out https://reviews.llvm.org/D144708 to stop the bleeding.

clang-tools-extra/clangd/Config.h

struct Config {
   /// Controls index behavior.
   struct {
     /// Whether this TU should be background-indexed.
     BackgroundPolicy Background = BackgroundPolicy::Build;
     ExternalIndexSpec External;
     bool StandardLibrary = true;
   } Index;
 
-  enum UnusedIncludesPolicy {
+  enum class UnusedIncludesPolicy {
     /// Diagnose unused includes.
     Strict,
     None,
     /// The same as Strict, but using the include-cleaner library.
     Experiment,
   };
   /// Controls warnings and errors when parsing code.
   struct {
     bool SuppressAll = false;
     llvm::StringSet<> Suppress;
 
     /// Configures what clang-tidy checks to run and options to use with them.
     struct {
       // A comma-seperated list of globs specify which clang-tidy checks to run.
       std::string Checks;
       llvm::StringMap<std::string> CheckOptions;
     } ClangTidy;
 
-    UnusedIncludesPolicy UnusedIncludes = None;
+    UnusedIncludesPolicy UnusedIncludes = UnusedIncludesPolicy::None;
+
+    /// Enable emitting diagnostics using stale preambles.
+    bool AllowStalePreamble = false;
 
     /// IncludeCleaner will not diagnose usages of these headers matched by
     /// these regexes.
     struct {
       std::vector<std::function<bool(llvm::StringRef)>> IgnoreHeader;
     } Includes;
   } Diagnostics;
 

clang-tools-extra/clangd/ConfigCompile.cpp

if (F.UnusedIncludes)
                                                         **F.UnusedIncludes)
                   .map("Strict", Config::UnusedIncludesPolicy::Strict)
                   .map("Experiment", Config::UnusedIncludesPolicy::Experiment)
                   .map("None", Config::UnusedIncludesPolicy::None)
                   .value())
         Out.Apply.push_back([Val](const Params &, Config &C) {
           C.Diagnostics.UnusedIncludes = *Val;
         });
-    compile(std::move(F.Includes));
+    if (F.AllowStalePreamble) {
+      if (auto Val = F.AllowStalePreamble)
+        Out.Apply.push_back([Val](const Params &, Config &C) {
+          C.Diagnostics.AllowStalePreamble = **Val;
+        });
+    }
 
+    compile(std::move(F.Includes));
     compile(std::move(F.ClangTidy));
   }
 
   void compile(Fragment::StyleBlock &&F) {
     if (!F.FullyQualifiedNamespaces.empty()) {
       std::vector<std::string> FullyQualifiedNamespaces;
       for (auto &N : F.FullyQualifiedNamespaces) {
         // Normalize the data by dropping both leading and trailing ::

clang-tools-extra/clangd/ConfigFragment.h

struct DiagnosticsBlock {
     //
     /// Strict means a header is unused if it does not *directly* provide any
     /// symbol used in the file. Removing it may still break compilation if it
     /// transitively includes headers that are used. This should be fixed by
     /// including those headers directly.
     ///
     /// Valid values are:
     /// - Strict
+    /// - Experiment
     /// - None
     std::optional<Located<std::string>> UnusedIncludes;
 
+    /// Enable emitting diagnostics using stale preambles.
+    std::optional<Located<bool>> AllowStalePreamble;
+
     /// Controls IncludeCleaner diagnostics.
     struct IncludesBlock {
       /// Regexes that will be used to avoid diagnosing certain includes as
       /// unused or missing. These can match any suffix of the header file in
       /// question.
       std::vector<Located<std::string>> IgnoreHeader;
     };
     IncludesBlock Includes;
 
     /// Controls how clang-tidy will run over the code base.

clang-tools-extra/clangd/ConfigYAML.cpp

Dict.handle("Suppress", [&](Node &N) {
       if (auto Values = scalarValues(N))
         F.Suppress = std::move(*Values);
     });
     Dict.handle("UnusedIncludes", [&](Node &N) {
       F.UnusedIncludes = scalarValue(N, "UnusedIncludes");
     });
     Dict.handle("Includes", [&](Node &N) { parse(F.Includes, N); });
     Dict.handle("ClangTidy", [&](Node &N) { parse(F.ClangTidy, N); });
+    Dict.handle("AllowStalePreamble", [&](Node &N) {
+      F.AllowStalePreamble = boolValue(N, "AllowStalePreamble");
+    });
     Dict.parse(N);
   }
 
   void parse(Fragment::DiagnosticsBlock::ClangTidyBlock &F, Node &N) {
     DictParser Dict("ClangTidy", this);
     Dict.handle("Add", [&](Node &N) {
       if (auto Values = scalarValues(N))
         F.Add = std::move(*Values);
public:
     DictParser(llvm::StringRef Description, Parser *Outer)
         : Description(Description), Outer(Outer) {}
 
     // Parse is called when Key is encountered, and passed the associated value.
     // It should emit diagnostics if the value is invalid (e.g. wrong type).
     // If Key is seen twice, Parse runs only once and an error is reported.
     void handle(llvm::StringLiteral Key, std::function<void(Node &)> Parse) {
       for (const auto &Entry : Keys) {
-        (void) Entry;
+        (void)Entry;
         assert(Entry.first != Key && "duplicate key handler");
       }
       Keys.emplace_back(Key, std::move(Parse));
     }
 
     // Handler is called when a Key is not matched by any handle().
     // If this is unset or the Handler returns true, a warning is emitted for
     // the unknown key.

clang-tools-extra/clangd/Preamble.h

public:
 
   /// 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(); }
+  bool preserveDiagnostics() const;
 
 private:
   static PreamblePatch create(llvm::StringRef FileName,
                               const ParseInputs &Modified,
                               const PreambleData &Baseline,
                               PatchType PatchType);
 
   PreamblePatch() = default;

clang-tools-extra/clangd/Preamble.cpp

if (IncludeLoc.isValid() && SM.isWrittenInBuiltinFile(IncludeLoc) &&
         Loc = SM.translateLineCol(SM.getMainFileID(), Presumed.getLine(),
                                   Presumed.getColumn());
       }
     }
   }
   return Loc;
 }
 
+bool PreamblePatch::preserveDiagnostics() const {
+  return PatchContents.empty() ||
+         Config::current().Diagnostics.AllowStalePreamble;
+}
 } // namespace clangd
 } // namespace clang

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

 
 TEST_F(ConfigCompileTests, Style) {
   Frag = {};
   Frag.Style.FullyQualifiedNamespaces.push_back(std::string("foo"));
   Frag.Style.FullyQualifiedNamespaces.push_back(std::string("bar"));
   EXPECT_TRUE(compileAndApply());
   EXPECT_THAT(Conf.Style.FullyQualifiedNamespaces, ElementsAre("foo", "bar"));
 }
+
+TEST_F(ConfigCompileTests, AllowDiagsFromStalePreamble) {
+  Frag = {};
+  EXPECT_TRUE(compileAndApply());
+  // Off by default.
+  EXPECT_EQ(Conf.Diagnostics.AllowStalePreamble, false);
+
+  Frag.Diagnostics.AllowStalePreamble.emplace(true);
+  EXPECT_TRUE(compileAndApply());
+  EXPECT_EQ(Conf.Diagnostics.AllowStalePreamble, true);
+
+  Frag.Diagnostics.AllowStalePreamble.emplace(false);
+  EXPECT_TRUE(compileAndApply());
+  EXPECT_EQ(Conf.Diagnostics.AllowStalePreamble, false);
+}
 } // namespace
 } // namespace config
 } // namespace clangd
 } // namespace clang

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

Style:
   FullyQualifiedNamespaces: [foo, bar])yaml");
   auto Results =
       Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
   ASSERT_THAT(Diags.Diagnostics, IsEmpty());
   ASSERT_EQ(Results.size(), 1u);
   EXPECT_THAT(Results[0].Style.FullyQualifiedNamespaces,
               ElementsAre(val("foo"), val("bar")));
 }
+
+TEST(ParseYAML, DiagnosticsMode) {
+  CapturedDiags Diags;
+  {
+    Annotations YAML(R"yaml(
+Diagnostics:
+  AllowStalePreamble: Yes)yaml");
+    auto Results =
+        Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
+    ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+    ASSERT_EQ(Results.size(), 1u);
+    EXPECT_THAT(Results[0].Diagnostics.AllowStalePreamble,
+                llvm::ValueIs(val(true)));
+  }
+
+  {
+    Annotations YAML(R"yaml(
+Diagnostics:
+  AllowStalePreamble: No)yaml");
+    auto Results =
+        Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
+    ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+    ASSERT_EQ(Results.size(), 1u);
+    EXPECT_THAT(Results[0].Diagnostics.AllowStalePreamble,
+                llvm::ValueIs(val(false)));
+  }
+}
 } // namespace
 } // namespace config
 } // namespace clangd
 } // namespace clang

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

TEST(IncludeCleaner, IWYUPragmas) {
   TU.AdditionalFiles["behind_keep.h"] = guard("");
   TU.AdditionalFiles["exported.h"] = guard("");
   TU.AdditionalFiles["public.h"] = guard("#include \"private.h\"");
   TU.AdditionalFiles["private.h"] = guard(R"cpp(
     // IWYU pragma: private, include "public.h"
     void foo() {}
   )cpp");
   Config Cfg;
-  Cfg.Diagnostics.UnusedIncludes = Config::Experiment;
+  Cfg.Diagnostics.UnusedIncludes = Config::UnusedIncludesPolicy::Experiment;
   WithContextValue Ctx(Config::Key, std::move(Cfg));
   ParsedAST AST = TU.build();
 
   auto ReferencedFiles = findReferencedFiles(
       findReferencedLocations(AST), AST.getIncludeStructure(),
       AST.getCanonicalIncludes(), AST.getSourceManager());
   EXPECT_EQ(ReferencedFiles.SpelledUmbrellas.size(), 1u);
   EXPECT_EQ(ReferencedFiles.SpelledUmbrellas.begin()->getKey(), "\"public.h\"");
TU.AdditionalFiles["foo.h"] = R"cpp(
     #ifndef FOO_H
     #define FOO_H
 
     #endif
   )cpp";
   TU.ExtraArgs.emplace_back("-xobjective-c");
 
   Config Cfg;
-  Cfg.Diagnostics.UnusedIncludes = Config::Strict;
+  Cfg.Diagnostics.UnusedIncludes = Config::UnusedIncludesPolicy::Strict;
   WithContextValue Ctx(Config::Key, std::move(Cfg));
   ParsedAST AST = TU.build();
   EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
 }
 } // namespace
 } // namespace clangd
 } // namespace clang

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

 //===--- PreambleTests.cpp --------------------------------------*- 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 "Annotations.h"
 #include "Compiler.h"
+#include "Config.h"
+#include "Diagnostics.h"
 #include "Headers.h"
 #include "Hover.h"
+#include "ParsedAST.h"
 #include "Preamble.h"
+#include "Protocol.h"
 #include "SourceCode.h"
 #include "TestFS.h"
 #include "TestTU.h"
 #include "XRefs.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Format/Format.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Frontend/PrecompiledPreamble.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/VirtualFileSystem.h"
+#include "llvm/Testing/Support/SupportHelpers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest-matchers.h"
 #include "gtest/gtest.h"
 #include <memory>
 #include <optional>
 #include <string>
 #include <vector>
 
+using testing::AllOf;
 using testing::Contains;
+using testing::ElementsAre;
+using testing::Eq;
 using testing::Field;
+using testing::HasSubstr;
+using testing::IsEmpty;
 using testing::Matcher;
 using testing::MatchesRegex;
+using testing::Not;
+using testing::UnorderedElementsAre;
+using testing::UnorderedElementsAreArray;
 
 namespace clang {
 namespace clangd {
 namespace {
 
 MATCHER_P2(Distance, File, D, "") {
   return arg.first() == File && arg.second == D;
 }
EXPECT_THAT(
       ElementsAre(AllOf(
           Field(&Inclusion::Written, "\"a.h\""),
           Field(&Inclusion::Resolved, testPath("a.h")),
           Field(&Inclusion::HeaderID, testing::Not(testing::Eq(std::nullopt))),
           Field(&Inclusion::BehindPragmaKeep, true),
           Field(&Inclusion::FileKind, SrcMgr::CharacteristicKind::C_User))));
 }
 
-std::optional<ParsedAST> createPatchedAST(llvm::StringRef Baseline,
-                                          llvm::StringRef Modified) {
-  auto BaselinePreamble = TestTU::withCode(Baseline).preamble();
+std::optional<ParsedAST>
+createPatchedAST(llvm::StringRef Baseline, llvm::StringRef Modified,
+                 llvm::StringMap<std::string> AdditionalFiles = {}) {
+  auto PreambleTU = TestTU::withCode(Baseline);
+  PreambleTU.AdditionalFiles = std::move(AdditionalFiles);
+  auto BaselinePreamble = PreambleTU.preamble();
   if (!BaselinePreamble) {
     ADD_FAILURE() << "Failed to build baseline preamble";
     return std::nullopt;
   }
 
   IgnoreDiagnostics Diags;
   MockFS FS;
-  auto TU = TestTU::withCode(Modified);
+  auto &TU = PreambleTU;
+  TU.Code = Modified.str();
   auto CI = buildCompilerInvocation(TU.inputs(FS), Diags);
   if (!CI) {
     ADD_FAILURE() << "Failed to build compiler invocation";
     return std::nullopt;
   }
   return ParsedAST::build(testPath(TU.Filename), TU.inputs(FS), std::move(CI),
                           {}, BaselinePreamble);
 }
TEST(PreamblePatch, NoopWhenNotRequested) {
   ASSERT_TRUE(BaselinePreamble);
 
   TU.Code = Modified.str();
   MockFS FS;
   auto PP = PreamblePatch::createMacroPatch(testPath(TU.Filename),
                                             TU.inputs(FS), *BaselinePreamble);
   EXPECT_TRUE(PP.text().empty());
 }
+
+::testing::Matcher<const Diag &>
+withNote(::testing::Matcher<Note> NoteMatcher) {
+  return Field(&Diag::Notes, ElementsAre(NoteMatcher));
+}
+MATCHER_P(Diag, Range, "Diag at " + llvm::to_string(Range)) {
+  return arg.Range == Range;
+}
+MATCHER_P2(Diag, Range, Name,
+           "Diag at " + llvm::to_string(Range) + " = [" + Name + "]") {
+  return arg.Range == Range && arg.Name == Name;
+}
+
+TEST(PreamblePatch, DiagnosticsFromMainASTAreInRightPlace) {
+  Config Cfg;
+  Cfg.Diagnostics.AllowStalePreamble = true;
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+
+  {
+    Annotations Code("#define FOO");
+    // Check with removals from preamble.
+    Annotations NewCode("[[x]];/* error-ok */");
+    auto AST = createPatchedAST(Code.code(), NewCode.code());
+    EXPECT_THAT(*AST->getDiagnostics(),
+                ElementsAre(Diag(NewCode.range(), "missing_type_specifier")));
+  }
+  {
+    // Check with additions to preamble.
+    Annotations Code("#define FOO");
+    Annotations NewCode(R"(
+#define FOO
+#define BAR
+[[x]];/* error-ok */)");
+    auto AST = createPatchedAST(Code.code(), NewCode.code());
+    EXPECT_THAT(*AST->getDiagnostics(),
+                ElementsAre(Diag(NewCode.range(), "missing_type_specifier")));
+  }
+}
+
+TEST(PreamblePatch, DiagnosticsToPreamble) {
+  Config Cfg;
+  Cfg.Diagnostics.AllowStalePreamble = true;
+  Cfg.Diagnostics.UnusedIncludes = Config::UnusedIncludesPolicy::Strict;
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+
+  llvm::StringMap<std::string> AdditionalFiles;
+  AdditionalFiles["foo.h"] = "#pragma once";
+  AdditionalFiles["bar.h"] = "#pragma once";
+  {
+    Annotations Code(R"(
+// Test comment
+[[#include "foo.h"]])");
+    // Check with removals from preamble.
+    Annotations NewCode(R"([[#  include "foo.h"]])");
+    auto AST = createPatchedAST(Code.code(), NewCode.code(), AdditionalFiles);
+    EXPECT_THAT(*AST->getDiagnostics(),
+                ElementsAre(Diag(NewCode.range(), "unused-includes")));
+  }
+  {
+    // Check with additions to preamble.
+    Annotations Code(R"(
+// Test comment
+[[#include "foo.h"]])");
+    Annotations NewCode(R"(
+$bar[[#include "bar.h"]]
+// Test comment
+$foo[[#include "foo.h"]])");
+    auto AST = createPatchedAST(Code.code(), NewCode.code(), AdditionalFiles);
+    EXPECT_THAT(
+        *AST->getDiagnostics(),
+        UnorderedElementsAre(Diag(NewCode.range("bar"), "unused-includes"),
+                             Diag(NewCode.range("foo"), "unused-includes")));
+  }
+  {
+    Annotations Code("#define [[FOO]] 1\n");
+    // Check ranges for notes.
+    Annotations NewCode(R"(#define $barxyz[[BARXYZ]] 1
+#define $foo1[[FOO]] 1
+void foo();
+#define $foo2[[FOO]] 2)");
+    auto AST = createPatchedAST(Code.code(), NewCode.code(), AdditionalFiles);
+    EXPECT_THAT(
+        *AST->getDiagnostics(),
+        ElementsAre(
+            // FIXME: This diagnostics shouldn't exist. It's emitted from the
+            // preamble patch to the stale location inside preamble.
+            AllOf(Field(&Diag::Name, Eq("-Wmacro-redefined")),
+                  Field(&Diag::File, HasSubstr("_preamble_patch_")),
+                  withNote(Diag(NewCode.range("barxyz")))),
+            AllOf(
+                Diag(NewCode.range("foo2"), "-Wmacro-redefined"),
+                // FIXME: This should be translated into main file.
+                withNote(Field(&Note::File, HasSubstr("_preamble_patch_"))))));
+  }
+}
+
+TEST(PreamblePatch, TranslatesDiagnosticsInPreamble) {
+  Config Cfg;
+  Cfg.Diagnostics.AllowStalePreamble = true;
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+
+  {
+    // Check with additions to preamble.
+    Annotations Code("#include [[<foo>]]");
+    Annotations NewCode(R"(
+#define BAR
+#include [[<foo>]])");
+    auto AST = createPatchedAST(Code.code(), NewCode.code());
+    // FIXME: We should point at the correct coordinates in NewCode.
+    EXPECT_THAT(*AST->getDiagnostics(),
+                ElementsAre(Diag(Code.range(), "pp_file_not_found")));
+  }
+  {
+    // Check with removals from preamble.
+    Annotations Code(R"(
+#define BAR
+#include [[<foo>]])");
+    Annotations NewCode("#include [[<foo>]]");
+    auto AST = createPatchedAST(Code.code(), NewCode.code());
+    // FIXME: We should point at the correct coordinates in NewCode.
+    EXPECT_THAT(*AST->getDiagnostics(),
+                ElementsAre(Diag(Code.range(), "pp_file_not_found")));
+  }
+  {
+    // Drop line with diags.
+    Annotations Code("#include [[<foo>]]");
+    Annotations NewCode("#define BAR\n#define BAZ\n");
+    auto AST = createPatchedAST(Code.code(), NewCode.code());
+    // FIXME: No diagnostics.
+    EXPECT_THAT(*AST->getDiagnostics(),
+                ElementsAre(Diag(Code.range(), "pp_file_not_found")));
+  }
+  {
+    // Picks closest line in case of multiple alternatives.
+    Annotations Code("#include [[<foo>]]");
+    Annotations NewCode(R"(
+#define BAR
+#include [[<foo>]]
+#define BAR
+#include <foo>)");
+    auto AST = createPatchedAST(Code.code(), NewCode.code());
+    // FIXME: We should point at the correct coordinates in NewCode.
+    EXPECT_THAT(*AST->getDiagnostics(),
+                ElementsAre(Diag(Code.range(), "pp_file_not_found")));
+  }
+  {
+    // Drop diag if line spelling has changed.
+    Annotations Code("#include [[<foo>]]");
+    Annotations NewCode(" # include <foo>");
+    auto AST = createPatchedAST(Code.code(), NewCode.code());
+    // FIXME: No diags.
+    EXPECT_THAT(*AST->getDiagnostics(),
+                ElementsAre(Diag(Code.range(), "pp_file_not_found")));
+  }
+}
 } // namespace
 } // namespace clangd
 } // namespace clang