hokein updated this revision to Diff 532117.
hokein marked 2 inline comments as done.
hokein added a comment.
rebase, and address review comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D149437/new/
https://reviews.llvm.org/D149437
Files:
clang-tools-extra/clangd/IncludeCleaner.cpp
clang-tools-extra/clangd/IncludeCleaner.h
clang-tools-extra/clangd/test/include-cleaner-batch-fix.test
Index: clang-tools-extra/clangd/test/include-cleaner-batch-fix.test
===================================================================
--- clang-tools-extra/clangd/test/include-cleaner-batch-fix.test
+++ 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,
Index: clang-tools-extra/clangd/IncludeCleaner.h
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.h
+++ 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 {
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ 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 <algorithm>
#include <cassert>
#include <iterator>
#include <optional>
@@ -144,10 +146,11 @@
llvm_unreachable("Unknown header kind");
}
-std::vector<Diag> generateMissingIncludeDiagnostics(
+using MissingIncludeEdits = llvm::MapVector<include_cleaner::Header, TextEdit>;
+MissingIncludeEdits generateMissingIncludeEdits(
ParsedAST &AST, llvm::ArrayRef<MissingIncludeDiagInfo> MissingIncludes,
llvm::StringRef Code, HeaderFilter IgnoreHeaders) {
- std::vector<Diag> 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<Diag> generateMissingIncludeDiagnostics(
+ ParsedAST &AST, llvm::ArrayRef<MissingIncludeDiagInfo> MissingIncludes,
+ llvm::StringRef Code, const MissingIncludeEdits &Edits) {
+ std::vector<Diag> 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<Fix>
-removeAllUnusedIncludes(llvm::ArrayRef<Diag> UnusedIncludes) {
- if (UnusedIncludes.empty())
+removeAllUnusedIncludes(llvm::ArrayRef<const Inclusion *> 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<Fix>
-addAllMissingIncludes(llvm::ArrayRef<Diag> MissingIncludeDiags) {
+std::optional<Fix> addAllMissingIncludes(
+ llvm::ArrayRef<MissingIncludeDiagInfo> 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<TextEdit> 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<include_cleaner::Header,
+ /*symbol names*/std::vector<std::string>>
+ 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<std::string> 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<include_cleaner::SymbolReference>
collectMacroReferences(ParsedAST &AST) {
const auto &SM = AST.getSourceManager();
@@ -493,11 +534,15 @@
trace::Span Tracer("IncludeCleaner::issueIncludeCleanerDiagnostics");
std::vector<Diag> UnusedIncludes = generateUnusedIncludeDiagnostics(
AST.tuPath(), Findings.UnusedIncludes, Code, IgnoreHeaders);
- std::optional<Fix> RemoveAllUnused = removeAllUnusedIncludes(UnusedIncludes);
+ std::optional<Fix> RemoveAllUnused =
+ removeAllUnusedIncludes(Findings.UnusedIncludes);
- std::vector<Diag> MissingIncludeDiags = generateMissingIncludeDiagnostics(
+ auto AddMissingIncludesEdits = generateMissingIncludeEdits(
AST, Findings.MissingIncludes, Code, IgnoreHeaders);
- std::optional<Fix> AddAllMissing = addAllMissingIncludes(MissingIncludeDiags);
+ auto MissingIncludeDiags = generateMissingIncludeDiagnostics(
+ AST, Findings.MissingIncludes, Code, AddMissingIncludesEdits);
+ std::optional<Fix> AddAllMissing =
+ addAllMissingIncludes(Findings.MissingIncludes, AddMissingIncludesEdits);
std::optional<Fix> FixAll;
if (RemoveAllUnused && AddAllMissing)
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
