[clangd] Make global rename file limit adjustable

[hokein]

oops, I should have synced with you first (the task was assigned to me in the Tuesday meeting), I have https://reviews.llvm.org/D74834 which is under review. Apology for this.

Maybe you subscribe all clangd patches in phabricator as well (to allow better tracking), since most of our teammates have already done it.

[kbobyrev]

In D74961#1886479, @hokein wrote:

oops, I should have synced with you first (the task was assigned to me in the Tuesday meeting), I have https://reviews.llvm.org/D74834 which is under review. Apology for this.

Maybe you subscribe all clangd patches in phabricator as well (to allow better tracking), since most of our teammates have already done it.

No worries, I should've talked to you first! Thanks for working on this!

clang-tools-extra/clangd/ClangdServer.h

struct Options {
     std::vector<std::string> QueryDriverGlobs;
 
     /// Enable semantic highlighting features.
     bool SemanticHighlighting = false;
 
     /// Enable cross-file rename feature.
     bool CrossFileRename = false;
 
+    unsigned GlobalRenameFileLimit = 50;
+
     /// Returns true if the tweak should be enabled.
     std::function<bool(const Tweak &)> TweakFilter = [](const Tweak &T) {
       return !T.hidden(); // only enable non-hidden tweaks.
     };
 
     explicit operator TUScheduler::Options() const;
   };
   // Sensible default options for use in tests.
void symbolInfo(PathRef File, Position Pos,
                   Callback<std::vector<SymbolDetails>> CB);
 
   /// Get semantic ranges around a specified position in a file.
   void semanticRanges(PathRef File, Position Pos,
                       Callback<std::vector<Range>> CB);
 
   /// Get all document links in a file.
   void documentLinks(PathRef File, Callback<std::vector<DocumentLink>> CB);
- 
   /// Returns estimated memory usage for each of the currently open files.
   /// The order of results is unspecified.
   /// Overall memory usage of clangd may be significantly more than reported
   /// here, as this metric does not account (at least) for:
   ///   - memory occupied by static and dynamic index,
   ///   - memory required for in-flight requests,
   /// FIXME: those metrics might be useful too, we should add them.
   std::vector<std::pair<Path, std::size_t>> getUsedBytesPerFile() const;
private:
   // When set, provides clang-tidy options for a specific file.
   ClangTidyOptionsBuilder GetClangTidyOptions;
 
   // If this is true, suggest include insertion fixes for diagnostic errors that
   // can be caused by missing includes (e.g. member access in incomplete type).
   bool SuggestMissingIncludes = false;
 
   bool CrossFileRename = false;
+  const unsigned GlobalRenameFileLimit = 50;
 
   std::function<bool(const Tweak &)> TweakFilter;
 
   // GUARDED_BY(CachedCompletionFuzzyFindRequestMutex)
   llvm::StringMap<llvm::Optional<FuzzyFindRequest>>
       CachedCompletionFuzzyFindRequestByFile;
   mutable std::mutex CachedCompletionFuzzyFindRequestMutex;
 

clang-tools-extra/clangd/ClangdServer.cpp

ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB,
                            const FileSystemProvider &FSProvider,
                            const Options &Opts, Callbacks *Callbacks)
     : FSProvider(FSProvider),
       DynamicIdx(Opts.BuildDynamicSymbolIndex
                      ? new FileIndex(Opts.HeavyweightDynamicSymbolIndex)
                      : nullptr),
       GetClangTidyOptions(Opts.GetClangTidyOptions),
       SuggestMissingIncludes(Opts.SuggestMissingIncludes),
