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 chain completions support #2544

Merged
merged 9 commits into from
Jun 26, 2023
Merged

Add chain completions support #2544

merged 9 commits into from
Jun 26, 2023

Conversation

gayanper
Copy link
Contributor

Bring chain completions support to JDT.LS which was started by @rgrunber. I clean it up a little bit and add

  • Support for overloaded methods where expected type signatures in the context is more than one.
  • Some support suggesting chain completions using static methods in classes like Collectors as a POC. The idea is to improve the this to read from a simple model file where either Microsoft IntelliCore (cc: @jdneo @testforstephen) can provide or any other interested extension provider. We need to discuss the best way to provide this as a extension point for providing model information (cc: @rgrunber @mickaelistria @fbricon )

@gayanper
Copy link
Contributor Author

Related vscode extension PR redhat-developer/vscode-java#3008

@gayanper
Copy link
Contributor Author

@rgrunber will you have time to look into this change ?

@gayanper
Copy link
Contributor Author

@rgrunber let me know how it looks now, so if things look good, I can see to add some tests.

@rgrunber
Copy link
Contributor

rgrunber commented Mar 30, 2023

I think the postfix completion tests and chain ones were done in the exact same way in JDT-UI. So I would have a look at https://github.com/eclipse-jdt/eclipse.jdt.ui/blob/master/org.eclipse.jdt.text.tests/src/org/eclipse/jdt/text/tests/contentassist/ChainCompletionTest.java for test ideas and https://github.com/eclipse/eclipse.jdt.ls/blob/master/org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/PostfixCompletionTest.java for how the utility methods were written that do the actual testing.

Hope to find some time to have another look at this.

@gayanper
Copy link
Contributor Author

I think the postfix completion tests and chain ones were done in the exact same way in JDT-UI. So I would have a look at https://github.com/eclipse-jdt/eclipse.jdt.ui/blob/master/org.eclipse.jdt.text.tests/src/org/eclipse/jdt/text/tests/contentassist/ChainCompletionTest.java for test ideas and https://github.com/eclipse/eclipse.jdt.ls/blob/master/org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/PostfixCompletionTest.java for how the utility methods were written that do the actual testing.

Hope to find some time to have another look at this.

I added one test, But i can also include the similar scenarios in JDT UI Chain Completion, if you think the approach of this implementation is good to be included in JDT.LS.

I have done a additional thing to discover the context specific entry points to find completions at computeContextEntrypoint, I think we could later introduce a SPI or commandHandler extension for this too.

@rgrunber Let me know what you think so we can finish this soon.

@rgrunber
Copy link
Contributor

rgrunber commented May 26, 2023

It works well for me, and as long as we disable by default, I think it should be fine to merge. only other thing I noticed is that since we started using the labelDetails field of completion items, the items are displayed a bit differently now :

image

We could always address this in a subsequent change though.

@gayanper
Copy link
Contributor Author

It works well for me, and as long as we disable by default, I think it should be fine to merge. only other thing I noticed is that since we started using the labelDetails field of completion items, the items are displayed a bit differently now :

image

We could always address this in a subsequent change though.

Check the latest chains @rgrunber I also tried to make the label details closer to other completion items.

@gayanper gayanper marked this pull request as ready for review May 27, 2023 18:03
@rgrunber rgrunber self-assigned this May 31, 2023
@gayanper gayanper force-pushed the chain branch 2 times, most recently from 21b0924 to e55ebc3 Compare June 14, 2023 19:39
@gayanper
Copy link
Contributor Author

@rgrunber when will you be able toe check the latest changes ?

@rgrunber
Copy link
Contributor

rgrunber commented Jun 22, 2023

chain-completion-display-bugs

I think it's just a matter of getting rid of duplicates, and cleaning up how the return value is displayed (unnecessary .)

I'm not sure why there are duplicates though. The example above comes from a testcase in JDT where we assert only 1 element is discovered (from the chain completion proposal computer).

Update: I think there are duplicates added to entrypoints in findEntrypoints(..).

rgrunber and others added 9 commits June 24, 2023 19:42
- Add IJavaElement (IMethod/IField) to data object of CompletionItem
  for computation at a later point
- Set default chain completion preferences
- Update target platform to 4.28-I-builds
- React to change in StringConcatToTextBlockFixCore API

Signed-off-by: Roland Grunberg <rgrunber@redhat.com>
- add new configuration to enable chain completions
- add support for static chain completions for provided know types
- Exclude primitives and boxed primitives
- Exclude String and Object types
- Update default chain preferences for better suggestions
- Fix type parameter ending up in completed text with latest JDT.LS
- Add support context List, Set and Map
- add support for snippet
- add support for variable names
- fix label and details
- handle duplicates
@gayanper
Copy link
Contributor Author

@rgrunber

I think it's just a matter of getting rid of duplicates, and cleaning up how the return value is displayed (unnecessary .)
Fixed

Update: I think there are duplicates added to entrypoints in findEntrypoints(..).
Fixed

Copy link
Contributor

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change works well so I'm going to merge. In addition it's disabled so at least it if there's anything we missed, it won't be a problem by deafult.

Just one thing I noticed. I took a project like https://github.com/eclipse/lemminx/blob/main/org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/XMLLanguageServer.java#L129C5-L129C73 and wanted to show that the suggestion would be made for something like ExtendedClientCapabilities cap = |, but i couldn't see the suggestion until I increased the chain completion timeout setting.

I think in the future we should look at how we prioritize the entry points, perhaps choosing local variables before static types.

@rgrunber rgrunber merged commit 230d394 into eclipse-jdtls:master Jun 26, 2023
6 checks passed
@gayanper
Copy link
Contributor Author

I can improve it @rgrunber. Will push a new PR during next few days or next week.

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.

None yet

2 participants