[clangd] Don't run the prepare for tweaks that are disabled.

[hokein]

Summary

Previously, we ran the prepare, even for the tweaks that are disabled.

[hokein]

Simplify the code further based on offline discussion.

[ilya-biryukov]

clang-tools-extra/clangd/ClangdServer.h

135–139

NIT: the comment mentions StringRef, but function accepts a Tweak now.
Maybe update the comment?

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

117

NIT: maybe use function_ref here

[hokein]

Address comments.

clang-tools-extra/clangd/ClangdServer.h

135–139

good catch, thanks!

[sammccall]

clang-tools-extra/clangd/ClangdServer.h

137

the default value should reject hidden checks, for API users. The duplication is a bit sad

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

535

nit: -hidden-features

536

nit: seems clearer to write:

if (hidden && !HiddenFeatures)
  return false;
if (flag passed && !name matches flag)
  return false;
return true

[hokein]

Address comments.

clang-tools-extra/clangd/ClangdServer.h

struct Options {
     llvm::Optional<std::string> ResourceDir = llvm::None;
 
     /// Time to wait after a new file version before computing diagnostics.
     std::chrono::steady_clock::duration UpdateDebounce =
         std::chrono::milliseconds(500);
 
     bool SuggestMissingIncludes = false;
 
-    /// Enable hidden features mostly useful to clangd developers.
-    /// e.g. tweaks to dump the AST.
-    bool HiddenFeatures = false;
-
     /// 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;
 
-    /// Returns true if the StringRef is a tweak that should be enabled
-    std::function<bool(llvm::StringRef)> TweakFilter =
-        [](llvm::StringRef TweakToSearch) { return true; };
+    /// 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.
+    };
   };
   // Sensible default options for use in tests.
   // Features like indexing must be enabled if desired.
   static Options optsForTest();
 
   /// Creates a new ClangdServer instance.
   ///
   /// ClangdServer uses \p CDB to obtain compilation arguments for parsing. Note
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 EnableHiddenFeatures = false;
 
-  std::function<bool(llvm::StringRef)> TweakFilter;
+  std::function<bool(const Tweak &)> TweakFilter;
 
   // GUARDED_BY(CachedCompletionFuzzyFindRequestMutex)
   llvm::StringMap<llvm::Optional<FuzzyFindRequest>>
       CachedCompletionFuzzyFindRequestByFile;
   mutable std::mutex CachedCompletionFuzzyFindRequestMutex;
 
   llvm::Optional<std::string> WorkspaceRoot;
   // WorkScheduler has to be the last member, because its destructor has to be

clang-tools-extra/clangd/ClangdServer.cpp

ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB,
                            DiagnosticsConsumer &DiagConsumer,
                            const Options &Opts)
     : FSProvider(FSProvider),
       DynamicIdx(Opts.BuildDynamicSymbolIndex
                      ? new FileIndex(Opts.HeavyweightDynamicSymbolIndex)
                      : nullptr),
       GetClangTidyOptions(Opts.GetClangTidyOptions),
       SuggestMissingIncludes(Opts.SuggestMissingIncludes),
