[clangd] Ignore diags from builtin files

[kadircet]

Summary

This fixes a case where we show diagnostics on arbitrary lines, in an
internal codebase.

Open for ideas on unittesting this.

[ilya-biryukov]

Thanks for switching to SM everywhere, makes the code much more readable!

A rough proposal for testing this:

// test.cpp
// RUN: clang++ -DFOO=b ./test.cpp
int a = FOO;

This produces a note inside a <command line> buffer. We could probably have something similar pointing into a <builtin>

clang-tools-extra/clangd/Diagnostics.cpp

477

There is also at least isWrittenInCommandLineFile and isWrittenInScratchSpace.
We should probably handle both of them here too.

What are we actually checking here? That we can later create a URI for this file? Is there a good way to check exactly that without breaking layering?

[hokein]

clang-tools-extra/clangd/Diagnostics.cpp

477

shall we put this into the isInsideMainFile? I think the location written in CommandLineFile/ScratchSpace is not inside main file logically.

[ilya-biryukov]

clang-tools-extra/clangd/Diagnostics.cpp

477

Very good point.
@kadircet does isInsideMainFile returns true for locations from command line or scratch-pad?

[kadircet]

• Add tests.
• Change the layer we ignore the diags:
  • Mark diags from headers as insidemainfile when we decide to surface them.

[kadircet]

clang-tools-extra/clangd/Diagnostics.cpp

477

yes it does

[ilya-biryukov]

clang-tools-extra/clangd/Diagnostics.cpp

601

It's not clear why we these conditions should be checked somewhere else.
Could you explain in more detail?

[kadircet]

• Move deduplication logic back into the flushLastDiag as discussed offline.

[ilya-biryukov]

clang-tools-extra/clangd/Diagnostics.cpp

128

NIT: inline StartPos, it has online a single usage now.

563

We probably want to always set the value of this field to avoid accidentally reading the flag for the previous LastDiag

608

NIT: maybe be more specific? coming from each particular header?

clang-tools-extra/clangd/Diagnostics.h

148

NIT: add a comment that it was adjustDiagFromHeader that made the adjustment.

[kadircet]

• Address comments

[kadircet]

clang-tools-extra/clangd/Diagnostics.cpp

563

I don't follow, this is always set in line 475 at the beginning of HandleDiagnostic method.

[ilya-biryukov]

clang-tools-extra/clangd/Diagnostics.cpp

563

I might be missing something, but the only assignment to LastDiagWasAdjusted seems to be here (apart from initialization at the declaration site)

[kadircet]

• Always set LastDiagWasAdjusted.

clang-tools-extra/clangd/Diagnostics.cpp

563

oopsy, fixed

[ilya-biryukov]

LGTM. Many thanks for fixing this.

clang-tools-extra/clangd/Diagnostics.cpp

111

NIT: add a small comment mentioning that returned value indicates if Diag was changed

476

NIT: could use auto & here, the SourceManager is mentioned explicitly in the initializer anyway.

564

NIT: I suggest moving the control-flow (&&) into a separate if statement.
adjustDiagFromHeader mutates its arguments and having complex expressions in the same statement makes it a little hard to notice that things might get mutated there.

But up to you.

clang-tools-extra/clangd/Diagnostics.h

148

NIT: it's not adjustDiagFromHeader that actually sets the value, maybe rephrase to avoid confusion?
Something like

/// Set iff adjustDiagFromHeader resulted in changes to LastDiag.

[kadircet]

• Address comments

clang-tools-extra/clangd/Diagnostics.h

 private:
   void flushLastDiag();
 
   DiagFixer Fixer = nullptr;
   LevelAdjuster Adjuster = nullptr;
   std::vector<Diag> Output;
   llvm::Optional<LangOptions> LangOpts;
   llvm::Optional<Diag> LastDiag;
+  bool LastDiagWasAdjusted = false;
   llvm::DenseSet<int> IncludeLinesWithErrors;
   bool LastPrimaryDiagnosticWasSuppressed = false;
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_DIAGNOSTICS_H

