[clang-tools-extra] 3c3aca2 - [clangd] Don't perform rename when the refs result from index is incomplete.
Haojian Wu via cfe-commits
cfe-commits at lists.llvm.org
Thu Nov 28 04:59:06 PST 2019
Author: Haojian Wu
Date: 2019-11-28T13:56:19+01:00
New Revision: 3c3aca245e67fa70b6f49b9062983fbdf120ba04
URL: https://github.com/llvm/llvm-project/commit/3c3aca245e67fa70b6f49b9062983fbdf120ba04
DIFF: https://github.com/llvm/llvm-project/commit/3c3aca245e67fa70b6f49b9062983fbdf120ba04.diff
LOG: [clangd] Don't perform rename when the refs result from index is incomplete.
Summary:
Also do an early return if the number of affected files > limit to save
some unnecessary FileURI computations.
Reviewers: ilya-biryukov
Subscribers: MaskRay, jkorous, arphaman, kadircet, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D70811
Added:
Modified:
clang-tools-extra/clangd/refactor/Rename.cpp
clang-tools-extra/clangd/unittests/RenameTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index f775539cb63d..e57bf61dc2e5 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -281,20 +281,37 @@ Range toRange(const SymbolLocation &L) {
// Return all rename occurrences (per the index) outside of the main file,
// grouped by the absolute file path.
-llvm::StringMap<std::vector<Range>>
+llvm::Expected<llvm::StringMap<std::vector<Range>>>
findOccurrencesOutsideFile(const NamedDecl &RenameDecl,
llvm::StringRef MainFile, const SymbolIndex &Index) {
RefsRequest RQuest;
RQuest.IDs.insert(*getSymbolID(&RenameDecl));
- // Absolute file path => rename ocurrences in that file.
+ // Absolute file path => rename occurrences in that file.
llvm::StringMap<std::vector<Range>> AffectedFiles;
- Index.refs(RQuest, [&](const Ref &R) {
+ // FIXME: make the limit customizable.
+ static constexpr size_t MaxLimitFiles = 50;
+ bool HasMore = Index.refs(RQuest, [&](const Ref &R) {
+ if (AffectedFiles.size() > MaxLimitFiles)
+ return;
if (auto RefFilePath = filePath(R.Location, /*HintFilePath=*/MainFile)) {
if (*RefFilePath != MainFile)
AffectedFiles[*RefFilePath].push_back(toRange(R.Location));
}
});
+
+ if (AffectedFiles.size() > MaxLimitFiles)
+ return llvm::make_error<llvm::StringError>(
+ llvm::formatv("The number of affected files exceeds the max limit {0}",
+ MaxLimitFiles),
+ llvm::inconvertibleErrorCode());
+ if (HasMore) {
+ return llvm::make_error<llvm::StringError>(
+ llvm::formatv("The symbol {0} has too many occurrences",
+ RenameDecl.getQualifiedNameAsString()),
+ llvm::inconvertibleErrorCode());
+ }
+
return AffectedFiles;
}
@@ -321,17 +338,10 @@ llvm::Expected<FileEdits> renameOutsideFile(
llvm::function_ref<llvm::Expected<std::string>(PathRef)> GetFileContent) {
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::StringError>(
- llvm::formatv(
- "The number of affected files exceeds the max limit {0}: {1}",
- MaxLimitFiles, AffectedFiles.size()),
- llvm::inconvertibleErrorCode());
-
+ if (!AffectedFiles)
+ return AffectedFiles.takeError();
FileEdits Results;
- for (auto &FileAndOccurrences : AffectedFiles) {
+ for (auto &FileAndOccurrences : *AffectedFiles) {
llvm::StringRef FilePath = FileAndOccurrences.first();
auto AffectedFileCode = GetFileContent(FilePath);
diff --git a/clang-tools-extra/clangd/unittests/RenameTests.cpp b/clang-tools-extra/clangd/unittests/RenameTests.cpp
index 47aca380f3e9..89efb32a2bb5 100644
--- a/clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ b/clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -621,6 +621,34 @@ TEST(RenameTests, CrossFile) {
UnorderedElementsAre(
Pair(Eq(BarPath), Eq(expectedResult(BarCode, NewName))),
Pair(Eq(MainFilePath), Eq(expectedResult(MainCode, NewName)))));
+
+ // Run rename on a pagination index which couldn't return all refs in one
+ // request, we reject rename on this case.
+ class PaginationIndex : public SymbolIndex {
+ bool refs(const RefsRequest &Req,
+ llvm::function_ref<void(const Ref &)> Callback) const override {
+ return true; // has more references
+ }
+
+ bool fuzzyFind(
+ const FuzzyFindRequest &Req,
+ llvm::function_ref<void(const Symbol &)> Callback) const override {
+ return false;
+ }
+ void
+ lookup(const LookupRequest &Req,
+ llvm::function_ref<void(const Symbol &)> Callback) const override {}
+
+ void relations(const RelationsRequest &Req,
+ llvm::function_ref<void(const SymbolID &, const Symbol &)>
+ Callback) const override {}
+ size_t estimateMemoryUsage() const override { return 0; }
+ } PIndex;
+ 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"));
}
TEST(CrossFileRenameTests, CrossFileOnLocalSymbol) {
More information about the cfe-commits
mailing list