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 @@ -281,20 +281,37 @@ // Return all rename occurrences (per the index) outside of the main file, // grouped by the absolute file path. -llvm::StringMap> +llvm::Expected>> 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> 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::formatv("The number of affected files exceeds the max limit {0}", + MaxLimitFiles), + llvm::inconvertibleErrorCode()); + if (HasMore) { + return llvm::make_error( + llvm::formatv("The symbol {0} has too many occurrences", + RenameDecl.getQualifiedNameAsString()), + llvm::inconvertibleErrorCode()); + } + return AffectedFiles; } @@ -321,17 +338,10 @@ llvm::function_ref(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::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 --- a/clang-tools-extra/clangd/unittests/RenameTests.cpp +++ b/clang-tools-extra/clangd/unittests/RenameTests.cpp @@ -621,6 +621,34 @@ 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 Callback) const override { + return true; // has more references + } + + bool fuzzyFind( + const FuzzyFindRequest &Req, + llvm::function_ref Callback) const override { + return false; + } + void + lookup(const LookupRequest &Req, + llvm::function_ref Callback) const override {} + + void relations(const RelationsRequest &Req, + llvm::function_ref + 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) {