-      CrossFileRename(Opts.CrossFileRename), TweakFilter(Opts.TweakFilter),
-      WorkspaceRoot(Opts.WorkspaceRoot),
+      CrossFileRename(Opts.CrossFileRename),
+      GlobalRenameFileLimit(Opts.GlobalRenameFileLimit),
+      TweakFilter(Opts.TweakFilter), WorkspaceRoot(Opts.WorkspaceRoot),
       // Pass a callback into `WorkScheduler` to extract symbols from a newly
       // parsed file and rebuild the file index synchronously each time an AST
       // is parsed.
       // FIXME(ioeric): this can be slow and we may be able to index on less
       // critical paths.
       WorkScheduler(
           CDB, TUScheduler::Options(Opts),
           std::make_unique<UpdateIndexCallbacks>(DynamicIdx.get(), Callbacks,
auto Range = halfOpenToRange(
                                           TouchingIdentifier->endLocation()));
 
     if (CrossFileRename)
       // 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,
-                                   /*AllowCrossFile=*/false,
-                                   /*GetDirtyBuffer=*/nullptr});
+    auto Changes =
+        clangd::rename({Pos, "dummy", AST, File, Index,
+                        /*AllowCrossFile=*/false, GlobalRenameFileLimit,
     if (!Changes) {
       // 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(Range);
   };
if (!InpAST)
       return CB(InpAST.takeError());
     auto GetDirtyBuffer =
         [&Snapshot](PathRef AbsPath) -> llvm::Optional<std::string> {
       auto It = Snapshot.find(AbsPath);
       if (It == Snapshot.end())
         return llvm::None;
       return It->second;
     };
-    auto Edits = clangd::rename({Pos, NewName, InpAST->AST, File, Index,
-                                 CrossFileRename, GetDirtyBuffer});
+    auto Edits =
+        clangd::rename({Pos, NewName, InpAST->AST, File, Index, CrossFileRename,
+                        GlobalRenameFileLimit, GetDirtyBuffer});
     if (!Edits)
       return CB(Edits.takeError());
 
     if (WantFormat) {
       auto Style = getFormatStyleForFile(File, InpAST->Inputs.Contents,
                                          InpAST->Inputs.FS.get());
       llvm::Error Err = llvm::Error::success();
       for (auto &E : *Edits)

clang-tools-extra/clangd/refactor/Rename.h

struct RenameInputs {
   llvm::StringRef NewName;
 
   ParsedAST *
   llvm::StringRef MainFilePath;
 
   const SymbolIndex *Index = nullptr;
 
   bool AllowCrossFile = false;
+  const unsigned GlobalRenameFileLimit = 50;
   // When set, used by the rename to get file content for all rename-related
   // files.
   // If there is no corresponding dirty buffer, we will use the file content
   // from disk.
   DirtyBufferGetter GetDirtyBuffer = nullptr;
 };
 
 /// Renames all occurrences of the symbol.

clang-tools-extra/clangd/refactor/Rename.cpp

std::vector<const CXXConstructorDecl *> getConstructors(const NamedDecl *ND) {
   }
   return Ctors;
 }
 
 // Return all rename occurrences (using the index) outside of the main file,
 // grouped by the absolute file path.
 llvm::Expected<llvm::StringMap<std::vector<Range>>>
 findOccurrencesOutsideFile(const NamedDecl &RenameDecl,
-                           llvm::StringRef MainFile, const SymbolIndex &Index) {
+                           llvm::StringRef MainFile, const SymbolIndex &Index,
+                           const unsigned GlobalRenameFileLimit) {
   trace::Span Tracer("FindOccurrencesOutsideFile");
   RefsRequest RQuest;
   RQuest.IDs.insert(*getSymbolID(&RenameDecl));
   // Classes and their constructors are different symbols, and have different
   // symbol ID.
   // When querying references for a class, clangd's own index will also return
   // references of the corresponding class constructors, but this is not true
   // for all index backends, e.g. kythe, so we add all constructors to the query
   // request.
   for (const auto *Ctor : getConstructors(&RenameDecl))
     RQuest.IDs.insert(*getSymbolID(Ctor));
 
   // Absolute file path => rename occurrences in that file.
   llvm::StringMap<std::vector<Range>> AffectedFiles;
-  // FIXME: Make the limit customizable.
-  static constexpr size_t MaxLimitFiles = 50;
   bool HasMore = Index.refs(RQuest, [&](const Ref &R) {
-    if (AffectedFiles.size() > MaxLimitFiles)
+    if (AffectedFiles.size() > GlobalRenameFileLimit)
       return;
     if ((R.Kind & RefKind::Spelled) == RefKind::Unknown)
       return;
     if (auto RefFilePath = filePath(R.Location, /*HintFilePath=*/MainFile)) {
       if (*RefFilePath != MainFile)
         AffectedFiles[*RefFilePath].push_back(toRange(R.Location));
     }
   });
 
-  if (AffectedFiles.size() > MaxLimitFiles)
+  if (AffectedFiles.size() > GlobalRenameFileLimit)
     return llvm::make_error<llvm::StringError>(
         llvm::formatv("The number of affected files exceeds the max limit {0}",
-                      MaxLimitFiles),
+                      GlobalRenameFileLimit),
         llvm::inconvertibleErrorCode());
   if (HasMore) {
     return llvm::make_error<llvm::StringError>(
         llvm::formatv("The symbol {0} has too many occurrences",
                       RenameDecl.getQualifiedNameAsString()),
         llvm::inconvertibleErrorCode());
   }
   // Sort and deduplicate the results, in case that index returns duplications.

 // index of the whole codebase. Dynamic index is up-to-date (respects dirty
 // buffers) as long as clangd finishes processing opened files, while static
 // index (background index) is relatively stale. We choose the dirty buffers
 // as the file content we rename on, and fallback to file content on disk if
 // there is no dirty buffer.
 llvm::Expected<FileEdits> renameOutsideFile(
     const NamedDecl &RenameDecl, llvm::StringRef MainFilePath,
     llvm::StringRef NewName, const SymbolIndex &Index,
-    llvm::function_ref<llvm::Expected<std::string>(PathRef)> GetFileContent) {
+    llvm::function_ref<llvm::Expected<std::string>(PathRef)> GetFileContent,
+    const unsigned GlobalRenameFileLimit) {
   trace::Span Tracer("RenameOutsideFile");
-  auto AffectedFiles =
-      findOccurrencesOutsideFile(RenameDecl, MainFilePath, Index);
+  auto AffectedFiles = findOccurrencesOutsideFile(RenameDecl, MainFilePath,
+                                                  Index, GlobalRenameFileLimit);
   if (!AffectedFiles)
     return AffectedFiles.takeError();
   FileEdits Results;
   for (auto &FileAndOccurrences : *AffectedFiles) {
     llvm::StringRef FilePath = FileAndOccurrences.first();
 
     auto AffectedFileCode = GetFileContent(FilePath);
     if (!AffectedFileCode) {
return FileEdits(
         {std::make_pair(RInputs.MainFilePath,
                         Edit{MainFileCode, std::move(*MainFileRenameEdit)})});
   }
 
   FileEdits Results;
   // Renameable safely guards us that at this point we are renaming a local
   // symbol if we don't have index.
   if (RInputs.Index) {
-    auto OtherFilesEdits =
-        renameOutsideFile(RenameDecl, RInputs.MainFilePath, RInputs.NewName,
-                          *RInputs.Index, GetFileContent);
+    auto OtherFilesEdits = renameOutsideFile(
+        RenameDecl, RInputs.MainFilePath, RInputs.NewName, *RInputs.Index,
+        GetFileContent, RInputs.GlobalRenameFileLimit);
     if (!OtherFilesEdits)
       return OtherFilesEdits.takeError();
     Results = std::move(*OtherFilesEdits);
   }
   // Attach the rename edits for the main file.
   Results.try_emplace(RInputs.MainFilePath, MainFileCode,
                       std::move(*MainFileRenameEdit));
   return Results;

clang-tools-extra/clangd/tool/ClangdMain.cpp

 
 opt<bool> PrettyPrint{
     "pretty",
     cat(Protocol),
     desc("Pretty-print JSON output"),
     init(false),
 };
 
+opt<unsigned> GlobalRenameFileLimit{
+    "global-rename-file-limit",
+    cat(Features),
+    desc("Renaming symbol that appears in more files will fail"),
+    init(50),
+    Hidden,
+};
+
 /// Supports a test URI scheme with relaxed constraints for lit tests.
 /// The path in a test URI will be combined with a platform-specific fake
 /// directory to form an absolute path. For example, test:///a.cpp is resolved
 /// C:\clangd-test\a.cpp on Windows and /clangd-test/a.cpp on Unix.
 class TestScheme : public URIScheme {
 public:
   llvm::Expected<std::string>
   getAbsolutePath(llvm::StringRef /*Authority*/, llvm::StringRef Body,
if (!CompileCommandsDir.empty()) {
     } else {
       llvm::errs()
           << "Path specified by --compile-commands-dir does not exist. The "
              "argument will be ignored.\n";
     }
   }
 
   ClangdServer::Options Opts;
+  Opts.GlobalRenameFileLimit = GlobalRenameFileLimit;
   switch (PCHStorage) {
   case PCHStorageFlag::Memory:
     Opts.StorePreamblesInMemory = true;
     break;
   case PCHStorageFlag::Disk:
     Opts.StorePreamblesInMemory = false;
     break;
   }

clang-tools-extra/clangd/unittests/RenameTests.cpp

TEST(CrossFileRenameTests, DirtyBuffer) {
   };
 
   // Run rename on Foo, there is a dirty buffer for foo.cc, rename should
   // respect the dirty buffer.
   TestTU TU = TestTU::withCode(MainCode.code());
   auto AST = TU.build();
   llvm::StringRef NewName = "newName";
   auto Results = rename({MainCode.point(), NewName, AST, MainFilePath,
-                         Index.get(), /*CrossFile=*/true, GetDirtyBuffer});
+                         Index.get(), /*CrossFile=*/true,
+                         /*GlobalRenameFileLimit=*/50, GetDirtyBuffer});
   ASSERT_TRUE(bool(Results)) << Results.takeError();
   EXPECT_THAT(
       applyEdits(std::move(*Results)),
       UnorderedElementsAre(
           Pair(Eq(FooPath), Eq(expectedResult(FooDirtyBuffer, NewName))),
           Pair(Eq(MainFilePath), Eq(expectedResult(MainCode, NewName)))));
 
   // Run rename on Bar, there is no dirty buffer for the affected file bar.cc,
   // so we should read file content from VFS.
   MainCode = Annotations("void [[Bar]]() { [[B^ar]](); }");
   TU = TestTU::withCode(MainCode.code());
   // Set a file "bar.cc" on disk.
   TU.AdditionalFiles["bar.cc"] = std::string(BarCode.code());
   AST = TU.build();
   Results = rename({MainCode.point(), NewName, AST, MainFilePath, Index.get(),
-                    /*CrossFile=*/true, GetDirtyBuffer});
+                    /*CrossFile=*/true, /*GlobalRenameFileLimit=*/50,
+                    GetDirtyBuffer});
   ASSERT_TRUE(bool(Results)) << Results.takeError();
   EXPECT_THAT(
       applyEdits(std::move(*Results)),
       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
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});
+                    /*CrossFile=*/true, /*GlobalRenameFileLimit=*/50,
+                    GetDirtyBuffer});
   EXPECT_FALSE(Results);
   EXPECT_THAT(llvm::toString(Results.takeError()),
               testing::HasSubstr("too many occurrences"));
 }
 
 TEST(CrossFileRename, QueryCtorInIndex) {
   const auto MainCode = Annotations("F^oo f;");
   auto TU = TestTU::withCode(MainCode.code());