-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Add support for renaming objc methods, even those with multiple selector pieces #76466
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
We'll treat multi-arg methods as spelled once we have full rename support for them.
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clangd Author: David Goldman (DavidGoldman) ChangesThis is based on top of #76410 Patch is 37.52 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/76466.diff 11 Files Affected:
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index a87da252b7a7e9..0e628cfc71c2de 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<std::optional<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..3a60b6e5db86bc 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<std::optional<PrepareRenameResult>>);
void onRename(const RenameParams &, Callback<WorkspaceEdit>);
void onHover(const TextDocumentPositionParams &,
Callback<std::optional<Hover>>);
diff --git a/clang-tools-extra/clangd/Protocol.cpp b/clang-tools-extra/clangd/Protocol.cpp
index a6370649f5ad1c..29b99da27101a8 100644
--- a/clang-tools-extra/clangd/Protocol.cpp
+++ b/clang-tools-extra/clangd/Protocol.cpp
@@ -1187,6 +1187,16 @@ 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) {
+ 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..18ac530e9c1a0f 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 set.
+ std::optional<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
diff --git a/clang-tools-extra/clangd/SourceCode.cpp b/clang-tools-extra/clangd/SourceCode.cpp
index 835038423fdf37..73e12fc7ebde51 100644
--- a/clang-tools-extra/clangd/SourceCode.cpp
+++ b/clang-tools-extra/clangd/SourceCode.cpp
@@ -1243,6 +1243,14 @@ SourceLocation translatePreamblePatchLocation(SourceLocation Loc,
return Loc;
}
+clangd::Range tokenRangeForLoc(SourceLocation TokLoc, const SourceManager &SM, const LangOptions &LangOpts) {
+ auto TokenLength = clang::Lexer::MeasureTokenLength(TokLoc, SM, LangOpts);
+ clangd::Range Result;
+ Result.start = sourceLocToPosition(SM, TokLoc);
+ Result.end = sourceLocToPosition(SM, TokLoc.getLocWithOffset(TokenLength));
+ return Result;
+}
+
clangd::Range rangeTillEOL(llvm::StringRef Code, unsigned HashOffset) {
clangd::Range Result;
Result.end = Result.start = offsetToPosition(Code, HashOffset);
diff --git a/clang-tools-extra/clangd/SourceCode.h b/clang-tools-extra/clangd/SourceCode.h
index a1bb44c1761202..d671e05aedd76e 100644
--- a/clang-tools-extra/clangd/SourceCode.h
+++ b/clang-tools-extra/clangd/SourceCode.h
@@ -338,6 +338,10 @@ inline bool isReservedName(llvm::StringRef Name) {
SourceLocation translatePreamblePatchLocation(SourceLocation Loc,
const SourceManager &SM);
+clangd::Range tokenRangeForLoc(SourceLocation TokLoc,
+ const SourceManager &SM,
+ const LangOptions &LangOpts);
+
/// Returns the range starting at offset and spanning the whole line. Escaped
/// newlines are not handled.
clangd::Range rangeTillEOL(llvm::StringRef Code, unsigned HashOffset);
diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp
index 7ef4b15febad22..7749727fa2ca1b 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();
@@ -183,6 +186,8 @@ bool isSpelled(SourceLocation Loc, const NamedDecl &ND) {
if (clang::Lexer::getRawToken(Loc, Tok, SM, LO))
return false;
auto StrName = Name.getAsString();
+ if (const auto *MD = dyn_cast<ObjCMethodDecl>(&ND))
+ StrName = MD->getSelector().getNameForSlot(0).str();
return clang::Lexer::getSpelling(Tok, SM, LO) == StrName;
}
} // namespace
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index 11f9e4627af760..4f945f74e85000 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -498,7 +498,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,7 +508,8 @@ static bool mayBeValidIdentifier(llvm::StringRef Ident) {
!isAsciiIdentifierStart(Ident.front(), AllowDollar))
return false;
for (char C : Ident) {
- if (llvm::isASCII(C) && !isAsciiIdentifierContinue(C, AllowDollar))
+ if (llvm::isASCII(C) && !isAsciiIdentifierContinue(C, AllowDollar) &&
+ (!AllowColon || C != ':'))
return false;
}
return true;
@@ -525,7 +526,7 @@ std::optional<InvalidName> checkName(const NamedDecl &RenameDecl,
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.
@@ -551,6 +552,7 @@ renameWithinFile(ParsedAST &AST, const NamedDecl &RenameDecl,
const SourceManager &SM = AST.getSourceManager();
tooling::Replacements FilteredChanges;
+ std::vector<SourceLocation> Locs;
for (SourceLocation Loc : findOccurrencesWithinFile(AST, RenameDecl)) {
SourceLocation RenameLoc = Loc;
// We don't rename in any macro bodies, but we allow rename the symbol
@@ -569,8 +571,43 @@ renameWithinFile(ParsedAST &AST, const NamedDecl &RenameDecl,
// }
if (!isInsideMainFile(RenameLoc, SM))
continue;
+ Locs.push_back(RenameLoc);
+ }
+ if (const auto *MD = dyn_cast<ObjCMethodDecl>(&RenameDecl)) {
+ auto Code = SM.getBufferData(SM.getMainFileID());
+ auto RenameIdentifier = MD->getSelector().getNameForSlot(0).str();
+ llvm::SmallVector<llvm::StringRef, 8> NewNames;
+ NewName.split(NewNames, ":");
+ if (NewNames.empty())
+ NewNames.push_back(NewName);
+ std::vector<Range> Ranges;
+ const auto &LangOpts = RenameDecl.getASTContext().getLangOpts();
+ for (const auto &Loc : Locs)
+ Ranges.push_back(tokenRangeForLoc(Loc, SM, LangOpts));
+ auto FilePath = AST.tuPath();
+ auto RenameRanges =
+ adjustRenameRanges(Code, RenameIdentifier,
+ std::move(Ranges),
+ RenameDecl.getASTContext().getLangOpts(),
+ MD->getSelector());
+ 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
+ // entire rename.
+ return error("Lex results don't match the content of file {0} "
+ "(selector handling may be incorrect)",
+ FilePath);
+ }
+ auto RenameEdit =
+ buildRenameEdit(FilePath, Code, *RenameRanges, NewNames);
+ if (!RenameEdit)
+ return error("failed to rename in file {0}: {1}", FilePath,
+ RenameEdit.takeError());
+ return RenameEdit->Replacements;
+ }
+ for (const auto &Loc : Locs) {
if (auto Err = FilteredChanges.add(tooling::Replacement(
- SM, CharSourceRange::getTokenRange(RenameLoc), NewName)))
+ SM, CharSourceRange::getTokenRange(Loc), NewName)))
return std::move(Err);
}
return FilteredChanges;
@@ -681,12 +718,26 @@ renameOutsideFile(const NamedDecl &RenameDecl, llvm::StringRef MainFilePath,
ExpBuffer.getError().message());
continue;
}
+ std::string RenameIdentifier = RenameDecl.getNameAsString();
+ std::optional<Selector> Selector = std::nullopt;
+ 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, ":");
+ }
+ }
+ if (NewNames.empty()) {
+ NewNames.push_back(NewName);
+ }
auto AffectedFileCode = (*ExpBuffer)->getBuffer();
auto RenameRanges =
- adjustRenameRanges(AffectedFileCode, RenameDecl.getNameAsString(),
+ adjustRenameRanges(AffectedFileCode, RenameIdentifier,
std::move(FileAndOccurrences.second),
- RenameDecl.getASTContext().getLangOpts());
+ RenameDecl.getASTContext().getLangOpts(),
+ Selector);
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
@@ -696,7 +747,7 @@ renameOutsideFile(const NamedDecl &RenameDecl, llvm::StringRef MainFilePath,
FilePath);
}
auto RenameEdit =
- buildRenameEdit(FilePath, AffectedFileCode, *RenameRanges, NewName);
+ buildRenameEdit(FilePath, AffectedFileCode, *RenameRanges, NewNames);
if (!RenameEdit)
return error("failed to rename in file {0}: {1}", FilePath,
RenameEdit.takeError());
@@ -724,7 +775,7 @@ bool impliesSimpleEdit(const Position &LHS, const Position &RHS) {
// *line* or *column*, but not both (increases chance of a robust mapping)
void findNearMiss(
std::vector<size_t> &PartialMatch, ArrayRef<Range> IndexedRest,
- ArrayRef<Range> LexedRest, int LexedIndex, int &Fuel,
+ ArrayRef<SymbolRange> LexedRest, int LexedIndex, int &Fuel,
llvm::function_ref<void(const std::vector<size_t> &)> MatchedCB) {
if (--Fuel < 0)
return;
@@ -734,7 +785,7 @@ void findNearMiss(
MatchedCB(PartialMatch);
return;
}
- if (impliesSimpleEdit(IndexedRest.front().start, LexedRest.front().start)) {
+ if (impliesSimpleEdit(IndexedRest.front().start, LexedRest.front().range().start)) {
PartialMatch.push_back(LexedIndex);
findNearMiss(PartialMatch, IndexedRest.drop_front(), LexedRest.drop_front(),
LexedIndex + 1, Fuel, MatchedCB);
@@ -746,6 +797,19 @@ void findNearMiss(
} // namespace
+std::vector<SymbolRange> symbolRanges(const std::vector<Range> Ranges) {
+ std::vector<SymbolRange> Result;
+ for (const auto &R : Ranges)
+ Result.emplace_back(R);
+ return Result;
+}
+
+std::vector<SymbolRange>
+collectRenameIdentifierRanges(
+ llvm::StringRef Identifier, llvm::StringRef Content,
+ const LangOptions &LangOpts,
+ std::optional<Selector> Selector);
+
llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
assert(!RInputs.Index == !RInputs.FS &&
"Index and FS must either both be specified or both null.");
@@ -778,12 +842,25 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
return makeError(ReasonToReject::NoSymbolFound);
if (DeclsUnderCursor.size() > 1)
return makeError(ReasonToReject::AmbiguousSymbol);
+ std::optional<std::string> Placeholder;
const auto &RenameDecl = **DeclsUnderCursor.begin();
- const auto *ID = RenameDecl.getIdentifier();
- if (!ID)
- return makeError(ReasonToReject::UnsupportedSymbol);
- if (ID->getName() == RInputs.NewName)
- return makeError(ReasonToReject::SameName);
+ if (const auto *MD = dyn_cast<ObjCMethodDecl>(&RenameDecl)) {
+ const auto Sel = MD->getSelector();
+ if (Sel.getAsString() == RInputs.NewName)
+ return makeError(ReasonToReject::SameName);
+ if (Sel.getNumArgs() != RInputs.NewName.count(':') &&
+ RInputs.NewName != "__clangd_rename_placeholder")
+ return makeError(
+ InvalidName{InvalidName::BadIdentifier, RInputs.NewName.str()});
+ if (Sel.getNumArgs() > 1)
+ Placeholder = Sel.getAsString();
+ } else {
+ const auto *ID = RenameDecl.getIdentifier();
+ if (!ID)
+ return makeError(ReasonToReject::UnsupportedSymbol);
+ if (ID->getName() == RInputs.NewName)
+ return makeError(ReasonToReject::SameName);
+ }
auto Invalid = checkName(RenameDecl, RInputs.NewName);
if (Invalid)
return makeError(std::move(*Invalid));
@@ -827,6 +904,7 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
}
RenameResult Result;
Result.Target = CurrentIdentifier;
+ Result.Placeholder = Placeholder;
Edit MainFileEdits = Edit(MainFileCode, std::move(*MainFileRenameEdit));
for (const TextEdit &TE : MainFileEdits.asTextEdits())
Result.LocalChanges.push_back(TE.range);
@@ -862,8 +940,8 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
llvm::Expected<Edit> buildRenameEdit(llvm::StringRef AbsFilePath,
llvm::StringRef InitialCode,
- std::vector<Range> Occurrences,
- llvm::StringRef NewName) {
+ std::vector<SymbolRange> Occurrences,
+ llvm::SmallVectorImpl<llvm::StringRef> &NewNames) {
trace::Span Tracer("BuildRenameEdit");
SPAN_ATTACH(Tracer, "file_path", AbsFilePath);
SPAN_ATTACH(Tracer, "rename_occurrences",
@@ -893,22 +971,40 @@ llvm::Expected<Edit> buildRenameEdit(llvm::StringRef AbsFilePath,
return LastOffset;
};
- std::vector<std::pair</*start*/ size_t, /*end*/ size_t>> OccurrencesOffsets;
- for (const auto &R : Occurrences) {
- auto StartOffset = Offset(R.start);
- if (!StartOffset)
- return StartOffset.takeError();
- auto EndOffset = Offset(R.end);
- if (!EndOffset)
- return EndOffset.takeError();
- OccurrencesOffsets.push_back({*StartOffset, *EndOffset});
+ struct OccurrenceOffset {
+ size_t Start;
+ size_t End;
+ llvm::StringRef NewName;
+
+ OccurrenceOffset(size_t Start, size_t End, llvm::StringRef NewName) :
+ Start(Start), End(End), NewName(NewName) {}
+ };
+
+ std::vector<OccurrenceOffset> OccurrencesOffsets;
+ for (const auto &SR : Occurrences) {
+ for (auto It = SR.Ranges.begin(); It != SR.Ranges.end(); ++It) {
+ const auto &R = *It;
+ auto StartOffset = Offset(R.start);
+ if (!StartOffset)
+ return StartOffset.takeError();
+ auto EndOffset = Offset(R.end);
+ if (!EndOffset)
+ return EndOffset.takeError();
+ auto Index = It - SR.Ranges.begin();
+ // Nothing to do if the token/name hasn't changed.
+ auto CurName = InitialCode.substr(*StartOffset, *EndOffset - *StartOffset);
+ if (CurName == NewNames[Index])
+ continue;
+ OccurrencesOffsets.emplace_back(*StartOffset, *EndOffset,
+ NewNames[Index]);
+ }
}
tooling::Replacements RenameEdit;
for (const auto &R : OccurrencesOffsets) {
- auto ByteLength = R.second - R.first;
+ auto ByteLength = R.End - R.Start;
if (auto Err = RenameEdit.add(
- tooling::Replacement(AbsFilePath, R.first, ByteLength, NewName)))
+ tooling::Replacement(AbsFilePath, R.Start, ByteLength, R.NewName)))
return std::move(Err);
}
return Edit(InitialCode, std::move(RenameEdit));
@@ -927,20 +1023,21 @@ llvm::Expected<Edit> buildRenameEdit(llvm::StringRef AbsFilePath,
// ranges onto candidates in a plausible way (e.g. guess that lines
// were inserted). If such a "near miss" is found, the rename is still
// possible
-std::optional<std::vector<Range>>
+std::optional<std::vector<SymbolRange>>
adjustRenameRanges(llvm::StringRef DraftCode, llvm::StringRef Identifier,
- std::vector<Range> Indexed, const LangOptions &LangOpts) {
+ std::vector<Range> Indexed, const LangOptions &LangOpts,
+ std::optional<Selector> Selector) {
trace::Span Tracer("AdjustRenameRanges");
assert(!Indexed.empty());
assert(llvm::is_sorted(Indexed));
- std::vector<Range> Lexed =
- collectIdentifierRanges(Identifier, DraftCode, LangOpts);
+ std::vector<SymbolRange> Lexed =
+ collectRenameIdentifierRanges(Identifier, DraftCode, LangOpts, Selector);
llvm::sort(Lexed);
return getMappedRanges(Indexed, Lexed);
}
-std::optional<std::vector<Range>> getMappedRanges(ArrayRef<Range> Indexed,
- ArrayRef<Range> Lexed) {
+std::optional<std::vector<SymbolRange>> getMappedRanges(ArrayRef<Range> Indexed,
+ ArrayRef<SymbolRange> Lexed) {
trace::Span Tracer("GetMappedRanges");
assert(!Indexed.empty());
assert(llvm::is_sorted(Indexed));
@@ -955,7 +1052,7 @@ std::optional<std::vector<Range>> getMappedRanges(ArrayRef<Range> Indexed,
}
// Fast check for the special subset case.
if (std::includes(Indexed.begin(), Indexed.end(), Lexed.begin(), Lexed.end()))
- return Indexed.vec();
+ return Lexed.vec();
std::vector<size_t> Best;
size_t BestCost = std::numeric_limits<size_t>::max();
@@ -985,7 +1082,7 @@ std::optional<std::vector<Range>> getMappedRanges(ArrayRef<Range> Indexed,
SPAN_ATTACH(Tracer, "error", "Didn't find a near miss");
return std::nullopt;
}
- std::vector<Range> Mapped;
+ std::vector<SymbolRange> Mapped;
for (auto I : Best)
Mapped.push_back(Lexed[I]);
SPAN_ATTACH(Tracer, "mapped_ranges", static_cast<int64_t>(Mapped.size()));
@@ -1007,7 +1104,8 @@ std::optional<std::vector<Range>> getMappedRanges(ArrayRef<Range> Indexed,
// diff[0]: line + 1 <- insert a line before edit 0.
// diff[1]: column + 1 <- remove a line between edits 0 and 1, and insert a
// character on edit 1.
-size_t renameRangeAdjustmentCost(ArrayRef<Range> Indexed, ArrayRef<Range> Lexed,
+size_t renameRangeAdjustmen...
[truncated]
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, this LG in direction. only reviewed the pieces leading to in-file rename so far.
We now support renaming ObjC methods even with multiple selector pieces. This requires clangd to lex each file to discover the selector pieces since the index only stores the location of the initial selector token.
7df3c27
to
2e9b17a
Compare
@@ -696,7 +982,7 @@ renameOutsideFile(const NamedDecl &RenameDecl, llvm::StringRef MainFilePath, | |||
FilePath); | |||
} | |||
auto RenameEdit = | |||
buildRenameEdit(FilePath, AffectedFileCode, *RenameRanges, NewName); | |||
buildRenameEdit(FilePath, AffectedFileCode, *RenameRanges, NewNames); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you also move the logic that splits NewName
into NewNames
here? with a comment explaining why it's done for objc selectors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as discussed yesterday, i was suggesting to move:
llvm::SmallVector<llvm::StringRef, 8> NewNames;
NewName.split(NewNames, ":");
from lines 928/936 to right before this call to buildRenameEdit
std::string RenameIdentifier = RenameDecl.getNameAsString(); | ||
std::optional<Selector> Selector = std::nullopt; | ||
llvm::SmallVector<llvm::StringRef, 8> NewNames; | ||
if (const auto *MD = dyn_cast<ObjCMethodDecl>(&RenameDecl)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we just push this logic all the way into collectRenameIdentifierRanges
?
it already performs a different logic when we have a selector set, we can just tart by setting Identifier = Selector.getNameForSlot(0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A high-level comment. Instead of dealing with a vector of StringRef
for NewNames
, I think we should use SymbolName
. It is already designed to be able to represent Objective-C selectors and that way you could do the name splitting in SymbolName
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd rather leave that to a follow up change so that we can close the loop here, if that's OK.
@@ -508,24 +513,46 @@ static bool mayBeValidIdentifier(llvm::StringRef Ident) { | |||
!isAsciiIdentifierStart(Ident.front(), AllowDollar)) | |||
return false; | |||
for (char C : Ident) { | |||
if (AllowColon && C == ':') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also allow spaces here if you spell the new method name as doSomething: with:
?
Should we also check that the first selector piece is not empty? Ie. that : with:
is not a valid selector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't check the entire selector - only a fragment, so I don't think there's a need to check spaces here. AFAIK selectors can have an empty first fragment too - any idea where that restriction is from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I just assumed that the first selector argument had to be named. I never checked if it can be unnamed.
if (Sel.getNumArgs() != NewName.count(':') && | ||
NewName != "__clangd_rename_placeholder") | ||
return makeError(InvalidName{InvalidName::BadIdentifier, NewName.str()}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems awfully ad-hoc to check for __clangd_rename_placeholder
. I would prefer to change the prepare rename request to just change one of the selector pieces to __clangd_rename_placeholder
.
I’ve been doing this here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we want a proper solution, i think it's better to signal that this is a prepare-rename request via RenameInputs
. (but this change is already hard to review, so i'd rather see that as a follow up)
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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be simplified to
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(); | |
return isSelectorLike(Cur, Next) && Cur.text(SM) == SelectorName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Cur.endLocation() == Next.location(); | ||
} | ||
|
||
bool parseMessageExpression(llvm::ArrayRef<syntax::Token> Tokens, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a doc comment of what this method does on a high level would be very helpful.
if (const auto *MD = dyn_cast<ObjCMethodDecl>(&RenameDecl)) | ||
return renameObjCMethodWithinFile(AST, MD, NewName, std::move(Locs)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is a zero-arg or one-arg Objective-C selector, I think we can still use the old, faster path that doesn’t need to scan the source file for Objective-C selectors using renameObjCMethodWithinFile
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM, I'll look into this in a follow up (+ add some tests for that).
unsigned NumArgs = Selector->getNumArgs(); | ||
|
||
std::vector<Range> SelectorPieces; | ||
auto Tokens = syntax::tokenize(SM.getMainFileID(), SM, LangOpts); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When calling collectRenameIdentifierRanges
from renameObjCMethodWithinFile
, we already have a ParsedAST
and we shouldn’t need to re-parse the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true, although a bit annoying since we need a SM to fetch this and I wouldn't want to force that into the caller. I could pass an optional Array ref of tokens to use to help avoid this, WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think it’s too bad to force that onto the caller. AFAICT we have two calls to collectRenameIdentifierRanges
at the moment, one of which already has the tokens and one doesn’t. So it doesn’t make sense to optimize for the case where the tokens aren’t present. FWIW in my PR I introduced UnexpandedTokenBuffer
for that reason and creating it is a one-liner.
Ranges.push_back(tokenRangeForLoc(AST, Loc, SM, LangOpts)); | ||
auto FilePath = AST.tuPath(); | ||
auto RenameRanges = collectRenameIdentifierRanges( | ||
RenameIdentifier, Code, LangOpts, MD->getSelector()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I missing something here or is this doing a purely syntactic rename, ignoring SelectorOccurrences
altogether?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, initially I did have a custom AST visitor to do a proper semantic rename but we decided to stick with the same approach as the global rename.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems quite error-prone to me. If we have index data that tells us where the selectors appear, we should be using it in my opinion – both for local and global rename.
std::vector<Range> &SelectorPieces) { | ||
|
||
unsigned NumArgs = Sel.getNumArgs(); | ||
llvm::SmallVector<char, 8> Closes; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn’t it be cleaner if Closes
is a vector of token kinds? I think then you could also just check something like the following instead of repeating it in the switch
.
if (!Closes.empty() && Closes.back() == Tok.kind()) {
Closes.pop_back();
continue;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
/// the symbol are split, e.g. ObjC selectors. | ||
std::vector<Range> Ranges; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to clarify if
- These ranges are only the selector piece names or also the colons
- If an empty selector piece is represented by an empty range in here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, I think we might need to improve the empty selector case - I'll do that in a follow up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could also include my tests from https://github.com/llvm/llvm-project/pull/78872/files#diff-26ff7c74af8a1d882abbad43625d3cd4bf8d024ef5c7064993f5ede3eec8752eR834
More tests never hurt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool stuff @DavidGoldman! Nice to see ObjC getting some love. It's a bummer we duplicated here, but seems like both you and @ahoppen took fairly similar approaches. The main difference looks to be the use of SymbolName
in the other PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks a lot for bearing with me @DavidGoldman !
if (Sel.getNumArgs() != NewName.count(':') && | ||
NewName != "__clangd_rename_placeholder") | ||
return makeError(InvalidName{InvalidName::BadIdentifier, NewName.str()}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we want a proper solution, i think it's better to signal that this is a prepare-rename request via RenameInputs
. (but this change is already hard to review, so i'd rather see that as a follow up)
@@ -696,7 +982,7 @@ renameOutsideFile(const NamedDecl &RenameDecl, llvm::StringRef MainFilePath, | |||
FilePath); | |||
} | |||
auto RenameEdit = | |||
buildRenameEdit(FilePath, AffectedFileCode, *RenameRanges, NewName); | |||
buildRenameEdit(FilePath, AffectedFileCode, *RenameRanges, NewNames); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
up
std::string RenameIdentifier = RenameDecl.getNameAsString(); | ||
std::optional<Selector> Selector = std::nullopt; | ||
llvm::SmallVector<llvm::StringRef, 8> NewNames; | ||
if (const auto *MD = dyn_cast<ObjCMethodDecl>(&RenameDecl)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd rather leave that to a follow up change so that we can close the loop here, if that's OK.
Thanks, PTAL, I'll save the remaining comments for follow ups. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The outstanding comments from last review are:
- Use
SymbolName
instead ofStringRef
- Don’t re-lex the source file in
collectRenameIdentifierRanges
- Use index or AST data to find edits in the current file. AFAICT we do use index-data for the multi-file rename case in
adjustRenameRanges
but I AFAICT no semantic information is used for the current file.- https://github.com/llvm/llvm-project/pull/76466/files#r1479029824
- I think a good test case to add would be
// Input
R"cpp(
@interface Foo
- (void)performA^ction:(int)action with:(int)value;
@end
@implementation Foo
- (void)performAction:(int)action with:(int)value {
[self performAction:action with:value];
}
@end
@interface Bar
- (void)performAction:(int)action with:(int)value;
@end
@implementation Bar
- (void)performAction:(int)action with:(int)value {
}
@end
)cpp",
// New name
"performNewAction:by:",
// Expected
R"cpp(
@interface Foo
- (void)performNewAction:(int)action by:(int)value;
@end
@implementation Foo
- (void)performNewAction:(int)action by:(int)value {
[self performNewAction:action by:value];
}
@end
@interface Bar
- (void)performAction:(int)action with:(int)value;
@end
@implementation Bar
- (void)performAction:(int)action with:(int)value {
}
)cpp",
// We require the selector name and : to be contiguous. | ||
// e.g. support `foo:` but not `foo :`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don’t we support foo :
? Or what am I missing here? [bar foo : 1]
is perfectly valid AFAICT.
} | ||
|
||
// Scan through Tokens to find ranges for each selector fragment in Sel at the | ||
// top level (not nested in any () or {} or []). The search will terminate upon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the not nested in any () or {} or []
is quite misleading. Based on this comment, I would have expected doStuff
in [someObject someArgLabel: [foo doStuff: 1]]
or
- (void) test {
[foo doStuff: 1];
}
to not be found because it’s nested in {}
or []
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but that's true for this function, as it's achieved by the outer loop that calls this function in collectRenameIdentifierRanges
.
in here we're only trying to match Sel
assuming its first segment is located at Tokens.front()
.
Ranges.emplace_back(R); | ||
return Ranges; | ||
} | ||
// FIXME: InMemoryFileAdapter crashes unless the buffer is null terminated! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my rename PR I found that this is no longer an issue. See swiftlang#7915 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, lgtm!
} | ||
|
||
// Scan through Tokens to find ranges for each selector fragment in Sel at the | ||
// top level (not nested in any () or {} or []). The search will terminate upon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but that's true for this function, as it's achieved by the outer loop that calls this function in collectRenameIdentifierRanges
.
in here we're only trying to match Sel
assuming its first segment is located at Tokens.front()
.
@@ -696,7 +982,7 @@ renameOutsideFile(const NamedDecl &RenameDecl, llvm::StringRef MainFilePath, | |||
FilePath); | |||
} | |||
auto RenameEdit = | |||
buildRenameEdit(FilePath, AffectedFileCode, *RenameRanges, NewName); | |||
buildRenameEdit(FilePath, AffectedFileCode, *RenameRanges, NewNames); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as discussed yesterday, i was suggesting to move:
llvm::SmallVector<llvm::StringRef, 8> NewNames;
NewName.split(NewNames, ":");
from lines 928/936 to right before this call to buildRenameEdit
This is based on top of #76410