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" @@ -34,9 +36,11 @@ #include "clang/Serialization/ASTWriter.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/ScopedPrinter.h" #include "llvm/Support/raw_ostream.h" #include #include @@ -45,6 +49,21 @@ namespace clangd { namespace { +static constexpr int MaxDiagsPerIncludeDirective = 10; + +bool mentionsMainFile(const Diag &D) { + if (D.InsideMainFile) + return true; + // Fixes are always in the main file. + if (!D.Fixes.empty()) + return true; + for (auto &N : D.Notes) { + if (N.InsideMainFile) + return true; + } + return false; +} + bool compileCommandsAreEqual(const tooling::CompileCommand &LHS, const tooling::CompileCommand &RHS) { // We don't check for Output, it should not matter to clangd. @@ -236,6 +255,15 @@ const LangOptions &LangOpts; }; +// Generates a mapping from start of an include directive to its range. +std::map +positionToRangeMap(const std::vector &MainFileIncludes) { + std::map RangeMap; + for (const Inclusion &Inc : MainFileIncludes) + RangeMap[Inc.R.start] = Inc.R; + return RangeMap; +} + } // namespace void dumpAST(ParsedAST &AST, llvm::raw_ostream &OS) { @@ -371,14 +399,49 @@ // So just inform the preprocessor of EOF, while keeping everything alive. Clang->getPreprocessor().EndSourceFile(); - std::vector Diags = ASTDiags.take(); + llvm::DenseMap NumberOfDiagsAtLine; + auto ShouldAddDiag = [&NumberOfDiagsAtLine](const Diag &D) { + if (mentionsMainFile(D)) + return true; + if (D.Severity != DiagnosticsEngine::Level::Error && + D.Severity != DiagnosticsEngine::Level::Fatal) + return false; + if (NumberOfDiagsAtLine[D.IncludeDirective->line] < + MaxDiagsPerIncludeDirective) { + ++NumberOfDiagsAtLine[D.IncludeDirective->line]; + return true; + } + return false; + }; + + std::vector Diags; + for (const Diag &D : ASTDiags.take()) { + if (ShouldAddDiag(D)) + Diags.push_back(D); + } // Populate diagnostic source. for (auto &D : Diags) D.S = !CTContext->getCheckName(D.ID).empty() ? Diag::ClangTidy : Diag::Clang; // Add diagnostics from the preamble, if any. - if (Preamble) - Diags.insert(Diags.begin(), Preamble->Diags.begin(), Preamble->Diags.end()); + if (Preamble) { + for (const Diag &D : Preamble->Diags) { + if (ShouldAddDiag(D)) + Diags.push_back(D); + } + } + + // Remap diagnostics from headers into their include location inside main + // file. + const auto RangeMap = positionToRangeMap(Includes.MainFileIncludes); + for (auto &D : Diags) { + if (!mentionsMainFile(D)) { + assert(D.IncludeDirective && + "Diagnostic is not mentioned in mainfile but also not imported?"); + D.Range = RangeMap.at(*D.IncludeDirective); + D.File = MainInput.getFile(); + } + } return ParsedAST(std::move(Preamble), std::move(Clang), std::move(Action), std::move(ParsedDecls), std::move(Diags), std::move(Includes), std::move(CanonIncludes)); 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; + /// If diagnostics is coming outside main file, points to relevant include + /// directive, otherwise None. + llvm::Optional IncludeDirective = llvm::None; }; llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Diag &D); Index: clangd/Diagnostics.cpp =================================================================== --- clangd/Diagnostics.cpp +++ clangd/Diagnostics.cpp @@ -10,30 +10,19 @@ #include "Compiler.h" #include "Logger.h" #include "SourceCode.h" +#include "clang/Basic/Diagnostic.h" +#include "clang/Basic/SourceLocation.h" #include "clang/Basic/SourceManager.h" #include "clang/Lex/Lexer.h" #include "llvm/Support/Capacity.h" #include "llvm/Support/Path.h" +#include "llvm/Support/Signals.h" #include namespace clang { namespace clangd { namespace { - -bool mentionsMainFile(const Diag &D) { - if (D.InsideMainFile) - return true; - // Fixes are always in the main file. - if (!D.Fixes.empty()) - return true; - for (auto &N : D.Notes) { - if (N.InsideMainFile) - return true; - } - return false; -} - // 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, @@ -379,6 +368,38 @@ LastDiag = Diag(); LastDiag->ID = Info.getID(); FillDiagBase(*LastDiag); + auto IncludeLocation = Info.getSourceManager() + .getPresumedLoc(Info.getLocation(), + /*UseLineDirectives=*/false) + .getIncludeLoc(); + std::vector IncludeStack; + while (IncludeLocation.isValid()) { + LastDiag->IncludeDirective = + sourceLocToPosition(Info.getSourceManager(), IncludeLocation); + IncludeStack.push_back( + IncludeLocation.printToString(Info.getSourceManager())); + IncludeLocation = Info.getSourceManager() + .getPresumedLoc(IncludeLocation, + /*UseLineDirectives=*/false) + .getIncludeLoc(); + } + if (LastDiag->IncludeDirective) { + std::string Message; + { + llvm::raw_string_ostream OS(Message); + // 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 (llvm::StringRef Inc : IncludeStack) + OS << "In file included from: " << Inc << ":\n"; + } + OS << Info.getLocation().printToString(Info.getSourceManager()) << ": " + << LastDiag->Message; + } + LastDiag->Message = std::move(Message); + } if (!Info.getFixItHints().empty()) AddFix(true /* try to invent a message instead of repeating the diag */); @@ -413,11 +434,7 @@ void StoreDiags::flushLastDiag() { if (!LastDiag) return; - if (mentionsMainFile(*LastDiag)) - Output.push_back(std::move(*LastDiag)); - else - vlog("Dropped diagnostic outside main file: {0}: {1}", LastDiag->File, - LastDiag->Message); + Output.push_back(std::move(*LastDiag)); 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" @@ -73,7 +74,6 @@ return true; } - // Helper function to make tests shorter. Position pos(int line, int character) { Position Res; @@ -603,7 +603,177 @@ "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, PCHs, + /*StoreInMemory=*/true, /*PreambleCallback=*/nullptr); + auto AST = buildAST(MainFile, createInvocationFromCommandLine(Cmd), Inputs, + Preamble, PCHs); + 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(), "/clangd-test/a.h:1:1: C++ requires a " + "type specifier for all declarations"))); +} + +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(), + "In file included from: " + "/clangd-test/a.h:1:10:\n/clangd-test/b.h:1:1: 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"); + 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"), "/clangd-test/a.h:1:1: C++ requires a type " + "specifier for all declarations"), + Diag(Main.range("b"), "/clangd-test/b.h:1:1: C++ requires a type " + "specifier for all declarations"))); +} + +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(), + "In file included from: " + "/clangd-test/a.h:1:10:\n/clangd-test/b.h:3:1: 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"); + 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(), + "In file included from: " + "/clangd-test/b.h:1:10:\n/clangd-test/c.h:3:1: C++ " + "requires a type specifier for all declarations"))); +} + +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_EQ(build(MainFile, Files).getDiagnostics().size(), 10U); +} + +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(), "/clangd-test/a.h:2:44: C++ requires a " + "type specifier for all declarations"))); +} + } // namespace } // namespace clangd } // namespace clang -