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

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In D59811#1445701 <https://reviews.llvm.org/D59811#1445701>, @ilya-biryukov wrote:

> 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)


All very good points. Thanks! I've added an option.



================
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.",
----------------
ilya-biryukov wrote:
> 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);
> ```
> void runWithPreamble(… Callback<InputsAndPreamble> Action, Callback<Inputs> FallbackAction);
This is what I started with in the first iteration. The main problem is that we would need to bind `CB` to both actions; however, `CB` is not copyable/shareable by design, I think. This seems to leave us with the only option to handle everything in the same action. I thought this was fine because an parameter `AllowFallback` seems to be clear as well. I'm open to suggestions ;)

> This way we don't distinguish between the failure to build a preamble and the fallback mode.
I am assuming that a fallback for `runWithPreamble` is either compile command is missing/broken or preamble fails to be built. And in both cases, fallback mode could be useful. Do you think we should have more distinctions made here?


================
Comment at: clangd/TUScheduler.cpp:186
 
   std::shared_ptr<const PreambleData> getPossiblyStalePreamble() const;
+
----------------
ilya-biryukov wrote:
> 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.
Could you elaborate the advantages of keeping them consistent?

They seem to be updated and used relatively independently. Grouping them into one function seems to add more work to the call sites.  And it seems inevitable that we would some inconsistency between the two.


================
Comment at: clangd/TUScheduler.cpp:371
+      std::lock_guard<std::mutex> Lock(Mutex);
+      this->CompileCommand = Inputs.CompileCommand;
+    }
----------------
ilya-biryukov wrote:
> 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.
> After this point the clients might start getting a new compile command and an old preamble.
Did we have guarantee about this before? It seems that we could also have the similar situation.

> 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.
I added documentation for `getCurrentPreamble`; would it be the right place? `getPossiblyStalePreamble` seems self-explaining enough. And since compile command is always fresher than preamble, I think it would be fine without commenting?


================
Comment at: clangd/TUScheduler.cpp:918
+  // asynchronous mode, as TU update should finish before this is run.
+  if (!It->second->Worker->isFirstPreambleBuilt() && AllowFallback &&
+      PreambleTasks) {
----------------
ilya-biryukov wrote:
> 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.
It sounds like a very small window (~ms?) that we are trying to optimize for.  Given that users type relatively slow and the first preamble usually only becomes ready for each file once, it seems unlikely to get value out of this often. And once the preamble is built, the decision is likely to be always no fallback, so it would be a bit wasteful to run a separate task for this.


================
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.
----------------
ilya-biryukov wrote:
> 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?
We could potentially use compile command to shorten includes for headers from global index.

The intention here was to share the same action and thus the same input structure for both modes. 


================
Comment at: unittests/clangd/ClangdTests.cpp:1099
+                                       clangd::CodeCompleteOptions()))
+                  .Completions,
+              IsEmpty());
----------------
ilya-biryukov wrote:
> 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.
Good point. I just realized we could use the `Context` that already exists in the result. We will set it to `Recovery` in fallback mode. Although sema can also enter recovery mode, they don't seem to conflict much? WDYT?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59811