Skip to content
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

Allow limiting the workspace folders for a language server #267

Merged
merged 1 commit into from
Oct 24, 2022

Conversation

sebthom
Copy link
Member

@sebthom sebthom commented Oct 22, 2022

This change enables custom language clients to control which workspace folders are send to the language server initialize request and during workspace/didChangeWorkspaceFolders event by overriding the workspaceFolders method.
E.g.

class MyLanguageServerClientImpl extends LanguageServerClientImpl {
   @Override
   public CompletableFuture<List<WorkspaceFolder>> workspaceFolders() {
      return CompletableFuture.completedFuture(
         allProjectsWithNature("my.project.nature").stream() //
         .map(LSPEclipseUtils::toWorkspaceFolder) //
         .collect(Collectors.toList()));
   }
}

Without this change the initialize request will always contain all workspace projects no matter if they are relevant to the LS or not.
This also means the LS requests configuration settings for all projects via workspace/configuration.

Being able to limit the list of workspace folders can improve LS performance for slow language servers that search for resources across projects. It also means less work on the client side to construct JSON messages, e.g. for workspace/didChangeWorkspaceFolders and esp. for per-project configurations via workspace/configuration.

@sebthom sebthom force-pushed the wks-folders branch 2 times, most recently from c64610f to ae18f11 Compare October 23, 2022 19:01
@mickaelistria
Copy link
Contributor

I don't think this is a client side isuse. As you state, the problem is "Being able to limit the list of workspace folders can improve LS performance for slow language servers that search for resources across projects."; but then shouldn't such checks happen on server-side? What you suggest here IMO moves language/LS-specific concern ("can I support this project") into the client implementation while a full server side implementation could probably work better and be fully portable.

@sebthom
Copy link
Member Author

sebthom commented Oct 24, 2022

Eclipse plugin developers do not necessarily have control over third party language servers and I doubt you can convince any LS provider to implement server-side project/folder filtering e. g. based on Eclipse specific settings such as project natures.

After all. the client is already controlling the workspace folders announced to the server. Currently it simply is hard-coded to all projects of the Eclipse workspace.

This patch makes this configurable by plugin developers but does not change the current default behavior.

@mickaelistria
Copy link
Contributor

Eclipse plugin developers do not necessarily have control over third party language servers and I doubt you can convince any LS providers to implement server-side project/folder filtering e. g. based on Eclipse specific settings such as project natures.

How does it work in VSCode? Are the workspaceFolders filtered according to some metadata?
Can you please provide an example of such a LS?

In my current understanding, the LS is provided the whole IDE context per workspaceFolders, and I believe it's how things are designed in LSP; and then the server does validate whether a folder is relevant or not. So even if we can implement workarounds in LSP4E, I'm not really enthusiast in enabling workarounds and distracting overall manpower from fixing the real issue where it is (ie in the langauge server). Being rigorous about not allowing workaround ultimately force people to invest in implementing proper solutions, which are then reusable and better server the world.
Also, selfishely, accepting workarounds means we increase the maintenance cost. Why should LSP4E increase its maintenance cost if it's not the reason of the problem?

@sebthom
Copy link
Member Author

sebthom commented Oct 24, 2022

At some of our customers we have to work with very large Eclipse workspaces, which I think you usually don't do (or even can) with other IDEs e.g. like IntelliJ. I also think it is very unique to Eclipse to announce each project separately as a workspace folder. Editors like VSCode don't have the notion of projects and only announce the workspace root as a single entry.

I also don't think this PR puts unreasonable/increased maintenance burden on the project. I would say it actually increases maintainability. Currently there are three distinct locations in the code where the list of workspace folders - based on the open projects - is calculated via long chained stream operations. With this PR it is reduced to one location.

The PR also works towards the principle of least surprise. While it is possible to extend the LanguageClientImpl via an Eclipse extension which allows customization it's behaviour including the return value of the workspaceFolders(), the not-extendable LanguageServerWrapper continues to stick to another hard-coded internal logic to calculating this list and thus may send diverging values to the LS.

@mickaelistria
Copy link
Contributor

I think you usually don't do (or even can) with other IDEs e.g. like IntelliJ.

IntelliJ has "New > Module from Existing Sources..."; VSCode has the even more explicit "Add folder to workspace".
I'm curious about what kind of capabilites VSCode provides on that matter. I don't think we should support in LSP4E such a LSP pattern that is not supported by the reference implementation.

However, I also see that -as you advertise-, this improves factorization without adding API nor regression; so it qualifies objectively as a code improvement and worth being merged.

So let's just merge and keep in mind that maybe what you're trying to build here as a client-specific workaround is probably not the most profitable way to solve a problem that can happen in any IDE and that putting more responsibility on client instead of the server goes towards the opposite goal of what LSP is intended for :P

@mickaelistria mickaelistria merged commit 8b7f7cc into eclipse-lsp4e:master Oct 24, 2022
@sebthom
Copy link
Member Author

sebthom commented Oct 24, 2022

@mickaelistria thank you!

@sebthom sebthom deleted the wks-folders branch November 2, 2022 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants