Repository navigation
Spec should discourage abuse of initializationOptions and didChangeConfiguration #567
Description
Activity
Interesting. I use
workspace/didChangeConfigurationas that's what was there but did not consider this case.At what point would you consider the use of this "abuse" though?
As an adopter of LS I don't develop, I consider any usage is abuse, as long as it requires to create a specific UI or workflow to interact with it in the client ;) But I hope this discussion can lead to a more flexible definition.
As I see it, a server initialized without
initializationOptionsshould just work in any editor. If the editor allows servers to be started by custom extensions with possible deeper integration (like VSCode or Atom for example), theninitializationOptionscan be used to enable specific features that can't be used in other editors.Laurent Tréguier (@LaurentTreguier): so your proposal is that the settings should be limited to only client-specific configuration? I think it'd be fair.
This is how I understood it. The spec describes it as
User provided initialization options, which can be misleading; it makes it sound like the user should change it themselves when it should be up to a specific client to do this.IMO the spec should encourage all server providers to spec the following to things:
- the initialization options it supports. I agree with Laurent Tréguier (@LaurentTreguier) that a server should work with an empty literal as well and simply assume a set of defaults.
- which
workspace/configurationrequest it sends.
Having a push model for configurations was a mistake and got basically replaced by the pull model where the server sends
workspace/configurationrequests.The client should still send
workspace/didChangeConfigurationnotification so that the server can clear caches if it caches configurations. But it should not send any values since values can differ based on the scope used in theworkspace/configurationrequest.- - the initialization options it supports. I agree with @LaurentTreguier <https://github.com/LaurentTreguier> that a server should work with an empty literal as well and simply assume a set of defaults. Just to give examples and food for thought, VSCode CSS language serverrequires explicit enablement for sass, scss and so on; and VSCode JSon language server doesn't pre-load a typical list of JSon schema. Both assume client discover this settings by reverse engineering VSCode and repeat the same settings. The question is what drove the developers of those LS to rely on those options instead of making them default? It'd be interesting to get their POV on this question.Having a push model for configurations was a mistake and got basically replaced by the pull model where the server sends workspace/configuration requests.Still, the expected type is `any[]` which means that it's some LS specific settings that require specific integration. I don't think the flow of the operation was the issue here (while it's still good to know it was improved), it's really than any `any` leads to the unspecified world and specific effort of integration between client and LS, which is the opposite of LSP goal. I believe instead of specifying some operations with `any`, it's better to leave these as extensions. For clients, it's a similar effort to support one or the other, and it's not reusable between LS, so the protocol should remain strictly made of specified, portable, reusable operations, and whenever there is `any`, consider deprecating the operation basically because it's not specified enough to be useful by most tools.
I disagree here. The reason is that the fact that
initializationOptionsis in the spec say that this property should be used and not any other random property.I still think that it is a fair thing to require a server to depend on some
initializationOptions. But I do fully agree that these need to be speced by the server (and not be reverse engineered from code) and that server should work with a resonable default set if they are not provided. Same is true for settings.I do fully agree that the spec need to spec this assumptions.
Martin Aeschlimann (@aeschli) any comments on why the CSS language server can't work with a reasonable default set ?
One pattern I saw emerge is that many language servers interpret
initializationOptionsas containing user configuration, i.e. the same that is sent inworkspace/didChangeConfigurationor in response toworkspace/configuration. But LSP doesn't actually really say this, it is very vague on whatinitializationOptionsactually means:User provided initialization options.
It doesn't use the term "configuration", but it does say "user provided" (not client provided).
The reason why language servers use it for configuration is because there is otherwise no way to read configuration in
initialize.workspace/didChangeConfigurationis only sent afterinitializereturned (if at all), andworkspace/configurationis not among the whitelisted requests allowed duringinitialize.
The problem is that sinceinitializationOptionsis not clearly defined, most clients do not send configuration in it.
Could LSP just be clearer about whatinitializationOptionsis intended to be used for (maybe with an example in the spec)? And couldworkspace/configurationbe whitelisted to be used duringinitialize?I will clarify the spec in a way that the
initializationOptionsis typically something that could be passed on the command line when starting the server. It shouldn't be user configurations.I am actually against whitelisting
workspace/configuration. If a server needs the configuration to register providers it should use dynamic registration instead of static registration which allows to mixworkspace/configurationwith registration calls.Please ping if you think dynamic registration is not the right path to go.
- I will clarify the spec in a way that the initializationOptions is typically something that could be passed on the command line when starting the server. It shouldn't be user configurations.I disagree with that. The initializationOptions can be a good way to guarantee that user settings are passed immediately to the LS before to starts up. I think that basically, the initializationOptions have to be a super-set of the didChangeConfiguration as there are case where we want the configuration immediately. From a client perspective, the didChangeConfiguration is over-used, hard to maintain and is semantically often used with wrong semantic since several LS use it even to retrieve a default configuration. Eclipse Corrosion had an important discussion with RLS on that matter, and the resolution that initializationOptions can contain a mirror of didChangeConfiguration improved things a lot: rust-lang/rls#1026
I am actually against whitelisting workspace/configuration. If a server needs the configuration to register providers it should use dynamic registration instead of static registration which allows to mix workspace/configuration with registration calls.
Dirk Bäumer (@dbaeumer) I agree for determining whether a provider should be registered or not, but a server might need configuration during initialisation for a lot of reasons. For example, a loglevel or logfile, whether file watchers should be set up with polling or OS events, the HTTP endpoint of a service that needs to be contacted, whether dependencies should be installed, if yes an access token for that, the path of an external tool that needs to be shelled into, something like JAVA_HOME or GOPATH, ...
Requiring to delay all of these afterinitializecomplicates a lot of things for no apparent reason. Also not every client supports dynamic registration, and these clients should gracefully degrade in functionality, i.e. they should work with a static set of capabilities from server initialize and work with restarting the server instead of not working at all.Reacted by XeroOl35 remaining items
Alex Kladov (@matklad) Why would that be any different than
initializationOptionsOr do you simply suggest a different name. Typing wise it would still be any.For two reasons:
- it will guarantee to submit the same values as the
workspace/configuration - clients would be able to automatically populate it from settings, instead of needing custom per-language code to constuct
initializationOptions
I guess we can just reuse existing
initializationOptionskey, and just make sure that it automatically gets populated from the settings.Reacted by Michael Peyton Jones and Charles- it will guarantee to submit the same values as the
It is possible to send
getConfigurationrequests in theinitializednotification.I will close the issue since I am really not a fan of having another property during initialization. Please ping if you think otherwise.
It is possible to send getConfiguration requests in the initialized notification.
The current spec says
In addition the server is not allowed to send any requests or notifications to the client until it has responded with an InitializeResult, with the exception that during the initialize request the server is allowed to send the notifications window/showMessage, window/logMessage and telemetry/event as well as the window/showMessageRequest request to the client.
Which seems to contradict what you said, Dirk Bäumer (@dbaeumer) ?
No this contradicts nothing.
initializeis a request sent from client to server.initializedis a notification from the client. Clients are expected to send this notification after receiving a response to theinitializerequest.
A server can therefore do a
workspace/configurationrequest in its handler forinitializednotifications.Gotcha, thanks!
I will look into white listing
workspace/configuration.It is possible to send
getConfigurationrequests in theinitializednotification.Dirk Bäumer (@dbaeumer) The latest solution doesn't seem sufficient and I think
workspace/configurationshould be allow-listed duringinitialize.Good servers should pull relevant workspace settings by namespace, for each workspace folder if interested, and incorporate that before they start processing normal document notifications/requests. Currently you can only do that in the
initializedhandler. This means every server must have a boilerplate initializer lock which blocks out other message handling until that part of theinitializedhandler is completed.Is that the basic recommended approach? Or are servers supposed to initialize once with default settings, initialize again with pulled config slightly later while possibly receiving doc info in between? I don't see any other interpretation.
Well, extension authors have avoided this and are still heavily relying on
initializationOptionsfor startup workspace settings despite using pull-based config when it comes to receivingworkspace/didChangeConfiguration. IMO that should only be a fallback for clients that don't support pull-based config, if they're worth the effort. My problem is thatinitializationOptionshas no convention for per-workspace-folder settings.Is there a good reason to not allow-list
workspace/configuration?Is that the basic recommended approach? Or are servers supposed to initialize once with default settings, initialize again with pulled config slightly later while possibly receiving doc info in between?
This is what we're doing in HLS. In fact we do all of what you said:
- Start with default config
- Take the config from
intializationOptionsif it's there - Fire off
workspace/configurationin theinitializedhandler
I agree that this is not very good, and it certainly seems that if you do the recommended thing you can start getting sent requests before you have your config set up.
good
Is there a good reason to not allow-list workspace/configuration?
In general I try to keep the message that can be sent to the client in initialized as small as possible to make the initialization phase on the client simple.
Currently you can only do that in the initialized handler
I usually do that when actually processing a request and then cache the result of the
workspace/configuration. When I receive aworkspace/didChangeConfigurationI simply clear the cache.I usually do that when actually processing a request and then cache the result of the
workspace/configuration.This is not a good candidate for lazy init because:
workspace/configurationgenerally is behind an async binding, and accessing config shouldn't need to be.- It spreads to other structures that depend on config. Now everything must be computed lazily and asynchronously.
- Lazy computation is used to make things more responsive. In this case it's less responsive compared to acting on
workspace/didChangeConfiguration.
In general I try to keep the message that can be sent to the client in initialized as small as possible to make the initialization phase on the client simple.
I think that would mean just storing the initialize params and returning server info and capabilities, since technically everything else can be done lazily. But server authors won't use it that way.
A more meaningful differentiation is that
initializeis the request that all other requests are blocked on, hinting that it should get up-to-date everything required for handlingtextDocument/*notifications. That makes more sense so server authors are using it that way (but relying oninitializationOptions).Please reconsider allow-listing
workspace/configurationduringinitialize.
I'm working on having Eclipse IDE adopting some language servers.
I see a possible bad trend for Language Servers to heavily rely on initializationOptions and didChangeConfigurations to enable/disable features. The issue with those is that they are unspecified placeholders and that whatever usage is made of it requires all clients to write code specific to this language server to support those options.
The main example I have in mind right now is RLS that, by abusing those settings is progressively, and most likely without really willing it, breaking rich compatibility with other IDEs: rust-lang/rls#1047
Abuse of those properties should be deprecated in the spec, with some disclaimer explaining how relying on those make the LS integration less likely to be trivially portable from an editor to another.