[clangd] Retire some flags for uncontroversial, stable features.

[sammccall]

Summary

And mark a couple to be retired afther the next release branch.

[hokein]

the code looks good to me, but we need to be a bit careful on landing this -- as we have an internal client setting this flag.

clang-tools-extra/clangd/ClangdServer.h

139

our internal client explicitly set this to true, so we need a migration plan for this, otherwise this would break our build of internal client during the integration, a possible plan is

1. set this flag to true by default in upstream, wait for the integration
2. remove the explicit setting internally
3. remove this flag (this patch) in upstream

Or just remove the flag internally, then land this patch in upstream (but internal release has to pick-up these two together)

[sammccall]

clang-tools-extra/clangd/ClangdServer.h

139

Discussed offline - there's no reason to block for out-of-tree clients here.

(We're removing support for a configuration - SuggestMissingIncludes = false - which is AFAIK unused anywhere. And we're removing the flag as well, but this is a trivial API change to adapt to)

[ivanmurashko]

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

762

@sammccall The option is always true, do we need it as an option?

[sammccall]

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

762

It's false in tests, except when we're testing the index specifically.
On balance I think this is a good think because it allows us to reason about a simpler system when designing/debugging tests. In particular we don't have to worry about the AST-paths in tests "cheating" by looking at the index.

Seems sensible to make the struct default true and set it to false explicitly in optsForTest, though.

clang-tools-extra/clangd/ClangdServer.h

struct Options {
 
     /// Time to wait after a new file version before computing diagnostics.
     DebouncePolicy UpdateDebounce = DebouncePolicy{
         /*Min=*/std::chrono::milliseconds(50),
         /*Max=*/std::chrono::milliseconds(500),
         /*RebuildRatio=*/1,
     };
 
-    bool SuggestMissingIncludes = false;
-
     /// Clangd will execute compiler drivers matching one of these globs to
     /// fetch system include path.
     std::vector<std::string> QueryDriverGlobs;
 
     /// Enable notification-based semantic highlighting.
     bool TheiaSemanticHighlighting = false;
 
     /// Enable preview of FoldingRanges feature.
private:
   // If present, the new "auto-index" maintained in background threads.
   std::unique_ptr<BackgroundIndex> BackgroundIdx;
   // Storage for merged views of the various indexes.
   std::vector<std::unique_ptr<SymbolIndex>> MergedIdx;
 
   // When set, provides clang-tidy options for a specific file.
   TidyProviderRef ClangTidyProvider;
 
-  // 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;
-
   // 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
   // called before all other members to stop the worker thread that references
   // ClangdServer.
   TUScheduler WorkScheduler;
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/clangd/ClangdServer.cpp

ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB,
                            Callbacks *Callbacks)
     : ConfigProvider(Opts.ConfigProvider), CDB(CDB), TFS(TFS),
       ServerCallbacks(Callbacks),
       DynamicIdx(Opts.BuildDynamicSymbolIndex
                      ? new FileIndex(Opts.HeavyweightDynamicSymbolIndex,
                                      Opts.CollectMainFileRefs)
                      : nullptr),
       ClangTidyProvider(Opts.ClangTidyProvider),
-      SuggestMissingIncludes(Opts.SuggestMissingIncludes),
       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,
ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB,
   if (DynamicIdx)
     AddIndex(DynamicIdx.get());
 }
 
 void ClangdServer::addDocument(PathRef File, llvm::StringRef Contents,
                                llvm::StringRef Version,
                                WantDiagnostics WantDiags, bool ForceRebuild) {
   ParseOptions Opts;
-  Opts.SuggestMissingIncludes = SuggestMissingIncludes;
 
   // Compile command is set asynchronously during update, as it can be slow.
   ParseInputs Inputs;
   Inputs.TFS = &TFS;
   Inputs.Contents = std::string(Contents);
   Inputs.Version = Version.str();
   Inputs.ForceRebuild = ForceRebuild;
   Inputs.Opts = std::move(Opts);

clang-tools-extra/clangd/Compiler.h

static void log(DiagnosticsEngine::Level DiagLevel,
                   const clang::Diagnostic &Info);
 
   void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
                         const clang::Diagnostic &Info) override;
 };
 
 // Options to run clang e.g. when parsing AST.
 struct ParseOptions {
-  bool SuggestMissingIncludes = false;
+  // (empty at present, formerly controlled recovery AST, include-fixer etc)
 };
 
 /// Information required to run clang, e.g. to parse AST or do code completion.
 struct ParseInputs {
   tooling::CompileCommand CompileCommand;
   const ThreadsafeFS *TFS;
   std::string Contents;
   // Version identifier for Contents, provided by the client and opaque to us.

clang-tools-extra/clangd/ParsedAST.cpp

if (!CTChecks.empty()) {
       });
     }
   }
 
   // Add IncludeFixer which can recover diagnostics caused by missing includes
   // (e.g. incomplete type) and attach include insertion fixes to diagnostics.
   llvm::Optional<IncludeFixer> FixIncludes;
   auto BuildDir = VFS->getCurrentWorkingDirectory();
