[clang-tools-extra] [clangd] Add support for renaming ObjC properties and implicit properties (PR #81775)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Feb 14 10:48:36 PST 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clangd
Author: David Goldman (DavidGoldman)
<details>
<summary>Changes</summary>
Add support for renaming Objective-C properties. These are special since a single property decl could generate 3 different decls - a getter method, a setter method, and an instance variable. In addition, support renaming implicit properties, e.g. referring to a getter method `- (id)foo;` via `self.foo` or a setter method `- (void)setFoo:(id)foo;` via `self.foo =` without an explicit property decl.
---
Patch is 63.64 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/81775.diff
12 Files Affected:
- (modified) clang-tools-extra/clangd/ClangdLSPServer.cpp (+5-2)
- (modified) clang-tools-extra/clangd/ClangdLSPServer.h (+1-1)
- (modified) clang-tools-extra/clangd/FindTarget.cpp (+16)
- (modified) clang-tools-extra/clangd/Protocol.cpp (+9)
- (modified) clang-tools-extra/clangd/Protocol.h (+30)
- (modified) clang-tools-extra/clangd/index/SymbolCollector.cpp (+8-3)
- (modified) clang-tools-extra/clangd/refactor/Rename.cpp (+651-100)
- (modified) clang-tools-extra/clangd/refactor/Rename.h (+32-7)
- (modified) clang-tools-extra/clangd/test/rename.test (+3)
- (modified) clang-tools-extra/clangd/unittests/RenameTests.cpp (+149-17)
- (modified) clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp (+1-1)
- (modified) clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp (+44)
``````````diff
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index a87da252b7a7e9..3e097cbeed5929 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -844,14 +844,17 @@ 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;
+ PrepareResult.range = Result->Target;
+ PrepareResult.placeholder = Result->Placeholder;
+ 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/FindTarget.cpp b/clang-tools-extra/clangd/FindTarget.cpp
index 839cf6332fe8b0..559f971ef758f2 100644
--- a/clang-tools-extra/clangd/FindTarget.cpp
+++ b/clang-tools-extra/clangd/FindTarget.cpp
@@ -733,6 +733,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/Protocol.cpp b/clang-tools-extra/clangd/Protocol.cpp
index a6370649f5ad1c..f93a941a5c27e4 100644
--- a/clang-tools-extra/clangd/Protocol.cpp
+++ b/clang-tools-extra/clangd/Protocol.cpp
@@ -1187,6 +1187,15 @@ bool fromJSON(const llvm::json::Value &Params, RenameParams &R,
O.map("position", R.position) && O.map("newName", R.newName);
}
+llvm::json::Value toJSON(const PrepareRenameResult &PRR) {
+ if (PRR.placeholder.empty())
+ return toJSON(PRR.range);
+ return llvm::json::Object{
+ {"range", toJSON(PRR.range)},
+ {"placeholder", PRR.placeholder},
+ };
+}
+
llvm::json::Value toJSON(const DocumentHighlight &DH) {
return llvm::json::Object{
{"range", toJSON(DH.range)},
diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h
index e88c804692568f..dc3fdb5b5f6c9b 100644
--- a/clang-tools-extra/clangd/Protocol.h
+++ b/clang-tools-extra/clangd/Protocol.h
@@ -1436,6 +1436,14 @@ struct RenameParams {
};
bool fromJSON(const llvm::json::Value &, RenameParams &, llvm::json::Path);
+struct PrepareRenameResult {
+ /// Range of the string to rename.
+ Range range;
+ /// Placeholder text to use in the editor if non-empty.
+ std::string placeholder;
+};
+llvm::json::Value toJSON(const PrepareRenameResult &PRR);
+
enum class DocumentHighlightKind { Text = 1, Read = 2, Write = 3 };
/// A document highlight is a range inside a text document which deserves
@@ -1969,6 +1977,28 @@ llvm::raw_ostream &operator<<(llvm::raw_ostream &, const ASTNode &);
} // namespace clang
namespace llvm {
+
+template <> struct DenseMapInfo<clang::clangd::Range> {
+ using Range = clang::clangd::Range;
+ static inline Range getEmptyKey() {
+ static clang::clangd::Position Tomb{-1, -1};
+ static Range R{Tomb, Tomb};
+ return R;
+ }
+ static inline Range getTombstoneKey() {
+ static clang::clangd::Position Tomb{-2, -2};
+ static Range R{Tomb, Tomb};
+ return R;
+ }
+ static unsigned getHashValue(const Range &Val) {
+ return llvm::hash_combine(Val.start.line, Val.start.character, Val.end.line,
+ Val.end.character);
+ }
+ static bool isEqual(const Range &LHS, const Range &RHS) {
+ return std::tie(LHS.start, LHS.end) == std::tie(RHS.start, RHS.end);
+ }
+};
+
template <> struct format_provider<clang::clangd::Position> {
static void format(const clang::clangd::Position &Pos, raw_ostream &OS,
StringRef Style) {
diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp
index 7ef4b15febad22..f7afa5c099742e 100644
--- a/clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -174,7 +174,10 @@ bool isSpelled(SourceLocation Loc, const NamedDecl &ND) {
auto Name = ND.getDeclName();
const auto NameKind = Name.getNameKind();
if (NameKind != DeclarationName::Identifier &&
- NameKind != DeclarationName::CXXConstructorName)
+ NameKind != DeclarationName::CXXConstructorName &&
+ NameKind != DeclarationName::ObjCZeroArgSelector &&
+ NameKind != DeclarationName::ObjCOneArgSelector &&
+ NameKind != DeclarationName::ObjCMultiArgSelector)
return false;
const auto &AST = ND.getASTContext();
const auto &SM = AST.getSourceManager();
@@ -182,8 +185,10 @@ bool isSpelled(SourceLocation Loc, const NamedDecl &ND) {
clang::Token Tok;
if (clang::Lexer::getRawToken(Loc, Tok, SM, LO))
return false;
- auto StrName = Name.getAsString();
- return clang::Lexer::getSpelling(Tok, SM, LO) == StrName;
+ auto TokSpelling = clang::Lexer::getSpelling(Tok, SM, LO);
+ if (const auto *MD = dyn_cast<ObjCMethodDecl>(&ND))
+ return TokSpelling == MD->getSelector().getNameForSlot(0);
+ return TokSpelling == Name.getAsString();
}
} // namespace
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index 11f9e4627af760..004a091bce7692 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,
@@ -222,6 +341,11 @@ std::optional<ReasonToReject> renameable(const NamedDecl &RenameDecl,
return ReasonToReject::UnsupportedSymbol;
}
}
+ // We allow renaming ObjC methods although they don't have a simple
+ // identifier.
+ const auto *ID = RenameDecl.getIdentifier();
+ if (!ID && !isa<ObjCMethodDecl>(&RenameDecl))
+ return ReasonToReject::UnsupportedSymbol;
// Filter out symbols that are unsupported in both rename modes.
if (llvm::isa<NamespaceDecl>(&RenameDecl))
return ReasonToReject::UnsupportedSymbol;
@@ -269,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";
}
@@ -284,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);
@@ -498,7 +639,7 @@ llvm::Error makeError(InvalidName Reason) {
return error("invalid name: {0}", Message(Reason));
}
-static bool mayBeValidIdentifier(llvm::StringRef Ident) {
+static bool mayBeValidIdentifier(llvm::StringRef Ident, bool AllowColon) {
assert(llvm::json::isUTF8(Ident));
if (Ident.empty())
return false;
@@ -508,24 +649,46 @@ static bool mayBeValidIdentifier(llvm::StringRef Ident) {
!isAsciiIdentifierStart(Ident.front(), AllowDollar))
return false;
for (char C : Ident) {
+ if (AllowColon && C == ':')
+ continue;
if (llvm::isASCII(C) && !isAsciiIdentifierContinue(C, AllowDollar))
return false;
}
return true;
}
+std::string getName(const NamedDecl &RenameDecl) {
+ if (const auto *MD = dyn_cast<ObjCMethodDecl>(&RenameDecl))
+ return MD->getSelector().getAsString();
+ if (const auto *ID = RenameDecl.getIdentifier())
+ return ID->getName().str();
+ return "";
+}
+
// 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) {
+std::optional<llvm::Error> checkName(const NamedDecl &RenameDecl,
+ llvm::StringRef NewName,
+ llvm::StringRef OldName) {
trace::Span Tracer("CheckName");
static constexpr trace::Metric InvalidNameMetric(
"rename_name_invalid", trace::Metric::Counter, "invalid_kind");
+
+ if (OldName == NewName)
+ return makeError(ReasonToReject::SameName);
+
+ if (const auto *MD = dyn_cast<ObjCMethodDecl>(&RenameDecl)) {
+ const auto Sel = MD->getSelector();
+ if (Sel.getNumArgs() != NewName.count(':') &&
+ NewName != "__clangd_rename_placeholder")
+ return makeError(InvalidName{InvalidName::BadIdentifier, NewName.str()});
+ }
+
auto &ASTCtx = RenameDecl.getASTContext();
std::optional<InvalidName> Result;
if (isKeyword(NewName, ASTCtx.getLangOpts()))
Result = InvalidName{InvalidName::Keywords, NewName.str()};
- else if (!mayBeValidIdentifier(NewName))
+ else if (!mayBeValidIdentifier(NewName, isa<ObjCMethodDecl>(&RenameDecl)))
Result = InvalidName{InvalidName::BadIdentifier, NewName.str()};
else {
// Name conflict detection.
@@ -538,40 +701,321 @@ std::optional<InvalidName> checkName(const NamedDecl &RenameDecl,
Conflict->getLocation().printToString(ASTCtx.getSourceManager())};
}
}
- if (Result)
+ if (Result) {
InvalidNameMetric.record(1, toString(Result->K));
+ return makeError(*Result);
+ }
+ return std::nullopt;
+}
+
+bool isMatchingSelectorName(const syntax::Token &Cur, const syntax::Token &Next,
+ const SourceManager &SM,
+ llvm::StringRef SelectorName) {
+ if (SelectorName.empty())
+ return Cur.kind() == tok::colon;
+ return Cur.kind() == tok::identifier && Next.kind() == tok::colon &&
+ Cur.text(SM) == SelectorName &&
+ // We require the selector name and : to be contiguous to avoid
+ // potential conflicts with ternary expression.
+ //
+ // e.g. support `foo:` but not `foo :`.
+ Cur.endLocation() == Next.location();
+}
+
+bool isSelectorLike(const syntax::Token &Cur, const syntax::Token &Next) {
+ return Cur.kind() == tok::identifier && Next.kind() == tok::colon &&
+ // We require the selector name and : to be contiguous.
+ // e.g. support `foo:` but not `foo :`.
+ Cur.endLocation() == Next.location();
+}
+
+bool parseMessageExpression(llvm::ArrayRef<syntax::Token> Tokens,
+ const SourceManager &SM, unsigned Index,
+ unsigned Last, Selector Sel,
+ std::vector<Range> &SelectorPieces) {
+
+ unsigned NumAr...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/81775
More information about the cfe-commits
mailing list