diff --git a/clang-tools-extra/clangd/Config.h b/clang-tools-extra/clangd/Config.h --- a/clang-tools-extra/clangd/Config.h +++ b/clang-tools-extra/clangd/Config.h @@ -88,6 +88,11 @@ bool StandardLibrary = true; } Index; + enum class MissingIncludesPolicy { + /// Diagnose missing includes. + Strict, + None, + }; enum UnusedIncludesPolicy { /// Diagnose unused includes. Strict, @@ -108,6 +113,7 @@ } ClangTidy; UnusedIncludesPolicy UnusedIncludes = None; + MissingIncludesPolicy MissingIncludes = MissingIncludesPolicy::None; /// IncludeCleaner will not diagnose usages of these headers matched by /// these regexes. diff --git a/clang-tools-extra/clangd/ConfigCompile.cpp b/clang-tools-extra/clangd/ConfigCompile.cpp --- a/clang-tools-extra/clangd/ConfigCompile.cpp +++ b/clang-tools-extra/clangd/ConfigCompile.cpp @@ -441,6 +441,18 @@ Out.Apply.push_back([Val](const Params &, Config &C) { C.Diagnostics.UnusedIncludes = *Val; }); + + if (F.MissingIncludes) + if (auto Val = + compileEnum("MissingIncludes", + **F.MissingIncludes) + .map("Strict", Config::MissingIncludesPolicy::Strict) + .map("None", Config::MissingIncludesPolicy::None) + .value()) + Out.Apply.push_back([Val](const Params &, Config &C) { + C.Diagnostics.MissingIncludes = *Val; + }); + compile(std::move(F.Includes)); compile(std::move(F.ClangTidy)); diff --git a/clang-tools-extra/clangd/ConfigFragment.h b/clang-tools-extra/clangd/ConfigFragment.h --- a/clang-tools-extra/clangd/ConfigFragment.h +++ b/clang-tools-extra/clangd/ConfigFragment.h @@ -221,7 +221,7 @@ /// This often has other advantages, such as skipping some analysis. std::vector> Suppress; - /// Controls how clangd will correct "unnecessary #include directives. + /// Controls how clangd will correct "unnecessary" #include directives. /// clangd can warn if a header is `#include`d but not used, and suggest /// removing it. // @@ -235,6 +235,18 @@ /// - None std::optional> UnusedIncludes; + /// Controls how clangd handles missing #include directives. + /// clangd can warn if a header for a symbol is not `#include`d (missing), + /// and suggest adding it. + /// + /// Strict means a header is missing if it is not *directly #include'd. + /// The file might still compile if the header is included transitively. + /// + /// Valid values are: + /// - Strict + /// - None + std::optional> MissingIncludes; + /// Controls IncludeCleaner diagnostics. struct IncludesBlock { /// Regexes that will be used to avoid diagnosing certain includes as diff --git a/clang-tools-extra/clangd/ConfigYAML.cpp b/clang-tools-extra/clangd/ConfigYAML.cpp --- a/clang-tools-extra/clangd/ConfigYAML.cpp +++ b/clang-tools-extra/clangd/ConfigYAML.cpp @@ -128,6 +128,9 @@ Dict.handle("UnusedIncludes", [&](Node &N) { F.UnusedIncludes = scalarValue(N, "UnusedIncludes"); }); + Dict.handle("MissingIncludes", [&](Node &N) { + F.MissingIncludes = scalarValue(N, "MissingIncludes"); + }); Dict.handle("Includes", [&](Node &N) { parse(F.Includes, N); }); Dict.handle("ClangTidy", [&](Node &N) { parse(F.ClangTidy, N); }); Dict.parse(N); diff --git a/clang-tools-extra/clangd/IncludeCleaner.h b/clang-tools-extra/clangd/IncludeCleaner.h --- a/clang-tools-extra/clangd/IncludeCleaner.h +++ b/clang-tools-extra/clangd/IncludeCleaner.h @@ -96,12 +96,16 @@ const llvm::DenseSet &ReferencedFiles, const llvm::StringSet<> &ReferencedPublicHeaders); +std::vector> +computeMissingIncludes(ParsedAST &AST); std::vector computeUnusedIncludes(ParsedAST &AST); // The same as computeUnusedIncludes, but it is an experimental and // include-cleaner-lib-based implementation. std::vector computeUnusedIncludesExperimental(ParsedAST &AST); +std::vector issueMissingIncludesDiagnostics(ParsedAST &AST, + llvm::StringRef Code); std::vector issueUnusedIncludesDiagnostics(ParsedAST &AST, llvm::StringRef Code); diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp --- a/clang-tools-extra/clangd/IncludeCleaner.cpp +++ b/clang-tools-extra/clangd/IncludeCleaner.cpp @@ -20,6 +20,7 @@ #include "clang/AST/ASTContext.h" #include "clang/AST/ExprCXX.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/Basic/FileEntry.h" #include "clang/Basic/SourceLocation.h" #include "clang/Basic/SourceManager.h" #include "clang/Lex/HeaderSearch.h" @@ -28,12 +29,14 @@ #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/STLFunctionalExtras.h" #include "llvm/ADT/SmallString.h" +#include "llvm/ADT/StringRef.h" #include "llvm/ADT/StringSet.h" #include "llvm/Support/FormatVariadic.h" #include "llvm/Support/Path.h" #include "llvm/Support/Regex.h" #include #include +#include namespace clang { namespace clangd { @@ -85,7 +88,7 @@ // Using templateName case is handled by the override TraverseTemplateName. if (TST->getTemplateName().getKind() == TemplateName::UsingTemplate) return true; - add(TST->getAsCXXRecordDecl()); // Specialization + add(TST->getAsCXXRecordDecl()); // Specialization return true; } @@ -460,6 +463,27 @@ return TranslatedHeaderIDs; } +std::vector +collectMacroReferences(ParsedAST &AST) { + const auto &SM = AST.getSourceManager(); + // FIXME: !!this is a hacky way to collect macro references. + std::vector Macros; + auto &PP = AST.getPreprocessor(); + for (const syntax::Token &Tok : + AST.getTokens().spelledTokens(SM.getMainFileID())) { + auto Macro = locateMacroAt(Tok, PP); + if (!Macro) + continue; + if (auto DefLoc = Macro->Info->getDefinitionLoc(); DefLoc.isValid()) + Macros.push_back( + {Tok.location(), + include_cleaner::Macro{/*Name=*/PP.getIdentifierInfo(Tok.text(SM)), + DefLoc}, + include_cleaner::RefType::Explicit}); + } + return Macros; +} + // This is the original clangd-own implementation for computing unused // #includes. Eventually it will be deprecated and replaced by the // include-cleaner-lib-based implementation. @@ -474,56 +498,167 @@ translateToHeaderIDs(ReferencedFiles, AST.getIncludeStructure(), SM); return getUnused(AST, ReferencedHeaders, ReferencedFiles.SpelledUmbrellas); } -std::vector computeUnusedIncludesExperimental(ParsedAST &AST) { - const auto &SM = AST.getSourceManager(); - const auto &Includes = AST.getIncludeStructure(); - // FIXME: this map should probably be in IncludeStructure. - llvm::StringMap> BySpelling; - for (const auto &Inc : Includes.MainFileIncludes) { + +// Compute unused #includes using the include-cleaner-lib-based implementation. +std::vector +computeUnusedIncludesExperimental(ParsedAST &AST) { + const auto &SM = AST.getSourceManager(); + const auto &Includes = AST.getIncludeStructure(); + // FIXME: this map should probably be in IncludeStructure. + llvm::StringMap> BySpelling; + for (const auto &Inc : Includes.MainFileIncludes) { if (Inc.HeaderID) BySpelling.try_emplace(Inc.Written) .first->second.push_back( static_cast(*Inc.HeaderID)); - } - // FIXME: !!this is a hacky way to collect macro references. - std::vector Macros; - auto& PP = AST.getPreprocessor(); - for (const syntax::Token &Tok : - AST.getTokens().spelledTokens(SM.getMainFileID())) { - auto Macro = locateMacroAt(Tok, PP); - if (!Macro) - continue; - if (auto DefLoc = Macro->Info->getDefinitionLoc(); DefLoc.isValid()) - Macros.push_back( - {Tok.location(), - include_cleaner::Macro{/*Name=*/PP.getIdentifierInfo(Tok.text(SM)), - DefLoc}, - include_cleaner::RefType::Explicit}); - } - llvm::DenseSet Used; - include_cleaner::walkUsed( - AST.getLocalTopLevelDecls(), /*MacroRefs=*/Macros, - AST.getPragmaIncludes(), SM, - [&](const include_cleaner::SymbolReference &Ref, - llvm::ArrayRef Providers) { - for (const auto &H : Providers) { - switch (H.kind()) { - case include_cleaner::Header::Physical: - if (auto HeaderID = Includes.getID(H.physical())) - Used.insert(*HeaderID); - break; - case include_cleaner::Header::Standard: - for (auto HeaderID : Includes.StdlibHeaders.lookup(H.standard())) - Used.insert(HeaderID); - break; - case include_cleaner::Header::Verbatim: - for (auto HeaderID : BySpelling.lookup(H.verbatim())) - Used.insert(HeaderID); - break; - } - } - }); - return getUnused(AST, Used, /*ReferencedPublicHeaders*/{}); + } + + std::vector Macros = + collectMacroReferences(AST); + + llvm::DenseSet Used; + include_cleaner::walkUsed( + AST.getLocalTopLevelDecls(), /*MacroRefs=*/Macros, + AST.getPragmaIncludes(), SM, + [&](const include_cleaner::SymbolReference &Ref, + llvm::ArrayRef Providers) { + for (const auto &H : Providers) { + switch (H.kind()) { + case include_cleaner::Header::Physical: + if (auto HeaderID = Includes.getID(H.physical())) + Used.insert(*HeaderID); + break; + case include_cleaner::Header::Standard: + for (auto HeaderID : Includes.StdlibHeaders.lookup(H.standard())) + Used.insert(HeaderID); + break; + case include_cleaner::Header::Verbatim: + for (auto HeaderID : BySpelling.lookup(H.verbatim())) + Used.insert(HeaderID); + break; + } + } + }); + return getUnused(AST, Used, /*ReferencedPublicHeaders*/ {}); +} + +include_cleaner::Includes +convertIncludes(const SourceManager &SM, + std::vector MainFileIncludes) { + include_cleaner::Includes Includes; + for (const Inclusion &Inc : MainFileIncludes) { + llvm::ErrorOr ResolvedOrError = + SM.getFileManager().getFile(Inc.Resolved); + const FileEntry *Resolved = nullptr; + if (bool(ResolvedOrError)) { + Resolved = ResolvedOrError.get(); + } + SourceLocation HashLocation = + SM.getComposedLoc(SM.getMainFileID(), Inc.HashOffset); + llvm::StringRef WrittenRef = llvm::StringRef(Inc.Written); + bool Angled = WrittenRef.starts_with("<") ? true : false; + Includes.add(include_cleaner::Include{WrittenRef.trim("\"<>"), Resolved, + HashLocation, (unsigned)Inc.HashLine, + Angled}); + } + return Includes; +} + +// Compute missing #includes. Uses the include-cleaner-lib-based implementation. +std::vector> +computeMissingIncludes(ParsedAST &AST) { + std::vector Macros = + collectMacroReferences(AST); + std::vector MainFileIncludes = + AST.getIncludeStructure().MainFileIncludes; + + include_cleaner::Includes IncludeCleanerIncludes = + convertIncludes(AST.getSourceManager(), MainFileIncludes); + std::string FileName = + AST.getSourceManager() + .getFileEntryRefForID(AST.getSourceManager().getMainFileID()) + ->getName() + .str(); + if (FileName.find("foo") != std::string::npos) { + vlog("Include cleaner includes: {0}", IncludeCleanerIncludes.all().size()); + } + + std::vector> Missing; + SourceManager &SM = AST.getSourceManager(); + const FileEntry *MainFile = SM.getFileEntryForID(SM.getMainFileID()); + include_cleaner::walkUsed( + AST.getLocalTopLevelDecls(), Macros, AST.getPragmaIncludes(), SM, + [&](const include_cleaner::SymbolReference &Ref, + llvm::ArrayRef Providers) { + bool Satisfied = false; + for (const include_cleaner::Header &H : Providers) { + if (H.kind() == include_cleaner::Header::Physical && + H.physical() == MainFile) { + Satisfied = true; + } + if (!IncludeCleanerIncludes.match(H).empty()) { + Satisfied = true; + } + } + if (!Satisfied && !Providers.empty() && + Ref.RT == include_cleaner::RefType::Explicit) { + std::string SpelledHeader = include_cleaner::spellHeader( + Providers.front(), AST.getPreprocessor().getHeaderSearchInfo(), + MainFile); + std::pair FileIDAndOffset = + AST.getSourceManager().getDecomposedLoc(Ref.RefLocation); + Missing.push_back({SpelledHeader, FileIDAndOffset.second}); + } + }); + return Missing; +} + +std::vector issueMissingIncludesDiagnostics(ParsedAST &AST, + llvm::StringRef Code) { + const Config &Cfg = Config::current(); + if (Cfg.Diagnostics.MissingIncludes == Config::MissingIncludesPolicy::None || + Cfg.Diagnostics.SuppressAll || + Cfg.Diagnostics.Suppress.contains("missing-includes")) { + return {}; + } + // Interaction is only polished for C/CPP. + if (AST.getLangOpts().ObjC) + return {}; + trace::Span Tracer("IncludeCleaner::issueMissingIncludesDiagnostics"); + std::string FileName = + AST.getSourceManager() + .getFileEntryRefForID(AST.getSourceManager().getMainFileID()) + ->getName() + .str(); + + unsigned FixLine = 0; + std::vector MainFileIncludes = + AST.getIncludeStructure().MainFileIncludes; + if (!MainFileIncludes.empty()) + FixLine = MainFileIncludes[MainFileIncludes.size() - 1].HashLine + 1; + + std::vector Result; + const auto &MissingIncludes = computeMissingIncludes(AST); + for (const std::pair &Missing : MissingIncludes) { + Diag D; + D.Message = llvm::formatv("header {0} is missing", Missing.first); + D.Name = "missing-includes"; + D.Source = Diag::DiagSource::Clangd; + D.File = FileName; + D.Severity = DiagnosticsEngine::Warning; + + D.Range = getDiagnosticRange(Code, Missing.second); + D.Fixes.emplace_back(); + D.Fixes.back().Message = "add #include directive"; + D.Fixes.back().Edits.emplace_back(); + D.Fixes.back().Edits.back().newText = "#include " + Missing.first + "\n"; + D.Fixes.back().Edits.back().range.start.line = FixLine; + D.Fixes.back().Edits.back().range.end.line = FixLine; + D.InsideMainFile = true; + Result.push_back(std::move(D)); + } + + return Result; } std::vector issueUnusedIncludesDiagnostics(ParsedAST &AST, diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -691,9 +691,14 @@ if (Result.Diags) { auto UnusedHeadersDiags = issueUnusedIncludesDiagnostics(Result, Inputs.Contents); + auto MissingHeadersDiags = + issueMissingIncludesDiagnostics(Result, Inputs.Contents); Result.Diags->insert(Result.Diags->end(), make_move_iterator(UnusedHeadersDiags.begin()), make_move_iterator(UnusedHeadersDiags.end())); + Result.Diags->insert(Result.Diags->end(), + make_move_iterator(MissingHeadersDiags.begin()), + make_move_iterator(MissingHeadersDiags.end())); } return std::move(Result); } diff --git a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp --- a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp +++ b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp @@ -385,6 +385,43 @@ Pointee(writtenInclusion("\"dir/unused.h\"")))); } +TEST(IncludeCleaner, GetMissingHeaders) { + llvm::StringLiteral MainFile = R"cpp( + #include "a.h" + #include "dir/c.h" + #include + + void foo() { + b(); + d(); + f(); + })cpp"; + // Build expected ast with symbols coming from headers. + TestTU TU; + TU.Filename = "foo.cpp"; + TU.AdditionalFiles["foo.h"] = guard("void foo();"); + TU.AdditionalFiles["a.h"] = guard("#include \"b.h\""); + TU.AdditionalFiles["b.h"] = guard("void b();"); + + TU.AdditionalFiles["dir/c.h"] = guard("#include \"d.h\""); + TU.AdditionalFiles["dir/d.h"] = guard("void d();"); + + TU.AdditionalFiles["system/e.h"] = guard("#include "); + TU.AdditionalFiles["system/f.h"] = guard("void f();"); + TU.ExtraArgs.push_back("-isystem" + testPath("system")); + + TU.Code = MainFile.str(); + ParsedAST AST = TU.build(); + + std::vector> MissingIncludes = + computeMissingIncludes(AST); + + std::pair b{"\"b.h\"", 86}; + std::pair d{"\"dir/d.h\"", 97}; + std::pair h{"", 108}; + EXPECT_THAT(MissingIncludes, UnorderedElementsAre(b, d, h)); +} + TEST(IncludeCleaner, VirtualBuffers) { TestTU TU; TU.Code = R"cpp( @@ -628,6 +665,7 @@ Config Cfg; Cfg.Diagnostics.UnusedIncludes = Config::Strict; + Cfg.Diagnostics.MissingIncludes = Config::MissingIncludesPolicy::Strict; WithContextValue Ctx(Config::Key, std::move(Cfg)); ParsedAST AST = TU.build(); EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty())); diff --git a/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h b/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h --- a/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h +++ b/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h @@ -73,6 +73,8 @@ std::string fixIncludes(const AnalysisResults &Results, llvm::StringRef Code, const format::FormatStyle &IncludeStyle); +static std::string spellHeader(const Header &H, HeaderSearch &HS, + const FileEntry *Main); } // namespace include_cleaner } // namespace clang