Add support for workspace/didChangeConfiguration

[jwortmann]

This PR implements the second point in #2800

• Removed all remaining "default_clients".
• Introduces a new settings file LanguageServers.sublime-settings for the manual configurations. This allows for a cleaner separation of LSP settings and server configs, so we can attach different change listeners to these files.
• The "clients" in LSP.sublime-settings still work, but I've marked it as deprecated in the JSON schema. If there is a configuration with the same name also in LanguageServers.sublime-settings, then that will take precedence.
• Adds support for workspace/didChangeConfiguration to notify the server when settings were changed. This is only implemented for configs from the new LanguageServers.sublime-settings file and for external configs (from helper packages), but not for the "clients" in the LSP settings. Servers can opt-in to change notifications via dynamic registration (see https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_configuration). This allows to dynamically change settings while the server is running, without forcing a restart of the server (restart can take a long time for example if the server is implemented in a JIT-compiled language, or if project indexing is slow).
• workspace/didChangeConfiguration can be tested for example with JETLS. (That server shows the updated settings after a new configuration request in a window/showMessage notification, which we log to the console.)
• If a server doesn't support (or opts into) workspace/didChangeConfiguration, nothing happens until the server initiates a new workspace/configuration request by itself (which might never happen), or until the server is manually restarted via LSP: Restart Server. This could be unexpected to users, so I wonder if we should handle that case also in LSP directly. This happens for example with Pyright.

Is the change with the new config file too intrusive? IMO it's nice to have a clean separation between LSP settings and server configs, and I always disliked that server configurations were named "clients" in the settings file.

[netlify]

✅ Deploy Preview for sublime-lsp ready!

Name	Link
🔨 Latest commit	d3bea01
🔍 Latest deploy log	https://app.netlify.com/projects/sublime-lsp/deploys/69de3b48b0bddc00083ab25e
😎 Deploy Preview	https://deploy-preview-2862--sublime-lsp.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[rchl]

I did not look at the changes yet but I have a question.

Some packages like for example LSP-pyright implement a custom configuration setting like pyright.dev_environment that is not really a server configuration but rather a package configuration. Server doesn't know anything about it and we should actually restart server when it changes.

Does this change affect that? Will we need to update those packages and move the setting to the root of the settings, for example?

[jwortmann]

Some packages like for example LSP-pyright implement a custom configuration setting like pyright.dev_environment that is not really a server configuration but rather a package configuration.

I know, and I believe I have written somewhere before that it is a bad idea and helper packages shouldn't mix custom package settings with server settings. So I would consider that as a bug in LSP-pyright. In LSP-julia I have ensured that custom package settings don't mix with server settings: https://github.com/sublimelsp/LSP-julia/blob/4751ae10ab015451292c11a8b1e9cc65594c8fac/LSP-julia.sublime-settings#L2-L9

Server doesn't know anything about it and we should actually restart server when it changes.

With this PR, servers are restarted if the ClientConfig does not equal (==) the old config anymore. But the _all_settings are currently excluded from that due to starting with an underscore (see __eq__ implementation). I don't know if there was a reason for that, but we could consider to change that property name if servers should be restarted on custom package settings change. So this should be a trivial change. But we might want to discuss if that should actually happen. These custom settings might be unrelated to the running server, so it might be desired to keep the server running. If the server should be restarted on a change, I guess the helper package could also do it manually via lsp_restart_server command. Although that would require another change listener to be implemented also in the helper package, so I'm not saying that I'm particular against restarting servers on package setting change.

Does this change affect that? Will we need to update those packages and move the setting to the root of the settings, for example?

Servers that opt-in to didChangeConfiguration notifications would get a notification and then should pull the configuration. If there is no change to the settings that are known by the server, in theory nothing should happen.

[rchl]

Some packages like for example LSP-pyright implement a custom configuration setting like pyright.dev_environment that is not really a server configuration but rather a package configuration.