clang-tools-extra/clangd/Diagnostics.cpp

 #include "clang/Basic/AllDiagnostics.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticIDs.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Lex/Token.h"
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/Capacity.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
Range diagnosticRange(const clang::Diagnostic &D, const LangOptions &L) {
   auto R = CharSourceRange::getCharRange(Loc);
   Token Tok;
   if (!Lexer::getRawToken(Loc, Tok, M, L, true) && Tok.isNot(tok::comment)) {
     R = CharSourceRange::getTokenRange(Tok.getLocation(), Tok.getEndLoc());
   }
   return halfOpenToRange(M, R);
 }
 
-void adjustDiagFromHeader(Diag &D, const clang::Diagnostic &Info,
+bool adjustDiagFromHeader(Diag &D, const clang::Diagnostic &Info,
                           const LangOptions &LangOpts) {
+  // We only report diagnostics with at least error severity from headers.
+  if (D.Severity < DiagnosticsEngine::Level::Error)
+    return false;
+
   const SourceLocation &DiagLoc = Info.getLocation();
   const SourceManager &SM = Info.getSourceManager();
   SourceLocation IncludeInMainFile;
   auto GetIncludeLoc = [&SM](SourceLocation SLoc) {
     return SM.getIncludeLoc(SM.getFileID(SLoc));
   };
   for (auto IncludeLocation = GetIncludeLoc(DiagLoc); IncludeLocation.isValid();
        IncludeLocation = GetIncludeLoc(IncludeLocation))
     IncludeInMainFile = IncludeLocation;
   if (IncludeInMainFile.isInvalid())
-    return;
+    return false;
+  Position StartPos = sourceLocToPosition(SM, IncludeInMainFile);
 
   // Update diag to point at include inside main file.
   D.File = SM.getFileEntryForID(SM.getMainFileID())->getName().str();
-  D.Range.start = sourceLocToPosition(SM, IncludeInMainFile);
+  D.Range.start = std::move(StartPos);
   D.Range.end = sourceLocToPosition(
       SM, Lexer::getLocForEndOfToken(IncludeInMainFile, 0, SM, LangOpts));
+  D.InsideMainFile = true;
 
   // Add a note that will point to real diagnostic.
   const auto *FE = SM.getFileEntryForID(SM.getFileID(DiagLoc));
   D.Notes.emplace_back();
   Note &N = D.Notes.back();
   N.AbsFile = FE->tryGetRealPathName();
   N.File = FE->getName();
   N.Message = "error occurred here";
   N.Range = diagnosticRange(Info, LangOpts);
 
   // Update message to mention original file.
   D.Message = llvm::Twine("in included file: ", D.Message).str();
+  return true;
 }
 
 bool isInsideMainFile(const clang::Diagnostic &D) {
   if (!D.hasSourceManager())
     return false;
 
   return clangd::isInsideMainFile(D.getLocation(), D.getSourceManager());
 }
void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
   DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info);
 
   if (!LangOpts || !Info.hasSourceManager()) {
     IgnoreDiagnostics::log(DiagLevel, Info);
     return;
   }
 
   bool InsideMainFile = isInsideMainFile(Info);
