[clangd] Surface diagnostics from headers inside main file

Reviewers: ioeric, ilya-biryukov

Subscribers: MaskRay, jkorous, arphaman, jdoerfert, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D59302

llvm-svn: 359432

5 files changed, 198 insertions, 19 deletions

diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp
index a734f233c8f..c004fa3283d 100644
--- a/clang-tools-extra/clangd/Diagnostics.cpp
+++ b/clang-tools-extra/clangd/Diagnostics.cpp
@@ -13,11 +13,18 @@
 #include "Protocol.h"
 #include "SourceCode.h"
 #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 "llvm/ADT/ArrayRef.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 <algorithm>
 
 namespace clang {
@@ -102,6 +109,39 @@ Range diagnosticRange(const clang::Diagnostic &D, const LangOptions &L) {
   return halfOpenToRange(M, R);
 }
 
+void adjustDiagFromHeader(Diag &D, const clang::Diagnostic &Info,
+                          const LangOptions &LangOpts) {
+  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;
+
+  // 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));
+
+  // 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();
+}
+
 bool isInsideMainFile(const SourceLocation Loc, const SourceManager &M) {
   return Loc.isValid() && M.isWrittenInMainFile(M.getFileLoc(Loc));
 }
@@ -378,7 +418,7 @@ std::vector<Diag> StoreDiags::take(const clang::tidy::ClangTidyContext *Tidy) {
             Msg.resize(Rest.size());
         };
         CleanMessage(Diag.Message);
-        for (auto& Note : Diag.Notes)
+        for (auto &Note : Diag.Notes)
           CleanMessage(Note.Message);
         continue;
       }
@@ -477,6 +517,7 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
     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 */);
@@ -511,11 +552,15 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
 void StoreDiags::flushLastDiag() {
   if (!LastDiag)
     return;
-  if (mentionsMainFile(*LastDiag))
+  // Only keeps diagnostics inside main file or the first one coming from a
+  // header.
+  if (mentionsMainFile(*LastDiag) ||
+      (LastDiag->Severity >= DiagnosticsEngine::Level::Error &&
+       IncludeLinesWithErrors.insert(LastDiag->Range.start.line).second)) {
     Output.push_back(std::move(*LastDiag));
-  else
-    vlog("Dropped diagnostic outside main file: {0}: {1}", LastDiag->File,
-         LastDiag->Message);
+  } else {
+    vlog("Dropped diagnostic: {0}: {1}", LastDiag->File, LastDiag->Message);
+  }
   LastDiag.reset();
 }
 
diff --git a/clang-tools-extra/clangd/Diagnostics.h b/clang-tools-extra/clangd/Diagnostics.h
index c8045807cb6..a0ab7c664fd 100644
--- a/clang-tools-extra/clangd/Diagnostics.h
+++ b/clang-tools-extra/clangd/Diagnostics.h
@@ -14,6 +14,9 @@
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/LangOptions.h"
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringSet.h"
 #include <cassert>
@@ -135,6 +138,7 @@ private:
   std::vector<Diag> Output;
   llvm::Optional<LangOptions> LangOpts;
   llvm::Optional<Diag> LastDiag;
+  llvm::DenseSet<int> IncludeLinesWithErrors;
 };
 
 } // namespace clangd
diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
index a742c9d6206..ed12897e08d 100644
--- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -9,10 +9,11 @@
 #include "Annotations.h"
 #include "ClangdUnit.h"
 #include "Diagnostics.h"
+#include "Path.h"
 #include "Protocol.h"
 #include "SourceCode.h"
-#include "TestIndex.h"
 #include "TestFS.h"
+#include "TestIndex.h"
 #include "TestTU.h"
 #include "index/MemIndex.h"
 #include "clang/Basic/Diagnostic.h"
