Needed refactorings

[jwortmann]

This is a list of refactorings that I think would be useful, in order to make the behavior of LSP more correct, more efficient, and would make it possible to implement certain new functionalities. Please comment if you have opinions about these, or better or more suggestions.

---

Move logic for code action requests into SessionBuffer class implemented in #2835

One way how code action requests are triggered is whenever new diagnostics arrive (from publishDiagnostics notifications, from pull diagnostic responses, or from workspace diagnostics partial result progress reports):

LSP/plugin/documents.py

Lines 343 to 349 in 294a1af

	def on_diagnostics_updated_async(self, is_view_visible: bool) -> None:
	self._clear_code_actions_annotation()
	if userprefs().show_code_actions:
	if is_view_visible:
	self._do_code_actions_for_selection_async()
	else:
	self._code_actions_for_selection_needs_refresh = True

Currently, code action requests for all sessions are created at the same time in

LSP/plugin/code_actions.py

Lines 149 to 161 in 294a1af

	tasks: list[Promise[CodeActionsByConfigName]] = []
	for sb in listener.session_buffers_async('codeActionProvider'):
	session = sb.session
	if request := request_factory(sb):
	# Pull for diagnostics to ensure that server computes them before receiving code action request.
	listener.purge_changes_async()
	sb.do_document_diagnostic_async(listener.view, listener.view.change_count())
	response_handler = partial(on_response, sb)
	task = session.send_request_task(request)
	tasks.append(task.then(response_handler))
	# Return only results for non-empty lists.
	return Promise.all(tasks) \
	.then(lambda actions_list: list(filter(lambda actions: len(actions[1]), actions_list)))

Therefore, in a multi-session environment, a new code action request is sent again to a particular session, even when the diagnostics from another session arrive. This results in many useless and repeated code action requests for multi-session setups.

Instead, when diagnostics arrive, the code action request should only be triggered for that particular session. So the logic for code action requests, or at least some parts of that, should be moved into the SessionBuffer, and then it can be triggered directly from SessionBuffer.on_diagnostics_async. When code actions are triggered due to selection change, or due to code actions on save/format, we then still need to iterate all SessionBuffers for the individual requests.

Then the

LSP/plugin/documents.py

Line 217 in 294a1af

self._code_actions_for_selection_needs_refresh = True

flag could be moved into the SessionBuffer, as part of the SessionBuffer.pending_refreshes bitflag.

---

Move server configurations into its own settings file implemented in #2862

Currently, server configurations are stored under a "clients" key in the main LSP.sublime-settings file.
There is a change listener for that settings file and previously all servers were restarted whenever anything in that settings file changed. In #2448 I implemented that LSP keeps a hash value derived from the "clients" setting in memory, and when a change to the settings file is detected, it is compared with the new hash value and servers are only restarted if something in the "clients" did actually change. This allowed general LSP settings to be modified without automatically restarting all servers.

One idea is to move the server configurations into a new settings file, then we could use a separate change listener for that file and the current hash comparison for "clients" wouldn't be necessary. The individual server configs could then be top level keys in that settings file. Possible names for the new file could be

• Servers.sublime-settings
• Server Configs.sublime-settings
• ServerConfigs.sublime-settings
• Configurations.sublime-settings

Of course, we don't want to break all current user configs, therefore such an implementation should be made backwards compatible with the current "clients" setting. We could add a deprecation message for "clients" via JSON schema, and perhaps eventually remove compatibility for that after a year or so.

There are still some possible improvements. First, only the server whose config was modified should be restarted. But maybe it's not even useful to restart the server. Ideally we should detect if the "settings" in the server config changed, and in that case send a workspace/didChangeConfiguration notification with the changed settings to the language server. In theory, if for example the "command" or the "initialization_options" in the config changes, we still would have to restart the server. But maybe we could just ignore that and leave it to the user to trigger the "LSP: Restart Server" command manually in that case.

I guess the implementation of workspace/didChangeConfiguration wouldn't strictly need a separate settings file for the configs, and we need to keep track of changes to the "settings" key from the individual configs anyway (also for settings in files from hepler packages). But it is just an idea to have a clean division between the LSP settings and the server configs, and possibly make the implementation easier. I also think it would be a good time to finally remove the remaining "default_clients" configs.

---

Rewrite dynamic registration

The current implementation assumes that a server can only register a single provider per capability. But this is a wrong assumption (compare microsoft/language-server-protocol#1069 (comment)). Capability registration uses registration IDs, and currently the code at

LSP/plugin/core/types.py

Lines 616 to 618 in 294a1af

	if isinstance(stored_registration_id, str):
	msg = "{} is already registered at {} with ID {}, overwriting"
	debug(msg.format(capability_path, registration_path, stored_registration_id))

just overrides previously registered capabilities, which is bad.

I made a workaround for this when fixing dynamic registrations for diagnostic providers in #2727, and currently the pull diagnostics seem to be the only feature where multiple providers per server are relevant. But ideally it should be implemented in a clean way and generalized to all capablities, so that registering multiple providers for any capability becomes possible. This is probably low priority for now.

Then we have to rethink the has_capability and get_capability methods that are used in various places. That logic should probably be replaced by something like has_provider and get_providers, or alternatively the current has_capability needs to check all registered providers for that capability.