Skip to content

Commit a45df47

Browse files
tom-andersHighCommander4
tom-anders
and
HighCommander4
authored
[clangd] forward clang-tidy's readability-identifier-naming fix to textDocument/rename (#78454)
Co-authored-by: Nathan Ridge <[email protected]>
1 parent 45c226d commit a45df47

File tree

10 files changed

+182
-9
lines changed

10 files changed

+182
-9
lines changed

‎clang-tools-extra/clangd/ClangdLSPServer.cpp

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,23 @@ std::optional<int64_t> decodeVersion(llvm::StringRef Encoded) {
7373

7474
const llvm::StringLiteral ApplyFixCommand = "clangd.applyFix";
7575
const llvm::StringLiteral ApplyTweakCommand = "clangd.applyTweak";
76+
const llvm::StringLiteral ApplyRenameCommand = "clangd.applyRename";
77+
78+
CodeAction toCodeAction(const ClangdServer::CodeActionResult::Rename &R,
79+
const URIForFile &File) {
80+
CodeAction CA;
81+
CA.title = R.FixMessage;
82+
CA.kind = std::string(CodeAction::REFACTOR_KIND);
83+
CA.command.emplace();
84+
CA.command->title = R.FixMessage;
85+
CA.command->command = std::string(ApplyRenameCommand);
86+
RenameParams Params;
87+
Params.textDocument = TextDocumentIdentifier{File};
88+
Params.position = R.Diag.Range.start;
89+
Params.newName = R.NewName;
90+
CA.command->argument = Params;
91+
return CA;
92+
}
7693

7794
/// Transforms a tweak into a code action that would apply it if executed.
7895
/// EXPECTS: T.prepare() was called and returned true.
@@ -808,6 +825,16 @@ void ClangdLSPServer::onCommandApplyTweak(const TweakArgs &Args,
808825
std::move(Action));
809826
}
810827

828+
void ClangdLSPServer::onCommandApplyRename(const RenameParams &R,
829+
Callback<llvm::json::Value> Reply) {
830+
onRename(R, [this, Reply = std::move(Reply)](
831+
llvm::Expected<WorkspaceEdit> Edit) mutable {
832+
if (!Edit)
833+
Reply(Edit.takeError());
834+
applyEdit(std::move(*Edit), "Rename applied.", std::move(Reply));
835+
});
836+
}
837+
811838
void ClangdLSPServer::applyEdit(WorkspaceEdit WE, llvm::json::Value Success,
812839
Callback<llvm::json::Value> Reply) {
813840
ApplyWorkspaceEditParams Edit;
@@ -1046,6 +1073,10 @@ void ClangdLSPServer::onCodeAction(const CodeActionParams &Params,
10461073
CAs.back().diagnostics = {It->second};
10471074
}
10481075
}
1076+
1077+
for (const auto &R : Fixits->Renames)
1078+
CAs.push_back(toCodeAction(R, File));
1079+
10491080
for (const auto &TR : Fixits->TweakRefs)
10501081
CAs.push_back(toCodeAction(TR, File, Selection));
10511082

