[clangd] forward clang-tidy's readability-identifier-naming fix to textDocument/rename

Author: tom-anders

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;
@@ -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");

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,

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

clang-tools-extra/clangd/ClangdServer.h

@@ -380,6 +380,12 @@ class ClangdServer {
     };
     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.

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)},

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 };
 

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:      },

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);

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; }

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();