[clangd] Use SymbolName to represent Objective-C selectors
#82061
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clangd @llvm/pr-subscribers-clang Author: Alex Hoppen (ahoppen) ChangesThis is a cleaner design than using identifier and an optional CC @DavidGoldman @bnbarham @kadircet Full diff: https://github.com/llvm/llvm-project/pull/82061.diff 8 Files Affected:
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
|
|
@llvm/pr-subscribers-clang-tools-extra Author: Alex Hoppen (ahoppen) ChangesThis is a cleaner design than using identifier and an optional CC @DavidGoldman @bnbarham @kadircet Full diff: https://github.com/llvm/llvm-project/pull/82061.diff 8 Files Affected:
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
|
ahoppen
force-pushed
the
ahoppen/symbol-name
branch
3 times, most recently
from
8b4d813
to
a8f769d
Compare
bnbarham
approved these changes
Feb 22, 2024
| @@ -13,6 +13,7 @@ add_clang_library(clangToolingRefactoring | |||
| Rename/USRFinder.cpp | |||
| Rename/USRFindingAction.cpp | |||
| Rename/USRLocFinder.cpp | |||
| SymbolName.cpp | |||
There was a problem hiding this comment.
Should SymbolName.cpp live in Rename to match its header?
There was a problem hiding this comment.
Goode suggestions 👍🏽
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.
ahoppen
force-pushed
the
ahoppen/symbol-name
branch
from
a8f769d
to
fca2389
Compare
|
I don't know that this class brings enough value to warrant the dependency - we don't really seem to be simplifying the code, we're mostly just using it as a fancy (For some context: we went to some effort in the past to untangle this from tooling/Refactoring/Rename, and a lot of pieces of clangd make tradeoffs between keeping design simple and handling all the special cases of C, C++, ObjC precisely). Is the underlying goal here to be able to use (I don't think there are any plans to make use of Selector in other ways, @kadircet @DavidGoldman would know) |
sam-mccall
requested changes
Feb 23, 2024
| /// 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. |
There was a problem hiding this comment.
This looks too much like guesswork. For example in an ObjC++ file (which is our default parse language for *.h), SymbolName(operator std::string) will be parsed as the ObjC selector `[" operator std", "", "string"].
If we want to clearly distinguish the different types of names (as clang AST does) then we should avoid implicit conversions like this where it's easy to confuse the encoded/decoded state.
If we're content to mostly use strings and heuristically interpret them as selector names at the edges, that seems OK too - but then this class could just be a "split" function or so.
(This is up to you - I don't mind much what this API looks like unless we end up depending on it)
| @@ -27,19 +31,45 @@ namespace tooling { | |||
| /// // ^~ string 0 ~~~~~ ^~ string 1 ~~~~~ | |||
| /// \endcode | |||
| class SymbolName { | |||
| llvm::SmallVector<std::string, 1> NamePieces; | |||
There was a problem hiding this comment.
I'm a little wary of using this class as:
- it seems like the design has only been informed by objective-C selectors. It's nice to abstract away difficulties of dealing with names, but this doesn't handle many (scope qualifiers, operator names, UDL-names) and it's not clear how/whether it should. If not, I think clangd is better off without the layer of indirection.
- it's not really clear what you would do with this model (name chunks as strings) other than semi-textual rename. If it's a helper class as part of tooling/refactor/rename's API, it might be helpful if it were named/documented as such.
|
For some context: When I’m talking about * finding the ranges to rename based on an index that’s not clangd’s built-in index* I meant a request like apple#7973. This allows us to use Apple’s IndexStore to find the locations of symbols to rename and then invoke clangd to get the edits to rename the symbols and deal with the parsing of Objective-C selector pieces. Functionality-wise, I would be fine with using a |
I see. That makes sense, it's just a bit non-obvious because we don't usually design these pieces as libraries to be consumed outside clangd (either by code calling into clangd's internals, or a modified version of clangd). I think we usually won't support major design changes to accommodate these, but small ones like this look totally fine.
Yeah, "clean" can mean various things, e.g. "clearly communicating intent" here is in tension with "fewer moving parts". |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 noSelectorinstance 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