-      EnableHiddenFeatures(Opts.HiddenFeatures), TweakFilter(Opts.TweakFilter),
+      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, Opts.AsyncThreadsCount, Opts.StorePreamblesInMemory,
void ClangdServer::enumerateTweaks(PathRef File, Range Sel,
   auto Action = [this, Sel](decltype(CB) CB, std::string File,
                             Expected<InputsAndAST> InpAST) {
     if (!InpAST)
       return CB(InpAST.takeError());
     auto Selection = tweakSelection(Sel, *InpAST);
     if (!Selection)
       return CB(Selection.takeError());
     std::vector<TweakRef> Res;
-    for (auto &T : prepareTweaks(*Selection)) {
-      if (!TweakFilter(T->id()) || (T->hidden() && !EnableHiddenFeatures))
-        continue;
+    for (auto &T : prepareTweaks(*Selection, TweakFilter))
       Res.push_back({T->id(), T->title(), T->intent()});
-    }
+
     CB(std::move(Res));
   };
 
   WorkScheduler.runWithAST("EnumerateTweaks", File,
                            Bind(Action, std::move(CB), File.str()));
 }
 
 void ClangdServer::applyTweak(PathRef File, Range Sel, StringRef TweakID,

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

 };
 
 // All tweaks must be registered in the .cpp file next to their definition.
 #define REGISTER_TWEAK(Subclass)                                               \
   ::llvm::Registry<::clang::clangd::Tweak>::Add<Subclass>                      \
       TweakRegistrationFor##Subclass(#Subclass, /*Description=*/"");           \
   const char *Subclass::id() const { return #Subclass; }
 
-/// Calls prepare() on all tweaks, returning those that can run on the
-/// selection.
-std::vector<std::unique_ptr<Tweak>> prepareTweaks(const Tweak::Selection &S);
+/// Calls prepare() on all tweaks that satisfy the filter, returning those that
+/// can run on the selection.
+std::vector<std::unique_ptr<Tweak>>
+prepareTweaks(const Tweak::Selection &S,
+              llvm::function_ref<bool(const Tweak &)> Filter);
 
 // Calls prepare() on the tweak with a given ID.
 // If prepare() returns false, returns an error.
 // If prepare() returns true, returns the corresponding tweak.
 llvm::Expected<std::unique_ptr<Tweak>> prepareTweak(StringRef TweakID,
                                                     const Tweak::Selection &S);
 
 } // namespace clangd
 } // namespace clang
 
 #endif

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

 Tweak::Selection::Selection(ParsedAST &AST, unsigned RangeBegin,
                             unsigned RangeEnd)
     : AST(AST), ASTSelection(AST.getASTContext(), RangeBegin, RangeEnd) {
   auto &SM = AST.getSourceManager();
   Code = SM.getBufferData(SM.getMainFileID());
   Cursor = SM.getComposedLoc(SM.getMainFileID(), RangeBegin);
 }
 
-std::vector<std::unique_ptr<Tweak>> prepareTweaks(const Tweak::Selection &S) {
+std::vector<std::unique_ptr<Tweak>>
+prepareTweaks(const Tweak::Selection &S,
+              llvm::function_ref<bool(const Tweak &)> Filter) {
   validateRegistry();
 
   std::vector<std::unique_ptr<Tweak>> Available;
   for (const auto &E : TweakRegistry::entries()) {
     std::unique_ptr<Tweak> T = E.instantiate();
-    if (!T->prepare(S))
+    if (!Filter(*T) || !T->prepare(S))
       continue;
     Available.push_back(std::move(T));
   }
   // Ensure deterministic order of the results.
   llvm::sort(Available,
              [](const std::unique_ptr<Tweak> &L,
                 const std::unique_ptr<Tweak> &R) { return L->id() < R->id(); });
   return Available;

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.HiddenFeatures = HiddenFeatures;
 
   clangd::CodeCompleteOptions CCOpts;
   CCOpts.IncludeIneligibleResults = IncludeIneligibleResults;
   CCOpts.Limit = LimitResults;
   if (CompletionStyle.getNumOccurrences())
     CCOpts.BundleOverloads = CompletionStyle != Detailed;
   CCOpts.ShowOrigins = ShowOrigins;
   CCOpts.InsertIncludes = HeaderInsertion;
Opts.GetClangTidyOptions = [&](llvm::vfs::FileSystem &,
       // This function must be thread-safe and tidy option providers are not.
       std::lock_guard<std::mutex> Lock(ClangTidyOptMu);
       // FIXME: use the FS provided to the function.
       return ClangTidyOptProvider->getOptions(File);
     };
   }
   Opts.SuggestMissingIncludes = SuggestMissingIncludes;
   Opts.QueryDriverGlobs = std::move(QueryDriverGlobs);
+
+  Opts.TweakFilter = [&](const Tweak &T) {
+    if (T.hidden() && !HiddenFeatures)
+      return false;
     if (TweakList.getNumOccurrences())
-    Opts.TweakFilter = [&](llvm::StringRef TweakToSearch) {
-      // return true if any tweak matches the TweakToSearch
-      return llvm::find(TweakList, TweakToSearch) != TweakList.end();
-    };
+      return llvm::is_contained(TweakList, T.id());
+    return true;
   llvm::Optional<OffsetEncoding> OffsetEncodingFromFlag;
   if (ForceOffsetEncoding != OffsetEncoding::UnsupportedEncoding)
     OffsetEncodingFromFlag = ForceOffsetEncoding;
   ClangdLSPServer LSPServer(
       *TransportLayer, FSProvider, CCOpts, CompileCommandsDirPath,
       /*UseDirBasedCDB=*/CompileArgsFrom == FilesystemCompileArgs,
       OffsetEncodingFromFlag, Opts);
   llvm::set_thread_name("clangd.main");
   return LSPServer.run() ? 0
                          : static_cast<int>(ErrorResultCode::NoShutdownRequest);
 }