folderWorkspaces support

[krzyzanowskim]

• A server maintains an array of Workspace
• Client may add/remove workspaces as defined here: https://microsoft.github.io/language-server-protocol/specification#workspace_workspaceFolders
• Server reports capability
• Workspace.Configuration holds the configuration values
• Added more workspace related, but not used here models (request, notification) to the protocol.
• Special purpose noRootWorkspace instance is used to collect all the opened documents without an actual workspace

[krzyzanowskim]

It's done. Ready to review.

[benlangmuir]

So far I've just skimmed through this, but what's supposed to happen if a workspace configuration changes while there are still documents open in that workspace? It looks like we'll leak the documents (consider a document open in sourcekitd/clangd - it has no idea what happened) and drop any future requests (at the very least, any request needs to get a response).

[krzyzanowskim]

That's a valid question. I guess I assumed that it's up to the client to close the files first, then remove workspace - it was too naive. Most likely DocumentManager should close all the files and post the notifications. Will do that.

[krzyzanowskim]

Second thought. The client can't be notified what documents are closed (no such notification). The only DidCloseDocument is from client -> server. If the server will close documents, and the client won't... it will be out of sync. Either way, the client has to close the documents before removing the workspace folder.

[benlangmuir]

There's no requirement that documents be in any workspace folder, is there?

[krzyzanowskim]

no, there is not. I'll try to transfer opened documents to the "no workspace" workspace - this should do the trick.

[krzyzanowskim]

it's done so feedback is welcome

[benlangmuir]

A high-level question about this: how should multiple workspace folders interact with the build system and the index. In the LSP terminology, I think it would be correct to say there is a single workspace with multiple folders. This patch currently assumes each folder is an independent workspace with its own build system and index. That might or might not be what you want.

For example, if you have a swiftpm package dependency in editing mode, it could show up as an independent folder (you can specify the path, so it might not be inside the parent package), and it's unclear to me what we want in that case. Ideally you probably want the index to be global, but we're not setup to do that right now. And for the build system, you might want a single swiftpm workspace shared with the parent project (more efficient, more likely to have consistent settings), or you might want to use a separate swiftpm workspace just for the dependent package (e.g. you are actively working on just that part, and building it separately).

@akyrtzi any thoughts on this?

---

I left some specific comments on other parts of the patch, and before we merge this I'd also like to spend some time to play with how this works in vscode. Sorry for the long review cycle. If you see me not replying for more than a week, please feel free to ping me.

[benlangmuir]

Why can't we lookup the workspace here and pass it in?

[krzyzanowskim]

it's too generic.
However, it could if registerWorkspaceRequest constraint to TextDocumentRequest - I'll do that - only apply to requests though.

[benlangmuir]

This patch changed the server to contain multiple workspaces, but this particular change to factor the fields into a Configuration struct seems to assume the opposite: a single workspace with multiple folders. Can you clarify the intent?

[krzyzanowskim]

Looks like post #65 merge issue.
Configuration only encapsulate config settings, doesn't change the previous meaning, does it? The BuildSetup though is overriding the setup for all workspaces that is not intended here.

[krzyzanowskim]

The BuildSetup though is overriding the setup for all workspaces that is not intended here.

After revisiting the code, it looks ok to me (I did refactor it to make the initializer intent clearer: d11029c).
The BuildSetup doesn't matter for ".noRoot" Workspace. Each property is setup per Workspace instance.

[akyrtzi]

Considering the model as "single workspace with multiple folders" makes sense to me. Not sure about the build system but I think it is valuable to have the index be global for the workspace in such a model.

Even if you have multiple folders, you arranged them in a workspace presumably because they are inter-related and you want to be able to "jump" between them easily. The index being global would allow doing "jump-to-definition" across folders and search for symbols in the whole workspace.
A similar model would be an Xcode workspace with multiple projects, where the index is global for the same benefits.

[krzyzanowskim]

@benlangmuir: In the LSP terminology, I think it would be correct to say there is a single workspace with multiple folders.

see microsoft/language-server-protocol#298 (comment). if I understand the protocol designers intent correctly the workspaceFolders replaces rootUriand creates a container for the workspaces

That's how it looks in VSCode:

@akyrtzi Not sure about the build system but I think it is valuable to have the index be global for the workspace in such a model.

yeah. not sure how to use a single index though.

[benlangmuir]

if I understand the protocol designers intent correctly the workspaceFolders replaces rootUriand creates a container for the workspaces

I agree about the functionality, but the terminology is that there is a single workspace with multiple (root) folders. For example, the spec says:

Many tools support more than one root folder per workspace.

It's similar in Xcode where we have a single workspace with one or more projects in it. If we could preserve that in the code I think it would be clearer how it relates back to the spec. E.g. one Workspace with multiple WorkspaceFolder/Workspace.Folder/similar. What do you think?

@akyrtzi Not sure about the build system but I think it is valuable to have the index be global for the workspace in such a model.
yeah. not sure how to use a single index though.

This isn't something we're setup for right now, but it's good to have in mind for future index improvements. If the folders have separate builds I think we'd need to teach the index to handle multple datastores and then figure out where to put the global index files, etc. I'm sure we'd run into lots of fun problems to solve.

I'm still not sure what to think about the build system side of this. I like to use swift package edit and there are times when I would want it to share a swiftpm workspace and use the index from the parent project, and there are times when I would want to separate them since I'm actively building only the dependency. Maybe there just isn't a right answer for this.

[akyrtzi]

I think the build and index should be interlinked together, meaning a build should be accompanied by an index that is associated with that the build. Trying to using one index with multiple builds is going to lead to a very complex and hard to manage model.

[krzyzanowskim]

but the terminology is that there is a single workspace with multiple (root) folders. For example, the spec says:

in terms of terminology, I see it as:

1. Rename Workspace -> Project
2. New Workspace is a container for [Project]

Proof of concept is here:
https://github.com/apple/sourcekit-lsp/compare/master...krzyzanowskim:workspace-projects?expand=1

[krzyzanowskim]

Don't merge - I need to re-start it from the scratch applying Workspace/Project #64 (comment) idea. Also current version break "folding" tests (shrug) for some reason.

I'm closing this PR so it won't stay open forever.