[clangd] Record the file being processed in a TUScheduler thread in context.

[ioeric]

Summary

This allows implementations like different symbol indexes to know what
the current active file is. For example, some customized index implementation
might decide to only return results for some files.

[ilya-biryukov]

Short summary of the offline discussion: I suggest adding this parameter into the corresponding requests of the index (FuzzyFindRequest, FindDefinitionsRequest) instead of stashing it in the context. Context has all the same problems as the global variables, so we should try to avoid using it where possible.
On the other hand, where this key is stored might not be terribly important if we don't have too many usages and remove it when we can workaround the limitations that we're currently facing.

In any case, we should add a comment that the active file should be avoided and will be removed.

clangd/ClangdServer.cpp

162 ↗

(On Diff #159701)

If we want to set this value more consistently, maybe do it in TUScheduler::runWithPreamble/runWithAST?
All interesting operations that work on files go through those, so it's easier to certify that the file is set correctly.

clangd/Context.cpp

35 ↗

(On Diff #159701)

Maybe move it somewhere else? E.g. to Index.h, since that's where it's supposed to be used to workaround current limitations?
Context.h/cpp is a general-purpose support library for the contexts, application-specific keys do not fit in well there.

35 ↗

(On Diff #159701)

Maybe use std::string? Contexts can easily be cloned/migrated between threads, so it's very easy to end up with pointers to dead strings here.

clangd/Context.h

223 ↗

(On Diff #159701)

Maybe choose a scarier name and add a comment that we're not gonna use it in the long-term?
Maybe also add a comment that this is only used in our index implementation to workaround the limitations of the current design?

[ioeric]

In D50446#1192282, @ilya-biryukov wrote:

Short summary of the offline discussion: I suggest adding this parameter into the corresponding requests of the index (FuzzyFindRequest, FindDefinitionsRequest) instead of stashing it in the context. Context has all the same problems as the global variables, so we should try to avoid using it where possible.

Thanks for the summary! Just for the record, my concerns were:

1. The concept of active files are not really index-specific.
2. We need to do this for all 3 different requests (need "scary" comments 3 places?).
3. The context value seems to be trivial enough and hard to be misused.

On the other hand, where this key is stored might not be terribly important if we don't have too many usages and remove it when we can workaround the limitations that we're currently facing.

clangd/ClangdServer.cpp

162 ↗

(On Diff #159701)

I have two concerns about doing this:

1. Setting it in the TUScheduler seems to bury this deeper e.g. into scheduling logic. Limiting this in specific callbacks right before it's needed seems to make it less accessible.

2. There are operations like workspaceSymbols (although not handled in this patch) that do not go through TUScheduler.

clangd/Context.cpp

35 ↗

(On Diff #159701)

I intentionally used StringRef here to make it harder to use and hoped that users would be more careful when using this. For the current simple use cases, StringRef should be sufficient IMO.

35 ↗

(On Diff #159701)

We should probably look for some other place to put this, but I don't think Index.h is the right place. Although context might not be the best place to host the active file, the active file seems to be a well-defined concept in clangd in general and not index-specific.

[ilya-biryukov]

clangd/ClangdServer.cpp

162 ↗

(On Diff #159701)

1. Setting it in the TUScheduler seems to bury this deeper e.g. into scheduling logic. Limiting this in specific callbacks right before it's needed seems to make it less accessible.

If we want to make the data-flow explicit, let's add this to the index request parameters.
Otherwise, I suggest we do something consistent across all requests, rather than special-casing the index-calling ones. E.g. if someone adds an index call in onHover, they'll need to remember to also update the ClangdServer call to set this value and they really shouldn't care about this hack of ours. The less intrusive this thing is, the better.

2. There are operations like workspaceSymbols (although not handled in this patch) that do not go through TUScheduler.

And we don't set the file path for them anyway, because there's not active file in that case. How is that different in either of the approaches?

clangd/Context.cpp

35 ↗

(On Diff #159701)

Maybe move it ClangdServer.h or TUScheduler,h then? (depending on where we set it).

I still don't think Context.h is the right place for it, it should not contain any application-specific keys.

35 ↗

(On Diff #159701)

We're relying too much on the data flow of contexts, which is implicit and we completely don't control. Even if we don't access dead StringRefs now, it's very easy to accidentally introduce this behavior later.

We should use string here, there's just too high risk of hitting a bug otherwise. What are the disadvantages of using string here?

[ioeric]

• Addressed review comments.

[ioeric]

Ok, I am convinced :) Putting the context key into TUScheduler.cpp and exposing a static method to access it sound like a better idea afterall. Thanks for the suggestions!

[ilya-biryukov]

LGTM. Thanks!

clangd/TUScheduler.h

145

NIT: accidental whitespace change?

clangd/TUScheduler.h

private:
   /// This class stores per-file data in the Files map.
   struct FileData;
 
 public:
   /// Responsible for retaining and rebuilding idle ASTs. An implementation is
   /// an LRU cache.
   class ASTCache;
 
+  // The file being built/processed in the current thread. This is a hack in
+  // order to get the file name into the index implementations. Do not depend on
+  // this inside clangd.
+  // FIXME: remove this when there is proper index support via build system
+  // integration.
+  static llvm::Optional<llvm::StringRef> getFileBeingProcessedInContext();
+
 private:
   const bool StorePreamblesInMemory;
   const std::shared_ptr<PCHContainerOperations> PCHOps;
   const PreambleParsedCallback PreambleCallback;
   Semaphore Barrier;
   llvm::StringMap<std::unique_ptr<FileData>> Files;
   std::unique_ptr<ASTCache> IdleASTs;
   // None when running tasks synchronously and non-None when running tasks
   // asynchronously.
   llvm::Optional<AsyncTaskRunner> PreambleTasks;
   llvm::Optional<AsyncTaskRunner> WorkerThreads;
   std::chrono::steady_clock::duration UpdateDebounce;
 };
+
 } // namespace clangd
 } // namespace clang
 
 #endif

clangd/TUScheduler.cpp

 namespace clang {
 namespace clangd {
 using std::chrono::steady_clock;
 
 namespace {
 class ASTWorker;
 }
 
+static const clang::clangd::Key<std::string> kFileBeingProcessed;
+
+llvm::Optional<llvm::StringRef> TUScheduler::getFileBeingProcessedInContext() {
+  if (auto *File = Context::current().get(kFileBeingProcessed))
+    return StringRef(*File);
+  return llvm::None;
+}
+
 /// An LRU cache of idle ASTs.
 /// Because we want to limit the overall number of these we retain, the cache
 /// owns ASTs (and may evict them) while their workers are idle.
 /// Workers borrow ASTs when active, and return them when done.
 class TUScheduler::ASTCache {
 public:
   using Key = const ASTWorker *;
 
if (RunSync) {
     trace::Span Tracer(Name + ":" + llvm::sys::path::filename(FileName));
     Task();
     return;
   }
 
   {
     std::lock_guard<std::mutex> Lock(Mutex);
     assert(!Done && "running a task after stop()");
-    Requests.push_back({std::move(Task), Name, steady_clock::now(),
-                        Context::current().clone(), UpdateType});
+    Requests.push_back(
+        {std::move(Task), Name, steady_clock::now(),
+         Context::current().derive(kFileBeingProcessed, FileName), UpdateType});
   }
   RequestsCV.notify_all();
 }
 
 void ASTWorker::run() {
   while (true) {
     Request Req;
     {
auto Task = [Worker, this](std::string Name, std::string File,
     WithContext Guard(std::move(Ctx));
     trace::Span Tracer(Name);
     SPAN_ATTACH(Tracer, "file", File);
     std::shared_ptr<const PreambleData> Preamble =
         Worker->getPossiblyStalePreamble();
     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)));
+  PreambleTasks->runAsync(
+      "task:" + llvm::sys::path::filename(File),
+      Bind(Task, std::string(Name), std::string(File), It->second->Contents,
+           It->second->Command,
+           Context::current().derive(kFileBeingProcessed, File),
+           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(

unittests/clangd/TUSchedulerTests.cpp

for (int FileI = 0; FileI < FilesCount; ++FileI) {
         auto Contents = AllContents[(FileI + UpdateI) % AllContentsSize];
 
         auto File = Files[FileI];
         auto Inputs = getInputs(File, Contents.str());
 
         {
           WithContextValue WithNonce(NonceKey, ++Nonce);
           S.update(File, Inputs, WantDiagnostics::Auto,
-                   [Nonce, &Mut,
+                   [File, Nonce, &Mut,
                     &TotalUpdates](llvm::Optional<std::vector<Diag>> Diags) {
                      EXPECT_THAT(Context::current().get(NonceKey),
                                  Pointee(Nonce));
 
                      std::lock_guard<std::mutex> Lock(Mut);
                      ++TotalUpdates;
+                     EXPECT_EQ(File,
+                               *TUScheduler::getFileBeingProcessedInContext());
                    });
         }
 
         {
           WithContextValue WithNonce(NonceKey, ++Nonce);
           S.runWithAST("CheckAST", File,
-                       [Inputs, Nonce, &Mut,
+                       [File, Inputs, Nonce, &Mut,
                         &TotalASTReads](llvm::Expected<InputsAndAST> AST) {
                          EXPECT_THAT(Context::current().get(NonceKey),
                                      Pointee(Nonce));
 
                          ASSERT_TRUE((bool)AST);
                          EXPECT_EQ(AST->Inputs.FS, Inputs.FS);
                          EXPECT_EQ(AST->Inputs.Contents, Inputs.Contents);
 
                          std::lock_guard<std::mutex> Lock(Mut);
                          ++TotalASTReads;
+                         EXPECT_EQ(
+                             File,
+                             *TUScheduler::getFileBeingProcessedInContext());
                        });
         }
 
         {
           WithContextValue WithNonce(NonceKey, ++Nonce);
-          S.runWithPreamble("CheckPreamble", File,
-                            [Inputs, Nonce, &Mut, &TotalPreambleReads](
-                                llvm::Expected<InputsAndPreamble> Preamble) {
-                              EXPECT_THAT(Context::current().get(NonceKey),
-                                          Pointee(Nonce));
+          S.runWithPreamble(
+              "CheckPreamble", File,
+              [File, Inputs, Nonce, &Mut, &TotalPreambleReads](
+                EXPECT_THAT(Context::current().get(NonceKey), Pointee(Nonce));
 
-                              ASSERT_TRUE((bool)Preamble);
-                              EXPECT_EQ(Preamble->Contents, Inputs.Contents);
 
-                              std::lock_guard<std::mutex> Lock(Mut);
-                              ++TotalPreambleReads;
-                            });
+                EXPECT_EQ(File, *TUScheduler::getFileBeingProcessedInContext());
         }
       }
     }
   } // TUScheduler destructor waits for all operations to finish.
 
   std::lock_guard<std::mutex> Lock(Mut);
   EXPECT_EQ(TotalUpdates, FilesCount * UpdatesPerFile);
   EXPECT_EQ(TotalASTReads, FilesCount * UpdatesPerFile);