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

Check completion proposal is compatible or not. #2733

Merged
merged 2 commits into from
Jul 4, 2023

Conversation

jdneo
Copy link
Contributor

@jdneo jdneo commented Jun 28, 2023

  • Early return if the completion proposal is not compatible with the expected type.

- Early return if the completion proposal is not compatible with the
  expected type.

Signed-off-by: Sheng Chen <sheche@microsoft.com>
Copy link
Contributor

@testforstephen testforstephen left a comment

Choose a reason for hiding this comment

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

https://github.com/eclipse/eclipse.jdt.ls/blob/1cd21c63ff65d369415179e4b93dc4ca09e1a9ac/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/contentassist/CompletionProposalReplacementProvider.java#L804-L810

Previously, we had to use type hierarchy to find the inheritance path between the proposed type and the expected type for each constructor completion proposal. This was slow for a long proposal list. Now we can use the cached compatibility between the proposed type and the expected type to filter out the proposals that are not inherited earlier. This makes sense for performance improvement.

LGTM.

@fbricon
Copy link
Contributor

fbricon commented Jun 28, 2023

Continuing the discussion here

The returned completion proposal from JDT Core won't contain MyClass.MyList(). Besides, if the completion is triggered after MyL|, MyClass.MyList() is not in the proposal list either. (Same result in Eclipse).

So we have no chance to check isCompatible in this case.

I don't expect MyClass.MyList() to be proposed (although with chained completion that would be possible I guess), but today I can do :
Screenshot 2023-06-28 at 10 33 46

then once MyClass is selected, can complete on MyList.

With the new change I wouldn't be able to complete on MyClass.
I understand the appeal about performance, but I'm not comfortable losing that, when we don't have tow-tiered completion like IDEA, where "everybody's welcome" is used by default but you can switch to "isCompatible" by doing Shift+Ctrl+space

@jdneo
Copy link
Contributor Author

jdneo commented Jun 28, 2023

Wired...

I tried the completion with v1.20.2023062804 (without this PR), and I got following list:
image

It only has MyClass(), but no MyClass. (You wish to have MyClass without the () right?)

Is there any special setting to make MyClass appear in the list?

@fbricon
Copy link
Contributor

fbricon commented Jun 28, 2023

you probably have MyClass down the list. In my case, I selected once already so I believe VS Code might have bumped its priority to put it up the list.

@fbricon
Copy link
Contributor

fbricon commented Jun 28, 2023

also try MyC, then trigger completion

Signed-off-by: Sheng Chen <sheche@microsoft.com>
@jdneo
Copy link
Contributor Author

jdneo commented Jun 29, 2023

Checked the inner class case. It's working with this PR actually. I added a new test case to cover that.

@testforstephen
Copy link
Contributor

Checked the inner class case. It's working with this PR actually. I added a new test case to cover that.

since it doesn't break the case Fred mentioned, I think we can merge this PR now.

@testforstephen testforstephen added this to the End July 2023 milestone Jul 4, 2023
@testforstephen testforstephen merged commit 66669c8 into eclipse-jdtls:master Jul 4, 2023
4 checks passed
@jdneo jdneo deleted the cs/compatible-proposal branch July 4, 2023 03:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants