diff --git a/clang-tools-extra/clangd/ClangdLSPServer.h b/clang-tools-extra/clangd/ClangdLSPServer.h --- a/clang-tools-extra/clangd/ClangdLSPServer.h +++ b/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. diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp --- a/clang-tools-extra/clangd/ClangdLSPServer.cpp +++ b/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 &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); diff --git a/clang-tools-extra/clangd/Diagnostics.h b/clang-tools-extra/clangd/Diagnostics.h --- a/clang-tools-extra/clangd/Diagnostics.h +++ b/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 Edits; + + /// Annotations for the Edits. + llvm::SmallVector> + Annotations; }; llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Fix &F); 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 @@ -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 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 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); + } + } + + 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 generateIncludeCleanerDiagnostic( + ParsedAST &AST, const IncludeCleanerFindings &Findings, + llvm::StringRef Code) { + std::optional RemoveAllUnused, AddAllMissing, FixAll; + std::vector UnusedIncludes = generateUnusedIncludeDiagnostics( + AST.tuPath(), Findings.UnusedIncludes, Code); + if (!UnusedIncludes.empty()) + RemoveAllUnused = removeAllUnusedIncludes(UnusedIncludes); + + std::vector MissingIncludeDiags = generateMissingIncludeDiagnostics( + AST, Findings.MissingIncludes, Code); + if (!MissingIncludeDiags.empty()) + AddAllMissing = addAllMissingIncludes(MissingIncludeDiags); + + if (RemoveAllUnused && AddAllMissing) + FixAll = fixAll(*RemoveAllUnused, *AddAllMissing); + + auto AddBatchFix = [](const std::optional &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 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 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 diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h --- a/clang-tools-extra/clangd/Protocol.h +++ b/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,43 @@ /// The string to be inserted. For delete operations use an /// empty string. std::string newText; + + /// The actual annotation identifier. + std::optional 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 &); +// A special text edit with an additional change annotation. +struct AnnotatedTextEdit : TextEdit { + // std::string + std::string annotationId; +}; +bool fromJSON(const llvm::json::Value &, AnnotatedTextEdit &, llvm::json::Path); +llvm::json::Value toJSON(const AnnotatedTextEdit &); + + +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 needsConfirmation; + + // A human-readable string which is rendered less prominent in + // the user interface. + std::optional 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 +561,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 +1030,10 @@ /// `workspace.workspaceEdit.resourceOperations` then only plain `TextEdit`s /// using the `changes` property are supported. std::optional> documentChanges; + + /// A map of change annotations that can be referenced in + /// AnnotatedTextEdit. + std::optional> changeAnnotations; }; bool fromJSON(const llvm::json::Value &, WorkspaceEdit &, llvm::json::Path); llvm::json::Value toJSON(const WorkspaceEdit &WE); diff --git a/clang-tools-extra/clangd/Protocol.cpp b/clang-tools-extra/clangd/Protocol.cpp --- a/clang-tools-extra/clangd/Protocol.cpp +++ b/clang-tools-extra/clangd/Protocol.cpp @@ -187,16 +187,50 @@ 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) { + 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, AnnotatedTextEdit &R, + llvm::json::Path P) { + llvm::json::ObjectMapper O(Params, P); + return O && O.map("range", R.range) && O.map("newText", R.newText) && + O.map("annotationId", R.annotationId); +} +llvm::json::Value toJSON(const AnnotatedTextEdit &P) { return llvm::json::Object{ {"range", P.range}, {"newText", P.newText}, + {"annotationId", P.annotationId}, }; } +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, llvm::json::Path P) { llvm::json::ObjectMapper O(Params, P); @@ -458,6 +492,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 +771,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 +927,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; } diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp --- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp +++ b/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")); diff --git a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp --- a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp +++ b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp @@ -45,8 +45,8 @@ using ::testing::Pointee; using ::testing::UnorderedElementsAre; -Matcher withFix(::testing::Matcher FixMatcher) { - return Field(&Diag::Fixes, ElementsAre(FixMatcher)); +Matcher withFix(std::vector<::testing::Matcher> 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 \n", - "#include "))), + withFix({Fix(MainFile.range("insert_f"), "#include \n", + "#include "), + 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 \n", "#include "))), + withFix({Fix(MainFile.range("insert_vector"), + "#include \n", "#include "), + 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) {