[clang-tools-extra] [clang] [clangd] Add support to rename Objective-C selectors (PR #78872)

Alex Hoppen via cfe-commits cfe-commits at lists.llvm.org
Wed Jan 31 16:06:11 PST 2024


https://github.com/ahoppen updated https://github.com/llvm/llvm-project/pull/78872

>From 26d9d1a5a342df34cde921f7d9921e7b94dadd87 Mon Sep 17 00:00:00 2001
From: Alex Hoppen <ahoppen at apple.com>
Date: Tue, 19 Dec 2023 15:54:40 -0800
Subject: [PATCH 1/5] [Refactoring] Add capabilities to `SymbolName` to
 represent Objective-C selector names

---
 .../Tooling/Refactoring/Rename/SymbolName.h   | 30 +++++++-----
 clang/lib/Tooling/Refactoring/CMakeLists.txt  |  1 +
 .../Refactoring/Rename/RenamingAction.cpp     |  4 +-
 .../Refactoring/Rename/USRLocFinder.cpp       |  4 +-
 clang/lib/Tooling/Refactoring/SymbolName.cpp  | 46 +++++++++++++++++++
 clang/tools/clang-rename/CMakeLists.txt       |  1 +
 6 files changed, 70 insertions(+), 16 deletions(-)
 create mode 100644 clang/lib/Tooling/Refactoring/SymbolName.cpp

diff --git a/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h b/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h
index 6c28d40f3679c..077f315f657e7 100644
--- a/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h
+++ b/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h
@@ -15,6 +15,9 @@
 #include "llvm/ADT/StringRef.h"
 
 namespace clang {
+
+class LangOptions;
+
 namespace tooling {
 
 /// A name of a symbol.
@@ -27,19 +30,22 @@ namespace tooling {
 /// //       ^~ string 0 ~~~~~         ^~ string 1 ~~~~~
 /// \endcode
 class SymbolName {
+  llvm::SmallVector<std::string, 1> NamePieces;
+
 public:
-  explicit SymbolName(StringRef Name) {
-    // While empty symbol names are valid (Objective-C selectors can have empty
-    // name pieces), occurrences Objective-C selectors are created using an
-    // array of strings instead of just one string.
-    assert(!Name.empty() && "Invalid symbol name!");
-    this->Name.push_back(Name.str());
-  }
-
-  ArrayRef<std::string> getNamePieces() const { return Name; }
-
-private:
-  llvm::SmallVector<std::string, 1> Name;
+  /// Create a new \c SymbolName with the specified pieces.
+  explicit SymbolName(ArrayRef<StringRef> NamePieces);
+
+  /// Creates a \c SymbolName from the given string representation.
+  ///
+  /// For Objective-C symbol names, this splits a selector into multiple pieces
+  /// on `:`. For all other languages the name is used as the symbol name.
+  SymbolName(StringRef Name, bool IsObjectiveCSelector);
+  SymbolName(StringRef Name, const LangOptions &LangOpts);
+
+  ArrayRef<std::string> getNamePieces() const { return NamePieces; }
+
+  void print(raw_ostream &OS) const;
 };
 
 } // end namespace tooling
diff --git a/clang/lib/Tooling/Refactoring/CMakeLists.txt b/clang/lib/Tooling/Refactoring/CMakeLists.txt
index d3077be8810aa..a4a80ce834431 100644
--- a/clang/lib/Tooling/Refactoring/CMakeLists.txt
+++ b/clang/lib/Tooling/Refactoring/CMakeLists.txt
@@ -13,6 +13,7 @@ add_clang_library(clangToolingRefactoring
   Rename/USRFinder.cpp
   Rename/USRFindingAction.cpp
   Rename/USRLocFinder.cpp
+  SymbolName.cpp
 
   LINK_LIBS
   clangAST
diff --git a/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp b/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
index 72598601d47d6..4965977d1f7aa 100644
--- a/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
+++ b/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
@@ -82,7 +82,7 @@ RenameOccurrences::createSourceReplacements(RefactoringRuleContext &Context) {
   if (!Occurrences)
     return Occurrences.takeError();
   // FIXME: Verify that the new name is valid.
-  SymbolName Name(NewName);
+  SymbolName Name(NewName, /*IsObjectiveCSelector=*/false);
   return createRenameReplacements(
       *Occurrences, Context.getASTContext().getSourceManager(), Name);
 }
@@ -219,7 +219,7 @@ class RenamingASTConsumer : public ASTConsumer {
     }
     // FIXME: Support multi-piece names.
     // FIXME: better error handling (propagate error out).
-    SymbolName NewNameRef(NewName);
+    SymbolName NewNameRef(NewName, /*IsObjectiveCSelector=*/false);
     Expected<std::vector<AtomicChange>> Change =
         createRenameReplacements(Occurrences, SourceMgr, NewNameRef);
     if (!Change) {
diff --git a/clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp b/clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
index c18f9290471fe..43e48f24caa9e 100644
--- a/clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
+++ b/clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
@@ -60,8 +60,8 @@ class USRLocFindingASTVisitor
                                    const ASTContext &Context)
       : RecursiveSymbolVisitor(Context.getSourceManager(),
                                Context.getLangOpts()),
-        USRSet(USRs.begin(), USRs.end()), PrevName(PrevName), Context(Context) {
-  }
+        USRSet(USRs.begin(), USRs.end()),
+        PrevName(PrevName, /*IsObjectiveCSelector=*/false), Context(Context) {}
 
   bool visitSymbolOccurrence(const NamedDecl *ND,
                              ArrayRef<SourceRange> NameRanges) {
diff --git a/clang/lib/Tooling/Refactoring/SymbolName.cpp b/clang/lib/Tooling/Refactoring/SymbolName.cpp
new file mode 100644
index 0000000000000..7fd50b2c4df7b
--- /dev/null
+++ b/clang/lib/Tooling/Refactoring/SymbolName.cpp
@@ -0,0 +1,46 @@
+//===--- SymbolName.cpp - Clang refactoring library -----------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/Tooling/Refactoring/Rename/SymbolName.h"
+#include "clang/Basic/LangOptions.h"
+#include "llvm/Support/raw_ostream.h"
+
+namespace clang {
+namespace tooling {
+
+SymbolName::SymbolName(StringRef Name, const LangOptions &LangOpts)
+    : SymbolName(Name, LangOpts.ObjC) {}
+
+SymbolName::SymbolName(StringRef Name, bool IsObjectiveCSelector) {
+  if (!IsObjectiveCSelector) {
+    NamePieces.push_back(Name.str());
+    return;
+  }
+  // Decompose an Objective-C selector name into multiple strings.
+  do {
+    auto StringAndName = Name.split(':');
+    NamePieces.push_back(StringAndName.first.str());
+    Name = StringAndName.second;
+  } while (!Name.empty());
+}
+
+SymbolName::SymbolName(ArrayRef<StringRef> NamePieces) {
+  for (const auto &Piece : NamePieces)
+    this->NamePieces.push_back(Piece.str());
+}
+
+void SymbolName::print(raw_ostream &OS) const {
+  for (size_t I = 0, E = NamePieces.size(); I != E; ++I) {
+    if (I != 0)
+      OS << ':';
+    OS << NamePieces[I];
+  }
+}
+
+} // end namespace tooling
+} // end namespace clang
diff --git a/clang/tools/clang-rename/CMakeLists.txt b/clang/tools/clang-rename/CMakeLists.txt
index f4c4e520520d9..2f79f6ce4f3ff 100644
--- a/clang/tools/clang-rename/CMakeLists.txt
+++ b/clang/tools/clang-rename/CMakeLists.txt
@@ -16,6 +16,7 @@ clang_target_link_libraries(clang-rename
   clangTooling
   clangToolingCore
   clangToolingRefactoring
+  clangToolingSyntax
   )
 
 install(FILES clang-rename.py

>From 6b2cf9e33c883671b4e2918a26623733d63ce410 Mon Sep 17 00:00:00 2001
From: Alex Hoppen <ahoppen at apple.com>
Date: Tue, 19 Dec 2023 16:26:45 -0800
Subject: [PATCH 2/5] [clangd] Support renaming of Objective-C method selectors

---
 clang-tools-extra/clangd/refactor/Rename.cpp  | 75 ++++++++++-----
 .../clangd/unittests/RenameTests.cpp          | 55 ++++++++++-
 .../Refactoring/Rename/RenamingAction.h       | 21 +++++
 .../Tooling/Refactoring/Rename/SymbolName.h   | 21 +++++
 clang/lib/Tooling/Refactoring/CMakeLists.txt  |  1 +
 .../Refactoring/Rename/RenamingAction.cpp     | 94 +++++++++++++++++++
 clang/lib/Tooling/Refactoring/SymbolName.cpp  | 20 ++++
 clang/tools/clang-refactor/CMakeLists.txt     |  1 +
 clang/tools/libclang/CMakeLists.txt           |  1 +
 9 files changed, 263 insertions(+), 26 deletions(-)

diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index 11f9e4627af76..258c1a23d0fe2 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -26,6 +26,8 @@
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Tooling/Refactoring/Rename/RenamingAction.h"
+#include "clang/Tooling/Refactoring/Rename/SymbolName.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringExtras.h"
@@ -40,6 +42,8 @@ namespace clang {
 namespace clangd {
 namespace {
 
+using tooling::SymbolName;
+
 std::optional<std::string> filePath(const SymbolLocation &Loc,
                                     llvm::StringRef HintFilePath) {
   if (!Loc)
@@ -517,25 +521,28 @@ static bool mayBeValidIdentifier(llvm::StringRef Ident) {
 // Check if we can rename the given RenameDecl into NewName.
 // Return details if the rename would produce a conflict.
 std::optional<InvalidName> checkName(const NamedDecl &RenameDecl,
-                                     llvm::StringRef NewName) {
+                                     const SymbolName &NewName) {
   trace::Span Tracer("CheckName");
   static constexpr trace::Metric InvalidNameMetric(
       "rename_name_invalid", trace::Metric::Counter, "invalid_kind");
   auto &ASTCtx = RenameDecl.getASTContext();
   std::optional<InvalidName> Result;
-  if (isKeyword(NewName, ASTCtx.getLangOpts()))
-    Result = InvalidName{InvalidName::Keywords, NewName.str()};
-  else if (!mayBeValidIdentifier(NewName))
-    Result = InvalidName{InvalidName::BadIdentifier, NewName.str()};
-  else {
-    // Name conflict detection.
-    // Function conflicts are subtle (overloading), so ignore them.
-    if (RenameDecl.getKind() != Decl::Function &&
-        RenameDecl.getKind() != Decl::CXXMethod) {
-      if (auto *Conflict = lookupSiblingWithName(ASTCtx, RenameDecl, NewName))
-        Result = InvalidName{
-            InvalidName::Conflict,
-            Conflict->getLocation().printToString(ASTCtx.getSourceManager())};
+  if (auto Identifier = NewName.getSinglePiece()) {
+    if (isKeyword(*Identifier, ASTCtx.getLangOpts()))
+      Result = InvalidName{InvalidName::Keywords, *Identifier};
+    else if (!mayBeValidIdentifier(*Identifier))
+      Result = InvalidName{InvalidName::BadIdentifier, *Identifier};
+    else {
+      // Name conflict detection.
+      // Function conflicts are subtle (overloading), so ignore them.
+      if (RenameDecl.getKind() != Decl::Function &&
+          RenameDecl.getKind() != Decl::CXXMethod) {
+        if (auto *Conflict =
+                lookupSiblingWithName(ASTCtx, RenameDecl, *Identifier))
+          Result = InvalidName{
+              InvalidName::Conflict,
+              Conflict->getLocation().printToString(ASTCtx.getSourceManager())};
+      }
     }
   }
   if (Result)
@@ -546,7 +553,7 @@ std::optional<InvalidName> checkName(const NamedDecl &RenameDecl,
 // AST-based rename, it renames all occurrences in the main file.
 llvm::Expected<tooling::Replacements>
 renameWithinFile(ParsedAST &AST, const NamedDecl &RenameDecl,
-                 llvm::StringRef NewName) {
+                 const SymbolName &OldName, const SymbolName &NewName) {
   trace::Span Tracer("RenameWithinFile");
   const SourceManager &SM = AST.getSourceManager();
 
@@ -569,9 +576,28 @@ renameWithinFile(ParsedAST &AST, const NamedDecl &RenameDecl,
     //   }
     if (!isInsideMainFile(RenameLoc, SM))
       continue;
-    if (auto Err = FilteredChanges.add(tooling::Replacement(
-            SM, CharSourceRange::getTokenRange(RenameLoc), NewName)))
-      return std::move(Err);
+    if (std::optional<std::string> Identifier = NewName.getSinglePiece()) {
+      tooling::Replacement NewReplacement(
+          SM, CharSourceRange::getTokenRange(RenameLoc), *Identifier);
+      if (auto Err = FilteredChanges.add(NewReplacement))
+        return std::move(Err);
+    } else {
+      llvm::Expected<SmallVector<SourceLocation>> PieceLocations =
+          findObjCSymbolSelectorPieces(
+              AST.getTokens().expandedTokens(), SM, RenameLoc, OldName,
+              tooling::ObjCSymbolSelectorKind::Unknown);
+      if (!PieceLocations) {
+        return PieceLocations.takeError();
+      }
+      assert(PieceLocations->size() == NewName.getNamePieces().size());
+      for (auto [Location, NewPiece] :
+           llvm::zip_equal(*PieceLocations, NewName.getNamePieces())) {
+        tooling::Replacement NewReplacement(
+            SM, CharSourceRange::getTokenRange(Location), NewPiece);
+        if (auto Err = FilteredChanges.add(NewReplacement))
+          return std::move(Err);
+      }
+    }
   }
   return FilteredChanges;
 }
@@ -779,12 +805,14 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
   if (DeclsUnderCursor.size() > 1)
     return makeError(ReasonToReject::AmbiguousSymbol);
   const auto &RenameDecl = **DeclsUnderCursor.begin();
-  const auto *ID = RenameDecl.getIdentifier();
-  if (!ID)
+  DeclarationName Name = RenameDecl.getDeclName();
+  if (!Name)
     return makeError(ReasonToReject::UnsupportedSymbol);
-  if (ID->getName() == RInputs.NewName)
+  SymbolName OldSymbolName(Name);
+  SymbolName NewSymbolName(RInputs.NewName, AST.getLangOpts());
+  if (OldSymbolName == NewSymbolName)
     return makeError(ReasonToReject::SameName);
-  auto Invalid = checkName(RenameDecl, RInputs.NewName);
+  auto Invalid = checkName(RenameDecl, NewSymbolName);
   if (Invalid)
     return makeError(std::move(*Invalid));
 
@@ -802,7 +830,8 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
   // To make cross-file rename work for local symbol, we use a hybrid solution:
   //   - run AST-based rename on the main file;
   //   - run index-based rename on other affected files;
-  auto MainFileRenameEdit = renameWithinFile(AST, RenameDecl, RInputs.NewName);
+  auto MainFileRenameEdit =
+      renameWithinFile(AST, RenameDecl, OldSymbolName, NewSymbolName);
   if (!MainFileRenameEdit)
     return MainFileRenameEdit.takeError();
 
diff --git a/clang-tools-extra/clangd/unittests/RenameTests.cpp b/clang-tools-extra/clangd/unittests/RenameTests.cpp
index 9cbf59684fbc1..ded636b0562bc 100644
--- a/clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ b/clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -876,6 +876,55 @@ TEST(RenameTest, WithinFileRename) {
   }
 }
 
+TEST(RenameTest, ObjCWithinFileRename) {
+  struct TestCase {
+    llvm::StringRef Input;
+    llvm::StringRef NewName;
+    llvm::StringRef Expected;
+  };
+  TestCase Tests[] = {{
+      R"cpp(
+        @interface Foo
+        - (int)performA^ction:(int)action w^ith:(int)value;
+        @end
+        @implementation Foo
+        - (int)performAc^tion:(int)action w^ith:(int)value {
+          [self performAction:action with:value];
+        }
+        @end
+      )cpp",
+      "performNewAction:by:",
+      R"cpp(
+        @interface Foo
+        - (int)performNewAction:(int)action by:(int)value;
+        @end
+        @implementation Foo
+        - (int)performNewAction:(int)action by:(int)value {
+          [self performNewAction:action by:value];
+        }
+        @end
+      )cpp",
+  }};
+  for (TestCase T : Tests) {
+    SCOPED_TRACE(T.Input);
+    Annotations Code(T.Input);
+    auto TU = TestTU::withCode(Code.code());
+    TU.ExtraArgs.push_back("-xobjective-c");
+    auto AST = TU.build();
+    auto Index = TU.index();
+    for (const auto &RenamePos : Code.points()) {
+      auto RenameResult =
+          rename({RenamePos, T.NewName, AST, testPath(TU.Filename),
+                  getVFSFromAST(AST), Index.get()});
+      ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
+      ASSERT_EQ(1u, RenameResult->GlobalChanges.size());
+      EXPECT_EQ(
+          applyEdits(std::move(RenameResult->GlobalChanges)).front().second,
+          T.Expected);
+    }
+  }
+}
+
 TEST(RenameTest, Renameable) {
   struct Case {
     const char *Code;
@@ -926,12 +975,12 @@ TEST(RenameTest, Renameable) {
         void f(X x) {x+^+;})cpp",
           "no symbol", HeaderFile},
 
-      {R"cpp(// disallow rename on non-normal identifiers.
+      {R"cpp(
          @interface Foo {}
-         -(int) fo^o:(int)x; // Token is an identifier, but declaration name isn't a simple identifier.
+         -(int) [[fo^o]]:(int)x;
          @end
        )cpp",
-       "not a supported kind", HeaderFile},
+       nullptr, HeaderFile},
       {R"cpp(
          void foo(int);
          void foo(char);
diff --git a/clang/include/clang/Tooling/Refactoring/Rename/RenamingAction.h b/clang/include/clang/Tooling/Refactoring/Rename/RenamingAction.h
index 43a8d56e4e717..79c2b384b4eb0 100644
--- a/clang/include/clang/Tooling/Refactoring/Rename/RenamingAction.h
+++ b/clang/include/clang/Tooling/Refactoring/Rename/RenamingAction.h
@@ -19,6 +19,7 @@
 #include "clang/Tooling/Refactoring/RefactoringActionRules.h"
 #include "clang/Tooling/Refactoring/RefactoringOptions.h"
 #include "clang/Tooling/Refactoring/Rename/SymbolOccurrences.h"
+#include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/Support/Error.h"
 
 namespace clang {
@@ -116,6 +117,26 @@ class QualifiedRenamingAction {
   std::map<std::string, tooling::Replacements> &FileToReplaces;
 };
 
+enum class ObjCSymbolSelectorKind {
+  /// The rename location is an Objective-C method call, eg. `[self add: 1]`.
+  MessageSend,
+
+  /// The rename location is an Objective-C method definition, eg.
+  /// ` - (void)add:(int)theValue`
+  MethodDecl,
+
+  /// It is unknown if the renamed location is a method call or declaration.
+  ///
+  /// The selector kind is being used to improve error recovery, passing unknown
+  /// does not lead to correctness issues.
+  Unknown
+};
+
+llvm::Expected<SmallVector<SourceLocation>> findObjCSymbolSelectorPieces(
+    ArrayRef<syntax::Token> Tokens, const SourceManager &SrcMgr,
+    SourceLocation RenameLoc, const SymbolName &OldName,
+    ObjCSymbolSelectorKind Kind);
+
 } // end namespace tooling
 } // end namespace clang
 
diff --git a/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h b/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h
index 077f315f657e7..31520ecabb31a 100644
--- a/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h
+++ b/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h
@@ -9,6 +9,7 @@
 #ifndef LLVM_CLANG_TOOLING_REFACTORING_RENAME_SYMBOLNAME_H
 #define LLVM_CLANG_TOOLING_REFACTORING_RENAME_SYMBOLNAME_H
 
+#include "clang/AST/DeclarationName.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallVector.h"
@@ -36,6 +37,8 @@ class SymbolName {
   /// Create a new \c SymbolName with the specified pieces.
   explicit SymbolName(ArrayRef<StringRef> NamePieces);
 
+  explicit SymbolName(const DeclarationName &Name);
+
   /// Creates a \c SymbolName from the given string representation.
   ///
   /// For Objective-C symbol names, this splits a selector into multiple pieces
@@ -45,7 +48,25 @@ class SymbolName {
 
   ArrayRef<std::string> getNamePieces() const { return NamePieces; }
 
+  /// If this symbol consists of a single piece return it, otherwise return
+  /// `None`.
+  ///
+  /// Only symbols in Objective-C can consist of multiple pieces, so this
+  /// function always returns a value for non-Objective-C symbols.
+  std::optional<std::string> getSinglePiece() const;
+
+  /// Returns a human-readable version of this symbol name.
+  ///
+  /// If the symbol consists of multiple pieces (aka. it is an Objective-C
+  /// selector/method name), the pieces are separated by `:`, otherwise just an
+  /// identifier name.
+  std::string getAsString() const;
+
   void print(raw_ostream &OS) const;
+
+  bool operator==(const SymbolName &Other) const {
+    return NamePieces == Other.NamePieces;
+  }
 };
 
 } // end namespace tooling
diff --git a/clang/lib/Tooling/Refactoring/CMakeLists.txt b/clang/lib/Tooling/Refactoring/CMakeLists.txt
index a4a80ce834431..f78f64ea2ef64 100644
--- a/clang/lib/Tooling/Refactoring/CMakeLists.txt
+++ b/clang/lib/Tooling/Refactoring/CMakeLists.txt
@@ -24,6 +24,7 @@ add_clang_library(clangToolingRefactoring
   clangLex
   clangRewrite
   clangToolingCore
+  clangToolingSyntax
 
   DEPENDS
   omp_gen
diff --git a/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp b/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
index 4965977d1f7aa..b76cb06bc8aa9 100644
--- a/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
+++ b/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
@@ -275,5 +275,99 @@ std::unique_ptr<ASTConsumer> QualifiedRenamingAction::newASTConsumer() {
   return std::make_unique<USRSymbolRenamer>(NewNames, USRList, FileToReplaces);
 }
 
+static bool isMatchingSelectorName(const syntax::Token &Tok,
+                                   const syntax::Token &Next,
+                                   StringRef NamePiece,
+                                   const SourceManager &SrcMgr) {
+  if (NamePiece.empty())
+    return Tok.kind() == tok::colon;
+  return (Tok.kind() == tok::identifier || Tok.kind() == tok::raw_identifier) &&
+         Next.kind() == tok::colon && Tok.text(SrcMgr) == NamePiece;
+}
+
+llvm::Expected<SmallVector<SourceLocation>>
+findObjCSymbolSelectorPieces(ArrayRef<syntax::Token> AllTokens,
+                             const SourceManager &SM, SourceLocation RenameLoc,
+                             const SymbolName &OldName,
+                             ObjCSymbolSelectorKind Kind) {
+  ArrayRef<syntax::Token> Tokens =
+      AllTokens.drop_while([RenameLoc](syntax::Token Tok) -> bool {
+        return Tok.location() != RenameLoc;
+      });
+  assert(!Tokens.empty() && "no tokens");
+  assert(OldName.getNamePieces()[0].empty() ||
+         Tokens[0].text(SM) == OldName.getNamePieces()[0]);
+  assert(OldName.getNamePieces().size() > 1);
+  SmallVector<SourceLocation> Result;
+
+  Result.push_back(Tokens[0].location());
+
+  // We have to track square brackets, parens and braces as we want to skip the
+  // tokens inside them. This ensures that we don't use identical selector
+  // pieces in inner message sends, blocks, lambdas and @selector expressions.
+  unsigned SquareCount = 0;
+  unsigned ParenCount = 0;
+  unsigned BraceCount = 0;
+
+  // Start looking for the next selector piece.
+  unsigned Last = Tokens.size() - 1;
+  // Skip the ':' or any other token after the first selector piece token.
+  for (unsigned Index = OldName.getNamePieces()[0].empty() ? 1 : 2;
+       Index < Last; ++Index) {
+    const auto &Tok = Tokens[Index];
+
+    bool NoScoping = SquareCount == 0 && BraceCount == 0 && ParenCount == 0;
+    if (NoScoping &&
+        isMatchingSelectorName(Tok, Tokens[Index + 1],
+                               OldName.getNamePieces()[Result.size()], SM)) {
+      if (!OldName.getNamePieces()[Result.size()].empty()) {
+        // Skip the ':' after the name. This ensures that it won't match a
+        // follow-up selector piece with an empty name.
+        ++Index;
+      }
+      Result.push_back(Tok.location());
+      // All the selector pieces have been found.
+      if (Result.size() == OldName.getNamePieces().size())
+        return Result;
+    } else if (Tok.kind() == tok::r_square) {
+      // Stop scanning at the end of the message send.
+      // Also account for spurious ']' in blocks or lambdas.
+      if (Kind == ObjCSymbolSelectorKind::MessageSend && !SquareCount &&
+          !BraceCount)
+        break;
+      if (SquareCount)
+        --SquareCount;
+    } else if (Tok.kind() == tok::l_square)
+      ++SquareCount;
+    else if (Tok.kind() == tok::l_paren)
+      ++ParenCount;
+    else if (Tok.kind() == tok::r_paren) {
+      if (!ParenCount)
+        break;
+      --ParenCount;
+    } else if (Tok.kind() == tok::l_brace) {
+      // Stop scanning at the start of the of the method's body.
+      // Also account for any spurious blocks inside argument parameter types
+      // or parameter attributes.
+      if (Kind == ObjCSymbolSelectorKind::MethodDecl && !BraceCount &&
+          !ParenCount)
+        break;
+      ++BraceCount;
+    } else if (Tok.kind() == tok::r_brace) {
+      if (!BraceCount)
+        break;
+      --BraceCount;
+    }
+    // Stop scanning at the end of the method's declaration.
+    if (Kind == ObjCSymbolSelectorKind::MethodDecl && NoScoping &&
+        (Tok.kind() == tok::semi || Tok.kind() == tok::minus ||
+         Tok.kind() == tok::plus))
+      break;
+  }
+  return llvm::make_error<llvm::StringError>(
+      "failed to find all selector pieces in the source code",
+      inconvertibleErrorCode());
+}
+
 } // end namespace tooling
 } // end namespace clang
diff --git a/clang/lib/Tooling/Refactoring/SymbolName.cpp b/clang/lib/Tooling/Refactoring/SymbolName.cpp
index 7fd50b2c4df7b..be0dd721b1d22 100644
--- a/clang/lib/Tooling/Refactoring/SymbolName.cpp
+++ b/clang/lib/Tooling/Refactoring/SymbolName.cpp
@@ -13,6 +13,11 @@
 namespace clang {
 namespace tooling {
 
+SymbolName::SymbolName(const DeclarationName &DeclName)
+    : SymbolName(DeclName.getAsString(),
+                 /*IsObjectiveCSelector=*/DeclName.getNameKind() ==
+                     DeclarationName::NameKind::ObjCMultiArgSelector) {}
+
 SymbolName::SymbolName(StringRef Name, const LangOptions &LangOpts)
     : SymbolName(Name, LangOpts.ObjC) {}
 
@@ -34,6 +39,21 @@ SymbolName::SymbolName(ArrayRef<StringRef> NamePieces) {
     this->NamePieces.push_back(Piece.str());
 }
 
+std::optional<std::string> SymbolName::getSinglePiece() const {
+  if (getNamePieces().size() == 1) {
+    return NamePieces.front();
+  } else {
+    return std::nullopt;
+  }
+}
+
+std::string SymbolName::getAsString() const {
+  std::string Result;
+  llvm::raw_string_ostream OS(Result);
+  this->print(OS);
+  return Result;
+}
+
 void SymbolName::print(raw_ostream &OS) const {
   for (size_t I = 0, E = NamePieces.size(); I != E; ++I) {
     if (I != 0)
diff --git a/clang/tools/clang-refactor/CMakeLists.txt b/clang/tools/clang-refactor/CMakeLists.txt
index a21d84d5385b4..092740be15f01 100644
--- a/clang/tools/clang-refactor/CMakeLists.txt
+++ b/clang/tools/clang-refactor/CMakeLists.txt
@@ -20,4 +20,5 @@ clang_target_link_libraries(clang-refactor
   clangTooling
   clangToolingCore
   clangToolingRefactoring
+  clangToolingSyntax
   )
diff --git a/clang/tools/libclang/CMakeLists.txt b/clang/tools/libclang/CMakeLists.txt
index 1cfc46eb1a52f..f400986be797a 100644
--- a/clang/tools/libclang/CMakeLists.txt
+++ b/clang/tools/libclang/CMakeLists.txt
@@ -69,6 +69,7 @@ set(LIBS
   clangSema
   clangSerialization
   clangTooling
+  clangToolingSyntax
 )
 
 if (CLANG_ENABLE_ARCMT)

>From 01234dd67a6bd64df44fea0369335d37482c1a67 Mon Sep 17 00:00:00 2001
From: Alex Hoppen <ahoppen at apple.com>
Date: Wed, 20 Dec 2023 10:57:49 -0800
Subject: [PATCH 3/5] [clangd] Support renaming of Objective-C selectors across
 files

---
 clang-tools-extra/clangd/SourceCode.cpp       |  18 +-
 clang-tools-extra/clangd/SourceCode.h         |   6 +-
 .../clangd/index/SymbolCollector.cpp          |  27 +-
 clang-tools-extra/clangd/refactor/Rename.cpp  |  68 +++--
 clang-tools-extra/clangd/refactor/Rename.h    |  21 +-
 .../clangd/unittests/RenameTests.cpp          | 284 +++++++++++++++++-
 .../clangd/unittests/SourceCodeTests.cpp      |   4 +-
 clang/include/clang/Tooling/Syntax/Tokens.h   |  14 +
 clang/lib/Tooling/Refactoring/SymbolName.cpp  |   4 +-
 clang/lib/Tooling/Syntax/Tokens.cpp           |  11 +
 10 files changed, 395 insertions(+), 62 deletions(-)

diff --git a/clang-tools-extra/clangd/SourceCode.cpp b/clang-tools-extra/clangd/SourceCode.cpp
index 835038423fdf3..0081a69357b02 100644
--- a/clang-tools-extra/clangd/SourceCode.cpp
+++ b/clang-tools-extra/clangd/SourceCode.cpp
@@ -625,16 +625,16 @@ llvm::StringMap<unsigned> collectIdentifiers(llvm::StringRef Content,
   return Identifiers;
 }
 
-std::vector<Range> collectIdentifierRanges(llvm::StringRef Identifier,
-                                           llvm::StringRef Content,
-                                           const LangOptions &LangOpts) {
+std::vector<Range>
+collectIdentifierRanges(llvm::StringRef Identifier,
+                        const syntax::UnexpandedTokenBuffer &Tokens) {
   std::vector<Range> Ranges;
-  lex(Content, LangOpts,
-      [&](const syntax::Token &Tok, const SourceManager &SM) {
-        if (Tok.kind() != tok::identifier || Tok.text(SM) != Identifier)
-          return;
-        Ranges.push_back(halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
-      });
+  const SourceManager &SM = Tokens.sourceManager();
+  for (const syntax::Token &Tok : Tokens.tokens()) {
+    if (Tok.kind() != tok::identifier || Tok.text(SM) != Identifier)
+      continue;
+    Ranges.push_back(halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
+  }
   return Ranges;
 }
 
diff --git a/clang-tools-extra/clangd/SourceCode.h b/clang-tools-extra/clangd/SourceCode.h
index a1bb44c176120..f60683fc4f21c 100644
--- a/clang-tools-extra/clangd/SourceCode.h
+++ b/clang-tools-extra/clangd/SourceCode.h
@@ -217,9 +217,9 @@ llvm::StringMap<unsigned> collectIdentifiers(llvm::StringRef Content,
                                              const format::FormatStyle &Style);
 
 /// Collects all ranges of the given identifier in the source code.
-std::vector<Range> collectIdentifierRanges(llvm::StringRef Identifier,
-                                           llvm::StringRef Content,
-                                           const LangOptions &LangOpts);
+std::vector<Range>
+collectIdentifierRanges(llvm::StringRef Identifier,
+                        const syntax::UnexpandedTokenBuffer &Tokens);
 
 /// Collects words from the source code.
 /// Unlike collectIdentifiers:
diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp
index bf838e53f2a21..24bd3c426b3e8 100644
--- a/clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -173,9 +173,20 @@ std::optional<RelationKind> indexableRelation(const index::SymbolRelation &R) {
 bool isSpelled(SourceLocation Loc, const NamedDecl &ND) {
   auto Name = ND.getDeclName();
   const auto NameKind = Name.getNameKind();
-  if (NameKind != DeclarationName::Identifier &&
-      NameKind != DeclarationName::CXXConstructorName)
+  bool PrefixComparison;
+  switch (NameKind) {
+  case DeclarationName::Identifier:
+  case DeclarationName::CXXConstructorName:
+  case DeclarationName::ObjCZeroArgSelector:
+    PrefixComparison = false;
+    break;
+  case DeclarationName::ObjCOneArgSelector:
+  case DeclarationName::ObjCMultiArgSelector:
+    PrefixComparison = true;
+    break;
+  default:
     return false;
+  }
   const auto &AST = ND.getASTContext();
   const auto &SM = AST.getSourceManager();
   const auto &LO = AST.getLangOpts();
@@ -183,7 +194,17 @@ bool isSpelled(SourceLocation Loc, const NamedDecl &ND) {
   if (clang::Lexer::getRawToken(Loc, Tok, SM, LO))
     return false;
   auto StrName = Name.getAsString();
-  return clang::Lexer::getSpelling(Tok, SM, LO) == StrName;
+  std::string LexerSpelling = clang::Lexer::getSpelling(Tok, SM, LO);
+  if (PrefixComparison) {
+    // The lexer spelling at the source location is only the first label of an
+    // Objective-C selector, eg. if `StrName` is `performAction:with:`, then the
+    // token at the requested location is `performAction`. Re-building the
+    // entire selector from the lexer is too complicated here, so just perform
+    // a prefix comparison.
+    return StringRef(StrName).starts_with(LexerSpelling);
+  } else {
+    return StrName == LexerSpelling;
+  }
 }
 } // namespace
 
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index 258c1a23d0fe2..c6af9c137751f 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -690,8 +690,9 @@ findOccurrencesOutsideFile(const NamedDecl &RenameDecl,
 // there is no dirty buffer.
 llvm::Expected<FileEdits>
 renameOutsideFile(const NamedDecl &RenameDecl, llvm::StringRef MainFilePath,
-                  llvm::StringRef NewName, const SymbolIndex &Index,
-                  size_t MaxLimitFiles, llvm::vfs::FileSystem &FS) {
+                  SymbolName OldName, SymbolName NewName,
+                  const SymbolIndex &Index, size_t MaxLimitFiles,
+                  llvm::vfs::FileSystem &FS) {
   trace::Span Tracer("RenameOutsideFile");
   auto AffectedFiles = findOccurrencesOutsideFile(RenameDecl, MainFilePath,
                                                   Index, MaxLimitFiles);
@@ -709,10 +710,11 @@ renameOutsideFile(const NamedDecl &RenameDecl, llvm::StringRef MainFilePath,
     }
 
     auto AffectedFileCode = (*ExpBuffer)->getBuffer();
-    auto RenameRanges =
-        adjustRenameRanges(AffectedFileCode, RenameDecl.getNameAsString(),
-                           std::move(FileAndOccurrences.second),
-                           RenameDecl.getASTContext().getLangOpts());
+    syntax::UnexpandedTokenBuffer Tokens(
+        AffectedFileCode, RenameDecl.getASTContext().getLangOpts());
+    std::optional<std::vector<Range>> RenameRanges =
+        adjustRenameRanges(Tokens, OldName.getNamePieces().front(),
+                           std::move(FileAndOccurrences.second));
     if (!RenameRanges) {
       // Our heuristics fails to adjust rename ranges to the current state of
       // the file, it is most likely the index is stale, so we give up the
@@ -721,8 +723,8 @@ renameOutsideFile(const NamedDecl &RenameDecl, llvm::StringRef MainFilePath,
                    "(the index may be stale)",
                    FilePath);
     }
-    auto RenameEdit =
-        buildRenameEdit(FilePath, AffectedFileCode, *RenameRanges, NewName);
+    auto RenameEdit = buildRenameEdit(FilePath, AffectedFileCode, *RenameRanges,
+                                      OldName, NewName, Tokens);
     if (!RenameEdit)
       return error("failed to rename in file {0}: {1}", FilePath,
                    RenameEdit.takeError());
@@ -876,7 +878,8 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
   }
 
   auto OtherFilesEdits = renameOutsideFile(
-      RenameDecl, RInputs.MainFilePath, RInputs.NewName, *RInputs.Index,
+      RenameDecl, RInputs.MainFilePath, OldSymbolName, NewSymbolName,
+      *RInputs.Index,
       Opts.LimitFiles == 0 ? std::numeric_limits<size_t>::max()
                            : Opts.LimitFiles,
       *RInputs.FS);
@@ -889,10 +892,11 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
   return Result;
 }
 
-llvm::Expected<Edit> buildRenameEdit(llvm::StringRef AbsFilePath,
-                                     llvm::StringRef InitialCode,
-                                     std::vector<Range> Occurrences,
-                                     llvm::StringRef NewName) {
+llvm::Expected<Edit>
+buildRenameEdit(llvm::StringRef AbsFilePath, llvm::StringRef InitialCode,
+                std::vector<Range> Occurrences, SymbolName OldName,
+                SymbolName NewName,
+                const syntax::UnexpandedTokenBuffer &Tokens) {
   trace::Span Tracer("BuildRenameEdit");
   SPAN_ATTACH(Tracer, "file_path", AbsFilePath);
   SPAN_ATTACH(Tracer, "rename_occurrences",
@@ -933,12 +937,34 @@ llvm::Expected<Edit> buildRenameEdit(llvm::StringRef AbsFilePath,
     OccurrencesOffsets.push_back({*StartOffset, *EndOffset});
   }
 
+  const SourceManager &SM = Tokens.sourceManager();
+
   tooling::Replacements RenameEdit;
   for (const auto &R : OccurrencesOffsets) {
-    auto ByteLength = R.second - R.first;
-    if (auto Err = RenameEdit.add(
-            tooling::Replacement(AbsFilePath, R.first, ByteLength, NewName)))
-      return std::move(Err);
+    if (std::optional<std::string> Identifier = NewName.getSinglePiece()) {
+      auto ByteLength = R.second - R.first;
+      if (auto Err = RenameEdit.add(tooling::Replacement(
+              AbsFilePath, R.first, ByteLength, *Identifier)))
+        return std::move(Err);
+    } else {
+      llvm::Expected<SmallVector<SourceLocation>> PieceLocations =
+          findObjCSymbolSelectorPieces(
+              Tokens.tokens(), SM,
+              SM.getLocForStartOfFile(SM.getMainFileID())
+                  .getLocWithOffset(R.first),
+              OldName, tooling::ObjCSymbolSelectorKind::Unknown);
+      if (!PieceLocations) {
+        return PieceLocations.takeError();
+      }
+      assert(PieceLocations->size() == NewName.getNamePieces().size());
+      for (auto [Location, NewPiece] :
+           llvm::zip_equal(*PieceLocations, NewName.getNamePieces())) {
+        tooling::Replacement NewReplacement(
+            SM, CharSourceRange::getTokenRange(Location), NewPiece);
+        if (auto Err = RenameEdit.add(NewReplacement))
+          return std::move(Err);
+      }
+    }
   }
   return Edit(InitialCode, std::move(RenameEdit));
 }
@@ -957,13 +983,13 @@ llvm::Expected<Edit> buildRenameEdit(llvm::StringRef AbsFilePath,
 //          were inserted). If such a "near miss" is found, the rename is still
 //          possible
 std::optional<std::vector<Range>>
-adjustRenameRanges(llvm::StringRef DraftCode, llvm::StringRef Identifier,
-                   std::vector<Range> Indexed, const LangOptions &LangOpts) {
+adjustRenameRanges(const syntax::UnexpandedTokenBuffer &Tokens,
+                   llvm::StringRef Identifier, std::vector<Range> Indexed) {
   trace::Span Tracer("AdjustRenameRanges");
   assert(!Indexed.empty());
   assert(llvm::is_sorted(Indexed));
-  std::vector<Range> Lexed =
-      collectIdentifierRanges(Identifier, DraftCode, LangOpts);
+
+  std::vector<Range> Lexed = collectIdentifierRanges(Identifier, Tokens);
   llvm::sort(Lexed);
   return getMappedRanges(Indexed, Lexed);
 }
diff --git a/clang-tools-extra/clangd/refactor/Rename.h b/clang-tools-extra/clangd/refactor/Rename.h
index 91728ba59e5d8..f14f20a6e1759 100644
--- a/clang-tools-extra/clangd/refactor/Rename.h
+++ b/clang-tools-extra/clangd/refactor/Rename.h
@@ -12,6 +12,7 @@
 #include "Protocol.h"
 #include "SourceCode.h"
 #include "clang/Basic/LangOptions.h"
+#include "clang/Tooling/Refactoring/Rename/SymbolName.h"
 #include "llvm/Support/Error.h"
 #include <optional>
 
@@ -66,13 +67,19 @@ struct RenameResult {
 llvm::Expected<RenameResult> rename(const RenameInputs &RInputs);
 
 /// Generates rename edits that replaces all given occurrences with the
-/// NewName.
+/// `NewName`.
+///
+/// `OldName` is and `Tokens` are used to to find the argument labels of
+/// Objective-C selectors.
+///
 /// Exposed for testing only.
+///
 /// REQUIRED: Occurrences is sorted and doesn't have duplicated ranges.
-llvm::Expected<Edit> buildRenameEdit(llvm::StringRef AbsFilePath,
-                                     llvm::StringRef InitialCode,
-                                     std::vector<Range> Occurrences,
-                                     llvm::StringRef NewName);
+llvm::Expected<Edit>
+buildRenameEdit(llvm::StringRef AbsFilePath, llvm::StringRef InitialCode,
+                std::vector<Range> Occurrences, tooling::SymbolName OldName,
+                tooling::SymbolName NewName,
+                const syntax::UnexpandedTokenBuffer &Tokens);
 
 /// Adjusts indexed occurrences to match the current state of the file.
 ///
@@ -85,8 +92,8 @@ llvm::Expected<Edit> buildRenameEdit(llvm::StringRef AbsFilePath,
 /// occurrence has the same length).
 /// REQUIRED: Indexed is sorted.
 std::optional<std::vector<Range>>
-adjustRenameRanges(llvm::StringRef DraftCode, llvm::StringRef Identifier,
-                   std::vector<Range> Indexed, const LangOptions &LangOpts);
+adjustRenameRanges(const syntax::UnexpandedTokenBuffer &Tokens,
+                   llvm::StringRef Identifier, std::vector<Range> Indexed);
 
 /// Calculates the lexed occurrences that the given indexed occurrences map to.
 /// Returns std::nullopt if we don't find a mapping.
diff --git a/clang-tools-extra/clangd/unittests/RenameTests.cpp b/clang-tools-extra/clangd/unittests/RenameTests.cpp
index ded636b0562bc..008639d573da9 100644
--- a/clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ b/clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -878,33 +878,130 @@ TEST(RenameTest, WithinFileRename) {
 
 TEST(RenameTest, ObjCWithinFileRename) {
   struct TestCase {
+    /// Annotated source code that should be renamed. Every point (indicated by
+    /// `^`) will be used as a rename location.
     llvm::StringRef Input;
+    /// The new name that should be given to the rename locaitons.
     llvm::StringRef NewName;
-    llvm::StringRef Expected;
+    /// The expected rename source code or `nullopt` if we expect rename to
+    /// fail.
+    std::optional<llvm::StringRef> Expected;
   };
-  TestCase Tests[] = {{
+  TestCase Tests[] = {
+    // Simple rename
+    {
+      // Input
       R"cpp(
         @interface Foo
         - (int)performA^ction:(int)action w^ith:(int)value;
         @end
         @implementation Foo
         - (int)performAc^tion:(int)action w^ith:(int)value {
-          [self performAction:action with:value];
+          return [self performAction:action with:value];
         }
         @end
       )cpp",
+      // New name
       "performNewAction:by:",
+      // Expected
       R"cpp(
         @interface Foo
         - (int)performNewAction:(int)action by:(int)value;
         @end
         @implementation Foo
         - (int)performNewAction:(int)action by:(int)value {
-          [self performNewAction:action by:value];
+          return [self performNewAction:action by:value];
+        }
+        @end
+      )cpp",
+    },
+    // Rename selector with macro
+    {
+      // Input
+      R"cpp(
+        #define mySelector - (int)performAction:(int)action with:(int)value
+        @interface Foo
+        ^mySelector;
+        @end
+        @implementation Foo
+        mySelector {
+          return [self performAction:action with:value];
+        }
+        @end
+      )cpp",
+      // New name
+      "performNewAction:by:",
+      // Expected error
+      std::nullopt,
+    },
+    // Rename selector in macro definition
+    {
+      // Input
+      R"cpp(
+        #define mySelector - (int)perform^Action:(int)action with:(int)value
+        @interface Foo
+        mySelector;
+        @end
+        @implementation Foo
+        mySelector {
+          return [self performAction:action with:value];
+        }
+        @end
+      )cpp",
+      // New name
+      "performNewAction:by:",
+      // Expected error
+      std::nullopt,
+    },
+    // Don't rename `@selector`
+    // `@selector` is not tied to a single selector. Eg. there might be multiple
+    // classes in the codebase that implement that selector. It's thus more like
+    // a string literal and we shouldn't rename it.
+    {
+      // Input
+      R"cpp(
+        @interface Foo
+        - (void)performA^ction:(int)action with:(int)value;
+        @end
+        @implementation Foo
+        - (void)performAction:(int)action with:(int)value {
+          SEL mySelector = @selector(performAction:with:);
+        }
+        @end
+      )cpp",
+      // New name
+      "performNewAction:by:",
+      // Expected
+      R"cpp(
+        @interface Foo
+        - (void)performNewAction:(int)action by:(int)value;
+        @end
+        @implementation Foo
+        - (void)performNewAction:(int)action by:(int)value {
+          SEL mySelector = @selector(performAction:with:);
         }
         @end
       )cpp",
-  }};
+    },
+    // Fail if rename initiated inside @selector
+    {
+      // Input
+      R"cpp(
+        @interface Foo
+        - (void)performAction:(int)action with:(int)value;
+        @end
+        @implementation Foo
+        - (void)performAction:(int)action with:(int)value {
+          SEL mySelector = @selector(perfo^rmAction:with:);
+        }
+        @end
+      )cpp",
+      // New name
+      "performNewAction:by:",
+      // Expected
+      std::nullopt,
+    }
+  };
   for (TestCase T : Tests) {
     SCOPED_TRACE(T.Input);
     Annotations Code(T.Input);
@@ -916,11 +1013,16 @@ TEST(RenameTest, ObjCWithinFileRename) {
       auto RenameResult =
           rename({RenamePos, T.NewName, AST, testPath(TU.Filename),
                   getVFSFromAST(AST), Index.get()});
-      ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
-      ASSERT_EQ(1u, RenameResult->GlobalChanges.size());
-      EXPECT_EQ(
-          applyEdits(std::move(RenameResult->GlobalChanges)).front().second,
-          T.Expected);
+      if (std::optional<StringRef> Expected = T.Expected) {
+        ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
+        ASSERT_EQ(1u, RenameResult->GlobalChanges.size());
+        EXPECT_EQ(
+                  applyEdits(std::move(RenameResult->GlobalChanges)).front().second,
+                  *Expected);
+      } else {
+        ASSERT_FALSE(bool(RenameResult));
+        consumeError(RenameResult.takeError());
+      }
     }
   }
 }
@@ -1186,7 +1288,7 @@ TEST(RenameTest, Renameable) {
           int [[V^ar]];
         }
       )cpp",
-        nullptr, !HeaderFile},
+       nullptr, !HeaderFile},
   };
 
   for (const auto& Case : Cases) {
@@ -1808,6 +1910,147 @@ TEST(CrossFileRenameTests, WithUpToDateIndex) {
   }
 }
 
+TEST(CrossFileRenameTests, ObjC) {
+  MockCompilationDatabase CDB;
+  CDB.ExtraClangFlags = {"-xobjective-c"};
+  // rename is runnning on all "^" points in FooH, and "[[]]" ranges are the
+  // expected rename occurrences.
+  struct Case {
+    llvm::StringRef FooH;
+    llvm::StringRef FooM;
+    llvm::StringRef NewName;
+    llvm::StringRef ExpectedFooH;
+    llvm::StringRef ExpectedFooM;
+  };
+  Case Cases[] = {
+    // --- Zero arg selector
+    {
+      // Input
+      R"cpp(
+        @interface Foo
+        - (int)performA^ction;
+        @end
+      )cpp",
+      R"cpp(
+        @implementation Foo
+        - (int)performAction {
+          [self performAction];
+        }
+        @end
+      )cpp",
+      // New name
+      "performNewAction",
+      // Expected
+      R"cpp(
+        @interface Foo
+        - (int)performNewAction;
+        @end
+      )cpp",
+      R"cpp(
+        @implementation Foo
+        - (int)performNewAction {
+          [self performNewAction];
+        }
+        @end
+      )cpp",
+    },
+    // --- Single arg selector
+    {
+      // Input
+      R"cpp(
+        @interface Foo
+        - (int)performA^ction:(int)action;
+        @end
+      )cpp",
+      R"cpp(
+        @implementation Foo
+        - (int)performAction:(int)action {
+          [self performAction:action];
+        }
+        @end
+      )cpp",
+      // New name
+      "performNewAction:",
+      // Expected
+      R"cpp(
+        @interface Foo
+        - (int)performNewAction:(int)action;
+        @end
+      )cpp",
+      R"cpp(
+        @implementation Foo
+        - (int)performNewAction:(int)action {
+          [self performNewAction:action];
+        }
+        @end
+      )cpp",
+    },
+    // --- Multi arg selector
+    {
+      // Input
+      R"cpp(
+        @interface Foo
+        - (int)performA^ction:(int)action with:(int)value;
+        @end
+      )cpp",
+      R"cpp(
+        @implementation Foo
+        - (int)performAction:(int)action with:(int)value {
+          [self performAction:action with:value];
+        }
+        @end
+      )cpp",
+      // New name
+      "performNewAction:by:",
+      // Expected
+      R"cpp(
+        @interface Foo
+        - (int)performNewAction:(int)action by:(int)value;
+        @end
+      )cpp",
+      R"cpp(
+        @implementation Foo
+        - (int)performNewAction:(int)action by:(int)value {
+          [self performNewAction:action by:value];
+        }
+        @end
+      )cpp",
+    }
+  };
+
+  trace::TestTracer Tracer;
+  for (const auto &T : Cases) {
+    SCOPED_TRACE(T.FooH);
+    Annotations FooH(T.FooH);
+    Annotations FooM(T.FooM);
+    std::string FooHPath = testPath("foo.h");
+    std::string FooMPath = testPath("foo.m");
+
+    MockFS FS;
+    FS.Files[FooHPath] = std::string(FooH.code());
+    FS.Files[FooMPath] = std::string(FooM.code());
+
+    auto ServerOpts = ClangdServer::optsForTest();
+    ServerOpts.BuildDynamicSymbolIndex = true;
+    ClangdServer Server(CDB, FS, ServerOpts);
+
+    // Add all files to clangd server to make sure the dynamic index has been
+    // built.
+    runAddDocument(Server, FooHPath, FooH.code());
+    runAddDocument(Server, FooMPath, FooM.code());
+
+    for (const auto &RenamePos : FooH.points()) {
+      EXPECT_THAT(Tracer.takeMetric("rename_files"), SizeIs(0));
+      auto FileEditsList =
+          llvm::cantFail(runRename(Server, FooHPath, RenamePos, T.NewName, {}));
+      EXPECT_THAT(Tracer.takeMetric("rename_files"), ElementsAre(2));
+      EXPECT_THAT(applyEdits(std::move(FileEditsList.GlobalChanges)),
+                  UnorderedElementsAre(Pair(Eq(FooHPath), Eq(T.ExpectedFooH)),
+                                       Pair(Eq(FooMPath), Eq(T.ExpectedFooM))));
+    }
+  }
+}
+
 TEST(CrossFileRenameTests, CrossFileOnLocalSymbol) {
   // cross-file rename should work for function-local symbols, even there is no
   // index provided.
@@ -1828,7 +2071,13 @@ TEST(CrossFileRenameTests, BuildRenameEdits) {
   auto LSPRange = Code.range();
   llvm::StringRef FilePath = "/test/TestTU.cpp";
   llvm::StringRef NewName = "abc";
-  auto Edit = buildRenameEdit(FilePath, Code.code(), {LSPRange}, NewName);
+  tooling::SymbolName NewSymbolName(NewName, /*IsObjectiveCSelector=*/false);
+  tooling::SymbolName OldSymbolName("😂", /*IsObjectiveCSelector=*/false);
+
+  syntax::UnexpandedTokenBuffer Tokens(Code.code(), LangOptions());
+
+  auto Edit = buildRenameEdit(FilePath, Code.code(), {LSPRange}, OldSymbolName,
+                              NewSymbolName, Tokens);
   ASSERT_TRUE(bool(Edit)) << Edit.takeError();
   ASSERT_EQ(1UL, Edit->Replacements.size());
   EXPECT_EQ(FilePath, Edit->Replacements.begin()->getFilePath());
@@ -1836,7 +2085,8 @@ TEST(CrossFileRenameTests, BuildRenameEdits) {
 
   // Test invalid range.
   LSPRange.end = {10, 0}; // out of range
-  Edit = buildRenameEdit(FilePath, Code.code(), {LSPRange}, NewName);
+  Edit = buildRenameEdit(FilePath, Code.code(), {LSPRange}, OldSymbolName,
+                         NewSymbolName, Tokens);
   EXPECT_FALSE(Edit);
   EXPECT_THAT(llvm::toString(Edit.takeError()),
               testing::HasSubstr("fail to convert"));
@@ -1847,7 +2097,8 @@ TEST(CrossFileRenameTests, BuildRenameEdits) {
               [[range]]
       [[range]]
   )cpp");
-  Edit = buildRenameEdit(FilePath, T.code(), T.ranges(), NewName);
+  Edit = buildRenameEdit(FilePath, T.code(), T.ranges(), OldSymbolName,
+                         NewSymbolName, Tokens);
   ASSERT_TRUE(bool(Edit)) << Edit.takeError();
   EXPECT_EQ(applyEdits(FileEdits{{T.code(), std::move(*Edit)}}).front().second,
             expectedResult(T, NewName));
@@ -1904,8 +2155,9 @@ TEST(CrossFileRenameTests, adjustRenameRanges) {
   for (const auto &T : Tests) {
     SCOPED_TRACE(T.DraftCode);
     Annotations Draft(T.DraftCode);
-    auto ActualRanges = adjustRenameRanges(
-        Draft.code(), "x", Annotations(T.IndexedCode).ranges(), LangOpts);
+    syntax::UnexpandedTokenBuffer Tokens(Draft.code(), LangOpts);
+    auto ActualRanges =
+        adjustRenameRanges(Tokens, "x", Annotations(T.IndexedCode).ranges());
     if (!ActualRanges)
        EXPECT_THAT(Draft.ranges(), testing::IsEmpty());
     else
diff --git a/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp b/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
index 08abde87df6d4..e22bd817f9a42 100644
--- a/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
+++ b/clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
@@ -772,8 +772,8 @@ o foo2;
   )cpp");
   LangOptions LangOpts;
   LangOpts.CPlusPlus = true;
-  EXPECT_EQ(Code.ranges(),
-            collectIdentifierRanges("Foo", Code.code(), LangOpts));
+  syntax::UnexpandedTokenBuffer Tokens(Code.code(), LangOpts);
+  EXPECT_EQ(Code.ranges(), collectIdentifierRanges("Foo", Tokens));
 }
 
 TEST(SourceCodeTests, isHeaderFile) {
diff --git a/clang/include/clang/Tooling/Syntax/Tokens.h b/clang/include/clang/Tooling/Syntax/Tokens.h
index b1bdefed7c97f..8e057687d2e6a 100644
--- a/clang/include/clang/Tooling/Syntax/Tokens.h
+++ b/clang/include/clang/Tooling/Syntax/Tokens.h
@@ -145,6 +145,20 @@ class Token {
 /// For debugging purposes. Equivalent to a call to Token::str().
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Token &T);
 
+/// A list of tokens as lexed from the input file, without expanding
+/// preprocessor macros.
+class UnexpandedTokenBuffer {
+  std::string Code;
+  std::vector<syntax::Token> Tokens;
+  std::unique_ptr<SourceManagerForFile> SrcMgr;
+
+public:
+  UnexpandedTokenBuffer(StringRef Code, const LangOptions &LangOpts);
+
+  ArrayRef<syntax::Token> tokens() const { return Tokens; }
+  const SourceManager &sourceManager() const { return SrcMgr->get(); }
+};
+
 /// A list of tokens obtained by preprocessing a text buffer and operations to
 /// map between the expanded and spelled tokens, i.e. TokenBuffer has
 /// information about two token streams:
diff --git a/clang/lib/Tooling/Refactoring/SymbolName.cpp b/clang/lib/Tooling/Refactoring/SymbolName.cpp
index be0dd721b1d22..b136e8a59bd23 100644
--- a/clang/lib/Tooling/Refactoring/SymbolName.cpp
+++ b/clang/lib/Tooling/Refactoring/SymbolName.cpp
@@ -16,7 +16,9 @@ namespace tooling {
 SymbolName::SymbolName(const DeclarationName &DeclName)
     : SymbolName(DeclName.getAsString(),
                  /*IsObjectiveCSelector=*/DeclName.getNameKind() ==
-                     DeclarationName::NameKind::ObjCMultiArgSelector) {}
+                         DeclarationName::NameKind::ObjCMultiArgSelector ||
+                     DeclName.getNameKind() ==
+                         DeclarationName::NameKind::ObjCOneArgSelector) {}
 
 SymbolName::SymbolName(StringRef Name, const LangOptions &LangOpts)
     : SymbolName(Name, LangOpts.ObjC) {}
diff --git a/clang/lib/Tooling/Syntax/Tokens.cpp b/clang/lib/Tooling/Syntax/Tokens.cpp
index 8d32c45a4a70c..c85edf82518a7 100644
--- a/clang/lib/Tooling/Syntax/Tokens.cpp
+++ b/clang/lib/Tooling/Syntax/Tokens.cpp
@@ -225,6 +225,17 @@ llvm::StringRef FileRange::text(const SourceManager &SM) const {
   return Text.substr(Begin, length());
 }
 
+UnexpandedTokenBuffer::UnexpandedTokenBuffer(StringRef Code,
+                                             const LangOptions &LangOpts) {
+  // InMemoryFileAdapter crashes unless the buffer is null terminated, so ensure
+  // the string is null-terminated.
+  this->Code = Code.str();
+  SrcMgr =
+      std::make_unique<SourceManagerForFile>("mock_file_name.cpp", this->Code);
+  Tokens = syntax::tokenize(sourceManager().getMainFileID(), sourceManager(),
+                            LangOpts);
+}
+
 void TokenBuffer::indexExpandedTokens() {
   // No-op if the index is already created.
   if (!ExpandedTokIndex.empty())

>From 8ce6dd84fbae77420ee0fbd2ee27840b35aabfae Mon Sep 17 00:00:00 2001
From: Alex Hoppen <ahoppen at apple.com>
Date: Wed, 20 Dec 2023 12:02:27 -0800
Subject: [PATCH 4/5] [clangd] Return a placeholder string for the
 `prepareRename` request

This allows us to return the old name of an Objective-C method, which pre-populates the new name field in an editor.
---
 clang-tools-extra/clangd/ClangdLSPServer.cpp  |  5 +--
 clang-tools-extra/clangd/ClangdLSPServer.h    |  2 +-
 clang-tools-extra/clangd/ClangdServer.cpp     |  3 +-
 clang-tools-extra/clangd/Protocol.cpp         |  4 +++
 clang-tools-extra/clangd/Protocol.h           | 11 ++++++
 clang-tools-extra/clangd/refactor/Rename.cpp  | 16 ++++++++-
 clang-tools-extra/clangd/refactor/Rename.h    | 18 ++++++----
 clang-tools-extra/clangd/test/rename.test     |  3 ++
 .../clangd/unittests/RenameTests.cpp          | 34 +++++++++++++++++++
 .../Tooling/Refactoring/Rename/SymbolName.h   |  1 +
 clang/lib/Tooling/Refactoring/SymbolName.cpp  |  5 +++
 11 files changed, 90 insertions(+), 12 deletions(-)

diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index a87da252b7a7e..5dfd12045b657 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -844,14 +844,15 @@ void ClangdLSPServer::onWorkspaceSymbol(
 }
 
 void ClangdLSPServer::onPrepareRename(const TextDocumentPositionParams &Params,
-                                      Callback<std::optional<Range>> Reply) {
+                                      Callback<PrepareRenameResult> Reply) {
   Server->prepareRename(
       Params.textDocument.uri.file(), Params.position, /*NewName*/ std::nullopt,
       Opts.Rename,
       [Reply = std::move(Reply)](llvm::Expected<RenameResult> Result) mutable {
         if (!Result)
           return Reply(Result.takeError());
-        return Reply(std::move(Result->Target));
+        PrepareRenameResult PrepareResult{Result->Target, Result->OldName};
+        return Reply(std::move(PrepareResult));
       });
 }
 
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.h b/clang-tools-extra/clangd/ClangdLSPServer.h
index 79579c22b788a..6a9f097d551ae 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.h
+++ b/clang-tools-extra/clangd/ClangdLSPServer.h
@@ -134,7 +134,7 @@ class ClangdLSPServer : private ClangdServer::Callbacks,
   void onWorkspaceSymbol(const WorkspaceSymbolParams &,
                          Callback<std::vector<SymbolInformation>>);
   void onPrepareRename(const TextDocumentPositionParams &,
-                       Callback<std::optional<Range>>);
+                       Callback<PrepareRenameResult>);
   void onRename(const RenameParams &, Callback<WorkspaceEdit>);
   void onHover(const TextDocumentPositionParams &,
                Callback<std::optional<Hover>>);
diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
index 6fb2641e8793d..b04ebc7049c66 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -578,8 +578,7 @@ void ClangdServer::prepareRename(PathRef File, Position Pos,
     // prepareRename is latency-sensitive: we don't query the index, as we
     // only need main-file references
     auto Results =
-        clangd::rename({Pos, NewName.value_or("__clangd_rename_placeholder"),
-                        InpAST->AST, File, /*FS=*/nullptr,
+        clangd::rename({Pos, NewName, InpAST->AST, File, /*FS=*/nullptr,
                         /*Index=*/nullptr, RenameOpts});
     if (!Results) {
       // LSP says to return null on failure, but that will result in a generic
diff --git a/clang-tools-extra/clangd/Protocol.cpp b/clang-tools-extra/clangd/Protocol.cpp
index a6370649f5ad1..0291e5d71d65c 100644
--- a/clang-tools-extra/clangd/Protocol.cpp
+++ b/clang-tools-extra/clangd/Protocol.cpp
@@ -905,6 +905,10 @@ llvm::json::Value toJSON(const DocumentSymbol &S) {
   return std::move(Result);
 }
 
+llvm::json::Value toJSON(const PrepareRenameResult &R) {
+  return llvm::json::Object{{"range", R.range}, {"placeholder", R.placeholder}};
+}
+
 llvm::json::Value toJSON(const WorkspaceEdit &WE) {
   llvm::json::Object Result;
   if (WE.changes) {
diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h
index e88c804692568..65366a7f1307b 100644
--- a/clang-tools-extra/clangd/Protocol.h
+++ b/clang-tools-extra/clangd/Protocol.h
@@ -1004,6 +1004,17 @@ struct CodeActionParams {
 };
 bool fromJSON(const llvm::json::Value &, CodeActionParams &, llvm::json::Path);
 
+struct PrepareRenameResult {
+  /// The range of the string to rename.
+  Range range;
+  /// A placeholder text of the string content to be renamed.
+  ///
+  /// This is useful to populate the rename field with an Objective-C selector
+  /// name (eg. `performAction:with:`) when renaming Objective-C methods.
+  std::string placeholder;
+};
+llvm::json::Value toJSON(const PrepareRenameResult &WE);
+
 /// The edit should either provide changes or documentChanges. If the client
 /// can handle versioned document edits and if documentChanges are present,
 /// the latter are preferred over changes.
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index c6af9c137751f..51a809927c670 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -811,7 +811,20 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
   if (!Name)
     return makeError(ReasonToReject::UnsupportedSymbol);
   SymbolName OldSymbolName(Name);
-  SymbolName NewSymbolName(RInputs.NewName, AST.getLangOpts());
+  SymbolName NewSymbolName(ArrayRef<StringRef>{});
+  if (std::optional<StringRef> NewName = RInputs.NewName) {
+    NewSymbolName = SymbolName(*NewName, AST.getLangOpts());
+  } else {
+    // If no new name is given, we are perfoming a pseudo rename for the
+    // prepareRename request to check if the rename is possible. Construct a
+    // new symbol name that has as many name pieces as the old name and is thus
+    // a valid new name.
+    std::vector<std::string> NewNamePieces;
+    for (StringRef Piece : OldSymbolName.getNamePieces()) {
+      NewNamePieces.push_back(Piece.str() + "__clangd_rename_placeholder");
+    }
+    NewSymbolName = SymbolName(NewNamePieces);
+  }
   if (OldSymbolName == NewSymbolName)
     return makeError(ReasonToReject::SameName);
   auto Invalid = checkName(RenameDecl, NewSymbolName);
@@ -858,6 +871,7 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
   }
   RenameResult Result;
   Result.Target = CurrentIdentifier;
+  Result.OldName = RenameDecl.getNameAsString();
   Edit MainFileEdits = Edit(MainFileCode, std::move(*MainFileRenameEdit));
   for (const TextEdit &TE : MainFileEdits.asTextEdits())
     Result.LocalChanges.push_back(TE.range);
diff --git a/clang-tools-extra/clangd/refactor/Rename.h b/clang-tools-extra/clangd/refactor/Rename.h
index f14f20a6e1759..b7ca7e885fc90 100644
--- a/clang-tools-extra/clangd/refactor/Rename.h
+++ b/clang-tools-extra/clangd/refactor/Rename.h
@@ -35,8 +35,12 @@ struct RenameOptions {
 };
 
 struct RenameInputs {
-  Position Pos; // the position triggering the rename
-  llvm::StringRef NewName;
+  /// The position triggering the rename
+  Position Pos;
+
+  /// The new name to give to the symbol or `nullopt` to perform a fake rename
+  /// that checks if rename is possible.
+  std::optional<llvm::StringRef> NewName;
 
   ParsedAST &AST;
   llvm::StringRef MainFilePath;
@@ -52,12 +56,14 @@ struct RenameInputs {
 };
 
 struct RenameResult {
-  // The range of the symbol that the user can attempt to rename.
+  /// The range of the symbol that the user can attempt to rename.
   Range Target;
-  // Rename occurrences for the current main file.
+  /// The current name of the declaration at the cursor.
+  std::string OldName;
+  /// Rename occurrences for the current main file.
   std::vector<Range> LocalChanges;
-  // Complete edits for the rename, including LocalChanges.
-  // If the full set of changes is unknown, this field is empty.
+  /// Complete edits for the rename, including LocalChanges.
+  /// If the full set of changes is unknown, this field is empty.
   FileEdits GlobalChanges;
 };
 
diff --git a/clang-tools-extra/clangd/test/rename.test b/clang-tools-extra/clangd/test/rename.test
index 527b4263443a7..0dc1a103002b2 100644
--- a/clang-tools-extra/clangd/test/rename.test
+++ b/clang-tools-extra/clangd/test/rename.test
@@ -10,6 +10,8 @@
 #      CHECK:  "id": 1,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": {
+# CHECK-NEXT:    "placeholder": "foo",
+# CHECK-NEXT:    "range": {
 # CHECK-NEXT:      "end": {
 # CHECK-NEXT:        "character": 7,
 # CHECK-NEXT:        "line": 0
@@ -18,6 +20,7 @@
 # CHECK-NEXT:        "character": 4,
 # CHECK-NEXT:        "line": 0
 # CHECK-NEXT:      }
+# CHECK-NEXT:    }
 # CHECK-NEXT:  }
 ---
 {"jsonrpc":"2.0","id":2,"method":"textDocument/prepareRename","params":{"textDocument":{"uri":"test:///foo.cpp"},"position":{"line":0,"character":2}}}
diff --git a/clang-tools-extra/clangd/unittests/RenameTests.cpp b/clang-tools-extra/clangd/unittests/RenameTests.cpp
index 008639d573da9..94e5917ea6bc5 100644
--- a/clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ b/clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -1476,6 +1476,7 @@ TEST(RenameTest, PrepareRename) {
                                   /*NewName=*/std::nullopt, {});
   // Verify that for multi-file rename, we only return main-file occurrences.
   ASSERT_TRUE(bool(Results)) << Results.takeError();
+  ASSERT_EQ(Results->OldName, "func");
   // We don't know the result is complete in prepareRename (passing a nullptr
   // index internally), so GlobalChanges should be empty.
   EXPECT_TRUE(Results->GlobalChanges.empty());
@@ -1507,6 +1508,39 @@ TEST(RenameTest, PrepareRename) {
   }
 }
 
+TEST(RenameTest, PrepareRenameObjC) {
+  Annotations Input(R"cpp(
+    @interface Foo
+    - (int)performA^ction:(int)action w^ith:(int)value;
+    @end
+    @implementation Foo
+    - (int)performA^ction:(int)action w^ith:(int)value {
+      return [self ^performAction^:action ^w^ith^:value];
+    }
+    @end
+  )cpp");
+  std::string Path = testPath("foo.m");
+  MockFS FS;
+  FS.Files[Path] = std::string(Input.code());
+
+  auto ServerOpts = ClangdServer::optsForTest();
+  ServerOpts.BuildDynamicSymbolIndex = true;
+
+  trace::TestTracer Tracer;
+  MockCompilationDatabase CDB;
+  CDB.ExtraClangFlags = {"-xobjective-c"};
+  ClangdServer Server(CDB, FS, ServerOpts);
+  runAddDocument(Server, Path, Input.code());
+
+  for (Position Point : Input.points()) {
+    auto Results = runPrepareRename(Server, Path, Point,
+                                    /*NewName=*/std::nullopt, {});
+    // Verify that for multi-file rename, we only return main-file occurrences.
+    ASSERT_TRUE(bool(Results)) << Results.takeError();
+    ASSERT_EQ(Results->OldName, "performAction:with:");
+  }
+}
+
 TEST(CrossFileRenameTests, DirtyBuffer) {
   Annotations FooCode("class [[Foo]] {};");
   std::string FooPath = testPath("foo.cc");
diff --git a/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h b/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h
index 31520ecabb31a..8f22cdbba8ee5 100644
--- a/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h
+++ b/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h
@@ -36,6 +36,7 @@ class SymbolName {
 public:
   /// Create a new \c SymbolName with the specified pieces.
   explicit SymbolName(ArrayRef<StringRef> NamePieces);
+  explicit SymbolName(ArrayRef<std::string> NamePieces);
 
   explicit SymbolName(const DeclarationName &Name);
 
diff --git a/clang/lib/Tooling/Refactoring/SymbolName.cpp b/clang/lib/Tooling/Refactoring/SymbolName.cpp
index b136e8a59bd23..fbfdc41f2c8b3 100644
--- a/clang/lib/Tooling/Refactoring/SymbolName.cpp
+++ b/clang/lib/Tooling/Refactoring/SymbolName.cpp
@@ -41,6 +41,11 @@ SymbolName::SymbolName(ArrayRef<StringRef> NamePieces) {
     this->NamePieces.push_back(Piece.str());
 }
 
+SymbolName::SymbolName(ArrayRef<std::string> NamePieces) {
+  for (const auto &Piece : NamePieces)
+    this->NamePieces.push_back(Piece);
+}
+
 std::optional<std::string> SymbolName::getSinglePiece() const {
   if (getNamePieces().size() == 1) {
     return NamePieces.front();

>From 868d2e0324d951a46c1cd20a90985d19c6d9aed7 Mon Sep 17 00:00:00 2001
From: Alex Hoppen <ahoppen at apple.com>
Date: Wed, 10 Jan 2024 14:37:07 -0800
Subject: [PATCH 5/5] [clangd] Address review comments to support rename of
 Objective-C selectors

---
 clang-tools-extra/clangd/refactor/Rename.cpp  | 102 +++++++++---------
 clang-tools-extra/clangd/refactor/Rename.h    |   2 +-
 .../clangd/unittests/RenameTests.cpp          |   4 +-
 .../Refactoring/Rename/RenamingAction.h       |   4 +-
 .../Tooling/Refactoring/Rename/SymbolName.h   |   2 +
 clang/include/clang/Tooling/Syntax/Tokens.h   |   1 -
 .../Refactoring/Rename/RenamingAction.cpp     |  20 ++--
 clang/lib/Tooling/Refactoring/SymbolName.cpp  |  11 +-
 clang/lib/Tooling/Syntax/Tokens.cpp           |   6 +-
 .../Tooling/RefactoringActionRulesTest.cpp    |   6 +-
 10 files changed, 77 insertions(+), 81 deletions(-)

diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index 51a809927c670..f639cbc9b4b12 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -526,23 +526,25 @@ std::optional<InvalidName> checkName(const NamedDecl &RenameDecl,
   static constexpr trace::Metric InvalidNameMetric(
       "rename_name_invalid", trace::Metric::Counter, "invalid_kind");
   auto &ASTCtx = RenameDecl.getASTContext();
+  auto Identifier = NewName.getSinglePiece();
+  if (!Identifier) {
+    return std::nullopt;
+  }
   std::optional<InvalidName> Result;
-  if (auto Identifier = NewName.getSinglePiece()) {
-    if (isKeyword(*Identifier, ASTCtx.getLangOpts()))
-      Result = InvalidName{InvalidName::Keywords, *Identifier};
-    else if (!mayBeValidIdentifier(*Identifier))
-      Result = InvalidName{InvalidName::BadIdentifier, *Identifier};
-    else {
-      // Name conflict detection.
-      // Function conflicts are subtle (overloading), so ignore them.
-      if (RenameDecl.getKind() != Decl::Function &&
-          RenameDecl.getKind() != Decl::CXXMethod) {
-        if (auto *Conflict =
-                lookupSiblingWithName(ASTCtx, RenameDecl, *Identifier))
-          Result = InvalidName{
-              InvalidName::Conflict,
-              Conflict->getLocation().printToString(ASTCtx.getSourceManager())};
-      }
+  if (isKeyword(*Identifier, ASTCtx.getLangOpts())) {
+    Result = InvalidName{InvalidName::Keywords, *Identifier};
+  } else if (!mayBeValidIdentifier(*Identifier)) {
+    Result = InvalidName{InvalidName::BadIdentifier, *Identifier};
+  } else {
+    // Name conflict detection.
+    // Function conflicts are subtle (overloading), so ignore them.
+    if (RenameDecl.getKind() != Decl::Function &&
+        RenameDecl.getKind() != Decl::CXXMethod) {
+      if (auto *Conflict =
+              lookupSiblingWithName(ASTCtx, RenameDecl, *Identifier))
+        Result = InvalidName{
+            InvalidName::Conflict,
+            Conflict->getLocation().printToString(ASTCtx.getSourceManager())};
     }
   }
   if (Result)
@@ -579,24 +581,26 @@ renameWithinFile(ParsedAST &AST, const NamedDecl &RenameDecl,
     if (std::optional<std::string> Identifier = NewName.getSinglePiece()) {
       tooling::Replacement NewReplacement(
           SM, CharSourceRange::getTokenRange(RenameLoc), *Identifier);
-      if (auto Err = FilteredChanges.add(NewReplacement))
+      if (auto Err = FilteredChanges.add(NewReplacement)) {
         return std::move(Err);
-    } else {
-      llvm::Expected<SmallVector<SourceLocation>> PieceLocations =
-          findObjCSymbolSelectorPieces(
-              AST.getTokens().expandedTokens(), SM, RenameLoc, OldName,
-              tooling::ObjCSymbolSelectorKind::Unknown);
-      if (!PieceLocations) {
-        return PieceLocations.takeError();
-      }
-      assert(PieceLocations->size() == NewName.getNamePieces().size());
-      for (auto [Location, NewPiece] :
-           llvm::zip_equal(*PieceLocations, NewName.getNamePieces())) {
-        tooling::Replacement NewReplacement(
-            SM, CharSourceRange::getTokenRange(Location), NewPiece);
-        if (auto Err = FilteredChanges.add(NewReplacement))
-          return std::move(Err);
       }
+      continue;
+    }
+    SmallVector<SourceLocation> PieceLocations;
+    llvm::Error Error = findObjCSymbolSelectorPieces(
+        AST.getTokens().expandedTokens(), SM, RenameLoc, OldName,
+        tooling::ObjCSymbolSelectorKind::Unknown, PieceLocations);
+    if (Error) {
+      // Ignore the error. We simply skip over all selectors that didn't match.
+      consumeError(std::move(Error));
+      continue;
+    }
+    for (auto [Location, NewPiece] :
+         llvm::zip_equal(PieceLocations, NewName.getNamePieces())) {
+      tooling::Replacement NewReplacement(
+          SM, CharSourceRange::getTokenRange(Location), NewPiece);
+      if (auto Err = FilteredChanges.add(NewReplacement))
+        return std::move(Err);
     }
   }
   return FilteredChanges;
@@ -811,18 +815,16 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
   if (!Name)
     return makeError(ReasonToReject::UnsupportedSymbol);
   SymbolName OldSymbolName(Name);
-  SymbolName NewSymbolName(ArrayRef<StringRef>{});
-  if (std::optional<StringRef> NewName = RInputs.NewName) {
-    NewSymbolName = SymbolName(*NewName, AST.getLangOpts());
+  SymbolName NewSymbolName;
+  if (RInputs.NewName) {
+    NewSymbolName = SymbolName(*RInputs.NewName, AST.getLangOpts());
   } else {
     // If no new name is given, we are perfoming a pseudo rename for the
     // prepareRename request to check if the rename is possible. Construct a
     // new symbol name that has as many name pieces as the old name and is thus
     // a valid new name.
-    std::vector<std::string> NewNamePieces;
-    for (StringRef Piece : OldSymbolName.getNamePieces()) {
-      NewNamePieces.push_back(Piece.str() + "__clangd_rename_placeholder");
-    }
+    std::vector<std::string> NewNamePieces = OldSymbolName.getNamePieces();
+    NewNamePieces[0] += "__clangd_rename_placeholder";
     NewSymbolName = SymbolName(NewNamePieces);
   }
   if (OldSymbolName == NewSymbolName)
@@ -961,18 +963,20 @@ buildRenameEdit(llvm::StringRef AbsFilePath, llvm::StringRef InitialCode,
               AbsFilePath, R.first, ByteLength, *Identifier)))
         return std::move(Err);
     } else {
-      llvm::Expected<SmallVector<SourceLocation>> PieceLocations =
-          findObjCSymbolSelectorPieces(
-              Tokens.tokens(), SM,
-              SM.getLocForStartOfFile(SM.getMainFileID())
-                  .getLocWithOffset(R.first),
-              OldName, tooling::ObjCSymbolSelectorKind::Unknown);
-      if (!PieceLocations) {
-        return PieceLocations.takeError();
+      SmallVector<SourceLocation> PieceLocations;
+      llvm::Error Error = findObjCSymbolSelectorPieces(
+          Tokens.tokens(), SM,
+          SM.getLocForStartOfFile(SM.getMainFileID()).getLocWithOffset(R.first),
+          OldName, tooling::ObjCSymbolSelectorKind::Unknown, PieceLocations);
+      if (Error) {
+        // Ignore the error. We simply skip over all selectors that didn't
+        // match.
+        consumeError(std::move(Error));
+        continue;
       }
-      assert(PieceLocations->size() == NewName.getNamePieces().size());
+      assert(PieceLocations.size() == NewName.getNamePieces().size());
       for (auto [Location, NewPiece] :
-           llvm::zip_equal(*PieceLocations, NewName.getNamePieces())) {
+           llvm::zip_equal(PieceLocations, NewName.getNamePieces())) {
         tooling::Replacement NewReplacement(
             SM, CharSourceRange::getTokenRange(Location), NewPiece);
         if (auto Err = RenameEdit.add(NewReplacement))
diff --git a/clang-tools-extra/clangd/refactor/Rename.h b/clang-tools-extra/clangd/refactor/Rename.h
index b7ca7e885fc90..5b43875bf56a3 100644
--- a/clang-tools-extra/clangd/refactor/Rename.h
+++ b/clang-tools-extra/clangd/refactor/Rename.h
@@ -75,7 +75,7 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs);
 /// Generates rename edits that replaces all given occurrences with the
 /// `NewName`.
 ///
-/// `OldName` is and `Tokens` are used to to find the argument labels of
+/// `OldName` and `Tokens` are used to to find the argument labels of
 /// Objective-C selectors.
 ///
 /// Exposed for testing only.
diff --git a/clang-tools-extra/clangd/unittests/RenameTests.cpp b/clang-tools-extra/clangd/unittests/RenameTests.cpp
index 94e5917ea6bc5..ff64e1764513e 100644
--- a/clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ b/clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -1535,7 +1535,6 @@ TEST(RenameTest, PrepareRenameObjC) {
   for (Position Point : Input.points()) {
     auto Results = runPrepareRename(Server, Path, Point,
                                     /*NewName=*/std::nullopt, {});
-    // Verify that for multi-file rename, we only return main-file occurrences.
     ASSERT_TRUE(bool(Results)) << Results.takeError();
     ASSERT_EQ(Results->OldName, "performAction:with:");
   }
@@ -1947,8 +1946,7 @@ TEST(CrossFileRenameTests, WithUpToDateIndex) {
 TEST(CrossFileRenameTests, ObjC) {
   MockCompilationDatabase CDB;
   CDB.ExtraClangFlags = {"-xobjective-c"};
-  // rename is runnning on all "^" points in FooH, and "[[]]" ranges are the
-  // expected rename occurrences.
+  // rename is runnning on all "^" points in FooH.
   struct Case {
     llvm::StringRef FooH;
     llvm::StringRef FooM;
diff --git a/clang/include/clang/Tooling/Refactoring/Rename/RenamingAction.h b/clang/include/clang/Tooling/Refactoring/Rename/RenamingAction.h
index 79c2b384b4eb0..e6b38c40c8182 100644
--- a/clang/include/clang/Tooling/Refactoring/Rename/RenamingAction.h
+++ b/clang/include/clang/Tooling/Refactoring/Rename/RenamingAction.h
@@ -132,10 +132,10 @@ enum class ObjCSymbolSelectorKind {
   Unknown
 };
 
-llvm::Expected<SmallVector<SourceLocation>> findObjCSymbolSelectorPieces(
+llvm::Error findObjCSymbolSelectorPieces(
     ArrayRef<syntax::Token> Tokens, const SourceManager &SrcMgr,
     SourceLocation RenameLoc, const SymbolName &OldName,
-    ObjCSymbolSelectorKind Kind);
+    ObjCSymbolSelectorKind Kind, SmallVectorImpl<SourceLocation> &Result);
 
 } // end namespace tooling
 } // end namespace clang
diff --git a/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h b/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h
index 8f22cdbba8ee5..887ab0929445d 100644
--- a/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h
+++ b/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h
@@ -34,6 +34,8 @@ class SymbolName {
   llvm::SmallVector<std::string, 1> NamePieces;
 
 public:
+  SymbolName();
+
   /// Create a new \c SymbolName with the specified pieces.
   explicit SymbolName(ArrayRef<StringRef> NamePieces);
   explicit SymbolName(ArrayRef<std::string> NamePieces);
diff --git a/clang/include/clang/Tooling/Syntax/Tokens.h b/clang/include/clang/Tooling/Syntax/Tokens.h
index 8e057687d2e6a..f765788d4e857 100644
--- a/clang/include/clang/Tooling/Syntax/Tokens.h
+++ b/clang/include/clang/Tooling/Syntax/Tokens.h
@@ -148,7 +148,6 @@ llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Token &T);
 /// A list of tokens as lexed from the input file, without expanding
 /// preprocessor macros.
 class UnexpandedTokenBuffer {
-  std::string Code;
   std::vector<syntax::Token> Tokens;
   std::unique_ptr<SourceManagerForFile> SrcMgr;
 
diff --git a/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp b/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
index b76cb06bc8aa9..20802d3a5f101 100644
--- a/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
+++ b/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
@@ -285,11 +285,12 @@ static bool isMatchingSelectorName(const syntax::Token &Tok,
          Next.kind() == tok::colon && Tok.text(SrcMgr) == NamePiece;
 }
 
-llvm::Expected<SmallVector<SourceLocation>>
-findObjCSymbolSelectorPieces(ArrayRef<syntax::Token> AllTokens,
-                             const SourceManager &SM, SourceLocation RenameLoc,
-                             const SymbolName &OldName,
-                             ObjCSymbolSelectorKind Kind) {
+Error findObjCSymbolSelectorPieces(ArrayRef<syntax::Token> AllTokens,
+                                   const SourceManager &SM,
+                                   SourceLocation RenameLoc,
+                                   const SymbolName &OldName,
+                                   ObjCSymbolSelectorKind Kind,
+                                   SmallVectorImpl<SourceLocation> &Result) {
   ArrayRef<syntax::Token> Tokens =
       AllTokens.drop_while([RenameLoc](syntax::Token Tok) -> bool {
         return Tok.location() != RenameLoc;
@@ -298,7 +299,6 @@ findObjCSymbolSelectorPieces(ArrayRef<syntax::Token> AllTokens,
   assert(OldName.getNamePieces()[0].empty() ||
          Tokens[0].text(SM) == OldName.getNamePieces()[0]);
   assert(OldName.getNamePieces().size() > 1);
-  SmallVector<SourceLocation> Result;
 
   Result.push_back(Tokens[0].location());
 
@@ -328,7 +328,7 @@ findObjCSymbolSelectorPieces(ArrayRef<syntax::Token> AllTokens,
       Result.push_back(Tok.location());
       // All the selector pieces have been found.
       if (Result.size() == OldName.getNamePieces().size())
-        return Result;
+        return Error::success();
     } else if (Tok.kind() == tok::r_square) {
       // Stop scanning at the end of the message send.
       // Also account for spurious ']' in blocks or lambdas.
@@ -337,11 +337,11 @@ findObjCSymbolSelectorPieces(ArrayRef<syntax::Token> AllTokens,
         break;
       if (SquareCount)
         --SquareCount;
-    } else if (Tok.kind() == tok::l_square)
+    } else if (Tok.kind() == tok::l_square) {
       ++SquareCount;
-    else if (Tok.kind() == tok::l_paren)
+    } else if (Tok.kind() == tok::l_paren) {
       ++ParenCount;
-    else if (Tok.kind() == tok::r_paren) {
+    } else if (Tok.kind() == tok::r_paren) {
       if (!ParenCount)
         break;
       --ParenCount;
diff --git a/clang/lib/Tooling/Refactoring/SymbolName.cpp b/clang/lib/Tooling/Refactoring/SymbolName.cpp
index fbfdc41f2c8b3..896a6cf09a3a9 100644
--- a/clang/lib/Tooling/Refactoring/SymbolName.cpp
+++ b/clang/lib/Tooling/Refactoring/SymbolName.cpp
@@ -13,6 +13,8 @@
 namespace clang {
 namespace tooling {
 
+SymbolName::SymbolName() : NamePieces({}) {}
+
 SymbolName::SymbolName(const DeclarationName &DeclName)
     : SymbolName(DeclName.getAsString(),
                  /*IsObjectiveCSelector=*/DeclName.getNameKind() ==
@@ -49,9 +51,8 @@ SymbolName::SymbolName(ArrayRef<std::string> NamePieces) {
 std::optional<std::string> SymbolName::getSinglePiece() const {
   if (getNamePieces().size() == 1) {
     return NamePieces.front();
-  } else {
-    return std::nullopt;
   }
+  return std::nullopt;
 }
 
 std::string SymbolName::getAsString() const {
@@ -62,11 +63,7 @@ std::string SymbolName::getAsString() const {
 }
 
 void SymbolName::print(raw_ostream &OS) const {
-  for (size_t I = 0, E = NamePieces.size(); I != E; ++I) {
-    if (I != 0)
-      OS << ':';
-    OS << NamePieces[I];
-  }
+  llvm::interleave(NamePieces, OS, ":");
 }
 
 } // end namespace tooling
diff --git a/clang/lib/Tooling/Syntax/Tokens.cpp b/clang/lib/Tooling/Syntax/Tokens.cpp
index c85edf82518a7..1f7a2cb4f499b 100644
--- a/clang/lib/Tooling/Syntax/Tokens.cpp
+++ b/clang/lib/Tooling/Syntax/Tokens.cpp
@@ -227,11 +227,7 @@ llvm::StringRef FileRange::text(const SourceManager &SM) const {
 
 UnexpandedTokenBuffer::UnexpandedTokenBuffer(StringRef Code,
                                              const LangOptions &LangOpts) {
-  // InMemoryFileAdapter crashes unless the buffer is null terminated, so ensure
-  // the string is null-terminated.
-  this->Code = Code.str();
-  SrcMgr =
-      std::make_unique<SourceManagerForFile>("mock_file_name.cpp", this->Code);
+  SrcMgr = std::make_unique<SourceManagerForFile>("mock_file_name.cpp", Code);
   Tokens = syntax::tokenize(sourceManager().getMainFileID(), sourceManager(),
                             LangOpts);
 }
diff --git a/clang/unittests/Tooling/RefactoringActionRulesTest.cpp b/clang/unittests/Tooling/RefactoringActionRulesTest.cpp
index cdd1faf0e38d4..c405ea02f90f6 100644
--- a/clang/unittests/Tooling/RefactoringActionRulesTest.cpp
+++ b/clang/unittests/Tooling/RefactoringActionRulesTest.cpp
@@ -211,9 +211,9 @@ TEST_F(RefactoringActionRulesTest, ReturnSymbolOccurrences) {
     Expected<SymbolOccurrences>
     findSymbolOccurrences(RefactoringRuleContext &) override {
       SymbolOccurrences Occurrences;
-      Occurrences.push_back(SymbolOccurrence(SymbolName("test"),
-                                             SymbolOccurrence::MatchingSymbol,
-                                             Selection.getBegin()));
+      Occurrences.push_back(SymbolOccurrence(
+          SymbolName("test", /*IsObjectiveCSelector=*/false),
+          SymbolOccurrence::MatchingSymbol, Selection.getBegin()));
       return std::move(Occurrences);
     }
   };



More information about the cfe-commits mailing list