Page MenuHomePhabricator

Remove possibility to change compile database path at runtime
ClosedPublic

Authored by simark on Oct 12 2018, 2:39 PM.

Details

Summary

This patch removes the possibility to change the compilation database
path at runtime using the didChangeConfiguration request. Instead, it
is suggested to use the setting on the initialize request, and clangd
whenever the user wants to use a different build configuration.

Diff Detail

Event Timeline

simark created this revision.Oct 12 2018, 2:39 PM

Thanks for cleaning this up!

clangd/ClangdLSPServer.cpp
435–436

This isn't needed, the compilation database can only be set during initialization.

clangd/ClangdLSPServer.h
90 ↗(On Diff #169493)

Prefer a different name for this function - an overload set should have similar semantics and these are quite different (pseudo-constructor vs mutation allowed at any time).

clangd/Protocol.h
422

Can we just move this to InitializeParams as a clangd extension?
Doing tricks with inheritance here makes the protocol harder to understand.

simark marked an inline comment as done.Oct 15 2018, 8:54 AM
simark added inline comments.
clangd/ClangdLSPServer.h
90 ↗(On Diff #169493)

Ok, I'll think about a better name.

clangd/Protocol.h
422

Can we just move this to InitializeParams as a clangd extension?

Do you mean move it in the JSON, so it looks like this on the wire?

{
  "method": "initialize",
  "params": {
    "compilationDatabasePath": "<path>",
    ...
  }
}

instead of

{
  "method": "initialize",
  "params": {
    "initializationOptions": {
          "compilationDatabasePath": "<path>"
    },
    ...
  }
}

?

I think initializationOptions is a good place for this to be, I wouldn't change that.. If you don't like the inheritance, we can just get rid of it in our code and have two separate versions of the deserializing code. We designed it so didChangeConfiguration and the initialization options would share the same structure, but it doesn't have to stay that way.

sammccall accepted this revision.Oct 15 2018, 9:44 AM

I think it'd be a good idea to separate out the on-initialization vs dynamically-changing parameters more - I think they should probably be disjoint in fact.

But we can discuss/implement that separately from this patch - the change itself is really nice.

clangd/ClangdLSPServer.cpp
435–436

It's still here... maybe forgot to upload a new diff?
(Just to be clear: I meant reparseOpenFiles doesn't need to be called, as there are none.)

clangd/ClangdLSPServer.h
90 ↗(On Diff #169493)

(In fact, this could also live directly in onInitialize, I think?)

clangd/Protocol.h
422

Do you mean move it in the JSON, so it looks like this on the wire?

Well, since you asked... :-) I'm not going to push hard for it (this patch is certainly a win already), but I do think that would be much clearer.

The current protocol has InitializeParams and ClangdInitializationOptions, and it's not clear semantically what the distinction between them is.

With hindsight, I think something like this would be easier to follow:

// Clangd options that may be set at startup.
struct InitializeParams {
  // Clangd extension: override the path used to load the CDB.
  Optional<string> compilationDatabasePath;
  // Provides initial configuration as if by workspace/updateConfiguration.
  Optional<ClangdConfigurationParamsChange> initialConfiguration;
}
// Clangd options that may be set dynamically at runtime.
struct ClangdConfigurationParamsChange { ... }

though even here, the benefit from being able to inline the initial configuration into the initalize message is unclear to me. The implementation has to support dynamic updates in any case, so why not make use of that?

We designed it so didChangeConfiguration and the initialization options would share the same structure

This makes sense, but if they're diverging, I'm not sure that keeping them *mostly* the same brings more benefits than confusion.


That said, if you prefer to keep the JSON as it is, that's fine. (If we grow more extensions, we may want to reorganize in future though?)
My main concern is the use of inheritance here, and how it provides a default (configuration-change options can be provided at startup) that doesn't seem obviously correct and is hard to opt out of.

This revision is now accepted and ready to land.Oct 15 2018, 9:44 AM
simark marked 7 inline comments as done.Oct 16 2018, 8:27 AM
simark added inline comments.
clangd/ClangdLSPServer.cpp
435–436

Oh I have only done the change locally, I have not uploaded a new diff yet.

clangd/ClangdLSPServer.h
90 ↗(On Diff #169493)

Indeed, since it's things that can only be set during initialization.

clangd/Protocol.h
422

The current protocol has InitializeParams and ClangdInitializationOptions, and it's not clear semantically what the distinction between them is.

InitializeParams is the type for the parameters of the initialize request. In it, there is this field:

	/**
	 * User provided initialization options.
	 */
	initializationOptions?: any;

which is made to pass such language-server-specific options. Since it's of the any type, we gave it a name, ClangdInitializationOptions.

With hindsight, I think something like this would be easier to follow:

[snippet]

I don't really understand why putting fields as extra fields in InitializeParams would be any easier than putting them in the object that exists specifically for that purpose.

though even here, the benefit from being able to inline the initial configuration into the initalize message is unclear to me. The implementation has to support dynamic updates in any case, so why not make use of that?

I'd say, to avoid having the server start some work (like indexing some files using a certain set of flags) that will be invalidated when it receives some config changes moments later.

This makes sense, but if they're diverging, I'm not sure that keeping them *mostly* the same brings more benefits than confusion.

I think you are exaggerating the confusion. It is pretty straightforward: everything you can change during execution, you can also specify at initialize time.

simark updated this revision to Diff 169828.Oct 16 2018, 8:27 AM
simark marked 3 inline comments as done.

Address comments

sammccall accepted this revision.Oct 16 2018, 8:50 AM

Still LG!

clangd/Protocol.h
422

which is made to pass such language-server-specific options. Since it's of the any type, we gave it a name, ClangdInitializationOptions.

Fair enough. I personally think initializationOptions in LSP is not useful as specified for various reasons, and would rather avoid it (it's optional), but following the spirit of the spec makes sense too.

I'd say, to avoid having the server start some work (like indexing some files using a certain set of flags) that will be invalidated when it receives some config changes moments later.

Doing this to enable optimizations definitely makes sense.

It does seem premature when we don't have any such optimization: it puts more burden on clients by giving them multiple equivalent ways to do things, and hinting that they're not quite equivalent.

This revision was automatically updated to reflect the committed changes.