[clangd] Address review comments to support rename of Objective-C selectors

Author: ahoppen

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

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.

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;

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

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

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;
 

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;

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

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

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