https://github.com/ahoppen updated https://github.com/llvm/llvm-project/pull/82061
>From a8f769d2376e01c789ebf10df95e18b8c23cb79f Mon Sep 17 00:00:00 2001 From: Alex Hoppen <ahop...@apple.com> Date: Fri, 16 Feb 2024 14:50:25 -0800 Subject: [PATCH] [clangd] Use `SymbolName` to represent Objective-C selectors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a cleaner design than using identifier and an optional `Selector`. It also allows rename of Objective-C method names if no declaration is at hand and thus no `Selector` instance can be formed. For example, when finding the ranges to rename based on an index that’s not clangd’s built-in index. --- clang-tools-extra/clangd/refactor/Rename.cpp | 57 +++++++-------- clang-tools-extra/clangd/refactor/Rename.h | 6 +- .../clangd/unittests/RenameTests.cpp | 6 +- .../Tooling/Refactoring/Rename/SymbolName.h | 50 ++++++++++--- clang/lib/Tooling/Refactoring/CMakeLists.txt | 2 + .../Refactoring/Rename/RenamingAction.cpp | 4 +- .../Refactoring/Rename/USRLocFinder.cpp | 4 +- clang/lib/Tooling/Refactoring/SymbolName.cpp | 70 +++++++++++++++++++ .../Tooling/RefactoringActionRulesTest.cpp | 6 +- 9 files changed, 150 insertions(+), 55 deletions(-) create mode 100644 clang/lib/Tooling/Refactoring/SymbolName.cpp diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp index 650862c99bcd12..bd2fcbb7ab1008 100644 --- a/clang-tools-extra/clangd/refactor/Rename.cpp +++ b/clang-tools-extra/clangd/refactor/Rename.cpp @@ -40,6 +40,8 @@ namespace clang { namespace clangd { namespace { +using tooling::SymbolName; + std::optional<std::string> filePath(const SymbolLocation &Loc, llvm::StringRef HintFilePath) { if (!Loc) @@ -591,11 +593,11 @@ bool isMatchingSelectorName(const syntax::Token &Cur, const syntax::Token &Next, // The search will terminate upon seeing Terminator or a ; at the top level. std::optional<SymbolRange> findAllSelectorPieces(llvm::ArrayRef<syntax::Token> Tokens, - const SourceManager &SM, Selector Sel, + const SourceManager &SM, const SymbolName &Name, tok::TokenKind Terminator) { assert(!Tokens.empty()); - unsigned NumArgs = Sel.getNumArgs(); + unsigned NumArgs = Name.getNamePieces().size(); llvm::SmallVector<tok::TokenKind, 8> Closes; std::vector<Range> SelectorPieces; for (unsigned Index = 0, Last = Tokens.size(); Index < Last - 1; ++Index) { @@ -605,12 +607,12 @@ findAllSelectorPieces(llvm::ArrayRef<syntax::Token> Tokens, auto PieceCount = SelectorPieces.size(); if (PieceCount < NumArgs && isMatchingSelectorName(Tok, Tokens[Index + 1], SM, - Sel.getNameForSlot(PieceCount))) { + Name.getNamePieces()[PieceCount])) { // If 'foo:' instead of ':' (empty selector), we need to skip the ':' // token after the name. We don't currently properly support empty // selectors since we may lex them improperly due to ternary statements // as well as don't properly support storing their ranges for edits. - if (!Sel.getNameForSlot(PieceCount).empty()) + if (!Name.getNamePieces()[PieceCount].empty()) ++Index; SelectorPieces.push_back( halfOpenToRange(SM, Tok.range(SM).toCharRange(SM))); @@ -662,16 +664,17 @@ findAllSelectorPieces(llvm::ArrayRef<syntax::Token> Tokens, /// Collects all ranges of the given identifier/selector in the source code. /// -/// If a selector is given, this does a full lex of the given source code in -/// order to identify all selector fragments (e.g. in method exprs/decls) since -/// they are non-contiguous. -std::vector<SymbolRange> collectRenameIdentifierRanges( - llvm::StringRef Identifier, llvm::StringRef Content, - const LangOptions &LangOpts, std::optional<Selector> Selector) { +/// If `Name` is an Objective-C symbol name, this does a full lex of the given +/// source code in order to identify all selector fragments (e.g. in method +/// exprs/decls) since they are non-contiguous. +std::vector<SymbolRange> +collectRenameIdentifierRanges(const tooling::SymbolName &Name, + llvm::StringRef Content, + const LangOptions &LangOpts) { std::vector<SymbolRange> Ranges; - if (!Selector) { + if (auto SinglePiece = Name.getSinglePiece()) { auto IdentifierRanges = - collectIdentifierRanges(Identifier, Content, LangOpts); + collectIdentifierRanges(*SinglePiece, Content, LangOpts); for (const auto &R : IdentifierRanges) Ranges.emplace_back(R); return Ranges; @@ -685,7 +688,7 @@ std::vector<SymbolRange> collectRenameIdentifierRanges( // parsing a method declaration or definition which isn't at the top level or // similar looking expressions (e.g. an @selector() expression). llvm::SmallVector<tok::TokenKind, 8> Closes; - llvm::StringRef FirstSelPiece = Selector->getNameForSlot(0); + llvm::StringRef FirstSelPiece = Name.getNamePieces()[0]; auto Tokens = syntax::tokenize(SM.getMainFileID(), SM, LangOpts); unsigned Last = Tokens.size() - 1; @@ -717,7 +720,7 @@ std::vector<SymbolRange> collectRenameIdentifierRanges( // Check if we can find all the relevant selector peices starting from // this token auto SelectorRanges = - findAllSelectorPieces(ArrayRef(Tokens).slice(Index), SM, *Selector, + findAllSelectorPieces(ArrayRef(Tokens).slice(Index), SM, Name, Closes.empty() ? tok::l_brace : Closes.back()); if (SelectorRanges) Ranges.emplace_back(std::move(*SelectorRanges)); @@ -764,7 +767,6 @@ renameObjCMethodWithinFile(ParsedAST &AST, const ObjCMethodDecl *MD, std::vector<SourceLocation> SelectorOccurences) { const SourceManager &SM = AST.getSourceManager(); auto Code = SM.getBufferData(SM.getMainFileID()); - auto RenameIdentifier = MD->getSelector().getNameForSlot(0).str(); llvm::SmallVector<llvm::StringRef, 8> NewNames; NewName.split(NewNames, ":"); @@ -774,7 +776,7 @@ renameObjCMethodWithinFile(ParsedAST &AST, const ObjCMethodDecl *MD, Ranges.push_back(tokenRangeForLoc(AST, Loc, SM, LangOpts)); auto FilePath = AST.tuPath(); auto RenameRanges = collectRenameIdentifierRanges( - RenameIdentifier, Code, LangOpts, MD->getSelector()); + SymbolName(MD->getDeclName()), Code, LangOpts); auto RenameEdit = buildRenameEdit(FilePath, Code, RenameRanges, NewNames); if (!RenameEdit) return error("failed to rename in file {0}: {1}", FilePath, @@ -926,22 +928,14 @@ renameOutsideFile(const NamedDecl &RenameDecl, llvm::StringRef MainFilePath, ExpBuffer.getError().message()); continue; } - std::string RenameIdentifier = RenameDecl.getNameAsString(); - std::optional<Selector> Selector = std::nullopt; + SymbolName RenameName(RenameDecl.getDeclName()); llvm::SmallVector<llvm::StringRef, 8> NewNames; - if (const auto *MD = dyn_cast<ObjCMethodDecl>(&RenameDecl)) { - if (MD->getSelector().getNumArgs() > 1) { - RenameIdentifier = MD->getSelector().getNameForSlot(0).str(); - Selector = MD->getSelector(); - } - } NewName.split(NewNames, ":"); auto AffectedFileCode = (*ExpBuffer)->getBuffer(); - auto RenameRanges = - adjustRenameRanges(AffectedFileCode, RenameIdentifier, - std::move(FileAndOccurrences.second), - RenameDecl.getASTContext().getLangOpts(), Selector); + auto RenameRanges = adjustRenameRanges( + AffectedFileCode, RenameName, std::move(FileAndOccurrences.second), + RenameDecl.getASTContext().getLangOpts()); 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 @@ -1226,14 +1220,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<SymbolRange>> -adjustRenameRanges(llvm::StringRef DraftCode, llvm::StringRef Identifier, - std::vector<Range> Indexed, const LangOptions &LangOpts, - std::optional<Selector> Selector) { +adjustRenameRanges(llvm::StringRef DraftCode, const tooling::SymbolName &Name, + std::vector<Range> Indexed, const LangOptions &LangOpts) { trace::Span Tracer("AdjustRenameRanges"); assert(!Indexed.empty()); assert(llvm::is_sorted(Indexed)); std::vector<SymbolRange> Lexed = - collectRenameIdentifierRanges(Identifier, DraftCode, LangOpts, Selector); + collectRenameIdentifierRanges(Name, DraftCode, LangOpts); 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 be5c346ae150f7..681b01e8dfc993 100644 --- a/clang-tools-extra/clangd/refactor/Rename.h +++ b/clang-tools-extra/clangd/refactor/Rename.h @@ -13,6 +13,7 @@ #include "SourceCode.h" #include "clang/Basic/IdentifierTable.h" #include "clang/Basic/LangOptions.h" +#include "clang/Tooling/Refactoring/Rename/SymbolName.h" #include "llvm/ADT/SmallVector.h" #include "llvm/Support/Error.h" #include <optional> @@ -108,9 +109,8 @@ llvm::Expected<Edit> buildRenameEdit(llvm::StringRef AbsFilePath, /// occurrence has the same length). /// REQUIRED: Indexed is sorted. std::optional<std::vector<SymbolRange>> -adjustRenameRanges(llvm::StringRef DraftCode, llvm::StringRef Identifier, - std::vector<Range> Indexed, const LangOptions &LangOpts, - std::optional<Selector> Selector); +adjustRenameRanges(llvm::StringRef DraftCode, const tooling::SymbolName &Name, + std::vector<Range> Indexed, const LangOptions &LangOpts); /// 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 d91ef85d672711..9c83a7416e9581 100644 --- a/clang-tools-extra/clangd/unittests/RenameTests.cpp +++ b/clang-tools-extra/clangd/unittests/RenameTests.cpp @@ -2040,9 +2040,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, std::nullopt); + auto ActualRanges = adjustRenameRanges( + Draft.code(), tooling::SymbolName("x", /*IsObjectiveCSelector=*/false), + Annotations(T.IndexedCode).ranges(), LangOpts); if (!ActualRanges) EXPECT_THAT(Draft.ranges(), testing::IsEmpty()); else diff --git a/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h b/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h index 6c28d40f3679c2..887ab0929445dc 100644 --- a/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h +++ b/clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h @@ -9,12 +9,16 @@ #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" #include "llvm/ADT/StringRef.h" namespace clang { + +class LangOptions; + namespace tooling { /// A name of a symbol. @@ -27,19 +31,45 @@ 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()); - } + SymbolName(); + + /// 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); - ArrayRef<std::string> getNamePieces() const { return Name; } + /// 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); -private: - llvm::SmallVector<std::string, 1> Name; + 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 d3077be8810aad..f78f64ea2ef649 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 @@ -23,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 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..896a6cf09a3a98 --- /dev/null +++ b/clang/lib/Tooling/Refactoring/SymbolName.cpp @@ -0,0 +1,70 @@ +//===--- 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() : NamePieces({}) {} + +SymbolName::SymbolName(const DeclarationName &DeclName) + : SymbolName(DeclName.getAsString(), + /*IsObjectiveCSelector=*/DeclName.getNameKind() == + DeclarationName::NameKind::ObjCMultiArgSelector || + DeclName.getNameKind() == + DeclarationName::NameKind::ObjCOneArgSelector) {} + +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()); +} + +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(); + } + 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 { + llvm::interleave(NamePieces, OS, ":"); +} + +} // end namespace tooling +} // end namespace clang 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); } }; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits