diff --git a/clang-tools-extra/clangd/Hover.cpp b/clang-tools-extra/clangd/Hover.cpp --- a/clang-tools-extra/clangd/Hover.cpp +++ b/clang-tools-extra/clangd/Hover.cpp @@ -1225,22 +1225,6 @@ HI.Provider = spellHeader(AST, SM.getFileEntryForID(SM.getMainFileID()), H); } -// FIXME: similar functions are present in FindHeaders.cpp (symbolName) -// and IncludeCleaner.cpp (getSymbolName). Introduce a name() method into -// include_cleaner::Symbol instead. -std::string getSymbolName(include_cleaner::Symbol Sym) { - std::string Name; - switch (Sym.kind()) { - case include_cleaner::Symbol::Declaration: - if (const auto *ND = llvm::dyn_cast(&Sym.declaration())) - Name = ND->getDeclName().getAsString(); - break; - case include_cleaner::Symbol::Macro: - Name = Sym.macro().Name->getName(); - break; - } - return Name; -} void maybeAddUsedSymbols(ParsedAST &AST, HoverInfo &HI, const Inclusion &Inc) { const SourceManager &SM = AST.getSourceManager(); @@ -1266,7 +1250,7 @@ }); for (const auto &UsedSymbolDecl : UsedSymbols) - HI.UsedSymbolNames.push_back(getSymbolName(UsedSymbolDecl)); + HI.UsedSymbolNames.emplace_back(UsedSymbolDecl.name()); llvm::sort(HI.UsedSymbolNames); } 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 @@ -24,6 +24,7 @@ #include "clang/Tooling/Syntax/Tokens.h" #include "llvm/ADT/DenseSet.h" #include "llvm/ADT/StringSet.h" +#include #include #include @@ -40,6 +41,10 @@ return std::tie(SymRefRange, Providers, Symbol) == std::tie(Other.SymRefRange, Other.Providers, Other.Symbol); } + include_cleaner::Header header() const { + assert(!Providers.empty()); + return Providers[0]; + } }; struct IncludeCleanerFindings { 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 @@ -39,13 +39,15 @@ #include "clang/Tooling/Inclusions/StandardLibrary.h" #include "clang/Tooling/Syntax/Tokens.h" #include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/DenseMap.h" #include "llvm/ADT/DenseSet.h" #include "llvm/ADT/GenericUniformityImpl.h" +#include "llvm/ADT/MapVector.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/STLFunctionalExtras.h" +#include "llvm/ADT/SetVector.h" #include "llvm/ADT/SmallString.h" #include "llvm/ADT/SmallVector.h" -#include "llvm/ADT/StringMap.h" #include "llvm/ADT/StringRef.h" #include "llvm/ADT/StringSet.h" #include "llvm/Support/Casting.h" @@ -54,6 +56,7 @@ #include "llvm/Support/FormatVariadic.h" #include "llvm/Support/Path.h" #include "llvm/Support/Regex.h" +#include "llvm/Support/raw_ostream.h" #include #include #include @@ -163,19 +166,14 @@ llvm_unreachable("Unknown symbol kind"); } -std::vector generateMissingIncludeDiagnostics( +using MissingIncludeEdits = llvm::MapVector; +MissingIncludeEdits generateMissingIncludeEdits( 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; - } - + MissingIncludeEdits Result; const SourceManager &SM = AST.getSourceManager(); const FileEntry *MainFile = SM.getFileEntryForID(SM.getMainFileID()); + const Config &Cfg = Config::current(); auto FileStyle = format::getStyle( format::DefaultFormatStyle, AST.tuPath(), format::DefaultFallbackStyle, @@ -188,8 +186,11 @@ tooling::HeaderIncludes HeaderIncludes(AST.tuPath(), Code, FileStyle->IncludeStyle); for (const auto &SymbolWithMissingInclude : MissingIncludes) { + if (Result.contains(SymbolWithMissingInclude.header())) + continue; + llvm::StringRef ResolvedPath = - getResolvedPath(SymbolWithMissingInclude.Providers.front()); + getResolvedPath(SymbolWithMissingInclude.header()); if (isFilteredByConfig(Cfg, ResolvedPath)) { dlog("IncludeCleaner: not diagnosing missing include {0}, filtered by " "config", @@ -198,7 +199,7 @@ } std::string Spelling = - spellHeader(AST, MainFile, SymbolWithMissingInclude.Providers.front()); + spellHeader(AST, MainFile, SymbolWithMissingInclude.header()); llvm::StringRef HeaderRef{Spelling}; bool Angled = HeaderRef.starts_with("<"); // We might suggest insertion of an existing include in edge cases, e.g., @@ -209,8 +210,35 @@ HeaderRef.trim("\"<>"), Angled, tooling::IncludeDirective::Include); if (!Replacement.has_value()) continue; + TextEdit Edit = replacementToEdit(Code, *Replacement); + Result.insert({SymbolWithMissingInclude.header(), Edit}); + } + return Result; +} - Diag &D = Result.emplace_back(); +std::vector generateMissingIncludeDiagnostics( + ParsedAST &AST, llvm::ArrayRef MissingIncludes, + llvm::StringRef Code, const MissingIncludeEdits &Edits) { + 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()); + + for (const auto &SymbolWithMissingInclude : MissingIncludes) { + auto EditIt = Edits.find(SymbolWithMissingInclude.header()); + if (EditIt == Edits.end()) + continue; + + std::string Spelling = + spellHeader(AST, MainFile, SymbolWithMissingInclude.header()); + + auto & D = Result.emplace_back(); D.Message = llvm::formatv("No header providing \"{0}\" is directly included", getSymbolName(SymbolWithMissingInclude.Symbol)); @@ -225,9 +253,8 @@ 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)); + F.Message = llvm::StringRef(EditIt->second.newText).rtrim("\n"); + F.Edits.emplace_back(EditIt->second); } return Result; } @@ -269,6 +296,95 @@ } return Result; } + +Fix removeAllUnusedIncludes(llvm::ArrayRef Includes) { + assert(!Includes.empty()); + Fix RemoveAll; + RemoveAll.Message = "remove all unused includes"; + static const ChangeAnnotationIdentifier RemoveAllUnusedID = + "RemoveAllUnusedIncludes"; + unsigned I = 0; + for (const auto *Inc : Includes) { + RemoveAll.Edits.emplace_back(); + RemoveAll.Edits.back().range.start.line = Inc->HashLine; + RemoveAll.Edits.back().range.end.line = Inc->HashLine + 1; + ChangeAnnotationIdentifier ID = RemoveAllUnusedID + std::to_string(I++); + RemoveAll.Edits.back().annotationId = ID; + RemoveAll.Annotations.push_back( + {ID, ChangeAnnotation{ + /*label=*/llvm::formatv("Remove '#include {0}'", Inc->Written), + /*needsConfirmation=*/true, + /*description=*/""}}); + } + return RemoveAll; +} + +Fix addAllMissingIncludes( + llvm::ArrayRef MissingIncludeDiags, + const MissingIncludeEdits &Edits) { + assert(!MissingIncludeDiags.empty()); + + Fix AddAllMissing; + AddAllMissing.Message = "add all missing includes"; + llvm::DenseMap> + HeaderToSymbols; + for (const auto &Diag : MissingIncludeDiags) + HeaderToSymbols[Diag.header()].insert(Diag.Symbol); + + static const ChangeAnnotationIdentifier AddAllMissingID = + "AddAllMissingIncludes"; + auto ProvidedSymbols = [](llvm::ArrayRef Symbols) { + std::string Result; + llvm::raw_string_ostream OS(Result); + static constexpr int MaxSymbols = 5; + OS << "Provides "; + llvm::interleave( + Symbols.take_front(MaxSymbols), + [&OS](const include_cleaner::Symbol &S) { OS << S.name(); }, + [&OS]() { OS << ", "; }); + if (Symbols.size() > MaxSymbols) + OS << " and " << Symbols.size() - MaxSymbols << " more"; + return OS.str(); + }; + unsigned I = 0; + for (auto &[Header, Edit] : Edits) { + ChangeAnnotationIdentifier ID = AddAllMissingID + std::to_string(I++); + AddAllMissing.Edits.push_back(Edit); + AddAllMissing.Edits.back().annotationId = ID; + + auto Symbols = HeaderToSymbols[Header].takeVector(); + llvm::sort(Symbols, [](const auto &L, const auto &R) { + return L.name() < R.name(); + }); + + AddAllMissing.Annotations.push_back( + {ID, ChangeAnnotation{ + /*label=*/llvm::formatv("{0}", + StringRef(Edit.newText).trim('\n')), + /*needsConfirmation=*/true, + /*description=*/ + ProvidedSymbols(Symbols)}}); + } + return AddAllMissing; +} + +Fix fixAll(const Fix& RemoveAllUnused, const Fix& AddAllMissing) { + Fix FixAll; + FixAll.Message = "fix all includes"; + + for (const auto &F : RemoveAllUnused.Edits) + FixAll.Edits.push_back(F); + for (const auto &F : AddAllMissing.Edits) + FixAll.Edits.push_back(F); + + for (const auto& A : RemoveAllUnused.Annotations) + FixAll.Annotations.push_back(A); + for (const auto& A : AddAllMissing.Annotations) + FixAll.Annotations.push_back(A); + return FixAll; +} + } // namespace std::vector @@ -440,77 +556,6 @@ return {std::move(UnusedIncludes), std::move(MissingIncludes)}; } -Fix removeAllUnusedIncludes(llvm::ArrayRef UnusedIncludes) { - assert(!UnusedIncludes.empty()); - - Fix RemoveAll; - RemoveAll.Message = "remove all unused includes"; - for (const auto &Diag : UnusedIncludes) { - assert(Diag.Fixes.size() == 1 && "Expected exactly one fix."); - RemoveAll.Edits.insert(RemoveAll.Edits.end(), - Diag.Fixes.front().Edits.begin(), - Diag.Fixes.front().Edits.end()); - } - - // TODO(hokein): emit a suitable text for the label. - ChangeAnnotation Annotation = {/*label=*/"", - /*needsConfirmation=*/true, - /*description=*/""}; - static const ChangeAnnotationIdentifier RemoveAllUnusedID = - "RemoveAllUnusedIncludes"; - for (unsigned I = 0; I < RemoveAll.Edits.size(); ++I) { - ChangeAnnotationIdentifier ID = RemoveAllUnusedID + std::to_string(I); - RemoveAll.Edits[I].annotationId = ID; - RemoveAll.Annotations.push_back({ID, Annotation}); - } - return RemoveAll; -} -Fix addAllMissingIncludes(llvm::ArrayRef MissingIncludeDiags) { - assert(!MissingIncludeDiags.empty()); - - Fix AddAllMissing; - AddAllMissing.Message = "add all missing includes"; - // A map to deduplicate the edits with the same new text. - // newText (#include "my_missing_header.h") -> TextEdit. - llvm::StringMap Edits; - for (const auto &Diag : MissingIncludeDiags) { - assert(Diag.Fixes.size() == 1 && "Expected exactly one fix."); - for (const auto& Edit : Diag.Fixes.front().Edits) { - Edits.try_emplace(Edit.newText, Edit); - } - } - // FIXME(hokein): emit used symbol reference in the annotation. - ChangeAnnotation Annotation = {/*label=*/"", - /*needsConfirmation=*/true, - /*description=*/""}; - static const ChangeAnnotationIdentifier AddAllMissingID = - "AddAllMissingIncludes"; - unsigned I = 0; - for (auto &It : Edits) { - ChangeAnnotationIdentifier ID = AddAllMissingID + std::to_string(I++); - AddAllMissing.Edits.push_back(std::move(It.getValue())); - AddAllMissing.Edits.back().annotationId = ID; - - AddAllMissing.Annotations.push_back({ID, Annotation}); - } - return AddAllMissing; -} -Fix fixAll(const Fix& RemoveAllUnused, const Fix& AddAllMissing) { - Fix FixAll; - FixAll.Message = "fix all includes"; - - for (const auto &F : RemoveAllUnused.Edits) - FixAll.Edits.push_back(F); - for (const auto &F : AddAllMissing.Edits) - FixAll.Edits.push_back(F); - - for (const auto& A : RemoveAllUnused.Annotations) - FixAll.Annotations.push_back(A); - for (const auto& A : AddAllMissing.Annotations) - FixAll.Annotations.push_back(A); - return FixAll; -} - std::vector generateIncludeCleanerDiagnostic( ParsedAST &AST, const IncludeCleanerFindings &Findings, llvm::StringRef Code) { @@ -518,13 +563,16 @@ AST.tuPath(), Findings.UnusedIncludes, Code); std::optional RemoveAllUnused;; if (UnusedIncludes.size() > 1) - RemoveAllUnused = removeAllUnusedIncludes(UnusedIncludes); + RemoveAllUnused = removeAllUnusedIncludes(Findings.UnusedIncludes); - std::vector MissingIncludeDiags = generateMissingIncludeDiagnostics( - AST, Findings.MissingIncludes, Code); + auto AddMissingIncludesEdits = + generateMissingIncludeEdits(AST, Findings.MissingIncludes, Code); + auto MissingIncludeDiags = generateMissingIncludeDiagnostics( + AST, Findings.MissingIncludes, Code, AddMissingIncludesEdits); std::optional AddAllMissing; if (MissingIncludeDiags.size() > 1) - AddAllMissing = addAllMissingIncludes(MissingIncludeDiags); + AddAllMissing = addAllMissingIncludes(Findings.MissingIncludes, + AddMissingIncludesEdits); std::optional FixAll; if (RemoveAllUnused && AddAllMissing) diff --git a/clang-tools-extra/clangd/test/include-cleaner-batch-fix.test b/clang-tools-extra/clangd/test/include-cleaner-batch-fix.test --- a/clang-tools-extra/clangd/test/include-cleaner-batch-fix.test +++ b/clang-tools-extra/clangd/test/include-cleaner-batch-fix.test @@ -152,11 +152,13 @@ # CHECK-NEXT: { # CHECK-NEXT: "changeAnnotations": { # CHECK-NEXT: "AddAllMissingIncludes0": { -# CHECK-NEXT: "label": "", +# CHECK-NEXT: "description": "Provides Foo", +# CHECK-NEXT: "label": "{{.*}}foo.h{{.*}}", # CHECK-NEXT: "needsConfirmation": true # CHECK-NEXT: }, # CHECK-NEXT: "AddAllMissingIncludes1": { -# CHECK-NEXT: "label": "", +# CHECK-NEXT: "description": "Provides Bar", +# CHECK-NEXT: "label": "{{.*}}bar.h{{.*}}", # CHECK-NEXT: "needsConfirmation": true # CHECK-NEXT: } # CHECK-NEXT: }, @@ -165,7 +167,7 @@ # CHECK-NEXT: "edits": [ # CHECK-NEXT: { # CHECK-NEXT: "annotationId": "AddAllMissingIncludes0", -# CHECK-NEXT: "newText": "#include {{.*}}bar.h{{.*}}", +# CHECK-NEXT: "newText": "#include {{.*}}foo.h{{.*}}", # CHECK-NEXT: "range": { # CHECK-NEXT: "end": { # CHECK-NEXT: "character": 0, @@ -179,7 +181,7 @@ # CHECK-NEXT: }, # CHECK-NEXT: { # CHECK-NEXT: "annotationId": "AddAllMissingIncludes1", -# CHECK-NEXT: "newText": "#include {{.*}}foo.h{{.*}}", +# CHECK-NEXT: "newText": "#include {{.*}}bar.h{{.*}}", # CHECK-NEXT: "range": { # CHECK-NEXT: "end": { # CHECK-NEXT: "character": 0, @@ -208,19 +210,21 @@ # CHECK-NEXT: { # CHECK-NEXT: "changeAnnotations": { # CHECK-NEXT: "AddAllMissingIncludes0": { -# CHECK-NEXT: "label": "", +# CHECK-NEXT: "description": "Provides Foo", +# CHECK-NEXT: "label": "{{.*}}foo.h{{.*}}", # CHECK-NEXT: "needsConfirmation": true # CHECK-NEXT: }, # CHECK-NEXT: "AddAllMissingIncludes1": { -# CHECK-NEXT: "label": "", +# CHECK-NEXT: "description": "Provides Bar", +# CHECK-NEXT: "label": "{{.*}}bar.h{{.*}}", # CHECK-NEXT: "needsConfirmation": true # CHECK-NEXT: }, # CHECK-NEXT: "RemoveAllUnusedIncludes0": { -# CHECK-NEXT: "label": "", +# CHECK-NEXT: "label": "Remove {{.*}}all1.h{{.*}}", # CHECK-NEXT: "needsConfirmation": true # CHECK-NEXT: }, # CHECK-NEXT: "RemoveAllUnusedIncludes1": { -# CHECK-NEXT: "label": "", +# CHECK-NEXT: "label": "Remove {{.*}}all2.h{{.*}}", # CHECK-NEXT: "needsConfirmation": true # CHECK-NEXT: } # CHECK-NEXT: }, @@ -257,7 +261,7 @@ # CHECK-NEXT: }, # CHECK-NEXT: { # CHECK-NEXT: "annotationId": "AddAllMissingIncludes0", -# CHECK-NEXT: "newText": "#include {{.*}}bar.h{{.*}}", +# CHECK-NEXT: "newText": "#include {{.*}}foo.h{{.*}}", # CHECK-NEXT: "range": { # CHECK-NEXT: "end": { # CHECK-NEXT: "character": 0, @@ -271,7 +275,7 @@ # CHECK-NEXT: }, # CHECK-NEXT: { # CHECK-NEXT: "annotationId": "AddAllMissingIncludes1", -# CHECK-NEXT: "newText": "#include {{.*}}foo.h{{.*}}", +# CHECK-NEXT: "newText": "#include {{.*}}bar.h{{.*}}", # CHECK-NEXT: "range": { # CHECK-NEXT: "end": { # CHECK-NEXT: "character": 0, @@ -337,11 +341,11 @@ # CHECK-NEXT: { # CHECK-NEXT: "changeAnnotations": { # CHECK-NEXT: "RemoveAllUnusedIncludes0": { -# CHECK-NEXT: "label": "", +# CHECK-NEXT: "label": "Remove {{.*}}", # CHECK-NEXT: "needsConfirmation": true # CHECK-NEXT: }, # CHECK-NEXT: "RemoveAllUnusedIncludes1": { -# CHECK-NEXT: "label": "", +# CHECK-NEXT: "label": "Remove {{.*}}", # CHECK-NEXT: "needsConfirmation": true # CHECK-NEXT: } # CHECK-NEXT: }, @@ -393,19 +397,21 @@ # CHECK-NEXT: { # CHECK-NEXT: "changeAnnotations": { # CHECK-NEXT: "AddAllMissingIncludes0": { -# CHECK-NEXT: "label": "", +# CHECK-NEXT: "description": "Provides {{.*}}", +# CHECK-NEXT: "label": "{{.*}}", # CHECK-NEXT: "needsConfirmation": true # CHECK-NEXT: }, # CHECK-NEXT: "AddAllMissingIncludes1": { -# CHECK-NEXT: "label": "", +# CHECK-NEXT: "description": "Provides {{.*}}", +# CHECK-NEXT: "label": "{{.*}}", # CHECK-NEXT: "needsConfirmation": true # CHECK-NEXT: }, # CHECK-NEXT: "RemoveAllUnusedIncludes0": { -# CHECK-NEXT: "label": "", +# CHECK-NEXT: "label": "Remove {{.*}}", # CHECK-NEXT: "needsConfirmation": true # CHECK-NEXT: }, # CHECK-NEXT: "RemoveAllUnusedIncludes1": { -# CHECK-NEXT: "label": "", +# CHECK-NEXT: "label": "Remove {{.*}}", # CHECK-NEXT: "needsConfirmation": true # CHECK-NEXT: } # CHECK-NEXT: }, @@ -442,7 +448,7 @@ # CHECK-NEXT: }, # CHECK-NEXT: { # CHECK-NEXT: "annotationId": "AddAllMissingIncludes0", -# CHECK-NEXT: "newText": "#include {{.*}}bar.h{{.*}}", +# CHECK-NEXT: "newText": "#include {{.*}}foo.h{{.*}}", # CHECK-NEXT: "range": { # CHECK-NEXT: "end": { # CHECK-NEXT: "character": 0, @@ -456,7 +462,7 @@ # CHECK-NEXT: }, # CHECK-NEXT: { # CHECK-NEXT: "annotationId": "AddAllMissingIncludes1", -# CHECK-NEXT: "newText": "#include {{.*}}foo.h{{.*}}", +# CHECK-NEXT: "newText": "#include {{.*}}bar.h{{.*}}", # CHECK-NEXT: "range": { # CHECK-NEXT: "end": { # CHECK-NEXT: "character": 0, diff --git a/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h b/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h --- a/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h +++ b/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h @@ -68,6 +68,8 @@ const Decl &declaration() const { return *std::get(Storage); } struct Macro macro() const { return std::get(Storage); } + std::string name() const; + private: // Order must match Kind enum! std::variant Storage; diff --git a/clang-tools-extra/include-cleaner/lib/Types.cpp b/clang-tools-extra/include-cleaner/lib/Types.cpp --- a/clang-tools-extra/include-cleaner/lib/Types.cpp +++ b/clang-tools-extra/include-cleaner/lib/Types.cpp @@ -15,6 +15,18 @@ namespace clang::include_cleaner { +std::string Symbol::name() const { + switch (kind()) { + case Symbol::Declaration: + if (const auto *ND = llvm::dyn_cast(&declaration())) + return ND->getNameAsString(); + return ""; + case Symbol::Macro: + return macro().Name->getName().str(); + } + llvm_unreachable("unhandled Symbol kind!"); +} + llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Symbol &S) { switch (S.kind()) { case Symbol::Declaration: @@ -136,4 +148,5 @@ } llvm_unreachable("unhandled Header kind"); } + } // namespace clang::include_cleaner