-  if (Inputs.Opts.SuggestMissingIncludes && Inputs.Index &&
-      !BuildDir.getError()) {
+  if (Inputs.Index && !BuildDir.getError()) {
     auto Style = getFormatStyleForFile(Filename, Inputs.Contents, *Inputs.TFS);
     auto Inserter = std::make_shared<IncludeInserter>(
         Filename, Inputs.Contents, Style, BuildDir.get(),
         &Clang->getPreprocessor().getHeaderSearchInfo());
     if (Preamble) {
       for (const auto &Inc : Preamble->Includes.MainFileIncludes)
         Inserter->addExisting(Inc);
     }

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

 using llvm::cl::values;
 
 // All flags must be placed in a category, or they will be shown neither in
 // --help, nor --help-hidden!
 OptionCategory CompileCommands("clangd compilation flags options");
 OptionCategory Features("clangd feature options");
 OptionCategory Misc("clangd miscellaneous options");
 OptionCategory Protocol("clangd protocol and logging options");
+OptionCategory Retired("clangd flags no longer in use");
 const OptionCategory *ClangdCategories[] = {&Features, &Protocol,
-                                            &CompileCommands, &Misc};
+                                            &CompileCommands, &Misc, &Retired};
+
+template <typename T> class RetiredFlag {
+  opt<T> Option;
+
+public:
+  RetiredFlag(llvm::StringRef Name)
+      : Option(Name, cat(Retired), desc("Obsolete flag, ignored"), Hidden,
+               llvm::cl::callback([Name](const T &) {
+                 llvm::errs()
+                     << "The flag `-" << Name << "` is obsolete and ignored.\n";
+               })) {}
+};
 
 enum CompileArgsFrom { LSPCompileArgs, FilesystemCompileArgs };
 opt<CompileArgsFrom> CompileArgsFrom{
     "compile_args_from",
     cat(CompileCommands),
     desc("The source of compile commands"),
     values(clEnumValN(LSPCompileArgs, "lsp",
                       "All compile commands come from LSP and "

 opt<bool> IncludeIneligibleResults{
     "include-ineligible-results",
     cat(Features),
     desc("Include ineligible completion results (e.g. private members)"),
     init(CodeCompleteOptions().IncludeIneligibleResults),
     Hidden,
 };
 
-opt<bool> EnableIndex{
-    "index",
-    cat(Features),
-    desc("Enable index-based features. By default, clangd maintains an index "
-         "built from symbols in opened files. Global index support needs to "
-         "enabled separatedly"),
-    init(true),
-    Hidden,
-};
+RetiredFlag<bool> EnableIndex("index");
 
 opt<int> LimitResults{
     "limit-results",
     cat(Features),
     desc("Limit the number of results returned by clangd. "
          "0 means no limit (default=100)"),
     init(100),
 };
 
-opt<bool> SuggestMissingIncludes{
-    "suggest-missing-includes",
-    cat(Features),
-    desc("Attempts to fix diagnostic errors caused by missing "
-         "includes using index"),
-    init(true),
-};
+RetiredFlag<bool> SuggestMissingIncludes("suggest-missing-includes");
 
 list<std::string> TweakList{
     "tweaks",
     cat(Features),
     desc("Specify a list of Tweaks to enable (only for clangd developers)."),
     Hidden,
     CommaSeparated,
 };
 
 opt<bool> CrossFileRename{
     "cross-file-rename",
     cat(Features),
     desc("Enable cross-file rename feature."),
     init(true),
 };
 
-opt<bool> RecoveryAST{
-    "recovery-ast",
-    cat(Features),
-    desc("Preserve expressions in AST for broken code."),
-    init(false),
-    Hidden,
-};
-
-opt<bool> RecoveryASTType{
-    "recovery-ast-type",
-    cat(Features),
-    desc("Preserve the type for recovery AST."),
-    init(false),
-    Hidden,
-};
+RetiredFlag<bool> RecoveryAST("recovery-ast");
+RetiredFlag<bool> RecoveryASTType("recovery-ast-type");
 
 opt<bool> FoldingRanges{
     "folding-ranges",
     cat(Features),
     desc("Enable preview of FoldingRanges feature"),
     init(false),
     Hidden,
 };

 
 opt<bool> PrettyPrint{
     "pretty",
     cat(Protocol),
     desc("Pretty-print JSON output"),
     init(false),
 };
 
+// FIXME: retire this flag in llvm 13 release cycle.
 opt<bool> AsyncPreamble{
     "async-preamble",
     cat(Misc),
     desc("Reuse even stale preambles, and rebuild them in the background. This "
          "improves latency at the cost of accuracy."),
     init(ClangdServer::Options().AsyncPreambleBuilds),
     Hidden,
 };
 
 opt<bool> EnableConfig{
     "enable-config",
     cat(Misc),
     desc(
         "Read user and project configuration from YAML files.\n"
         "Project config is from a .clangd file in the project directory.\n"
         "User config is from clangd/config.yaml in the following directories:\n"
         "\tWindows: %USERPROFILE%\\AppData\\Local\n"
         "\tMac OS: ~/Library/Preferences/\n"
         "\tOthers: $XDG_CONFIG_HOME, usually ~/.config\n"
         "Configuration is documented at https://clangd.llvm.org/config.html"),
     init(true),
 };
 
+// FIXME: retire this flag in llvm 13 release cycle.
 opt<bool> CollectMainFileRefs{
     "collect-main-file-refs",
     cat(Misc),
     desc("Store references to main-file-only symbols in the index"),
     init(ClangdServer::Options().CollectMainFileRefs),
+    Hidden,
 };
 
 #if defined(__GLIBC__) && CLANGD_MALLOC_TRIM
 opt<bool> EnableMallocTrim{
     "malloc-trim",
     cat(Misc),
     desc("Release memory periodically via malloc_trim(3)."),
     init(true),
case PCHStorageFlag::Memory:
     Opts.StorePreamblesInMemory = true;
     break;
   case PCHStorageFlag::Disk:
     Opts.StorePreamblesInMemory = false;
     break;
   }
   if (!ResourceDir.empty())
     Opts.ResourceDir = ResourceDir;
-  Opts.BuildDynamicSymbolIndex = EnableIndex;
+  Opts.BuildDynamicSymbolIndex = true;
   Opts.CollectMainFileRefs = CollectMainFileRefs;
   std::vector<std::unique_ptr<SymbolIndex>> IdxStack;
   std::unique_ptr<SymbolIndex> StaticIdx;
   std::future<void> AsyncIndexLoad; // Block exit while loading the index.
-  if (EnableIndex && !IndexFile.empty()) {
+  if (!IndexFile.empty()) {
     // Load the index asynchronously. Meanwhile SwapIndex returns no results.
     SwapIndex *Placeholder;
     StaticIdx.reset(Placeholder = new SwapIndex(std::make_unique<MemIndex>()));
     AsyncIndexLoad = runAsync<void>([Placeholder] {
       if (auto Idx = loadIndex(IndexFile, /*UseDex=*/true))
         Placeholder->reset(std::move(Idx));
     });
     if (Sync)
if (!ClangTidyChecks.empty())
       Providers.push_back(addTidyChecks(ClangTidyChecks));
     else
       Providers.push_back(provideDefaultChecks());
     Providers.push_back(disableUnusableChecks());
     ClangTidyOptProvider = combine(std::move(Providers));
     Opts.ClangTidyProvider = ClangTidyOptProvider;
   }
   Opts.AsyncPreambleBuilds = AsyncPreamble;
-  Opts.SuggestMissingIncludes = SuggestMissingIncludes;
   Opts.QueryDriverGlobs = std::move(QueryDriverGlobs);
   Opts.TweakFilter = [&](const Tweak &T) {
     if (T.hidden() && !HiddenFeatures)
       return false;
     if (TweakList.getNumOccurrences())
       return llvm::is_contained(TweakList, T.id());
     return true;
   };

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

ParseInputs TestTU::inputs(MockFS &FS) const {
   Inputs.Contents = Code;
   if (OverlayRealFileSystemForModules)
     FS.OverlayRealFileSystemForModules = true;
   Inputs.TFS = &FS;
   Inputs.Opts = ParseOptions();
   if (ClangTidyProvider)
     Inputs.ClangTidyProvider = ClangTidyProvider;
   Inputs.Index = ExternalIndex;
-  if (Inputs.Index)
-    Inputs.Opts.SuggestMissingIncludes = true;
   return Inputs;
 }
 
 void initializeModuleCache(CompilerInvocation &CI) {
   llvm::SmallString<128> ModuleCachePath;
   ASSERT_FALSE(
       llvm::sys::fs::createUniqueDirectory("module-cache", ModuleCachePath));
   CI.getHeaderSearchOpts().ModuleCachePath = ModuleCachePath.c_str();