This revision was automatically updated to reflect the committed changes. Closed by commit rG34d0e1bd6d05: [clangd] Expose the rename LimitFiles option to the C++ API, NFC. (authored by hokein).
Changed prior to commit: https://reviews.llvm.org/D74834?vs=246640&id=246641#toc Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74834/new/ https://reviews.llvm.org/D74834 Files: clang-tools-extra/clangd/ClangdLSPServer.cpp clang-tools-extra/clangd/ClangdLSPServer.h clang-tools-extra/clangd/ClangdServer.cpp clang-tools-extra/clangd/ClangdServer.h clang-tools-extra/clangd/refactor/Rename.cpp clang-tools-extra/clangd/refactor/Rename.h clang-tools-extra/clangd/tool/ClangdMain.cpp clang-tools-extra/clangd/unittests/ClangdTests.cpp clang-tools-extra/clangd/unittests/RenameTests.cpp clang-tools-extra/clangd/unittests/SyncAPI.cpp clang-tools-extra/clangd/unittests/SyncAPI.h
Index: clang-tools-extra/clangd/unittests/SyncAPI.h =================================================================== --- clang-tools-extra/clangd/unittests/SyncAPI.h +++ clang-tools-extra/clangd/unittests/SyncAPI.h @@ -39,7 +39,8 @@ runFindDocumentHighlights(ClangdServer &Server, PathRef File, Position Pos); llvm::Expected<FileEdits> runRename(ClangdServer &Server, PathRef File, - Position Pos, StringRef NewName); + Position Pos, StringRef NewName, + const clangd::RenameOptions &RenameOpts); std::string runDumpAST(ClangdServer &Server, PathRef File); Index: clang-tools-extra/clangd/unittests/SyncAPI.cpp =================================================================== --- clang-tools-extra/clangd/unittests/SyncAPI.cpp +++ clang-tools-extra/clangd/unittests/SyncAPI.cpp @@ -97,9 +97,10 @@ } llvm::Expected<FileEdits> runRename(ClangdServer &Server, PathRef File, - Position Pos, llvm::StringRef NewName) { + Position Pos, llvm::StringRef NewName, + const RenameOptions &RenameOpts) { llvm::Optional<llvm::Expected<FileEdits>> Result; - Server.rename(File, Pos, NewName, /*WantFormat=*/false, capture(Result)); + Server.rename(File, Pos, NewName, RenameOpts, capture(Result)); return std::move(*Result); } Index: clang-tools-extra/clangd/unittests/RenameTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/RenameTests.cpp +++ clang-tools-extra/clangd/unittests/RenameTests.cpp @@ -721,8 +721,13 @@ TestTU TU = TestTU::withCode(MainCode.code()); auto AST = TU.build(); llvm::StringRef NewName = "newName"; - auto Results = rename({MainCode.point(), NewName, AST, MainFilePath, - Index.get(), /*CrossFile=*/true, GetDirtyBuffer}); + auto Results = rename({MainCode.point(), + NewName, + AST, + MainFilePath, + Index.get(), + {/*CrossFile=*/true}, + GetDirtyBuffer}); ASSERT_TRUE(bool(Results)) << Results.takeError(); EXPECT_THAT( applyEdits(std::move(*Results)), @@ -737,8 +742,13 @@ // Set a file "bar.cc" on disk. TU.AdditionalFiles["bar.cc"] = std::string(BarCode.code()); AST = TU.build(); - Results = rename({MainCode.point(), NewName, AST, MainFilePath, Index.get(), - /*CrossFile=*/true, GetDirtyBuffer}); + Results = rename({MainCode.point(), + NewName, + AST, + MainFilePath, + Index.get(), + {/*CrossFile=*/true}, + GetDirtyBuffer}); ASSERT_TRUE(bool(Results)) << Results.takeError(); EXPECT_THAT( applyEdits(std::move(*Results)), @@ -768,8 +778,13 @@ Callback) const override {} size_t estimateMemoryUsage() const override { return 0; } } PIndex; - Results = rename({MainCode.point(), NewName, AST, MainFilePath, &PIndex, - /*CrossFile=*/true, GetDirtyBuffer}); + Results = rename({MainCode.point(), + NewName, + AST, + MainFilePath, + &PIndex, + {/*CrossFile=*/true}, + GetDirtyBuffer}); EXPECT_FALSE(Results); EXPECT_THAT(llvm::toString(Results.takeError()), testing::HasSubstr("too many occurrences")); @@ -810,9 +825,12 @@ RefsRequest *Out; } RIndex(&Req); - auto Results = - rename({MainCode.point(), "NewName", AST, testPath("main.cc"), &RIndex, - /*CrossFile=*/true}); + auto Results = rename({MainCode.point(), + "NewName", + AST, + testPath("main.cc"), + &RIndex, + {/*CrossFile=*/true}}); ASSERT_TRUE(bool(Results)) << Results.takeError(); const auto HeaderSymbols = TU.headerSymbols(); EXPECT_THAT(Req.IDs, @@ -857,8 +875,12 @@ Ref ReturnedRef; } DIndex(XRefInBarCC); llvm::StringRef NewName = "newName"; - auto Results = rename({MainCode.point(), NewName, AST, MainFilePath, &DIndex, - /*CrossFile=*/true}); + auto Results = rename({MainCode.point(), + NewName, + AST, + MainFilePath, + &DIndex, + {/*CrossFile=*/true}}); ASSERT_TRUE(bool(Results)) << Results.takeError(); EXPECT_THAT( applyEdits(std::move(*Results)), @@ -1045,7 +1067,6 @@ FS.Files[FooCCPath] = std::string(FooCC.code()); auto ServerOpts = ClangdServer::optsForTest(); - ServerOpts.CrossFileRename = true; ServerOpts.BuildDynamicSymbolIndex = true; ClangdServer Server(CDB, FS, ServerOpts); @@ -1056,8 +1077,8 @@ llvm::StringRef NewName = "NewName"; for (const auto &RenamePos : FooH.points()) { - auto FileEditsList = - llvm::cantFail(runRename(Server, FooHPath, RenamePos, NewName)); + auto FileEditsList = llvm::cantFail(runRename( + Server, FooHPath, RenamePos, NewName, {/*CrossFile=*/true})); EXPECT_THAT( applyEdits(std::move(FileEditsList)), UnorderedElementsAre( Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/ClangdTests.cpp +++ clang-tools-extra/clangd/unittests/ClangdTests.cpp @@ -533,7 +533,8 @@ EXPECT_EQ(runDumpAST(Server, FooCpp), "<no-ast>"); EXPECT_ERROR(runLocateSymbolAt(Server, FooCpp, Position())); EXPECT_ERROR(runFindDocumentHighlights(Server, FooCpp, Position())); - EXPECT_ERROR(runRename(Server, FooCpp, Position(), "new_name")); + EXPECT_ERROR(runRename(Server, FooCpp, Position(), "new_name", + clangd::RenameOptions())); // Identifier-based fallback completion. EXPECT_THAT(cantFail(runCodeComplete(Server, FooCpp, Position(), clangd::CodeCompleteOptions())) Index: clang-tools-extra/clangd/tool/ClangdMain.cpp =================================================================== --- clang-tools-extra/clangd/tool/ClangdMain.cpp +++ clang-tools-extra/clangd/tool/ClangdMain.cpp @@ -17,6 +17,7 @@ #include "Transport.h" #include "index/Background.h" #include "index/Serialization.h" +#include "refactor/Rename.h" #include "clang/Basic/Version.h" #include "clang/Format/Format.h" #include "llvm/ADT/Optional.h" @@ -628,7 +629,6 @@ } Opts.StaticIndex = StaticIdx.get(); Opts.AsyncThreadsCount = WorkerThreadsCount; - Opts.CrossFileRename = CrossFileRename; clangd::CodeCompleteOptions CCOpts; CCOpts.IncludeIneligibleResults = IncludeIneligibleResults; @@ -708,8 +708,13 @@ llvm::Optional<OffsetEncoding> OffsetEncodingFromFlag; if (ForceOffsetEncoding != OffsetEncoding::UnsupportedEncoding) OffsetEncodingFromFlag = ForceOffsetEncoding; + + clangd::RenameOptions RenameOpts; + // Shall we allow to custimize the file limit? + RenameOpts.AllowCrossFile = CrossFileRename; + ClangdLSPServer LSPServer( - *TransportLayer, FSProvider, CCOpts, CompileCommandsDirPath, + *TransportLayer, FSProvider, CCOpts, RenameOpts, CompileCommandsDirPath, /*UseDirBasedCDB=*/CompileArgsFrom == FilesystemCompileArgs, OffsetEncodingFromFlag, Opts); llvm::set_thread_name("clangd.main"); Index: clang-tools-extra/clangd/refactor/Rename.h =================================================================== --- clang-tools-extra/clangd/refactor/Rename.h +++ clang-tools-extra/clangd/refactor/Rename.h @@ -26,6 +26,18 @@ using DirtyBufferGetter = llvm::function_ref<llvm::Optional<std::string>(PathRef AbsPath)>; +struct RenameOptions { + /// If true, enable cross-file rename; otherwise, only allows to rename a + /// symbol that's only used in the current file. + bool AllowCrossFile = false; + /// The mamimum number of affected files (0 means no limit), only meaningful + /// when AllowCrossFile = true. + /// If the actual number exceeds the limit, rename is forbidden. + size_t LimitFiles = 50; + /// If true, format the rename edits, only meaningful in ClangdServer layer. + bool WantFormat = false; +}; + struct RenameInputs { Position Pos; // the position triggering the rename llvm::StringRef NewName; @@ -35,7 +47,7 @@ const SymbolIndex *Index = nullptr; - bool AllowCrossFile = false; + RenameOptions Opts = {}; // When set, used by the rename to get file content for all rename-related // files. // If there is no corresponding dirty buffer, we will use the file content @@ -43,7 +55,7 @@ DirtyBufferGetter GetDirtyBuffer = nullptr; }; -/// Renames all occurrences of the symbol. +/// Renames all occurrences of the symbol. The result edits are unformatted. /// If AllowCrossFile is false, returns an error if rename a symbol that's used /// in another file (per the index). llvm::Expected<FileEdits> rename(const RenameInputs &RInputs); Index: clang-tools-extra/clangd/refactor/Rename.cpp =================================================================== --- clang-tools-extra/clangd/refactor/Rename.cpp +++ clang-tools-extra/clangd/refactor/Rename.cpp @@ -316,7 +316,8 @@ // grouped by the absolute file path. llvm::Expected<llvm::StringMap<std::vector<Range>>> findOccurrencesOutsideFile(const NamedDecl &RenameDecl, - llvm::StringRef MainFile, const SymbolIndex &Index) { + llvm::StringRef MainFile, const SymbolIndex &Index, + size_t MaxLimitFiles) { trace::Span Tracer("FindOccurrencesOutsideFile"); RefsRequest RQuest; RQuest.IDs.insert(*getSymbolID(&RenameDecl)); @@ -331,8 +332,6 @@ // Absolute file path => rename occurrences in that file. llvm::StringMap<std::vector<Range>> AffectedFiles; - // FIXME: Make the limit customizable. - static constexpr size_t MaxLimitFiles = 50; bool HasMore = Index.refs(RQuest, [&](const Ref &R) { if (AffectedFiles.size() > MaxLimitFiles) return; @@ -381,11 +380,11 @@ // there is no dirty buffer. llvm::Expected<FileEdits> renameOutsideFile( const NamedDecl &RenameDecl, llvm::StringRef MainFilePath, - llvm::StringRef NewName, const SymbolIndex &Index, + llvm::StringRef NewName, const SymbolIndex &Index, size_t MaxLimitFiles, llvm::function_ref<llvm::Expected<std::string>(PathRef)> GetFileContent) { trace::Span Tracer("RenameOutsideFile"); - auto AffectedFiles = - findOccurrencesOutsideFile(RenameDecl, MainFilePath, Index); + auto AffectedFiles = findOccurrencesOutsideFile(RenameDecl, MainFilePath, + Index, MaxLimitFiles); if (!AffectedFiles) return AffectedFiles.takeError(); FileEdits Results; @@ -467,6 +466,7 @@ llvm::Expected<FileEdits> rename(const RenameInputs &RInputs) { trace::Span Tracer("Rename flow"); + const auto &Opts = RInputs.Opts; ParsedAST &AST = RInputs.AST; const SourceManager &SM = AST.getSourceManager(); llvm::StringRef MainFileCode = SM.getBufferData(SM.getMainFileID()); @@ -514,7 +514,7 @@ const auto &RenameDecl = llvm::cast<NamedDecl>(*(*DeclsUnderCursor.begin())->getCanonicalDecl()); auto Reject = renameable(RenameDecl, RInputs.MainFilePath, RInputs.Index, - RInputs.AllowCrossFile); + Opts.AllowCrossFile); if (Reject) return makeError(*Reject); @@ -531,7 +531,7 @@ if (!MainFileRenameEdit) return MainFileRenameEdit.takeError(); - if (!RInputs.AllowCrossFile) { + if (!Opts.AllowCrossFile) { // Within-file rename: just return the main file results. return FileEdits( {std::make_pair(RInputs.MainFilePath, @@ -542,9 +542,11 @@ // Renameable safely guards us that at this point we are renaming a local // symbol if we don't have index. if (RInputs.Index) { - auto OtherFilesEdits = - renameOutsideFile(RenameDecl, RInputs.MainFilePath, RInputs.NewName, - *RInputs.Index, GetFileContent); + auto OtherFilesEdits = renameOutsideFile( + RenameDecl, RInputs.MainFilePath, RInputs.NewName, *RInputs.Index, + Opts.LimitFiles == 0 ? std::numeric_limits<size_t>::max() + : Opts.LimitFiles - 1, + GetFileContent); if (!OtherFilesEdits) return OtherFilesEdits.takeError(); Results = std::move(*OtherFilesEdits); Index: clang-tools-extra/clangd/ClangdServer.h =================================================================== --- clang-tools-extra/clangd/ClangdServer.h +++ clang-tools-extra/clangd/ClangdServer.h @@ -145,9 +145,6 @@ /// 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<bool(const Tweak &)> TweakFilter = [](const Tweak &T) { return !T.hidden(); // only enable non-hidden tweaks. @@ -257,6 +254,7 @@ /// Test the validity of a rename operation. void prepareRename(PathRef File, Position Pos, + const RenameOptions &RenameOpts, Callback<llvm::Optional<Range>> CB); /// Rename all occurrences of the symbol at the \p Pos in \p File to @@ -265,6 +263,9 @@ /// 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, + const RenameOptions &Opts, Callback<FileEdits> CB); + // FIXME: remove this compatibility method in favor above. + void rename(PathRef File, Position Pos, llvm::StringRef NewName, bool WantFormat, Callback<FileEdits> CB); struct TweakRef { @@ -343,8 +344,6 @@ // can be caused by missing includes (e.g. member access in incomplete type). bool SuggestMissingIncludes = false; - bool CrossFileRename = false; - std::function<bool(const Tweak &)> TweakFilter; // GUARDED_BY(CachedCompletionFuzzyFindRequestMutex) Index: clang-tools-extra/clangd/ClangdServer.cpp =================================================================== --- clang-tools-extra/clangd/ClangdServer.cpp +++ clang-tools-extra/clangd/ClangdServer.cpp @@ -131,8 +131,7 @@ : nullptr), GetClangTidyOptions(Opts.GetClangTidyOptions), SuggestMissingIncludes(Opts.SuggestMissingIncludes), - CrossFileRename(Opts.CrossFileRename), TweakFilter(Opts.TweakFilter), - WorkspaceRoot(Opts.WorkspaceRoot), + 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. @@ -319,8 +318,9 @@ } void ClangdServer::prepareRename(PathRef File, Position Pos, + const RenameOptions &RenameOpts, Callback<llvm::Optional<Range>> CB) { - auto Action = [Pos, File = File.str(), CB = std::move(CB), + auto Action = [Pos, File = File.str(), CB = std::move(CB), RenameOpts, this](llvm::Expected<InputsAndAST> InpAST) mutable { if (!InpAST) return CB(InpAST.takeError()); @@ -338,14 +338,13 @@ SM, CharSourceRange::getCharRange(TouchingIdentifier->location(), TouchingIdentifier->endLocation())); - if (CrossFileRename) + if (RenameOpts.AllowCrossFile) // FIXME: we now assume cross-file rename always succeeds, revisit this. return CB(Range); // Performing the local rename isn't substantially more expensive than // doing an AST-based check, so we just rename and throw away the results. - auto Changes = clangd::rename({Pos, "dummy", AST, File, Index, - /*AllowCrossFile=*/false, + auto Changes = clangd::rename({Pos, "dummy", AST, File, Index, RenameOpts, /*GetDirtyBuffer=*/nullptr}); if (!Changes) { // LSP says to return null on failure, but that will result in a generic @@ -359,10 +358,10 @@ } void ClangdServer::rename(PathRef File, Position Pos, llvm::StringRef NewName, - bool WantFormat, Callback<FileEdits> CB) { + const RenameOptions &Opts, Callback<FileEdits> CB) { // A snapshot of all file dirty buffers. llvm::StringMap<std::string> Snapshot = WorkScheduler.getAllFileContents(); - auto Action = [File = File.str(), NewName = NewName.str(), Pos, WantFormat, + auto Action = [File = File.str(), NewName = NewName.str(), Pos, Opts, CB = std::move(CB), Snapshot = std::move(Snapshot), this](llvm::Expected<InputsAndAST> InpAST) mutable { if (!InpAST) @@ -374,12 +373,12 @@ return llvm::None; return It->second; }; - auto Edits = clangd::rename({Pos, NewName, InpAST->AST, File, Index, - CrossFileRename, GetDirtyBuffer}); + auto Edits = clangd::rename( + {Pos, NewName, InpAST->AST, File, Index, Opts, GetDirtyBuffer}); if (!Edits) return CB(Edits.takeError()); - if (WantFormat) { + if (Opts.WantFormat) { auto Style = getFormatStyleForFile(File, InpAST->Inputs.Contents, InpAST->Inputs.FS.get()); llvm::Error Err = llvm::Error::success(); @@ -395,6 +394,14 @@ WorkScheduler.runWithAST("Rename", File, std::move(Action)); } +void ClangdServer::rename(PathRef File, Position Pos, llvm::StringRef NewName, + bool WantFormat, Callback<FileEdits> CB) { + RenameOptions Opts; + Opts.WantFormat = WantFormat; + Opts.AllowCrossFile = false; + rename(File, Pos, NewName, Opts, std::move(CB)); +} + // May generate several candidate selections, due to SelectionTree ambiguity. // vector of pointers because GCC doesn't like non-copyable Selection. static llvm::Expected<std::vector<std::unique_ptr<Tweak::Selection>>> Index: clang-tools-extra/clangd/ClangdLSPServer.h =================================================================== --- clang-tools-extra/clangd/ClangdLSPServer.h +++ clang-tools-extra/clangd/ClangdLSPServer.h @@ -42,6 +42,7 @@ // FIXME: Clean up signature around CDBs. ClangdLSPServer(Transport &Transp, const FileSystemProvider &FSProvider, const clangd::CodeCompleteOptions &CCOpts, + const clangd::RenameOptions &RenameOpts, llvm::Optional<Path> CompileCommandsDir, bool UseDirBasedCDB, llvm::Optional<OffsetEncoding> ForcedOffsetEncoding, const ClangdServer::Options &Opts); @@ -197,6 +198,8 @@ const FileSystemProvider &FSProvider; /// Options used for code completion clangd::CodeCompleteOptions CCOpts; + /// Options used for rename. + clangd::RenameOptions RenameOpts; /// Options used for diagnostics. ClangdDiagnosticOptions DiagOpts; /// The supported kinds of the client. Index: clang-tools-extra/clangd/ClangdLSPServer.cpp =================================================================== --- clang-tools-extra/clangd/ClangdLSPServer.cpp +++ clang-tools-extra/clangd/ClangdLSPServer.cpp @@ -761,7 +761,7 @@ void ClangdLSPServer::onPrepareRename(const TextDocumentPositionParams &Params, Callback<llvm::Optional<Range>> Reply) { Server->prepareRename(Params.textDocument.uri.file(), Params.position, - std::move(Reply)); + RenameOpts, std::move(Reply)); } void ClangdLSPServer::onRename(const RenameParams &Params, @@ -772,8 +772,7 @@ return Reply(llvm::make_error<LSPError>( "onRename called for non-added file", ErrorCode::InvalidParams)); Server->rename( - File, Params.position, Params.newName, - /*WantFormat=*/true, + File, Params.position, Params.newName, RenameOpts, [File, Params, Reply = std::move(Reply), this](llvm::Expected<FileEdits> Edits) mutable { if (!Edits) @@ -1230,12 +1229,14 @@ ClangdLSPServer::ClangdLSPServer( class Transport &Transp, const FileSystemProvider &FSProvider, const clangd::CodeCompleteOptions &CCOpts, + const clangd::RenameOptions &RenameOpts, llvm::Optional<Path> CompileCommandsDir, bool UseDirBasedCDB, llvm::Optional<OffsetEncoding> ForcedOffsetEncoding, const ClangdServer::Options &Opts) : BackgroundContext(Context::current().clone()), Transp(Transp), MsgHandler(new MessageHandler(*this)), FSProvider(FSProvider), - CCOpts(CCOpts), SupportedSymbolKinds(defaultSymbolKinds()), + CCOpts(CCOpts), RenameOpts(RenameOpts), + SupportedSymbolKinds(defaultSymbolKinds()), SupportedCompletionItemKinds(defaultCompletionItemKinds()), UseDirBasedCDB(UseDirBasedCDB), CompileCommandsDir(std::move(CompileCommandsDir)), ClangdServerOpts(Opts),
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits