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) { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
