[clangd] Add option to use dirty file contents when building preambles.

[njames93]

Summary

Adds a option use-dirty-preambles to enable using unsaved in editor contents when building pre-ambles.
This enables a more seamless user experience when switching between header and implementation files and forgetting to save inbetween.
It's also in line with the LSP spec that states open files in the editor should be used instead of on the contents on disk - https://microsoft.github.io/language-server-protocol/overviews/lsp/overview/
For now the option is defaulted to off and hidden, Though I have a feeling it should be moved into the .clangd config and possibly defaulted to true.

Addresses https://github.com/clangd/clangd/issues/488

[njames93]

Rebase

[njames93]

@sammccall What else is needed for this to go in?

[sammccall]

Sorry about letting this sit. I think this is good to go, with a naming tweak.

For the 14 cycle, it should be hidden: we may want to tweak the interactions with didSave reparsing etc, and there may be other unexpected wrinkles.

Regarding moving to config: we should do this, but there's some dumb bikeshedding of names (mostly: does this go at the top level or do we want a Parsing block or something, will other things fit in there, etc). So a hidden flag seems like the easiest thing for now.

clang-tools-extra/clangd/ClangdServer.h

169

Naming (here and throughout). On reflection the use of "preamble" isn't ideal:

• it mostly affects the preamble, but not exclusively: #includes in the non-preamble region also use this FS.
• it's jargony, and if we want to put this in config we should use a friendlier name. Might as well find one now.

I'd suggest a boolean UseDirtyHeaders for now, and if we move it to config then something like HeaderContents: Saved|Dirty.

[njames93]

In D95046#3247793, @sammccall wrote:

Sorry about letting this sit. I think this is good to go, with a naming tweak.

For the 14 cycle, it should be hidden: we may want to tweak the interactions with didSave reparsing etc, and there may be other unexpected wrinkles.

Regarding moving to config: we should do this, but there's some dumb bikeshedding of names (mostly: does this go at the top level or do we want a Parsing block or something, will other things fit in there, etc). So a hidden flag seems like the easiest thing for now.

I've been using this locally for months. The one nit i have is if you edit a header file then switch to a different file, diagnostics aren't updated until you force clangd to reparse the file (by editing it for example).
However I'm not sure of a nice way around this that doesn't involve trying to rebuild the world when you make an edit to a header.

[njames93]

Update name

[sammccall]

In D95046#3247844, @njames93 wrote:

In D95046#3247793, @sammccall wrote:

Sorry about letting this sit. I think this is good to go, with a naming tweak.

For the 14 cycle, it should be hidden: we may want to tweak the interactions with didSave reparsing etc, and there may be other unexpected wrinkles.

Regarding moving to config: we should do this, but there's some dumb bikeshedding of names (mostly: does this go at the top level or do we want a Parsing block or something, will other things fit in there, etc). So a hidden flag seems like the easiest thing for now.

I've been using this locally for months. The one nit i have is if you edit a header file then switch to a different file, diagnostics aren't updated until you force clangd to reparse the file (by editing it for example).

That's the only real interaction I can think of too.
The didSave trigger for rebuild will still trigger the rebuild too I think. It doesn't make a great deal of sense in this mode, but it may still be a helpful workflow.

However I'm not sure of a nice way around this that doesn't involve trying to rebuild the world when you make an edit to a header.

Agree, this is tricky.

Maybe one idea is a large global debounce: we reparse all files when a header is saved *or* e.g. 30 seconds have passed with no edits to any file.

Exact details could vary, I think important aspects are: rebuilds should not scale with rate of edits, prefer the extra work to happen when relatively idle, clangd should still eventually reach a steady state where it's doing no work until it receives requests.

In any case I'd prefer to land in its current simple form, and land any further bits after the branch cut (soon).

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

497

Nit: when parsing headers

[njames93]

Nit

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 preview of FoldingRanges feature.
     bool FoldingRanges = false;
 
     FeatureModuleSet *FeatureModules = nullptr;
+    /// If true, use the dirty buffer contents when building Preambles.
+    bool UseDirtyHeaders = false;
 
     explicit operator TUScheduler::Options() const;
   };
   // Sensible default options for use in tests.
   // Features like indexing must be enabled if desired.
   static Options optsForTest();
 
   /// Creates a new ClangdServer instance.
public:
   blockUntilIdleForTest(llvm::Optional<double> TimeoutSeconds = 10);
 
   /// Builds a nested representation of memory used by components.
   void profile(MemoryTree &MT) const;
 
 private:
   FeatureModuleSet *FeatureModules;
   const GlobalCompilationDatabase &CDB;
+  const ThreadsafeFS &getHeaderFS() const {
+    return UseDirtyHeaders ? *DirtyFS : TFS;
+  }
   const ThreadsafeFS &TFS;
 
   Path ResourceDir;
   // The index used to look up symbols. This could be:
   //   - null (all index functionality is optional)
   //   - the dynamic index owned by ClangdServer (DynamicIdx)
   //   - the static index passed to the constructor
   //   - a merged view of a static and dynamic index (MergedIndex)
   const SymbolIndex *Index = nullptr;
   // If present, an index of symbols in open files. Read via *Index.
   std::unique_ptr<FileIndex> DynamicIdx;
   // 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;
 
