Page MenuHomePhabricator

[clangd] Receive compilationDatabasePath in 'initialize' request
ClosedPublic

Authored by simark on Jul 25 2018, 8:47 PM.

Details

Summary

That way, as soon as the "initialize" is received by the server, it can start
parsing/indexing with a valid compilation database and not have to wait for a
an initial 'didChangeConfiguration' that might or might not happen.
Then, when the user changes configuration, a didChangeConfiguration can be sent.

Signed-off-by: Marc-Andre Laperle <marc-andre.laperle@ericsson.com>

Diff Detail

Repository
rL LLVM

Event Timeline

malaperle created this revision.Jul 25 2018, 8:47 PM
ilya-biryukov added a comment.EditedJul 26 2018, 2:05 AM

Not strictly opposed to this change, but is there any reason why the clients can't guarantee they'll send didChangeConfiguration right after clangd is initialized?

Not strictly opposed to this change, but is there any reason why the clients can't guarantee they'll send didChangeConfiguration right after clangd is initialized?

LSP:

Until the server has responded to the initialize request with an InitializeResult, the client must not send any additional requests or notifications to the server.

So the client is free to send "didOpen" or any request after the "initialize", which means we could end up parsing files with wrong commands before we get the first "didChangeConfiguration". In fact, in our indexing prototype we start indexing as soon as the "initialize" is processed and we do not know whether or not there will be a didChangeConfiguration soon after.
Setting the CDB path as part of the initialize seems more natural and less error-prone.

Was there any objection to this patch? It would have been nice to have this before the branching.

Was there any objection to this patch? It would have been nice to have this before the branching.

Sorry for the delay, somehow missed this update in my inbox.

LSP:

Until the server has responded to the initialize request with an InitializeResult, the client must not send any additional requests or notifications to the server.

So the client is free to send "didOpen" or any request after the "initialize", which means we could end up parsing files with wrong commands before we get the first "didChangeConfiguration". In fact, in our indexing prototype we start indexing as soon as the "initialize" is processed and we do not know whether or not there will be a didChangeConfiguration soon after.
Setting the CDB path as part of the initialize seems more natural and less error-prone.

Thanks for clarifying the use-case, that does seem like the best option if you want to start indexing as soon as you initialize.

Overall LG, just a small suggestion on how to rearrange the options.

clangd/Protocol.h
362 ↗(On Diff #157420)

Maybe add another level of indirection from the start to make sure we'll be able to add more init options later without breaking the existing clients?

/// Clangd-specific initialization options that can be passed on the initial initialization request.
struct ClangdInitOptions {
  llvm::Optional<ClangdConfigurationParamsChange> initialConfiguration; 
  // can add more options here in a backwards-compatible manner
};

struct InitializeParams {
  /// ....
  llvm::Optional<ClangdConfigurationParamsChange> initializationOptions;
};

@simark would you mind finishing this patch and committing it? I won't be able to finish it given that I started it at a different company, etc.

@simark would you mind finishing this patch and committing it? I won't be able to finish it given that I started it at a different company, etc.

Yes, I'll take it over, thanks for getting it started.

simark commandeered this revision.Jul 31 2018, 9:45 AM
simark edited reviewers, added: malaperle; removed: simark.
simark updated this revision to Diff 158366.Jul 31 2018, 1:07 PM

Address Ilya's comment (add an indirection for initializationOptions).

ilya-biryukov accepted this revision.Aug 1 2018, 2:23 AM

LGTM with a small nit. Thanks for the change!

clangd/ClangdLSPServer.cpp
419 ↗(On Diff #158366)

NIT: replace with applyConfiguration(Params.settings) to get rid of the extra local var

This revision is now accepted and ready to land.Aug 1 2018, 2:23 AM
This revision was automatically updated to reflect the committed changes.
simark marked 2 inline comments as done.Aug 1 2018, 4:30 AM

Thanks, done.