llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clangd Author: David Goldman (DavidGoldman) <details> <summary>Changes</summary> This 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: - (modified) clang-tools-extra/clangd/ClangdLSPServer.cpp (+5-2) - (modified) clang-tools-extra/clangd/ClangdLSPServer.h (+1-1) - (modified) clang-tools-extra/clangd/Protocol.cpp (+10) - (modified) clang-tools-extra/clangd/Protocol.h (+8) - (modified) clang-tools-extra/clangd/SourceCode.cpp (+8) - (modified) clang-tools-extra/clangd/SourceCode.h (+4) - (modified) clang-tools-extra/clangd/index/SymbolCollector.cpp (+6-1) - (modified) clang-tools-extra/clangd/refactor/Rename.cpp (+283-38) - (modified) clang-tools-extra/clangd/refactor/Rename.h (+37-7) - (modified) clang-tools-extra/clangd/unittests/RenameTests.cpp (+34-14) - (modified) clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp (+44) ``````````diff 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] `````````` </details> https://github.com/llvm/llvm-project/pull/76466 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits