[clangd] Surface diagnostics from headers inside main file

[ilya-biryukov]

I'll look more closely into the details, but just a high-level question now: why would we want to make this optional and not simply surface these extra diagnostics?

[kadircet]

In D59302#1431045, @ilya-biryukov wrote:

I'll look more closely into the details, but just a high-level question now: why would we want to make this optional and not simply surface these extra diagnostics?

The optional part is rather limiting number of diagnostics that is going to be surfaced. So if you have thousands of errors inside preamble only first 100 of them will appear inside main file by default.
But looking at it again, currently there is no way for user to say "do not limit number of diagnostics" :D They can only change this limit to different values.

[kadircet]

• Only surface diagnostics with level fatal or error

[ilya-biryukov]

In D59302#1431274, @kadircet wrote:

The optional part is rather limiting number of diagnostics that is going to be surfaced. So if you have thousands of errors inside preamble only first 100 of them will appear inside main file by default.
But looking at it again, currently there is no way for user to say "do not limit number of diagnostics" :D They can only change this limit to different values.

Let's pick a number and hard-code it. We'll get consistent behavior in all clients of clangd, I can hardly think of any existing client that would want to surface everything to the user.
FWIW, 100 seem too much to my taste, since we're only showing errors. Would 10 be a better limit?

[ilya-biryukov]

clangd/ClangdUnit.cpp

247

We should find a way to point into an exact include directive (source locations have this information).

395

Could we limit per-include-directive instead?
The limit to avoid creating too verbose error messages, rather than reporting too many messages, right?

421

We should provide the include stack, similar to how compiler does it.
Otherwise it would be really hard to figure out the exact problem the diagnostic is pointing at.

[kadircet]

• Show include stack in diagnostic message
• Point to exact include location

[kadircet]

• Limit per include directive
• Use hardcoded value for limit

[kadircet]

• Make limit a hardcoded value rather then a command line argument
• Limit diags per include directive

[ilya-biryukov]

Thanks for changes. A few more comments.

clangd/ClangdUnit.cpp

246

Could we binary-search in a sorted vector<Inclusion> with a custom comparator instead?

401

The function name suggests it does not have side-effects.
Maybe handle the update and query of NumberOfDiagstAtLine outside this function instead?

clangd/Diagnostics.cpp

419

We can still drop diagnostics at some point, right?
Could we move the log statement in a different point?

451

NIT: could we reuse the function from clang that does the same thing?

The code here is pretty simple, though, so writing it here is fine. Just wanted to make sure we considered this option and found it's easier to redo this work ourselves.

clangd/Diagnostics.h

87

Could we avoid changing this interface?
We have enough information to fill the corresponding range and message when collecting diagnostics inside our implementation of clang's DiagnosticConsumer, there's really no point in carrying this information in this struct.

If that means intermediate structures stored in our consumer, so be it, it's a private implementation detail. Diag is a public interface, so keeping it simpler is valuable.

unittests/clangd/DiagnosticsTests.cpp

787

NIT: maybe use raw string literals to keep the message a one-liner?

801

Could we add a test for reaching the limit?
Could we mention that some diagnostics were dropped in that case? Something like:

In file included from a.h:20:1:
error: unresolved identifier 'foo'.

And 10 more diagnostics...

[kadircet]

• Address comments

clangd/Diagnostics.cpp

451

there is TextDiagnostic which prints the desired output expect the fact that it also prints the first line saying the header was included in main file. Therefore, I didn't make use of it since we decided that was not very useful for our use case. And it requires inheriting from DiagnosticRenderer which was requiring too much boiler plate code just to get this functionality so instead I've chosen doing it like this.

Can fallback to TextDiagnostic if you believe showing Included in mainfile.cc:x:y: at the beginning of the diagnostic won't be annoying.

unittests/clangd/DiagnosticsTests.cpp

801

There is already one, see LimitDiagsOutsideMainFile

[ilya-biryukov]

clangd/ClangdUnit.cpp

392

The name suggests we filter all diagnostics based on this helper.
Maybe add rename to something more specific?

E.g. IsErrorOrFatal or IsImporantHeaderDiagnostic...

395

NIT: simplify to

return D.Severity == DiagnosticsEngine::Level::Error
    || D.Severity == DiagnosticsEngine::Level::Fatal

400

Can we do this in StoreDiags::flushLastDiag?
All code handling the diagnostics lives there and it has all the information necessary to map to the included line.

401

NIT: rename to AddDiagnosticFromInclude or something similar, but more specific than the current name

411

