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 @@ -45,6 +45,7 @@ return std::tie(SymRefRange, Providers, Symbol) == std::tie(Other.SymRefRange, Other.Providers, Other.Symbol); } + include_cleaner::Header header() const; }; 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 @@ -45,6 +45,7 @@ #include "llvm/ADT/STLFunctionalExtras.h" #include "llvm/ADT/SmallString.h" #include "llvm/ADT/SmallVector.h" +#include "llvm/ADT/StringExtras.h" #include "llvm/ADT/StringMap.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Casting.h" @@ -53,6 +54,7 @@ #include "llvm/Support/FormatVariadic.h" #include "llvm/Support/Path.h" #include "llvm/Support/Regex.h" +#include #include #include #include @@ -144,10 +146,11 @@ llvm_unreachable("Unknown header kind"); } -std::vector generateMissingIncludeDiagnostics( +using MissingIncludeEdits = llvm::MapVector; +MissingIncludeEdits generateMissingIncludeEdits( ParsedAST &AST, llvm::ArrayRef MissingIncludes, llvm::StringRef Code, HeaderFilter IgnoreHeaders) { - std::vector Result; + MissingIncludeEdits Result; const SourceManager &SM = AST.getSourceManager(); const FileEntry *MainFile = SM.getFileEntryForID(SM.getMainFileID()); @@ -162,8 +165,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 (isIgnored(ResolvedPath, IgnoreHeaders)) { dlog("IncludeCleaner: not diagnosing missing include {0}, filtered by " "config", @@ -172,7 +178,7 @@ } std::string Spelling = include_cleaner::spellHeader( - {SymbolWithMissingInclude.Providers.front(), + {SymbolWithMissingInclude.header(), AST.getPreprocessor().getHeaderSearchInfo(), MainFile}); llvm::StringRef HeaderRef{Spelling}; @@ -185,6 +191,22 @@ HeaderRef.trim("\"<>"), Angled, tooling::IncludeDirective::Include); if (!Replacement.has_value()) continue; + TextEdit Edit = replacementToEdit(Code, *Replacement); + Result.insert({SymbolWithMissingInclude.header(), Edit}); + } + + return Result; +} + +std::vector generateMissingIncludeDiagnostics( + ParsedAST &AST, llvm::ArrayRef MissingIncludes, + llvm::StringRef Code, const MissingIncludeEdits &Edits) { + std::vector Result; + + for (const auto &SymbolWithMissingInclude : MissingIncludes) { + auto EditIt = Edits.find(SymbolWithMissingInclude.header()); + if (EditIt == Edits.end()) + continue; Diag &D = Result.emplace_back(); D.Message = @@ -217,9 +239,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; } @@ -259,62 +280,77 @@ } std::optional -removeAllUnusedIncludes(llvm::ArrayRef UnusedIncludes) { - if (UnusedIncludes.empty()) +removeAllUnusedIncludes(llvm::ArrayRef Includes) { + if (Includes.empty()) return std::nullopt; 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}); + 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; } -std::optional -addAllMissingIncludes(llvm::ArrayRef MissingIncludeDiags) { +std::optional addAllMissingIncludes( + llvm::ArrayRef MissingIncludeDiags, + const MissingIncludeEdits &Edits) { if (MissingIncludeDiags.empty()) return std::nullopt; 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); - } + + llvm::DenseMap> + HeaderToSymbols; + for (const auto &Diag : MissingIncludeDiags) + HeaderToSymbols[Diag.header()].push_back(Diag.Symbol.name()); + for (auto &[_, Symbols] : HeaderToSymbols) { + // Deduplicate symbol names. + llvm::sort(Symbols); + Symbols.erase(std::unique(Symbols.begin(), Symbols.end()), Symbols.end()); } - // FIXME(hokein): emit used symbol reference in the annotation. - ChangeAnnotation Annotation = {/*label=*/"", - /*needsConfirmation=*/true, - /*description=*/""}; + 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::join(Symbols.take_front(MaxSymbols), ", "); + if (Symbols.size() > MaxSymbols) + OS << " and " << Symbols.size() - MaxSymbols << " more"; + return OS.str(); + }; unsigned I = 0; - for (auto &It : Edits) { + // FIXME: figure out a way to better handle cases where multiple #include are + // inserted at the same location, the order is somewhat arbitrary now. + // LSP spec says that order reported by the server will be preserved. + for (auto &[Header, Edit] : Edits) { ChangeAnnotationIdentifier ID = AddAllMissingID + std::to_string(I++); - AddAllMissing.Edits.push_back(std::move(It.getValue())); + AddAllMissing.Edits.push_back(Edit); AddAllMissing.Edits.back().annotationId = ID; - AddAllMissing.Annotations.push_back({ID, Annotation}); + AddAllMissing.Annotations.push_back( + {ID, ChangeAnnotation{ + /*label=*/llvm::formatv("{0}", + StringRef(Edit.newText).trim('\n')), + /*needsConfirmation=*/true, + /*description=*/ + ProvidedSymbols(HeaderToSymbols[Header])}}); } return AddAllMissing; } @@ -357,6 +393,11 @@ } // namespace +include_cleaner::Header MissingIncludeDiagInfo::header() const { + assert(!Providers.empty()); + return Providers.front(); +} + std::vector collectMacroReferences(ParsedAST &AST) { const auto &SM = AST.getSourceManager(); @@ -493,11 +534,15 @@ trace::Span Tracer("IncludeCleaner::issueIncludeCleanerDiagnostics"); std::vector UnusedIncludes = generateUnusedIncludeDiagnostics( AST.tuPath(), Findings.UnusedIncludes, Code, IgnoreHeaders); - std::optional RemoveAllUnused = removeAllUnusedIncludes(UnusedIncludes); + std::optional RemoveAllUnused = + removeAllUnusedIncludes(Findings.UnusedIncludes); - std::vector MissingIncludeDiags = generateMissingIncludeDiagnostics( + auto AddMissingIncludesEdits = generateMissingIncludeEdits( AST, Findings.MissingIncludes, Code, IgnoreHeaders); - std::optional AddAllMissing = addAllMissingIncludes(MissingIncludeDiags); + auto MissingIncludeDiags = generateMissingIncludeDiagnostics( + AST, Findings.MissingIncludes, Code, AddMissingIncludesEdits); + std::optional 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 @@ -118,7 +118,7 @@ # CHECK-NEXT: "version": 0 # CHECK-NEXT: } --- -{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///simple.cpp"},"range":{"start":{"line":2,"character":1},"end":{"line":2,"character":4}},"context":{"diagnostics":[{"range":{"start": {"line": 2, "character": 1}, "end": {"line": 2, "character": 4}},"severity":3,"message":"No header providing \"Foo\" is directly included (fixes available)", "code": "missing-includes", "source": "clangd"}]}}} +{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///simple.cpp"},"range":{"start":{"line":2,"character":1},"end":{"line":2,"character":4}},"context":{"diagnostics":[{"range":{"start": {"line": 2, "character": 1}, "end": {"line": 2, "character": 4}},"severity":2,"message":"No header providing \"Foo\" is directly included (fixes available)", "code": "missing-includes", "source": "clangd"}]}}} # CHECK: "id": 2, # CHECK-NEXT: "jsonrpc": "2.0", # CHECK-NEXT: "result": [ @@ -158,11 +158,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: }, @@ -171,7 +173,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, @@ -185,7 +187,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, @@ -214,19 +216,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: }, @@ -263,7 +267,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, @@ -277,7 +281,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, @@ -343,11 +347,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: }, @@ -399,19 +403,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: }, @@ -448,7 +454,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, @@ -462,7 +468,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,