I know, and I believe I have written somewhere before that it is a bad idea and helper packages shouldn't mix custom package settings with server settings. So I would consider that as a bug in LSP-pyright. In LSP-julia I have ensured that custom package settings don't mix with server settings: https://github.com/sublimelsp/LSP-julia/blob/4751ae10ab015451292c11a8b1e9cc65594c8fac/LSP-julia.sublime-settings#L2-L9

It was required to do that that way before because otherwise you wouldn't be able to do project-specific override.

With this PR, servers are restarted if the ClientConfig does not equal (==) the old config anymore. But the _all_settings are currently excluded from that due to starting with an underscore (see __eq__ implementation). I don't know if there was a reason for that, but we could consider to change that property name if servers should be restarted on custom package settings change. So this should be a trivial change. But we might want to discuss if that should actually happen. These custom settings might be unrelated to the running server, so it might be desired to keep the server running.

I would think that restarting on a change of those would be more often correct given what those settings are used for currently (setting server path or toggling server manage). In your lsp-julia example one would require restart, the other perhaps not?

The _all_settings feature is not currently used by anything, I believe, so we should still be able to make breaking changes. And as I said in my newly opened discussion, I think I would want to do something differently there to be able to type things better.

Servers that opt-in to didChangeConfiguration notifications would get a notification and then should pull the configuration. If there is no change to the settings that are known by the server, in theory nothing should happen.

That's understandable but I'm obviously talking about cases like the one from LSP-pyright in which case we should do something.

Can/should we handle those cases gracefully somehow?
Perhaps opt-in mechanism so that packages have a chance to migrate?
I know that this is not ideal and requires more work for little gain but breaking user's config is not great either.

[predragnikolic]

related issues:
#2044
#2243

[rchl]

Should we actually have a dedicated method for comparing two configs? I feel like overriding the __eq__ implementation for that purpose is not right. Technically __eq__ should answer the question whether two objects are identical but we're using it more for "did settings that cause server restart change".

Also it's hard to search for places that rely on this check to verify if we didn't break something by changing this implementation.

[jwortmann]

Yeah maybe. I also had the same thoughts before. But I don't think there is any other code that would compare two ClientConfigs, and this already had some custom implementation (which excludes properties starting with _ from being considered when compared for equality, which for example already includes the "enabled", so I guess we could do the same with "settings"). There is also no comment in the source code that explains the exact purpose of the current custom __eq__ implementation. So I thought we could just use __eq__ when checking for "server-restart-equality"...

[jwortmann]

I have noticed one strange bug with this PR that makes helper packages often not work correctly.
Sometimes the change listener for the settings files from helper packages don't work. Sometimes I can set "enabled": false and then the server is shutdown as expected, but when I set "enabled": true again, no more change event gets triggered. But I don't know yet what causes this or how I broke it in this PR.

I really wonder what the purpose of this code in SettingsRegistration is:

LSP/plugin/core/types.py

Lines 179 to 188 in b2cd51d

	weak_self = weakref.ref(self)
	def on_change_handler() -> None:
	if self_ := weak_self():
	on_change(self_)
	self.settings.add_on_change("LSP", on_change_handler)
	def __del__(self) -> None:
	self.settings.clear_on_change("LSP")

When helper plugins are registered, we create a ClientConfig for each of them. This included a SettingsRegistration object, which is stored as part of the ClientConfig. But when it is garbage collected, the change listener is removed, so new changes in the sublime-settings file don't trigger the callback function anymore. And when something in the settings file changes, we have to create a new ClientConfig and replace the old one that was stored in the ClientConfigs.all and ClientConfigs.external dicts, which in turn triggers the old ones (including the SettingsRegistration) to be garbage collected. In theory I think it should work, because a new ClientConfig object with new SettingsRegistration should be created before replacing the old one:

LSP/plugin/core/settings.py

Lines 83 to 85 in b2cd51d

	def _update_external_config(self, name: str, settings_registration: SettingsRegistration) -> None:
	try:
	config = ClientConfig.from_sublime_settings(name, settings_registration)

