Index: clangd/ClangdServer.h =================================================================== --- clangd/ClangdServer.h +++ clangd/ClangdServer.h @@ -119,6 +119,8 @@ std::chrono::milliseconds(500); bool SuggestMissingIncludes = false; + + llvm::Optional LimitDiagsOutsideMainFile = llvm::None; }; // Sensible default options for use in tests. // Features like indexing must be enabled if desired. @@ -294,6 +296,10 @@ // can be caused by missing includes (e.g. member access in incomplete type). bool SuggestMissingIncludes = false; + // When set only that many number of diagnostics from headers will be surfaced + // in main file. + llvm::Optional LimitDiagsOutsideMainFile = llvm::None; + // GUARDED_BY(CachedCompletionFuzzyFindRequestMutex) llvm::StringMap> CachedCompletionFuzzyFindRequestByFile; Index: clangd/ClangdServer.cpp =================================================================== --- clangd/ClangdServer.cpp +++ clangd/ClangdServer.cpp @@ -113,6 +113,7 @@ : nullptr), ClangTidyOptProvider(Opts.ClangTidyOptProvider), SuggestMissingIncludes(Opts.SuggestMissingIncludes), + LimitDiagsOutsideMainFile(Opts.LimitDiagsOutsideMainFile), WorkspaceRoot(Opts.WorkspaceRoot), PCHs(std::make_shared()), // Pass a callback into `WorkScheduler` to extract symbols from a newly @@ -153,6 +154,7 @@ if (ClangTidyOptProvider) Opts.ClangTidyOpts = ClangTidyOptProvider->getOptions(File); Opts.SuggestMissingIncludes = SuggestMissingIncludes; + Opts.LimitDiagsOutsideMainFile = LimitDiagsOutsideMainFile; // FIXME: some build systems like Bazel will take time to preparing // environment to build the file, it would be nice if we could emit a // "PreparingBuild" status to inform users, it is non-trivial given the 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" @@ -45,6 +47,19 @@ 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; +} + bool compileCommandsAreEqual(const tooling::CompileCommand &LHS, const tooling::CompileCommand &RHS) { // We don't check for Output, it should not matter to clangd. @@ -236,6 +251,20 @@ const LangOptions &LangOpts; }; +// Generates a mapping from resolved path of an include to appropriate include +// directive inside main file. +// If a header is transitively included in multiple direct includes of main, we +// choose the first one. +llvm::StringMap includesToRangeMap(const IncludeStructure &Includes) { + llvm::StringMap RangeMap; + for (const Inclusion &Inc : Includes.MainFileIncludes) { + RangeMap[Inc.Resolved] = Inc.R; + for (const auto &Entry : Includes.includeDepth(Inc.Resolved)) + RangeMap.try_emplace(Entry.getKey(), Inc.R); + } + return RangeMap; +} + } // namespace void dumpAST(ParsedAST &AST, llvm::raw_ostream &OS) { @@ -371,14 +400,44 @@ // So just inform the preprocessor of EOF, while keeping everything alive. Clang->getPreprocessor().EndSourceFile(); - std::vector Diags = ASTDiags.take(); + std::vector Diags; + int64_t DiagsOutsideMainFile = 0; + for (const Diag &D : ASTDiags.take()) { + if (!mentionsMainFile(D) && Opts.LimitDiagsOutsideMainFile && + ++DiagsOutsideMainFile > *Opts.LimitDiagsOutsideMainFile) + continue; + 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 (!mentionsMainFile(D)) { + if (D.Severity != DiagnosticsEngine::Level::Error && + D.Severity != DiagnosticsEngine::Level::Fatal) + continue; + if (Opts.LimitDiagsOutsideMainFile && + DiagsOutsideMainFile >= *Opts.LimitDiagsOutsideMainFile) + continue; + DiagsOutsideMainFile++; + } + Diags.push_back(D); + } + } + + // Remap diagnostics from headers into their include location inside main + // file. + const auto RangeMap = includesToRangeMap(Includes); + for (auto &D : Diags) { + if (!mentionsMainFile(D)) { + D.Range = RangeMap.lookup(D.File); + D.File = MainInput.getFile(); + D.Message = "Error in header: " + D.Message; + } + } 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/Compiler.h =================================================================== --- clangd/Compiler.h +++ clangd/Compiler.h @@ -21,6 +21,7 @@ #include "clang/Frontend/CompilerInvocation.h" #include "clang/Frontend/PrecompiledPreamble.h" #include "clang/Tooling/CompilationDatabase.h" +#include "llvm/ADT/Optional.h" namespace clang { namespace clangd { @@ -38,6 +39,7 @@ struct ParseOptions { tidy::ClangTidyOptions ClangTidyOpts; bool SuggestMissingIncludes = false; + llvm::Optional LimitDiagsOutsideMainFile; }; /// Information required to run clang, e.g. to parse AST or do code completion. Index: clangd/Diagnostics.cpp =================================================================== --- clangd/Diagnostics.cpp +++ clangd/Diagnostics.cpp @@ -14,26 +14,13 @@ #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, @@ -413,11 +400,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: clangd/tool/ClangdMain.cpp =================================================================== --- clangd/tool/ClangdMain.cpp +++ clangd/tool/ClangdMain.cpp @@ -219,6 +219,12 @@ "includes using index."), llvm::cl::init(true)); +static llvm::cl::opt LimitDiagsOutsideMainFile( + "limit-diags-outside-mainfile", + llvm::cl::desc("Attempts to fix diagnostic errors caused by missing " + "includes using index."), + llvm::cl::init(100), llvm::cl::Hidden); + namespace { /// \brief Supports a test URI scheme with relaxed constraints for lit tests. @@ -458,6 +464,7 @@ } Opts.ClangTidyOptProvider = ClangTidyOptProvider.get(); Opts.SuggestMissingIncludes = SuggestMissingIncludes; + Opts.LimitDiagsOutsideMainFile = LimitDiagsOutsideMainFile; ClangdLSPServer LSPServer( *TransportLayer, FSProvider, CCOpts, CompileCommandsDirPath, /*UseDirBasedCDB=*/CompileArgsFrom == FilesystemCompileArgs, Opts); 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,163 @@ "Add include \"x.h\" for symbol a::X"))))); } +ParsedAST +build(const std::string &MainFile, const llvm::StringMap &Files, + llvm::Optional LimitDiagsOutsideMainFile = llvm::None) { + 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(); + Inputs.Opts.LimitDiagsOutsideMainFile = LimitDiagsOutsideMainFile; + 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(), + "Error in header: 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(), + "Error in header: 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"), "Error in header: C++ requires a type " + "specifier for all declarations"), + Diag(Main.range("b"), "Error in header: C++ requires a type " + "specifier for all declarations"))); +} + +TEST(DiagsInHeaders, PreferDirectIncludeLocation) { + 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(), + "Error in header: C++ requires a type " + "specifier for all declarations"))); +} + +TEST(DiagsInHeaders, PreferFirstInclude) { + 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"), "#ifndef X\n#define X\nno_type_spec;\n#endif"}}; + auto FS = buildTestFS(Files); + + EXPECT_THAT(build(MainFile, Files).getDiagnostics(), + UnorderedElementsAre(Diag(Main.range(), + "Error in header: 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"), "#ifndef X\n#define X\nno_type_spec;\n#endif"}}; + auto FS = buildTestFS(Files); + + EXPECT_THAT(build(MainFile, Files).getDiagnostics(), + UnorderedElementsAre(Diag(Main.range(), + "Error in header: C++ requires a type " + "specifier for all declarations"))); + + EXPECT_THAT(build(MainFile, Files, 0).getDiagnostics(), + UnorderedElementsAre()); +} + +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(), + "Error in header: C++ requires a type " + "specifier for all declarations"))); +} + } // namespace } // namespace clangd } // namespace clang -