[clangd] Expose the rename LimitFiles option to the C++ API, NFC.

[sammccall]

clang-tools-extra/clangd/ClangdServer.h

145

rename is just a normal API call, why is this a server option rather than a parameter to rename()?
It probably needs to be a member in ClangdLSPServer though, like CodeCompleteOptions.

(WantFormat belongs in this struct, I think)

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

30

This needs a comment.

Or combine with LimitFiles, so that 0 means no cross-file rename.

[hokein]

address review comments - move the options to clangdServer::rename API

[hokein]

keep the old clangdServer::rename around temporarily to make internal integration life easier.

clang-tools-extra/clangd/ClangdServer.h

struct Options {
 
     /// Clangd will execute compiler drivers matching one of these globs to
     /// fetch system include path.
     std::vector<std::string> QueryDriverGlobs;
 
     /// Enable semantic highlighting features.
     bool SemanticHighlighting = false;
 
-    /// Enable cross-file rename feature.
-    bool CrossFileRename = false;
+    /// Options for rename.
+    RenameOptions RenameOpts;
 
     /// 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;
   };
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;
+  RenameOptions RenameOpts;
 
   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),
+      RenameOpts(Opts.RenameOpts), 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),
const auto *TouchingIdentifier =
         spelledIdentifierTouching(*Loc, AST.getTokens());
     if (!TouchingIdentifier)
       return CB(llvm::None); // no rename on non-identifiers.
 
     auto Range = halfOpenToRange(
         SM, CharSourceRange::getCharRange(TouchingIdentifier->location(),
                                           TouchingIdentifier->endLocation()));
 
-    if (CrossFileRename)
+    if (RenameOpts.AllowCrossFile)
       // 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,
+    auto Changes = clangd::rename({Pos, "dummy", AST, File, Index, RenameOpts,
                                    /*GetDirtyBuffer=*/nullptr});
     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, RenameOpts, 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

 class ParsedAST;
 class SymbolIndex;
 
 /// Gets dirty buffer for a given file \p AbsPath.
 /// Returns None if there is no dirty buffer for the given file.
 using DirtyBufferGetter =
     llvm::function_ref<llvm::Optional<std::string>(PathRef AbsPath)>;
 