This extra complexity does not seem to be worth it after all.
I'd suggest to remove this (at least from the first version of the file), even though I was the one who proposed it.

Still think it's valuable, but the patch is complicated enough as it is, keeping it simpler seems to be more important at this point.

412

Why not merge the ShouldSurfaceDiag and TryAddDiag into a single function and handle all the complexities there?
This would simplify the client code, it would be as simple as

else
 AddDiagnosticFromInclude(D)

clangd/Diagnostics.cpp

426

That's a lot of code, could we extract this into a separate function?

427

Why use getPresumedLoc? Making clangd sensitive to pp directives that rename file or change line numbers does not make any sense.

Could you also add tests for that?

451

LG, it's not too hard to reconstruct the same output.
Note that D60267 starts outputting notes in a structured way, using the RelatedInformation struct from the LSP.

We should eventually add the include stack into related information too. With that in mind, we should probably add the include stack as a new field to the struct we use for diagnostics.

[kadircet]

• Address comments

clangd/Diagnostics.cpp

427

It was the way DiagnosticRenderer emitted include stacks, I had just copied it over.
Changing with SourceManger::getIncludeLoc

451

SG, delaying serialization to LSP layer then.

[kadircet]

Ping

[ilya-biryukov]

Sorry, lost this revision. Looking now

[ilya-biryukov]

clangd/ClangdUnit.cpp

251

NIT: use llvm::lower_bound

252

NIT: lower_bound also works when you work on a different type, no need for the temporary variable.

lower_bound(MainFileIncludes…, [](const Inclusion &L, const Position &Pos) {
  return L.start.line < Pos.line;
})

