hokein updated this revision to Diff 246640.
hokein added a comment.
keep the old clangdServer::rename around temporarily to make internal
integration life easier.
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
@@ -264,6 +262,9 @@
/// If WantFormat is false, the final TextEdit will be not formatted,
/// 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);
@@ -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
