[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
Sat Jan 20 19:25:32 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 6c28d40f3679c2..077f315f657e77 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 d3077be8810aad..a4a80ce8344313 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 72598601d47d67..4965977d1f7aa4 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 c18f9290471fe4..43e48f24caa9ea 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 00000000000000..7fd50b2c4df7bd
--- /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 f4c4e520520d9e..2f79f6ce4f3ff2 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 11f9e4627af760..258c1a23d0fe26 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 9cbf59684fbc10..ded636b0562bc5 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 43a8d56e4e7176..79c2b384b4eb04 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 077f315f657e77..31520ecabb31a0 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 a4a80ce8344313..f78f64ea2ef649 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 4965977d1f7aa4..b76cb06bc8aa9b 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 7fd50b2c4df7bd..be0dd721b1d227 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 a21d84d5385b4a..092740be15f011 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 1cfc46eb1a52f6..f400986be797a8 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 835038423fdf37..0081a69357b02f 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 a1bb44c1761202..f60683fc4f21c3 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 bf838e53f2a21e..24bd3c426b3e86 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 258c1a23d0fe26..c6af9c137751f4 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 91728ba59e5d84..f14f20a6e17590 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 ded636b0562bc5..008639d573da9b 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 08abde87df6d4d..e22bd817f9a423 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 b1bdefed7c97f1..8e057687d2e6a0 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 be0dd721b1d227..b136e8a59bd23a 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 8d32c45a4a70cf..c85edf82518a7a 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 64fa99c9cd6fc8c4f5afb168508999ccad6381bf 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 a87da252b7a7e9..5dfd12045b6573 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 79579c22b788a9..6a9f097d551ae6 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 6fb2641e8793db..b04ebc7049c661 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 a6370649f5ad1c..0291e5d71d65c8 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 e88c804692568f..274c7fe4391062 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 usueful 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 c6af9c137751f4..51a809927c6701 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 f14f20a6e17590..b7ca7e885fc906 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 527b4263443a70..0dc1a103002b26 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 008639d573da9b..94e5917ea6bc5f 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 31520ecabb31a0..8f22cdbba8ee5d 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 b136e8a59bd23a..fbfdc41f2c8b37 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 0428ffc034f8d8a25b613913232b10f52a6b6c2a 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 51a809927c6701..f639cbc9b4b127 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 b7ca7e885fc906..5b43875bf56a3e 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 94e5917ea6bc5f..ff64e1764513e9 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 79c2b384b4eb04..e6b38c40c8182a 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 8f22cdbba8ee5d..887ab0929445dc 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 8e057687d2e6a0..f765788d4e857f 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 b76cb06bc8aa9b..20802d3a5f1013 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 fbfdc41f2c8b37..896a6cf09a3a98 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 c85edf82518a7a..1f7a2cb4f499b1 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 cdd1faf0e38d46..c405ea02f90f62 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