diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp --- a/clang-tools-extra/clangd/ClangdLSPServer.cpp +++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp @@ -103,13 +103,13 @@ return LookupTable; } -// Makes sure edits in \p E are applicable to latest file contents reported by +// Makes sure edits in \p FE are applicable to latest file contents reported by // editor. If not generates an error message containing information about files // that needs to be saved. -llvm::Error validateEdits(const DraftStore &DraftMgr, const Tweak::Effect &E) { +llvm::Error validateEdits(const DraftStore &DraftMgr, const FileEdits &FE) { size_t InvalidFileCount = 0; llvm::StringRef LastInvalidFile; - for (const auto &It : E.ApplyEdits) { + for (const auto &It : FE) { if (auto Draft = DraftMgr.getDraft(It.first())) { // If the file is open in user's editor, make sure the version we // saw and current version are compatible as this is the text that @@ -704,7 +704,7 @@ if (R->ApplyEdits.empty()) return Reply("Tweak applied."); - if (auto Err = validateEdits(DraftMgr, *R)) + if (auto Err = validateEdits(DraftMgr, R->ApplyEdits)) return Reply(std::move(Err)); WorkspaceEdit WE; @@ -758,17 +758,24 @@ if (!Code) return Reply(llvm::make_error( "onRename called for non-added file", ErrorCode::InvalidParams)); - - Server->rename(File, Params.position, Params.newName, /*WantFormat=*/true, - [File, Code, Params, Reply = std::move(Reply)]( - llvm::Expected> Edits) mutable { - if (!Edits) - return Reply(Edits.takeError()); - - WorkspaceEdit WE; - WE.changes = {{Params.textDocument.uri.uri(), *Edits}}; - Reply(WE); - }); + Server->rename( + File, Params.position, Params.newName, + /*WantFormat=*/true, + [this](PathRef File) { return DraftMgr.getDraft(File); }, + [File, Code, Params, Reply = std::move(Reply), + this](llvm::Expected Edits) mutable { + if (!Edits) + return Reply(Edits.takeError()); + if (auto Err = validateEdits(DraftMgr, *Edits)) + return Reply(std::move(Err)); + WorkspaceEdit Result; + Result.changes.emplace(); + for (const auto &Rep : *Edits) { + (*Result.changes)[URI::createFile(Rep.first()).toString()] = + Rep.second.asTextEdits(); + } + Reply(Result); + }); } void ClangdLSPServer::onDocumentDidClose( diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h --- a/clang-tools-extra/clangd/ClangdServer.h +++ b/clang-tools-extra/clangd/ClangdServer.h @@ -23,6 +23,7 @@ #include "index/Background.h" #include "index/FileIndex.h" #include "index/Index.h" +#include "refactor/Rename.h" #include "refactor/Tweak.h" #include "clang/Tooling/CompilationDatabase.h" #include "clang/Tooling/Core/Replacement.h" @@ -132,6 +133,9 @@ /// Enable semantic highlighting features. bool SemanticHighlighting = false; + /// Enable cross-file rename feature. + bool CrossFileRename = false; + /// Returns true if the tweak should be enabled. std::function TweakFilter = [](const Tweak &T) { return !T.hidden(); // only enable non-hidden tweaks. @@ -251,7 +255,8 @@ /// embedders could use this method to get all occurrences of the symbol (e.g. /// highlighting them in prepare stage). void rename(PathRef File, Position Pos, llvm::StringRef NewName, - bool WantFormat, Callback> CB); + bool WantFormat, DirtyBufferGetter GetDirtyBuffer, + Callback CB); struct TweakRef { std::string ID; /// ID to pass for applyTweak. @@ -326,6 +331,8 @@ // can be caused by missing includes (e.g. member access in incomplete type). bool SuggestMissingIncludes = false; + bool CrossFileRename = false; + std::function TweakFilter; // GUARDED_BY(CachedCompletionFuzzyFindRequestMutex) diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -119,7 +119,8 @@ : nullptr), GetClangTidyOptions(Opts.GetClangTidyOptions), SuggestMissingIncludes(Opts.SuggestMissingIncludes), - TweakFilter(Opts.TweakFilter), WorkspaceRoot(Opts.WorkspaceRoot), + CrossFileRename(Opts.CrossFileRename), TweakFilter(Opts.TweakFilter), + WorkspaceRoot(Opts.WorkspaceRoot), // Pass a callback into `WorkScheduler` to extract symbols from a newly // parsed file and rebuild the file index synchronously each time an AST // is parsed. @@ -308,16 +309,28 @@ if (!InpAST) return CB(InpAST.takeError()); auto &AST = InpAST->AST; - // Performing the rename isn't substantially more expensive than doing an - // AST-based check, so we just rename and throw away the results. We may - // have to revisit this when we support cross-file rename. - auto Changes = renameWithinFile(AST, File, Pos, "dummy", Index); - if (!Changes) { - // LSP says to return null on failure, but that will result in a generic - // failure message. If we send an LSP error response, clients can surface - // the message to users (VSCode does). - return CB(Changes.takeError()); + // FIXME: for cross-file rename, we presume prepareRename always succeeds, + // revisit this strategy. + if (!CrossFileRename) { + // Performing the local rename isn't substantially more expensive than + // doing an AST-based check, so we just rename and throw away the results. + RenameInputs RInputs; + RInputs.MainFileCode = InpAST->Inputs.Contents; + RInputs.MainFilePath = File; + RInputs.NewName = "dummy"; + RInputs.Pos = Pos; + RInputs.AllowCrossFile = false; + RInputs.Index = Index; + RInputs.VFS = FSProvider.getFileSystem(); + auto Changes = clangd::rename(AST, RInputs); // within-file rename. + if (!Changes) { + // LSP says to return null on failure, but that will result in a generic + // failure message. If we send an LSP error response, clients can + // surface the message to users (VSCode does). + return CB(Changes.takeError()); + } } + SourceLocation Loc = getBeginningOfIdentifier( Pos, AST.getSourceManager(), AST.getASTContext().getLangOpts()); if (auto Range = getTokenRange(AST.getSourceManager(), @@ -330,32 +343,36 @@ } void ClangdServer::rename(PathRef File, Position Pos, llvm::StringRef NewName, - bool WantFormat, Callback> CB) { + bool WantFormat, DirtyBufferGetter GetDirtyBuffer, + Callback CB) { auto Action = [File = File.str(), NewName = NewName.str(), Pos, WantFormat, - CB = std::move(CB), + GetDirtyBuffer = std::move(GetDirtyBuffer), CB = std::move(CB), this](llvm::Expected InpAST) mutable { if (!InpAST) return CB(InpAST.takeError()); - auto Changes = renameWithinFile(InpAST->AST, File, Pos, NewName, Index); - if (!Changes) - return CB(Changes.takeError()); + RenameInputs RInputs; + RInputs.MainFileCode = InpAST->Inputs.Contents; + RInputs.MainFilePath = File; + RInputs.NewName = NewName; + RInputs.Pos = Pos; + RInputs.AllowCrossFile = CrossFileRename; + RInputs.Index = Index; + RInputs.VFS = FSProvider.getFileSystem(); + RInputs.GetDirtyBuffer = std::move(GetDirtyBuffer); + auto Edits = clangd::rename(InpAST->AST, RInputs); + if (!Edits) + return CB(Edits.takeError()); if (WantFormat) { auto Style = getFormatStyleForFile(File, InpAST->Inputs.Contents, InpAST->Inputs.FS.get()); - if (auto Formatted = - cleanupAndFormat(InpAST->Inputs.Contents, *Changes, Style)) - *Changes = std::move(*Formatted); - else - elog("Failed to format replacements: {0}", Formatted.takeError()); + for (auto &E : *Edits) { + if (auto Err = reformatEdit(E.getValue(), Style)) + elog("Failed to format replacements: {0}", std::move(Err)); + } } - - std::vector Edits; - for (const auto &Rep : *Changes) - Edits.push_back(replacementToEdit(InpAST->Inputs.Contents, Rep)); - return CB(std::move(Edits)); + CB(std::move(*Edits)); }; - WorkScheduler.runWithAST("Rename", File, std::move(Action)); } diff --git a/clang-tools-extra/clangd/SourceCode.h b/clang-tools-extra/clangd/SourceCode.h --- a/clang-tools-extra/clangd/SourceCode.h +++ b/clang-tools-extra/clangd/SourceCode.h @@ -223,6 +223,7 @@ /// Checks whether the Replacements are applicable to given Code. bool canApplyTo(llvm::StringRef Code) const; }; +using FileEdits = llvm::StringMap; /// Formats the edits and code around it according to Style. Changes /// Replacements to formatted ones if succeeds. diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp --- a/clang-tools-extra/clangd/index/SymbolCollector.cpp +++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp @@ -263,10 +263,6 @@ Decl::FriendObjectKind::FOK_None) && !(Roles & static_cast(index::SymbolRole::Definition))) return true; - // Skip non-semantic references, we should start processing these when we - // decide to implement renaming with index support. - if ((Roles & static_cast(index::SymbolRole::NameReference))) - return true; // A declaration created for a friend declaration should not be used as the // canonical declaration in the index. Use OrigD instead, unless we've already // picked a replacement for D diff --git a/clang-tools-extra/clangd/refactor/Rename.h b/clang-tools-extra/clangd/refactor/Rename.h --- a/clang-tools-extra/clangd/refactor/Rename.h +++ b/clang-tools-extra/clangd/refactor/Rename.h @@ -9,7 +9,9 @@ #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_RENAME_H #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_RENAME_H +#include "Path.h" #include "Protocol.h" +#include "SourceCode.h" #include "clang/Tooling/Core/Replacement.h" #include "llvm/Support/Error.h" @@ -18,13 +20,27 @@ class ParsedAST; class SymbolIndex; +using DirtyBufferGetter = + std::function(PathRef Path)>; + +struct RenameInputs { + Position Pos; // the position triggering the rename + llvm::StringRef NewName; + + llvm::StringRef MainFilePath; + llvm::StringRef MainFileCode; + + llvm::IntrusiveRefCntPtr VFS; + const SymbolIndex *Index = nullptr; + + bool AllowCrossFile = false; + DirtyBufferGetter GetDirtyBuffer; +}; + /// Renames all occurrences of the symbol at \p Pos to \p NewName. -/// Occurrences outside the current file are not modified. -/// Returns an error if rename a symbol that's used in another file (per the -/// index). -llvm::Expected -renameWithinFile(ParsedAST &AST, llvm::StringRef File, Position Pos, - llvm::StringRef NewName, const SymbolIndex *Index = nullptr); +/// If AllowCrossFile is false, returns an error if rename a symbol that's used +/// in another file (per the index). +llvm::Expected rename(ParsedAST &AST, const RenameInputs &RInputs); } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp --- a/clang-tools-extra/clangd/refactor/Rename.cpp +++ b/clang-tools-extra/clangd/refactor/Rename.cpp @@ -64,56 +64,83 @@ NoSymbolFound, NoIndexProvided, NonIndexable, - UsedOutsideFile, + UsedOutsideFile, // for within-file rename only. UnsupportedSymbol, }; -// Check the symbol Decl is renameable (per the index) within the file. -llvm::Optional renamableWithinFile(const Decl &RenameDecl, - StringRef MainFile, - const SymbolIndex *Index) { +llvm::Optional renameable(const Decl &RenameDecl, + StringRef MainFilePath, + const SymbolIndex *Index, + bool CrossFile) { + // Filter out symbols that are unsupported in both rename mode. if (llvm::isa(&RenameDecl)) return ReasonToReject::UnsupportedSymbol; if (const auto *FD = llvm::dyn_cast(&RenameDecl)) { if (FD->isOverloadedOperator()) return ReasonToReject::UnsupportedSymbol; } - auto &ASTCtx = RenameDecl.getASTContext(); - const auto &SM = ASTCtx.getSourceManager(); - bool MainFileIsHeader = ASTCtx.getLangOpts().IsHeaderFile; - bool DeclaredInMainFile = isInsideMainFile(RenameDecl.getBeginLoc(), SM); - - if (!DeclaredInMainFile) - // We are sure the symbol is used externally, bail out early. - return UsedOutsideFile; - - // If the symbol is declared in the main file (which is not a header), we - // rename it. - if (!MainFileIsHeader) - return None; - - // Below are cases where the symbol is declared in the header. - // If the symbol is function-local, we rename it. + // function-local symbols is safe to rename. if (RenameDecl.getParentFunctionOrMethod()) return None; + bool IsIndexable = + isa(RenameDecl) && + SymbolCollector::shouldCollectSymbol( + cast(RenameDecl), RenameDecl.getASTContext(), + SymbolCollector::Options(), CrossFile); + if (!IsIndexable) // If the symbol is not indexable, we disallow rename. + return ReasonToReject::NonIndexable; + + if (!CrossFile) { + // Within-file rename. + auto &ASTCtx = RenameDecl.getASTContext(); + const auto &SM = ASTCtx.getSourceManager(); + bool MainFileIsHeader = ASTCtx.getLangOpts().IsHeaderFile; + bool DeclaredInMainFile = isInsideMainFile(RenameDecl.getBeginLoc(), SM); + + if (!DeclaredInMainFile) + // We are sure the symbol is used externally, bail out early. + return UsedOutsideFile; + + // If the symbol is declared in the main file (which is not a header), we + // rename it. + if (!MainFileIsHeader) + return None; + + if (!Index) + return NoIndexProvided; + + auto OtherFile = getOtherRefFile(RenameDecl, MainFilePath, *Index); + // If the symbol is indexable and has no refs from other files in the index, + // we rename it. + if (!OtherFile) + return None; + // If the symbol is indexable and has refs from other files in the index, + // we disallow rename. + return ReasonToReject::UsedOutsideFile; + } + + assert(CrossFile); if (!Index) - return ReasonToReject::NoIndexProvided; + return NoIndexProvided; - bool IsIndexable = isa(RenameDecl) && - SymbolCollector::shouldCollectSymbol( - cast(RenameDecl), ASTCtx, {}, false); - // If the symbol is not indexable, we disallow rename. - if (!IsIndexable) - return ReasonToReject::NonIndexable; - auto OtherFile = getOtherRefFile(RenameDecl, MainFile, *Index); - // If the symbol is indexable and has no refs from other files in the index, - // we rename it. - if (!OtherFile) + // FIXME: renaming templates requries to rename all related specializations, + // which is not supported in our index. + if (RenameDecl.getDescribedTemplate()) + return ReasonToReject::UnsupportedSymbol; + + // A whitelist of renamable symbols. + if (llvm::isa(RenameDecl)) return None; - // If the symbol is indexable and has refs from other files in the index, - // we disallow rename. - return ReasonToReject::UsedOutsideFile; + if (const auto *S = llvm::dyn_cast(&RenameDecl)) { + // FIXME: renaming virtual methods requires to rename all overridens in + // subclasses, which our index doesn't support yet. + if (!S->isVirtual()) + return None; + } else if (isa(&RenameDecl)) { + return None; + } + return UnsupportedSymbol; } llvm::Error makeError(ReasonToReject Reason) { @@ -137,7 +164,7 @@ llvm::inconvertibleErrorCode()); } -// Return all rename occurrences in the main file. +// Find all rename occurrences in the main file based on the MainAST. tooling::SymbolOccurrences findOccurrencesWithinFile(ParsedAST &AST, const NamedDecl *RenameDecl) { const NamedDecl *CanonicalRenameDecl = @@ -156,28 +183,10 @@ return Result; } -} // namespace - +// AST-based rename, it renames all occurrences in the main file. llvm::Expected -renameWithinFile(ParsedAST &AST, llvm::StringRef File, Position Pos, - llvm::StringRef NewName, const SymbolIndex *Index) { - const SourceManager &SM = AST.getSourceManager(); - SourceLocation SourceLocationBeg = SM.getMacroArgExpandedLocation( - getBeginningOfIdentifier(Pos, SM, AST.getASTContext().getLangOpts())); - // FIXME: renaming macros is not supported yet, the macro-handling code should - // be moved to rename tooling library. - if (locateMacroAt(SourceLocationBeg, AST.getPreprocessor())) - return makeError(UnsupportedSymbol); - - const auto *RenameDecl = - tooling::getNamedDeclAt(AST.getASTContext(), SourceLocationBeg); - if (!RenameDecl) - return makeError(NoSymbolFound); - - if (auto Reject = - renamableWithinFile(*RenameDecl->getCanonicalDecl(), File, Index)) - return makeError(*Reject); - +renameWithinFile(ParsedAST &AST, const NamedDecl *RenameDecl, + llvm::StringRef NewName) { // Rename sometimes returns duplicate edits (which is a bug). A side-effect of // adding them to a single Replacements object is these are deduplicated. tooling::Replacements FilteredChanges; @@ -198,5 +207,184 @@ return FilteredChanges; } +// Return all rename occurrences (per the index) outside of the main file. +llvm::StringMap> +findOccurrencesOutsideFile(const NamedDecl *RenameDecl, + llvm::StringRef MainFile, const SymbolIndex *Index) { + if (!Index) + return {}; + RefsRequest RQuest; + // FIXME: revisit the limit, and our index should have a way to inform + // us whether the ref results is completed. + RQuest.Limit = 100; + if (auto ID = getSymbolID(RenameDecl)) + RQuest.IDs.insert(*ID); + + // Helper. + auto ToRange = [](const SymbolLocation &L) { + Range R; + R.start.line = L.Start.line(); + R.start.character = L.Start.column(); + R.end.line = L.End.line(); + R.end.character = L.End.column(); + return R; + }; + llvm::StringMap> + AffectedFiles; // absolute file path => rename ocurrences in that file. + Index->refs(RQuest, [&](const Ref &R) { + if (auto RefFilePath = filePath(R.Location, /*HintFilePath=*/MainFile)) { + if (*RefFilePath != MainFile) + AffectedFiles[*RefFilePath].push_back(ToRange(R.Location)); + } + }); + return AffectedFiles; +} + +llvm::Expected> toRangeOffset(const clangd::Range &R, + llvm::StringRef Code) { + auto StartOffset = positionToOffset(Code, R.start); + if (!StartOffset) + return StartOffset.takeError(); + auto EndOffset = positionToOffset(Code, R.end); + if (!EndOffset) + return EndOffset.takeError(); + return std::make_pair<>(*StartOffset, *EndOffset); +}; + +llvm::Expected buildRenameEdit(llvm::StringRef InitialCode, + const std::vector &Occurrences, + llvm::StringRef NewName) { + tooling::Replacements RenameEdit; + for (const Range &Occurrence : Occurrences) { + // !positionToOffset is O(N), it is okay at the moment since we only + // process at most 100 references. + auto RangeOffset = toRangeOffset(Occurrence, InitialCode); + if (!RangeOffset) + return RangeOffset.takeError(); + auto ByteLength = RangeOffset->second - RangeOffset->first; + if (auto Err = RenameEdit.add(tooling::Replacement( + InitialCode, RangeOffset->first, ByteLength, NewName))) + return std::move(Err); + } + return Edit(InitialCode, std::move(RenameEdit)); +} + +// Index-based rename, it renames all occurrences outside of the main file. +llvm::Expected +renameOutsideFile(const NamedDecl *RenameDecl, llvm::StringRef MainFilePath, + llvm::StringRef NewName, const SymbolIndex *Index, + llvm::IntrusiveRefCntPtr VFS, + const DirtyBufferGetter &GetDirtyBuffer) { + auto AffectedFiles = + findOccurrencesOutsideFile(RenameDecl, MainFilePath, Index); + // FIXME: make the limit customizable. + static constexpr size_t MaxLimitFiles = 50; + if (AffectedFiles.size() >= MaxLimitFiles) + return llvm::make_error( + llvm::formatv( + "The number of affected files exceeds the max limit {0}: {1}", + MaxLimitFiles, AffectedFiles.size()), + llvm::inconvertibleErrorCode()); + + // The cross-file rename is purely based on the index, as we don't want to + // build all ASTs for affected files, which may cause a performance hit. + // We choose to trade off some correctness for performance and scalability. + // + // Clangd builds a dynamic index for all opened files on top of the static + // index of the whole codebase. Dynamic index is up-to-date (respects dirty + // buffers) as long as clangd finishes processing opened files, while static + // index (background index) is relatively stale. We choose the dirty buffers + // as the file content we rename on, and fallback to file content on disk if + // there is no dirty buffer. + // + // FIXME: add range patching heuristics to detect staleness of the index, and + // report to users. + // FIXME: our index may return implicit references, which are non-eligitble + // for rename, we should filter out these references. + FileEdits Results; + std::string OldName = RenameDecl->getNameAsString(); + for (const auto &FileAndOccurrences : AffectedFiles) { + llvm::StringRef FilePath = FileAndOccurrences.first(); + + std::string AffectedFileCode; + llvm::Optional DirtyBuffer; + if (GetDirtyBuffer && (DirtyBuffer = GetDirtyBuffer(FilePath))) { + AffectedFileCode = std::move(*DirtyBuffer); + } else { + auto Content = VFS->getBufferForFile(FilePath); + if (!Content) { + elog("Fail to read file {0}: {1}", FilePath, + Content.getError().message()); + continue; + } + if (!*Content) + continue; + AffectedFileCode = (*Content)->getBuffer().str(); + } + auto RenameEdit = buildRenameEdit(AffectedFileCode, + FileAndOccurrences.getValue(), NewName); + if (!RenameEdit) + return RenameEdit.takeError(); + if (!RenameEdit->Replacements.empty()) + Results.insert({FilePath, std::move(*RenameEdit)}); + } + return Results; +} + +} // namespace + +llvm::Expected rename(ParsedAST &AST, const RenameInputs &RInputs) { + const SourceManager &SM = AST.getSourceManager(); + SourceLocation SourceLocationBeg = + SM.getMacroArgExpandedLocation(getBeginningOfIdentifier( + RInputs.Pos, SM, AST.getASTContext().getLangOpts())); + // FIXME: renaming macros is not supported yet, the macro-handling code should + // be moved to rename tooling library. + if (locateMacroAt(SourceLocationBeg, AST.getPreprocessor())) + return makeError(UnsupportedSymbol); + + const NamedDecl *RenameDecl = tooling::getCanonicalSymbolDeclaration( + tooling::getNamedDeclAt(AST.getASTContext(), SourceLocationBeg)); + if (!RenameDecl) + return makeError(NoSymbolFound); + + auto Reject = + renameable(*RenameDecl->getCanonicalDecl(), RInputs.MainFilePath, + RInputs.Index, RInputs.AllowCrossFile); + if (Reject) + return makeError(*Reject); + + // We have two implemenations of the rename: + // - AST-based rename: used for renaming local symbols, e.g. variables + // defined in a function body; + // - index-based rename: used for renaming non-local symbols, and not + // feasible for local symbols (as by design our index don't index these + // symbols by design; + // 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); + if (!MainFileRenameEdit) + return MainFileRenameEdit.takeError(); + + if (!RInputs.AllowCrossFile) { + // within-file rename, just return the main file results. + return FileEdits({std::make_pair<>( + RInputs.MainFilePath, + Edit{RInputs.MainFileCode, std::move(*MainFileRenameEdit)})}); + } + + // Cross-file rename. + auto Results = + renameOutsideFile(RenameDecl, RInputs.MainFilePath, RInputs.NewName, + RInputs.Index, RInputs.VFS, RInputs.GetDirtyBuffer); + if (!Results) + return Results.takeError(); + // Attach the rename edits for the main file. + Results->try_emplace(RInputs.MainFilePath, RInputs.MainFileCode, + std::move(*MainFileRenameEdit)); + return Results; +} + } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/refactor/Tweak.h b/clang-tools-extra/clangd/refactor/Tweak.h --- a/clang-tools-extra/clangd/refactor/Tweak.h +++ b/clang-tools-extra/clangd/refactor/Tweak.h @@ -75,7 +75,7 @@ llvm::Optional ShowMessage; /// A mapping from file path(the one used for accessing the underlying VFS) /// to edits. - llvm::StringMap ApplyEdits; + FileEdits ApplyEdits; static Effect showMessage(StringRef S) { Effect E; diff --git a/clang-tools-extra/clangd/tool/ClangdMain.cpp b/clang-tools-extra/clangd/tool/ClangdMain.cpp --- a/clang-tools-extra/clangd/tool/ClangdMain.cpp +++ b/clang-tools-extra/clangd/tool/ClangdMain.cpp @@ -263,6 +263,13 @@ CommaSeparated, }; +opt CrossFileRename{ + "cross-file-rename", + cat(Features), + desc("Enable cross-file rename feature."), + init(false), +}; + opt WorkerThreadsCount{ "j", cat(Misc), @@ -594,6 +601,7 @@ } Opts.StaticIndex = StaticIdx.get(); Opts.AsyncThreadsCount = WorkerThreadsCount; + Opts.CrossFileRename = CrossFileRename; clangd::CodeCompleteOptions CCOpts; CCOpts.IncludeIneligibleResults = IncludeIneligibleResults; diff --git a/clang-tools-extra/clangd/unittests/RenameTests.cpp b/clang-tools-extra/clangd/unittests/RenameTests.cpp --- a/clang-tools-extra/clangd/unittests/RenameTests.cpp +++ b/clang-tools-extra/clangd/unittests/RenameTests.cpp @@ -9,8 +9,10 @@ #include "Annotations.h" #include "TestFS.h" #include "TestTU.h" +#include "index/Ref.h" #include "refactor/Rename.h" #include "clang/Tooling/Core/Replacement.h" +#include "llvm/Support/MemoryBuffer.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -18,9 +20,61 @@ namespace clangd { namespace { +using testing::Eq; +using testing::Matches; +MATCHER_P2(PairMatcher, KeyMatcher, ValueMatcher, "") { + return Matches(KeyMatcher)(arg.first()) && Matches(ValueMatcher)(arg.second); +} MATCHER_P2(RenameRange, Code, Range, "") { return replacementToEdit(Code, arg).range == Range; } +MATCHER_P2(EqualFileEdit, Code, Ranges, "") { + using ReplacementMatcher = testing::Matcher; + std::vector Expected; + for (const auto &R : Ranges) + Expected.push_back(RenameRange(Code, R)); + auto Matcher = + testing::internal::UnorderedElementsAreArrayMatcher( + Expected.begin(), Expected.end()); + return arg.InitialCode == Code && Matches(Matcher)(arg.Replacements); +} + +std::unique_ptr buildRefSlab(const Annotations &Code, + llvm::StringRef SymbolName, + llvm::StringRef Path) { + RefSlab::Builder Builder; + TestTU TU; + TU.HeaderCode = Code.code(); + auto Symbols = TU.headerSymbols(); + const auto &SymbolID = findSymbol(Symbols, SymbolName).ID; + for (const auto &Range : Code.ranges()) { + Ref R; + R.Kind = RefKind::Reference; + R.Location.Start.setLine(Range.start.line); + R.Location.Start.setColumn(Range.start.character); + R.Location.End.setLine(Range.end.line); + R.Location.End.setColumn(Range.end.character); + auto U = URI::create(Path).toString(); + R.Location.FileURI = U.c_str(); + Builder.insert(SymbolID, R); + } + + return std::make_unique(std::move(Builder).build()); +} + +RenameInputs renameInputs(const Annotations &Code, llvm::StringRef NewName, + llvm::StringRef Path, + const SymbolIndex *Index = nullptr, + bool CrossFile = false) { + RenameInputs Inputs; + Inputs.Pos = Code.point(); + Inputs.MainFileCode = Code.code(); + Inputs.MainFilePath = Path; + Inputs.NewName = NewName; + Inputs.Index = Index; + Inputs.AllowCrossFile = CrossFile; + return Inputs; +} TEST(RenameTest, SingleFile) { struct Test { @@ -80,10 +134,13 @@ auto TU = TestTU::withCode(Code.code()); auto AST = TU.build(); auto RenameResult = - renameWithinFile(AST, testPath(TU.Filename), Code.point(), "abcde"); + rename(AST, renameInputs(Code, "abcde", testPath(TU.Filename))); + ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError(); - auto ApplyResult = - tooling::applyAllReplacements(Code.code(), *RenameResult); + ASSERT_TRUE(RenameResult->size() == 1); + EXPECT_EQ(Code.code(), RenameResult->begin()->second.InitialCode); + auto ApplyResult = tooling::applyAllReplacements( + Code.code(), RenameResult->begin()->second.Replacements); ASSERT_TRUE(bool(ApplyResult)) << ApplyResult.takeError(); EXPECT_EQ(T.After, *ApplyResult) << T.Before; @@ -177,28 +234,108 @@ } auto AST = TU.build(); - auto Results = renameWithinFile(AST, testPath(TU.Filename), T.point(), - "dummyNewName", Case.Index); + auto Results = rename(AST, renameInputs(T, "dummyNewName", + testPath(TU.Filename), Case.Index)); bool WantRename = true; if (T.ranges().empty()) WantRename = false; if (!WantRename) { assert(Case.ErrorMessage && "Error message must be set!"); - EXPECT_FALSE(Results) << "expected renameWithinFile returned an error: " - << T.code(); + EXPECT_FALSE(Results) + << "expected rename returned an error: " << T.code(); auto ActualMessage = llvm::toString(Results.takeError()); EXPECT_THAT(ActualMessage, testing::HasSubstr(Case.ErrorMessage)); } else { - EXPECT_TRUE(bool(Results)) << "renameWithinFile returned an error: " + EXPECT_TRUE(bool(Results)) << "rename returned an error: " << llvm::toString(Results.takeError()); - std::vector> Expected; - for (const auto &R : T.ranges()) - Expected.push_back(RenameRange(TU.Code, R)); - EXPECT_THAT(*Results, UnorderedElementsAreArray(Expected)); + EXPECT_THAT(*Results, + testing::UnorderedElementsAre(PairMatcher( + testing::_, EqualFileEdit(T.code(), T.ranges())))); } } } +TEST(RenameTests, CrossFile) { + Annotations FooCode("class [[Foo]] {};"); + std::string FooPath = testPath("foo.cc"); + std::string FooDirtyBuffer = + (FooCode.code() + "\n// this is dirty buffer").str(); + Annotations BarCode("void [[Bar]]() {}"); + std::string BarPath = testPath("bar.cc"); + // Build the index, the index has "Foo" references in foo.cc and "Bar" + // references from bar.cc. + FileSymbols FSymbols; + FSymbols.update(FooPath, nullptr, buildRefSlab(FooCode, "Foo", FooPath), + nullptr, false); + FSymbols.update(BarPath, nullptr, buildRefSlab(BarCode, "Bar", BarPath), + nullptr, false); + auto Index = FSymbols.buildIndex(IndexType::Light); + + // Prepare the rename environment: + // - dirty buffer for foo.cc; + // - file on VFS for bar.cc; + Annotations MainCode("class [[Fo^o]] {};"); + auto MainFilePath = testPath("main.cc"); + RenameInputs Input = + renameInputs(MainCode, "newName", MainFilePath, Index.get(), + /*CrossFile*/ true); + Input.GetDirtyBuffer = [&](PathRef Path) -> llvm::Optional { + if (Path == FooPath) + return FooDirtyBuffer; + return llvm::None; + }; + llvm::IntrusiveRefCntPtr MemFS( + new llvm::vfs::InMemoryFileSystem); + MemFS->addFile(testPath("bar.cc"), 0, + llvm::MemoryBuffer::getMemBuffer(BarCode.code())); + Input.VFS = MemFS; + + // Run rename on Foo, there is a dirty buffer for foo.cc, rename should + // respect the dirty buffer. + TestTU TU = TestTU::withCode(MainCode.code()); + auto AST = TU.build(); + auto Results = rename(AST, Input); + ASSERT_TRUE(bool(Results)) << Results.takeError(); + EXPECT_THAT( + *Results, + testing::UnorderedElementsAre( + PairMatcher(Eq(FooPath), + EqualFileEdit(FooDirtyBuffer, FooCode.ranges())), + PairMatcher(Eq(MainFilePath), + EqualFileEdit(MainCode.code(), MainCode.ranges())))); + + // Run rename on Bar, there is no dirty buffer for the affected file bar.cc, + // so we should read file content from VFS. + MainCode = Annotations("void [[Bar]]() { [[B^ar]](); }"); + Input.MainFileCode = MainCode.code(); + Input.Pos = MainCode.point(); + TU = TestTU::withCode(MainCode.code()); + AST = TU.build(); + Results = rename(AST, Input); + ASSERT_TRUE(bool(Results)) << Results.takeError(); + EXPECT_THAT( + *Results, + testing::UnorderedElementsAre( + PairMatcher(Eq(BarPath), + EqualFileEdit(BarCode.code(), BarCode.ranges())), + PairMatcher(Eq(MainFilePath), + EqualFileEdit(MainCode.code(), MainCode.ranges())))); +} + +TEST(CrossFileRenameTests, CrossFileOnLocalSymbol) { + // cross-file rename should work for function-local symbols, even there is no + // index provided. + Annotations Code("void f(int [[abc]]) { [[a^bc]] = 3; }"); + auto TU = TestTU::withCode(Code.code()); + auto Path = testPath(TU.Filename); + auto AST = TU.build(); + auto Results = rename(AST, renameInputs(Code, "abcde", Path)); + ASSERT_TRUE(bool(Results)) << Results.takeError(); + EXPECT_THAT(*Results, + testing::UnorderedElementsAre(PairMatcher( + Eq(Path), EqualFileEdit(Code.code(), Code.ranges())))); +} + } // namespace } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/unittests/SyncAPI.h b/clang-tools-extra/clangd/unittests/SyncAPI.h --- a/clang-tools-extra/clangd/unittests/SyncAPI.h +++ b/clang-tools-extra/clangd/unittests/SyncAPI.h @@ -38,8 +38,8 @@ llvm::Expected> runFindDocumentHighlights(ClangdServer &Server, PathRef File, Position Pos); -llvm::Expected> -runRename(ClangdServer &Server, PathRef File, Position Pos, StringRef NewName); +llvm::Expected runRename(ClangdServer &Server, PathRef File, + Position Pos, StringRef NewName); std::string runDumpAST(ClangdServer &Server, PathRef File); diff --git a/clang-tools-extra/clangd/unittests/SyncAPI.cpp b/clang-tools-extra/clangd/unittests/SyncAPI.cpp --- a/clang-tools-extra/clangd/unittests/SyncAPI.cpp +++ b/clang-tools-extra/clangd/unittests/SyncAPI.cpp @@ -96,11 +96,11 @@ return std::move(*Result); } -llvm::Expected> runRename(ClangdServer &Server, - PathRef File, Position Pos, - llvm::StringRef NewName) { - llvm::Optional>> Result; - Server.rename(File, Pos, NewName, /*WantFormat=*/true, capture(Result)); +llvm::Expected runRename(ClangdServer &Server, PathRef File, + Position Pos, llvm::StringRef NewName) { + llvm::Optional> Result; + Server.rename(File, Pos, NewName, /*WantFormat=*/true, + /*DirtyBuffaer*/ nullptr, capture(Result)); return std::move(*Result); }