Page MenuHomePhabricator

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

Authored by ioeric on Mar 26 2019, 2:37 AM.

Details

Summary

When calling TUScehduler::runWithPreamble (e.g. in code compleiton), allow
entering a fallback mode when compile command or preamble is not ready, instead of
waiting. This allows clangd to perform naive code completion e.g. using identifiers
in the current file or symbols in the index.

This patch simply returns empty result for code completion in fallback mode. Identifier-based
plus more advanced index-based completion will be added in followup patches.

Diff Detail

Repository
rL LLVM

Event Timeline

ioeric created this revision.Mar 26 2019, 2:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2019, 2:37 AM

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)
clangd/ClangdServer.cpp
197 ↗(On Diff #192251)

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);
clangd/TUScheduler.cpp
186 ↗(On Diff #192251)

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.

264 ↗(On Diff #192251)

NIT: name it LastCompileCommand for consistency with the name of LastBuiltPreamble.

371 ↗(On Diff #192251)

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.

918 ↗(On Diff #192251)

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.

977 ↗(On Diff #192251)

NIT: remove braces

clangd/TUScheduler.h
44 ↗(On Diff #192251)

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?

156 ↗(On Diff #192251)

NIT: keep the parameter named Inputs

unittests/clangd/ClangdTests.cpp
1099 ↗(On Diff #192251)

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.

ioeric updated this revision to Diff 192683.Mar 28 2019, 11:00 AM
ioeric marked 9 inline comments as done.
  • address review comments.

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.

clangd/ClangdServer.cpp
197 ↗(On Diff #192251)

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?

clangd/TUScheduler.cpp
186 ↗(On Diff #192251)

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.

371 ↗(On Diff #192251)

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?

918 ↗(On Diff #192251)

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.

clangd/TUScheduler.h
44 ↗(On Diff #192251)

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.

unittests/clangd/ClangdTests.cpp
1099 ↗(On Diff #192251)

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?

ioeric updated this revision to Diff 193268.Apr 2 2019, 6:09 AM
  • Rebase

Leaving a few first comments, have to run.
Will take another look later, sorry about the delay today (also feel free to add more reviewers in case this is urgent).

clangd/ClangdServer.cpp
197 ↗(On Diff #192251)

Ah, right, I did not realize that design of Callback makes it hard for two callbacks to own the same data.
And that's what we technically need for this. I'll have to think about this two, maybe there's a better way to express this in the code (AllowFallback looks like the option with least amount of friction so far).

clangd/TUScheduler.cpp
186 ↗(On Diff #192251)

I was originally thinking about consistency between the provided compile command and the one used to build the preamble.
Now I realize that they are set at different times, so we can't really make them consistent (unless we decide to delay the update of the compile command, but I don't see why it's useful).

You approach looks legit, but could you please document in these getters (maybe more importantly in the public callback interfaces?) that preamble and compile commands might be out of sync (command can be newer) and clients should handle that gracefully.

Got here trying to understand how D60126 works.

It seems there's two fairly independent changes here:

  • the one described, allow actions to run without a preamble
  • defer the blocking getCompileCommand() call until we're building the preamble

Certainly they interact to make zero-latency code completion work, but is it possible to pull the latter out of this patch?
There are other approaches to the second part (e.g. giving TUScheduler a reference to the global CDB) that might be cleaner. I don't think it needs to block D60126.

clangd/TUScheduler.h
204 ↗(On Diff #193268)

I think this isn't orthogonal to PreambleConsistency.
When would we use AllowFallback = true but PreambleConsistency = Consistent?

Two possible options:

  • adding a new StaleOrAbsent option to PreambleConsistency
  • changing Stale to these new semantics, as codeComplete is the only caller

The problem with the latter is we can't put it behind a flag.

Sam, thanks for taking a look and the useful comments!

@ioeric, I second Sam's suggestion to split the compile command and the fallback action into two changes. This would make it easier to review those in isolation. Could you do this please?

clangd/TUScheduler.h
204 ↗(On Diff #193268)

Ah, I was totally looking past the PreambleConsistency flag.
Thanks for spotting this. Indeed, modeling the fallback in the PreambleConsistency would make sense.

ioeric updated this revision to Diff 194129.Apr 8 2019, 6:14 AM
ioeric marked 3 inline comments as done.
  • split out the compile command change.
ioeric added inline comments.Apr 8 2019, 6:16 AM
clangd/TUScheduler.h
204 ↗(On Diff #193268)

Sounds good. Thanks for the suggestion!

ioeric planned changes to this revision.Apr 8 2019, 6:16 AM

Oops, forgot to update tests...

ioeric updated this revision to Diff 194132.Apr 8 2019, 6:41 AM
  • Fix unit test

API looks good. Implementation looks like it can be simplified a bit, unless I'm missing something.

clangd/CodeComplete.h
117 ↗(On Diff #194129)

nit: "mode" doesn't add anything here

clangd/TUScheduler.cpp
50 ↗(On Diff #194129)

not needed or bad patch base?

51 ↗(On Diff #194129)

not needed?

881 ↗(On Diff #194129)

Does this need to be a special case at the top of TUScheduler::runWithPreamble, rather than unified with the rest of the body?

e.g. the if (!PreambleTasks) appears to block appears to handle that case correctly already, and the Consistency == Consistent block won't trigger.
I think all you need to do is make the Worker->waitForFirstPreamble() line conditional?

(This means no changes to ASTWorker, Notification etc)

clangd/TUScheduler.h
190 ↗(On Diff #194129)

This sentence has grown unwieldy:can we just say "If there's no up-to-date preamble, we follow the PreambleConsistency policy"?
(I personally find the \p s hurt readability, but up to you)

193 ↗(On Diff #194129)

bad reflow, break before "if an error occurs" (or just drop the sentence)

194 ↗(On Diff #194129)

I think this is a bad reflow, need a line break before "Context cancellation is ignored..."

unittests/clangd/ClangdTests.cpp
1068 ↗(On Diff #194129)

does this comment need an update?

1082 ↗(On Diff #194129)

do I understand right that this test is currently racy?

sammccall added inline comments.Apr 8 2019, 6:44 AM
unittests/clangd/ClangdTests.cpp
1082 ↗(On Diff #194129)

Sorry, missed your comment. Test LG

1080 ↗(On Diff #194132)

(-this-flag-does-not-exist might be clearer)

ioeric updated this revision to Diff 194141.Apr 8 2019, 7:05 AM
ioeric marked 11 inline comments as done.
  • address review comments
clangd/TUScheduler.cpp
881 ↗(On Diff #194129)

whoops! absolutely right!

unittests/clangd/ClangdTests.cpp
1080 ↗(On Diff #194132)

This would still cause preamble to be built:

Updating file /clangd-test/foo.cpp with command [/clangd-test] clang -this-flag-does-not-exist /clangd-test/foo.cpp
Ignored diagnostic. unknown argument: '-this-flag-does-not-exist'
sammccall accepted this revision.Apr 8 2019, 7:30 AM

Thanks!

This revision is now accepted and ready to land.Apr 8 2019, 7:30 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2019, 7:51 AM