[PATCH] D59811: [clangd] Add fallback mode for code completion when compile command or preamble is not ready.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

I think this option should be configurable (and off by default) for the transition period. A few reasons to do so:

- Before we have an actual implementation of fallback completions, the current behavior (waiting for the first preamble) actually seems like a better experience than returning empty results.
- Some clients do this kind of fallback on their own (e.g. VSCode), so until we can provide actually semantically interesting results (anything more than identifier-based really, e.g. keyword completions) we would be better off keeping it off.
- We can still test it if it's off by flipping the corresponding flag in the test code.
- Would make rollout easier (clients can flip the flag back and forth and make sure it does not break stuff for them)



================
Comment at: clangd/ClangdServer.cpp:197
       return CB(llvm::make_error<CancelledError>());
+    if (!IP->Preamble) {
+      vlog("File {0} is not ready for code completion. Enter fallback mode.",
----------------
This way we don't distinguish between the failure to build a preamble and the fallback mode.
Have you considered introducing a different callback instead to clearly separate two cases for the clients?
The code handling those would hardly have any similarities anyway, given that the nature of those two cases is so different.

Would look something like this:
```
/// When \p FallbackAction is not null, it would be called instead of \p Action in cases when preamble is 
/// not yet ready.
void runWithPreamble(… Callback<InputsAndPreamble> Action, Callback<Inputs> FallbackAction);
```


================
Comment at: clangd/TUScheduler.cpp:186
 
   std::shared_ptr<const PreambleData> getPossiblyStalePreamble() const;
+
----------------
Could we put the preamble and compile command into a single function?
Getting them in the lock-step would mean they're always consistent, which is a good thing.


================
Comment at: clangd/TUScheduler.cpp:264
+  llvm::Optional<tooling::CompileCommand>
+      CompileCommand; /* GUARDED_BY(Mutex) */
+  /// Becomes ready when the first compile command is set.
----------------
NIT: name it `LastCompileCommand` for consistency with the name of `LastBuiltPreamble`.


================
Comment at: clangd/TUScheduler.cpp:371
+      std::lock_guard<std::mutex> Lock(Mutex);
+      this->CompileCommand = Inputs.CompileCommand;
+    }
----------------
After this point the clients might start getting a new compile command and an old preamble.
This seems fine (the clients should be ready for this), but let's document it in the methods that expose a compile command and a preamble.


================
Comment at: clangd/TUScheduler.cpp:918
+  // asynchronous mode, as TU update should finish before this is run.
+  if (!It->second->Worker->isFirstPreambleBuilt() && AllowFallback &&
+      PreambleTasks) {
----------------
It would be better to make a decision on whether to use a fallback mode in the actual function scheduled on a different thread (i.e. inside the body of `Task`).
Imagine the preamble is being built right now and would finish before scheduling this task. In that case we would have a chance to hit the "preamble" if we make a decision in the body of the handler, but won't have that chance in the current implementation.


================
Comment at: clangd/TUScheduler.cpp:977
+    if (!Worker->isFirstCompileCommandSet()) {
+      Worker->waitForFirstCompileCommand();
+    }
----------------
NIT: remove braces


================
Comment at: clangd/TUScheduler.h:44
+  // Can be None in fallback mode when neither Command nor Preamble is ready.
+  llvm::Optional<tooling::CompileCommand> Command;
+  // In fallback mode, this can be nullptr when preamble is not ready.
----------------
How would we expect to use the compile command when preamble is not ready?
Maybe go with passing only contents to the fallback action until we actually have a use for compile command?


================
Comment at: clangd/TUScheduler.h:156
   /// (i.e. WantDiagnostics is downgraded to Auto).
-  void update(PathRef File, ParseInputs Inputs, WantDiagnostics WD);
+  void update(PathRef File, FileUpdateInputs UpdateInputs, WantDiagnostics WD);
 
----------------
NIT: keep the parameter named `Inputs`


================
Comment at: unittests/clangd/ClangdTests.cpp:1099
+                                       clangd::CodeCompleteOptions()))
+                  .Completions,
+              IsEmpty());
----------------
Could we provide a flag in the results indicating this was a fallback-mode completion?
This could be used here in tests and we could later use it in the clients to show the corresponding UI indicators for users.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59811/new/

https://reviews.llvm.org/D59811