[ty] Support multiple workspace folders in a single ty LSP server instance by BurntSushi · Pull Request #22953 · astral-sh/ruff (original) (raw)
This is a small code change meant to implement a simpler semantic in preparation for supporting multiple workspace folders. Previously, we were combining global options across all workspace folders. In this change, we now only combine global options from initialization and each workspace folder independently. We then take the most recent combination and use that for our global settings.
It's not clear that this actually changes anything in practice. I've been unable to make VS Code send different LSP configurations for each workspace folder within a single workspace. (It's plausible our ty VS Code extension is actually doing this.)
… server instance
This is pretty much all we need to support multiple workspace folders. Previously, we were going out of our way to force only allowing a single workspace. This commit just removes that restriction.
This works by making each workspace folder use its own project state (i.e., salsa database).
We chose to go this route for now since it's the simplest to implement and unlocks folks using multi-root workspaces inside of VS Code. We hadn't done this before because we were undecided on how this would impact memory usage. In particular, this will result in a separate auto-import cache for each workspace folder. We live with this limitation for now.
This does not add support for responding to
workspace/didChangeWorkspaceFolders notifications. We'll do that in a
subsequent commit.
I find this is a bit easier to reason about.
Previously, we only kept a count of the number of workspaces initialized, but we had no way of knowing which workspaces were initialized. Knowing which are initialized is useful for avoiding initializing a workspace again.
This is relevant because previously we only called
initialize_workspace_folder at actual initialization time. But soon
we're going to want to call it at arbitrary points in response to
workspace/didChangeWorkspaceFolders notifications. Making
initialization a no-op if a workspace is already initialized seems more
flexible and harder to misuse. With that said, presumably, LSP clients
won't try to re-initialize workspace folders.
This does change a couple checks that were previously constant time to be linear over the number of workspace folders. I think this is fine given that the unit of work here is tiny and the number of workspace folders is not expected to be enormous. (The point at which a linear scan over all workspaces is problematic likely means we have much bigger problems.)
This basically moves the logic out of Server and into Session.
Moreover, instead of iterating over all registered workspace folders,
it only iterates over registered workspace folders that haven't yet
been initialized. This in turn makes the routine idempotent and easier
to call (for when we register new workspace folders).
This lifts the already existing helper routine to a place where more things can use it. We'll want this for when we remove a workspace folder.
…cations
With all of the refactoring done in previous commits, this mostly comes down to adding a new notification type and a new method for removing a workspace folder (which requires some clean-up).
This tests various permutations for adding and removing workspace folders both during and after server initialization. We also add some tests checking the behavior of settings as passed from the LSP client.
We make use of workspace diagnostics here as a way to check whether the server is "aware" of a workspace folder or not.
BurntSushi added a commit to astral-sh/ty-vscode that referenced this pull request
With astral-sh/ruff#22953 merged, the ty server now supports multiple workspace folders. This means we can deal with workspace settings diverging between folders.
This change specifically only applies to workspace settings. The other
global settings are left with a scope of window, since the ty server
expects there to be only one value for global settings.
Closes #319
BurntSushi added a commit to astral-sh/ty-vscode that referenced this pull request
With astral-sh/ruff#22953 merged, the ty server now supports multiple workspace folders. This means we can deal with workspace settings diverging between folders.
This change specifically only applies to workspace settings. The other
global settings are left with a scope of window, since the ty server
expects there to be only one value for global settings.
Closes #319
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})