[clang-tools-extra] 3c3aca2 - [clangd] Don't perform rename when the refs result from index is incomplete.

[Haojian Wu via cfe-commits]

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) {