[clangd] Attempt to fix https://github.com/clangd/clangd/issues/1536 #79448
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clangd Author: Tor Shepherd (torshepherd) ChangesThis PR attempts to fix 1536.. See in the unit tests, when all quick fixes are of the same
Full diff: https://github.com/llvm/llvm-project/pull/79448.diff 3 Files Affected:
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index a87da252b7a7e9..5fbd09fdcfdf42 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)
diff --git a/clang-tools-extra/clangd/test/fixits-codeaction-documentchanges.test b/clang-tools-extra/clangd/test/fixits-codeaction-documentchanges.test
index 7a591319a74054..d796453679a4cb 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 75d0fb012ffc81..09e2e22aefe57f 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: }
|
I applied the patch locally, and it does have an effect on the steps from #1536 (running
Unfortunately, it's not the effect we were hoping for (automatically applying the multiple fixes at once). It's possible that in my analysis in #830 / #1536 I misunderstood the purpose of VSCode's "Auto fix" command. This will need more investigation on the VSCode side. |
|
I read up on this a bit; microsoft/vscode#62110 seems particularly relevant. Based on this comment:
it seems that I did misunderstand the purpose of the "Auto fix" command in vscode; it looks like it's only intended to fix a single error. The proposal in that thread (which has since been implemented in vscode) does include a way to apply all quick-fixes in a file: it's the Conclusion: there's probably not much point proceeding with this patch (the slight behaviour change described in the previous comment doesn't seem very helpful), but instead we should fix clangd/clangd#1446 and then the desired scenario should work using the "Fix all" command (and as an added bonus, there is no need to select the entire file contents, "Fix all" automatically applies to the whole file). |
|
I'll close this patch and pursue the other things we discussed |
This PR attempts to fix 1536.. See in the unit tests, when all quick fixes are of the same
code,isPreferredwill be true. However, this doesn't seem to change the behavior in vscode: