[clangd] Provide patched diagnostics with preamble patch

[kadircet]

Summary

Translates diagnostics from baseline preamble to relevant modified
contents.

Translation is done by looking for a set of lines that have the same
contents in diagnostic/note/fix ranges inside baseline and modified
contents.

A diagnostic is preserved if its main range is outside of main file or
there's a translation from baseline to modified contents. Later on fixes
and notes attached to that diagnostic with relevant ranges are also
translated and preserved.

Depends on D143095

[sammccall]

It looks like this fixes up the location only of diagnostics attached to particular directives (#include) based on some "deep" idea about the content of the directive (the spelled header name).

Some shortcomings:

• This misses diagnostics attached to other directives / continued lines in macro definitions / etc
• it allows diagnostics to be translated across lines even if the directive spelling changed (in which case the ranges are incorrect since only line numbers are updated.)
• it requires modelling the directive content in some way that needs to be extended if we find another place that diagnostics can be attached

We discussed the idea of attaching to the text of the line, which seems pretty generic. Needs some handling of duplicate line content but seems like something pretty naive (closest line number with matching content?) would work well, be just as simple and more generic. Any reason this alternative was rejected?

clang-tools-extra/clangd/Preamble.h

163

this looks like a gratuitous copy - ParsedAST copies again

[kadircet]

In D143096#4099662, @sammccall wrote:

It looks like this fixes up the location only of diagnostics attached to particular directives (#include) based on some "deep" idea about the content of the directive (the spelled header name).

Some shortcomings:

• This misses diagnostics attached to other directives / continued lines in macro definitions / etc
• it allows diagnostics to be translated across lines even if the directive spelling changed (in which case the ranges are incorrect since only line numbers are updated.)
• it requires modelling the directive content in some way that needs to be extended if we find another place that diagnostics can be attached

We discussed the idea of attaching to the text of the line, which seems pretty generic. Needs some handling of duplicate line content but seems like something pretty naive (closest line number with matching content?) would work well, be just as simple and more generic. Any reason this alternative was rejected?

agreed this is better, also somewhat more elegant on the implementation side. switching to a line-content based translation, while keeping the logic around limiting diagnostics to single lines. as multiple lines seem more wonky and unclear how common they're.

[kadircet]

• Change to a line based translation logic

[sammccall]

clang-tools-extra/clangd/ParsedAST.cpp

679

llvm::append_range(Diags, Patch->patchedDiags()) ?

clang-tools-extra/clangd/Preamble.cpp

374

llvm::append_range(SP.Lines, llvm::split(Contents, "\n")) ?

711

nit: comment sense is the opposite of the function name

maybe say why this is an important property, or give the function a higher-level name?

712

this function + translateDiag look like a validate/parse pair that need to be kept in sync.

they could be a single translateDiag() function that could fail, instead.

717

this check looks dubious to me:

• File is a human-readable string with ill-defined format, comparing them for equality isn't safe, you want InsideMainFile instead
• if the note points to another file, then it's not that translation is impossible, but rather no translation is needed
• if the note points to the same file, then why can't we apply the same translation?

I think this should rather be something like:

bool translateDiag(DiagBase& D, bool IsMainDiag) {
  if (IsMainDiag) {
    for (const auto& N : cast<Diag>(D).Notes) {
      if (!translateDiag(N, /*IsMainDiag=*/false))
        return false;
    }
  }
  if (D.InsideMainFile) {
    // attempt patching range of D itself
  }
}

752

nit: name seems backwards (but as noted below I think this function can go away entirely)

777

If I'm understanding the algorithm right:

• we make a map of line content => original line number
• we iterate through each line of the modified preamble, and:
  • find the corresponding original line
  • look for diagnostics to translate

Why not loop over diagnostics instead of lines in the modified preamble? There should be a lot fewer, and you avoid the lines-to-diags map.

779

large preamble with no diagnostics seems pretty plausible, maybe bail out early to avoid building the content map?

791

I think this change and the IncludeKey class can be reverted now?

clang-tools-extra/clangd/Preamble.h

161

the stuff about #include/#define doesn't seem to correspond to anything in the code (obsolete?)
and below

175–176

if you want to undoxygen this comment, also remove \p?

[sammccall]

clang-tools-extra/clangd/Preamble.cpp

720

Oh yeah, one more thing :-)

Currently if the note/fix aren't at this line we're bailing out of the diagnostic entirely.

We have three behaviors available:

1. patch the note/fix range as best we can and emit it
2. drop the note/fix but keep the diagnostic
3. drop the diagnostic entirely

I think we should generally prefer 1 when we can, maybe there's a case for choosing 2 sometimes (heuristics about when diagnostics have probably been invalidated, with comments), and we should ~never do 3.

[kadircet]

• Change patching logic to iterate over diags instead of preamble lines
• Change translation logic to:
  • Preserve a diagnostic if its range can be translated.
  • Preserve all the notes whose range can be translated.
  • Preserve all the fixes whose edit ranges can be translated.
• Drop restrictions on ranges being a single line, make sure contents for all the lines are matched between modified and basline contents.
• Add couple new tests to demonstrate what's preserved.

[kadircet]

• Reflow comments

[kadircet]

• Update commit message

[sammccall]

clang-tools-extra/clangd/Preamble.cpp

713

I think this function is too long with too many local lambdas, consider converting it to a class instead (call me old-fashioned!)

733

we're assuming that the ranges are within the preamble and everyone agrees about the bounds. If not, BaselineStart may not be a valid index into BaselineScan.Lines

This assumption seems broadly reasonable, but is *very much* not locally enforced. I'd suggest a defensive check or at least an assert.

736

this doesn't look right: you're first deciding which possible starting point is closest, and then deciding whether it matches. So a range that matches can be masked by a range where only the first line matches, if the latter is closer.

771

probably doesn't matter, but you're inconsistent about moving vs copying range

794

reasoning about the fields you're *not* rewriting feels fragile. (Here and in TranslateFix).

Consider copying the whole object and mutating in place (bool TranslateDiag(Diag&) together with erase_if)

clang-tools-extra/clangd/Preamble.h

162

nit: blank line after

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

819

i'm confused about what this comment is getting at - a note without a diag is not even representable

[kadircet]

• Move patch translation logic to a separate class
• Perform eager copies and use bool translateX(X&) type of APIs instead of returning optionals.
• Perform multi line content check for each alternative, not just the closest match.

clang-tools-extra/clangd/Preamble.cpp

713

well it's gonna be an equally long class, but sure :D

733

oops, thanks! both here and also the multi-ine check below.

736

i thought because this would be rare it's fine to not do that, but the same applies in the other direction as well :D
moved the content based matching inside the loop to consider a line as alternative only after it matches the contents.

794

well i am a little worried about copying around notes/fixes just to drop them again (which can have quite some strings). but probably rare enough in practice to not matter.

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

819

well, it is to make sure we're not promoting an orphaned note to a main diag. updated the comment but happy to drop if you think it's confusing rather than being helpful.

[sammccall]

LG thanks!

clang-tools-extra/clangd/Preamble.cpp

496

up to you, but I find avoiding iterators easier to read

for (int AlternateLine : CurrentContentsToLine.lookup(OldLines[OldStart]))

(or with extracted variables)

509

why not just if (RangeContents != CurrentLines.slice(...))? I feel like I'm missing something subtle :-)

Also if you replace the slice(a, b) with drop_front(a).take_front(b) then you don't need the CurrentEnd > CurrentLines.size() check

517

if you make Closest the delta rather than the line number this gets a bit easier to follow IMO:
abs(Closest) < abs(Delta) here
R.start.line += Closest; R.end.line += Closest; below

You need to make Closest an optional<int> but that's also clearer than a sentinel -1 for me

568

FWIW if we're willing to pay a copy for a successful patch, I don't think it's worth any hassle to avoid the copy for a failed patch. Up to you though.

[kadircet]

• Use direct comparison of ArrayRefs
• optional for closest
• loop over lookup results rather than through iterator

[kadircet]

thanks for the review!

clang-tools-extra/clangd/Preamble.cpp

509

why not just if (RangeContents != CurrentLines.slice(...))? I feel like I'm missing something subtle :-)

no that's absolutely right, i guess i had a brain fart 😂

[uabelho]

Hello,

I noticed that if I compile the compiler with ubsan, then lots of Clangd tests start failing with this patch:

Failed Tests (165):
  Clangd :: ast-no-range.test
  Clangd :: ast.test
  Clangd :: call-hierarchy.test
  Clangd :: check-lines.test
  Clangd :: check.test
  Clangd :: code-action-request.test
  Clangd :: completion-auto-trigger.test
  Clangd :: completion-snippets.test
  Clangd :: completion.test
  Clangd :: config.test
  Clangd :: crash-parse.test
  Clangd :: dependency-output.test
  Clangd :: diagnostic-category.test
  Clangd :: diagnostics-no-tidy.test
  Clangd :: diagnostics-notes.test
  Clangd :: diagnostics-tidy.test
  Clangd :: did-change-configuration-params.test
  Clangd :: execute-command.test
  Clangd :: filestatus.test
  Clangd :: fixits-codeaction.test
  Clangd :: fixits-command.test
  Clangd :: fixits-embed-in-diagnostic.test
  Clangd :: folding-range.test
  Clangd :: formatting.test
  Clangd :: hover.test
  Clangd :: implementations.test
  Clangd :: inlayHints.test
  Clangd :: memory_tree.test
  Clangd :: metrics.test
  Clangd :: protocol.test
  Clangd :: references-container.test
  Clangd :: references.test
  Clangd :: rename.test
  Clangd :: request-reply.test
  Clangd :: selection-range.test
  Clangd :: semantic-tokens-refresh.test
  Clangd :: semantic-tokens.test
  Clangd :: signature-help-with-offsets.test
  Clangd :: signature-help.test
  Clangd :: symbol-info.test
  Clangd :: symbols.test
  Clangd :: target_info.test
  Clangd :: test-uri-posix.test
  Clangd :: textdocument-didchange-fail.test
  Clangd :: trace.test
  Clangd :: tweaks-format.test
  Clangd :: type-definition.test
  Clangd :: type-hierarchy-ext.test
  Clangd :: type-hierarchy.test
  Clangd :: unsupported-method.test
  Clangd :: utf8.test
  Clangd :: version.test
  Clangd :: xrefs.test
  Clangd Unit Tests :: ./ClangdTests/0/146
  [...]
  Clangd Unit Tests :: ./ClangdTests/99/146


Testing Time: 362.21s
  Skipped          :    47
  Unsupported      :  1094
  Passed           : 89208
  Expectedly Failed:   192
  Failed           :   165

Not sure if all fail the same way but there are a lot of ubsan complaints like this

07:38:09 ../lib/Support/MemoryBuffer.cpp:138:33: runtime error: null pointer passed as argument 2, which is declared to never be null
07:38:09 /usr/include/string.h:43:28: note: nonnull attribute specified here
07:38:09 SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../lib/Support/MemoryBuffer.cpp:138:33 in

[kadircet]

thanks for the ping! see the discussion in https://reviews.llvm.org/D142890#4149679

clang-tools-extra/clangd/ParsedAST.cpp

ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
 
   std::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());