@@ -1667,6 +1698,7 @@ void ClangdLSPServer::bindMethods(LSPBinder &Bind,
16671698
Bind.method("textDocument/foldingRange", this, &ClangdLSPServer::onFoldingRange);
16681699
Bind.command(ApplyFixCommand, this, &ClangdLSPServer::onCommandApplyEdit);
16691700
Bind.command(ApplyTweakCommand, this, &ClangdLSPServer::onCommandApplyTweak);
1701+
Bind.command(ApplyRenameCommand, this, &ClangdLSPServer::onCommandApplyRename);
16701702

16711703
ApplyWorkspaceEdit = Bind.outgoingMethod("workspace/applyEdit");
16721704
PublishDiagnostics = Bind.outgoingNotification("textDocument/publishDiagnostics");

‎clang-tools-extra/clangd/ClangdLSPServer.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,7 @@ class ClangdLSPServer : private ClangdServer::Callbacks,
174174
/// Implement commands.
175175
void onCommandApplyEdit(const WorkspaceEdit &, Callback<llvm::json::Value>);
176176
void onCommandApplyTweak(const TweakArgs &, Callback<llvm::json::Value>);
177+
void onCommandApplyRename(const RenameParams &, Callback<llvm::json::Value>);
177178

178179
/// Outgoing LSP calls.
179180
LSPBinder::OutgoingMethod<ApplyWorkspaceEditParams,

‎clang-tools-extra/clangd/ClangdServer.cpp

Lines changed: 36 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -625,9 +625,10 @@ void ClangdServer::rename(PathRef File, Position Pos, llvm::StringRef NewName,
625625
WorkScheduler->runWithAST("Rename", File, std::move(Action));
626626
}
627627

628+
namespace {
628629
// May generate several candidate selections, due to SelectionTree ambiguity.
629630
// vector of pointers because GCC doesn't like non-copyable Selection.
630-
static llvm::Expected<std::vector<std::unique_ptr<Tweak::Selection>>>
631+
llvm::Expected<std::vector<std::unique_ptr<Tweak::Selection>>>
631632
tweakSelection(const Range &Sel, const InputsAndAST &AST,
632633
llvm::vfs::FileSystem *FS) {
633634
auto Begin = positionToOffset(AST.Inputs.Contents, Sel.start);
@@ -648,6 +649,27 @@ tweakSelection(const Range &Sel, const InputsAndAST &AST,
648649
return std::move(Result);
649650
}
650651

652+
// Some fixes may perform local renaming, we want to convert those to clangd
653+
// rename commands, such that we can leverage the index for more accurate
654+
// results.
655+
std::optional<ClangdServer::CodeActionResult::Rename>
656+
tryConvertToRename(const Diag *Diag, const Fix &Fix) {
657+
bool IsClangTidyRename = Diag->Source == Diag::ClangTidy &&
658+
Diag->Name == "readability-identifier-naming" &&
659+
!Fix.Edits.empty();
660+
if (IsClangTidyRename && Diag->InsideMainFile) {
661+
ClangdServer::CodeActionResult::Rename R;
662+
R.NewName = Fix.Edits.front().newText;
663+
R.FixMessage = Fix.Message;
664+
R.Diag = {Diag->Range, Diag->Message};
665+
return R;
666+
}
667+
668+
return std::nullopt;
669+
}
670+
671+
} // namespace
672+
651673
void ClangdServer::codeAction(const CodeActionInputs &Params,
652674
Callback<CodeActionResult> CB) {
653675
auto Action = [Params, CB = std::move(CB),
@@ -668,16 +690,22 @@ void ClangdServer::codeAction(const CodeActionInputs &Params,
668690
CodeActionResult Result;
669691
Result.Version = InpAST->AST.version().str();
670692
if (KindAllowed(CodeAction::QUICKFIX_KIND)) {
671-
auto FindMatchedFixes =
672-
[&InpAST](const DiagRef &DR) -> llvm::ArrayRef<Fix> {
693+
auto FindMatchedDiag = [&InpAST](const DiagRef &DR) -> const Diag * {
673694
for (const auto &Diag : InpAST->AST.getDiagnostics())
674695
if (Diag.Range == DR.Range && Diag.Message == DR.Message)
675-
return Diag.Fixes;
676-
return {};
696+
return &Diag;
697+
return nullptr;
677698
};
678-
for (const auto &Diag : Params.Diagnostics)
679-
for (const auto &Fix : FindMatchedFixes(Diag))
680-
Result.QuickFixes.push_back({Diag, Fix});
699+
for (const auto &DiagRef : Params.Diagnostics) {
700+
if (const auto *Diag = FindMatchedDiag(DiagRef))
701+
for (const auto &Fix : Diag->Fixes) {
702+
if (auto Rename = tryConvertToRename(Diag, Fix)) {
703+
Result.Renames.emplace_back(std::move(*Rename));
704+
} else {
705+
Result.QuickFixes.push_back({DiagRef, Fix});
706+
}
707+
}
708+
}
681709
}
682710

683711
// Collect Tweaks

‎clang-tools-extra/clangd/ClangdServer.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -380,6 +380,12 @@ class ClangdServer {
380380
};
381381
std::vector<QuickFix> QuickFixes;
382382
std::vector<TweakRef> TweakRefs;
383+
struct Rename {
384+
DiagRef Diag;
385+
std::string FixMessage;
386+
std::string NewName;
387+
};
388+
std::vector<Rename> Renames;
383389
};
384390
/// Surface code actions (quick-fixes for diagnostics, or available code
385391
/// tweaks) for a given range in a file.

‎clang-tools-extra/clangd/Protocol.cpp

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1187,6 +1187,14 @@ bool fromJSON(const llvm::json::Value &Params, RenameParams &R,
11871187
O.map("position", R.position) && O.map("newName", R.newName);
11881188
}
11891189

1190+
llvm::json::Value toJSON(const RenameParams &R) {
1191+
return llvm::json::Object{
1192+
{"textDocument", R.textDocument},
1193+
{"position", R.position},
1194+
{"newName", R.newName},
1195+
};
1196+
}
1197+
11901198
llvm::json::Value toJSON(const PrepareRenameResult &PRR) {
11911199
if (PRR.placeholder.empty())
11921200
return toJSON(PRR.range);

‎clang-tools-extra/clangd/Protocol.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1435,6 +1435,7 @@ struct RenameParams {
14351435
std::string newName;
14361436
};
14371437
bool fromJSON(const llvm::json::Value &, RenameParams &, llvm::json::Path);
1438+
llvm::json::Value toJSON(const RenameParams &);
14381439

14391440
struct PrepareRenameResult {
14401441
/// Range of the string to rename.

‎clang-tools-extra/clangd/test/initialize-params.test

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
# CHECK-NEXT: "executeCommandProvider": {
4141
# CHECK-NEXT: "commands": [
4242
# CHECK-NEXT: "clangd.applyFix",
43+
# CHECK-NEXT: "clangd.applyRename"
4344
# CHECK-NEXT: "clangd.applyTweak"
4445
# CHECK-NEXT: ]
4546
# CHECK-NEXT: },

‎clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,67 @@ TEST_F(LSPTest, RecordsLatencies) {
195195
EXPECT_THAT(Tracer.takeMetric("lsp_latency", MethodName), testing::SizeIs(1));
196196
}
197197

198+
// clang-tidy's renames are converted to clangd's internal rename functionality,
199+
// see clangd#1589 and clangd#741
200+
TEST_F(LSPTest, ClangTidyRename) {
201+
Annotations Header(R"cpp(
202+
void [[foo]]();
203+
)cpp");
204+
Annotations Source(R"cpp(
205+
void [[foo]]() {}
206+
)cpp");
207+
Opts.ClangTidyProvider = [](tidy::ClangTidyOptions &ClangTidyOpts,
208+
llvm::StringRef) {
209+
ClangTidyOpts.Checks = {"-*,readability-identifier-naming"};
210+
ClangTidyOpts.CheckOptions["readability-identifier-naming.FunctionCase"] =
211+
"CamelCase";
212+
};
213+
auto &Client = start();
214+
Client.didOpen("foo.hpp", Header.code());
215+
Client.didOpen("foo.cpp", Source.code());
216+
217+
auto RenameDiag = Client.diagnostics("foo.cpp").value().at(0);
218+
219+
auto RenameCommand =
220+
(*Client
221+
.call("textDocument/codeAction",
222+
llvm::json::Object{
223+
{"textDocument", Client.documentID("foo.cpp")},
224+
{"context",
225+
llvm::json::Object{
226+
{"diagnostics", llvm::json::Array{RenameDiag}}}},
227+
{"range", Source.range()}})
228+
.takeValue()
229+
.getAsArray())[0];
230+
231+
ASSERT_EQ((*RenameCommand.getAsObject())["title"], "change 'foo' to 'Foo'");
232+
233+
Client.expectServerCall("workspace/applyEdit");
234+
Client.call("workspace/executeCommand", RenameCommand);
235+
Client.sync();
236+
237+
auto Params = Client.takeCallParams("workspace/applyEdit");
238+
auto Uri = [&](llvm::StringRef Path) {
239+
return Client.uri(Path).getAsString().value().str();
240+
};
241+
llvm::json::Object ExpectedEdit = llvm::json::Object{
242+
{"edit", llvm::json::Object{
243+
{"changes",
244+
llvm::json::Object{
245+
{Uri("foo.hpp"), llvm::json::Array{llvm::json::Object{
246+
{"range", Header.range()},
247+
{"newText", "Foo"},
248+
}}},
249+
250+
{Uri("foo.cpp"), llvm::json::Array{llvm::json::Object{
251+
{"range", Source.range()},
252+
{"newText", "Foo"},
253+
}}}
254+
255+
}}}}};
256+
EXPECT_EQ(Params, std::vector{llvm::json::Value(std::move(ExpectedEdit))});
257+
}
258+
198259
TEST_F(LSPTest, IncomingCalls) {
199260
Annotations Code(R"cpp(
200261
void calle^e(int);

‎clang-tools-extra/clangd/unittests/LSPClient.cpp

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,20 @@ class LSPClient::TransportImpl : public Transport {
105105
return Result;
106106
}
107107

108+
void expectCall(llvm::StringRef Method) {
109+
std::lock_guard<std::mutex> Lock(Mu);
110+
Calls[Method] = {};
111+
}
112+
113+
std::vector<llvm::json::Value> takeCallParams(llvm::StringRef Method) {
114+
std::vector<llvm::json::Value> Result;
115+
{
116+
std::lock_guard<std::mutex> Lock(Mu);
117+
std::swap(Result, Calls[Method]);
118+
}
119+
return Result;
120+
}
121+
108122
private:
109123
void reply(llvm::json::Value ID,
110124
llvm::Expected<llvm::json::Value> V) override {
@@ -130,7 +144,12 @@ class LSPClient::TransportImpl : public Transport {
130144
void call(llvm::StringRef Method, llvm::json::Value Params,
131145
llvm::json::Value ID) override {
132146
logBody(Method, Params, /*Send=*/false);
133-
ADD_FAILURE() << "Unexpected server->client call " << Method;
147+
std::lock_guard<std::mutex> Lock(Mu);
148+
if (Calls.contains(Method)) {
149+
Calls[Method].push_back(std::move(Params));
150+
} else {
151+
ADD_FAILURE() << "Unexpected server->client call " << Method;
152+
}
134153
}
135154

136155
llvm::Error loop(MessageHandler &H) override {
@@ -152,6 +171,7 @@ class LSPClient::TransportImpl : public Transport {
152171
std::queue<std::function<void(Transport::MessageHandler &)>> Actions;
153172
std::condition_variable CV;
154173
llvm::StringMap<std::vector<llvm::json::Value>> Notifications;
174+
llvm::StringMap<std::vector<llvm::json::Value>> Calls;
155175
};
156176

157177
LSPClient::LSPClient() : T(std::make_unique<TransportImpl>()) {}
@@ -168,6 +188,10 @@ LSPClient::CallResult &LSPClient::call(llvm::StringRef Method,
168188
return *Slot.second;
169189
}
170190

191+
void LSPClient::expectServerCall(llvm::StringRef Method) {
192+
T->expectCall(Method);
193+
}
194+
171195
void LSPClient::notify(llvm::StringRef Method, llvm::json::Value Params) {
172196
T->enqueue([Method(Method.str()),
173197
Params(std::move(Params))](Transport::MessageHandler &H) {
@@ -181,6 +205,11 @@ LSPClient::takeNotifications(llvm::StringRef Method) {
181205
return T->takeNotifications(Method);
182206
}
183207

208+
std::vector<llvm::json::Value>
209+
LSPClient::takeCallParams(llvm::StringRef Method) {
210+
return T->takeCallParams(Method);
211+
}
212+
184213
void LSPClient::stop() { T->enqueue(nullptr); }
185214

186215
Transport &LSPClient::transport() { return *T; }

‎clang-tools-extra/clangd/unittests/LSPClient.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,10 +58,16 @@ class LSPClient {
5858

5959
// Enqueue an LSP method call, returns a promise for the reply. Threadsafe.
6060
CallResult &call(llvm::StringRef Method, llvm::json::Value Params);
61+
// Normally, any call from the server to the client will be marked as a test
62+
// failure. Use this to allow a call to pass through, use takeCallParams() to
63+
// retrieve it.
64+
void expectServerCall(llvm::StringRef Method);
6165
// Enqueue an LSP notification. Threadsafe.
6266
void notify(llvm::StringRef Method, llvm::json::Value Params);
6367
// Returns matching notifications since the last call to takeNotifications.
6468
std::vector<llvm::json::Value> takeNotifications(llvm::StringRef Method);
69+
// Returns matching parameters since the last call to takeCallParams.
70+
std::vector<llvm::json::Value> takeCallParams(llvm::StringRef Method);
6571
// The transport is shut down after all pending messages are sent.
6672
void stop();
6773

0 commit comments

Comments
 (0)