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

Add support for external completion providers. Fix #2379 #2653

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gayanper
Copy link
Contributor

@gayanper gayanper commented May 13, 2023

This PR introduce new contribution point point ICompletionProposalProvider which can be implemented to provide external CompletionProposals into jdt.ls which can be registered through ICompletionContributionService from any jdt.ls extension implementation.

@gayanper
Copy link
Contributor Author

@rgrunber @jdneo @testforstephen @snjeza Let me know what you think. Once finalize I can add javadoc for contribution points.

@gayanper
Copy link
Contributor Author

This might benefit extensions such as STS4 to avoid doing another ICompilationUnit.codeComplete.

@jdneo
Copy link
Contributor

jdneo commented May 16, 2023

Hi @gayanper, is there any existing use cases(or request) about this proposal? Like STS, are they aware of this new contribution points?

@gayanper
Copy link
Contributor Author

No I didn't see any project has raise a issue for this. But i did look at how STS does today and they use the command handler approach. But I assume if this support is there they can use that. We could ask them.

One advantage is that we can reuse already computed context instead trying to recreate it in such scenarios where external plugs want to provide completion items.

@jdneo
Copy link
Contributor

jdneo commented May 16, 2023

Hi @BoykoAlex, this PR proposes a contribution point from JDT.LS to allow third-party extensions contribute their owner completions. We would like to hear your feedback regarding this :)

@angelozerr
Copy link

angelozerr commented May 16, 2023

Hi @gayanper, is there any existing use cases(or request) about this proposal? Like STS, are they aware of this new contribution points?

Here another Language Servers which uses the same strategy than STS to customize completion in Java file:

@BoykoAlex
Copy link
Contributor

There isn't a lot of completion proposals in Java sources that we in STS. I think there are only 3 use cases:

  1. Snippets for request mapping methods
  2. Property name completion inside @Value kind of annotations
  3. Spring-Data repository methods completion

Of course we could definitely convert those completions to use the extension above. The downside is that for debugging we'd have to debug JDT LS and tests would be eclipse tests i suppose.

If we go the route of contributing completions via JDT LS extension points why not contribute the rest of the features via JDT LS then: hovers, reconciling, go to definition/declaration. etc. Also some workspace features such as symbols

Currently, we parse java sources on the Boot LS side to provide the features above for Spring Java code. Furthermore we even parse the same Java source using OpenRewrite to take advantage of the refactorings this project offers.

I see the benefit of such extension points in eliminating the need to parse java source on our LS side. However, just by being able to contribute completions without being able to contribute the rest doesn't help a lot unfortunately.

@gayanper
Copy link
Contributor Author

@angelozerr Do you think for microprofile and jakarta also having them separate as in STS is much better than being part of the jdt.ls completion list compute cycle ?

If we look at what we get out of this extension is also the expected types at the given location and the ICompilationUnit and enclosing element of the completion position.

I think if a extension need more information such as ASTNode details it needs to parse the file by it self. So does that means that letting extensions to parse the java file as fit for them is much better compared to being tied to jdt.ls ?

@angelozerr
Copy link

In those ls we never parsed any java files. If we need to visit AST we delegate this feature in a delegate command handler.

A good sample is that we support snippet completion written in JSON vscode syntax. For instance the ls store json files snippet to generate annotation.

To know if snippet should be shown, we call a custom delegate command in JDT to know if completion is triggered before a method

@gayanper
Copy link
Contributor Author

I tried to find how java completions are connected in STS4 for @Value annotation, but couldn't find how. @BoykoAlex or @angelozerr can you explain how it is hooked into JDT.LS completion phase ?

@angelozerr
Copy link

I tried to find how java completions are connected in STS4 for @value annotation, but couldn't find how. @BoykoAlex or @angelozerr can you explain how it is hooked into JDT.LS completion phase ?

See explanation at https://github.com/eclipse/lsp4mp/blob/master/docs/architecture.md#microprofile-configproperties-language-features with explanation or see the schema at https://github.com/eclipse/lsp4mp/blob/master/docs/images/java-hover-sequence.png

@BoykoAlex
Copy link
Contributor

@gayanper please have a look at classes under this package https://github.com/spring-projects/sts4/tree/main/headless-services/spring-boot-language-server/src/main/java/org/springframework/ide/vscode/boot/java/value. The ValueCompletionProcessor is the class to look at for the case of completions... if you follow the calls to this class you'd trace stuff up to the completion request handling. Essentially, request received, routed to "java" language completion handler which parses the source file and then based on the position context routes to the proper completion processor.

@gayanper
Copy link
Contributor Author

Ok I see, so if I got you correct both @BoykoAlex and @angelozerr , you actually have your own LS which supports performing completions for java files and that is then delegated to jdt when we need information from the jdt it self using workspace command support in jdt.ls

I think this enables to lift off weight from the jdt.ls process which I see as some advantage. I think I will try to implement the mapstruct support with this approach since it needs both completions, quick fixes at least.

WDYT ?

@BoykoAlex
Copy link
Contributor

Sounds good to me...
If reconciling, hovers, navigate to definition would have a similar contribution mechanism at some point in the future STS4 likely to find it useful.

@gayanper
Copy link
Contributor Author

@angelozerr @jdneo @BoykoAlex i tried to implementation mapstruct support as we discussed above. Following are some things i found which kind of demotivate a extension provider support similar features

  • need implement a LS server with completion support, but still it needs to take care of the LS lifecycle

  • need to implement jdt.ls extension to resolve java elements thats needed for completions

  • The ls needs to make a jdt.ls command to get required information which makes ls nearly just a wrapper

  • In the jdt.ls extensions we still need to take care of resolving eclipse resources, looking at what jdt.ls has implemented, its too much to replicate on each extension

  • The AST parsing is limited since the default AST parser available in jdt doesn't provide all the workarounds that CompletionParser use when parsing intermediate file state while writing code. And there is no diet parsing which could lead to bad user experience in vscode completions.

Therefore i this a feature like i have presented in this PR will make the life of such extension providers easy and motivated to add support for vscode.

About other areas like hover, diagnostics etc, we could implement similar SPIs for straightforward implementation.

But for any of above features, if a extension finds that they need more control, they can implement what I mentioned above in my points.

WDYT ?

@gayanper
Copy link
Contributor Author

@gayanper
Copy link
Contributor Author

gayanper commented Jul 9, 2023

Any feedback on this ?

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.

4 participants