[clang-tools-extra] [clangd] forward clang-tidy's readability-identifier-naming fix to textDocument/rename (PR #78454)
Tom Praschan via cfe-commits
cfe-commits at lists.llvm.org
Wed Jan 31 10:58:37 PST 2024
https://github.com/tom-anders updated https://github.com/llvm/llvm-project/pull/78454
>From 579d681323db5f92d494f0cd0aaa9158dc8c4e3b Mon Sep 17 00:00:00 2001
From: tom-anders <13141438+tom-anders at users.noreply.github.com>
Date: Wed, 17 Jan 2024 15:36:02 +0100
Subject: [PATCH] [clangd] forward clang-tidy's readability-identifier-naming
fix to textDocument/rename
---
clang-tools-extra/clangd/ClangdLSPServer.cpp | 32 ++++++++++
clang-tools-extra/clangd/ClangdLSPServer.h | 1 +
clang-tools-extra/clangd/ClangdServer.cpp | 43 +++++++++++---
clang-tools-extra/clangd/ClangdServer.h | 5 ++
clang-tools-extra/clangd/Protocol.cpp | 8 +++
clang-tools-extra/clangd/Protocol.h | 1 +
.../clangd/test/initialize-params.test | 1 +
.../clangd/unittests/ClangdLSPServerTests.cpp | 59 +++++++++++++++++++
.../clangd/unittests/LSPClient.cpp | 31 +++++++++-
.../clangd/unittests/LSPClient.h | 6 ++
10 files changed, 178 insertions(+), 9 deletions(-)
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index a87da252b7a7e..49d1eb0e8341c 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -73,6 +73,23 @@ std::optional<int64_t> decodeVersion(llvm::StringRef Encoded) {
const llvm::StringLiteral ApplyFixCommand = "clangd.applyFix";
const llvm::StringLiteral ApplyTweakCommand = "clangd.applyTweak";
+const llvm::StringLiteral ApplyRenameCommand = "clangd.applyRename";
+
+CodeAction toCodeAction(const ClangdServer::CodeActionResult::Rename &R,
+ const URIForFile &File) {
+ CodeAction CA;
+ CA.title = R.Diag.Message;
+ CA.kind = std::string(CodeAction::REFACTOR_KIND);
+ CA.command.emplace();
+ CA.command->title = R.Diag.Message;
+ CA.command->command = std::string(ApplyRenameCommand);
+ RenameParams Params;
+ Params.textDocument = TextDocumentIdentifier{File};
+ Params.position = R.Diag.Range.start;
+ Params.newName = R.NewName;
+ CA.command->argument = Params;
+ return CA;
+}
/// Transforms a tweak into a code action that would apply it if executed.
/// EXPECTS: T.prepare() was called and returned true.
@@ -808,6 +825,16 @@ void ClangdLSPServer::onCommandApplyTweak(const TweakArgs &Args,
std::move(Action));
}
+void ClangdLSPServer::onCommandApplyRename(const RenameParams &R,
+ Callback<llvm::json::Value> Reply) {
+ onRename(R, [this, Reply = std::move(Reply)](
+ llvm::Expected<WorkspaceEdit> Edit) mutable {
+ if (!Edit)
+ Reply(Edit.takeError());
+ applyEdit(std::move(*Edit), "Rename applied.", std::move(Reply));
+ });
+}
+
void ClangdLSPServer::applyEdit(WorkspaceEdit WE, llvm::json::Value Success,
Callback<llvm::json::Value> Reply) {
ApplyWorkspaceEditParams Edit;
@@ -1043,6 +1070,10 @@ void ClangdLSPServer::onCodeAction(const CodeActionParams &Params,
CAs.back().diagnostics = {It->second};
}
}
+
+ for (const auto &R : Fixits->Renames)
+ CAs.push_back(toCodeAction(R, File));
+
for (const auto &TR : Fixits->TweakRefs)
CAs.push_back(toCodeAction(TR, File, Selection));
@@ -1664,6 +1695,7 @@ void ClangdLSPServer::bindMethods(LSPBinder &Bind,
Bind.method("textDocument/foldingRange", this, &ClangdLSPServer::onFoldingRange);
Bind.command(ApplyFixCommand, this, &ClangdLSPServer::onCommandApplyEdit);
Bind.command(ApplyTweakCommand, this, &ClangdLSPServer::onCommandApplyTweak);
+ Bind.command(ApplyRenameCommand, this, &ClangdLSPServer::onCommandApplyRename);
ApplyWorkspaceEdit = Bind.outgoingMethod("workspace/applyEdit");
PublishDiagnostics = Bind.outgoingNotification("textDocument/publishDiagnostics");
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.h b/clang-tools-extra/clangd/ClangdLSPServer.h
index 79579c22b788a..69a349c6a5e08 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.h
+++ b/clang-tools-extra/clangd/ClangdLSPServer.h
@@ -174,6 +174,7 @@ class ClangdLSPServer : private ClangdServer::Callbacks,
/// Implement commands.
void onCommandApplyEdit(const WorkspaceEdit &, Callback<llvm::json::Value>);
void onCommandApplyTweak(const TweakArgs &, Callback<llvm::json::Value>);
+ void onCommandApplyRename(const RenameParams &, Callback<llvm::json::Value>);
/// Outgoing LSP calls.
LSPBinder::OutgoingMethod<ApplyWorkspaceEditParams,
diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
index 6fb2641e8793d..8ea7709561bcd 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -625,9 +625,10 @@ void ClangdServer::rename(PathRef File, Position Pos, llvm::StringRef NewName,
WorkScheduler->runWithAST("Rename", File, std::move(Action));
}
+namespace {
// May generate several candidate selections, due to SelectionTree ambiguity.
// vector of pointers because GCC doesn't like non-copyable Selection.
-static llvm::Expected<std::vector<std::unique_ptr<Tweak::Selection>>>
+llvm::Expected<std::vector<std::unique_ptr<Tweak::Selection>>>
tweakSelection(const Range &Sel, const InputsAndAST &AST,
llvm::vfs::FileSystem *FS) {
auto Begin = positionToOffset(AST.Inputs.Contents, Sel.start);
@@ -648,6 +649,26 @@ tweakSelection(const Range &Sel, const InputsAndAST &AST,
return std::move(Result);
}
+// Some fixes may perform local renaming, we want to convert those to clangd
+// rename commands, such that we can leverage the index for more accurate
+// results.
+std::optional<ClangdServer::CodeActionResult::Rename>
+TryConvertToRename(const Diag *Diag, const ClangdServer::DiagRef &DR, const Fix &Fix) {
+ bool IsClangTidyRename = Diag->Source == Diag::ClangTidy &&
+ Diag->Name == "readability-identifier-naming" &&
+ !Fix.Edits.empty();
+ if (IsClangTidyRename) {
+ ClangdServer::CodeActionResult::Rename R;
+ R.NewName = Fix.Edits.front().newText;
+ R.Diag = DR;
+ return R;
+ }
+
+ return std::nullopt;
+}
+
+} // namespace
+
void ClangdServer::codeAction(const CodeActionInputs &Params,
Callback<CodeActionResult> CB) {
auto Action = [Params, CB = std::move(CB),
@@ -668,16 +689,22 @@ void ClangdServer::codeAction(const CodeActionInputs &Params,
CodeActionResult Result;
Result.Version = InpAST->AST.version().str();
if (KindAllowed(CodeAction::QUICKFIX_KIND)) {
- auto FindMatchedFixes =
- [&InpAST](const DiagRef &DR) -> llvm::ArrayRef<Fix> {
+ auto FindMatchedDiag = [&InpAST](const DiagRef &DR) -> const Diag * {
for (const auto &Diag : InpAST->AST.getDiagnostics())
if (Diag.Range == DR.Range && Diag.Message == DR.Message)
- return Diag.Fixes;
- return {};
+ return &Diag;
+ return nullptr;
};
- for (const auto &Diag : Params.Diagnostics)
- for (const auto &Fix : FindMatchedFixes(Diag))
- Result.QuickFixes.push_back({Diag, Fix});
+ for (const auto &DiagRef : Params.Diagnostics) {
+ if (const auto *Diag = FindMatchedDiag(DiagRef))
+ for (const auto &Fix : Diag->Fixes) {
+ if (auto Rename = TryConvertToRename(Diag, DiagRef, Fix)) {
+ Result.Renames.emplace_back(std::move(*Rename));
+ } else {
+ Result.QuickFixes.push_back({DiagRef, Fix});
+ }
+ }
+ }
}
// Collect Tweaks
diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h
index a416602251428..46245205e5239 100644
--- a/clang-tools-extra/clangd/ClangdServer.h
+++ b/clang-tools-extra/clangd/ClangdServer.h
@@ -380,6 +380,11 @@ class ClangdServer {
};
std::vector<QuickFix> QuickFixes;
std::vector<TweakRef> TweakRefs;
+ struct Rename {
+ DiagRef Diag;
+ std::string NewName;
+ };
+ std::vector<Rename> Renames;
};
/// Surface code actions (quick-fixes for diagnostics, or available code
/// tweaks) for a given range in a file.
diff --git a/clang-tools-extra/clangd/Protocol.cpp b/clang-tools-extra/clangd/Protocol.cpp
index a6370649f5ad1..7cc248cd1da55 100644
--- a/clang-tools-extra/clangd/Protocol.cpp
+++ b/clang-tools-extra/clangd/Protocol.cpp
@@ -1187,6 +1187,14 @@ bool fromJSON(const llvm::json::Value &Params, RenameParams &R,
O.map("position", R.position) && O.map("newName", R.newName);
}
+llvm::json::Value toJSON(const RenameParams &R) {
+ return llvm::json::Object{
+ {"textDocument", R.textDocument},
+ {"position", R.position},
+ {"newName", R.newName},
+ };
+}
+
llvm::json::Value toJSON(const DocumentHighlight &DH) {
return llvm::json::Object{
{"range", toJSON(DH.range)},
diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h
index e88c804692568..d2812ae6b78ac 100644
--- a/clang-tools-extra/clangd/Protocol.h
+++ b/clang-tools-extra/clangd/Protocol.h
@@ -1435,6 +1435,7 @@ struct RenameParams {
std::string newName;
};
bool fromJSON(const llvm::json::Value &, RenameParams &, llvm::json::Path);
+llvm::json::Value toJSON(const RenameParams &);
enum class DocumentHighlightKind { Text = 1, Read = 2, Write = 3 };
diff --git a/clang-tools-extra/clangd/test/initialize-params.test b/clang-tools-extra/clangd/test/initialize-params.test
index a1fdae9870ab6..7c96eb9835b71 100644
--- a/clang-tools-extra/clangd/test/initialize-params.test
+++ b/clang-tools-extra/clangd/test/initialize-params.test
@@ -40,6 +40,7 @@
# CHECK-NEXT: "executeCommandProvider": {
# CHECK-NEXT: "commands": [
# CHECK-NEXT: "clangd.applyFix",
+# CHECK-NEXT: "clangd.applyRename"
# CHECK-NEXT: "clangd.applyTweak"
# CHECK-NEXT: ]
# CHECK-NEXT: },
diff --git a/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp b/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
index 1f58e2c3cb547..36468ea05ee45 100644
--- a/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
@@ -195,6 +195,65 @@ TEST_F(LSPTest, RecordsLatencies) {
EXPECT_THAT(Tracer.takeMetric("lsp_latency", MethodName), testing::SizeIs(1));
}
+// clang-tidy's renames are converted to clangd's internal rename functionality,
+// see clangd#1589 and clangd#741
+TEST_F(LSPTest, ClangTidyRename) {
+ Annotations Header(R"cpp(
+ void [[foo]]();
+ )cpp");
+ Annotations Source(R"cpp(
+ void [[foo]]() {}
+ )cpp");
+ Opts.ClangTidyProvider = [](tidy::ClangTidyOptions &ClangTidyOpts,
+ llvm::StringRef) {
+ ClangTidyOpts.Checks = {"-*,readability-identifier-naming"};
+ ClangTidyOpts.CheckOptions["readability-identifier-naming.FunctionCase"] =
+ "CamelCase";
+ };
+ auto &Client = start();
+ Client.didOpen("foo.hpp", Header.code());
+ Client.didOpen("foo.cpp", Source.code());
+
+ auto RenameDiag = Client.diagnostics("foo.cpp").value().at(0);
+
+ auto RenameCommand =
+ (*Client
+ .call("textDocument/codeAction",
+ llvm::json::Object{
+ {"textDocument", Client.documentID("foo.cpp")},
+ {"context",
+ llvm::json::Object{
+ {"diagnostics", llvm::json::Array{RenameDiag}}}},
+ {"range", Source.range()}})
+ .takeValue()
+ .getAsArray())[0];
+
+ Client.expectServerCall("workspace/applyEdit");
+ Client.call("workspace/executeCommand", RenameCommand);
+ Client.sync();
+
+ auto Params = Client.takeCallParams("workspace/applyEdit");
+ auto Uri = [&](llvm::StringRef Path) {
+ return Client.uri(Path).getAsString().value().str();
+ };
+ llvm::json::Object ExpectedEdit = llvm::json::Object{
+ {"edit", llvm::json::Object{
+ {"changes",
+ llvm::json::Object{
+ {Uri("foo.hpp"), llvm::json::Array{llvm::json::Object{
+ {"range", Header.range()},
+ {"newText", "Foo"},
+ }}},
+
+ {Uri("foo.cpp"), llvm::json::Array{llvm::json::Object{
+ {"range", Source.range()},
+ {"newText", "Foo"},
+ }}}
+
+ }}}}};
+ EXPECT_EQ(Params, std::vector{llvm::json::Value(std::move(ExpectedEdit))});
+}
+
TEST_F(LSPTest, IncomingCalls) {
Annotations Code(R"cpp(
void calle^e(int);
diff --git a/clang-tools-extra/clangd/unittests/LSPClient.cpp b/clang-tools-extra/clangd/unittests/LSPClient.cpp
index 4d8ba137e8c55..b930523407427 100644
--- a/clang-tools-extra/clangd/unittests/LSPClient.cpp
+++ b/clang-tools-extra/clangd/unittests/LSPClient.cpp
@@ -105,6 +105,20 @@ class LSPClient::TransportImpl : public Transport {
return Result;
}
+ void expectCall(llvm::StringRef Method) {
+ std::lock_guard<std::mutex> Lock(Mu);
+ Calls[Method] = {};
+ }
+
+ std::vector<llvm::json::Value> takeCallParams(llvm::StringRef Method) {
+ std::vector<llvm::json::Value> Result;
+ {
+ std::lock_guard<std::mutex> Lock(Mu);
+ std::swap(Result, Calls[Method]);
+ }
+ return Result;
+ }
+
private:
void reply(llvm::json::Value ID,
llvm::Expected<llvm::json::Value> V) override {
@@ -130,7 +144,12 @@ class LSPClient::TransportImpl : public Transport {
void call(llvm::StringRef Method, llvm::json::Value Params,
llvm::json::Value ID) override {
logBody(Method, Params, /*Send=*/false);
- ADD_FAILURE() << "Unexpected server->client call " << Method;
+ std::lock_guard<std::mutex> Lock(Mu);
+ if (Calls.contains(Method)) {
+ Calls[Method].push_back(std::move(Params));
+ } else {
+ ADD_FAILURE() << "Unexpected server->client call " << Method;
+ }
}
llvm::Error loop(MessageHandler &H) override {
@@ -152,6 +171,7 @@ class LSPClient::TransportImpl : public Transport {
std::queue<std::function<void(Transport::MessageHandler &)>> Actions;
std::condition_variable CV;
llvm::StringMap<std::vector<llvm::json::Value>> Notifications;
+ llvm::StringMap<std::vector<llvm::json::Value>> Calls;
};
LSPClient::LSPClient() : T(std::make_unique<TransportImpl>()) {}
@@ -168,6 +188,10 @@ LSPClient::CallResult &LSPClient::call(llvm::StringRef Method,
return *Slot.second;
}
+void LSPClient::expectServerCall(llvm::StringRef Method) {
+ T->expectCall(Method);
+}
+
void LSPClient::notify(llvm::StringRef Method, llvm::json::Value Params) {
T->enqueue([Method(Method.str()),
Params(std::move(Params))](Transport::MessageHandler &H) {
@@ -181,6 +205,11 @@ LSPClient::takeNotifications(llvm::StringRef Method) {
return T->takeNotifications(Method);
}
+std::vector<llvm::json::Value>
+LSPClient::takeCallParams(llvm::StringRef Method) {
+ return T->takeCallParams(Method);
+}
+
void LSPClient::stop() { T->enqueue(nullptr); }
Transport &LSPClient::transport() { return *T; }
diff --git a/clang-tools-extra/clangd/unittests/LSPClient.h b/clang-tools-extra/clangd/unittests/LSPClient.h
index 3d459076321ac..074a61a1d95c2 100644
--- a/clang-tools-extra/clangd/unittests/LSPClient.h
+++ b/clang-tools-extra/clangd/unittests/LSPClient.h
@@ -58,10 +58,16 @@ class LSPClient {
// Enqueue an LSP method call, returns a promise for the reply. Threadsafe.
CallResult &call(llvm::StringRef Method, llvm::json::Value Params);
+ // Normally, any call from the server to the client will be marked as a test
+ // failure. Use this to allow a call to pass through, use takeCallParams() to
+ // retrieve it.
+ void expectServerCall(llvm::StringRef Method);
// Enqueue an LSP notification. Threadsafe.
void notify(llvm::StringRef Method, llvm::json::Value Params);
// Returns matching notifications since the last call to takeNotifications.
std::vector<llvm::json::Value> takeNotifications(llvm::StringRef Method);
+ // Returns matching parameters since the last call to takeCallParams.
+ std::vector<llvm::json::Value> takeCallParams(llvm::StringRef Method);
// The transport is shut down after all pending messages are sent.
void stop();
More information about the cfe-commits
mailing list