Merging r367303:

zmodem · zmodem · commit fcfd891f22aa · 2019-08-13T12:37:46.000Z
------------------------------------------------------------------------ r367303 | kadircet | 2019-07-30 12:26:51 +0200 (Tue, 30 Jul 2019) | 15 lines [clangd] Ignore diags from builtin files Summary: This fixes a case where we show diagnostics on arbitrary lines, in an internal codebase. Open for ideas on unittesting this. Reviewers: ilya-biryukov Subscribers: MaskRay, jkorous, arphaman, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D64863 ------------------------------------------------------------------------ llvm-svn: 368682
diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp
@@ -20,6 +20,7 @@
 #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"
@@ -107,8 +108,13 @@ Range diagnosticRange(const clang::Diagnostic &D, const LangOptions &L) {
   return halfOpenToRange(M, R);
 }
 
-void adjustDiagFromHeader(Diag &D, const clang::Diagnostic &Info,
+// Returns whether the \p D is modified.
+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;
@@ -119,13 +125,14 @@ void adjustDiagFromHeader(Diag &D, const clang::Diagnostic &Info,
        IncludeLocation = GetIncludeLoc(IncludeLocation))
     IncludeInMainFile = IncludeLocation;
   if (IncludeInMainFile.isInvalid())
-    return;
+    return false;
 
   // 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.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));
@@ -138,6 +145,7 @@ void adjustDiagFromHeader(Diag &D, const clang::Diagnostic &Info,
 
   // Update message to mention original file.
   D.Message = llvm::Twine("in included file: ", D.Message).str();
+  return true;
 }
 
 bool isInsideMainFile(const clang::Diagnostic &D) {
@@ -465,15 +473,15 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
   }
 
   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;
@@ -496,19 +504,18 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
       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);
@@ -553,7 +560,9 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
     LastDiag = Diag();
     LastDiag->ID = Info.getID();
     FillDiagBase(*LastDiag);
-    adjustDiagFromHeader(*LastDiag, Info, *LangOpts);
+    LastDiagWasAdjusted = false;
+    if (!InsideMainFile)
+      LastDiagWasAdjusted = adjustDiagFromHeader(*LastDiag, Info, *LangOpts);
 
     if (!Info.getFixItHints().empty())
       AddFix(true /* try to invent a message instead of repeating the diag */);
@@ -595,10 +604,9 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
 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 each particular header.
        IncludeLinesWithErrors.insert(LastDiag->Range.start.line).second)) {
     Output.push_back(std::move(*LastDiag));
   } else {
diff --git a/clang-tools-extra/clangd/Diagnostics.h b/clang-tools-extra/clangd/Diagnostics.h
@@ -145,6 +145,8 @@ class StoreDiags : public DiagnosticConsumer {
   std::vector<Diag> Output;
   llvm::Optional<LangOptions> LangOpts;
   llvm::Optional<Diag> LastDiag;
+  /// Set iff adjustDiagFromHeader resulted in changes to LastDiag.
+  bool LastDiagWasAdjusted = false;
   llvm::DenseSet<int> IncludeLinesWithErrors;
   bool LastPrimaryDiagnosticWasSuppressed = false;
 };
diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -928,14 +928,26 @@ TEST(DiagsInHeaders, OnlyErrorOrFatal) {
     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
