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

Should you based on lsp4mp? #415

Open
angelozerr opened this issue Dec 16, 2022 · 6 comments
Open

Should you based on lsp4mp? #415

angelozerr opened this issue Dec 16, 2022 · 6 comments
Labels

Comments

@angelozerr
Copy link

I create the issue because I noticed your Jakarta LS uses some copy/paste code from lsp4mp MP LS and it seems that you do the same logic than MP LS :

  • delegate LSP request for Java file for codeAction, diagnostics
  • completion with snippets

IMHO I think it should be better to be based on lsp4mp, because you are using old code and we did a lot of improvement. You are using TextDocument which can takes some memory for big file. We improved the cancel checker too (when client cancel completion for instance, it should cancel the process on server side).

It seems your support works only with Eclipse IDE and not with vscode. If your project will be based on LSP4MP, it will work:

For Eclipse IDE and IJ, it is linked to Quarkus, but @jeffmaury could perhaps split this project in 2 project for IJ Eclipse IDE

  • jbosstools-microprofile, jbosstools-quarkus for Eclipse IDE
  • intellij-microprofile / intellij-quarkus for IJ

like we have with vscode-microprofile / vscode-quarkus

You could be based on jbosstools-microprofile and intellij-microprofile.

On JDT side, you could declare your codeAction with https://github.com/eclipse/lsp4mp/blob/master/microprofile.jdt/org.eclipse.lsp4mp.jdt.core/schema/javaFeatureParticipants.exsd#L9 extension point.

For completion for snippet, I think more and more, it is a bad idea to manage it with a MP LS, because snippet should take care of offset (ex : showing snippets for only annotations should appear before a method and not anywhere, because if you have a lot of snippets annotation, it will be very annoying for user to see it in a body method). We could do that in a MP LS side, but it means that it will consume on each key stroke a delegate command handler to know where the completion is triggered, which is very bad for performance). Since JST LS provides a new completion extension point, I think it is a better mean to manage snippets.

And I think that JDT LS should provide too diagnostics and codeAction extension point like completion @rgrunber to avoid doing somme communication between vscode and the JDT LS.

In other words, I think it should be very nice if we could share those ideas, and it is the reason I created this issue to know your opinion?

Do you think it could be a good idea for LSPJakarta to be based on LSP4MP?

@yeekangc
Copy link
Contributor

Thank you for the issue, Angelo. We are indeed noodling on some of these.

It does feel like we need a common layer where either the 2 LSs will build on or we need to clearly work out how the LSs should interact with each other. To refactor the relevant components too.

Also, one would think support for the APIs that MP reuses from Jakarta EE should come from LSP4Jakarta too? We wouldn't want duplication of support and efforts to build these?

Let us continue the discussion.

@angelozerr
Copy link
Author

@yeekangc here 2 new reasons why I think it should be better to be based on MP LS:

We have the intention too to try to improve memory for Java file (by removing TextDocument).

@datho7561
Copy link

  • @datho7561 is supporting snippet completion shown only if it is allowed

Here it is in action:

method-annotation-context

@ajm01
Copy link
Contributor

ajm01 commented Dec 18, 2023

@yeekangc, @angelozerr,

Reading the discussion above, we believe that a middle ground approach would allow LSP4Jakarta and potentially others to benefit from the infrastructure that LSP4MP has in place. Therefore, extracting this common infrastructure into a common set of artifacts (plugin/jar) would allow extenders/consumers to better use the infrastructure without placing direct dependency on any specific language server.

To this end, we have put together a proof of concept that separates LSP4MP common infrastructure. It basically separates common code within microprofile.jdt and microprofile.ls into their own common folders/components (common.ls and common.jdt). We placed the common components in the LSP4MP repository at the same level as the existing microprofile.ls and microprofile.jdt. These common components can easily be placed in a separate repository if it is preferred. They contain all of the parameter classes used to communicate between the language servers and their clients as well as common utilities, etc. found in both LSP4MP and LSP4Jakarta.

Here is the link to the proof of concept: https://github.com/ajm01/lsp4mp/tree/common-jdt-xtn. The proof of concept includes LSP4MP code base updates to integrate with the new common components.

Please take a look when you are able and provide your thoughts on it so that we might engage in a longer term discussion with you about it, its feasibility, and where we might go form here in terms of next steps.

@ajm01
Copy link
Contributor

ajm01 commented Jan 18, 2024

Hello @angelozerr, @fbricon, @yeekangc (and @scottkurz)

We wanted to see if you have had a chance to look at this POC we pointed to in the previous message. We would like to get your input on the approach it takes before we submit a PR to the LSP4MP repository. We would be happy to continue the discussion on a call or whatever channel you may prefer.

Thanks,

Andy Mauer and Ed Mezarina (@mezarin)

@angelozerr
Copy link
Author

angelozerr commented Jan 19, 2024

@ajm01 thanks for working on those topics. I fear it is a very hard and long issue.

I see quickly your work on https://github.com/ajm01/lsp4mp/tree/common-jdt-xtn but I think its is not enough because we should be more extensible, but today I'm very busy with https://github.com/redhat-developer/lsp4ij and I have no time today to do some review.

IMHO the refactoring of LSP4MP / Jakarta LS must be done step by step and not in a big PR. Let me trying to give you my feedback.

commons.ls

Indeed we need this kind of project. I wrote those classes for LemMinX at first and when I have implemented other language server like MicroProfile LS, Qute LS, I have copied/pasted those the LemMinX classes in MicroProfile and Qute LS. But we need really a library for that to share classes between those project (and your Jakarta LS).

I had planed to do that but the bad thing is that each LS did some API changes, so we need to unify the API between each LS. A thing which is very important is test, we need to integrate https://github.com/eclipse/lemminx/tree/main/org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/commons tests

Using "commons" package is a bad idea, the main idea with this new library is to provide an LSP4J support to help LSP client and server to implement an LSP client and an LSP server based on LSP4J.

We need to discuss with LSP4J if they think we could host this JAR in LSP4J himself. I had created some issues at:

So perhaps we could provide a new dependency org.eclipse.lsp4j.support which have the following packages:

  • org.eclipse.lsp4j.support.client which will host the TextDocument, etc classes
  • org.eclipse.lsp4j.support.server which will host the LSP snippet parser that I did for LSP4IJ (LSP support for IJ)

If LSP4J doesn't want to host this library, we could host it to redhat developer and provide com.redhat.lsp4j.support

commons.jdt.

IMHO I think it is not enough, because this project should provide the extension point codeaction, codelens for Java.

in other words:

In other words, we could create a new plugin org.eclipse.lsp4jdt which will contain those extension point and contains classes from:

But before doing that,

  • I suggest that you consume in your JakartaLS project the LSP4MP extension point to check if my idea is working for you.
  • org.eclipse.lsp4jdt is a good name?

JavaDelegateLanguageServer

The last thing to do is to provide a new LS JavaDelegateLanguageServer which will replace your JakartaLS and remove the JavaTextDocumentService from MicroProfileLanguageServer (this LS should take care only from properties file)

The JavaDelegateLanguageServer is an LS which delegates the compute the Java codelens, codeaction, etc to an external Java component (JDT, IJ Psi) which is able to parse Java files, visit them, etc (like today with MicroProfileLanguageServer and JakartaLanguageServer)

The JavaDelegateLanguageServer must be extensible to register snippets (register snippets for MP, for Jakarta) by using a SPI mechanism.

Hope you will agree with my suggestion and you will understand my explanation.

I'm sorry again but today I'm very busy with LSP4IJ, so it is very hard to help you more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants