[clangd] Change prepareCompilerInstance to take an FSProvider

[kadircet]

Summary

This makes API easier to use by moving setcwd and fscache setup into it.
Also ensures no side effects are observable on the VFS used by the
compiler instance.

[kadircet]

• Use Cmd.Directory when creating includefixer.

[sammccall]

Yay for getting rid of more VFSes passed around. Not sure about some of the other signature changes to prepareCompilerInstance. It seems a bit harder to call now.

In particular I'm not sure whether moving the preamble-stat-cache across setup from caller to callee is really an improvement once we modify that to wrap a ThreadsafeFS instead of a FS.
It's just... a reminder to use the cache, I guess? But the , nullptr, nullptr is annoying where we don't care about that.

clang-tools-extra/clangd/Compiler.cpp

104

why is this a reference if it can't be null?

(otherwise, remove the assert message - just repeats the assertion)

clang-tools-extra/clangd/Compiler.h

78

none of the new parameter names say anything beyond repeating the types, can we drop them?

79

nit: grouping of parameters seems a bit confusing, preamble + mainfile together (in either order) was clearer I think. DiagnosticConsumer is logically an output param, nice to keep it at the end. Why split the FSProvider and the FS cache, etc.

80

It doesn't seem reasonable to require both CompilerInvocation and CompileCommand. We can pass working dir in if we must...

(CompilerInvocation actually has a working directory field, but it looks like it potentially changes behavior. We could consider setting it in buildCompilerInvocation() in future...)

clang-tools-extra/clangd/ParsedAST.cpp

356

you appear to have added the fixme and... also fixed it

clang-tools-extra/clangd/index/Background.cpp

273

hmm!

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

78

why this change?

clang-tools-extra/clangd/CodeComplete.cpp

