hokein updated this revision to Diff 516384.
hokein added a comment.
some cleanups.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D147684/new/
https://reviews.llvm.org/D147684
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdLSPServer.h
clang-tools-extra/clangd/Diagnostics.h
clang-tools-extra/clangd/IncludeCleaner.cpp
clang-tools-extra/clangd/Protocol.cpp
clang-tools-extra/clangd/Protocol.h
clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
Index: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -45,8 +45,8 @@
using ::testing::Pointee;
using ::testing::UnorderedElementsAre;
-Matcher<const Diag &> withFix(::testing::Matcher<Fix> FixMatcher) {
- return Field(&Diag::Fixes, ElementsAre(FixMatcher));
+Matcher<const Diag &> withFix(std::vector<::testing::Matcher<Fix>> FixMatcheres) {
+ return Field(&Diag::Fixes, testing::UnorderedElementsAreArray(FixMatcheres));
}
MATCHER_P2(Diag, Range, Message,
@@ -60,6 +60,8 @@
return arg.Message == Message && arg.Edits.size() == 1 &&
arg.Edits[0].range == Range && arg.Edits[0].newText == Replacement;
}
+MATCHER_P(FixMessage, Message, "") { return arg.Message == Message; }
+
std::string guard(llvm::StringRef Code) {
return "#pragma once\n" + Code.str();
@@ -255,42 +257,51 @@
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\""))),
+ withFix({Fix(MainFile.range("insert_b"), "#include \"b.h\"\n",
+ "#include \"b.h\""),
+ FixMessage("add all missing includes")})),
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\""))),
+ withFix({Fix(MainFile.range("insert_d"),
+ "#include \"dir/d.h\"\n", "#include \"dir/d.h\""),
+ FixMessage("add all missing includes")})),
AllOf(Diag(MainFile.range("f"),
"No header providing \"f\" is directly included"),
- withFix(Fix(MainFile.range("insert_f"), "#include <f.h>\n",
- "#include <f.h>"))),
+ withFix({Fix(MainFile.range("insert_f"), "#include <f.h>\n",
+ "#include <f.h>"),
+ FixMessage("add all missing includes")})),
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\""))),
+ withFix({Fix(MainFile.range("insert_foobar"),
+ "#include \"public.h\"\n", "#include \"public.h\""),
+ FixMessage("add all missing includes")})),
AllOf(
Diag(MainFile.range("vector"),
"No header providing \"std::vector\" is directly included"),
- withFix(Fix(MainFile.range("insert_vector"),
- "#include <vector>\n", "#include <vector>"))),
+ withFix({Fix(MainFile.range("insert_vector"),
+ "#include <vector>\n", "#include <vector>"),
+ FixMessage("add all missing includes"),})),
AllOf(Diag(MainFile.range("FOO"),
"No header providing \"FOO\" is directly included"),
- withFix(Fix(MainFile.range("insert_foo"),
- "#include \"foo.h\"\n", "#include \"foo.h\""))),
+ withFix({Fix(MainFile.range("insert_foo"),
+ "#include \"foo.h\"\n", "#include \"foo.h\""),
+ FixMessage("add all missing includes")})),
AllOf(Diag(MainFile.range("DEF"),
"No header providing \"Foo\" is directly included"),
- withFix(Fix(MainFile.range("insert_foo"),
- "#include \"foo.h\"\n", "#include \"foo.h\""))),
+ withFix({Fix(MainFile.range("insert_foo"),
+ "#include \"foo.h\"\n", "#include \"foo.h\""),
+ FixMessage("add all missing includes")})),
AllOf(Diag(MainFile.range("BAR"),
"No header providing \"BAR\" is directly included"),
- withFix(Fix(MainFile.range("insert_foo"),
- "#include \"foo.h\"\n", "#include \"foo.h\""))),
+ withFix({Fix(MainFile.range("insert_foo"),
+ "#include \"foo.h\"\n", "#include \"foo.h\""),
+ FixMessage("add all missing includes")})),
AllOf(Diag(MainFile.range("Foo"),
"No header providing \"Foo\" is directly included"),
- withFix(Fix(MainFile.range("insert_foo"),
- "#include \"foo.h\"\n", "#include \"foo.h\"")))));
+ withFix({Fix(MainFile.range("insert_foo"),
+ "#include \"foo.h\"\n", "#include \"foo.h\""),
+ FixMessage("add all missing includes")}))));
}
TEST(IncludeCleaner, IWYUPragmas) {
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1908,11 +1908,12 @@
auto AST = TU.build();
EXPECT_THAT(
*AST.getDiagnostics(),
- UnorderedElementsAre(AllOf(
- Diag(Test.range("diag"),
- "included header unused.h is not used directly"),
- withTag(DiagnosticTag::Unnecessary), diagSource(Diag::Clangd),
- withFix(Fix(Test.range("fix"), "", "remove #include directive")))));
+ UnorderedElementsAre(
+ AllOf(Diag(Test.range("diag"),
+ "included header unused.h is not used directly"),
+ withTag(DiagnosticTag::Unnecessary), diagSource(Diag::Clangd),
+ withFix(Fix(Test.range("fix"), "", "remove #include directive"),
+ fixMessage("remove all unused includes")))));
auto &Diag = AST.getDiagnostics()->front();
EXPECT_EQ(getDiagnosticDocURI(Diag.Source, Diag.ID, Diag.Name),
std::string("https://clangd.llvm.org/guides/include-cleaner";));
Index: clang-tools-extra/clangd/Protocol.h
===================================================================
--- clang-tools-extra/clangd/Protocol.h
+++ clang-tools-extra/clangd/Protocol.h
@@ -238,6 +238,8 @@
llvm::json::Value toJSON(const ReferenceLocation &);
llvm::raw_ostream &operator<<(llvm::raw_ostream &, const ReferenceLocation &);
+using ChangeAnnotationIdentifier = std::string;
+// A combination of a LSP standard TextEdit and AnnotatedTextEdit.
struct TextEdit {
/// The range of the text document to be manipulated. To insert
/// text into a document create a range where start === end.
@@ -246,14 +248,34 @@
/// The string to be inserted. For delete operations use an
/// empty string.
std::string newText;
+
+ /// The actual annotation identifier.
+ std::optional<ChangeAnnotationIdentifier> annotationId = std::nullopt;
};
inline bool operator==(const TextEdit &L, const TextEdit &R) {
- return std::tie(L.newText, L.range) == std::tie(R.newText, R.range);
+ return std::tie(L.newText, L.range, L.annotationId) ==
+ std::tie(R.newText, R.range, L.annotationId);
}
bool fromJSON(const llvm::json::Value &, TextEdit &, llvm::json::Path);
llvm::json::Value toJSON(const TextEdit &);
llvm::raw_ostream &operator<<(llvm::raw_ostream &, const TextEdit &);
+struct ChangeAnnotation {
+ // A human-readable string describing the actual change. The string
+ // is rendered prominent in the user interface.
+ std::string label;
+
+ // A flag which indicates that user confirmation is needed
+ // before applying the change.
+ std::optional<bool> needsConfirmation;
+
+ // A human-readable string which is rendered less prominent in
+ // the user interface.
+ std::optional<std::string> description;
+};
+bool fromJSON(const llvm::json::Value &, ChangeAnnotation &, llvm::json::Path);
+llvm::json::Value toJSON(const ChangeAnnotation &);
+
struct TextDocumentEdit {
/// The text document to change.
VersionedTextDocumentIdentifier textDocument;
@@ -530,6 +552,9 @@
/// The client supports versioned document changes for WorkspaceEdit.
bool DocumentChanges = false;
+
+ /// The client supports change annotations on text edits,
+ bool ChangeAnnotation = false;
/// Whether the client supports the textDocument/inactiveRegions
/// notification. This is a clangd extension.
@@ -996,6 +1021,10 @@
/// `workspace.workspaceEdit.resourceOperations` then only plain `TextEdit`s
/// using the `changes` property are supported.
std::optional<std::vector<TextDocumentEdit>> documentChanges;
+
+ /// A map of change annotations that can be referenced in
+ /// AnnotatedTextEdit.
+ std::optional<std::map<std::string, ChangeAnnotation>> changeAnnotations;
};
bool fromJSON(const llvm::json::Value &, WorkspaceEdit &, llvm::json::Path);
llvm::json::Value toJSON(const WorkspaceEdit &WE);
Index: clang-tools-extra/clangd/Protocol.cpp
===================================================================
--- clang-tools-extra/clangd/Protocol.cpp
+++ clang-tools-extra/clangd/Protocol.cpp
@@ -187,14 +187,34 @@
bool fromJSON(const llvm::json::Value &Params, TextEdit &R,
llvm::json::Path P) {
llvm::json::ObjectMapper O(Params, P);
- return O && O.map("range", R.range) && O.map("newText", R.newText);
+ return O && O.map("range", R.range) && O.map("newText", R.newText) &&
+ O.map("annotationId", R.annotationId);
}
llvm::json::Value toJSON(const TextEdit &P) {
- return llvm::json::Object{
+ llvm::json::Object Result{
{"range", P.range},
{"newText", P.newText},
};
+ if (P.annotationId)
+ Result["annotationId"] = P.annotationId;
+ return Result;
+}
+
+bool fromJSON(const llvm::json::Value &Params, ChangeAnnotation &R,
+ llvm::json::Path P) {
+ llvm::json::ObjectMapper O(Params, P);
+ return O && O.map("label", R.label) &&
+ O.map("needsConfirmation", R.needsConfirmation) &&
+ O.map("description", R.description);
+}
+llvm::json::Value toJSON(const ChangeAnnotation & CA) {
+ llvm::json::Object Result{{"label", CA.label}};
+ if (CA.needsConfirmation)
+ Result["needsConfirmation"] = *CA.needsConfirmation;
+ if (CA.description)
+ Result["description"] = *CA.description;
+ return Result;
}
bool fromJSON(const llvm::json::Value &Params, TextDocumentEdit &R,
@@ -458,6 +478,10 @@
if (auto *WorkspaceEdit = Workspace->getObject("workspaceEdit")) {
if (auto DocumentChanges = WorkspaceEdit->getBoolean("documentChanges"))
R.DocumentChanges = *DocumentChanges;
+ if (const auto& ChangeAnnotation =
+ WorkspaceEdit->getObject("changeAnnotationSupport")) {
+ R.ChangeAnnotation = true;
+ }
}
}
if (auto *Window = O->getObject("window")) {
@@ -733,7 +757,8 @@
llvm::json::Path P) {
llvm::json::ObjectMapper O(Params, P);
return O && O.map("changes", R.changes) &&
- O.map("documentChanges", R.documentChanges);
+ O.map("documentChanges", R.documentChanges) &&
+ O.map("changeAnnotations", R.changeAnnotations);
}
bool fromJSON(const llvm::json::Value &Params, ExecuteCommandParams &R,
@@ -888,6 +913,12 @@
}
if (WE.documentChanges)
Result["documentChanges"] = *WE.documentChanges;
+ if (WE.changeAnnotations) {
+ llvm::json::Object ChangeAnnotations;
+ for (auto &Annotation : *(WE.changeAnnotations))
+ ChangeAnnotations[Annotation.first] = Annotation.second;
+ Result["changeAnnotations"] = std::move(ChangeAnnotations);
+ }
return Result;
}
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -44,6 +44,7 @@
#include "llvm/ADT/STLFunctionalExtras.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"
@@ -420,6 +421,108 @@
return {std::move(UnusedIncludes), std::move(MissingIncludes)};
}
+Fix removeAllUnusedIncludes(llvm::ArrayRef<Diag> 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());
+ }
+
+ static const ChangeAnnotationIdentifier RemoveAllUnusedID =
+ "RemoveAllUnusedIncludes";
+ for (auto &E : RemoveAll.Edits) {
+ E.annotationId.emplace();
+ *E.annotationId = RemoveAllUnusedID;
+ }
+ RemoveAll.Annotations.push_back({RemoveAllUnusedID,
+ {/*label=*/"", /*needsConfirmation=*/true,
+ /*description=*/std::nullopt}});
+ return RemoveAll;
+}
+Fix addAllMissingIncludes(llvm::ArrayRef<Diag> 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<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);
+ }
+ }
+
+ static const ChangeAnnotationIdentifier AddAllMissingID =
+ "AddAllMissingIncludes";
+ for (auto &E : AddAllMissing.Edits) {
+ E.annotationId.emplace();
+ *E.annotationId = AddAllMissingID;
+ }
+ // FIXME(hokein): emit used symbol reference in the annotation.
+ AddAllMissing.Annotations.push_back(
+ {AddAllMissingID,
+ {/*label=*/"", /*needsConfirmation=*/true,
+ /*description=*/std::nullopt}});
+ return AddAllMissing;
+}
+Fix fixAll(const Fix& RemoveAllUnused, const Fix& AddAllMissing) {
+ Fix FixAll;
+ FixAll.Message = "Fix all includes";
+ static const ChangeAnnotationIdentifier FixAllID = "FixAll";
+ for (const auto& F : RemoveAllUnused.Edits)
+ FixAll.Edits.push_back({F.range, F.newText, FixAllID});
+ for (const auto& F : AddAllMissing.Edits)
+ FixAll.Edits.push_back({F.range, F.newText, FixAllID});
+ FixAll.Annotations.push_back({FixAllID, {
+ /*label=*/"", /*needsConfirmation=*/true, /*description=*/std::nullopt
+ }});
+
+ return FixAll;
+}
+
+std::vector<Diag> generateIncludeCleanerDiagnostic(
+ ParsedAST &AST, const IncludeCleanerFindings &Findings,
+ llvm::StringRef Code) {
+ std::optional<Fix> RemoveAllUnused, AddAllMissing, FixAll;
+ std::vector<Diag> UnusedIncludes = generateUnusedIncludeDiagnostics(
+ AST.tuPath(), Findings.UnusedIncludes, Code);
+ if (!UnusedIncludes.empty())
+ RemoveAllUnused = removeAllUnusedIncludes(UnusedIncludes);
+
+ std::vector<Diag> MissingIncludeDiags = generateMissingIncludeDiagnostics(
+ AST, Findings.MissingIncludes, Code);
+ if (!MissingIncludeDiags.empty())
+ AddAllMissing = addAllMissingIncludes(MissingIncludeDiags);
+
+ if (RemoveAllUnused && AddAllMissing)
+ FixAll = fixAll(*RemoveAllUnused, *AddAllMissing);
+
+ auto AddBatchFix = [](const std::optional<Fix> &F, clang::clangd::Diag *Out) {
+ if (!F) return;
+ Out->Fixes.push_back(*F);
+ };
+ for (auto &Diag : MissingIncludeDiags) {
+ AddBatchFix(AddAllMissing, &Diag);
+ AddBatchFix(FixAll, &Diag);
+ }
+ for (auto &Diag : UnusedIncludes) {
+ AddBatchFix(RemoveAllUnused, &Diag);
+ AddBatchFix(FixAll, &Diag);
+ }
+
+ auto Result = std::move(MissingIncludeDiags);
+ llvm::move(UnusedIncludes,
+ std::back_inserter(Result));
+ return Result;
+}
+
std::vector<Diag> issueIncludeCleanerDiagnostics(ParsedAST &AST,
llvm::StringRef Code) {
// Interaction is only polished for C/CPP.
@@ -435,13 +538,7 @@
// will need include-cleaner results, call it once
Findings = computeIncludeCleanerFindings(AST);
}
-
- std::vector<Diag> Result = generateUnusedIncludeDiagnostics(
- AST.tuPath(), Findings.UnusedIncludes, Code);
- llvm::move(
- generateMissingIncludeDiagnostics(AST, Findings.MissingIncludes, Code),
- std::back_inserter(Result));
- return Result;
+ return generateIncludeCleanerDiagnostic(AST, Findings, Code);
}
std::optional<include_cleaner::Header>
Index: clang-tools-extra/clangd/Diagnostics.h
===================================================================
--- clang-tools-extra/clangd/Diagnostics.h
+++ clang-tools-extra/clangd/Diagnostics.h
@@ -83,6 +83,10 @@
std::string Message;
/// TextEdits from clang's fix-its. Must be non-empty.
llvm::SmallVector<TextEdit, 1> Edits;
+
+ /// Annotations for the Edits.
+ llvm::SmallVector<std::pair<ChangeAnnotationIdentifier, ChangeAnnotation>>
+ Annotations;
};
llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Fix &F);
Index: clang-tools-extra/clangd/ClangdLSPServer.h
===================================================================
--- clang-tools-extra/clangd/ClangdLSPServer.h
+++ clang-tools-extra/clangd/ClangdLSPServer.h
@@ -277,6 +277,9 @@
bool SupportsOffsetsInSignatureHelp = false;
/// Whether the client supports the versioned document changes.
bool SupportsDocumentChanges = false;
+ /// Whether the client supports change annotations on text edits.
+ bool SupportsChangeAnnotation = false;
+
std::mutex BackgroundIndexProgressMutex;
enum class BackgroundIndexProgress {
// Client doesn't support reporting progress. No transitions possible.
Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -98,19 +98,30 @@
/// Convert from Fix to LSP CodeAction.
CodeAction toCodeAction(const Fix &F, const URIForFile &File,
const std::optional<int64_t> &Version,
- bool SupportsDocumentChanges) {
+ bool SupportsDocumentChanges,
+ bool SupportChangeAnnotation) {
CodeAction Action;
Action.title = F.Message;
Action.kind = std::string(CodeAction::QUICKFIX_KIND);
Action.edit.emplace();
if (!SupportsDocumentChanges) {
Action.edit->changes.emplace();
- (*Action.edit->changes)[File.uri()] = {F.Edits.begin(), F.Edits.end()};
+ auto &Changes = (*Action.edit->changes)[File.uri()];
+ for (const auto &E : F.Edits)
+ Changes.push_back({E.range, E.newText, /*annotationId=*/std::nullopt});
} else {
Action.edit->documentChanges.emplace();
- TextDocumentEdit& Edit = Action.edit->documentChanges->emplace_back();
+ TextDocumentEdit &Edit = Action.edit->documentChanges->emplace_back();
Edit.textDocument = VersionedTextDocumentIdentifier{{File}, Version};
- Edit.edits = {F.Edits.begin(), F.Edits.end()};
+ for (const auto &E : F.Edits)
+ Edit.edits.push_back(
+ {E.range, E.newText,
+ SupportChangeAnnotation ? E.annotationId : std::nullopt});
+ if (SupportChangeAnnotation) {
+ Action.edit->changeAnnotations.emplace();
+ for (const auto &[AID, Annotation]: F.Annotations)
+ (*Action.edit->changeAnnotations)[AID] = Annotation;
+ }
}
return Action;
}
@@ -508,6 +519,7 @@
SupportsReferenceContainer = Params.capabilities.ReferenceContainer;
SupportFileStatus = Params.initializationOptions.FileStatus;
SupportsDocumentChanges = Params.capabilities.DocumentChanges;
+ SupportsChangeAnnotation = Params.capabilities.ChangeAnnotation;
HoverContentFormat = Params.capabilities.HoverContentFormat;
Opts.LineFoldingOnly = Params.capabilities.LineFoldingOnly;
SupportsOffsetsInSignatureHelp = Params.capabilities.OffsetsInSignatureHelp;
@@ -1741,7 +1753,8 @@
for (const auto &Fix : Fixes)
CodeActions.push_back(toCodeAction(Fix, Notification.uri,
Notification.version,
- SupportsDocumentChanges));
+ SupportsDocumentChanges,
+ SupportsChangeAnnotation));
if (DiagOpts.EmbedFixesInDiagnostics && !CodeActions.empty()) {
Diag.codeActions.emplace(CodeActions);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