+struct RenameOptions {
+  bool AllowCrossFile = false;
+  /// The mamimum number of affected files (0 means no limit), only meaningful
+  /// when AllowCrossFile = true.
+  /// If the actual number exceeds the limit, rename is forbidden.
+  size_t LimitFiles = 50;
+};
+
 struct RenameInputs {
   Position Pos; // the position triggering the rename
   llvm::StringRef NewName;
 
   ParsedAST &AST;
   llvm::StringRef MainFilePath;
 
   const SymbolIndex *Index = nullptr;
 
-  bool AllowCrossFile = false;
+  RenameOptions Opts = {};
   // 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,
+                           size_t MaxLimitFiles) {
   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)
       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));

 // Clangd builds a dynamic index for all opened files on top of the static
 // 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::StringRef NewName, const SymbolIndex &Index, size_t MaxLimitFiles,
     llvm::function_ref<llvm::Expected<std::string>(PathRef)> GetFileContent) {
   trace::Span Tracer("RenameOutsideFile");
-  auto AffectedFiles =
-      findOccurrencesOutsideFile(RenameDecl, MainFilePath, Index);
+  auto AffectedFiles = findOccurrencesOutsideFile(RenameDecl, MainFilePath,
+                                                  Index, MaxLimitFiles);
   if (!AffectedFiles)
     return AffectedFiles.takeError();
   FileEdits Results;
   for (auto &FileAndOccurrences : *AffectedFiles) {
     llvm::StringRef FilePath = FileAndOccurrences.first();
 
     auto AffectedFileCode = GetFileContent(FilePath);
     if (!AffectedFileCode) {
void findNearMiss(
   findNearMiss(PartialMatch, IndexedRest, LexedRest.drop_front(),
                LexedIndex + 1, Fuel, MatchedCB);
 }
 
 } // namespace
 
 llvm::Expected<FileEdits> rename(const RenameInputs &RInputs) {
   trace::Span Tracer("Rename flow");
+  const auto &Opts = RInputs.Opts;
   ParsedAST &AST = RInputs.AST;
   const SourceManager &SM = AST.getSourceManager();
   llvm::StringRef MainFileCode = SM.getBufferData(SM.getMainFileID());
   auto GetFileContent = [&RInputs,
                          &SM](PathRef AbsPath) -> llvm::Expected<std::string> {
     llvm::Optional<std::string> DirtyBuffer;
     if (RInputs.GetDirtyBuffer &&
         (DirtyBuffer = RInputs.GetDirtyBuffer(AbsPath)))
llvm::Expected<FileEdits> rename(const RenameInputs &RInputs) {
   if (DeclsUnderCursor.empty())
     return makeError(ReasonToReject::NoSymbolFound);
   if (DeclsUnderCursor.size() > 1)
     return makeError(ReasonToReject::AmbiguousSymbol);
 
   const auto &RenameDecl =
       llvm::cast<NamedDecl>(*(*DeclsUnderCursor.begin())->getCanonicalDecl());
   auto Reject = renameable(RenameDecl, RInputs.MainFilePath, RInputs.Index,
-                           RInputs.AllowCrossFile);
+                           Opts.AllowCrossFile);
   if (Reject)
     return makeError(*Reject);
 
   // We have two implementations of the rename:
   //   - AST-based rename: used for renaming local symbols, e.g. variables
   //     defined in a function body;
   //   - index-based rename: used for renaming non-local symbols, and not
   //     feasible for local symbols (as by design our index don't index these
   //     symbols by design;
   // To make cross-file rename work for local symbol, we use a hybrid solution:
   //   - run AST-based rename on the main file;
   //   - run index-based rename on other affected files;
   auto MainFileRenameEdit = renameWithinFile(AST, RenameDecl, RInputs.NewName);
   if (!MainFileRenameEdit)
     return MainFileRenameEdit.takeError();
 
-  if (!RInputs.AllowCrossFile) {
+  if (!Opts.AllowCrossFile) {
     // Within-file rename: just return the main file results.
     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,
+        Opts.LimitFiles == 0 ? std::numeric_limits<size_t>::max()
+                             : Opts.LimitFiles - 1,
+        GetFileContent);
     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

AsyncIndexLoad = runAsync<void>([Placeholder] {
       if (auto Idx = loadIndex(IndexFile, /*UseDex=*/true))
         Placeholder->reset(std::move(Idx));
     });
     if (Sync)
       AsyncIndexLoad.wait();
   }
   Opts.StaticIndex = StaticIdx.get();
   Opts.AsyncThreadsCount = WorkerThreadsCount;
-  Opts.CrossFileRename = CrossFileRename;
+  Opts.RenameOpts.AllowCrossFile = CrossFileRename;
 
   clangd::CodeCompleteOptions CCOpts;
   CCOpts.IncludeIneligibleResults = IncludeIneligibleResults;
   CCOpts.Limit = LimitResults;
   if (CompletionStyle.getNumOccurrences())
     CCOpts.BundleOverloads = CompletionStyle != Detailed;
   CCOpts.ShowOrigins = ShowOrigins;
   CCOpts.InsertIncludes = HeaderInsertion;

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

auto GetDirtyBuffer = [&](PathRef Path) -> llvm::Optional<std::string> {
     return llvm::None;
   };
 
   // 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});
+  auto Results = rename({MainCode.point(),
+                         NewName,
+                         AST,
+                         MainFilePath,
+                         Index.get(),
+                         {/*CrossFile=*/true},
+                         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});
+  Results = rename({MainCode.point(),
+                    NewName,
+                    AST,
+                    MainFilePath,
+                    Index.get(),
+                    {/*CrossFile=*/true},
+                    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
class PaginationIndex : public SymbolIndex {
     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});
+  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(CrossFileRename, QueryCtorInIndex) {
   const auto MainCode = Annotations("F^oo f;");
   auto TU = TestTU::withCode(MainCode.code());
public:
 
     void relations(const RelationsRequest &,
                    llvm::function_ref<void(const SymbolID &, const Symbol &)>)
         const override {}
     size_t estimateMemoryUsage() const override { return 0; }
 
     RefsRequest *Out;
   } RIndex(&Req);
-  auto Results =
-      rename({MainCode.point(), "NewName", AST, testPath("main.cc"), &RIndex,
-              /*CrossFile=*/true});
+  auto Results = rename({MainCode.point(),
+                         "NewName",
+                         AST,
+                         testPath("main.cc"),
+                         &RIndex,
+                         {/*CrossFile=*/true}});
   ASSERT_TRUE(bool(Results)) << Results.takeError();
   const auto HeaderSymbols = TU.headerSymbols();
   EXPECT_THAT(Req.IDs,
               testing::Contains(findSymbol(HeaderSymbols, "Foo::Foo").ID));
 }
 
 TEST(CrossFileRenameTests, DeduplicateRefsFromIndex) {
   auto MainCode = Annotations("int [[^x]] = 2;");
public:
 
     void relations(const RelationsRequest &,
                    llvm::function_ref<void(const SymbolID &, const Symbol &)>)
         const override {}
     size_t estimateMemoryUsage() const override { return 0; }
     Ref ReturnedRef;
   } DIndex(XRefInBarCC);
   llvm::StringRef NewName = "newName";
-  auto Results = rename({MainCode.point(), NewName, AST, MainFilePath, &DIndex,
-                         /*CrossFile=*/true});
+  RenameOptions RenameOpts{true, 50};
+  auto Results = rename(
+      {MainCode.point(), NewName, AST, MainFilePath, &DIndex, RenameOpts});
   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)))));
 }
 
for (const auto& T : Cases) {
     std::string FooHPath = testPath("foo.h");
     std::string FooCCPath = testPath("foo.cc");
 
     MockFSProvider FS;
     FS.Files[FooHPath] = std::string(FooH.code());
     FS.Files[FooCCPath] = std::string(FooCC.code());
 
     auto ServerOpts = ClangdServer::optsForTest();
-    ServerOpts.CrossFileRename = true;
+    ServerOpts.RenameOpts.AllowCrossFile = true;
     ServerOpts.BuildDynamicSymbolIndex = true;
     ClangdServer Server(CDB, FS, ServerOpts);
 
     // Add all files to clangd server to make sure the dynamic index has been
     // built.
     runAddDocument(Server, FooHPath, FooH.code());
     runAddDocument(Server, FooCCPath, FooCC.code());