@@ -20,6 +21,7 @@
 #include "llvm/Support/ScopedPrinter.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include <algorithm>
 
 namespace clang {
 namespace clangd {
@@ -61,7 +63,8 @@ MATCHER_P(EqualToLSPDiag, LSPDiag,
           "LSP diagnostic " + llvm::to_string(LSPDiag)) {
   if (toJSON(arg) != toJSON(LSPDiag)) {
     *result_listener << llvm::formatv("expected:\n{0:2}\ngot\n{1:2}",
-                                      toJSON(LSPDiag), toJSON(arg)).str();
+                                      toJSON(LSPDiag), toJSON(arg))
+                            .str();
     return false;
   }
   return true;
@@ -83,7 +86,6 @@ MATCHER_P(EqualToFix, Fix, "LSP fix " + llvm::to_string(Fix)) {
   return true;
 }
 
-
 // Helper function to make tests shorter.
 Position pos(int line, int character) {
   Position Res;
@@ -114,8 +116,7 @@ o]]();
           // This range spans lines.
           AllOf(Diag(Test.range("typo"),
                      "use of undeclared identifier 'goo'; did you mean 'foo'?"),
-                DiagSource(Diag::Clang),
-                DiagName("undeclared_var_use_suggest"),
+                DiagSource(Diag::Clang), DiagName("undeclared_var_use_suggest"),
                 WithFix(
                     Fix(Test.range("typo"), "foo", "change 'go\\ o' to 'foo'")),
                 // This is a pretty normal range.
@@ -301,7 +302,8 @@ TEST(DiagnosticsTest, ToLSP) {
   MainLSP.severity = getSeverity(DiagnosticsEngine::Error);
   MainLSP.code = "enum_class_reference";
   MainLSP.source = "clang";
-  MainLSP.message = R"(Something terrible happened (fix available)
+  MainLSP.message =
+      R"(Something terrible happened (fix available)
 
 main.cpp:6:7: remark: declared somewhere in the main file
 
@@ -354,9 +356,8 @@ main.cpp:2:3: error: something terrible happened)";
   NoteInHeaderDRI.location.range = NoteInHeader.Range;
   NoteInHeaderDRI.location.uri = HeaderFile;
   MainLSP.relatedInformation = {NoteInMainDRI, NoteInHeaderDRI};
-  EXPECT_THAT(
-      LSPDiags,
-      ElementsAre(Pair(EqualToLSPDiag(MainLSP), ElementsAre(EqualToFix(F)))));
+  EXPECT_THAT(LSPDiags, ElementsAre(Pair(EqualToLSPDiag(MainLSP),
+                                         ElementsAre(EqualToFix(F)))));
 }
 
 struct SymbolWithHeader {
@@ -646,7 +647,124 @@ namespace c {
                               "Add include \"x.h\" for symbol a::X")))));
 }
 
+TEST(DiagsInHeaders, DiagInsideHeader) {
+  Annotations Main(R"cpp(
+    #include [["a.h"]]
+    void foo() {})cpp");
+  Annotations Header("[[no_type_spec]];");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.AdditionalFiles = {{"a.h", Header.code()}};
+  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(DiagsInHeaders, DiagInTransitiveInclude) {
+  Annotations Main(R"cpp(
+    #include [["a.h"]]
+    void foo() {})cpp");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.AdditionalFiles = {{"a.h", "#include \"b.h\""}, {"b.h", "no_type_spec;"}};
+  EXPECT_THAT(TU.build().getDiagnostics(),
+              UnorderedElementsAre(
+                  Diag(Main.range(), "in included file: C++ requires a "
+                                     "type specifier for all declarations")));
+}
+
+TEST(DiagsInHeaders, DiagInMultipleHeaders) {
+  Annotations Main(R"cpp(
+    #include $a[["a.h"]]
+    #include $b[["b.h"]]
+    void foo() {})cpp");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.AdditionalFiles = {{"a.h", "no_type_spec;"}, {"b.h", "no_type_spec;"}};
+  EXPECT_THAT(TU.build().getDiagnostics(),
+              UnorderedElementsAre(
+                  Diag(Main.range("a"), "in included file: C++ requires a type "
+                                        "specifier for all declarations"),
+                  Diag(Main.range("b"), "in included file: C++ requires a type "
+                                        "specifier for all declarations")));
+}
+
+TEST(DiagsInHeaders, PreferExpansionLocation) {
+  Annotations Main(R"cpp(
+    #include [["a.h"]]
+    #include "b.h"
+    void foo() {})cpp");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.AdditionalFiles = {{"a.h", "#include \"b.h\"\n"},
+                        {"b.h", "#ifndef X\n#define X\nno_type_spec;\n#endif"}};
+  EXPECT_THAT(TU.build().getDiagnostics(),
+              UnorderedElementsAre(Diag(Main.range(),
+                                        "in included file: C++ requires a type "
+                                        "specifier for all declarations")));
+}
+
+TEST(DiagsInHeaders, PreferExpansionLocationMacros) {
+  Annotations Main(R"cpp(
+    #define X
+    #include "a.h"
+    #undef X
+    #include [["b.h"]]
+    void foo() {})cpp");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.AdditionalFiles = {{"a.h", "#include \"c.h\"\n"},
+                        {"b.h", "#include \"c.h\"\n"},
+                        {"c.h", "#ifndef X\n#define X\nno_type_spec;\n#endif"}};
+  EXPECT_THAT(TU.build().getDiagnostics(),
+              UnorderedElementsAre(
+                  Diag(Main.range(), "in included file: C++ requires a "
+                                     "type specifier for all declarations")));
+}
+
+TEST(DiagsInHeaders, LimitDiagsOutsideMainFile) {
+  Annotations Main(R"cpp(
+    #include [["a.h"]]
+    #include "b.h"
+    void foo() {})cpp");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.AdditionalFiles = {{"a.h", "#include \"c.h\"\n"},
+                        {"b.h", "#include \"c.h\"\n"},
+                        {"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"}};
+  EXPECT_THAT(TU.build().getDiagnostics(),
+              UnorderedElementsAre(
+                  Diag(Main.range(), "in included file: C++ requires a "
+                                     "type specifier for all declarations")));
+}
+
+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")))));
+}
 } // namespace
