[clang] [clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)
via cfe-commits
cfe-commits at lists.llvm.org
Fri Feb 16 14:52:27 PST 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-tools-extra
Author: Alex Hoppen (ahoppen)
<details>
<summary>Changes</summary>
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.
CC @<!-- -->DavidGoldman @<!-- -->bnbarham @<!-- -->kadircet
---
Full diff: https://github.com/llvm/llvm-project/pull/82061.diff
8 Files Affected:
- (modified) clang-tools-extra/clangd/refactor/Rename.cpp (+25-32)
- (modified) clang-tools-extra/clangd/refactor/Rename.h (+3-3)
- (modified) clang-tools-extra/clangd/unittests/RenameTests.cpp (+3-3)
- (modified) clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h (+40-10)
- (modified) clang/lib/Tooling/Refactoring/CMakeLists.txt (+2)
- (modified) clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp (+2-2)
- (modified) clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp (+2-2)
- (added) clang/lib/Tooling/Refactoring/SymbolName.cpp (+70)
``````````diff
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
``````````
</details>
https://github.com/llvm/llvm-project/pull/82061
More information about the cfe-commits
mailing list