+  SourceManager &SM = Info.getSourceManager();
 
   auto FillDiagBase = [&](DiagBase &D) {
     D.Range = diagnosticRange(Info, *LangOpts);
     llvm::SmallString<64> Message;
     Info.FormatDiagnostic(Message);
     D.Message = Message.str();
     D.InsideMainFile = InsideMainFile;
-    D.File = Info.getSourceManager().getFilename(Info.getLocation());
-    auto &SM = Info.getSourceManager();
+    D.File = SM.getFilename(Info.getLocation());
     D.AbsFile = getCanonicalPath(
         SM.getFileEntryForID(SM.getFileID(Info.getLocation())), SM);
     D.Severity = DiagLevel;
     D.Category = DiagnosticIDs::getCategoryNameFromID(
                      DiagnosticIDs::getCategoryNumberForDiag(Info.getID()))
                      .str();
     return D;
   };
 
   auto AddFix = [&](bool SyntheticMessage) -> bool {
     assert(!Info.getFixItHints().empty() &&
            "diagnostic does not have attached fix-its");
     if (!InsideMainFile)
       return false;
 
     llvm::SmallVector<TextEdit, 1> Edits;
     for (auto &FixIt : Info.getFixItHints()) {
       // Follow clang's behavior, don't apply FixIt to the code in macros,
       // we are less certain it is the right fix.
       if (FixIt.RemoveRange.getBegin().isMacroID() ||
           FixIt.RemoveRange.getEnd().isMacroID())
         return false;
-      if (!isInsideMainFile(FixIt.RemoveRange.getBegin(),
-                            Info.getSourceManager()))
+      if (!isInsideMainFile(FixIt.RemoveRange.getBegin(), SM))
         return false;
-      Edits.push_back(toTextEdit(FixIt, Info.getSourceManager(), *LangOpts));
+      Edits.push_back(toTextEdit(FixIt, SM, *LangOpts));
     }
 
     llvm::SmallString<64> Message;
     // If requested and possible, create a message like "change 'foo' to 'bar'".
     if (SyntheticMessage && Info.getNumFixItHints() == 1) {
       const auto &FixIt = Info.getFixItHint(0);
       bool Invalid = false;
-      llvm::StringRef Remove = Lexer::getSourceText(
-          FixIt.RemoveRange, Info.getSourceManager(), *LangOpts, &Invalid);
+      llvm::StringRef Remove =
+          Lexer::getSourceText(FixIt.RemoveRange, SM, *LangOpts, &Invalid);
       llvm::StringRef Insert = FixIt.CodeToInsert;
       if (!Invalid) {
         llvm::raw_svector_ostream M(Message);
         if (!Remove.empty() && !Insert.empty()) {
           M << "change '";
           writeCodeToFixMessage(M, Remove);
           M << "' to '";
           writeCodeToFixMessage(M, Insert);
if (Adjuster) {
         return;
       }
     }
     LastPrimaryDiagnosticWasSuppressed = false;
 
     LastDiag = Diag();
     LastDiag->ID = Info.getID();
     FillDiagBase(*LastDiag);
-    adjustDiagFromHeader(*LastDiag, Info, *LangOpts);
+    if (!InsideMainFile)
+      LastDiagWasAdjusted = 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());
     }
if (!Info.getFixItHints().empty()) {
       LastDiag->Notes.push_back(std::move(N));
     }
   }
 }
 
 void StoreDiags::flushLastDiag() {
   if (!LastDiag)
     return;
-  // Only keeps diagnostics inside main file or the first one coming from a
-  // header.
-  if (mentionsMainFile(*LastDiag) ||
-      (LastDiag->Severity >= DiagnosticsEngine::Level::Error &&
+  if (mentionsMainFile(*LastDiag) &&
+      (!LastDiagWasAdjusted ||
+       // Only report the first diagnostic coming from a header.
        IncludeLinesWithErrors.insert(LastDiag->Range.start.line).second)) {
     Output.push_back(std::move(*LastDiag));
   } else {
     vlog("Dropped diagnostic: {0}: {1}", LastDiag->File, LastDiag->Message);
   }
   LastDiag.reset();
 }
 
 } // namespace clangd
 } // namespace clang

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

TEST(DiagsInHeaders, OnlyErrorOrFatal) {
   Annotations Main(R"cpp(
     #include [["a.h"]]
     void foo() {})cpp");
   Annotations Header(R"cpp(
     [[no_type_spec]];
     int x = 5/0;)cpp");
   TestTU TU = TestTU::withCode(Main.code());
   TU.AdditionalFiles = {{"a.h", Header.code()}};
-  auto diags = TU.build().getDiagnostics();
   EXPECT_THAT(TU.build().getDiagnostics(),
               UnorderedElementsAre(AllOf(
                   Diag(Main.range(), "in included file: C++ requires "
                                      "a type specifier for all declarations"),
                   WithNote(Diag(Header.range(), "error occurred here")))));
 }
 
+TEST(IgnoreDiags, FromNonWrittenSources) {
+  Annotations Main(R"cpp(
+    #include [["a.h"]]
+    void foo() {})cpp");
+  Annotations Header(R"cpp(
+    int x = 5/0;
+    int b = [[FOO]];)cpp");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.AdditionalFiles = {{"a.h", Header.code()}};
+  TU.ExtraArgs = {"-DFOO=NOOO"};
+  EXPECT_THAT(TU.build().getDiagnostics(), UnorderedElementsAre());
+}
+
 } // namespace
 
 } // namespace clangd
 } // namespace clang