+
 } // namespace clangd
 } // namespace clang
-
diff --git a/clang-tools-extra/clangd/unittests/TestTU.cpp b/clang-tools-extra/clangd/unittests/TestTU.cpp
index 05c7fbf8bf4..8f48eab537e 100644
--- a/clang-tools-extra/clangd/unittests/TestTU.cpp
+++ b/clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -21,11 +21,17 @@ ParsedAST TestTU::build() const {
   std::string FullFilename = testPath(Filename),
               FullHeaderName = testPath(HeaderFilename),
               ImportThunk = testPath("import_thunk.h");
-  std::vector<const char *> Cmd = {"clang", FullFilename.c_str()};
   // We want to implicitly include HeaderFilename without messing up offsets.
   // -include achieves this, but sometimes we want #import (to simulate a header
   // guard without messing up offsets). In this case, use an intermediate file.
   std::string ThunkContents = "#import \"" + FullHeaderName + "\"\n";
+
+  llvm::StringMap<std::string> Files(AdditionalFiles);
+  Files[FullFilename] = Code;
+  Files[FullHeaderName] = HeaderCode;
+  Files[ImportThunk] = ThunkContents;
+
+  std::vector<const char *> Cmd = {"clang", FullFilename.c_str()};
   // FIXME: this shouldn't need to be conditional, but it breaks a
   // GoToDefinition test for some reason (getMacroArgExpandedLocation fails).
   if (!HeaderCode.empty()) {
@@ -39,9 +45,7 @@ ParsedAST TestTU::build() const {
   Inputs.CompileCommand.CommandLine = {Cmd.begin(), Cmd.end()};
   Inputs.CompileCommand.Directory = testRoot();
   Inputs.Contents = Code;
-  Inputs.FS = buildTestFS({{FullFilename, Code},
-                           {FullHeaderName, HeaderCode},
-                           {ImportThunk, ThunkContents}});
+  Inputs.FS = buildTestFS(Files);
   Inputs.Opts = ParseOptions();
   Inputs.Opts.ClangTidyOpts.Checks = ClangTidyChecks;
   Inputs.Index = ExternalIndex;
diff --git a/clang-tools-extra/clangd/unittests/TestTU.h b/clang-tools-extra/clangd/unittests/TestTU.h
index 0f5951695f7..6ac4c86a59b 100644
--- a/clang-tools-extra/clangd/unittests/TestTU.h
+++ b/clang-tools-extra/clangd/unittests/TestTU.h
@@ -18,8 +18,13 @@
 #define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTTU_H
 
 #include "ClangdUnit.h"
+#include "Path.h"
 #include "index/Index.h"
+#include "llvm/ADT/StringMap.h"
 #include "gtest/gtest.h"
+#include <string>
+#include <utility>
+#include <vector>
 
 namespace clang {
 namespace clangd {
@@ -45,6 +50,9 @@ struct TestTU {
   std::string HeaderCode;
   std::string HeaderFilename = "TestTU.h";
 
+  // Name and contents of each file.
+  llvm::StringMap<std::string> AdditionalFiles;
+
   // Extra arguments for the compiler invocation.
   std::vector<const char *> ExtraArgs;