[clang-tools-extra] [clangd] Add support for renaming ObjC properties and implicit properties (PR #81775)
David Goldman via cfe-commits
cfe-commits at lists.llvm.org
Thu Mar 7 09:40:37 PST 2024
https://github.com/DavidGoldman updated https://github.com/llvm/llvm-project/pull/81775
>From b0a2fb25c25ecfb20bb3f0aab2d398ea80caeff2 Mon Sep 17 00:00:00 2001
From: David Goldman <davg at google.com>
Date: Fri, 26 Jan 2024 15:50:11 -0500
Subject: [PATCH 1/5] Add support for ObjC property renaming + implicit
property renaming
Objective-C properties can generate multiple decls:
- an ivar decl
- a getter method decl
- a setter method decl
Triggering the rename from any of those decls should trigger
a rename of the property and use the proper name for each one
according to the ObjC property naming conventions.
I've added similar support for implicit properties, which
is when a getter or setter like method is referred to via
the property syntax (self.method) without an explicit
property decl.
---
clang-tools-extra/clangd/FindTarget.cpp | 16 +
clang-tools-extra/clangd/refactor/Rename.cpp | 338 ++++++++++++++----
.../clangd/unittests/RenameTests.cpp | 90 +++++
.../unittests/SemanticHighlightingTests.cpp | 2 +-
4 files changed, 380 insertions(+), 66 deletions(-)
diff --git a/clang-tools-extra/clangd/FindTarget.cpp b/clang-tools-extra/clangd/FindTarget.cpp
index e702c6b3537a09..9def867011f696 100644
--- a/clang-tools-extra/clangd/FindTarget.cpp
+++ b/clang-tools-extra/clangd/FindTarget.cpp
@@ -740,6 +740,22 @@ llvm::SmallVector<ReferenceLoc> refInDecl(const Decl *D,
/*IsDecl=*/true,
{OIMD}});
}
+
+ void VisitObjCPropertyImplDecl(const ObjCPropertyImplDecl *OPID) {
+ // Skiped compiler synthesized property impl decls - they will always
+ // have an invalid loc.
+ if (OPID->getLocation().isInvalid())
+ return;
+ if (OPID->isIvarNameSpecified())
+ Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
+ OPID->getPropertyIvarDeclLoc(),
+ /*IsDecl=*/false,
+ {OPID->getPropertyIvarDecl()}});
+ Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
+ OPID->getLocation(),
+ /*IsDecl=*/false,
+ {OPID->getPropertyDecl()}});
+ }
};
Visitor V{Resolver};
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index 4e135801f6853d..b53c24b8331ddb 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -21,6 +21,7 @@
#include "clang/AST/DeclCXX.h"
#include "clang/AST/DeclObjC.h"
#include "clang/AST/DeclTemplate.h"
+#include "clang/AST/ExprObjC.h"
#include "clang/AST/ParentMapContext.h"
#include "clang/AST/Stmt.h"
#include "clang/Basic/CharInfo.h"
@@ -153,8 +154,111 @@ const NamedDecl *pickInterestingTarget(const NamedDecl *D) {
return D;
}
-llvm::DenseSet<const NamedDecl *> locateDeclAt(ParsedAST &AST,
- SourceLocation TokenStartLoc) {
+// Some AST nodes are synthesized by the compiler based on other nodes. e.g.
+// ObjC methods and ivars can be synthesized based on an Objective-C property.
+//
+// We perform this check outside of canonicalization since we need to know which
+// decl the user has actually triggered the rename on in order to remap all
+// derived decls properly, since the naming patterns can slightly differ for
+// every decl that the compiler synthesizes.
+const NamedDecl *findOriginDecl(const NamedDecl *D) {
+ if (const auto *MD = dyn_cast<ObjCMethodDecl>(D)) {
+ if (const auto *PD = MD->findPropertyDecl(/*CheckOverrides=*/false))
+ // FIXME(davg): We should only map to the protocol if the user hasn't
+ // explicitly given a setter/getter for the method - if they have we
+ // should either fail the rename or support basic 1 arg selector renames.
+ return canonicalRenameDecl(PD);
+ }
+ if (const auto *ID = dyn_cast<ObjCIvarDecl>(D)) {
+ for (const auto *PD : ID->getContainingInterface()->properties()) {
+ if (PD->getPropertyIvarDecl() == ID)
+ return canonicalRenameDecl(PD);
+ }
+ }
+ return D;
+}
+
+std::string propertySetterName(llvm::StringRef PropertyName) {
+ std::string Setter = PropertyName.str();
+ if (!Setter.empty())
+ Setter[0] = llvm::toUpper(Setter[0]);
+ return "set" + Setter + ":";
+}
+
+// Returns a non-empty string if valid.
+std::string setterToPropertyName(llvm::StringRef Setter) {
+ std::string Result;
+ if (!Setter.consume_front("set")) {
+ return Result;
+ }
+ Setter.consume_back(":"); // Optional.
+ Result = Setter.str();
+ if (!Result.empty())
+ Result[0] = llvm::toLower(Result[0]);
+ return Result;
+}
+
+llvm::DenseMap<const NamedDecl *, std::string>
+computeAllDeclsToNewName(const NamedDecl *Selected, llvm::StringRef NewName,
+ const NamedDecl *Origin) {
+ llvm::DenseMap<const NamedDecl *, std::string> DeclToName;
+ DeclToName[Selected] = NewName.str();
+
+ if (const auto *PD = dyn_cast<ObjCPropertyDecl>(Origin)) {
+ // Need to derive the property/getter name, setter name, and ivar name based
+ // on which Decl the user triggered the rename on and their single input.
+ std::string PropertyName;
+ std::string SetterName;
+ std::string IvarName;
+
+ if (isa<ObjCIvarDecl>(Selected)) {
+ IvarName = NewName.str();
+ NewName.consume_front("_");
+ PropertyName = NewName.str();
+ SetterName = propertySetterName(PropertyName);
+ } else if (isa<ObjCPropertyDecl>(Selected)) {
+ PropertyName = NewName.str();
+ IvarName = "_" + PropertyName;
+ SetterName = propertySetterName(PropertyName);
+ } else if (const auto *MD = dyn_cast<ObjCMethodDecl>(Selected)) {
+ if (MD->getReturnType()->isVoidType()) { // Setter selected.
+ SetterName = NewName.str();
+ PropertyName = setterToPropertyName(SetterName);
+ if (PropertyName.empty())
+ return DeclToName;
+ IvarName = "_" + PropertyName;
+ } else { // Getter selected.
+ PropertyName = NewName.str();
+ IvarName = "_" + PropertyName;
+ SetterName = propertySetterName(PropertyName);
+ }
+ } else {
+ return DeclToName;
+ }
+
+ DeclToName[PD] = PropertyName;
+ // We will only rename the getter/setter if the user didn't specify one
+ // explicitly in the property decl.
+ if (const auto *GD = PD->getGetterMethodDecl())
+ if (!PD->getGetterNameLoc().isValid())
+ DeclToName[GD] = PropertyName;
+ if (const auto *SD = PD->getSetterMethodDecl())
+ if (!PD->getSetterNameLoc().isValid())
+ DeclToName[SD] = SetterName;
+ // This is only visible in the impl, not the header. We only rename it if it
+ // follows the typical `foo` property => `_foo` ivar convention.
+ if (const auto *ID = PD->getPropertyIvarDecl())
+ if (ID->getNameAsString() == "_" + PD->getNameAsString())
+ DeclToName[ID] = IvarName;
+ }
+
+ return DeclToName;
+}
+
+llvm::DenseMap<const NamedDecl *, std::string>
+locateDeclAt(ParsedAST &AST, SourceLocation TokenStartLoc,
+ llvm::StringRef NewName) {
+ llvm::DenseMap<const NamedDecl *, std::string> Result;
unsigned Offset =
AST.getSourceManager().getDecomposedSpellingLoc(TokenStartLoc).second;
@@ -162,20 +266,34 @@ llvm::DenseSet<const NamedDecl *> locateDeclAt(ParsedAST &AST,
AST.getASTContext(), AST.getTokens(), Offset, Offset);
const SelectionTree::Node *SelectedNode = Selection.commonAncestor();
if (!SelectedNode)
- return {};
+ return Result;
+
+ std::string SetterName;
+ const NamedDecl *Setter;
+ if (const auto *D = SelectedNode->ASTNode.get<ObjCPropertyRefExpr>()) {
+ if (D->isImplicitProperty() && D->isMessagingSetter()) {
+ SetterName = propertySetterName(NewName);
+ Setter = canonicalRenameDecl(D->getImplicitPropertySetter());
+ }
+ }
- llvm::DenseSet<const NamedDecl *> Result;
for (const NamedDecl *D :
targetDecl(SelectedNode->ASTNode,
DeclRelation::Alias | DeclRelation::TemplatePattern,
AST.getHeuristicResolver())) {
D = pickInterestingTarget(D);
- Result.insert(canonicalRenameDecl(D));
+ D = canonicalRenameDecl(D);
+ if (D == Setter) {
+ Result[D] = SetterName;
+ continue;
+ }
+ Result[D] = NewName.str();
}
return Result;
}
-void filterRenameTargets(llvm::DenseSet<const NamedDecl *> &Decls) {
+void filterRenameTargets(
+ llvm::DenseMap<const NamedDecl *, std::string> &Decls) {
// For something like
// namespace ns { void foo(); }
// void bar() { using ns::f^oo; foo(); }
@@ -183,8 +301,8 @@ void filterRenameTargets(llvm::DenseSet<const NamedDecl *> &Decls) {
// For renaming, we're only interested in foo's declaration, so drop the other
// one. There should never be more than one UsingDecl here, otherwise the
// rename would be ambiguos anyway.
- auto UD = std::find_if(Decls.begin(), Decls.end(), [](const NamedDecl *D) {
- return llvm::isa<UsingDecl>(D);
+ auto UD = std::find_if(Decls.begin(), Decls.end(), [](const auto &P) {
+ return llvm::isa<UsingDecl>(P.first);
});
if (UD != Decls.end()) {
Decls.erase(UD);
@@ -206,6 +324,7 @@ enum class ReasonToReject {
NonIndexable,
UnsupportedSymbol,
AmbiguousSymbol,
+ OnlyRenameableFromDefinition,
// name validation. FIXME: reconcile with InvalidName
SameName,
@@ -274,6 +393,8 @@ llvm::Error makeError(ReasonToReject Reason) {
return "symbol is not a supported kind (e.g. namespace, macro)";
case ReasonToReject::AmbiguousSymbol:
return "there are multiple symbols at the given location";
+ case ReasonToReject::OnlyRenameableFromDefinition:
+ return "only renameable from the implementation";
case ReasonToReject::SameName:
return "new name is the same as the old name";
}
@@ -289,13 +410,28 @@ std::vector<SourceLocation> findOccurrencesWithinFile(ParsedAST &AST,
assert(canonicalRenameDecl(&ND) == &ND &&
"ND should be already canonicalized.");
+ bool IsSythesizedFromProperty = false;
+ if (const auto *ID = dyn_cast<ObjCIvarDecl>(&ND))
+ IsSythesizedFromProperty = ID->getSynthesize();
+ else if (const auto *MD = dyn_cast<ObjCMethodDecl>(&ND))
+ IsSythesizedFromProperty = MD->isPropertyAccessor() && MD->isImplicit();
+
std::vector<SourceLocation> Results;
+ // TODO(davg): Is this actually needed?
+ if (isa<ObjCPropertyDecl>(&ND))
+ Results.push_back(ND.getLocation());
+
for (Decl *TopLevelDecl : AST.getLocalTopLevelDecls()) {
findExplicitReferences(
TopLevelDecl,
[&](ReferenceLoc Ref) {
if (Ref.Targets.empty())
return;
+ // Some synthesized decls report their locations as the same as the
+ // decl they were derived from. We need to skip such decls but keep
+ // references otherwise we would rename the wrong decl.
+ if (IsSythesizedFromProperty && Ref.IsDecl)
+ return;
for (const auto *Target : Ref.Targets) {
if (canonicalRenameDecl(Target) == &ND) {
Results.push_back(Ref.NameLoc);
@@ -784,49 +920,70 @@ renameObjCMethodWithinFile(ParsedAST &AST, const ObjCMethodDecl *MD,
// AST-based rename, it renames all occurrences in the main file.
llvm::Expected<tooling::Replacements>
-renameWithinFile(ParsedAST &AST, const NamedDecl &RenameDecl,
- llvm::StringRef NewName) {
+renameWithinFile(ParsedAST &AST,
+ const llvm::DenseMap<const NamedDecl *, std::string> &DeclToNewName) {
trace::Span Tracer("RenameWithinFile");
const SourceManager &SM = AST.getSourceManager();
tooling::Replacements FilteredChanges;
- std::vector<SourceLocation> Locs;
- for (SourceLocation Loc : findOccurrencesWithinFile(AST, RenameDecl)) {
- SourceLocation RenameLoc = Loc;
- // We don't rename in any macro bodies, but we allow rename the symbol
- // spelled in a top-level macro argument in the main file.
- if (RenameLoc.isMacroID()) {
- if (isInMacroBody(SM, RenameLoc))
+ for (const auto &Entry : DeclToNewName) {
+ std::string ImplicitPropName;
+ std::string NewImplicitPropName;
+ if (const auto *MD = llvm::dyn_cast<ObjCMethodDecl>(Entry.first)) {
+ if (MD->getReturnType()->isVoidType() &&
+ MD->getSelector().getNumArgs() == 1) {
+ llvm::StringRef Name = MD->getSelector().getNameForSlot(0);
+ ImplicitPropName = setterToPropertyName(Name);
+ NewImplicitPropName = setterToPropertyName(Entry.second);
+ }
+ }
+
+ std::vector<SourceLocation> Locs;
+ for (SourceLocation Loc : findOccurrencesWithinFile(AST, *Entry.first)) {
+ SourceLocation RenameLoc = Loc;
+ // We don't rename in any macro bodies, but we allow rename the symbol
+ // spelled in a top-level macro argument in the main file.
+ if (RenameLoc.isMacroID()) {
+ if (isInMacroBody(SM, RenameLoc))
+ continue;
+ RenameLoc = SM.getSpellingLoc(Loc);
+ }
+ // Filter out locations not from main file.
+ // We traverse only main file decls, but locations could come from an
+ // non-preamble #include file e.g.
+ // void test() {
+ // int f^oo;
+ // #include "use_foo.inc"
+ // }
+ if (!isInsideMainFile(RenameLoc, SM))
continue;
- RenameLoc = SM.getSpellingLoc(Loc);
+ Locs.push_back(RenameLoc);
+ }
+ if (const auto *MD = dyn_cast<ObjCMethodDecl>(&RenameDecl)) {
+ // The custom ObjC selector logic doesn't handle the zero arg selector
+ // case, as it relies on parsing selectors via the trailing `:`.
+ // We also choose to use regular rename logic for the single-arg selectors
+ // as the AST/Index has the right locations in that case.
+ if (MD->getSelector().getNumArgs() > 1)
+ return renameObjCMethodWithinFile(AST, MD, NewName, std::move(Locs));
+
+ // Eat trailing : for single argument methods since they're actually
+ // considered a separate token during rename.
+ NewName.consume_back(":");
+ }
+ for (const auto &Loc : Locs) {
+ llvm::StringRef NewName = Entry.second;
+ if (!ImplicitPropName.empty() && !NewImplicitPropName.empty()) {
+ const auto T = AST.getTokens().spelledTokenAt(Loc);
+ if (T && T->text(SM) == ImplicitPropName) {
+ NewName = NewImplicitPropName;
+ }
+ }
+
+ if (auto Err = FilteredChanges.add(tooling::Replacement(
+ SM, CharSourceRange::getTokenRange(Loc), NewName)))
+ return std::move(Err);
}
- // Filter out locations not from main file.
- // We traverse only main file decls, but locations could come from an
- // non-preamble #include file e.g.
- // void test() {
- // int f^oo;
- // #include "use_foo.inc"
- // }
- if (!isInsideMainFile(RenameLoc, SM))
- continue;
- Locs.push_back(RenameLoc);
- }
- if (const auto *MD = dyn_cast<ObjCMethodDecl>(&RenameDecl)) {
- // The custom ObjC selector logic doesn't handle the zero arg selector
- // case, as it relies on parsing selectors via the trailing `:`.
- // We also choose to use regular rename logic for the single-arg selectors
- // as the AST/Index has the right locations in that case.
- if (MD->getSelector().getNumArgs() > 1)
- return renameObjCMethodWithinFile(AST, MD, NewName, std::move(Locs));
-
- // Eat trailing : for single argument methods since they're actually
- // considered a separate token during rename.
- NewName.consume_back(":");
- }
- for (const auto &Loc : Locs) {
- if (auto Err = FilteredChanges.add(tooling::Replacement(
- SM, CharSourceRange::getTokenRange(Loc), NewName)))
- return std::move(Err);
}
return FilteredChanges;
}
@@ -1054,23 +1211,61 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
if (locateMacroAt(*IdentifierToken, AST.getPreprocessor()))
return makeError(ReasonToReject::UnsupportedSymbol);
- auto DeclsUnderCursor = locateDeclAt(AST, IdentifierToken->location());
+ auto DeclsUnderCursor =
+ locateDeclAt(AST, IdentifierToken->location(), RInputs.NewName);
filterRenameTargets(DeclsUnderCursor);
if (DeclsUnderCursor.empty())
return makeError(ReasonToReject::NoSymbolFound);
if (DeclsUnderCursor.size() > 1)
return makeError(ReasonToReject::AmbiguousSymbol);
- const auto &RenameDecl = **DeclsUnderCursor.begin();
- std::string Placeholder = getName(RenameDecl);
- auto Invalid = checkName(RenameDecl, RInputs.NewName, Placeholder);
- if (Invalid)
- return std::move(Invalid);
-
- auto Reject =
- renameable(RenameDecl, RInputs.MainFilePath, RInputs.Index, Opts);
- if (Reject)
- return makeError(*Reject);
+ const auto &First = *DeclsUnderCursor.begin();
+ const auto &RenameDecl = *First.first;
+ const auto NewName = First.second;
+ const auto *Origin = findOriginDecl(&RenameDecl);
+ // We can only rename the property decl if we're able to find the property
+ // impl decl.
+ if (const auto *PD = dyn_cast<ObjCPropertyDecl>(Origin)) {
+ const auto &ASTCtx = PD->getASTContext();
+ const auto *DC = PD->getDeclContext();
+ if (const auto *ID = dyn_cast<ObjCInterfaceDecl>(DC)) {
+ if (!ASTCtx.getObjCPropertyImplDeclForPropertyDecl(
+ PD, ID->getImplementation())) {
+ return makeError(ReasonToReject::OnlyRenameableFromDefinition);
+ }
+ }
+ if (const auto *CD = dyn_cast<ObjCCategoryDecl>(DC)) {
+ if (const auto *Impl = CD->getImplementation()) {
+ if (!ASTCtx.getObjCPropertyImplDeclForPropertyDecl(PD, Impl)) {
+ return makeError(ReasonToReject::OnlyRenameableFromDefinition);
+ }
+ } else if (const auto *ID = CD->getClassInterface()) {
+ if (!ASTCtx.getObjCPropertyImplDeclForPropertyDecl(
+ PD, ID->getImplementation())) {
+ return makeError(ReasonToReject::OnlyRenameableFromDefinition);
+ }
+ }
+ }
+ }
+ // If we the user is triggering a rename on a ObjC Method from an implicit
+ // property decl, we need to fix up the name to be in the `setX` form.
+ const auto DeclToNewName =
+ computeAllDeclsToNewName(&RenameDecl, NewName, Origin);
+ std::string Placeholder;
+
+ for (const auto &Entry : DeclToNewName) {
+ std::string Name = getName(*Entry.first);
+ auto Invalid = checkName(*Entry.first, Entry.second, Name);
+ if (Invalid)
+ return std::move(Invalid);
+ if (Entry.first == &RenameDecl)
+ Placeholder = Name;
+
+ auto Reject =
+ renameable(*Entry.first, RInputs.MainFilePath, RInputs.Index, Opts);
+ if (Reject)
+ return makeError(*Reject);
+ }
// We have two implementations of the rename:
// - AST-based rename: used for renaming local symbols, e.g. variables
@@ -1081,7 +1276,7 @@ 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, DeclToNewName);
if (!MainFileRenameEdit)
return MainFileRenameEdit.takeError();
@@ -1137,14 +1332,27 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
return Result;
}
- auto OtherFilesEdits = renameOutsideFile(
- RenameDecl, RInputs.MainFilePath, RInputs.NewName, *RInputs.Index,
- Opts.LimitFiles == 0 ? std::numeric_limits<size_t>::max()
- : Opts.LimitFiles,
- *RInputs.FS);
- if (!OtherFilesEdits)
- return OtherFilesEdits.takeError();
- Result.GlobalChanges = *OtherFilesEdits;
+ FileEdits GlobalChanges;
+ for (const auto &Entry : DeclToNewName) {
+ auto OtherFilesEdits = renameOutsideFile(
+ *Entry.first, RInputs.MainFilePath, Entry.second, *RInputs.Index,
+ Opts.LimitFiles == 0 ? std::numeric_limits<size_t>::max()
+ : Opts.LimitFiles,
+ *RInputs.FS);
+ if (!OtherFilesEdits)
+ return OtherFilesEdits.takeError();
+ // Merge all edits.
+ for (const auto &E : *OtherFilesEdits) {
+ auto It = GlobalChanges.find(E.getKey());
+ if (It == GlobalChanges.end()) {
+ GlobalChanges.try_emplace(E.getKey(), std::move(E.getValue()));
+ continue;
+ }
+ It->second.Replacements =
+ It->second.Replacements.merge(E.getValue().Replacements);
+ }
+ }
+ Result.GlobalChanges = GlobalChanges;
// Attach the rename edits for the main file.
Result.GlobalChanges.try_emplace(RInputs.MainFilePath,
std::move(MainFileEdits));
diff --git a/clang-tools-extra/clangd/unittests/RenameTests.cpp b/clang-tools-extra/clangd/unittests/RenameTests.cpp
index 7d9252110b27df..a51e9d8984b358 100644
--- a/clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ b/clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -861,6 +861,19 @@ TEST(RenameTest, WithinFileRename) {
void func([[Fo^o]] *f) {}
)cpp",
+
+ // ObjC property.
+ R"cpp(
+ @interface Foo
+ @property(nonatomic) int [[f^oo]];
+ @end
+ @implementation Foo
+ @end
+
+ void func(Foo *f) {
+ f.[[f^oo]] += [f [[fo^o]]];
+ }
+ )cpp",
};
llvm::StringRef NewName = "NewName";
for (llvm::StringRef T : Tests) {
@@ -1008,18 +1021,95 @@ TEST(RenameTest, ObjCWithinFileRename) {
"performNewAction:by:",
// Expected
std::nullopt,
+ },
+ {
+ R"cpp(
+ @interface Foo
+ @property(nonatomic) int fo^o;
+ @end
+ @implementation Foo
+ @end
+
+ void func(Foo *f) {
+ [f setFoo:[f foo] ];
+ }
+ )cpp",
+ "bar",
+ R"cpp(
+ @interface Foo
+ @property(nonatomic) int bar;
+ @end
+ @implementation Foo
+ @end
+
+ void func(Foo *f) {
+ [f setBar:[f bar] ];
+ }
+ )cpp",
+ },
+ {
+ R"cpp(
+ @interface Foo
+ @property(nonatomic) int foo;
+ @end
+ @implementation Foo
+ @end
+
+ void func(Foo *f) {
+ [f setF^oo:[f foo] ];
+ }
+ )cpp",
+ "setBar:",
+ R"cpp(
+ @interface Foo
+ @property(nonatomic) int bar;
+ @end
+ @implementation Foo
+ @end
+
+ void func(Foo *f) {
+ [f setBar:[f bar] ];
+ }
+ )cpp",
+ },
+ {
+ R"cpp(
+ @interface Foo
+ @property(nonatomic) int foo;
+ @end
+ @implementation Foo
+ @end
+
+ void func(Foo *f) {
+ [f setFoo:[f fo^o] ];
+ }
+ )cpp",
+ "bar",
+ R"cpp(
+ @interface Foo
+ @property(nonatomic) int bar;
+ @end
+ @implementation Foo
+ @end
+
+ void func(Foo *f) {
+ [f setBar:[f bar] ];
+ }
+ )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()});
+
if (std::optional<StringRef> Expected = T.Expected) {
ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
ASSERT_EQ(1u, RenameResult->GlobalChanges.size());
diff --git a/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp b/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
index 4156921d83edf8..35aaa6f84fb40b 100644
--- a/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ b/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -733,7 +733,7 @@ sizeof...($TemplateParameter[[Elements]]);
@property(readonly, class) $Class[[Foo]] *$Field_decl_readonly_static[[sharedInstance]];
@end
@implementation $Class_def[[Foo]]
- @synthesize someProperty = _someProperty;
+ @synthesize $Field[[someProperty]] = $Field[[_someProperty]];
- (int)$Method_def[[otherMethod]] {
return 0;
}
>From 2dbfd64f035a53200c2166245f11a2e51e6e06be Mon Sep 17 00:00:00 2001
From: David Goldman <davg at google.com>
Date: Fri, 16 Feb 2024 15:09:15 -0500
Subject: [PATCH 2/5] Run clang-format
---
clang-tools-extra/clangd/refactor/Rename.cpp | 22 ++++++++++++-------
.../clangd/unittests/RenameTests.cpp | 18 +++++++--------
2 files changed, 23 insertions(+), 17 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index b53c24b8331ddb..b1145363acb303 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -919,9 +919,9 @@ renameObjCMethodWithinFile(ParsedAST &AST, const ObjCMethodDecl *MD,
}
// AST-based rename, it renames all occurrences in the main file.
-llvm::Expected<tooling::Replacements>
-renameWithinFile(ParsedAST &AST,
- const llvm::DenseMap<const NamedDecl *, std::string> &DeclToNewName) {
+llvm::Expected<tooling::Replacements> renameWithinFile(
+ ParsedAST &AST,
+ const llvm::DenseMap<const NamedDecl *, std::string> &DeclToNewName) {
trace::Span Tracer("RenameWithinFile");
const SourceManager &SM = AST.getSourceManager();
@@ -964,8 +964,14 @@ renameWithinFile(ParsedAST &AST,
// case, as it relies on parsing selectors via the trailing `:`.
// We also choose to use regular rename logic for the single-arg selectors
// as the AST/Index has the right locations in that case.
- if (MD->getSelector().getNumArgs() > 1)
- return renameObjCMethodWithinFile(AST, MD, NewName, std::move(Locs));
+ if (MD->getSelector().getNumArgs() > 1) {
+ auto Res =
+ renameObjCMethodWithinFile(AST, MD, Entry.second, std::move(Locs));
+ if (!Res)
+ return Res.takeError();
+ FilteredChanges = FilteredChanges.merge(Res.get());
+ continue;
+ }
// Eat trailing : for single argument methods since they're actually
// considered a separate token during rename.
@@ -1241,7 +1247,7 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
}
} else if (const auto *ID = CD->getClassInterface()) {
if (!ASTCtx.getObjCPropertyImplDeclForPropertyDecl(
- PD, ID->getImplementation())) {
+ PD, ID->getImplementation())) {
return makeError(ReasonToReject::OnlyRenameableFromDefinition);
}
}
@@ -1262,7 +1268,7 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
Placeholder = Name;
auto Reject =
- renameable(*Entry.first, RInputs.MainFilePath, RInputs.Index, Opts);
+ renameable(*Entry.first, RInputs.MainFilePath, RInputs.Index, Opts);
if (Reject)
return makeError(*Reject);
}
@@ -1337,7 +1343,7 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
auto OtherFilesEdits = renameOutsideFile(
*Entry.first, RInputs.MainFilePath, Entry.second, *RInputs.Index,
Opts.LimitFiles == 0 ? std::numeric_limits<size_t>::max()
- : Opts.LimitFiles,
+ : Opts.LimitFiles,
*RInputs.FS);
if (!OtherFilesEdits)
return OtherFilesEdits.takeError();
diff --git a/clang-tools-extra/clangd/unittests/RenameTests.cpp b/clang-tools-extra/clangd/unittests/RenameTests.cpp
index a51e9d8984b358..b10c4e674c021b 100644
--- a/clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ b/clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -1023,7 +1023,7 @@ TEST(RenameTest, ObjCWithinFileRename) {
std::nullopt,
},
{
- R"cpp(
+ R"cpp(
@interface Foo
@property(nonatomic) int fo^o;
@end
@@ -1034,8 +1034,8 @@ TEST(RenameTest, ObjCWithinFileRename) {
[f setFoo:[f foo] ];
}
)cpp",
- "bar",
- R"cpp(
+ "bar",
+ R"cpp(
@interface Foo
@property(nonatomic) int bar;
@end
@@ -1048,7 +1048,7 @@ TEST(RenameTest, ObjCWithinFileRename) {
)cpp",
},
{
- R"cpp(
+ R"cpp(
@interface Foo
@property(nonatomic) int foo;
@end
@@ -1059,8 +1059,8 @@ TEST(RenameTest, ObjCWithinFileRename) {
[f setF^oo:[f foo] ];
}
)cpp",
- "setBar:",
- R"cpp(
+ "setBar:",
+ R"cpp(
@interface Foo
@property(nonatomic) int bar;
@end
@@ -1073,7 +1073,7 @@ TEST(RenameTest, ObjCWithinFileRename) {
)cpp",
},
{
- R"cpp(
+ R"cpp(
@interface Foo
@property(nonatomic) int foo;
@end
@@ -1084,8 +1084,8 @@ TEST(RenameTest, ObjCWithinFileRename) {
[f setFoo:[f fo^o] ];
}
)cpp",
- "bar",
- R"cpp(
+ "bar",
+ R"cpp(
@interface Foo
@property(nonatomic) int bar;
@end
>From b9db7eb1746cd0d87dba849bb5c6c20695fca84a Mon Sep 17 00:00:00 2001
From: David Goldman <davg at google.com>
Date: Fri, 16 Feb 2024 15:36:47 -0500
Subject: [PATCH 3/5] Add more tests
---
.../clangd/unittests/RenameTests.cpp | 49 +++++++++++++++++++
1 file changed, 49 insertions(+)
diff --git a/clang-tools-extra/clangd/unittests/RenameTests.cpp b/clang-tools-extra/clangd/unittests/RenameTests.cpp
index b10c4e674c021b..24a2f0f7aed9a9 100644
--- a/clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ b/clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -1096,6 +1096,55 @@ TEST(RenameTest, ObjCWithinFileRename) {
[f setBar:[f bar] ];
}
)cpp",
+ },
+ {
+ R"cpp(
+ @interface Foo
+ - (int)fo^o;
+ - (void)setFoo:(int)foo;
+ @end
+ @implementation Foo
+ - (int)fo^o { return 0; }
+ - (void)setFoo:(int)foo {}
+ @end
+
+ void func(Foo *f) {
+ f.foo = f.fo^o + 1;
+ }
+ )cpp",
+ "bar",
+ R"cpp(
+ @interface Foo
+ - (int)bar;
+ - (void)setFoo:(int)foo;
+ @end
+ @implementation Foo
+ - (int)bar { return 0; }
+ - (void)setFoo:(int)foo {}
+ @end
+
+ void func(Foo *f) {
+ f.foo = f.bar + 1;
+ }
+ )cpp",
+ },
+ {
+ R"cpp(
+ @interface Foo
+ - (int)foo;
+ - (void)setFoo:(int)foo;
+ @end
+ @implementation Foo
+ - (int)foo { return 1; }
+ - (void)setFoo:(int)foo {}
+ @end
+
+ void func(Foo *f) {
+ f.f^oo += 1;
+ }
+ )cpp",
+ "bar",
+ std::nullopt,
}};
for (TestCase T : Tests) {
SCOPED_TRACE(T.Input);
>From f0c3ad571aabe6b31136a079ff7694475e52216a Mon Sep 17 00:00:00 2001
From: David Goldman <davg at google.com>
Date: Tue, 27 Feb 2024 11:29:39 -0500
Subject: [PATCH 4/5] Minor fixes for rebase
---
clang-tools-extra/clangd/refactor/Rename.cpp | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index b1145363acb303..c54b114d63442f 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -959,7 +959,7 @@ llvm::Expected<tooling::Replacements> renameWithinFile(
continue;
Locs.push_back(RenameLoc);
}
- if (const auto *MD = dyn_cast<ObjCMethodDecl>(&RenameDecl)) {
+ if (const auto *MD = dyn_cast<ObjCMethodDecl>(Entry.first)) {
// The custom ObjC selector logic doesn't handle the zero arg selector
// case, as it relies on parsing selectors via the trailing `:`.
// We also choose to use regular rename logic for the single-arg selectors
@@ -972,13 +972,14 @@ llvm::Expected<tooling::Replacements> renameWithinFile(
FilteredChanges = FilteredChanges.merge(Res.get());
continue;
}
-
- // Eat trailing : for single argument methods since they're actually
- // considered a separate token during rename.
- NewName.consume_back(":");
}
for (const auto &Loc : Locs) {
llvm::StringRef NewName = Entry.second;
+ if (isa<ObjCMethodDecl>(Entry.first))
+ // Eat trailing : for single argument methods since they're actually
+ // considered a separate token during rename.
+ NewName.consume_back(":");
+
if (!ImplicitPropName.empty() && !NewImplicitPropName.empty()) {
const auto T = AST.getTokens().spelledTokenAt(Loc);
if (T && T->text(SM) == ImplicitPropName) {
>From 7d99e1a85316cfadbf9fa3e05db0d3d66b075fb4 Mon Sep 17 00:00:00 2001
From: David Goldman <davg at google.com>
Date: Thu, 7 Mar 2024 12:40:22 -0500
Subject: [PATCH 5/5] Add implicit property support to isSpelled
---
.../clangd/index/SymbolCollector.cpp | 24 ++++++++++++++++++-
.../clangd/unittests/SymbolCollectorTests.cpp | 5 ++++
2 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp
index 85b8fc549b016e..2cc8aedb721f41 100644
--- a/clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -169,6 +169,19 @@ std::optional<RelationKind> indexableRelation(const index::SymbolRelation &R) {
return std::nullopt;
}
+// Returns a non-empty string if valid.
+std::string setterToPropertyName(llvm::StringRef Setter) {
+ std::string Result;
+ if (!Setter.consume_front("set")) {
+ return Result;
+ }
+ Setter.consume_back(":"); // Optional.
+ Result = Setter.str();
+ if (!Result.empty())
+ Result[0] = llvm::toLower(Result[0]);
+ return Result;
+}
+
// Check if there is an exact spelling of \p ND at \p Loc.
bool isSpelled(SourceLocation Loc, const NamedDecl &ND) {
auto Name = ND.getDeclName();
@@ -186,8 +199,17 @@ bool isSpelled(SourceLocation Loc, const NamedDecl &ND) {
if (clang::Lexer::getRawToken(Loc, Tok, SM, LO))
return false;
auto TokSpelling = clang::Lexer::getSpelling(Tok, SM, LO);
- if (const auto *MD = dyn_cast<ObjCMethodDecl>(&ND))
+ if (const auto *MD = dyn_cast<ObjCMethodDecl>(&ND)) {
+ // - (void)setFoo:(id)foo can be referenced via `self.foo = <expr>`.
+ if (MD->getReturnType()->isVoidType() &&
+ MD->getSelector().getNumArgs() == 1) {
+ std::string ImplicitPropName =
+ setterToPropertyName(MD->getSelector().getNameForSlot(0));
+ if (!ImplicitPropName.empty() && TokSpelling == ImplicitPropName)
+ return true;
+ }
return TokSpelling == MD->getSelector().getNameForSlot(0);
+ }
return TokSpelling == Name.getAsString();
}
} // namespace
diff --git a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
index 1e7a30e69fabe0..9406392ea5e514 100644
--- a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -536,6 +536,7 @@ TEST_F(SymbolCollectorTest, ObjCRefs) {
@interface Person (Category)
- (void)categoryMethod;
- (void)multiArg:(id)a method:(id)b;
+ - (void)$setter[[setFoo]]:(id)foo;
@end
)");
Annotations Main(R"(
@@ -549,6 +550,7 @@ TEST_F(SymbolCollectorTest, ObjCRefs) {
[p $say[[say]]:0];
[p categoryMethod];
[p multiArg:0 method:0];
+ p.$setter[[foo]] = 0;
}
)");
CollectorOpts.RefFilter = RefKind::All;
@@ -566,6 +568,9 @@ TEST_F(SymbolCollectorTest, ObjCRefs) {
EXPECT_THAT(Refs,
Contains(Pair(findSymbol(Symbols, "Person::multiArg:method:").ID,
ElementsAre(isSpelled()))));
+ EXPECT_THAT(Refs,
+ Contains(Pair(findSymbol(Symbols, "Person::setFoo:").ID,
+ ElementsAre(isSpelled()))));
}
TEST_F(SymbolCollectorTest, ObjCSymbols) {
More information about the cfe-commits
mailing list