Index: clangd/Diagnostics.h =================================================================== --- clangd/Diagnostics.h +++ 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 @@ -80,6 +83,8 @@ std::vector Notes; /// *Alternative* fixes for this diagnostic, one should be chosen. std::vector Fixes; + /// The include closest to main file is in front. + std::vector> IncludeStack; }; llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Diag &D); @@ -123,6 +128,8 @@ std::vector Output; llvm::Optional LangOpts; llvm::Optional LastDiag; + /// Stores lines of the includes containing a diagnostic. + llvm::DenseSet IncludeHasDiag; }; } // namespace clangd Index: clangd/Diagnostics.cpp =================================================================== --- clangd/Diagnostics.cpp +++ clangd/Diagnostics.cpp @@ -10,10 +10,16 @@ #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 namespace clang { @@ -34,6 +40,47 @@ return false; } +void adjustDiagFromHeader(Diag &D, const clang::Diagnostic &Info, + const LangOptions &LangOpts) { + const SourceLocation &DiagLoc = Info.getLocation(); + const SourceManager &SM = Info.getSourceManager(); + std::vector IncludeStack; + auto GetIncludeLoc = [&SM](SourceLocation SLoc) { + return SM.getIncludeLoc(SM.getFileID(SLoc)); + }; + for (auto IncludeLocation = GetIncludeLoc(DiagLoc); IncludeLocation.isValid(); + IncludeLocation = GetIncludeLoc(IncludeLocation)) { + IncludeStack.push_back(IncludeLocation); + } + if (IncludeStack.empty()) + return; + 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( + {SM.getFileEntryForID(SM.getFileID(Inc))->getName().str(), + sourceLocToPosition(SM, Inc)}); + } + D.IncludeStack.push_back( + {SM.getFileEntryForID(SM.getFileID(DiagLoc))->getName().str(), + sourceLocToPosition(SM, DiagLoc)}); +} + +// Position stores line and character offset 0-based. But we want 1-based +// offsets when displaying that information to users. +Position toOneBased(Position P) { + ++P.line; + ++P.character; + return P; +} + // 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, @@ -131,8 +178,7 @@ } // Note +1 to line and character. clangd::Range is zero-based, but when // printing for users we want one-based indexes. - auto Pos = D.Range.start; - OS << (Pos.line + 1) << ":" << (Pos.character + 1) << ":"; + OS << toOneBased(D.Range.start) << ":"; // The non-main-file paths are often too long, putting them on a separate // line improves readability. if (D.InsideMainFile) @@ -164,9 +210,21 @@ std::string mainMessage(const Diag &D, bool DisplayFixesCount) { std::string Result; llvm::raw_string_ostream OS(Result); + // Prepend filename if diag is coming from a header. + if (!D.IncludeStack.empty()) { + const auto &Inc = D.IncludeStack.back(); + OS << "In file " << Inc.first << ':' << toOneBased(Inc.second) << ": "; + } OS << D.Message; if (DisplayFixesCount && !D.Fixes.empty()) OS << " (" << (D.Fixes.size() > 1 ? "fixes" : "fix") << " available)"; + // We don't want to print last file in the include stack, since it has already + // been prepended to D.Message. + for (size_t I = 0, E = D.IncludeStack.size(); I + 1 < E; ++I) { + const auto &Inc = D.IncludeStack[I]; + OS << "\nIn file included from: " << Inc.first << ':' + << toOneBased(Inc.second); + } for (auto &Note : D.Notes) { OS << "\n\n"; printDiag(OS, Note); @@ -379,6 +437,7 @@ 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 */); @@ -413,11 +472,19 @@ void StoreDiags::flushLastDiag() { if (!LastDiag) return; - if (mentionsMainFile(*LastDiag)) + auto IsImportantHeaderDiag = [](const Diag &D) { + return D.Severity == DiagnosticsEngine::Level::Error || + D.Severity == DiagnosticsEngine::Level::Fatal; + }; + // Only keeps diagnostics inside main file or the first one coming from a + // header. + if (mentionsMainFile(*LastDiag) || + (IsImportantHeaderDiag(*LastDiag) && + IncludeHasDiag.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(); } Index: unittests/clangd/DiagnosticsTests.cpp =================================================================== --- unittests/clangd/DiagnosticsTests.cpp +++ unittests/clangd/DiagnosticsTests.cpp @@ -8,13 +8,17 @@ #include "Annotations.h" #include "ClangdUnit.h" +#include "Path.h" +#include "Protocol.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" +#include namespace clang { namespace clangd { @@ -45,6 +49,15 @@ 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; + return std::equal(IncludeStack.begin(), IncludeStack.end(), + arg.IncludeStack.begin()); +} + MATCHER_P3(Fix, Range, Replacement, Message, "Fix " + llvm::to_string(Range) + " => " + testing::PrintToString(Replacement) + " = [" + Message + "]") { @@ -73,7 +86,6 @@ return true; } - // Helper function to make tests shorter. Position pos(int line, int character) { Position Res; @@ -251,6 +263,8 @@ D.InsideMainFile = true; D.Severity = DiagnosticsEngine::Error; D.File = "foo/bar/main.cpp"; + D.IncludeStack.push_back({"a/b.h", pos(0, 1)}); + D.IncludeStack.push_back({"a/c.h", pos(1, 1)}); clangd::Note NoteInMain; NoteInMain.Message = "declared somewhere in the main file"; @@ -281,8 +295,9 @@ }; // Diagnostics should turn into these: - clangd::Diagnostic MainLSP = - MatchingLSP(D, R"(Something terrible happened (fix available) + clangd::Diagnostic MainLSP = MatchingLSP( + D, R"(In file a/c.h:2:2: something terrible happened (fix available) +In file included from: a/b.h:1:2 main.cpp:6:7: remark: declared somewhere in the main file @@ -603,7 +618,144 @@ "Add include \"x.h\" for symbol a::X"))))); } +TEST(DiagsInHeaders, DiagInsideHeader) { + Annotations Main(R"cpp( + #include [["a.h"]] + void foo() {})cpp"); + TestTU TU = TestTU::withCode(Main.code()); + TU.AdditionalFiles = {{testPath("a.h"), "no_type_spec;"}}; + EXPECT_THAT(TU.build().getDiagnostics(), + UnorderedElementsAre( + Diag(Main.range(), + "C++ requires a type specifier for all declarations", + std::vector>{ + {"/clangd-test/a.h", pos(0, 0)}}))); +} + +TEST(DiagsInHeaders, DiagInTransitiveInclude) { + Annotations Main(R"cpp( + #include [["a.h"]] + void foo() {})cpp"); + TestTU TU = TestTU::withCode(Main.code()); + TU.AdditionalFiles = {{testPath("a.h"), "#include \"b.h\""}, + {testPath("b.h"), "no_type_spec;"}}; + EXPECT_THAT(TU.build().getDiagnostics(), + UnorderedElementsAre( + Diag(Main.range(), + "C++ requires a type specifier for all declarations", + std::vector>{ + {"/clangd-test/a.h", pos(0, 9)}, + {"/clangd-test/b.h", pos(0, 0)}}))); +} + +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 = {{testPath("a.h"), "no_type_spec;"}, + {testPath("b.h"), "no_type_spec;"}}; + EXPECT_THAT(TU.build().getDiagnostics(), + UnorderedElementsAre( + Diag(Main.range("a"), + "C++ requires a type specifier for all declarations", + std::vector>{ + {"/clangd-test/a.h", pos(0, 0)}}), + Diag(Main.range("b"), + "C++ requires a type specifier for all declarations", + std::vector>{ + {"/clangd-test/b.h", pos(0, 0)}}))); +} + +TEST(DiagsInHeaders, PreferExpansionLocation) { + Annotations Main(R"cpp( + #include [["a.h"]] + #include "b.h" + void foo() {})cpp"); + TestTU TU = TestTU::withCode(Main.code()); + TU.AdditionalFiles = { + {testPath("a.h"), "#include \"b.h\"\n"}, + {testPath("b.h"), "#ifndef X\n#define X\nno_type_spec;\n#endif"}}; + EXPECT_THAT(TU.build().getDiagnostics(), + UnorderedElementsAre( + Diag(Main.range(), + "C++ requires a type specifier for all declarations", + std::vector>{ + {"/clangd-test/a.h", pos(0, 9)}, + {"/clangd-test/b.h", pos(2, 0)}}))); +} + +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 = { + {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"}}; + EXPECT_THAT(TU.build().getDiagnostics(), + UnorderedElementsAre( + Diag(Main.range(), + "C++ requires a type specifier for all declarations", + std::vector>{ + {"/clangd-test/b.h", pos(0, 9)}, + {"/clangd-test/c.h", pos(2, 0)}}))); +} + +TEST(DiagsInHeaders, LimitDiagsOutsideMainFile) { + Annotations Main(R"cpp( + #include [["a.h"]] + #include "b.h" + void foo() {})cpp"); + TestTU TU = TestTU::withCode(Main.code()); + TU.AdditionalFiles = {{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"}}; + EXPECT_THAT(TU.build().getDiagnostics(), + UnorderedElementsAre( + Diag(Main.range(), + "C++ requires a type specifier for all declarations", + std::vector>{ + {"/clangd-test/a.h", pos(0, 9)}, + {"/clangd-test/c.h", pos(3, 6)}}))); +} + +TEST(DiagsInHeaders, OnlyErrorOrFatal) { + Annotations Main(R"cpp( + #include [["a.h"]] + void foo() {})cpp"); + TestTU TU = TestTU::withCode(Main.code()); + TU.AdditionalFiles = {{testPath("a.h"), + R"cpp(no_type_spec; + int x = 5/0;)cpp"}}; + auto diags = TU.build().getDiagnostics(); + EXPECT_THAT(TU.build().getDiagnostics(), + UnorderedElementsAre( + Diag(Main.range(), + "C++ requires a type specifier for all declarations", + std::vector>{ + {"/clangd-test/a.h", pos(0, 0)}}))); +} + } // namespace } // namespace clangd } // namespace clang - Index: unittests/clangd/TestTU.h =================================================================== --- unittests/clangd/TestTU.h +++ unittests/clangd/TestTU.h @@ -18,8 +18,11 @@ #define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTTU_H #include "ClangdUnit.h" +#include "Path.h" #include "index/Index.h" #include "gtest/gtest.h" +#include +#include namespace clang { namespace clangd { @@ -45,6 +48,9 @@ std::string HeaderCode; std::string HeaderFilename = "TestTU.h"; + // Absolute path and contents of each file. + std::vector> AdditionalFiles; + // Extra arguments for the compiler invocation. std::vector ExtraArgs; Index: unittests/clangd/TestTU.cpp =================================================================== --- unittests/clangd/TestTU.cpp +++ unittests/clangd/TestTU.cpp @@ -20,6 +20,13 @@ ParsedAST TestTU::build() const { std::string FullFilename = testPath(Filename), FullHeaderName = testPath(HeaderFilename); + + llvm::StringMap Files; + Files[FullFilename] = Code; + Files[FullHeaderName] = HeaderCode; + for (const auto &Entry : AdditionalFiles) + Files[Entry.first] = Entry.second; + std::vector Cmd = {"clang", FullFilename.c_str()}; // FIXME: this shouldn't need to be conditional, but it breaks a // GoToDefinition test for some reason (getMacroArgExpandedLocation fails). @@ -33,7 +40,7 @@ Inputs.CompileCommand.CommandLine = {Cmd.begin(), Cmd.end()}; Inputs.CompileCommand.Directory = testRoot(); Inputs.Contents = Code; - Inputs.FS = buildTestFS({{FullFilename, Code}, {FullHeaderName, HeaderCode}}); + Inputs.FS = buildTestFS(Files); Inputs.Opts = ParseOptions(); Inputs.Opts.ClangTidyOpts.Checks = ClangTidyChecks; Inputs.Index = ExternalIndex;