+      llvm::append_range(*Diags, Patch->patchedDiags());
     // Finally, add diagnostics coming from the AST.
     {
       std::vector<Diag> D = ASTDiags.take(&*CTContext);
       Diags->insert(Diags->end(), D.begin(), D.end());
     }
   }
   ParsedAST Result(Filename, Inputs.Version, std::move(Preamble),
                    std::move(Clang), std::move(Action), std::move(Tokens),
                    std::move(Macros), std::move(Marks), std::move(ParsedDecls),

clang-tools-extra/clangd/Preamble.h

 #include "Headers.h"
 #include "clang-include-cleaner/Record.h"
 #include "index/CanonicalIncludes.h"
 #include "support/Path.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PrecompiledPreamble.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Tooling/CompilationDatabase.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
 
 #include <memory>
 #include <string>
 #include <vector>
 
 namespace clang {
 namespace clangd {
public:
   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;
 
+  /// Returns diag locations for Modified contents.
+  llvm::ArrayRef<Diag> patchedDiags() const { return PatchedDiags; }
+
   static constexpr llvm::StringLiteral HeaderName = "__preamble_patch__.h";
 
 private:
   static PreamblePatch create(llvm::StringRef FileName,
                               const ParseInputs &Modified,
                               const PreambleData &Baseline,
                               PatchType PatchType);
 
   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.
+  // Includes that are present in both Baseline and Modified. Used for
+  // patching includes of baseline preamble.
   std::vector<Inclusion> PreambleIncludes;
+  // Diags that were attached to a line preserved in Modified contents.
+  std::vector<Diag> PatchedDiags;
   PreambleBounds ModifiedBounds = {0, false};
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_PREAMBLE_H

clang-tools-extra/clangd/Preamble.cpp

 //===--- Preamble.cpp - Reusing expensive parts of the AST ----------------===//
 //
 // 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 "Preamble.h"
 #include "Compiler.h"
 #include "Config.h"
+#include "Diagnostics.h"
 #include "Headers.h"
+#include "Protocol.h"
 #include "SourceCode.h"
 #include "clang-include-cleaner/Record.h"
 #include "support/Logger.h"
 #include "support/ThreadsafeFS.h"
 #include "support/Trace.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticLex.h"

 #include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
-#include <iterator>
+#include <cstddef>
 #include <memory>
+#include <optional>
 #include <string>
 #include <system_error>
 #include <utility>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 namespace {
private:
   const LangOptions &LangOpts;
   const SourceManager &SM;
   std::vector<TextualPPDirective> &TextualDirectives;
 };
 
 struct ScannedPreamble {
   std::vector<Inclusion> Includes;
   std::vector<TextualPPDirective> TextualDirectives;
+  // Literal lines of the preamble contents.
+  std::vector<llvm::StringRef> Lines;
   PreambleBounds Bounds = {0, false};
 };
 
 /// Scans the preprocessor directives in the preamble section of the file by
 /// running preprocessor over \p Contents. Returned includes do not contain
 /// resolved paths. \p Cmd is used to build the compiler invocation, which might
 /// stat/read files.
 llvm::Expected<ScannedPreamble>
scanPreamble(llvm::StringRef Contents, const tooling::CompileCommand &Cmd) {
   PI.Contents = Contents.str();
   PI.TFS = &FS;
   PI.CompileCommand = Cmd;
   IgnoringDiagConsumer IgnoreDiags;
   auto CI = buildCompilerInvocation(PI, IgnoreDiags);
   if (!CI)
     return error("failed to create compiler invocation");
   CI->getDiagnosticOpts().IgnoreWarnings = true;
-  auto ContentsBuffer = llvm::MemoryBuffer::getMemBuffer(Contents);
+  auto ContentsBuffer = llvm::MemoryBuffer::getMemBuffer(PI.Contents);
   // This means we're scanning (though not preprocessing) the preamble section
   // twice. However, it's important to precisely follow the preamble bounds used
   // elsewhere.
   auto Bounds = ComputePreambleBounds(*CI->getLangOpts(), *ContentsBuffer, 0);
   auto PreambleContents =
       llvm::MemoryBuffer::getMemBufferCopy(Contents.substr(0, Bounds.Size));
   auto Clang = prepareCompilerInstance(
       std::move(CI), nullptr, std::move(PreambleContents),
scanPreamble(llvm::StringRef Contents, const tooling::CompileCommand &Cmd) {
   ScannedPreamble SP;
   SP.Bounds = Bounds;
   PP.addPPCallbacks(
       std::make_unique<DirectiveCollector>(PP, SP.TextualDirectives));
   if (llvm::Error Err = Action.Execute())
     return std::move(Err);
   Action.EndSourceFile();
   SP.Includes = std::move(Includes.MainFileIncludes);
+  llvm::append_range(SP.Lines, llvm::split(Contents, "\n"));
   return SP;
 }
 
 const char *spellingForIncDirective(tok::PPKeywordKind IncludeDirective) {
   switch (IncludeDirective) {
   case tok::pp_include:
     return "include";
   case tok::pp_import:
public:
   }
 
   double getTime() { return Timer.getTime(); }
 
 private:
   WallTimer Timer;
 };
 
+// Helpers for patching diagnostics between two versions of file contents.
+class DiagPatcher {
+  llvm::ArrayRef<llvm::StringRef> OldLines;
+  llvm::ArrayRef<llvm::StringRef> CurrentLines;
+  llvm::StringMap<llvm::SmallVector<int>> CurrentContentsToLine;
+
+  // Translates a range from old lines to current lines.
+  // Finds the consecutive set of lines that corresponds to the same contents in
+  // old and current, and applies the same translation to the range.
+  // Returns true if translation succeeded.
+  bool translateRange(Range &R) {
+    int OldStart = R.start.line;
+    int OldEnd = R.end.line;
+    assert(OldStart <= OldEnd);
+
+    size_t RangeLen = OldEnd - OldStart + 1;
+    auto RangeContents = OldLines.slice(OldStart).take_front(RangeLen);
+    // Make sure the whole range is covered in old contents.
+    if (RangeContents.size() < RangeLen)
+      return false;
+
+    std::optional<int> Closest;
+    for (int AlternateLine : CurrentContentsToLine.lookup(RangeContents[0])) {
+      // Check if AlternateLine matches all lines in the range.
+      if (RangeContents !=
+          CurrentLines.slice(AlternateLine).take_front(RangeLen))
+        continue;
+      int Delta = AlternateLine - OldStart;
+      if (!Closest.has_value() || abs(Delta) < abs(*Closest))
+        Closest = Delta;
+    }
+    // Couldn't find any viable matches in the current contents.
+    if (!Closest.has_value())
+      return false;
+    R.start.line += *Closest;
+    R.end.line += *Closest;
+    return true;
+  }
+
+  // Translates a Note by patching its range when inside main file. Returns true
+  // on success.
+  bool translateNote(Note &N) {
+    if (!N.InsideMainFile)
+      return true;
+    if (translateRange(N.Range))
+      return true;
+    return false;
+  }
+
+  // Tries to translate all the edit ranges inside the fix. Returns true on
+  // success. On failure fixes might be in an invalid state.
+  bool translateFix(Fix &F) {
+    return llvm::all_of(
+        F.Edits, [this](TextEdit &E) { return translateRange(E.range); });
+  }
+
+public:
+  DiagPatcher(llvm::ArrayRef<llvm::StringRef> OldLines,
+              llvm::ArrayRef<llvm::StringRef> CurrentLines) {
+    this->OldLines = OldLines;
+    this->CurrentLines = CurrentLines;
+    for (int Line = 0, E = CurrentLines.size(); Line != E; ++Line) {
+      llvm::StringRef Contents = CurrentLines[Line];
+      CurrentContentsToLine[Contents].push_back(Line);
+    }
+  }
+  // Translate diagnostic by moving its main range to new location (if inside
+  // the main file). Preserve all the notes and fixes that can be translated to
+  // new contents.
+  // Drops the whole diagnostic if main range can't be patched.
+  std::optional<Diag> translateDiag(const Diag &D) {
+    Range NewRange = D.Range;
+    // Patch range if it's inside main file.
+    if (D.InsideMainFile && !translateRange(NewRange)) {
+      // Drop the diagnostic if we couldn't patch the range.
+      return std::nullopt;
+    }
+
+    Diag NewD = D;
+    NewD.Range = NewRange;
+    // Translate ranges inside notes and fixes too, dropping the ones that are
+    // no longer relevant.
+    llvm::erase_if(NewD.Notes, [this](Note &N) { return !translateNote(N); });
+    llvm::erase_if(NewD.Fixes, [this](Fix &F) { return !translateFix(F); });
+    return NewD;
+  }
+};
 } // namespace
 
 std::shared_ptr<const PreambleData>
 buildPreamble(PathRef FileName, CompilerInvocation CI,
               const ParseInputs &Inputs, bool StoreInMemory,
               PreambleParsedCallback PreambleCallback,
               PreambleBuildStats *Stats) {
   // Note that we don't need to copy the input contents, preamble can live
   // without those.
   auto ContentsBuffer =
       llvm::MemoryBuffer::getMemBuffer(Inputs.Contents, FileName);
   auto Bounds = ComputePreambleBounds(*CI.getLangOpts(), *ContentsBuffer, 0);
 
case '"':
       break;
     default:
       break;
     }
     OS << C;
   }
 }
 
+// Translate diagnostics from baseline into modified for the lines that have the
+// same spelling.
+static std::vector<Diag> patchDiags(llvm::ArrayRef<Diag> BaselineDiags,
+                                    const ScannedPreamble &BaselineScan,
+                                    const ScannedPreamble &ModifiedScan) {
+  std::vector<Diag> PatchedDiags;
+  if (BaselineDiags.empty())
+    return PatchedDiags;
+  DiagPatcher Patcher(BaselineScan.Lines, ModifiedScan.Lines);
+  for (auto &D : BaselineDiags) {
+    if (auto NewD = Patcher.translateDiag(D))
+      PatchedDiags.emplace_back(std::move(*NewD));
+  }
+  return PatchedDiags;
+}
+
 PreamblePatch PreamblePatch::create(llvm::StringRef FileName,
                                     const ParseInputs &Modified,
                                     const PreambleData &Baseline,
                                     PatchType PatchType) {
   trace::Span Tracer("CreatePreamblePatch");
   SPAN_ATTACH(Tracer, "File", FileName);
   assert(llvm::sys::path::is_absolute(FileName) && "relative FileName!");
   // First scan preprocessor directives in Baseline and Modified. These will be
   // used to figure out newly added directives in Modified. Scanning can fail,
   // the code just bails out and creates an empty patch in such cases, as:
   // - If scanning for Baseline fails, no knowledge of existing includes hence
   //   patch will contain all the includes in Modified. Leading to rebuild of
   //   whole preamble, which is terribly slow.
   // - If scanning for Modified fails, cannot figure out newly added ones so
   //   there's nothing to do but generate an empty patch.
   auto BaselineScan = scanPreamble(
       // Contents needs to be null-terminated.
-      Baseline.Preamble.getContents().str(), Modified.CompileCommand);
+      Baseline.Preamble.getContents(), Modified.CompileCommand);
   if (!BaselineScan) {
     elog("Failed to scan baseline of {0}: {1}", FileName,
          BaselineScan.takeError());
     return PreamblePatch::unmodified(Baseline);
   }
   auto ModifiedScan = scanPreamble(Modified.Contents, Modified.CompileCommand);
   if (!ModifiedScan) {
     elog("Failed to scan modified contents of {0}: {1}", FileName,
          ModifiedScan.takeError());
     return PreamblePatch::unmodified(Baseline);
   }
 
   bool IncludesChanged = BaselineScan->Includes != ModifiedScan->Includes;
   bool DirectivesChanged =
       BaselineScan->TextualDirectives != ModifiedScan->TextualDirectives;
   if ((PatchType == PatchType::MacroDirectives || !IncludesChanged) &&
       !DirectivesChanged)
     return PreamblePatch::unmodified(Baseline);
 
   PreamblePatch PP;
   // This shouldn't coincide with any real file name.
   llvm::SmallString<128> PatchName;
   llvm::sys::path::append(PatchName, llvm::sys::path::parent_path(FileName),
                           PreamblePatch::HeaderName);
   PP.PatchFileName = PatchName.str().str();
   PP.ModifiedBounds = ModifiedScan->Bounds;
 
   llvm::raw_string_ostream Patch(PP.PatchContents);
   // Set default filename for subsequent #line directives
   Patch << "#line 0 \"";
   // FileName part of a line directive is subject to backslash escaping, which
   // might lead to problems on windows especially.
   escapeBackslashAndQuotes(FileName, Patch);
   Patch << "\"\n";
 
   if (IncludesChanged && PatchType == PatchType::All) {
     // We are only interested in newly added includes, record the ones in
     // Baseline for exclusion.
     llvm::DenseMap<std::pair<tok::PPKeywordKind, llvm::StringRef>,
                    const Inclusion *>
         ExistingIncludes;
     for (const auto &Inc : Baseline.Includes.MainFileIncludes)
       ExistingIncludes[{Inc.Directive, Inc.Written}] = &Inc;
     // There might be includes coming from disabled regions, record these for
     // exclusion too. note that we don't have resolved paths for those.
     for (const auto &Inc : BaselineScan->Includes)
       ExistingIncludes.try_emplace({Inc.Directive, Inc.Written});
     // Calculate extra includes that needs to be inserted.
     for (auto &Inc : ModifiedScan->Includes) {
       auto It = ExistingIncludes.find({Inc.Directive, Inc.Written});
       // Include already present in the baseline preamble. Set resolved path and
       // put into preamble includes.
       if (It != ExistingIncludes.end()) {
         if (It->second) {
           // If this header is included in an active region of the baseline
           // preamble, preserve it.
           auto &PatchedInc = PP.PreambleIncludes.emplace_back();
           // Copy everything from existing include, apart from the location,
for (const auto &TD : ModifiedScan->TextualDirectives) {
       // Introduce an #undef directive before #defines to suppress any
       // re-definition warnings.
       if (TD.Directive == tok::pp_define)
         Patch << "#undef " << TD.MacroName << '\n';
       Patch << "#line " << TD.DirectiveLine << '\n';
       Patch << TD.Text << '\n';
     }
   }
+
+  PP.PatchedDiags = patchDiags(Baseline.Diags, *BaselineScan, *ModifiedScan);
   dlog("Created preamble patch: {0}", Patch.str());
   Patch.flush();
   return PP;
 }
 
 PreamblePatch PreamblePatch::createFullPatch(llvm::StringRef FileName,
                                              const ParseInputs &Modified,
                                              const PreambleData &Baseline) {

 std::vector<Inclusion> PreamblePatch::preambleIncludes() const {
   return PreambleIncludes;
 }
 
 PreamblePatch PreamblePatch::unmodified(const PreambleData &Preamble) {
   PreamblePatch PP;
   PP.PreambleIncludes = Preamble.Includes.MainFileIncludes;
   PP.ModifiedBounds = Preamble.Preamble.getBounds();
+  PP.PatchedDiags = Preamble.Diags;
   return PP;
 }
 
 bool PreamblePatch::preserveDiagnostics() const {
   return PatchContents.empty() ||
          Config::current().Diagnostics.AllowStalePreamble;
 }
 } // namespace clangd
 } // namespace clang

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

TEST(PreamblePatch, TranslatesDiagnosticsInPreamble) {
 
   {
     // 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")));
+                ElementsAre(Diag(NewCode.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")));
+                ElementsAre(Diag(NewCode.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")));
+    EXPECT_THAT(*AST->getDiagnostics(), IsEmpty());
   }
   {
     // 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")));
+                ElementsAre(Diag(NewCode.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(), IsEmpty());
+  }
+  {
+    // Multiple lines.
+    Annotations Code(R"(
+#define BAR
+#include [[<fo\
+o>]])");
+    Annotations NewCode(R"(#include [[<fo\
+o>]])");
+    auto AST = createPatchedAST(Code.code(), NewCode.code());
     EXPECT_THAT(*AST->getDiagnostics(),
-                ElementsAre(Diag(Code.range(), "pp_file_not_found")));
+                ElementsAre(Diag(NewCode.range(), "pp_file_not_found")));
+  }
+  {
+    // Multiple lines with change.
+    Annotations Code(R"(
+#define BAR
+#include <fox>
+#include [[<fo\
+o>]])");
+    Annotations NewCode(R"(#include <fo\
+x>)");
+    auto AST = createPatchedAST(Code.code(), NewCode.code());
+    EXPECT_THAT(*AST->getDiagnostics(), IsEmpty());
+  }
+  {
+    // Preserves notes.
+    Annotations Code(R"(
+#define $note[[BAR]] 1
+#define $main[[BAR]] 2)");
+    Annotations NewCode(R"(
+#define BAZ 0
+#define $note[[BAR]] 1
+#define BAZ 0
+#define $main[[BAR]] 2)");
+    auto AST = createPatchedAST(Code.code(), NewCode.code());
+    EXPECT_THAT(
+        *AST->getDiagnostics(),
+        ElementsAre(AllOf(Diag(NewCode.range("main"), "-Wmacro-redefined"),
+                          withNote(Diag(NewCode.range("note"))))));
+  }
+  {
+    // Preserves diag without note.
+    Annotations Code(R"(
+#define $note[[BAR]] 1
+#define $main[[BAR]] 2)");
+    Annotations NewCode(R"(
+#define $main[[BAR]] 2)");
+    auto AST = createPatchedAST(Code.code(), NewCode.code());
+    EXPECT_THAT(
+        *AST->getDiagnostics(),
+        ElementsAre(AllOf(Diag(NewCode.range("main"), "-Wmacro-redefined"),
+                          Field(&Diag::Notes, IsEmpty()))));
+  }
+  {
+    // Make sure orphaned notes are not promoted to diags.
+    Annotations Code(R"(
+#define $note[[BAR]] 1
+#define $main[[BAR]] 2)");
+    Annotations NewCode(R"(
+#define BAZ 0
+#define BAR 1)");
+    auto AST = createPatchedAST(Code.code(), NewCode.code());
+    EXPECT_THAT(*AST->getDiagnostics(), IsEmpty());
   }
 }
 } // namespace
 } // namespace clangd
 } // namespace clang