-
Notifications
You must be signed in to change notification settings - Fork 185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Workspaces: one server instance per window #697
Conversation
I am so excited for this change. 😍 |
It's a WIP right now, and the added functionality might be somewhat underwhelming. Because if a language server is not multi-workspace-aware, it still uses the rootUri property of the initialize request, ignoring other folders that the user set up. Multiple workspaces are also somewhat of a chicken-and-egg problem: if the client doesn't support multi-workspaces, why bother implementing it in the language server? Conversely, if not many language servers are multi-workspace-aware, why bother implementing it in the client? I want to add a section in the wiki/docs targeted towards language server maintainers explaining how they should handle multi-workspaces for this reason. |
After reviewing some popular language servers it seems the only one capable of multiple workspaces is the Julia language server: https://github.com/julia-vscode/LanguageServer.jl/blob/908dbd7dbcf532a76fe1e3d26c6322c2a858b857/src/requests/init.jl#L76 I thought RLS would have support for multiple workspaces, but that isn't the case. And it doesn't seem likely that they'll be implementing that soon: rust-lang/rls#608 And as observed the flow language server also ignores workspaces. So the actual support for multi workspaces is lack luster. Should we move towards the "one server per workspace" strategy instead of the "one server per window" strategy? I believe we can actually accomplish both types of strategies. Again, WIP! |
Nice research and work towards implementation here! I think the users waiting for us to send Implementing 1-session-per-root on our side is likely a win for people already using multiple roots. For @plasticine's use case (#443), adding new roots for the sub paths "services/app" and "services/ui" in his sublime project will make his ruby and flow language servers work. Did you want to look into how we can improve the 0-folders session handling? Forcing a single |
Agreed. Though it's going to be more work for us, I predict not many language servers will ever implement multi-workspaces.
I haven't tried to reproduce this. But I'll incorporate it and keep it in mind. |
Just poking my head in here to comment that gopls uses workspaces. It is actually the recommended (read: only) way to work with multiple go modules at the same time. Trying the branch does appear to work for the workspaces in a sublime project on gopls. There does appear to be a bug in the autocomplete. Testing on windows. |
Good to hear! I'd like to pick this up some time in the near future now that the dust has settled on many other changes :) |
@oralordos, in what way is a "single-server-multiple-roots" approach "the only way" to work with multiple go modules? Keep in mind I don't actually know much about go. In particular, why would a "server-per-root" approach not work for gopls? |
There are at least two ways to implement workspaces: 1) One server connection per root folder 2) One server connection for all root folders in the window We're going with the 2nd option in this commit.
81d4cc8
to
c2fb940
Compare
Rebased to latest master and squashed all commits together. |
For the server per root method, not sure. Gopls is still in active development (latest version is v0.2.0), and all of their information is based on VS Code, where they use workspaces. All help is based on this currently. Server per root may work, but they have no posts saying such one way or another. I would guess that it would work, but is not officially supported? |
Server per root folder is essentially a simplified situation from the POV of the language server. If we handle a server connection per root folder, then even if the language server has support for multiple root folders, it should obviously be able to handle one root folder. I'm gonna go ahead and do another WIP PR where I implement the "server per root folder" approach. |
Since you're continuing with this branch: I think we need to consider where we use the SublimeLike/WindowLike/ViewLike protocols. They were introduced to have a minimal interface to sublime/window/view in code that doesn't import the sublime api. Is there some need/benefit to spreading WindowLike outside |
It's been a while, but I remember due to using them in plugin/core/workspace.py I encountered a bunch of mypy errors saying that sublime.View/sublime.Window mismatches with ViewLike/WindowLike. Changing the signatures to ViewLike/WindowLike made the mypy errors go away :) Then there was the problem that some methods of sublime.View return a sublime.View, while the ViewLike "protocol" classes returned a ViewLike, which then also mismatched. So I had to change those return types to Any to make those errors go away. I don't intend to branch off from this (although I will probably re-use some parts). I'll try to see if I can keep the sublime.View signatures. |
def is_valid(self) -> bool: | ||
... | ||
|
||
def folders(self) -> 'List[str]': | ||
... | ||
|
||
def find_open_file(self, path: str) -> 'Optional[ViewLike]': | ||
# should return Optional[ViewLike], but creates conflict when a real sublime.View is presented | ||
def find_open_file(self, path: str) -> 'Optional[Any]': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This for example will make mypy complain when the return type is an Optional[ViewLike]
@@ -633,7 +633,7 @@ class View: | |||
... | |||
|
|||
def find_all(self, pattern: str, flags: int = ..., fmt: Optional[Any] = ..., | |||
extractions: Optional[Any] = ...) -> 'List[Region]': | |||
extractions: Optional[Any] = ...) -> 'Iterable[Tuple[int, int]]': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another example where ViewLike and sublime.View were incompatible. Had to change the signature here.
Ah yes there are issues at the boundary where a sublime.Window is passed into a function expecting WindowLike. I've used |
I'm going to make a bunch of separate pull requests for each of the issues found in this PR. |
I'm closing this in favor of the one-server-per-workspacefolder approach (so, for instance, there will be multiple rust language servers active in one sublime project) |
Supersedes
#601, #472Close #443
Tested with the following language servers:
Tested on the following platforms:
The concept of a Workspace in LSP matches perfectly with the concept of a folder in SublimeText. I feel like this feature was added to LSP specifically for supporting this in the case of SublimeText. Although (as is usual ;) ) VSCode now also has the notion of workspaces: https://code.visualstudio.com/docs/editor/multi-root-workspaces
A language server is expected to be able to handle multiple workspaces: https://github.com/Microsoft/vscode/wiki/Adopting-Multi-Root-Workspace-APIs#language-client--language-server
Let's lay out the possible states of SublimeText:
When testing these situations, make sure to delete your session
file at
$sublime_config_dir/Local/Session.sublime-session
.window.project_file_name() is None and window.project_data() is
also None. According to the LSP specification, we're allowed to
report
null
forworkspaces
.state, window.project_file_name() is None and window.project_data()
contains a single folder that points to "/path/to/dir". We can
report that single folder (as URI) for
workspaces
.This state is the most common situation the user will be in.
("subl /path/to/dir/myproject.sublime-project"). In this state,
window.project_file_name() is an absolute path to the project file.
window.project_data() is a dictionary of names to (relative)
folders. We will convert all of the folders to absolute paths,
make them URIs, and then report that list as the current
workspaces
.There is a complication with the last two points. Namely, when you
have a Sublime window open in the background, and then open a single
file, say "subl /path/to/foo.json", then that single file will become
part of the existing window.
Such a single file may be outside the workspaces of the window.
I thought about it for a while, and decided to ignore this
complication. That is, when that single-file starts a new session,
it will still report the workspaces of the window.
directories, such a file can stand on its own (e.g. a JSON or
YAML configuration file). A server for such a file should be
able to handle single files and ignore workspaces.
occupies a single window where window.project_data() is None.
In that case we'll send
null
for the workspaces so we'realready expecting the language server to be able to work without
workspaces.
There are a few places where we assumed a single root URI. For
example, handling textDocument/references. I have modified these
occurences to take multiple folders into account, by using the
common prefix of all workspaces instead.
Finally, it's also possible that the window folders change when
using the "Quick Switch Project" feature. We should shut down old
sessions and start new sessions in that case. That behavior has
not changed.
This implementation does not try to be "smart". By that I mean that we won't be "merging" two workspaces if one is a subfolder of the other. It is up to the language server to handle this (if even necessary and/or desired).
When the number of workspaces is zero or >1, it is ambiguous what the
working directory of a language server should be. For this reason I think
it's best to use a temp directory.
When running under Sublime Text, the cache path is
When running under pytest, the cache path is
This PR also implements the
workspace/workspaceFolders
request, going from language server to us, as it is so tied to theworkspaces
capability.A future PR would implement the
workspace/didChangeWorkspaceFolders
notification, coming from us to the language server.