hokein created this revision.
hokein added a reviewer: sammccall.
Herald added subscribers: usaxena95, kadircet, arphaman.
Herald added a project: clang.
hokein requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.
several changes:
- return a structure result in rename API;
- prepareRename now returns more information (main-file occurrences)
- remove the duplicated detecting-touch-identifier code in prepareRename (which
is implemented in rename API);
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D88634
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
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/test/rename.test
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
@@ -40,9 +40,13 @@
llvm::Expected<std::vector<DocumentHighlight>>
runFindDocumentHighlights(ClangdServer &Server, PathRef File, Position Pos);
-llvm::Expected<FileEdits> runRename(ClangdServer &Server, PathRef File,
- Position Pos, StringRef NewName,
- const clangd::RenameOptions &RenameOpts);
+llvm::Expected<RenameResults>
+runRename(ClangdServer &Server, PathRef File, Position Pos, StringRef NewName,
+ const clangd::RenameOptions &RenameOpts);
+
+llvm::Expected<RenameResults>
+runPrepareRename(ClangdServer &Server, PathRef File, Position Pos,
+ const clangd::RenameOptions &RenameOpts);
llvm::Expected<tooling::Replacements>
runFormatFile(ClangdServer &Server, PathRef File, StringRef Code);
Index: clang-tools-extra/clangd/unittests/SyncAPI.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/SyncAPI.cpp
+++ clang-tools-extra/clangd/unittests/SyncAPI.cpp
@@ -97,14 +97,22 @@
return std::move(*Result);
}
-llvm::Expected<FileEdits> runRename(ClangdServer &Server, PathRef File,
- Position Pos, llvm::StringRef NewName,
- const RenameOptions &RenameOpts) {
- llvm::Optional<llvm::Expected<FileEdits>> Result;
+llvm::Expected<RenameResults> runRename(ClangdServer &Server, PathRef File,
+ Position Pos, llvm::StringRef NewName,
+ const RenameOptions &RenameOpts) {
+ llvm::Optional<llvm::Expected<RenameResults>> Result;
Server.rename(File, Pos, NewName, RenameOpts, capture(Result));
return std::move(*Result);
}
+llvm::Expected<RenameResults>
+runPrepareRename(ClangdServer &Server, PathRef File, Position Pos,
+ const RenameOptions &RenameOpts) {
+ llvm::Optional<llvm::Expected<RenameResults>> Result;
+ Server.prepareRename(File, Pos, RenameOpts, capture(Result));
+ return std::move(*Result);
+}
+
llvm::Expected<tooling::Replacements>
runFormatFile(ClangdServer &Server, PathRef File, StringRef Code) {
llvm::Optional<llvm::Expected<tooling::Replacements>> Result;
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -502,8 +502,8 @@
auto RenameResult =
rename({RenamePos, NewName, AST, testPath(TU.Filename)});
ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
- ASSERT_EQ(1u, RenameResult->size());
- EXPECT_EQ(applyEdits(std::move(*RenameResult)).front().second,
+ ASSERT_EQ(1u, RenameResult->Edits.size());
+ EXPECT_EQ(applyEdits(std::move(RenameResult->Edits)).front().second,
expectedResult(Code, NewName));
}
}
@@ -653,8 +653,8 @@
} else {
EXPECT_TRUE(bool(Results)) << "rename returned an error: "
<< llvm::toString(Results.takeError());
- ASSERT_EQ(1u, Results->size());
- EXPECT_EQ(applyEdits(std::move(*Results)).front().second,
+ ASSERT_EQ(1u, Results->Edits.size());
+ EXPECT_EQ(applyEdits(std::move(Results->Edits)).front().second,
expectedResult(T, NewName));
}
}
@@ -683,8 +683,8 @@
auto RenameResult =
rename({Code.point(), NewName, AST, testPath(TU.Filename)});
ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError() << Code.point();
- ASSERT_EQ(1u, RenameResult->size());
- EXPECT_EQ(applyEdits(std::move(*RenameResult)).front().second,
+ ASSERT_EQ(1u, RenameResult->Edits.size());
+ EXPECT_EQ(applyEdits(std::move(RenameResult->Edits)).front().second,
expectedResult(Code, NewName));
}
@@ -703,6 +703,42 @@
testing::HasSubstr("not a supported kind"));
}
+TEST(RenameTest, PrepareRename) {
+ Annotations FooH("void func();");
+ Annotations FooCC(R"cpp(
+ #include "foo.h"
+ void [[fu^nc]]() {}
+ )cpp");
+ std::string FooHPath = testPath("foo.h");
+ std::string FooCCPath = testPath("foo.cc");
+ MockFS FS;
+ FS.Files[FooHPath] = std::string(FooH.code());
+ FS.Files[FooCCPath] = std::string(FooCC.code());
+
+ auto ServerOpts = ClangdServer::optsForTest();
+ ServerOpts.BuildDynamicSymbolIndex = true;
+
+ MockCompilationDatabase CDB;
+ ClangdServer Server(CDB, FS, ServerOpts);
+ runAddDocument(Server, FooHPath, FooH.code());
+ runAddDocument(Server, FooCCPath, FooCC.code());
+
+ auto Results =
+ runPrepareRename(Server, FooCCPath, FooCC.point(), {/*CrossFile=*/true});
+ // verify that for multi-file rename, we only return main-file occurrences.
+ ASSERT_TRUE(bool(Results)) << Results.takeError();
+ EXPECT_THAT(applyEdits(std::move(Results->Edits)),
+ UnorderedElementsAre(
+ Pair(Eq(FooCCPath), Eq(expectedResult(FooCC, "dummy")))));
+
+ // single-file rename on global symbols, we should report an error.
+ Results =
+ runPrepareRename(Server, FooCCPath, FooCC.point(), {/*CrossFile=*/false});
+ EXPECT_FALSE(Results);
+ EXPECT_THAT(llvm::toString(Results.takeError()),
+ testing::HasSubstr("is used outside"));
+}
+
TEST(CrossFileRenameTests, DirtyBuffer) {
Annotations FooCode("class [[Foo]] {};");
std::string FooPath = testPath("foo.cc");
@@ -741,7 +777,7 @@
GetDirtyBuffer});
ASSERT_TRUE(bool(Results)) << Results.takeError();
EXPECT_THAT(
- applyEdits(std::move(*Results)),
+ applyEdits(std::move(Results->Edits)),
UnorderedElementsAre(
Pair(Eq(FooPath), Eq(expectedResult(FooDirtyBuffer, NewName))),
Pair(Eq(MainFilePath), Eq(expectedResult(MainCode, NewName)))));
@@ -762,7 +798,7 @@
GetDirtyBuffer});
ASSERT_TRUE(bool(Results)) << Results.takeError();
EXPECT_THAT(
- applyEdits(std::move(*Results)),
+ applyEdits(std::move(Results->Edits)),
UnorderedElementsAre(
Pair(Eq(BarPath), Eq(expectedResult(BarCode, NewName))),
Pair(Eq(MainFilePath), Eq(expectedResult(MainCode, NewName)))));
@@ -847,7 +883,7 @@
{/*CrossFile=*/true}});
ASSERT_TRUE(bool(Results)) << Results.takeError();
EXPECT_THAT(
- applyEdits(std::move(*Results)),
+ applyEdits(std::move(Results->Edits)),
UnorderedElementsAre(
Pair(Eq(BarPath), Eq(expectedResult(BarCode, NewName))),
Pair(Eq(MainFilePath), Eq(expectedResult(MainCode, NewName)))));
@@ -1047,7 +1083,7 @@
Server, FooHPath, RenamePos, NewName, {/*CrossFile=*/true}));
EXPECT_THAT(Tracer.takeMetric("rename_files"), ElementsAre(2));
EXPECT_THAT(
- applyEdits(std::move(FileEditsList)),
+ applyEdits(std::move(FileEditsList.Edits)),
UnorderedElementsAre(
Pair(Eq(FooHPath), Eq(expectedResult(T.FooH, NewName))),
Pair(Eq(FooCCPath), Eq(expectedResult(T.FooCC, NewName)))));
@@ -1066,7 +1102,7 @@
auto Results = rename({Code.point(), NewName, AST, Path});
ASSERT_TRUE(bool(Results)) << Results.takeError();
EXPECT_THAT(
- applyEdits(std::move(*Results)),
+ applyEdits(std::move(Results->Edits)),
UnorderedElementsAre(Pair(Eq(Path), Eq(expectedResult(Code, NewName)))));
}
Index: clang-tools-extra/clangd/test/rename.test
===================================================================
--- clang-tools-extra/clangd/test/rename.test
+++ clang-tools-extra/clangd/test/rename.test
@@ -21,9 +21,12 @@
# CHECK-NEXT: }
---
{"jsonrpc":"2.0","id":2,"method":"textDocument/prepareRename","params":{"textDocument":{"uri":"test:///foo.cpp"},"position":{"line":0,"character":2}}}
-# CHECK: "id": 2,
-# CHECK-NEXT: "jsonrpc": "2.0",
-# CHECK-NEXT: "result": null
+# CHECK: "error": {
+# CHECK-NEXT: "code": -32001,
+# CHECK-NEXT: "message": "Cannot rename symbol: there is no symbol at the given location"
+# CHECK-NEXT: },
+# CHECK-NEXT: "id": 2,
+# CHECK-NEXT: "jsonrpc": "2.0"
---
{"jsonrpc":"2.0","id":4,"method":"textDocument/rename","params":{"textDocument":{"uri":"test:///foo.cpp"},"position":{"line":0,"character":2},"newName":"bar"}}
# CHECK: "error": {
Index: clang-tools-extra/clangd/refactor/Rename.h
===================================================================
--- clang-tools-extra/clangd/refactor/Rename.h
+++ clang-tools-extra/clangd/refactor/Rename.h
@@ -55,10 +55,16 @@
DirtyBufferGetter GetDirtyBuffer = nullptr;
};
+struct RenameResults {
+ // The range of the symbol that the user can attempt to rename.
+ Range R;
+ FileEdits Edits;
+};
+
/// 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);
+llvm::Expected<RenameResults> rename(const RenameInputs &RInputs);
/// Generates rename edits that replaces all given occurrences with the
/// NewName.
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -427,7 +427,7 @@
} // namespace
-llvm::Expected<FileEdits> rename(const RenameInputs &RInputs) {
+llvm::Expected<RenameResults> rename(const RenameInputs &RInputs) {
trace::Span Tracer("Rename flow");
const auto &Opts = RInputs.Opts;
ParsedAST &AST = RInputs.AST;
@@ -456,9 +456,13 @@
return Loc.takeError();
const syntax::Token *IdentifierToken =
spelledIdentifierTouching(*Loc, AST.getTokens());
+
// Renames should only triggered on identifiers.
if (!IdentifierToken)
return makeError(ReasonToReject::NoSymbolFound);
+ Range CurrentIdentifier = halfOpenToRange(
+ SM, CharSourceRange::getCharRange(IdentifierToken->location(),
+ IdentifierToken->endLocation()));
// FIXME: Renaming macros is not supported yet, the macro-handling code should
// be moved to rename tooling library.
if (locateMacroAt(*IdentifierToken, AST.getPreprocessor()))
@@ -493,12 +497,14 @@
// return the main file edit if this is a within-file rename or the symbol
// being renamed is function local.
if (!Opts.AllowCrossFile || RenameDecl.getParentFunctionOrMethod()) {
- return FileEdits(
- {std::make_pair(RInputs.MainFilePath,
- Edit{MainFileCode, std::move(*MainFileRenameEdit)})});
+ return RenameResults{
+ CurrentIdentifier,
+ FileEdits({std::make_pair(
+ RInputs.MainFilePath,
+ Edit{MainFileCode, std::move(*MainFileRenameEdit)})})};
}
- FileEdits Results;
+ FileEdits Edits;
// Renameable safely guards us that at this point we are renaming a local
// symbol if we don't have index.
if (RInputs.Index) {
@@ -509,12 +515,12 @@
GetFileContent);
if (!OtherFilesEdits)
return OtherFilesEdits.takeError();
- Results = std::move(*OtherFilesEdits);
+ Edits = std::move(*OtherFilesEdits);
}
// Attach the rename edits for the main file.
- Results.try_emplace(RInputs.MainFilePath, MainFileCode,
- std::move(*MainFileRenameEdit));
- return Results;
+ Edits.try_emplace(RInputs.MainFilePath, MainFileCode,
+ std::move(*MainFileRenameEdit));
+ return RenameResults{CurrentIdentifier, Edits};
}
llvm::Expected<Edit> buildRenameEdit(llvm::StringRef AbsFilePath,
Index: clang-tools-extra/clangd/ClangdServer.h
===================================================================
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -273,9 +273,12 @@
StringRef TriggerText, Callback<std::vector<TextEdit>> CB);
/// Test the validity of a rename operation.
+ ///
+ /// The returned result may be incomplete as it only contains occurrences from
+ /// the current main file.
void prepareRename(PathRef File, Position Pos,
const RenameOptions &RenameOpts,
- Callback<llvm::Optional<Range>> CB);
+ Callback<RenameResults> CB);
/// Rename all occurrences of the symbol at the \p Pos in \p File to
/// \p NewName.
@@ -283,7 +286,7 @@
/// 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);
+ const RenameOptions &Opts, Callback<RenameResults> CB);
struct TweakRef {
std::string ID; /// ID to pass for applyTweak.
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -400,46 +400,35 @@
void ClangdServer::prepareRename(PathRef File, Position Pos,
const RenameOptions &RenameOpts,
- Callback<llvm::Optional<Range>> CB) {
+ Callback<RenameResults> CB) {
auto Action = [Pos, File = File.str(), CB = std::move(CB), RenameOpts,
this](llvm::Expected<InputsAndAST> InpAST) mutable {
if (!InpAST)
return CB(InpAST.takeError());
- auto &AST = InpAST->AST;
- const auto &SM = AST.getSourceManager();
- auto Loc = sourceLocationInMainFile(SM, Pos);
- if (!Loc)
- return CB(Loc.takeError());
- const auto *TouchingIdentifier =
- spelledIdentifierTouching(*Loc, AST.getTokens());
- if (!TouchingIdentifier)
- return CB(llvm::None); // no rename on non-identifiers.
-
- auto Range = halfOpenToRange(
- SM, CharSourceRange::getCharRange(TouchingIdentifier->location(),
- TouchingIdentifier->endLocation()));
-
- 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, RenameOpts,
- /*GetDirtyBuffer=*/nullptr});
- if (!Changes) {
+ clangd::FileIndex EmptyIndex;
+ // prepareRename is latency-sensitive:
+ // - for single-file rename, performing rename isn't substantially more
+ // expensive than doing an AST-based check;
+ // - for cross-file rename, we deliberately pass an empty index to save the
+ // cost, thus the result may be incomplete as it only contains main-file
+ // occurrences;
+ auto Results = clangd::rename(
+ {Pos, "dummy", InpAST->AST, File,
+ RenameOpts.AllowCrossFile ? &EmptyIndex : Index, RenameOpts});
+ if (!Results) {
// 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());
+ return CB(Results.takeError());
}
- return CB(Range);
+ return CB(*Results);
};
WorkScheduler.runWithAST("PrepareRename", File, std::move(Action));
}
void ClangdServer::rename(PathRef File, Position Pos, llvm::StringRef NewName,
- const RenameOptions &Opts, Callback<FileEdits> CB) {
+ const RenameOptions &Opts,
+ Callback<RenameResults> CB) {
// A snapshot of all file dirty buffers.
llvm::StringMap<std::string> Snapshot = WorkScheduler.getAllFileContents();
auto Action = [File = File.str(), NewName = NewName.str(), Pos, Opts,
@@ -457,24 +446,24 @@
return llvm::None;
return It->second;
};
- auto Edits = clangd::rename(
+ auto R = clangd::rename(
{Pos, NewName, InpAST->AST, File, Index, Opts, GetDirtyBuffer});
- if (!Edits)
- return CB(Edits.takeError());
+ if (!R)
+ return CB(R.takeError());
if (Opts.WantFormat) {
auto Style = getFormatStyleForFile(File, InpAST->Inputs.Contents,
*InpAST->Inputs.TFS);
llvm::Error Err = llvm::Error::success();
- for (auto &E : *Edits)
+ for (auto &E : R->Edits)
Err =
llvm::joinErrors(reformatEdit(E.getValue(), Style), std::move(Err));
if (Err)
return CB(std::move(Err));
}
- RenameFiles.record(Edits->size());
- return CB(std::move(*Edits));
+ RenameFiles.record(R->Edits.size());
+ return CB(*R);
};
WorkScheduler.runWithAST("Rename", File, std::move(Action));
}
Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -802,8 +802,13 @@
void ClangdLSPServer::onPrepareRename(const TextDocumentPositionParams &Params,
Callback<llvm::Optional<Range>> Reply) {
- Server->prepareRename(Params.textDocument.uri.file(), Params.position,
- RenameOpts, std::move(Reply));
+ Server->prepareRename(
+ Params.textDocument.uri.file(), Params.position, RenameOpts,
+ [Reply = std::move(Reply)](llvm::Expected<RenameResults> Result) mutable {
+ if (!Result)
+ return Reply(Result.takeError());
+ return Reply(std::move(Result->R));
+ });
}
void ClangdLSPServer::onRename(const RenameParams &Params,
@@ -815,14 +820,14 @@
Server->rename(
File, Params.position, Params.newName, RenameOpts,
[File, Params, Reply = std::move(Reply),
- this](llvm::Expected<FileEdits> Edits) mutable {
- if (!Edits)
- return Reply(Edits.takeError());
- if (auto Err = validateEdits(DraftMgr, *Edits))
+ this](llvm::Expected<RenameResults> R) mutable {
+ if (!R)
+ return Reply(R.takeError());
+ if (auto Err = validateEdits(DraftMgr, R->Edits))
return Reply(std::move(Err));
WorkspaceEdit Result;
Result.changes.emplace();
- for (const auto &Rep : *Edits) {
+ for (const auto &Rep : R->Edits) {
(*Result.changes)[URI::createFile(Rep.first()).toString()] =
Rep.second.asTextEdits();
}
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
