diff options
author | Tom Praschan <13141438+tom-anders@users.noreply.github.com> | 2024-02-20 08:35:20 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2024-02-20 08:35:20 +0100 |
commit | a45df47375e50914900dcc07abd2fa67bfa0dd3b (patch) | |
tree | 912bac57d5ad020c5590da37c3fde40c42fa7658 /clang-tools-extra | |
parent | 45c226d4521515ff1a38292331d82356b273fff7 (diff) | |
download | llvm-a45df47375e50914900dcc07abd2fa67bfa0dd3b.zip llvm-a45df47375e50914900dcc07abd2fa67bfa0dd3b.tar.gz llvm-a45df47375e50914900dcc07abd2fa67bfa0dd3b.tar.bz2 |
[clangd] forward clang-tidy's readability-identifier-naming fix to textDocument/rename (#78454)
Co-authored-by: Nathan Ridge <zeratul976@hotmail.com>
Diffstat (limited to 'clang-tools-extra')
-rw-r--r-- | clang-tools-extra/clangd/ClangdLSPServer.cpp | 32 | ||||
-rw-r--r-- | clang-tools-extra/clangd/ClangdLSPServer.h | 1 | ||||
-rw-r--r-- | clang-tools-extra/clangd/ClangdServer.cpp | 44 | ||||
-rw-r--r-- | clang-tools-extra/clangd/ClangdServer.h | 6 | ||||
-rw-r--r-- | clang-tools-extra/clangd/Protocol.cpp | 8 | ||||
-rw-r--r-- | clang-tools-extra/clangd/Protocol.h | 1 | ||||
-rw-r--r-- | clang-tools-extra/clangd/test/initialize-params.test | 1 | ||||
-rw-r--r-- | clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp | 61 | ||||
-rw-r--r-- | clang-tools-extra/clangd/unittests/LSPClient.cpp | 31 | ||||
-rw-r--r-- | clang-tools-extra/clangd/unittests/LSPClient.h | 6 |
10 files changed, 182 insertions, 9 deletions
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp index 3e097cb..f29dadd 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.FixMessage; + CA.kind = std::string(CodeAction::REFACTOR_KIND); + CA.command.emplace(); + CA.command->title = R.FixMessage; + 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; @@ -1046,6 +1073,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)); @@ -1667,6 +1698,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 6a9f097..8bcb295 100644 --- a/clang-tools-extra/clangd/ClangdLSPServer.h +++ b/clang-tools-extra/clangd/ClangdLSPServer.h @@ -174,6 +174,7 @@ private: /// 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 6fb2641..3f9fd01 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,27 @@ 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 Fix &Fix) { + bool IsClangTidyRename = Diag->Source == Diag::ClangTidy && + Diag->Name == "readability-identifier-naming" && + !Fix.Edits.empty(); + if (IsClangTidyRename && Diag->InsideMainFile) { + ClangdServer::CodeActionResult::Rename R; + R.NewName = Fix.Edits.front().newText; + R.FixMessage = Fix.Message; + R.Diag = {Diag->Range, Diag->Message}; + return R; + } + + return std::nullopt; +} + +} // namespace + void ClangdServer::codeAction(const CodeActionInputs &Params, Callback<CodeActionResult> CB) { auto Action = [Params, CB = std::move(CB), @@ -668,16 +690,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, 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 a416602..1661028 100644 --- a/clang-tools-extra/clangd/ClangdServer.h +++ b/clang-tools-extra/clangd/ClangdServer.h @@ -380,6 +380,12 @@ public: }; std::vector<QuickFix> QuickFixes; std::vector<TweakRef> TweakRefs; + struct Rename { + DiagRef Diag; + std::string FixMessage; + 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 f93a941..8aa18bb 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 PrepareRenameResult &PRR) { if (PRR.placeholder.empty()) return toJSON(PRR.range); diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h index dc3fdb5..358d4c6 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 &); struct PrepareRenameResult { /// Range of the string to rename. diff --git a/clang-tools-extra/clangd/test/initialize-params.test b/clang-tools-extra/clangd/test/initialize-params.test index a1fdae9..7c96eb9 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 1f58e2c..555c4c5 100644 --- a/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp +++ b/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp @@ -195,6 +195,67 @@ 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]; + + ASSERT_EQ((*RenameCommand.getAsObject())["title"], "change 'foo' to 'Foo'"); + + 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 4d8ba13..b930523 100644 --- a/clang-tools-extra/clangd/unittests/LSPClient.cpp +++ b/clang-tools-extra/clangd/unittests/LSPClient.cpp @@ -105,6 +105,20 @@ public: 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 @@ private: 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 @@ private: 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 3d45907..074a61a 100644 --- a/clang-tools-extra/clangd/unittests/LSPClient.h +++ b/clang-tools-extra/clangd/unittests/LSPClient.h @@ -58,10 +58,16 @@ public: // 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(); |