+  bool UseDirtyHeaders = false;
+
   // GUARDED_BY(CachedCompletionFuzzyFindRequestMutex)
   llvm::StringMap<llvm::Optional<FuzzyFindRequest>>
       CachedCompletionFuzzyFindRequestByFile;
   mutable std::mutex CachedCompletionFuzzyFindRequestMutex;
 
   llvm::Optional<std::string> WorkspaceRoot;
   llvm::Optional<TUScheduler> WorkScheduler;
   // Invalidation policy used for actions that we assume are "transient".

clang-tools-extra/clangd/ClangdServer.cpp

 }
 
 ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB,
                            const ThreadsafeFS &TFS, const Options &Opts,
                            Callbacks *Callbacks)
     : FeatureModules(Opts.FeatureModules), CDB(CDB), TFS(TFS),
       DynamicIdx(Opts.BuildDynamicSymbolIndex ? new FileIndex() : nullptr),
       ClangTidyProvider(Opts.ClangTidyProvider),
-      WorkspaceRoot(Opts.WorkspaceRoot),
+      UseDirtyHeaders(Opts.UseDirtyHeaders), WorkspaceRoot(Opts.WorkspaceRoot),
       Transient(Opts.ImplicitCancellation ? TUScheduler::InvalidateOnUpdate
                                           : TUScheduler::NoInvalidation),
       DirtyFS(std::make_unique<DraftStoreFS>(TFS, DraftMgr)) {
   // 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.
   WorkScheduler.emplace(
       CDB, TUScheduler::Options(Opts),

 void ClangdServer::addDocument(PathRef File, llvm::StringRef Contents,
                                llvm::StringRef Version,
                                WantDiagnostics WantDiags, bool ForceRebuild) {
   std::string ActualVersion = DraftMgr.addDraft(File, Version, Contents);
   ParseOptions Opts;
 
   // Compile command is set asynchronously during update, as it can be slow.
   ParseInputs Inputs;
-  Inputs.TFS = &TFS;
+  Inputs.TFS = &getHeaderFS();
   Inputs.Contents = std::string(Contents);
   Inputs.Version = std::move(ActualVersion);
   Inputs.ForceRebuild = ForceRebuild;
   Inputs.Opts = std::move(Opts);
   Inputs.Index = Index;
   Inputs.ClangTidyProvider = ClangTidyProvider;
   Inputs.FeatureModules = FeatureModules;
   bool NewFile = WorkScheduler->update(File, Inputs, WantDiags);
if (!IP->Preamble) {
       vlog("Build for file {0} is not ready. Enter fallback mode.", File);
     } else if (CodeCompleteOpts.Index) {
       SpecFuzzyFind.emplace();
       {
         std::lock_guard<std::mutex> Lock(CachedCompletionFuzzyFindRequestMutex);
         SpecFuzzyFind->CachedReq = CachedCompletionFuzzyFindRequestByFile[File];
       }
     }
-    ParseInputs ParseInput{IP->Command, &TFS, IP->Contents.str()};
+    ParseInputs ParseInput{IP->Command, &getHeaderFS(), IP->Contents.str()};
     // FIXME: Add traling new line if there is none at eof, workaround a crash,
     // see https://github.com/clangd/clangd/issues/332
     if (!IP->Contents.endswith("\n"))
       ParseInput.Contents.append("\n");
     ParseInput.Index = Index;
 
     CodeCompleteOpts.MainFileSignals = IP->Signals;
     CodeCompleteOpts.AllScopes = Config::current().Completion.AllScopes;
auto Action = [Pos, File = File.str(), CB = std::move(CB),
                  this](llvm::Expected<InputsAndPreamble> IP) mutable {
     if (!IP)
       return CB(IP.takeError());
 
     const auto *PreambleData = IP->Preamble;
     if (!PreambleData)
       return CB(error("Failed to parse includes"));
 
-    ParseInputs ParseInput{IP->Command, &TFS, IP->Contents.str()};
+    ParseInputs ParseInput{IP->Command, &getHeaderFS(), IP->Contents.str()};
     // FIXME: Add traling new line if there is none at eof, workaround a crash,
     // see https://github.com/clangd/clangd/issues/332
     if (!IP->Contents.endswith("\n"))
       ParseInput.Contents.append("\n");
     ParseInput.Index = Index;
     CB(clangd::signatureHelp(File, Pos, *PreambleData, ParseInput,
                              DocumentationFormat));
   };

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

desc(
         "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),
 };
 
+opt<bool> UseDirtyHeaders{"use-dirty-headers", cat(Misc),
+                          desc("Use files open in the editor when parsing "
+                               "headers instead of reading from the disk"),
+                          Hidden,
+                          init(ClangdServer::Options().UseDirtyHeaders)};
+
 #if defined(__GLIBC__) && CLANGD_MALLOC_TRIM
 opt<bool> EnableMallocTrim{
     "malloc-trim",
     cat(Misc),
     desc("Release memory periodically via malloc_trim(3)."),
     init(true),
 };
 
if (EnableClangTidy) {
     Providers.push_back(provideClangTidyFiles(TFS));
     if (EnableConfig)
       Providers.push_back(provideClangdConfig());
     Providers.push_back(provideDefaultChecks());
     Providers.push_back(disableUnusableChecks());
     ClangTidyOptProvider = combine(std::move(Providers));
     Opts.ClangTidyProvider = ClangTidyOptProvider;
   }
+  Opts.UseDirtyHeaders = UseDirtyHeaders;
   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;
   };