(the order of lamda parameters might need to be reversed, I always forget what's the correct one)

255

NIT: add a sanity check that the include was actually there: assert(Res->R == Pos)

clangd/Diagnostics.cpp

55

reduce nesting by inverting the condition:

if (IncludeStack.empty())
  return;
// rest of the code

205

Could we get the message first and the include stack later?
The first line is often showed in various lists and having In file included from there is not very helpful.

463

Could you inline ShouldAddDiag into its single callsite?

clangd/Diagnostics.h

87

Could you store pair</*Filename*/string, Position> instead?
Would make it simpler to adapt D60267 to this change.

131

NIT: use tripple slash comments

132

NIT: the name makes me believe this does a different thing. Maybe mention "include" in the name to make it less confusing?
Something like DiagsInsideIncludes would work.

Another NIT: we don't need a map anymore: DenseSet<int> should be enough

unittests/clangd/DiagnosticsTests.cpp

54

NIT: maybe simplify to std::equal(IncludeStack.begin(), IncludeStack.end(), arg.IncludeStack.begin())

620

We were discussing adding the extra files to TestTU instead.
Any reason this did not work out?

[kadircet]

• Address comments

unittests/clangd/DiagnosticsTests.cpp

620

Yes, my LRU cache evicted the discussion :D

[ilya-biryukov]

clangd/Diagnostics.cpp

78

Could we avoid introducing a function that breaks the invariant of a type?
Having a function to print Position differently would be a much better fit.

218

WDYT about changing this to a shorter form?

include stack:

./project/foo.h:312
/usr/include/vector:344

Note that it does not mention column numbers as they aren't useful and is shorter.
Since we're already not 100% aligned with clang, why not have a more concise representation?

unittests/clangd/DiagnosticsTests.cpp

738

So the last location in the include stack is actually the original location of an error.
This is non-obvious design. Maybe move this into a separate field?

unittests/clangd/TestTU.h

51 ↗

(On Diff #195375)

Maybe use relative paths?
This would be consistent with Filename and HeaderFilename

52 ↗

(On Diff #195375)

Why not StringMap</*Content*/std::string>?
This would be consistent with buildTestFS and disallow adding duplicates.

[kadircet]

• Address comments
• Drop include stack

[kadircet]

clangd/Diagnostics.cpp

78

No longer needed reverting.

218

As discussed offline getting rid of include stack in this version of the patch.

[ilya-biryukov]

clangd/Diagnostics.cpp

118

replace vector<SourceLocation> with SourceLocation now that we only need one of the locations.

unittests/clangd/DiagnosticsTests.cpp

718

NIT: maybe quote the name of the header for better readability: error in include 'c.h' ...?

[ilya-biryukov]

This looks nice and minimal, thanks!
Happy to LGTM this, unless @sammccall want to take another look (e.g. to account for related information patch)

[kadircet]

• Address comments

[sammccall]

Nice! Just readability/wording nits

clangd/Diagnostics.cpp

140

Clang tends to use a phrase ending in "here" for such notes.
Suggest "error occurred here"

140

message should not be capitalized (capitalization will be added later if needed)

144

Include is not a noun. "included file"?

144

"Error" is the severity, which appears elsewhere in LSP. (Other messages do not include it)

144

message should not be capitalized

145

why are we including the filename here?

it obscures the error message, and will be available via the note.

463

Does this improve readability over (inline) D.Severity >= DiagnosticsEngine::Error?

clangd/Diagnostics.h

131

nit: "containing an error" (and variable name)

IncludeLinesWithErrors might be a clearer name

[kadircet]

• Address comments

[kadircet]

• Rebase

clangd/ClangdUnit.cpp

 //===--- ClangdUnit.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 "ClangdUnit.h"
 #include "../clang-tidy/ClangTidyDiagnosticConsumer.h"
 #include "../clang-tidy/ClangTidyModuleRegistry.h"
 #include "Compiler.h"
 #include "Diagnostics.h"
 #include "Headers.h"
 #include "IncludeFixer.h"
 #include "Logger.h"
+#include "Protocol.h"
 #include "SourceCode.h"
 #include "Trace.h"
 #include "index/CanonicalIncludes.h"
 #include "index/Index.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Frontend/Utils.h"
 #include "clang/Index/IndexDataConsumer.h"
 #include "clang/Index/IndexingAction.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Lex/MacroInfo.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Sema/Sema.h"
 #include "clang/Serialization/ASTWriter.h"
 #include "clang/Serialization/PCHContainerOperations.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <memory>
 
 namespace clang {
 namespace clangd {
 namespace {
 
private:
 
   const IncludeStructure &Includes;
   PPCallbacks *Delegate;
   const SourceManager &SM;
   Preprocessor &PP;
   const LangOptions &LangOpts;
 };
 
+// Get the range for the included header begining at Position.
+Range positionToRange(const Position &Pos,
+                      const std::vector<Inclusion> &MainFileIncludes) {
+  Inclusion Inc;
+  Inc.R.start = Pos;
+  auto Res =
+      std::lower_bound(MainFileIncludes.begin(), MainFileIncludes.end(), Inc,
+                       [](const Inclusion &LHS, const Inclusion &RHS) {
+                         return LHS.R.start.line < RHS.R.start.line;
+                       });
+  return Res->R;
+}
+
 } // namespace
 
 void dumpAST(ParsedAST &AST, llvm::raw_ostream &OS) {
   AST.getASTContext().getTranslationUnitDecl()->dump(OS, true);
 }
 
 llvm::Optional<ParsedAST>
 ParsedAST::build(std::unique_ptr<CompilerInvocation> CI,
ParsedAST::build(std::unique_ptr<CompilerInvocation> CI,
   Clang->getDiagnostics().setClient(new IgnoreDiagnostics);
   // CompilerInstance won't run this callback, do it directly.
   ASTDiags.EndSourceFile();
   // XXX: This is messy: clang-tidy checks flush some diagnostics at EOF.
   // However Action->EndSourceFile() would destroy the ASTContext!
   // So just inform the preprocessor of EOF, while keeping everything alive.
   Clang->getPreprocessor().EndSourceFile();
 
   std::vector<Diag> Diags = ASTDiags.take();
   // Populate diagnostic source.
   for (auto &D : Diags)
     D.S =
         !CTContext->getCheckName(D.ID).empty() ? Diag::ClangTidy : Diag::Clang;
   // Add diagnostics from the preamble, if any.
   if (Preamble)
     Diags.insert(Diags.begin(), Preamble->Diags.begin(), Preamble->Diags.end());
   return ParsedAST(std::move(Preamble), std::move(Clang), std::move(Action),
                    std::move(ParsedDecls), std::move(Diags),
                    std::move(Includes), std::move(CanonIncludes));
 }
 
 ParsedAST::ParsedAST(ParsedAST &&Other) = default;
 
 ParsedAST &ParsedAST::operator=(ParsedAST &&Other) = default;
 
 ParsedAST::~ParsedAST() {
   if (Action) {
     // We already notified the PP of end-of-file earlier, so detach it first.
     // We must keep it alive until after EndSourceFile(), Sema relies on this.
     auto PP = Clang->getPreprocessorPtr(); // Keep PP alive for now.
     Clang->setPreprocessor(nullptr);       // Detach so we don't send EOF again.
     Action->EndSourceFile();               // Destroy ASTContext and Sema.
     // Now Sema is gone, it's safe for PP to go out of scope.
   }
 }
 
 ASTContext &ParsedAST::getASTContext() { return Clang->getASTContext(); }
 
 const ASTContext &ParsedAST::getASTContext() const {
   return Clang->getASTContext();
 }
 
 Preprocessor &ParsedAST::getPreprocessor() { return Clang->getPreprocessor(); }
 
 std::shared_ptr<Preprocessor> ParsedAST::getPreprocessorPtr() {
   return Clang->getPreprocessorPtr();

clangd/Diagnostics.h

 //===--- Diagnostics.h -------------------------------------------*- 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_DIAGNOSTICS_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_DIAGNOSTICS_H
 
 #include "Path.h"
 #include "Protocol.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/LangOptions.h"
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringSet.h"
 #include <cassert>
 #include <string>
 
 namespace clang {
 namespace clangd {
 
enum Source {
     Clang,
     ClangTidy,
   };
   Source S = Clang;
   /// Elaborate on the problem, usually pointing to a related piece of code.
   std::vector<Note> Notes;
   /// *Alternative* fixes for this diagnostic, one should be chosen.
   std::vector<Fix> Fixes;
+  /// Stores strings in the form: "/path/a.h:3:4". The include closest to main
+  /// file is in front.
+  std::vector<std::string> IncludeStack;
 };
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Diag &D);
 
 /// Conversion to LSP diagnostics. Formats the error message of each diagnostic
 /// to include all its notes. Notes inside main file are also provided as
 /// separate diagnostics with their corresponding fixits. Notes outside main
 /// file do not have a corresponding LSP diagnostic, but can still be included
 /// as part of their main diagnostic's message.

 
 private:
   void flushLastDiag();
 
   DiagFixer Fixer = nullptr;
   std::vector<Diag> Output;
   llvm::Optional<LangOptions> LangOpts;
   llvm::Optional<Diag> LastDiag;
+  // Counts diagnostics coming from a header in the main file.
+  llvm::DenseMap<int, int> NumberOfDiagsAtLine;
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_DIAGNOSTICS_H

clangd/Diagnostics.cpp

 //===--- Diagnostics.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 "Diagnostics.h"
 #include "Compiler.h"
 #include "Logger.h"
 #include "SourceCode.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Capacity.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/Signals.h"
 #include <algorithm>
 
 namespace clang {
 namespace clangd {
 
 namespace {
 
 bool mentionsMainFile(const Diag &D) {
   if (D.InsideMainFile)
     return true;
   // Fixes are always in the main file.
   if (!D.Fixes.empty())
     return true;
   for (auto &N : D.Notes) {
     if (N.InsideMainFile)
       return true;
   }
   return false;
 }
 
+void adjustDiagFromHeader(Diag &D, const clang::Diagnostic &Info,
+                          const LangOptions &LangOpts) {
+  const SourceManager &SM = Info.getSourceManager();
+  std::vector<SourceLocation> IncludeStack;
+  auto GetIncludeLoc = [&SM](SourceLocation SLoc) {
+    return SM.getIncludeLoc(SM.getFileID(SLoc));
+  };
+  for (auto IncludeLocation = GetIncludeLoc(Info.getLocation());
+       IncludeLocation.isValid();
+       IncludeLocation = GetIncludeLoc(IncludeLocation)) {
+    IncludeStack.push_back(IncludeLocation);
+  }
+  if (!IncludeStack.empty()) {
+    D.File = SM.getFileEntryForID(SM.getMainFileID())->getName().str();
+    D.Range.start = sourceLocToPosition(SM, IncludeStack.back());
+    D.Range.end = sourceLocToPosition(
+        SM, Lexer::getLocForEndOfToken(IncludeStack.back(), 0, SM, LangOpts));
+    // We don't show the main file as part of include stack, which is the
+    // last element.
+    if (IncludeStack.size() > 1) {
+      IncludeStack.pop_back();
+      std::reverse(IncludeStack.begin(), IncludeStack.end());
+      for (auto &Inc : IncludeStack)
+        D.IncludeStack.push_back(Inc.printToString(SM));
+    }
+    D.IncludeStack.push_back(Info.getLocation().printToString(SM));
+  }
+}
+
 // Checks whether a location is within a half-open range.
 // Note that clang also uses closed source ranges, which this can't handle!
 bool locationInRange(SourceLocation L, CharSourceRange R,
                      const SourceManager &M) {
   assert(R.isCharRange());
   if (!R.isValid() || M.getFileID(R.getBegin()) != M.getFileID(R.getEnd()) ||
       M.getFileID(R.getBegin()) != M.getFileID(L))
     return false;
   return L != R.getEnd() && M.isPointWithin(L, R.getBegin(), R.getEnd());
 }
 
 // Clang diags have a location (shown as ^) and 0 or more ranges (~~~~).
 // LSP needs a single range.
 Range diagnosticRange(const clang::Diagnostic &D, const LangOptions &L) {
   auto &M = D.getSourceManager();
if (!R.isValid()) // Fall back to location only, let the editor deal with it.
     R = CharSourceRange::getCharRange(Loc);
   return halfOpenToRange(M, R);
 }
 
 bool isInsideMainFile(const SourceLocation Loc, const SourceManager &M) {
   return Loc.isValid() && M.isWrittenInMainFile(M.getFileLoc(Loc));
 }
 
 bool isInsideMainFile(const clang::Diagnostic &D) {
   if (!D.hasSourceManager())
     return false;
 
   return isInsideMainFile(D.getLocation(), D.getSourceManager());
 }
 
 bool isNote(DiagnosticsEngine::Level L) {
   return L == DiagnosticsEngine::Note || L == DiagnosticsEngine::Remark;
 }
 
 llvm::StringRef diagLeveltoString(DiagnosticsEngine::Level Lvl) {
   switch (Lvl) {
   case DiagnosticsEngine::Ignored:
     return "ignored";
   case DiagnosticsEngine::Note:
     return "note";
   case DiagnosticsEngine::Remark:
     return "remark";
   case DiagnosticsEngine::Warning:
     return "warning";
   case DiagnosticsEngine::Error:
     return "error";
   case DiagnosticsEngine::Fatal:
     return "fatal error";
   }
   llvm_unreachable("unhandled DiagnosticsEngine::Level");
 }
 
 /// Prints a single diagnostic in a clang-like manner, the output includes
 /// location, severity and error message. An example of the output message is:
 ///
 ///     main.cpp:12:23: error: undeclared identifier
 ///
 /// For main file we only print the basename and for all other files we print
 /// the filename on a separate line to provide a slightly more readable output

 ///
 ///     main.cpp:3:5: note: candidate function not viable: requires 2 arguments
 ///
 ///     dir1/dir2/dir3/../../dir4/header.h:12:23
 ///     note: candidate function not viable: requires 3 arguments
 std::string mainMessage(const Diag &D, bool DisplayFixesCount) {
   std::string Result;
   llvm::raw_string_ostream OS(Result);
+  if (!D.IncludeStack.empty()) {
+    for (size_t I = 0, E = D.IncludeStack.size(); I < E; ++I) {
+      if (I != E - 1)
+        OS << "In file included from: ";
+      OS << D.IncludeStack[I] << ": ";
+      if (I != E - 1)
+        OS << '\n';
+    }
+  }
   OS << D.Message;
   if (DisplayFixesCount && !D.Fixes.empty())
     OS << " (" << (D.Fixes.size() > 1 ? "fixes" : "fix") << " available)";
   for (auto &Note : D.Notes) {
     OS << "\n\n";
     printDiag(OS, Note);
   }
   OS.flush();
   return capitalize(std::move(Result));
 }
 
 /// Returns a message sent to LSP for the note of the main diagnostic.
 /// The message includes the main diagnostic to provide the necessary context
 /// for the user to understand the note.
 std::string noteMessage(const Diag &Main, const DiagBase &Note) {
   std::string Result;
void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
 
   if (!isNote(DiagLevel)) {
     // Handle the new main diagnostic.
     flushLastDiag();
 
     LastDiag = Diag();
     LastDiag->ID = Info.getID();
     FillDiagBase(*LastDiag);
+    adjustDiagFromHeader(*LastDiag, Info, *LangOpts);
 
     if (!Info.getFixItHints().empty())
       AddFix(true /* try to invent a message instead of repeating the diag */);
     if (Fixer) {
       auto ExtraFixes = Fixer(DiagLevel, Info);
       LastDiag->Fixes.insert(LastDiag->Fixes.end(), ExtraFixes.begin(),
                              ExtraFixes.end());
     }
   } else {
     // Handle a note to an existing diagnostic.
     if (!LastDiag) {
       assert(false && "Adding a note without main diagnostic");
       IgnoreDiagnostics::log(DiagLevel, Info);
       return;
     }
 
     if (!Info.getFixItHints().empty()) {
       // A clang note with fix-it is not a separate diagnostic in clangd. We
       // attach it as a Fix to the main diagnostic instead.
       if (!AddFix(false /* use the note as the message */))
         IgnoreDiagnostics::log(DiagLevel, Info);
     } else {
       // A clang note without fix-its corresponds to clangd::Note.
       Note N;
       FillDiagBase(N);
 
       LastDiag->Notes.push_back(std::move(N));
     }
   }
 }
 
 void StoreDiags::flushLastDiag() {
   if (!LastDiag)
     return;
-  if (mentionsMainFile(*LastDiag))
+  // Only keeps diagnostics inside main file or the first one coming from a
+  // header.
+  auto ShouldAddDiag = [this](const Diag &D) {
+    if (mentionsMainFile(D))
+      return true;
+    auto IsImportantHeaderDiag = [](const Diag &D) {
+      return D.Severity == DiagnosticsEngine::Level::Error ||
+             D.Severity == DiagnosticsEngine::Level::Fatal;
+    };
+    if (!IsImportantHeaderDiag(D) ||
+        ++NumberOfDiagsAtLine[D.Range.start.line] > 1) {
+      vlog("Dropped diagnostic: {0}: {1}", D.File, D.Message);
+      return false;
+    }
+    return true;
+  };
+  if (ShouldAddDiag(*LastDiag))
     Output.push_back(std::move(*LastDiag));
-  else
-    vlog("Dropped diagnostic outside main file: {0}: {1}", LastDiag->File,
-         LastDiag->Message);
   LastDiag.reset();
 }
 
 } // namespace clangd
 } // namespace clang

unittests/clangd/DiagnosticsTests.cpp

 //===--- DiagnosticsTests.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 "ClangdUnit.h"
 #include "SourceCode.h"
+#include "TestFS.h"
 #include "TestIndex.h"
 #include "TestTU.h"
 #include "index/MemIndex.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
testing::Matcher<const Diag &> WithNote(testing::Matcher<Note> NoteMatcher) {
   return Field(&Diag::Notes, ElementsAre(NoteMatcher));
 }
 
 MATCHER_P2(Diag, Range, Message,
            "Diag at " + llvm::to_string(Range) + " = [" + Message + "]") {
   return arg.Range == Range && arg.Message == Message;
 }
 
+MATCHER_P3(Diag, Range, Message, IncludeStack,
+           "Diag at " + llvm::to_string(Range) + " = [" + Message + "]") {
+  if (arg.Range != Range || arg.Message != Message ||
+      arg.IncludeStack.size() != IncludeStack.size())
+    return false;
+  for (size_t I = 0, E = IncludeStack.size(); I < E; ++I)
+    if (IncludeStack[I] != arg.IncludeStack[I])
+      return false;
+  return true;
+}
+
 MATCHER_P3(Fix, Range, Replacement, Message,
            "Fix " + llvm::to_string(Range) + " => " +
                testing::PrintToString(Replacement) + " = [" + Message + "]") {
   return arg.Message == Message && arg.Edits.size() == 1 &&
          arg.Edits[0].range == Range && arg.Edits[0].newText == Replacement;
 }
 
 MATCHER_P(EqualToLSPDiag, LSPDiag,
MATCHER_P(EqualToFix, Fix, "LSP fix " + llvm::to_string(Fix)) {
   for (std::size_t I = 0; I < arg.Edits.size(); ++I) {
     if (arg.Edits[I].range != Fix.Edits[I].range ||
         arg.Edits[I].newText != Fix.Edits[I].newText)
       return false;
   }
   return true;
 }
 
-
 // Helper function to make tests shorter.
 Position pos(int line, int character) {
   Position Res;
   Res.line = line;
   Res.character = character;
   return Res;
 }
 

 
 TEST(DiagnosticsTest, ToLSP) {
   clangd::Diag D;
   D.Message = "something terrible happened";
   D.Range = {pos(1, 2), pos(3, 4)};
   D.InsideMainFile = true;
   D.Severity = DiagnosticsEngine::Error;
   D.File = "foo/bar/main.cpp";
+  D.IncludeStack.push_back("a/b.h:1:2");
+  D.IncludeStack.push_back("a/c.h:2:2");
 
   clangd::Note NoteInMain;
   NoteInMain.Message = "declared somewhere in the main file";
   NoteInMain.Range = {pos(5, 6), pos(7, 8)};
   NoteInMain.Severity = DiagnosticsEngine::Remark;
   NoteInMain.File = "../foo/bar/main.cpp";
   NoteInMain.InsideMainFile = true;
   D.Notes.push_back(NoteInMain);
auto MatchingLSP = [](const DiagBase &D, StringRef Message) {
     Res.range = D.Range;
     Res.severity = getSeverity(D.Severity);
     Res.message = Message;
     return Res;
   };
 
   // Diagnostics should turn into these:
   clangd::Diagnostic MainLSP =
-      MatchingLSP(D, R"(Something terrible happened (fix available)
+      MatchingLSP(D, R"(In file included from: a/b.h:1:2: 
+a/c.h:2:2: something terrible happened (fix available)
 
 main.cpp:6:7: remark: declared somewhere in the main file
 
 ../foo/baz/header.h:10:11:
 note: declared somewhere in the header file)");
 
   clangd::Diagnostic NoteInMainLSP =
       MatchingLSP(NoteInMain, R"(Declared somewhere in the main file
namespace c {
 
   EXPECT_THAT(TU.build().getDiagnostics(),
               UnorderedElementsAre(AllOf(
                   Diag(Test.range(), "no type named 'X' in namespace 'a'"),
                   WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
                               "Add include \"x.h\" for symbol a::X")))));
 }
 
+ParsedAST build(const std::string &MainFile,
+                const llvm::StringMap<std::string> &Files) {
+  std::vector<const char *> Cmd = {"clang", MainFile.c_str()};
+  ParseInputs Inputs;
+  Inputs.CompileCommand.Filename = MainFile;
+  Inputs.CompileCommand.CommandLine = {Cmd.begin(), Cmd.end()};
+  Inputs.CompileCommand.Directory = testRoot();
+  Inputs.Contents = Files.lookup(MainFile);
+  Inputs.FS = buildTestFS(Files);
+  Inputs.Opts = ParseOptions();
+  auto PCHs = std::make_shared<PCHContainerOperations>();
+  auto CI = buildCompilerInvocation(Inputs);
+  assert(CI && "Failed to build compilation invocation.");
+  auto Preamble =
+      buildPreamble(MainFile, *CI,
+                    /*OldPreamble=*/nullptr,
+                    /*OldCompileCommand=*/Inputs.CompileCommand, Inputs,
+                    /*StoreInMemory=*/true, /*PreambleCallback=*/nullptr);
+  auto AST = buildAST(MainFile, createInvocationFromCommandLine(Cmd), Inputs,
+                      Preamble);
+  if (!AST.hasValue()) {
+    ADD_FAILURE() << "Failed to build code:\n" << Files.lookup(MainFile);
+    llvm_unreachable("Failed to build TestTU!");
+  }
+  return std::move(*AST);
+}
+
+TEST(DiagsInHeaders, DiagInsideHeader) {
+  Annotations Main(R"cpp(
+    #include [["a.h"]]
+    void foo() {})cpp");
+  std::string MainFile = testPath("main.cc");
+  llvm::StringMap<std::string> Files = {{MainFile, Main.code()},
+                                        {testPath("a.h"), "no_type_spec;"}};
+  auto FS = buildTestFS(Files);
+
+  EXPECT_THAT(build(MainFile, Files).getDiagnostics(),
+              UnorderedElementsAre(
+                  Diag(Main.range(),
+                       "C++ requires a type specifier for all declarations",
+                       std::vector<std::string>{"/clangd-test/a.h:1:1"})));
+}
+
+TEST(DiagsInHeaders, DiagInTransitiveInclude) {
+  Annotations Main(R"cpp(
+    #include [["a.h"]]
+    void foo() {})cpp");
+  std::string MainFile = testPath("main.cc");
+  llvm::StringMap<std::string> Files = {{MainFile, Main.code()},
+                                        {testPath("a.h"), "#include \"b.h\""},
+                                        {testPath("b.h"), "no_type_spec;"}};
+  auto FS = buildTestFS(Files);
+
+  EXPECT_THAT(build(MainFile, Files).getDiagnostics(),
+              UnorderedElementsAre(
+                  Diag(Main.range(),
+                       "C++ requires a type specifier for all declarations",
+                       std::vector<std::string>{"/clangd-test/a.h:1:10",
+                                                "/clangd-test/b.h:1:1"})));
+}
+
+TEST(DiagsInHeaders, DiagInMultipleHeaders) {
+  Annotations Main(R"cpp(
+    #include $a[["a.h"]]
+    #include $b[["b.h"]]
+    void foo() {})cpp");
+  std::string MainFile = testPath("main.cc");
+  llvm::StringMap<std::string> Files = {{MainFile, Main.code()},
+                                        {testPath("a.h"), "no_type_spec;"},
+                                        {testPath("b.h"), "no_type_spec;"}};
+  auto FS = buildTestFS(Files);
+
+  EXPECT_THAT(build(MainFile, Files).getDiagnostics(),
+              UnorderedElementsAre(
+                  Diag(Main.range("a"),
+                       "C++ requires a type specifier for all declarations",
+                       std::vector<std::string>{"/clangd-test/a.h:1:1"}),
+                  Diag(Main.range("b"),
+                       "C++ requires a type specifier for all declarations",
+                       std::vector<std::string>{"/clangd-test/b.h:1:1"})));
+}
+
+TEST(DiagsInHeaders, PreferExpansionLocation) {
+  Annotations Main(R"cpp(
+    #include [["a.h"]]
+    #include "b.h"
+    void foo() {})cpp");
+  std::string MainFile = testPath("main.cc");
+  llvm::StringMap<std::string> Files = {
+      {MainFile, Main.code()},
+      {testPath("a.h"), "#include \"b.h\"\n"},
+      {testPath("b.h"), "#ifndef X\n#define X\nno_type_spec;\n#endif"}};
+  auto FS = buildTestFS(Files);
+
+  EXPECT_THAT(build(MainFile, Files).getDiagnostics(),
+              UnorderedElementsAre(
+                  Diag(Main.range(),
+                       "C++ requires a type specifier for all declarations",
+                       std::vector<std::string>{"/clangd-test/a.h:1:10",
+                                                "/clangd-test/b.h:3:1"})));
+}
+
+TEST(DiagsInHeaders, PreferExpansionLocationMacros) {
+  Annotations Main(R"cpp(
+    #define X
+    #include "a.h"
+    #undef X
+    #include [["b.h"]]
+    void foo() {})cpp");
+  std::string MainFile = testPath("main.cc");
+  llvm::StringMap<std::string> Files = {
+      {MainFile, Main.code()},
+      {testPath("a.h"), "#include \"c.h\"\n"},
+      {testPath("b.h"), "#include \"c.h\"\n"},
+      {testPath("c.h"), "#ifndef X\n#define X\nno_type_spec;\n#endif"}};
+  auto FS = buildTestFS(Files);
+
+  EXPECT_THAT(build(MainFile, Files).getDiagnostics(),
+              UnorderedElementsAre(
+                  Diag(Main.range(),
+                       "C++ requires a type specifier for all declarations",
+                       std::vector<std::string>{"/clangd-test/b.h:1:10",
+                                                "/clangd-test/c.h:3:1"})));
+}
+
+TEST(DiagsInHeaders, LimitDiagsOutsideMainFile) {
+  Annotations Main(R"cpp(
+    #include [["a.h"]]
+    #include "b.h"
+    void foo() {})cpp");
+  std::string MainFile = testPath("main.cc");
+  llvm::StringMap<std::string> Files = {{MainFile, Main.code()},
+                                        {testPath("a.h"), "#include \"c.h\"\n"},
+                                        {testPath("b.h"), "#include \"c.h\"\n"},
+                                        {testPath("c.h"), R"cpp(
+      #ifndef X
+      #define X
+      no_type_spec_0;
+      no_type_spec_1;
+      no_type_spec_2;
+      no_type_spec_3;
+      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"}};
+  auto FS = buildTestFS(Files);
+
+  EXPECT_THAT(build(MainFile, Files).getDiagnostics(),
+              UnorderedElementsAre(
+                  Diag(Main.range(),
+                       "C++ requires a type specifier for all declarations",
+                       std::vector<std::string>{"/clangd-test/a.h:1:10",
+                                                "/clangd-test/c.h:4:7"})));
+}
+
+TEST(DiagsInHeaders, OnlyErrorOrFatal) {
+  Annotations Main(R"cpp(
+    #include [["a.h"]]
+    void foo() {})cpp");
+  std::string MainFile = testPath("main.cc");
+  llvm::StringMap<std::string> Files = {{MainFile, Main.code()},
+                                        {testPath("a.h"),
+                                         R"cpp(
+                                           no_type_spec;
+                                           int x = 5/0;
+                                         )cpp"}};
+  auto FS = buildTestFS(Files);
+
+  EXPECT_THAT(build(MainFile, Files).getDiagnostics(),
+              UnorderedElementsAre(
+                  Diag(Main.range(),
+                       "C++ requires a type specifier for all declarations",
+                       std::vector<std::string>{"/clangd-test/a.h:2:44"})));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
-