From fd8c9ef20a9519dccd5b8178b29ed4574285d36f Mon Sep 17 00:00:00 2001 From: Viktoriia Bakalova Date: Wed, 8 Mar 2023 12:34:15 +0000 Subject: [PATCH] Revert "Re-land [clangd] Add support for missing includes analysis." This reverts commit 85a5c17b66768353b7fff717904e42805bb6a547. --- clang-tools-extra/clangd/Config.h | 12 +- clang-tools-extra/clangd/ConfigCompile.cpp | 24 +- clang-tools-extra/clangd/ConfigFragment.h | 16 +- clang-tools-extra/clangd/ConfigYAML.cpp | 3 - clang-tools-extra/clangd/IncludeCleaner.cpp | 375 +++++---------------- clang-tools-extra/clangd/IncludeCleaner.h | 27 +- clang-tools-extra/clangd/ParsedAST.cpp | 10 +- clang-tools-extra/clangd/Preamble.cpp | 4 +- .../clangd/unittests/ConfigCompileTests.cpp | 6 +- .../clangd/unittests/DiagnosticsTests.cpp | 2 +- .../clangd/unittests/IncludeCleanerTests.cpp | 179 +--------- .../clangd/unittests/PreambleTests.cpp | 2 +- .../include/clang-include-cleaner/Analysis.h | 2 - clang-tools-extra/include-cleaner/lib/Analysis.cpp | 4 +- 14 files changed, 128 insertions(+), 538 deletions(-) diff --git a/clang-tools-extra/clangd/Config.h b/clang-tools-extra/clangd/Config.h index ab346aa..dffd54b 100644 --- a/clang-tools-extra/clangd/Config.h +++ b/clang-tools-extra/clangd/Config.h @@ -88,12 +88,11 @@ struct Config { bool StandardLibrary = true; } Index; - enum class IncludesPolicy { - /// Diagnose missing and unused includes. + enum class UnusedIncludesPolicy { + /// Diagnose unused includes. Strict, None, - /// The same as Strict, but using the include-cleaner library for - /// unused includes. + /// The same as Strict, but using the include-cleaner library. Experiment, }; /// Controls warnings and errors when parsing code. @@ -108,12 +107,11 @@ struct Config { llvm::StringMap CheckOptions; } ClangTidy; + UnusedIncludesPolicy UnusedIncludes = UnusedIncludesPolicy::None; + /// Enable emitting diagnostics using stale preambles. bool AllowStalePreamble = false; - IncludesPolicy UnusedIncludes = IncludesPolicy::None; - IncludesPolicy MissingIncludes = IncludesPolicy::None; - /// IncludeCleaner will not diagnose usages of these headers matched by /// these regexes. struct { diff --git a/clang-tools-extra/clangd/ConfigCompile.cpp b/clang-tools-extra/clangd/ConfigCompile.cpp index 2f0ef89..18de6e4 100644 --- a/clang-tools-extra/clangd/ConfigCompile.cpp +++ b/clang-tools-extra/clangd/ConfigCompile.cpp @@ -431,16 +431,16 @@ struct FragmentCompiler { }); if (F.UnusedIncludes) - if (auto Val = compileEnum("UnusedIncludes", - **F.UnusedIncludes) - .map("Strict", Config::IncludesPolicy::Strict) - .map("Experiment", Config::IncludesPolicy::Experiment) - .map("None", Config::IncludesPolicy::None) - .value()) + if (auto Val = + compileEnum("UnusedIncludes", + **F.UnusedIncludes) + .map("Strict", Config::UnusedIncludesPolicy::Strict) + .map("Experiment", Config::UnusedIncludesPolicy::Experiment) + .map("None", Config::UnusedIncludesPolicy::None) + .value()) Out.Apply.push_back([Val](const Params &, Config &C) { C.Diagnostics.UnusedIncludes = *Val; }); - if (F.AllowStalePreamble) { if (auto Val = F.AllowStalePreamble) Out.Apply.push_back([Val](const Params &, Config &C) { @@ -448,16 +448,6 @@ struct FragmentCompiler { }); } - if (F.MissingIncludes) - if (auto Val = compileEnum("MissingIncludes", - **F.MissingIncludes) - .map("Strict", Config::IncludesPolicy::Strict) - .map("None", Config::IncludesPolicy::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 index 956e8a8..a559759 100644 --- a/clang-tools-extra/clangd/ConfigFragment.h +++ b/clang-tools-extra/clangd/ConfigFragment.h @@ -221,7 +221,7 @@ struct Fragment { /// 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. // @@ -236,23 +236,9 @@ struct Fragment { /// - None std::optional> UnusedIncludes; - /// Enable emitting diagnostics using stale preambles. std::optional> AllowStalePreamble; - /// Controls if clangd should analyze missing #include directives. - /// clangd will warn if no header providing a symbol is `#include`d - /// (missing) directly, and suggest adding it. - /// - /// Strict means a header providing a symbol 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 index 84559f5..0ec0239 100644 --- a/clang-tools-extra/clangd/ConfigYAML.cpp +++ b/clang-tools-extra/clangd/ConfigYAML.cpp @@ -128,9 +128,6 @@ private: 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.handle("AllowStalePreamble", [&](Node &N) { diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp index 0d74b88..7d523d9 100644 --- a/clang-tools-extra/clangd/IncludeCleaner.cpp +++ b/clang-tools-extra/clangd/IncludeCleaner.cpp @@ -8,55 +8,32 @@ #include "IncludeCleaner.h" #include "Config.h" -#include "Diagnostics.h" #include "Headers.h" #include "ParsedAST.h" #include "Protocol.h" #include "SourceCode.h" -#include "URI.h" #include "clang-include-cleaner/Analysis.h" #include "clang-include-cleaner/Types.h" #include "index/CanonicalIncludes.h" #include "support/Logger.h" -#include "support/Path.h" #include "support/Trace.h" #include "clang/AST/ASTContext.h" -#include "clang/AST/DeclCXX.h" -#include "clang/AST/Expr.h" #include "clang/AST/ExprCXX.h" #include "clang/AST/RecursiveASTVisitor.h" -#include "clang/AST/TemplateName.h" -#include "clang/AST/Type.h" -#include "clang/Basic/Diagnostic.h" -#include "clang/Basic/LLVM.h" #include "clang/Basic/SourceLocation.h" #include "clang/Basic/SourceManager.h" -#include "clang/Format/Format.h" #include "clang/Lex/HeaderSearch.h" #include "clang/Lex/Preprocessor.h" -#include "clang/Tooling/Core/Replacement.h" -#include "clang/Tooling/Inclusions/HeaderIncludes.h" -#include "clang/Tooling/Inclusions/IncludeStyle.h" -#include "clang/Tooling/Inclusions/StandardLibrary.h" #include "clang/Tooling/Syntax/Tokens.h" #include "llvm/ADT/ArrayRef.h" -#include "llvm/ADT/DenseSet.h" -#include "llvm/ADT/STLExtras.h" #include "llvm/ADT/STLFunctionalExtras.h" #include "llvm/ADT/SmallString.h" -#include "llvm/ADT/SmallVector.h" -#include "llvm/ADT/StringRef.h" #include "llvm/ADT/StringSet.h" -#include "llvm/Support/Casting.h" -#include "llvm/Support/Error.h" -#include "llvm/Support/ErrorHandling.h" #include "llvm/Support/FormatVariadic.h" #include "llvm/Support/Path.h" #include "llvm/Support/Regex.h" -#include +#include #include -#include -#include namespace clang { namespace clangd { @@ -281,17 +258,6 @@ void findReferencedMacros(const SourceManager &SM, Preprocessor &PP, } } -bool isFilteredByConfig(const Config &Cfg, llvm::StringRef HeaderPath) { - // Convert the path to Unix slashes and try to match against the filter. - llvm::SmallString<64> NormalizedPath(HeaderPath); - llvm::sys::path::native(NormalizedPath, llvm::sys::path::Style::posix); - for (auto &Filter : Cfg.Diagnostics.Includes.IgnoreHeader) { - if (Filter(NormalizedPath)) - return true; - } - return false; -} - static bool mayConsiderUnused(const Inclusion &Inc, ParsedAST &AST, const Config &Cfg) { if (Inc.BehindPragmaKeep) @@ -322,10 +288,14 @@ static bool mayConsiderUnused(const Inclusion &Inc, ParsedAST &AST, FE->getName()); return false; } - - if (isFilteredByConfig(Cfg, Inc.Resolved)) { - dlog("{0} header is filtered out by the configuration", FE->getName()); - return false; + for (auto &Filter : Cfg.Diagnostics.Includes.IgnoreHeader) { + // Convert the path to Unix slashes and try to match against the filter. + llvm::SmallString<64> Path(Inc.Resolved); + llvm::sys::path::native(Path, llvm::sys::path::Style::posix); + if (Filter(Path)) { + dlog("{0} header is filtered out by the configuration", FE->getName()); + return false; + } } return true; } @@ -355,195 +325,6 @@ FileID headerResponsible(FileID ID, const SourceManager &SM, return ID; } -include_cleaner::Includes -convertIncludes(const SourceManager &SM, - const llvm::ArrayRef MainFileIncludes) { - include_cleaner::Includes Includes; - for (const Inclusion &Inc : MainFileIncludes) { - include_cleaner::Include TransformedInc; - llvm::StringRef WrittenRef = llvm::StringRef(Inc.Written); - TransformedInc.Spelled = WrittenRef.trim("\"<>"); - TransformedInc.HashLocation = - SM.getComposedLoc(SM.getMainFileID(), Inc.HashOffset); - TransformedInc.Line = Inc.HashLine + 1; - TransformedInc.Angled = WrittenRef.starts_with("<"); - auto FE = SM.getFileManager().getFile(Inc.Resolved); - if (!FE) { - elog("IncludeCleaner: Failed to get an entry for resolved path {0}: {1}", - Inc.Resolved, FE.getError().message()); - continue; - } - TransformedInc.Resolved = *FE; - Includes.add(std::move(TransformedInc)); - } - return Includes; -} - -std::string spellHeader(ParsedAST &AST, const FileEntry *MainFile, - include_cleaner::Header Provider) { - if (Provider.kind() == include_cleaner::Header::Physical) { - if (auto CanonicalPath = - getCanonicalPath(Provider.physical(), AST.getSourceManager())) { - std::string SpelledHeader = - llvm::cantFail(URI::includeSpelling(URI::create(*CanonicalPath))); - if (!SpelledHeader.empty()) - return SpelledHeader; - } - } - return include_cleaner::spellHeader( - Provider, AST.getPreprocessor().getHeaderSearchInfo(), MainFile); -} - -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; -} - -llvm::StringRef getResolvedPath(const include_cleaner::Header &SymProvider) { - switch (SymProvider.kind()) { - case include_cleaner::Header::Physical: - return SymProvider.physical()->tryGetRealPathName(); - case include_cleaner::Header::Standard: - return SymProvider.standard().name().trim("<>\""); - case include_cleaner::Header::Verbatim: - return SymProvider.verbatim().trim("<>\""); - } - llvm_unreachable("Unknown header kind"); -} - -std::string getSymbolName(const include_cleaner::Symbol &Sym) { - switch (Sym.kind()) { - case include_cleaner::Symbol::Macro: - return Sym.macro().Name->getName().str(); - case include_cleaner::Symbol::Declaration: - return llvm::dyn_cast(&Sym.declaration()) - ->getQualifiedNameAsString(); - } - llvm_unreachable("Unknown symbol kind"); -} - -std::vector generateMissingIncludeDiagnostics( - ParsedAST &AST, llvm::ArrayRef MissingIncludes, - llvm::StringRef Code) { - std::vector Result; - const Config &Cfg = Config::current(); - if (Cfg.Diagnostics.MissingIncludes != Config::IncludesPolicy::Strict || - Cfg.Diagnostics.SuppressAll || - Cfg.Diagnostics.Suppress.contains("missing-includes")) { - return Result; - } - - const SourceManager &SM = AST.getSourceManager(); - const FileEntry *MainFile = SM.getFileEntryForID(SM.getMainFileID()); - - auto FileStyle = format::getStyle( - format::DefaultFormatStyle, AST.tuPath(), format::DefaultFallbackStyle, - Code, &SM.getFileManager().getVirtualFileSystem()); - if (!FileStyle) { - elog("Couldn't infer style", FileStyle.takeError()); - FileStyle = format::getLLVMStyle(); - } - - tooling::HeaderIncludes HeaderIncludes(AST.tuPath(), Code, - FileStyle->IncludeStyle); - for (const auto &SymbolWithMissingInclude : MissingIncludes) { - llvm::StringRef ResolvedPath = - getResolvedPath(SymbolWithMissingInclude.Providers.front()); - if (isFilteredByConfig(Cfg, ResolvedPath)) { - dlog("IncludeCleaner: not diagnosing missing include {0}, filtered by " - "config", - ResolvedPath); - continue; - } - - std::string Spelling = - spellHeader(AST, MainFile, SymbolWithMissingInclude.Providers.front()); - llvm::StringRef HeaderRef{Spelling}; - bool Angled = HeaderRef.starts_with("<"); - // We might suggest insertion of an existing include in edge cases, e.g., - // include is present in a PP-disabled region, or spelling of the header - // turns out to be the same as one of the unresolved includes in the - // main file. - std::optional Replacement = HeaderIncludes.insert( - HeaderRef.trim("\"<>"), Angled, tooling::IncludeDirective::Include); - if (!Replacement.has_value()) - continue; - - Diag &D = Result.emplace_back(); - D.Message = - llvm::formatv("No header providing \"{0}\" is directly included", - getSymbolName(SymbolWithMissingInclude.Symbol)); - D.Name = "missing-includes"; - D.Source = Diag::DiagSource::Clangd; - D.File = AST.tuPath(); - D.InsideMainFile = true; - D.Severity = DiagnosticsEngine::Warning; - D.Range = clangd::Range{ - offsetToPosition(Code, - SymbolWithMissingInclude.SymRefRange.beginOffset()), - offsetToPosition(Code, - SymbolWithMissingInclude.SymRefRange.endOffset())}; - auto &F = D.Fixes.emplace_back(); - F.Message = "#include " + Spelling; - TextEdit Edit = replacementToEdit(Code, *Replacement); - F.Edits.emplace_back(std::move(Edit)); - } - return Result; -} - -std::vector generateUnusedIncludeDiagnostics( - PathRef FileName, llvm::ArrayRef UnusedIncludes, - llvm::StringRef Code) { - std::vector Result; - const Config &Cfg = Config::current(); - if (Cfg.Diagnostics.UnusedIncludes == Config::IncludesPolicy::None || - Cfg.Diagnostics.SuppressAll || - Cfg.Diagnostics.Suppress.contains("unused-includes")) { - return Result; - } - for (const auto *Inc : UnusedIncludes) { - Diag &D = Result.emplace_back(); - D.Message = - llvm::formatv("included header {0} is not used directly", - llvm::sys::path::filename( - Inc->Written.substr(1, Inc->Written.size() - 2), - llvm::sys::path::Style::posix)); - D.Name = "unused-includes"; - D.Source = Diag::DiagSource::Clangd; - D.File = FileName; - D.InsideMainFile = true; - D.Severity = DiagnosticsEngine::Warning; - D.Tags.push_back(Unnecessary); - D.Range = getDiagnosticRange(Code, Inc->HashOffset); - // FIXME(kirillbobyrev): Removing inclusion might break the code if the - // used headers are only reachable transitively through this one. Suggest - // including them directly instead. - // FIXME(kirillbobyrev): Add fix suggestion for adding IWYU pragmas - // (keep/export) remove the warning once we support IWYU pragmas. - auto &F = D.Fixes.emplace_back(); - F.Message = "remove #include directive"; - F.Edits.emplace_back(); - F.Edits.back().range.start.line = Inc->HashLine; - F.Edits.back().range.end.line = Inc->HashLine + 1; - } - return Result; -} } // namespace ReferencedLocations findReferencedLocations(ASTContext &Ctx, Preprocessor &PP, @@ -693,85 +474,105 @@ std::vector computeUnusedIncludes(ParsedAST &AST) { translateToHeaderIDs(ReferencedFiles, AST.getIncludeStructure(), SM); return getUnused(AST, ReferencedHeaders, ReferencedFiles.SpelledUmbrellas); } - -IncludeCleanerFindings computeIncludeCleanerFindings(ParsedAST &AST) { +std::vector +computeUnusedIncludesExperimental(ParsedAST &AST) { const auto &SM = AST.getSourceManager(); const auto &Includes = AST.getIncludeStructure(); - include_cleaner::Includes ConvertedIncludes = - convertIncludes(SM, Includes.MainFileIncludes); - const FileEntry *MainFile = SM.getFileEntryForID(SM.getMainFileID()); - std::vector Macros = - collectMacroReferences(AST); - std::vector MissingIncludes; + // 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; - trace::Span Tracer("include_cleaner::walkUsed"); include_cleaner::walkUsed( AST.getLocalTopLevelDecls(), /*MacroRefs=*/Macros, AST.getPragmaIncludes(), SM, [&](const include_cleaner::SymbolReference &Ref, llvm::ArrayRef Providers) { - bool Satisfied = false; for (const auto &H : Providers) { - if (H.kind() == include_cleaner::Header::Physical && - H.physical() == MainFile) { - Satisfied = true; - continue; - } - for (auto *Inc : ConvertedIncludes.match(H)) { - Satisfied = true; - auto HeaderID = Includes.getID(Inc->Resolved); - assert(HeaderID.has_value() && - "ConvertedIncludes only contains resolved includes."); - Used.insert(*HeaderID); + 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 *Inc : + Includes.mainFileIncludesWithSpelling(H.verbatim())) { + if (!Inc->HeaderID.has_value()) + continue; + IncludeStructure::HeaderID ID = + static_cast(*Inc->HeaderID); + Used.insert(ID); + } + break; } } - - if (Satisfied || Providers.empty() || - Ref.RT != include_cleaner::RefType::Explicit) - return; - - auto &Tokens = AST.getTokens(); - auto SpelledForExpanded = - Tokens.spelledForExpanded(Tokens.expandedTokens(Ref.RefLocation)); - if (!SpelledForExpanded) - return; - - auto Range = syntax::Token::range(SM, SpelledForExpanded->front(), - SpelledForExpanded->back()); - MissingIncludeDiagInfo DiagInfo{Ref.Target, Range, Providers}; - MissingIncludes.push_back(std::move(DiagInfo)); }); - std::vector UnusedIncludes = - getUnused(AST, Used, /*ReferencedPublicHeaders*/ {}); - return {std::move(UnusedIncludes), std::move(MissingIncludes)}; + return getUnused(AST, Used, /*ReferencedPublicHeaders*/ {}); } -std::vector issueIncludeCleanerDiagnostics(ParsedAST &AST, +std::vector issueUnusedIncludesDiagnostics(ParsedAST &AST, llvm::StringRef Code) { + const Config &Cfg = Config::current(); + if (Cfg.Diagnostics.UnusedIncludes == Config::UnusedIncludesPolicy::None || + Cfg.Diagnostics.SuppressAll || + Cfg.Diagnostics.Suppress.contains("unused-includes")) + return {}; // Interaction is only polished for C/CPP. if (AST.getLangOpts().ObjC) return {}; - - trace::Span Tracer("IncludeCleaner::issueIncludeCleanerDiagnostics"); - - const Config &Cfg = Config::current(); - IncludeCleanerFindings Findings; - if (Cfg.Diagnostics.MissingIncludes == Config::IncludesPolicy::Strict || - Cfg.Diagnostics.UnusedIncludes == Config::IncludesPolicy::Experiment) { - // will need include-cleaner results, call it once - Findings = computeIncludeCleanerFindings(AST); + trace::Span Tracer("IncludeCleaner::issueUnusedIncludesDiagnostics"); + std::vector Result; + std::string FileName = + AST.getSourceManager() + .getFileEntryRefForID(AST.getSourceManager().getMainFileID()) + ->getName() + .str(); + const auto &UnusedIncludes = + Cfg.Diagnostics.UnusedIncludes == Config::UnusedIncludesPolicy::Experiment + ? computeUnusedIncludesExperimental(AST) + : computeUnusedIncludes(AST); + for (const auto *Inc : UnusedIncludes) { + Diag D; + D.Message = + llvm::formatv("included header {0} is not used directly", + llvm::sys::path::filename( + Inc->Written.substr(1, Inc->Written.size() - 2), + llvm::sys::path::Style::posix)); + D.Name = "unused-includes"; + D.Source = Diag::DiagSource::Clangd; + D.File = FileName; + D.Severity = DiagnosticsEngine::Warning; + D.Tags.push_back(Unnecessary); + D.Range = getDiagnosticRange(Code, Inc->HashOffset); + // FIXME(kirillbobyrev): Removing inclusion might break the code if the + // used headers are only reachable transitively through this one. Suggest + // including them directly instead. + // FIXME(kirillbobyrev): Add fix suggestion for adding IWYU pragmas + // (keep/export) remove the warning once we support IWYU pragmas. + D.Fixes.emplace_back(); + D.Fixes.back().Message = "remove #include directive"; + D.Fixes.back().Edits.emplace_back(); + D.Fixes.back().Edits.back().range.start.line = Inc->HashLine; + D.Fixes.back().Edits.back().range.end.line = Inc->HashLine + 1; + D.InsideMainFile = true; + Result.push_back(std::move(D)); } - - std::vector Result = generateUnusedIncludeDiagnostics( - AST.tuPath(), - Cfg.Diagnostics.UnusedIncludes == Config::IncludesPolicy::Strict - ? computeUnusedIncludes(AST) - : Findings.UnusedIncludes, - Code); - llvm::move( - generateMissingIncludeDiagnostics(AST, Findings.MissingIncludes, Code), - std::back_inserter(Result)); return Result; } diff --git a/clang-tools-extra/clangd/IncludeCleaner.h b/clang-tools-extra/clangd/IncludeCleaner.h index 989067e..a7beb9c 100644 --- a/clang-tools-extra/clangd/IncludeCleaner.h +++ b/clang-tools-extra/clangd/IncludeCleaner.h @@ -20,38 +20,18 @@ #include "Headers.h" #include "ParsedAST.h" -#include "clang-include-cleaner/Types.h" #include "index/CanonicalIncludes.h" #include "clang/Basic/SourceLocation.h" #include "clang/Tooling/Inclusions/StandardLibrary.h" -#include "clang/Tooling/Syntax/Tokens.h" #include "llvm/ADT/DenseSet.h" #include "llvm/ADT/STLFunctionalExtras.h" #include "llvm/ADT/StringSet.h" #include -#include #include namespace clang { namespace clangd { -// Data needed for missing include diagnostics. -struct MissingIncludeDiagInfo { - include_cleaner::Symbol Symbol; - syntax::FileRange SymRefRange; - std::vector Providers; - - bool operator==(const MissingIncludeDiagInfo &Other) const { - return std::tie(SymRefRange, Providers, Symbol) == - std::tie(Other.SymRefRange, Other.Providers, Other.Symbol); - } -}; - -struct IncludeCleanerFindings { - std::vector UnusedIncludes; - std::vector MissingIncludes; -}; - struct ReferencedLocations { llvm::DenseSet User; llvm::DenseSet Stdlib; @@ -116,10 +96,13 @@ getUnused(ParsedAST &AST, const llvm::DenseSet &ReferencedFiles, const llvm::StringSet<> &ReferencedPublicHeaders); -IncludeCleanerFindings computeIncludeCleanerFindings(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 issueIncludeCleanerDiagnostics(ParsedAST &AST, +std::vector issueUnusedIncludesDiagnostics(ParsedAST &AST, llvm::StringRef Code); /// Affects whether standard library includes should be considered for diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp index 640b112..54a0f97 100644 --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -687,9 +687,13 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, std::move(Macros), std::move(Marks), std::move(ParsedDecls), std::move(Diags), std::move(Includes), std::move(CanonIncludes)); - if (Result.Diags) - llvm::move(issueIncludeCleanerDiagnostics(Result, Inputs.Contents), - std::back_inserter(*Result.Diags)); + if (Result.Diags) { + auto UnusedHeadersDiags = + issueUnusedIncludesDiagnostics(Result, Inputs.Contents); + Result.Diags->insert(Result.Diags->end(), + make_move_iterator(UnusedHeadersDiags.begin()), + make_move_iterator(UnusedHeadersDiags.end())); + } return std::move(Result); } diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp index c4aec74..fceea30 100644 --- a/clang-tools-extra/clangd/Preamble.cpp +++ b/clang-tools-extra/clangd/Preamble.cpp @@ -128,9 +128,7 @@ public: SourceMgr = &CI.getSourceManager(); Includes.collect(CI); if (Config::current().Diagnostics.UnusedIncludes == - Config::IncludesPolicy::Experiment || - Config::current().Diagnostics.MissingIncludes == - Config::IncludesPolicy::Strict) + Config::UnusedIncludesPolicy::Experiment) Pragmas.record(CI); if (BeforeExecuteCallback) BeforeExecuteCallback(CI); diff --git a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp index 45dacba..014db13 100644 --- a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp +++ b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp @@ -249,19 +249,19 @@ TEST_F(ConfigCompileTests, DiagnosticsIncludeCleaner) { // Defaults to None. EXPECT_TRUE(compileAndApply()); EXPECT_EQ(Conf.Diagnostics.UnusedIncludes, - Config::IncludesPolicy::None); + Config::UnusedIncludesPolicy::None); Frag = {}; Frag.Diagnostics.UnusedIncludes.emplace("None"); EXPECT_TRUE(compileAndApply()); EXPECT_EQ(Conf.Diagnostics.UnusedIncludes, - Config::IncludesPolicy::None); + Config::UnusedIncludesPolicy::None); Frag = {}; Frag.Diagnostics.UnusedIncludes.emplace("Strict"); EXPECT_TRUE(compileAndApply()); EXPECT_EQ(Conf.Diagnostics.UnusedIncludes, - Config::IncludesPolicy::Strict); + Config::UnusedIncludesPolicy::Strict); Frag = {}; EXPECT_TRUE(Conf.Diagnostics.Includes.IgnoreHeader.empty()) diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp index 289767b..a20be39 100644 --- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp +++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp @@ -1900,7 +1900,7 @@ $fix[[ $diag[[#include "unused.h"]] // Off by default. EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty()); Config Cfg; - Cfg.Diagnostics.UnusedIncludes = Config::IncludesPolicy::Strict; + Cfg.Diagnostics.UnusedIncludes = Config::UnusedIncludesPolicy::Strict; // Set filtering. Cfg.Diagnostics.Includes.IgnoreHeader.emplace_back( [](llvm::StringRef Header) { return Header.endswith("ignore.h"); }); diff --git a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp index 2e18c18..01b8e6a 100644 --- a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp +++ b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp @@ -8,58 +8,26 @@ #include "Annotations.h" #include "Config.h" -#include "Diagnostics.h" #include "IncludeCleaner.h" -#include "ParsedAST.h" #include "SourceCode.h" #include "TestFS.h" #include "TestTU.h" -#include "clang-include-cleaner/Analysis.h" -#include "clang-include-cleaner/Types.h" #include "support/Context.h" -#include "clang/AST/DeclBase.h" -#include "clang/Basic/SourceManager.h" -#include "clang/Tooling/Syntax/Tokens.h" #include "llvm/ADT/ScopeExit.h" -#include "llvm/ADT/StringRef.h" -#include "llvm/Support/Casting.h" -#include "llvm/Support/Error.h" -#include "llvm/Support/ScopedPrinter.h" #include "llvm/Testing/Support/SupportHelpers.h" #include "gmock/gmock.h" #include "gtest/gtest.h" -#include -#include -#include namespace clang { namespace clangd { namespace { -using ::testing::AllOf; using ::testing::ElementsAre; using ::testing::ElementsAreArray; using ::testing::IsEmpty; -using ::testing::Matcher; using ::testing::Pointee; using ::testing::UnorderedElementsAre; -Matcher withFix(::testing::Matcher FixMatcher) { - return Field(&Diag::Fixes, ElementsAre(FixMatcher)); -} - -MATCHER_P2(Diag, Range, Message, - "Diag at " + llvm::to_string(Range) + " = [" + Message + "]") { - return arg.Range == Range && arg.Message == Message; -} - -MATCHER_P3(Fix, Range, Replacement, Message, - "Fix " + llvm::to_string(Range) + " => " + - ::testing::PrintToString(Replacement) + " = [" + Message + "]") { - return arg.Message == Message && arg.Edits.size() == 1 && - arg.Edits[0].range == Range && arg.Edits[0].newText == Replacement; -} - std::string guard(llvm::StringRef Code) { return "#pragma once\n" + Code.str(); } @@ -374,8 +342,7 @@ TEST(IncludeCleaner, StdlibUnused) { auto AST = TU.build(); EXPECT_THAT(computeUnusedIncludes(AST), ElementsAre(Pointee(writtenInclusion("")))); - IncludeCleanerFindings Findings = computeIncludeCleanerFindings(AST); - EXPECT_THAT(Findings.UnusedIncludes, + EXPECT_THAT(computeUnusedIncludesExperimental(AST), ElementsAre(Pointee(writtenInclusion("")))); } @@ -412,139 +379,12 @@ TEST(IncludeCleaner, GetUnusedHeaders) { computeUnusedIncludes(AST), UnorderedElementsAre(Pointee(writtenInclusion("\"unused.h\"")), Pointee(writtenInclusion("\"dir/unused.h\"")))); - - IncludeCleanerFindings Findings = computeIncludeCleanerFindings(AST); EXPECT_THAT( - Findings.UnusedIncludes, + computeUnusedIncludesExperimental(AST), UnorderedElementsAre(Pointee(writtenInclusion("\"unused.h\"")), Pointee(writtenInclusion("\"dir/unused.h\"")))); } -TEST(IncludeCleaner, ComputeMissingHeaders) { - Annotations MainFile(R"cpp( - #include "a.h" - - void foo() { - $b[[b]](); - })cpp"); - TestTU TU; - TU.Filename = "foo.cpp"; - TU.AdditionalFiles["a.h"] = guard("#include \"b.h\""); - TU.AdditionalFiles["b.h"] = guard("void b();"); - - TU.Code = MainFile.code(); - ParsedAST AST = TU.build(); - - IncludeCleanerFindings Findings = computeIncludeCleanerFindings(AST); - const SourceManager &SM = AST.getSourceManager(); - const NamedDecl *BDecl = nullptr; - for (Decl *D : AST.getASTContext().getTranslationUnitDecl()->decls()) { - const NamedDecl *CandidateDecl = llvm::dyn_cast(D); - std::string Name = CandidateDecl->getQualifiedNameAsString(); - if (Name != "b") - continue; - BDecl = CandidateDecl; - } - ASSERT_TRUE(BDecl); - include_cleaner::Symbol B{*BDecl}; - auto Range = MainFile.range("b"); - size_t Start = llvm::cantFail(positionToOffset(MainFile.code(), Range.start)); - size_t End = llvm::cantFail(positionToOffset(MainFile.code(), Range.end)); - syntax::FileRange BRange{SM.getMainFileID(), static_cast(Start), - static_cast(End)}; - include_cleaner::Header Header{*SM.getFileManager().getFile("b.h")}; - MissingIncludeDiagInfo BInfo{B, BRange, {Header}}; - EXPECT_THAT(Findings.MissingIncludes, ElementsAre(BInfo)); -} - -TEST(IncludeCleaner, GenerateMissingHeaderDiags) { - Config Cfg; - Cfg.Diagnostics.MissingIncludes = Config::IncludesPolicy::Strict; - Cfg.Diagnostics.Includes.IgnoreHeader = {[](llvm::StringRef Header) { - return Header == testPath("buzz.h", llvm::sys::path::Style::posix); - }}; - WithContextValue Ctx(Config::Key, std::move(Cfg)); - Annotations MainFile(R"cpp( -#include "a.h" -$insert_b[[]]#include "baz.h" -#include "dir/c.h" -$insert_d[[]]#include "fuzz.h" -#include "header.h" -$insert_foobar[[]]#include -$insert_f[[]]$insert_vector[[]] - - void foo() { - $b[[b]](); - - ns::$bar[[Bar]] bar; - bar.d(); - $f[[f]](); - - // this should not be diagnosed, because it's ignored in the config - buzz(); - - $foobar[[foobar]](); - - std::$vector[[vector]] v; - })cpp"); - - TestTU TU; - TU.Filename = "foo.cpp"; - 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("namespace ns { struct Bar { 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.AdditionalFiles["fuzz.h"] = guard("#include \"buzz.h\""); - TU.AdditionalFiles["buzz.h"] = guard("void buzz();"); - - TU.AdditionalFiles["baz.h"] = guard("#include \"private.h\""); - TU.AdditionalFiles["private.h"] = guard(R"cpp( - // IWYU pragma: private, include "public.h" - void foobar(); - )cpp"); - TU.AdditionalFiles["header.h"] = guard(R"cpp( - namespace std { class vector {}; } - )cpp"); - - TU.Code = MainFile.code(); - ParsedAST AST = TU.build(); - - std::vector Diags = - issueIncludeCleanerDiagnostics(AST, TU.Code); - EXPECT_THAT( - Diags, - UnorderedElementsAre( - AllOf(Diag(MainFile.range("b"), - "No header providing \"b\" is directly included"), - withFix(Fix(MainFile.range("insert_b"), "#include \"b.h\"\n", - "#include \"b.h\""))), - AllOf(Diag(MainFile.range("bar"), - "No header providing \"ns::Bar\" is directly included"), - withFix(Fix(MainFile.range("insert_d"), - "#include \"dir/d.h\"\n", "#include \"dir/d.h\""))), - AllOf(Diag(MainFile.range("f"), - "No header providing \"f\" is directly included"), - withFix(Fix(MainFile.range("insert_f"), "#include \n", - "#include "))), - AllOf( - Diag(MainFile.range("foobar"), - "No header providing \"foobar\" is directly included"), - withFix(Fix(MainFile.range("insert_foobar"), - "#include \"public.h\"\n", "#include \"public.h\""))), - AllOf( - Diag(MainFile.range("vector"), - "No header providing \"std::vector\" is directly included"), - withFix(Fix(MainFile.range("insert_vector"), - "#include \n", "#include "))))); -} - TEST(IncludeCleaner, VirtualBuffers) { TestTU TU; TU.Code = R"cpp( @@ -698,7 +538,7 @@ TEST(IncludeCleaner, IWYUPragmas) { void foo() {} )cpp"); Config Cfg; - Cfg.Diagnostics.UnusedIncludes = Config::IncludesPolicy::Experiment; + Cfg.Diagnostics.UnusedIncludes = Config::UnusedIncludesPolicy::Experiment; WithContextValue Ctx(Config::Key, std::move(Cfg)); ParsedAST AST = TU.build(); @@ -714,8 +554,7 @@ TEST(IncludeCleaner, IWYUPragmas) { ReferencedFiles.User.contains(AST.getSourceManager().getMainFileID())); EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty())); EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty()); - IncludeCleanerFindings Findings = computeIncludeCleanerFindings(AST); - EXPECT_THAT(Findings.UnusedIncludes, IsEmpty()); + EXPECT_THAT(computeUnusedIncludesExperimental(AST), IsEmpty()); } TEST(IncludeCleaner, RecursiveInclusion) { @@ -744,8 +583,7 @@ TEST(IncludeCleaner, RecursiveInclusion) { EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty())); EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty()); - IncludeCleanerFindings Findings = computeIncludeCleanerFindings(AST); - EXPECT_THAT(Findings.UnusedIncludes, IsEmpty()); + EXPECT_THAT(computeUnusedIncludesExperimental(AST), IsEmpty()); } TEST(IncludeCleaner, IWYUPragmaExport) { @@ -770,8 +608,7 @@ TEST(IncludeCleaner, IWYUPragmaExport) { // FIXME: This is not correct: foo.h is unused but is not diagnosed as such // because we ignore headers with IWYU export pragmas for now. EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty()); - IncludeCleanerFindings Findings = computeIncludeCleanerFindings(AST); - EXPECT_THAT(Findings.UnusedIncludes, IsEmpty()); + EXPECT_THAT(computeUnusedIncludesExperimental(AST), IsEmpty()); } TEST(IncludeCleaner, NoDiagsForObjC) { @@ -790,9 +627,7 @@ TEST(IncludeCleaner, NoDiagsForObjC) { TU.ExtraArgs.emplace_back("-xobjective-c"); Config Cfg; - - Cfg.Diagnostics.UnusedIncludes = Config::IncludesPolicy::Strict; - Cfg.Diagnostics.MissingIncludes = Config::IncludesPolicy::Strict; + Cfg.Diagnostics.UnusedIncludes = Config::UnusedIncludesPolicy::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/clangd/unittests/PreambleTests.cpp b/clang-tools-extra/clangd/unittests/PreambleTests.cpp index bee22c8..94870ab 100644 --- a/clang-tools-extra/clangd/unittests/PreambleTests.cpp +++ b/clang-tools-extra/clangd/unittests/PreambleTests.cpp @@ -665,7 +665,7 @@ TEST(PreamblePatch, DiagnosticsFromMainASTAreInRightPlace) { TEST(PreamblePatch, DiagnosticsToPreamble) { Config Cfg; Cfg.Diagnostics.AllowStalePreamble = true; - Cfg.Diagnostics.UnusedIncludes = Config::IncludesPolicy::Strict; + Cfg.Diagnostics.UnusedIncludes = Config::UnusedIncludesPolicy::Strict; WithContextValue WithCfg(Config::Key, std::move(Cfg)); llvm::StringMap AdditionalFiles; 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 index cd11700..f6afaff 100644 --- 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,8 +73,6 @@ AnalysisResults analyze(llvm::ArrayRef ASTRoots, std::string fixIncludes(const AnalysisResults &Results, llvm::StringRef Code, const format::FormatStyle &IncludeStyle); -std::string spellHeader(const Header &H, HeaderSearch &HS, - const FileEntry *Main); } // namespace include_cleaner } // namespace clang diff --git a/clang-tools-extra/include-cleaner/lib/Analysis.cpp b/clang-tools-extra/include-cleaner/lib/Analysis.cpp index 6237bdb..c5559db 100644 --- a/clang-tools-extra/include-cleaner/lib/Analysis.cpp +++ b/clang-tools-extra/include-cleaner/lib/Analysis.cpp @@ -49,8 +49,8 @@ void walkUsed(llvm::ArrayRef ASTRoots, } } -std::string spellHeader(const Header &H, HeaderSearch &HS, - const FileEntry *Main) { +static std::string spellHeader(const Header &H, HeaderSearch &HS, + const FileEntry *Main) { switch (H.kind()) { case Header::Physical: { bool IsSystem = false; -- 2.7.4