llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clangd @llvm/pr-subscribers-clang Author: Alex Hoppen (ahoppen) <details> <summary>Changes</summary> This adds support to rename Objective-C method declarations and calls to clangd. --- Patch is 54.98 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/78872.diff 24 Files Affected: - (modified) clang-tools-extra/clangd/ClangdLSPServer.cpp (+3-2) - (modified) clang-tools-extra/clangd/ClangdLSPServer.h (+1-1) - (modified) clang-tools-extra/clangd/ClangdServer.cpp (+1-2) - (modified) clang-tools-extra/clangd/Protocol.cpp (+4) - (modified) clang-tools-extra/clangd/Protocol.h (+11) - (modified) clang-tools-extra/clangd/SourceCode.cpp (+9-9) - (modified) clang-tools-extra/clangd/SourceCode.h (+3-3) - (modified) clang-tools-extra/clangd/index/SymbolCollector.cpp (+24-3) - (modified) clang-tools-extra/clangd/refactor/Rename.cpp (+110-37) - (modified) clang-tools-extra/clangd/refactor/Rename.h (+26-13) - (modified) clang-tools-extra/clangd/unittests/RenameTests.cpp (+342-9) - (modified) clang-tools-extra/clangd/unittests/SourceCodeTests.cpp (+2-2) - (modified) clang/include/clang/Tooling/Refactoring/Rename/RenamingAction.h (+21) - (modified) clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h (+40-10) - (modified) clang/include/clang/Tooling/Syntax/Tokens.h (+13) - (modified) clang/lib/Tooling/Refactoring/CMakeLists.txt (+2) - (modified) clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp (+96-2) - (modified) clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp (+2-2) - (added) clang/lib/Tooling/Refactoring/SymbolName.cpp (+70) - (modified) clang/lib/Tooling/Syntax/Tokens.cpp (+7) - (modified) clang/tools/clang-refactor/CMakeLists.txt (+1) - (modified) clang/tools/clang-rename/CMakeLists.txt (+1) - (modified) clang/tools/libclang/CMakeLists.txt (+1) - (modified) clang/unittests/Tooling/RefactoringActionRulesTest.cpp (+3-3) ``````````diff diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp index a87da252b7a7e9b..5dfd12045b65738 100644 --- a/clang-tools-extra/clangd/ClangdLSPServer.cpp +++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp @@ -844,14 +844,15 @@ void ClangdLSPServer::onWorkspaceSymbol( } void ClangdLSPServer::onPrepareRename(const TextDocumentPositionParams &Params, - Callback<std::optional<Range>> Reply) { + Callback<PrepareRenameResult> Reply) { Server->prepareRename( Params.textDocument.uri.file(), Params.position, /*NewName*/ std::nullopt, Opts.Rename, [Reply = std::move(Reply)](llvm::Expected<RenameResult> Result) mutable { if (!Result) return Reply(Result.takeError()); - return Reply(std::move(Result->Target)); + PrepareRenameResult PrepareResult{Result->Target, Result->OldName}; + return Reply(std::move(PrepareResult)); }); } diff --git a/clang-tools-extra/clangd/ClangdLSPServer.h b/clang-tools-extra/clangd/ClangdLSPServer.h index 79579c22b788a9c..6a9f097d551ae6d 100644 --- a/clang-tools-extra/clangd/ClangdLSPServer.h +++ b/clang-tools-extra/clangd/ClangdLSPServer.h @@ -134,7 +134,7 @@ class ClangdLSPServer : private ClangdServer::Callbacks, void onWorkspaceSymbol(const WorkspaceSymbolParams &, Callback<std::vector<SymbolInformation>>); void onPrepareRename(const TextDocumentPositionParams &, - Callback<std::optional<Range>>); + Callback<PrepareRenameResult>); void onRename(const RenameParams &, Callback<WorkspaceEdit>); void onHover(const TextDocumentPositionParams &, Callback<std::optional<Hover>>); diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp index 6fb2641e8793db1..b04ebc7049c6619 100644 --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -578,8 +578,7 @@ void ClangdServer::prepareRename(PathRef File, Position Pos, // prepareRename is latency-sensitive: we don't query the index, as we // only need main-file references auto Results = - clangd::rename({Pos, NewName.value_or("__clangd_rename_placeholder"), - InpAST->AST, File, /*FS=*/nullptr, + clangd::rename({Pos, NewName, InpAST->AST, File, /*FS=*/nullptr, /*Index=*/nullptr, RenameOpts}); if (!Results) { // LSP says to return null on failure, but that will result in a generic diff --git a/clang-tools-extra/clangd/Protocol.cpp b/clang-tools-extra/clangd/Protocol.cpp index a6370649f5ad1cf..0291e5d71d65c88 100644 --- a/clang-tools-extra/clangd/Protocol.cpp +++ b/clang-tools-extra/clangd/Protocol.cpp @@ -905,6 +905,10 @@ llvm::json::Value toJSON(const DocumentSymbol &S) { return std::move(Result); } +llvm::json::Value toJSON(const PrepareRenameResult &R) { + return llvm::json::Object{{"range", R.range}, {"placeholder", R.placeholder}}; +} + llvm::json::Value toJSON(const WorkspaceEdit &WE) { llvm::json::Object Result; if (WE.changes) { diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h index e88c804692568f5..274c7fe4391062b 100644 --- a/clang-tools-extra/clangd/Protocol.h +++ b/clang-tools-extra/clangd/Protocol.h @@ -1004,6 +1004,17 @@ struct CodeActionParams { }; bool fromJSON(const llvm::json::Value &, CodeActionParams &, llvm::json::Path); +struct PrepareRenameResult { + /// The range of the string to rename. + Range range; + /// A placeholder text of the string content to be renamed. + /// + /// This is usueful to populate the rename field with an Objective-C selector + /// name (eg. `performAction:with:`) when renaming Objective-C methods. + std::string placeholder; +}; +llvm::json::Value toJSON(const PrepareRenameResult &WE); + /// The edit should either provide changes or documentChanges. If the client /// can handle versioned document edits and if documentChanges are present, /// the latter are preferred over changes. diff --git a/clang-tools-extra/clangd/SourceCode.cpp b/clang-tools-extra/clangd/SourceCode.cpp index 835038423fdf372..0081a69357b02f4 100644 --- a/clang-tools-extra/clangd/SourceCode.cpp +++ b/clang-tools-extra/clangd/SourceCode.cpp @@ -625,16 +625,16 @@ llvm::StringMap<unsigned> collectIdentifiers(llvm::StringRef Content, return Identifiers; } -std::vector<Range> collectIdentifierRanges(llvm::StringRef Identifier, - llvm::StringRef Content, - const LangOptions &LangOpts) { +std::vector<Range> +collectIdentifierRanges(llvm::StringRef Identifier, + const syntax::UnexpandedTokenBuffer &Tokens) { std::vector<Range> Ranges; - lex(Content, LangOpts, - [&](const syntax::Token &Tok, const SourceManager &SM) { - if (Tok.kind() != tok::identifier || Tok.text(SM) != Identifier) - return; - Ranges.push_back(halfOpenToRange(SM, Tok.range(SM).toCharRange(SM))); - }); + const SourceManager &SM = Tokens.sourceManager(); + for (const syntax::Token &Tok : Tokens.tokens()) { + if (Tok.kind() != tok::identifier || Tok.text(SM) != Identifier) + continue; + Ranges.push_back(halfOpenToRange(SM, Tok.range(SM).toCharRange(SM))); + } return Ranges; } diff --git a/clang-tools-extra/clangd/SourceCode.h b/clang-tools-extra/clangd/SourceCode.h index a1bb44c17612021..f60683fc4f21c35 100644 --- a/clang-tools-extra/clangd/SourceCode.h +++ b/clang-tools-extra/clangd/SourceCode.h @@ -217,9 +217,9 @@ llvm::StringMap<unsigned> collectIdentifiers(llvm::StringRef Content, const format::FormatStyle &Style); /// Collects all ranges of the given identifier in the source code. -std::vector<Range> collectIdentifierRanges(llvm::StringRef Identifier, - llvm::StringRef Content, - const LangOptions &LangOpts); +std::vector<Range> +collectIdentifierRanges(llvm::StringRef Identifier, + const syntax::UnexpandedTokenBuffer &Tokens); /// Collects words from the source code. /// Unlike collectIdentifiers: diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp index bf838e53f2a21e7..24bd3c426b3e86b 100644 --- a/clang-tools-extra/clangd/index/SymbolCollector.cpp +++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp @@ -173,9 +173,20 @@ std::optional<RelationKind> indexableRelation(const index::SymbolRelation &R) { bool isSpelled(SourceLocation Loc, const NamedDecl &ND) { auto Name = ND.getDeclName(); const auto NameKind = Name.getNameKind(); - if (NameKind != DeclarationName::Identifier && - NameKind != DeclarationName::CXXConstructorName) + bool PrefixComparison; + switch (NameKind) { + case DeclarationName::Identifier: + case DeclarationName::CXXConstructorName: + case DeclarationName::ObjCZeroArgSelector: + PrefixComparison = false; + break; + case DeclarationName::ObjCOneArgSelector: + case DeclarationName::ObjCMultiArgSelector: + PrefixComparison = true; + break; + default: return false; + } const auto &AST = ND.getASTContext(); const auto &SM = AST.getSourceManager(); const auto &LO = AST.getLangOpts(); @@ -183,7 +194,17 @@ bool isSpelled(SourceLocation Loc, const NamedDecl &ND) { if (clang::Lexer::getRawToken(Loc, Tok, SM, LO)) return false; auto StrName = Name.getAsString(); - return clang::Lexer::getSpelling(Tok, SM, LO) == StrName; + std::string LexerSpelling = clang::Lexer::getSpelling(Tok, SM, LO); + if (PrefixComparison) { + // The lexer spelling at the source location is only the first label of an + // Objective-C selector, eg. if `StrName` is `performAction:with:`, then the + // token at the requested location is `performAction`. Re-building the + // entire selector from the lexer is too complicated here, so just perform + // a prefix comparison. + return StringRef(StrName).starts_with(LexerSpelling); + } else { + return StrName == LexerSpelling; + } } } // namespace diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp index 11f9e4627af7609..f639cbc9b4b1273 100644 --- a/clang-tools-extra/clangd/refactor/Rename.cpp +++ b/clang-tools-extra/clangd/refactor/Rename.cpp @@ -26,6 +26,8 @@ #include "clang/Basic/CharInfo.h" #include "clang/Basic/LLVM.h" #include "clang/Basic/SourceLocation.h" +#include "clang/Tooling/Refactoring/Rename/RenamingAction.h" +#include "clang/Tooling/Refactoring/Rename/SymbolName.h" #include "clang/Tooling/Syntax/Tokens.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/StringExtras.h" @@ -40,6 +42,8 @@ namespace clang { namespace clangd { namespace { +using tooling::SymbolName; + std::optional<std::string> filePath(const SymbolLocation &Loc, llvm::StringRef HintFilePath) { if (!Loc) @@ -517,22 +521,27 @@ static bool mayBeValidIdentifier(llvm::StringRef Ident) { // Check if we can rename the given RenameDecl into NewName. // Return details if the rename would produce a conflict. std::optional<InvalidName> checkName(const NamedDecl &RenameDecl, - llvm::StringRef NewName) { + const SymbolName &NewName) { trace::Span Tracer("CheckName"); static constexpr trace::Metric InvalidNameMetric( "rename_name_invalid", trace::Metric::Counter, "invalid_kind"); auto &ASTCtx = RenameDecl.getASTContext(); + auto Identifier = NewName.getSinglePiece(); + if (!Identifier) { + return std::nullopt; + } std::optional<InvalidName> Result; - if (isKeyword(NewName, ASTCtx.getLangOpts())) - Result = InvalidName{InvalidName::Keywords, NewName.str()}; - else if (!mayBeValidIdentifier(NewName)) - Result = InvalidName{InvalidName::BadIdentifier, NewName.str()}; - else { + if (isKeyword(*Identifier, ASTCtx.getLangOpts())) { + Result = InvalidName{InvalidName::Keywords, *Identifier}; + } else if (!mayBeValidIdentifier(*Identifier)) { + Result = InvalidName{InvalidName::BadIdentifier, *Identifier}; + } else { // Name conflict detection. // Function conflicts are subtle (overloading), so ignore them. if (RenameDecl.getKind() != Decl::Function && RenameDecl.getKind() != Decl::CXXMethod) { - if (auto *Conflict = lookupSiblingWithName(ASTCtx, RenameDecl, NewName)) + if (auto *Conflict = + lookupSiblingWithName(ASTCtx, RenameDecl, *Identifier)) Result = InvalidName{ InvalidName::Conflict, Conflict->getLocation().printToString(ASTCtx.getSourceManager())}; @@ -546,7 +555,7 @@ std::optional<InvalidName> checkName(const NamedDecl &RenameDecl, // AST-based rename, it renames all occurrences in the main file. llvm::Expected<tooling::Replacements> renameWithinFile(ParsedAST &AST, const NamedDecl &RenameDecl, - llvm::StringRef NewName) { + const SymbolName &OldName, const SymbolName &NewName) { trace::Span Tracer("RenameWithinFile"); const SourceManager &SM = AST.getSourceManager(); @@ -569,9 +578,30 @@ renameWithinFile(ParsedAST &AST, const NamedDecl &RenameDecl, // } if (!isInsideMainFile(RenameLoc, SM)) continue; - if (auto Err = FilteredChanges.add(tooling::Replacement( - SM, CharSourceRange::getTokenRange(RenameLoc), NewName))) - return std::move(Err); + if (std::optional<std::string> Identifier = NewName.getSinglePiece()) { + tooling::Replacement NewReplacement( + SM, CharSourceRange::getTokenRange(RenameLoc), *Identifier); + if (auto Err = FilteredChanges.add(NewReplacement)) { + return std::move(Err); + } + continue; + } + SmallVector<SourceLocation> PieceLocations; + llvm::Error Error = findObjCSymbolSelectorPieces( + AST.getTokens().expandedTokens(), SM, RenameLoc, OldName, + tooling::ObjCSymbolSelectorKind::Unknown, PieceLocations); + if (Error) { + // Ignore the error. We simply skip over all selectors that didn't match. + consumeError(std::move(Error)); + continue; + } + for (auto [Location, NewPiece] : + llvm::zip_equal(PieceLocations, NewName.getNamePieces())) { + tooling::Replacement NewReplacement( + SM, CharSourceRange::getTokenRange(Location), NewPiece); + if (auto Err = FilteredChanges.add(NewReplacement)) + return std::move(Err); + } } return FilteredChanges; } @@ -664,8 +694,9 @@ findOccurrencesOutsideFile(const NamedDecl &RenameDecl, // there is no dirty buffer. llvm::Expected<FileEdits> renameOutsideFile(const NamedDecl &RenameDecl, llvm::StringRef MainFilePath, - llvm::StringRef NewName, const SymbolIndex &Index, - size_t MaxLimitFiles, llvm::vfs::FileSystem &FS) { + SymbolName OldName, SymbolName NewName, + const SymbolIndex &Index, size_t MaxLimitFiles, + llvm::vfs::FileSystem &FS) { trace::Span Tracer("RenameOutsideFile"); auto AffectedFiles = findOccurrencesOutsideFile(RenameDecl, MainFilePath, Index, MaxLimitFiles); @@ -683,10 +714,11 @@ renameOutsideFile(const NamedDecl &RenameDecl, llvm::StringRef MainFilePath, } auto AffectedFileCode = (*ExpBuffer)->getBuffer(); - auto RenameRanges = - adjustRenameRanges(AffectedFileCode, RenameDecl.getNameAsString(), - std::move(FileAndOccurrences.second), - RenameDecl.getASTContext().getLangOpts()); + syntax::UnexpandedTokenBuffer Tokens( + AffectedFileCode, RenameDecl.getASTContext().getLangOpts()); + std::optional<std::vector<Range>> RenameRanges = + adjustRenameRanges(Tokens, OldName.getNamePieces().front(), + std::move(FileAndOccurrences.second)); 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 @@ -695,8 +727,8 @@ renameOutsideFile(const NamedDecl &RenameDecl, llvm::StringRef MainFilePath, "(the index may be stale)", FilePath); } - auto RenameEdit = - buildRenameEdit(FilePath, AffectedFileCode, *RenameRanges, NewName); + auto RenameEdit = buildRenameEdit(FilePath, AffectedFileCode, *RenameRanges, + OldName, NewName, Tokens); if (!RenameEdit) return error("failed to rename in file {0}: {1}", FilePath, RenameEdit.takeError()); @@ -779,12 +811,25 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) { if (DeclsUnderCursor.size() > 1) return makeError(ReasonToReject::AmbiguousSymbol); const auto &RenameDecl = **DeclsUnderCursor.begin(); - const auto *ID = RenameDecl.getIdentifier(); - if (!ID) + DeclarationName Name = RenameDecl.getDeclName(); + if (!Name) return makeError(ReasonToReject::UnsupportedSymbol); - if (ID->getName() == RInputs.NewName) + SymbolName OldSymbolName(Name); + SymbolName NewSymbolName; + if (RInputs.NewName) { + NewSymbolName = SymbolName(*RInputs.NewName, AST.getLangOpts()); + } else { + // If no new name is given, we are perfoming a pseudo rename for the + // prepareRename request to check if the rename is possible. Construct a + // new symbol name that has as many name pieces as the old name and is thus + // a valid new name. + std::vector<std::string> NewNamePieces = OldSymbolName.getNamePieces(); + NewNamePieces[0] += "__clangd_rename_placeholder"; + NewSymbolName = SymbolName(NewNamePieces); + } + if (OldSymbolName == NewSymbolName) return makeError(ReasonToReject::SameName); - auto Invalid = checkName(RenameDecl, RInputs.NewName); + auto Invalid = checkName(RenameDecl, NewSymbolName); if (Invalid) return makeError(std::move(*Invalid)); @@ -802,7 +847,8 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) { // To make cross-file rename work for local symbol, we use a hybrid solution: // - run AST-based rename on the main file; // - run index-based rename on other affected files; - auto MainFileRenameEdit = renameWithinFile(AST, RenameDecl, RInputs.NewName); + auto MainFileRenameEdit = + renameWithinFile(AST, RenameDecl, OldSymbolName, NewSymbolName); if (!MainFileRenameEdit) return MainFileRenameEdit.takeError(); @@ -827,6 +873,7 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) { } RenameResult Result; Result.Target = CurrentIdentifier; + Result.OldName = RenameDecl.getNameAsString(); Edit MainFileEdits = Edit(MainFileCode, std::move(*MainFileRenameEdit)); for (const TextEdit &TE : MainFileEdits.asTextEdits()) Result.LocalChanges.push_back(TE.range); @@ -847,7 +894,8 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) { } auto OtherFilesEdits = renameOutsideFile( - RenameDecl, RInputs.MainFilePath, RInputs.NewName, *RInputs.Index, + RenameDecl, RInputs.MainFilePath, OldSymbolName, NewSymbolName, + *RInputs.Index, Opts.LimitFiles == 0 ? std::numeric_limits<size_t>::max() : Opts.LimitFiles, *RInputs.FS); @@ -860,10 +908,11 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) { return Result; } -llvm::Expected<Edit> buildRenameEdit(llvm::StringRef AbsFilePath, - llvm::StringRef InitialCode, - std::vector<Range> Occurrences, - llvm::StringRef NewName) { +llvm::Expected<Edit> +buildRenameEdit(llvm::StringRef AbsFilePath, llvm::StringRef InitialCode, + std::vector<Range> Occurrences, SymbolName OldName, + SymbolName NewName, + const syntax::UnexpandedTokenBuffer &Tokens) { trace::Span Tracer("BuildRenameEdit"); SPAN_ATTACH(Tracer, "file_path", AbsFilePath); SPAN_ATTACH(Tracer, "rename_occurrences", @@ -904,12 +953,36 @@ llvm::Expected<Edit> buildRenameEdit(llvm::StringRef AbsFilePath, OccurrencesOffsets.push_back({*StartOffset, *EndOffset}); } + const SourceManager &SM = Tokens.sourceManager(); + tooling::Replacements RenameEdit; for (const auto &R : OccurrencesOffsets) { - auto ByteLength = R.second - R.first; - if (auto Err = RenameEdit.add( - tooling::Replacement(AbsFilePath, R.first, ByteLength, NewName))) - return std::move(Err); + if (std::optional<std::string> Identifier = NewName.getSinglePiece()) { + auto ByteLength = R.second - R.first; + if (auto Err = RenameEdit.add(tooling::Replacement( + AbsFilePath, R.first, ByteLength, *Identifier))) + return std::move(Err); + } else { + SmallVector<SourceLocation> PieceLocations; + llvm::Error Error = findObjCSymbolSelectorPieces( + Tokens.tokens(), SM, + SM.getLocForStartOfFile(SM.getMainFileID()).getLocWithOffset(R.first), + OldName, tooling::ObjCSymbolSelectorKind::Unknown, PieceLocations); + if (Error) { + // Ignore the error. We simply skip over all selectors that didn't + // match. + consumeError(std::move(Error)); + continue; + } + assert(PieceLocations.size() == NewName.getNamePieces().size()); + for (auto [Location, NewPiece] : + llvm::zip_equal(PieceLocations, NewName.getNamePieces())) { + tooling::Replacement NewReplacement( + SM, CharSourceRange::getTokenRange(Location), NewPiece); + if (auto Err = RenameEdit.add(NewReplacement)) + return std::move(Err); + } + } } return Edit(InitialCode, std::move(RenameEdit)); } @@ -928,13 +1001,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<Range>> -adjustRenameRanges(llvm::St... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/78872 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
