[clangd] Use SymbolName to represent Objective-C selectors

This is a cleaner design than using identifier and an optional `Selector`. It also allows rename of Objective-C method names if no declaration is at hand and thus no `Selector` instance can be formed. For example, when finding the ranges to rename based on an index that’s not clangd’s built-in index.

Author: ahoppen

clang-tools-extra/clangd/refactor/Rename.cpp

@@ -40,6 +40,8 @@ namespace clang {
 namespace clangd {
 namespace {
 
+using tooling::SymbolName;
+
 std::optional<std::string> filePath(const SymbolLocation &Loc,
                                     llvm::StringRef HintFilePath) {
   if (!Loc)
@@ -591,11 +593,11 @@ bool isMatchingSelectorName(const syntax::Token &Cur, const syntax::Token &Next,
 // The search will terminate upon seeing Terminator or a ; at the top level.
 std::optional<SymbolRange>
 findAllSelectorPieces(llvm::ArrayRef<syntax::Token> Tokens,
-                      const SourceManager &SM, Selector Sel,
+                      const SourceManager &SM, const SymbolName &Name,
                       tok::TokenKind Terminator) {
   assert(!Tokens.empty());
 
-  unsigned NumArgs = Sel.getNumArgs();
+  unsigned NumArgs = Name.getNamePieces().size();
   llvm::SmallVector<tok::TokenKind, 8> Closes;
   std::vector<Range> SelectorPieces;
   for (unsigned Index = 0, Last = Tokens.size(); Index < Last - 1; ++Index) {
@@ -605,12 +607,12 @@ findAllSelectorPieces(llvm::ArrayRef<syntax::Token> Tokens,
       auto PieceCount = SelectorPieces.size();
       if (PieceCount < NumArgs &&
           isMatchingSelectorName(Tok, Tokens[Index + 1], SM,
-                                 Sel.getNameForSlot(PieceCount))) {
+                                 Name.getNamePieces()[PieceCount])) {
         // If 'foo:' instead of ':' (empty selector), we need to skip the ':'
         // token after the name. We don't currently properly support empty
         // selectors since we may lex them improperly due to ternary statements
         // as well as don't properly support storing their ranges for edits.
-        if (!Sel.getNameForSlot(PieceCount).empty())
+        if (!Name.getNamePieces()[PieceCount].empty())
           ++Index;
         SelectorPieces.push_back(
             halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
@@ -662,16 +664,17 @@ findAllSelectorPieces(llvm::ArrayRef<syntax::Token> Tokens,
 
 /// Collects all ranges of the given identifier/selector in the source code.
 ///
-/// If a selector is given, this does a full lex of the given source code in
-/// order to identify all selector fragments (e.g. in method exprs/decls) since
-/// they are non-contiguous.
-std::vector<SymbolRange> collectRenameIdentifierRanges(
-    llvm::StringRef Identifier, llvm::StringRef Content,
-    const LangOptions &LangOpts, std::optional<Selector> Selector) {
+/// If `Name` is an Objective-C symbol name, this does a full lex of the given
+/// source code in order to identify all selector fragments (e.g. in method
+/// exprs/decls) since they are non-contiguous.
+std::vector<SymbolRange>
+collectRenameIdentifierRanges(const tooling::SymbolName &Name,
+                              llvm::StringRef Content,
+                              const LangOptions &LangOpts) {
   std::vector<SymbolRange> Ranges;
-  if (!Selector) {
+  if (auto SinglePiece = Name.getSinglePiece()) {
     auto IdentifierRanges =
-        collectIdentifierRanges(Identifier, Content, LangOpts);
+        collectIdentifierRanges(*SinglePiece, Content, LangOpts);
     for (const auto &R : IdentifierRanges)
       Ranges.emplace_back(R);
     return Ranges;
@@ -685,7 +688,7 @@ std::vector<SymbolRange> collectRenameIdentifierRanges(
   // parsing a method declaration or definition which isn't at the top level or
   // similar looking expressions (e.g. an @selector() expression).
   llvm::SmallVector<tok::TokenKind, 8> Closes;
-  llvm::StringRef FirstSelPiece = Selector->getNameForSlot(0);
+  llvm::StringRef FirstSelPiece = Name.getNamePieces()[0];
 
   auto Tokens = syntax::tokenize(SM.getMainFileID(), SM, LangOpts);
   unsigned Last = Tokens.size() - 1;
@@ -717,7 +720,7 @@ std::vector<SymbolRange> collectRenameIdentifierRanges(
       // Check if we can find all the relevant selector peices starting from
       // this token
       auto SelectorRanges =
-          findAllSelectorPieces(ArrayRef(Tokens).slice(Index), SM, *Selector,
+          findAllSelectorPieces(ArrayRef(Tokens).slice(Index), SM, Name,
                                 Closes.empty() ? tok::l_brace : Closes.back());
       if (SelectorRanges)
         Ranges.emplace_back(std::move(*SelectorRanges));
@@ -764,7 +767,6 @@ renameObjCMethodWithinFile(ParsedAST &AST, const ObjCMethodDecl *MD,
                            std::vector<SourceLocation> SelectorOccurences) {
   const SourceManager &SM = AST.getSourceManager();
   auto Code = SM.getBufferData(SM.getMainFileID());
-  auto RenameIdentifier = MD->getSelector().getNameForSlot(0).str();
   llvm::SmallVector<llvm::StringRef, 8> NewNames;
   NewName.split(NewNames, ":");
 
@@ -774,7 +776,7 @@ renameObjCMethodWithinFile(ParsedAST &AST, const ObjCMethodDecl *MD,
     Ranges.push_back(tokenRangeForLoc(AST, Loc, SM, LangOpts));
   auto FilePath = AST.tuPath();
   auto RenameRanges = collectRenameIdentifierRanges(
-      RenameIdentifier, Code, LangOpts, MD->getSelector());
+      SymbolName(MD->getDeclName()), Code, LangOpts);
   auto RenameEdit = buildRenameEdit(FilePath, Code, RenameRanges, NewNames);
   if (!RenameEdit)
     return error("failed to rename in file {0}: {1}", FilePath,
@@ -926,22 +928,14 @@ renameOutsideFile(const NamedDecl &RenameDecl, llvm::StringRef MainFilePath,
            ExpBuffer.getError().message());
       continue;
     }
-    std::string RenameIdentifier = RenameDecl.getNameAsString();
-    std::optional<Selector> Selector = std::nullopt;
+    SymbolName RenameName(RenameDecl.getDeclName());
     llvm::SmallVector<llvm::StringRef, 8> NewNames;
-    if (const auto *MD = dyn_cast<ObjCMethodDecl>(&RenameDecl)) {
-      if (MD->getSelector().getNumArgs() > 1) {
-        RenameIdentifier = MD->getSelector().getNameForSlot(0).str();
-        Selector = MD->getSelector();
-      }
-    }
     NewName.split(NewNames, ":");
 
     auto AffectedFileCode = (*ExpBuffer)->getBuffer();
-    auto RenameRanges =
-        adjustRenameRanges(AffectedFileCode, RenameIdentifier,
-                           std::move(FileAndOccurrences.second),
-                           RenameDecl.getASTContext().getLangOpts(), Selector);
+    auto RenameRanges = adjustRenameRanges(
+        AffectedFileCode, RenameName, std::move(FileAndOccurrences.second),
+        RenameDecl.getASTContext().getLangOpts());
     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
@@ -1226,14 +1220,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<SymbolRange>>
-adjustRenameRanges(llvm::StringRef DraftCode, llvm::StringRef Identifier,
-                   std::vector<Range> Indexed, const LangOptions &LangOpts,
-                   std::optional<Selector> Selector) {
+adjustRenameRanges(llvm::StringRef DraftCode, const tooling::SymbolName &Name,
+                   std::vector<Range> Indexed, const LangOptions &LangOpts) {
   trace::Span Tracer("AdjustRenameRanges");
   assert(!Indexed.empty());
   assert(llvm::is_sorted(Indexed));
   std::vector<SymbolRange> Lexed =
-      collectRenameIdentifierRanges(Identifier, DraftCode, LangOpts, Selector);
+      collectRenameIdentifierRanges(Name, DraftCode, LangOpts);
   llvm::sort(Lexed);
   return getMappedRanges(Indexed, Lexed);
 }

clang-tools-extra/clangd/refactor/Rename.h

@@ -13,6 +13,7 @@
 #include "SourceCode.h"
 #include "clang/Basic/IdentifierTable.h"
 #include "clang/Basic/LangOptions.h"
+#include "clang/Tooling/Refactoring/Rename/SymbolName.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Error.h"
 #include <optional>
@@ -108,9 +109,8 @@ llvm::Expected<Edit> buildRenameEdit(llvm::StringRef AbsFilePath,
 /// occurrence has the same length).
 /// REQUIRED: Indexed is sorted.
 std::optional<std::vector<SymbolRange>>
-adjustRenameRanges(llvm::StringRef DraftCode, llvm::StringRef Identifier,
-                   std::vector<Range> Indexed, const LangOptions &LangOpts,
-                   std::optional<Selector> Selector);
+adjustRenameRanges(llvm::StringRef DraftCode, const tooling::SymbolName &Name,
+                   std::vector<Range> Indexed, const LangOptions &LangOpts);
 
 /// Calculates the lexed occurrences that the given indexed occurrences map to.
 /// Returns std::nullopt if we don't find a mapping.

clang-tools-extra/clangd/unittests/RenameTests.cpp

@@ -2040,9 +2040,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, std::nullopt);
+    auto ActualRanges = adjustRenameRanges(
+        Draft.code(), tooling::SymbolName("x", /*IsObjectiveCSelector=*/false),
+        Annotations(T.IndexedCode).ranges(), LangOpts);
     if (!ActualRanges)
        EXPECT_THAT(Draft.ranges(), testing::IsEmpty());
     else

clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h

@@ -9,12 +9,16 @@
 #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"
 #include "llvm/ADT/StringRef.h"
 
 namespace clang {
+
+class LangOptions;
+
 namespace tooling {
 
 /// A name of a symbol.
@@ -27,19 +31,45 @@ 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());
-  }
+  SymbolName();
+
+  /// 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);
 
-  ArrayRef<std::string> getNamePieces() const { return Name; }
+  /// 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);
 
-private:
-  llvm::SmallVector<std::string, 1> Name;
+  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

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
@@ -23,6 +24,7 @@ add_clang_library(clangToolingRefactoring
   clangLex
   clangRewrite
   clangToolingCore
+  clangToolingSyntax
 
   DEPENDS
   omp_gen

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

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

clang/lib/Tooling/Refactoring/SymbolName.cpp

@@ -0,0 +1,70 @@
+//===--- 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() : NamePieces({}) {}
+
+SymbolName::SymbolName(const DeclarationName &DeclName)
+    : SymbolName(DeclName.getAsString(),
+                 /*IsObjectiveCSelector=*/DeclName.getNameKind() ==
+                         DeclarationName::NameKind::ObjCMultiArgSelector ||
+                     DeclName.getNameKind() ==
+                         DeclarationName::NameKind::ObjCOneArgSelector) {}
+
+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());
+}
+
+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();
+  }
+  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 {
+  llvm::interleave(NamePieces, OS, ":");
+}
+
+} // end namespace tooling
+} // end namespace clang

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