Index: clangd/ClangdUnit.cpp =================================================================== --- clangd/ClangdUnit.cpp +++ clangd/ClangdUnit.cpp @@ -14,11 +14,13 @@ #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" @@ -35,9 +37,12 @@ #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 #include @@ -237,6 +242,19 @@ const LangOptions &LangOpts; }; +// Get the range for the included header begining at Position. +Range positionToRange(const Position &Pos, + const std::vector &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) { Index: clangd/Diagnostics.h =================================================================== --- clangd/Diagnostics.h +++ clangd/Diagnostics.h @@ -14,6 +14,8 @@ #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 @@ -80,6 +82,9 @@ std::vector Notes; /// *Alternative* fixes for this diagnostic, one should be chosen. std::vector Fixes; + /// Stores strings in the form: "/path/a.h:3:4". 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; + // Counts diagnostics coming from a header in the main file. + llvm::DenseMap NumberOfDiagsAtLine; }; } // 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,35 @@ return false; } +void adjustDiagFromHeader(Diag &D, const clang::Diagnostic &Info, + const LangOptions &LangOpts) { + const SourceManager &SM = Info.getSourceManager(); + std::vector 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, @@ -164,6 +199,15 @@ 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)"; @@ -379,6 +423,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 +458,24 @@ 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(); } Index: unittests/clangd/DiagnosticsTests.cpp =================================================================== --- unittests/clangd/DiagnosticsTests.cpp +++ unittests/clangd/DiagnosticsTests.cpp @@ -9,6 +9,7 @@ #include "Annotations.h" #include "ClangdUnit.h" #include "SourceCode.h" +#include "TestFS.h" #include "TestIndex.h" #include "TestTU.h" #include "index/MemIndex.h" @@ -45,6 +46,17 @@ 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 + "]") { @@ -73,7 +85,6 @@ return true; } - // Helper function to make tests shorter. Position pos(int line, int character) { Position Res; @@ -251,6 +262,8 @@ 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"; @@ -282,7 +295,8 @@ // 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 @@ -603,7 +617,185 @@ "Add include \"x.h\" for symbol a::X"))))); } +ParsedAST build(const std::string &MainFile, + const llvm::StringMap &Files) { + std::vector 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(); + 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 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{"/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 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{"/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 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{"/clangd-test/a.h:1:1"}), + Diag(Main.range("b"), + "C++ requires a type specifier for all declarations", + std::vector{"/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 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{"/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 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{"/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 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{"/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 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{"/clangd-test/a.h:2:44"}))); +} + } // namespace } // namespace clangd } // namespace clang -