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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
