[clang-tools-extra] [clangd] Attempt to fix https://github.com/clangd/clangd/issues/1536 (PR #79448)
Tor Shepherd via cfe-commits
cfe-commits at lists.llvm.org
Thu Jan 25 06:17:52 PST 2024
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.shepherd at 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.shepherd at 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: }
More information about the cfe-commits
mailing list