[clangd] Don't expose vfs in TUScheduler::runWithPreamble.

[ilya-biryukov]

Summary

It previously an easy way to concurrently access a mutable vfs, which
is a recipe for disaster.

[sammccall]

clangd/TUScheduler.h

81–82

I think this is the right place to describe the way FS interaction works (which implies why we don't propagate vfs):

/// The preamble may be stale, generated from an older version of the file.
/// Reading from locations in the preamble may cause the files to be re-read.
/// This gives callers two options:
/// - validate that the preamble is still valid, and only use it in this case
/// - accept that preamble contents may be outdated, and try to avoid reading source code from headers.

[ilya-biryukov]

• Rebase on top of master
• Add the suggested comment

clangd/ClangdServer.cpp

void ClangdServer::codeComplete(PathRef File, Position Pos,
   // Copy PCHs to avoid accessing this->PCHs concurrently
   std::shared_ptr<PCHContainerOperations> PCHs = this->PCHs;
   auto FS = FSProvider.getFileSystem();
   auto Task = [PCHs, Pos, FS, CodeCompleteOpts](
                   std::string Contents, Path File, Callback<CompletionList> CB,
                   llvm::Expected<InputsAndPreamble> IP) {
     assert(IP && "error when trying to read preamble for codeComplete");
     auto PreambleData = IP->Preamble;
-    auto &Command = IP->Inputs.CompileCommand;
 
     // FIXME(ibiryukov): even if Preamble is non-null, we may want to check
     // both the old and the new version in case only one of them matches.
     CompletionList Result = clangd::codeComplete(
-        File, Command, PreambleData ? &PreambleData->Preamble : nullptr,
+        File, IP->Command, PreambleData ? &PreambleData->Preamble : nullptr,
         Contents, Pos, FS, PCHs, CodeCompleteOpts);
     CB(std::move(Result));
   };
 
   WorkScheduler.runWithPreamble(
       "CodeComplete", File,
       Bind(Task, std::move(*Latest.Draft), File.str(), std::move(CB)));
 }
void ClangdServer::signatureHelp(PathRef File, Position Pos,
   auto FS = FSProvider.getFileSystem();
   auto Action = [Pos, FS, PCHs](std::string Contents, Path File,
                                 Callback<SignatureHelp> CB,
                                 llvm::Expected<InputsAndPreamble> IP) {
     if (!IP)
       return CB(IP.takeError());
 
     auto PreambleData = IP->Preamble;
-    auto &Command = IP->Inputs.CompileCommand;
-    CB(clangd::signatureHelp(File, Command,
+    CB(clangd::signatureHelp(File, IP->Command,
                              PreambleData ? &PreambleData->Preamble : nullptr,
                              Contents, Pos, FS, PCHs));
   };
 
   WorkScheduler.runWithPreamble(
       "SignatureHelp", File,
       Bind(Action, std::move(*Latest.Draft), File.str(), std::move(CB)));
 }

clangd/TUScheduler.h

 unsigned getDefaultAsyncThreadsCount();
 
 struct InputsAndAST {
   const ParseInputs &Inputs;
   ParsedAST *
 };
 
 struct InputsAndPreamble {
-  const ParseInputs &Inputs;
+  llvm::StringRef Contents;
+  const tooling::CompileCommand &Command;
   const PreambleData *Preamble;
 };
 
 /// Determines whether diagnostics should be generated for a file snapshot.
 enum class WantDiagnostics {
   Yes,  /// Diagnostics must be generated for this snapshot.
   No,   /// Diagnostics must not be generated for this snapshot.
   Auto, /// Diagnostics must be generated for this snapshot or a subsequent one,
public:
   /// Schedule an async read of the AST. \p Action will be called when AST is
   /// ready. The AST passed to \p Action refers to the version of \p File
   /// tracked at the time of the call, even if new updates are received before
   /// \p Action is executed.
   /// If an error occurs during processing, it is forwarded to the \p Action
   /// callback.
   void runWithAST(llvm::StringRef Name, PathRef File,
                   Callback<InputsAndAST> Action);
 
-  /// Schedule an async read of the Preamble. Preamble passed to \p Action may
-  /// be built for any version of the file, callers must not rely on it being
-  /// consistent with the current version of the file.
-  /// If an error occurs during processing, it is forwarded to the \p Action
-  /// callback.
+  /// Schedule an async read of the Preamble.
+  /// The preamble may be stale, generated from an older version of the file.
+  /// Reading from locations in the preamble may cause the files to be re-read.
+  /// This gives callers two options:
+  /// - validate that the preamble is still valid, and only use it in this case
+  /// - accept that preamble contents may be outdated, and try to avoid reading
+  /// source code from headers. If an error occurs during processing, it is
+  /// forwarded to the \p Action callback.
   void runWithPreamble(llvm::StringRef Name, PathRef File,
                        Callback<InputsAndPreamble> Action);
 
   /// Wait until there are no scheduled or running tasks.
   /// Mostly useful for synchronizing tests.
   bool blockUntilIdle(Deadline D) const;
 
 private:

clangd/TUScheduler.cpp

unsigned getDefaultAsyncThreadsCount() {
   // that case.
   if (HardwareConcurrency == 0)
     return 1;
   return HardwareConcurrency;
 }
 
 struct TUScheduler::FileData {
   /// Latest inputs, passed to TUScheduler::update().
-  ParseInputs Inputs;
+  std::string Contents;
+  tooling::CompileCommand Command;
   ASTWorkerHandle Worker;
 };
 
 TUScheduler::TUScheduler(unsigned AsyncThreadsCount,
                          bool StorePreamblesInMemory,
                          ASTParsedCallback ASTCallback,
                          steady_clock::duration UpdateDebounce)
     : StorePreamblesInMemory(StorePreamblesInMemory),
void TUScheduler::update(PathRef File, ParseInputs Inputs,
                          UniqueFunction<void(std::vector<Diag>)> OnUpdated) {
   std::unique_ptr<FileData> &FD = Files[File];
   if (!FD) {
     // Create a new worker to process the AST-related tasks.
     ASTWorkerHandle Worker = ASTWorker::Create(
         File, WorkerThreads ? WorkerThreads.getPointer() : nullptr, Barrier,
         CppFile(File, StorePreamblesInMemory, PCHOps, ASTCallback),
         UpdateDebounce);
-    FD = std::unique_ptr<FileData>(new FileData{Inputs, std::move(Worker)});
+    FD = std::unique_ptr<FileData>(new FileData{
+        Inputs.Contents, Inputs.CompileCommand, std::move(Worker)});
   } else {
-    FD->Inputs = Inputs;
+    FD->Contents = Inputs.Contents;
+    FD->Command = Inputs.CompileCommand;
   }
   FD->Worker->update(std::move(Inputs), WantDiags, std::move(OnUpdated));
 }
 
 void TUScheduler::remove(PathRef File) {
   bool Removed = Files.erase(File);
   if (!Removed)
     log("Trying to remove file from TUScheduler that is not tracked. File:" +
if (It == Files.end()) {
     return;
   }
 
   if (!PreambleTasks) {
     trace::Span Tracer(Name);
     SPAN_ATTACH(Tracer, "file", File);
     std::shared_ptr<const PreambleData> Preamble =
         It->second->Worker->getPossiblyStalePreamble();
-    Action(InputsAndPreamble{It->second->Inputs, Preamble.get()});
+    Action(InputsAndPreamble{It->second->Contents, It->second->Command,
+                             Preamble.get()});
     return;
   }
 
-  ParseInputs InputsCopy = It->second->Inputs;
   std::shared_ptr<const ASTWorker> Worker = It->second->Worker.lock();
-  auto Task = [InputsCopy, Worker, this](std::string Name, std::string File,
-                                         Context Ctx,
-                                         decltype(Action) Action) mutable {
+  auto Task = [Worker, this](std::string Name, std::string File,
+                             std::string Contents,
+                             tooling::CompileCommand Command, Context Ctx,
     std::lock_guard<Semaphore> BarrierLock(Barrier);
     WithContext Guard(std::move(Ctx));
     trace::Span Tracer(Name);
     SPAN_ATTACH(Tracer, "file", File);
     std::shared_ptr<const PreambleData> Preamble =
         Worker->getPossiblyStalePreamble();
-    Action(InputsAndPreamble{InputsCopy, Preamble.get()});
+    Action(InputsAndPreamble{Contents, Command, Preamble.get()});
   };
 
   PreambleTasks->runAsync("task:" + llvm::sys::path::filename(File),
                           Bind(Task, std::string(Name), std::string(File),
+                               It->second->Contents, It->second->Command,
                                Context::current().clone(), std::move(Action)));
 }
 
 std::vector<std::pair<Path, std::size_t>>
 TUScheduler::getUsedBytesPerFile() const {
   std::vector<std::pair<Path, std::size_t>> Result;
   Result.reserve(Files.size());
   for (auto &&PathAndFile : Files)
     Result.push_back(
         {PathAndFile.first(), PathAndFile.second->Worker->getUsedBytes()});
   return Result;
 }
 
 } // namespace clangd
 } // namespace clang

unittests/clangd/TUSchedulerTests.cpp

for (int FileI = 0; FileI < FilesCount; ++FileI) {
           WithContextValue WithNonce(NonceKey, ++Nonce);
           S.runWithPreamble(
               "CheckPreamble", File,
               [Inputs, Nonce, &Mut, &TotalPreambleReads](
                   llvm::Expected<InputsAndPreamble> Preamble) {
                 EXPECT_THAT(Context::current().get(NonceKey), Pointee(Nonce));
 
                 ASSERT_TRUE((bool)Preamble);
-                EXPECT_EQ(Preamble->Inputs.FS, Inputs.FS);
-                EXPECT_EQ(Preamble->Inputs.Contents, Inputs.Contents);
+                EXPECT_EQ(Preamble->Contents, Inputs.Contents);
 
                 std::lock_guard<std::mutex> Lock(Mut);
                 ++TotalPreambleReads;
               });
         }
       }
     }
   } // TUScheduler destructor waits for all operations to finish.