https://github.com/torshepherd created https://github.com/llvm/llvm-project/pull/79448
This PR attempts to fix [1536.](https://github.com/clangd/clangd/issues/1536). See in the unit tests, when all quick fixes are of the same `code`, `isPreferred` will be true. However, this doesn't seem to change the behavior in vscode:  >From 6bb871f64073132683bab2318c228a4ef7723c8e Mon Sep 17 00:00:00 2001 From: Tor Shepherd <tor.aksel.sheph...@gmail.com> Date: Wed, 24 Jan 2024 23:43:52 -0500 Subject: [PATCH 1/2] second try --- clang-tools-extra/clangd/ClangdLSPServer.cpp | 78 ++++++++++---------- 1 file changed, 41 insertions(+), 37 deletions(-) diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp index a87da252b7a7e9b..5fbd09fdcfdf420 100644 --- a/clang-tools-extra/clangd/ClangdLSPServer.cpp +++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp @@ -27,7 +27,9 @@ #include "clang/Tooling/Core/Replacement.h" #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/FunctionExtras.h" +#include "llvm/ADT/STLExtras.h" #include "llvm/ADT/ScopeExit.h" +#include "llvm/ADT/StringMap.h" #include "llvm/ADT/StringRef.h" #include "llvm/ADT/Twine.h" #include "llvm/Support/Allocator.h" @@ -117,10 +119,9 @@ CodeAction toCodeAction(const Fix &F, const URIForFile &File, Edit.textDocument = VersionedTextDocumentIdentifier{{File}, Version}; for (const auto &E : F.Edits) Edit.edits.push_back( - {E.range, E.newText, - SupportChangeAnnotation ? E.annotationId : ""}); + {E.range, E.newText, SupportChangeAnnotation ? E.annotationId : ""}); if (SupportChangeAnnotation) { - for (const auto &[AID, Annotation]: F.Annotations) + for (const auto &[AID, Annotation] : F.Annotations) Action.edit->changeAnnotations[AID] = Annotation; } } @@ -861,24 +862,24 @@ void ClangdLSPServer::onRename(const RenameParams &Params, if (!Server->getDraft(File)) return Reply(llvm::make_error<LSPError>( "onRename called for non-added file", ErrorCode::InvalidParams)); - Server->rename(File, Params.position, Params.newName, Opts.Rename, - [File, Params, Reply = std::move(Reply), - this](llvm::Expected<RenameResult> R) mutable { - if (!R) - return Reply(R.takeError()); - if (auto Err = validateEdits(*Server, R->GlobalChanges)) - return Reply(std::move(Err)); - WorkspaceEdit Result; - // FIXME: use documentChanges if SupportDocumentChanges is - // true. - Result.changes.emplace(); - for (const auto &Rep : R->GlobalChanges) { - (*Result - .changes)[URI::createFile(Rep.first()).toString()] = - Rep.second.asTextEdits(); - } - Reply(Result); - }); + Server->rename( + File, Params.position, Params.newName, Opts.Rename, + [File, Params, Reply = std::move(Reply), + this](llvm::Expected<RenameResult> R) mutable { + if (!R) + return Reply(R.takeError()); + if (auto Err = validateEdits(*Server, R->GlobalChanges)) + return Reply(std::move(Err)); + WorkspaceEdit Result; + // FIXME: use documentChanges if SupportDocumentChanges is + // true. + Result.changes.emplace(); + for (const auto &Rep : R->GlobalChanges) { + (*Result.changes)[URI::createFile(Rep.first()).toString()] = + Rep.second.asTextEdits(); + } + Reply(Result); + }); } void ClangdLSPServer::onDocumentDidClose( @@ -1014,7 +1015,7 @@ void ClangdLSPServer::onCodeAction(const CodeActionParams &Params, std::map<ClangdServer::DiagRef, clangd::Diagnostic> ToLSPDiags; ClangdServer::CodeActionInputs Inputs; - for (const auto& LSPDiag : Params.context.diagnostics) { + for (const auto &LSPDiag : Params.context.diagnostics) { if (auto DiagRef = getDiagRef(File.file(), LSPDiag)) { ToLSPDiags[*DiagRef] = LSPDiag; Inputs.Diagnostics.push_back(*DiagRef); @@ -1023,13 +1024,9 @@ void ClangdLSPServer::onCodeAction(const CodeActionParams &Params, Inputs.File = File.file(); Inputs.Selection = Params.range; Inputs.RequestedActionKinds = Params.context.only; - Inputs.TweakFilter = [this](const Tweak &T) { - return Opts.TweakFilter(T); - }; - auto CB = [this, - Reply = std::move(Reply), - ToLSPDiags = std::move(ToLSPDiags), File, - Selection = Params.range]( + Inputs.TweakFilter = [this](const Tweak &T) { return Opts.TweakFilter(T); }; + auto CB = [this, Reply = std::move(Reply), ToLSPDiags = std::move(ToLSPDiags), + File, Selection = Params.range]( llvm::Expected<ClangdServer::CodeActionResult> Fixits) mutable { if (!Fixits) return Reply(Fixits.takeError()); @@ -1038,27 +1035,34 @@ void ClangdLSPServer::onCodeAction(const CodeActionParams &Params, for (const auto &QF : Fixits->QuickFixes) { CAs.push_back(toCodeAction(QF.F, File, Version, SupportsDocumentChanges, SupportsChangeAnnotation)); - if (auto It = ToLSPDiags.find(QF.Diag); - It != ToLSPDiags.end()) { + if (auto It = ToLSPDiags.find(QF.Diag); It != ToLSPDiags.end()) { CAs.back().diagnostics = {It->second}; } } for (const auto &TR : Fixits->TweakRefs) CAs.push_back(toCodeAction(TR, File, Selection)); - // If there's exactly one quick-fix, call it "preferred". + // If there's exactly one quick-fix category, call it "preferred". // We never consider refactorings etc as preferred. - CodeAction *OnlyFix = nullptr; + llvm::SmallVector<CodeAction *, 1> OnlyFixes; + std::optional<std::string> OnlyFixCategory; for (auto &Action : CAs) { + std::string Code = + Action.diagnostics.has_value() && !Action.diagnostics->empty() + ? Action.diagnostics->front().code + : ""; if (Action.kind && *Action.kind == CodeAction::QUICKFIX_KIND) { - if (OnlyFix) { - OnlyFix = nullptr; + if (OnlyFixCategory && *OnlyFixCategory != Code) { + OnlyFixCategory = std::nullopt; break; } - OnlyFix = &Action; + if (!OnlyFixCategory) { + OnlyFixCategory = Code; + } + OnlyFixes.emplace_back(&Action); } } - if (OnlyFix) { + for (const auto &OnlyFix : OnlyFixes) { OnlyFix->isPreferred = true; if (ToLSPDiags.size() == 1 && ToLSPDiags.begin()->second.range == Selection) >From 70e33931b2a15e515538f64e48ef0c57490b1245 Mon Sep 17 00:00:00 2001 From: Tor Shepherd <tor.aksel.sheph...@gmail.com> Date: Thu, 25 Jan 2024 09:17:32 -0500 Subject: [PATCH 2/2] fix tests --- .../clangd/test/fixits-codeaction-documentchanges.test | 2 ++ clang-tools-extra/clangd/test/fixits-codeaction.test | 2 ++ 2 files changed, 4 insertions(+) diff --git a/clang-tools-extra/clangd/test/fixits-codeaction-documentchanges.test b/clang-tools-extra/clangd/test/fixits-codeaction-documentchanges.test index 7a591319a740547..d796453679a4cbd 100644 --- a/clang-tools-extra/clangd/test/fixits-codeaction-documentchanges.test +++ b/clang-tools-extra/clangd/test/fixits-codeaction-documentchanges.test @@ -87,6 +87,7 @@ # CHECK-NEXT: } # CHECK-NEXT: ] # CHECK-NEXT: }, +# CHECK-NEXT: "isPreferred": true, # CHECK-NEXT: "kind": "quickfix", # CHECK-NEXT: "title": "place parentheses around the assignment to silence this warning" # CHECK-NEXT: }, @@ -134,6 +135,7 @@ # CHECK-NEXT: } # CHECK-NEXT: ] # CHECK-NEXT: }, +# CHECK-NEXT: "isPreferred": true, # CHECK-NEXT: "kind": "quickfix", # CHECK-NEXT: "title": "use '==' to turn this assignment into an equality comparison" # CHECK-NEXT: } diff --git a/clang-tools-extra/clangd/test/fixits-codeaction.test b/clang-tools-extra/clangd/test/fixits-codeaction.test index 75d0fb012ffc814..09e2e22aefe57f1 100644 --- a/clang-tools-extra/clangd/test/fixits-codeaction.test +++ b/clang-tools-extra/clangd/test/fixits-codeaction.test @@ -81,6 +81,7 @@ # CHECK-NEXT: ] # CHECK-NEXT: } # CHECK-NEXT: }, +# CHECK-NEXT: "isPreferred": true, # CHECK-NEXT: "kind": "quickfix", # CHECK-NEXT: "title": "place parentheses around the assignment to silence this warning" # CHECK-NEXT: }, @@ -122,6 +123,7 @@ # CHECK-NEXT: ] # CHECK-NEXT: } # CHECK-NEXT: }, +# CHECK-NEXT: "isPreferred": true, # CHECK-NEXT: "kind": "quickfix", # CHECK-NEXT: "title": "use '==' to turn this assignment into an equality comparison" # CHECK-NEXT: } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