I don't think I would have written such complicated code with all these self references and weak references that rely on things not being garbage collected for change listeners to work. Why not just attach a single change listener function when a plugin gets registered, and keep that around forever (until plugin unregistered)?

In general when writing the code in this PR I saw so much really weird and complicated code with all kind of self references and custom change listener abstractions in abstract base classes which even result in reference cycles between objects. And all these abstract base class interfaces make "Goto Definition" not work in the LSP code base, because Pyright always jumps to the abstract base class instead of to the implementation (I noticed that same annoyance many times before with the SessionBufferProtocol and SessionViewProtocol classes).

For example the following chain of functions is called when the "clients" in LSP.sublime-settings gets modified:

File	Method
settings.py	_on_sublime_settings_changed
settings.py	ClientConfigs.update_configs
settings.py	ClientConfigs._notify_clients_listener
windows.py	WindowRegistry.on_client_config_updated
configurations.py	WindowConfigManager.update
configurations.py	WindowConfigManager._reload_configs
windows.py	WindowManager.on_configs_changed
windows.py	WindowManager.restart_session_async

This jumps from settings.py to windows.py to configurations.py back to windows.py...

---

Edit: And another point why I think relying for things trigger on garbage collection is bad:
Imagine a future Python version doesn't trigger garbage collector immediately. Then if there is a change to the settings file of a helper package, the old change listener from the SettingsRegistration object in the old ClientConfig is still around, so if there is another change to that settings file, we would now get 2 distinct change events, i.e. our functions would also trigger twice, which again creates 2 new ClientConfig objects with corresponding change listeners. So I'd say that entire SettingsRegistration class is super dangerous...

[rchl]

I really wonder what the purpose of this code in SettingsRegistration is:

It might be wrong but also self.settings.add_on_change is potentially a bit magic (at least to me). I guess a case where there are two listeners using same key is not really supported because calling clear_on_change from one of those places will then clear both listeners (I think). Maybe the issue has something to do with that behavior?

I don't think I would have written such complicated code with all these self references and weak references that rely on things not being garbage collected for change listeners to work. Why not just attach a single change listener function when a plugin gets registered, and keep that around forever (until plugin unregistered)?

Makes sense in theory but I would have to dig into the code to understand things better.

In general when writing the code in this PR I saw so much really weird and complicated code with all kind of self references and custom change listener abstractions in abstract base classes which even result in reference cycles between objects. And all these abstract base class interfaces make "Goto Definition" not work in the LSP code base, because Pyright always jumps to the abstract base class instead of to the implementation (I noticed that same annoyance many times before with the SessionBufferProtocol and SessionViewProtocol classes).

Yes, but there is "Go to implementation" command to jump from abstract class to implementations.

Edit: And another point why I think relying for things trigger on garbage collection is bad:

That specific case of SettingsRegistration didn't rely on GC originally. It was done like that to workaround an issue with ST freezing - #2822

[jwortmann]

I have noticed one strange bug with this PR that makes helper packages often not work correctly.

Okay, I figured out that this is unrelated to this PR and also happens on main (can be tested by manually setting "enabled": false, then save, and then set back to true in a config). This bug was introduced with e31ee1e. I think by using deepcopy for the SettingsRegistration object in

LSP/plugin/core/types.py

Line 985 in b2cd51d

settings_registration=deepcopy(src_config._settings_registration),

we lose the reference to the old SettingsRegistration object when the ClientConfig that is passed as an argument gets garbage collected. The deepcopy copies only the self.settings and self.settings_path properties (I think). And then the new copy doesn't have any change listener function attached.

[rchl]

Okay, I figured out that this is unrelated to this PR and also happens on main (can be tested by manually setting "enabled": false, then save, and then set back to true in a config). This bug was introduced with e31ee1e. I think by using deepcopy for the SettingsRegistration object in

Good catch. I didn't intend to copy non-plain objects like that and I didn't do it in from_sublime_settings method.

Created #2867