bool semaCodeComplete(std::unique_ptr<CodeCompleteConsumer> Consumer,
   // skip all includes in this case; these completions are really simple.
   PreambleBounds PreambleRegion =
       ComputePreambleBounds(*CI->getLangOpts(), ContentsBuffer.get(), 0);
   bool CompletingInPreamble = PreambleRegion.Size > Input.Offset;
   if (Input.Patch)
     Input.Patch->apply(*CI);
   // NOTE: we must call BeginSourceFile after prepareCompilerInstance. Otherwise
   // the remapped buffers do not get freed.
-  llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS =
-      Input.ParseInput.FSProvider->getFileSystem();
-  if (Input.Preamble.StatCache)
-    VFS = Input.Preamble.StatCache->getConsumingFS(std::move(VFS));
-  if (VFS->setCurrentWorkingDirectory(
-          Input.ParseInput.CompileCommand.Directory))
-    elog("Couldn't set working directory during code completion");
   auto Clang = prepareCompilerInstance(
-      std::move(CI), !CompletingInPreamble ? &Input.Preamble.Preamble : nullptr,
-      std::move(ContentsBuffer), std::move(VFS), IgnoreDiags);
+      std::move(CI), std::move(ContentsBuffer), IgnoreDiags,
+      Input.ParseInput.CompileCommand, Input.ParseInput.FSProvider,
+      !CompletingInPreamble ? &Input.Preamble.Preamble : nullptr,
+      Input.Preamble.StatCache.get());
   Clang->getPreprocessorOpts().SingleFileParseMode = CompletingInPreamble;
   Clang->setCodeCompletionConsumer(Consumer.release());
 
   SyntaxOnlyAction Action;
   if (!Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0])) {
     log("BeginSourceFile() failed when running codeComplete for {0}",
         Input.FileName);
     return false;

clang-tools-extra/clangd/Compiler.h

 // CodeComplete run their own compile actions that share these low-level pieces.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_COMPILER_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_COMPILER_H
 
 #include "../clang-tidy/ClangTidyOptions.h"
+#include "FS.h"
 #include "GlobalCompilationDatabase.h"
 #include "index/Index.h"
 #include "support/FSProvider.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/PrecompiledPreamble.h"
 #include "clang/Tooling/CompilationDatabase.h"
 
 namespace clang {

 buildCompilerInvocation(const ParseInputs &Inputs, clang::DiagnosticConsumer &D,
                         std::vector<std::string> *CC1Args = nullptr);
 
 /// Creates a compiler instance, configured so that:
 ///   - Contents of the parsed file are remapped to \p MainFile.
 ///   - Preamble is overriden to use PCH passed to this function. It means the
 ///     changes to the preamble headers or files included in the preamble are
 ///     not visible to this compiler instance.
-///   - llvm::vfs::FileSystem is used for all underlying file accesses. The
-///     actual vfs used by the compiler may be an overlay over the passed vfs.
+///   - \p FSProvider is used for all underlying file accesses. The actual vfs
+///     used by the compiler may be an overlay over the passed vfs.
 /// Returns null on errors. When non-null value is returned, it is expected to
 /// be consumed by FrontendAction::BeginSourceFile to properly destroy \p
 /// MainFile.
 std::unique_ptr<CompilerInstance> prepareCompilerInstance(
-    std::unique_ptr<clang::CompilerInvocation>, const PrecompiledPreamble *,
-    std::unique_ptr<llvm::MemoryBuffer> MainFile,
-    IntrusiveRefCntPtr<llvm::vfs::FileSystem>, DiagnosticConsumer &);
+    std::unique_ptr<clang::CompilerInvocation> CI,
+    std::unique_ptr<llvm::MemoryBuffer> Buffer, DiagnosticConsumer &DiagsClient,
+    const tooling::CompileCommand &Cmd, const FileSystemProvider *FSProvider,
+    const PrecompiledPreamble *Preamble,
+    const PreambleFileStatusCache *FSCache);
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_COMPILER_H

clang-tools-extra/clangd/Compiler.cpp

 //===--- Compiler.cpp --------------------------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Compiler.h"
+#include "FS.h"
+#include "support/FSProvider.h"
 #include "support/Logger.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Serialization/PCHContainerOperations.h"
+#include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/FormatVariadic.h"
 
 namespace clang {
 namespace clangd {
 
 void IgnoreDiagnostics::log(DiagnosticsEngine::Level DiagLevel,
                             const clang::Diagnostic &Info) {
buildCompilerInvocation(const ParseInputs &Inputs, clang::DiagnosticConsumer &D,
   // Recovery expression currently only works for C++.
   if (CI->getLangOpts()->CPlusPlus) {
     CI->getLangOpts()->RecoveryAST = Inputs.Opts.BuildRecoveryAST;
     CI->getLangOpts()->RecoveryASTType = Inputs.Opts.PreserveRecoveryASTType;
   }
   return CI;
 }
 
-std::unique_ptr<CompilerInstance>
-prepareCompilerInstance(std::unique_ptr<clang::CompilerInvocation> CI,
-                        const PrecompiledPreamble *Preamble,
-                        std::unique_ptr<llvm::MemoryBuffer> Buffer,
-                        llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS,
-                        DiagnosticConsumer &DiagsClient) {
-  assert(VFS && "VFS is null");
+std::unique_ptr<CompilerInstance> prepareCompilerInstance(
+    std::unique_ptr<clang::CompilerInvocation> CI,
+    std::unique_ptr<llvm::MemoryBuffer> Buffer, DiagnosticConsumer &DiagsClient,
+    const tooling::CompileCommand &Cmd, const FileSystemProvider *FSProvider,
+    const PreambleFileStatusCache *FSCache) {
+  assert(FSProvider && "FSProvider is null");
   assert(!CI->getPreprocessorOpts().RetainRemappedFileBuffers &&
          "Setting RetainRemappedFileBuffers to true will cause a memory leak "
          "of ContentsBuffer");
 
+  auto VFS = FSProvider->getFileSystem();
+  if (VFS->setCurrentWorkingDirectory(Cmd.Directory))
+    elog("Failed to set working directory while preparing compiler instance");
+  if (FSCache)
+    VFS = FSCache->getConsumingFS(std::move(VFS));
   // NOTE: we use Buffer.get() when adding remapped files, so we have to make
   // sure it will be released if no error is emitted.
   if (Preamble) {
     Preamble->OverridePreamble(*CI, VFS, Buffer.get());
   } else {
     CI->getPreprocessorOpts().addRemappedFile(
         CI->getFrontendOpts().Inputs[0].getFile(), Buffer.get());
   }

clang-tools-extra/clangd/ParsedAST.h

 
 /// Stores and provides access to parsed AST.
 class ParsedAST {
 public:
   /// Attempts to run Clang and store the parsed AST.
   /// If \p Preamble is non-null it is reused during parsing.
   /// This function does not check if preamble is valid to reuse.
   static llvm::Optional<ParsedAST>
-  build(llvm::StringRef Filename, const ParseInputs &Inputs,
+  build(PathRef AbsFilePath, const ParseInputs &Inputs,
         std::unique_ptr<clang::CompilerInvocation> CI,
         llvm::ArrayRef<Diag> CompilerInvocationDiags,
         std::shared_ptr<const PreambleData> Preamble);
 
   ParsedAST(ParsedAST &&Other);
   ParsedAST &operator=(ParsedAST &&Other);
 
   ~ParsedAST();

clang-tools-extra/clangd/ParsedAST.cpp

 //===--- ParsedAST.cpp -------------------------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "ParsedAST.h"
 #include "../clang-tidy/ClangTidyDiagnosticConsumer.h"
 #include "../clang-tidy/ClangTidyModuleRegistry.h"
 #include "AST.h"
 #include "Compiler.h"
 #include "Diagnostics.h"
+#include "FS.h"
 #include "Headers.h"
 #include "IncludeFixer.h"
 #include "Preamble.h"
 #include "SourceCode.h"
 #include "index/CanonicalIncludes.h"
 #include "index/Index.h"
 #include "support/Logger.h"
 #include "support/Trace.h"

 
 } // namespace
 
 void dumpAST(ParsedAST &AST, llvm::raw_ostream &OS) {
   AST.getASTContext().getTranslationUnitDecl()->dump(OS, true);
 }
 
 llvm::Optional<ParsedAST>
-ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
+ParsedAST::build(PathRef AbsFilePath, const ParseInputs &Inputs,
                  std::unique_ptr<clang::CompilerInvocation> CI,
                  llvm::ArrayRef<Diag> CompilerInvocationDiags,
                  std::shared_ptr<const PreambleData> Preamble) {
   trace::Span Tracer("BuildAST");
-  SPAN_ATTACH(Tracer, "File", Filename);
-
-  auto VFS = Inputs.FSProvider->getFileSystem();
-  if (Preamble && Preamble->StatCache)
-    VFS = Preamble->StatCache->getConsumingFS(std::move(VFS));
-  if (VFS->setCurrentWorkingDirectory(Inputs.CompileCommand.Directory)) {
-    log("Couldn't set working directory when building the preamble.");
-    // We proceed anyway, our lit-tests rely on results for non-existing working
-    // dirs.
-  }
+  SPAN_ATTACH(Tracer, "File", AbsFilePath);
 
   assert(CI);
   // Command-line parsing sets DisableFree to true by default, but we don't want
   // to leak memory in clangd.
   CI->getFrontendOpts().DisableFree = false;
-  const PrecompiledPreamble *PreamblePCH =
-      Preamble ? &Preamble->Preamble : nullptr;
+  const PrecompiledPreamble *PreamblePCH = nullptr;
+  const PreambleFileStatusCache *PreambleStatCache = nullptr;
+  if (Preamble) {
+    PreamblePCH = &Preamble->Preamble;
+    PreambleStatCache = Preamble->StatCache.get();
+  }
 
   // This is on-by-default in windows to allow parsing SDK headers, but it
   // breaks many features. Disable it for the main-file (not preamble).
   CI->getLangOpts()->DelayedTemplateParsing = false;
 
   StoreDiags ASTDiags;
 
   llvm::Optional<PreamblePatch> Patch;
   if (Preamble) {
-    Patch = PreamblePatch::create(Filename, Inputs, *Preamble);
+    Patch = PreamblePatch::create(AbsFilePath, Inputs, *Preamble);
     Patch->apply(*CI);
   }
   auto Clang = prepareCompilerInstance(
-      std::move(CI), PreamblePCH,
-      llvm::MemoryBuffer::getMemBufferCopy(Inputs.Contents, Filename), VFS,
-      ASTDiags);
+      std::move(CI),
+      llvm::MemoryBuffer::getMemBufferCopy(Inputs.Contents, AbsFilePath),
+      ASTDiags, Inputs.CompileCommand, Inputs.FSProvider, PreamblePCH,
+      PreambleStatCache);
   if (!Clang)
     return None;
 
   auto Action = std::make_unique<ClangdFrontendAction>();
   const FrontendInputFile &MainInput = Clang->getFrontendOpts().Inputs[0];
   if (!Action->BeginSourceFile(*Clang, MainInput)) {
     log("BeginSourceFile() failed when building AST for {0}",
         MainInput.getFile());
     return None;
   }
 
   // Set up ClangTidy. Must happen after BeginSourceFile() so ASTContext exists.
   // Clang-tidy has some limitations to ensure reasonable performance:
   //  - checks don't see all preprocessor events in the preamble
   //  - matchers run only over the main-file top-level decls (and can't see
   //    ancestors outside this scope).
   // In practice almost all checks work well without modifications.
   std::vector<std::unique_ptr<tidy::ClangTidyCheck>> CTChecks;
   ast_matchers::MatchFinder CTFinder;
   llvm::Optional<tidy::ClangTidyContext> CTContext;
   {
     trace::Span Tracer("ClangTidyInit");
-    dlog("ClangTidy configuration for file {0}: {1}", Filename,
+    dlog("ClangTidy configuration for file {0}: {1}", AbsFilePath,
          tidy::configurationAsText(Inputs.Opts.ClangTidyOpts));
     tidy::ClangTidyCheckFactories CTFactories;
     for (const auto &E : tidy::ClangTidyModuleRegistry::entries())
       E.instantiate()->addCheckFactories(CTFactories);
     CTContext.emplace(std::make_unique<tidy::DefaultOptionsProvider>(
         tidy::ClangTidyGlobalOptions(), Inputs.Opts.ClangTidyOpts));
     CTContext->setDiagnosticsEngine(&Clang->getDiagnostics());
     CTContext->setASTContext(&Clang->getASTContext());
-    CTContext->setCurrentFile(Filename);
+    CTContext->setCurrentFile(AbsFilePath);
     CTChecks = CTFactories.createChecks(CTContext.getPointer());
     ASTDiags.setLevelAdjuster([&CTContext](DiagnosticsEngine::Level DiagLevel,
                                            const clang::Diagnostic &Info) {
       if (CTContext) {
         std::string CheckName = CTContext->getCheckName(Info.getID());
         bool IsClangTidyDiag = !CheckName.empty();
         if (IsClangTidyDiag) {
           // Check for suppression comment. Skip the check for diagnostics not
for (const auto &Check : CTChecks) {
       Check->registerPPCallbacks(Clang->getSourceManager(), PP, PP);
       Check->registerMatchers(&CTFinder);
     }
   }
 
   // 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()) {
-    auto Style = getFormatStyleForFile(Filename, Inputs.Contents, VFS.get());
+  // FIXME: Why not use CompileCommand.Directory instead?
+  if (Inputs.Opts.SuggestMissingIncludes && Inputs.Index) {
+    auto Style = getFormatStyleForFile(
+        AbsFilePath, Inputs.Contents, Inputs.FSProvider->getFileSystem().get());
     auto Inserter = std::make_shared<IncludeInserter>(
-        Filename, Inputs.Contents, Style, BuildDir.get(),
+        AbsFilePath, Inputs.Contents, Style, Inputs.CompileCommand.Directory,
         &Clang->getPreprocessor().getHeaderSearchInfo());
     if (Preamble) {
       for (const auto &Inc : Preamble->Includes.MainFileIncludes)
         Inserter->addExisting(Inc);
     }
-    FixIncludes.emplace(Filename, Inserter, *Inputs.Index,
+    FixIncludes.emplace(AbsFilePath, Inserter, *Inputs.Index,
                         /*IndexRequestLimit=*/5);
     ASTDiags.contributeFixes([&FixIncludes](DiagnosticsEngine::Level DiagLevl,
                                             const clang::Diagnostic &Info) {
       return FixIncludes->fix(DiagLevl, Info);
     });
     Clang->setExternalSemaSource(FixIncludes->unresolvedNameRecorder());
   }
 

clang-tools-extra/clangd/Preamble.cpp

scanPreamble(llvm::StringRef Contents, const FileSystemProvider *FSProvider,
   // This means we're scanning (though not preprocessing) the preamble section
   // twice. However, it's important to precisely follow the preamble bounds used
   // elsewhere.
   auto Bounds =
       ComputePreambleBounds(*CI->getLangOpts(), ContentsBuffer.get(), 0);
   auto PreambleContents =
       llvm::MemoryBuffer::getMemBufferCopy(Contents.substr(0, Bounds.Size));
   auto Clang = prepareCompilerInstance(
-      std::move(CI), nullptr, std::move(PreambleContents),
+      std::move(CI), std::move(PreambleContents), IgnoreDiags, Cmd,
       // Provide an empty FS to prevent preprocessor from performing IO. This
       // also implies missing resolved paths for includes.
-      EmptyFSProvider()->getFileSystem(), IgnoreDiags);
+      EmptyFSProvider().get(), nullptr, nullptr);
   if (Clang->getFrontendOpts().Inputs.empty())
     return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                    "compiler instance had no inputs");
   // We are only interested in main file includes.
   Clang->getPreprocessorOpts().SingleFileParseMode = true;
   PreprocessOnlyAction Action;
   if (!Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]))
     return llvm::createStringError(llvm::inconvertibleErrorCode(),

clang-tools-extra/clangd/index/Background.cpp

for (const auto &FileIt : FilesToUpdate) {
     }
   }
 }
 
 llvm::Error BackgroundIndex::index(tooling::CompileCommand Cmd) {
   trace::Span Tracer("BackgroundIndex");
   SPAN_ATTACH(Tracer, "file", Cmd.Filename);
   auto AbsolutePath = getAbsolutePath(Cmd);
-
   auto FS = FSProvider.getFileSystem();
-  FS->setCurrentWorkingDirectory(Cmd.Directory);
   auto Buf = FS->getBufferForFile(AbsolutePath);
   if (!Buf)
     return llvm::errorCodeToError(Buf.getError());
   auto Hash = digest(Buf->get()->getBuffer());
 
   // Take a snapshot of the versions to avoid locking for each file in the TU.
   llvm::StringMap<ShardVersion> ShardVersionsSnapshot;
   {
     std::lock_guard<std::mutex> Lock(ShardVersionsMu);
     ShardVersionsSnapshot = ShardVersions;
   }
 
   vlog("Indexing {0} (digest:={1})", Cmd.Filename, llvm::toHex(Hash));
   ParseInputs Inputs;
   Inputs.FSProvider = &FSProvider;
   Inputs.CompileCommand = std::move(Cmd);
   IgnoreDiagnostics IgnoreDiags;
   auto CI = buildCompilerInvocation(Inputs, IgnoreDiags);
   if (!CI)
     return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                    "Couldn't build compiler invocation");
 
   auto Clang =
-      prepareCompilerInstance(std::move(CI), /*Preamble=*/nullptr,
-                              std::move(*Buf), std::move(FS), IgnoreDiags);
+      prepareCompilerInstance(std::move(CI), std::move(*Buf), IgnoreDiags,
+                              Inputs.CompileCommand, &FSProvider,
+                              /*Preamble=*/nullptr,
+                              /*FSCache=*/nullptr);
   if (!Clang)
     return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                    "Couldn't build compiler instance");
 
   SymbolCollector::Options IndexOpts;
   // Creates a filter to not collect index results from files with unchanged
   // digests.
   IndexOpts.FileFilter = [&ShardVersionsSnapshot](const SourceManager &SM,

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

std::unique_ptr<CompilerInstance> setupClang() {
 
     ParseInputs PI;
     PI.CompileCommand = *Cmd;
     PI.FSProvider = &FS;
     auto CI = buildCompilerInvocation(PI, IgnoreDiags);
     EXPECT_TRUE(static_cast<bool>(CI));
     // The diagnostic options must be set before creating a CompilerInstance.
     CI->getDiagnosticOpts().IgnoreWarnings = true;
-    auto VFS = FS.getFileSystem();
-    VFS->setCurrentWorkingDirectory(Cmd->Directory);
     auto Clang = prepareCompilerInstance(
-        std::move(CI), /*Preamble=*/nullptr,
+        std::move(CI),
         llvm::MemoryBuffer::getMemBuffer(FS.Files[MainFile], MainFile),
-        std::move(VFS), IgnoreDiags);
+        IgnoreDiags, *Cmd, &FS,
+        /*Preamble=*/nullptr,
+        /*FSCache=*/nullptr);
 
     EXPECT_FALSE(Clang->getFrontendOpts().Inputs.empty());
     return Clang;
   }
 
 protected:
   IncludeStructure collectIncludes() {
     auto Clang = setupClang();

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

collectPatchedIncludes(llvm::StringRef ModifiedContents,
   auto CI = buildCompilerInvocation(PI, Diags);
   PP.apply(*CI);
   // Run preprocessor over the modified contents with patched Invocation. We
   // provide a preamble and trim contents to ensure only the implicit header
   // introduced by the patch is parsed and nothing else.
   // We don't run PP directly over the patch cotents to test production
   // behaviour.
   auto Bounds = Lexer::ComputePreamble(ModifiedContents, *CI->getLangOpts());
-  auto Clang =
-      prepareCompilerInstance(std::move(CI), &BaselinePreamble->Preamble,
-                              llvm::MemoryBuffer::getMemBufferCopy(
-                                  ModifiedContents.slice(0, Bounds.Size).str()),
-                              PI.FSProvider->getFileSystem(), Diags);
+  auto Clang = prepareCompilerInstance(
+      std::move(CI),
+      Diags, PI.CompileCommand, PI.FSProvider, &BaselinePreamble->Preamble,
+      BaselinePreamble->StatCache.get());
   PreprocessOnlyAction Action;
   if (!Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0])) {
     ADD_FAILURE() << "failed begin source file";
     return {};
   }
   IncludeStructure Includes;
   Clang->getPreprocessor().addPPCallbacks(
       collectIncludeStructureCallback(Clang->getSourceManager(), &Includes));