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

Fix the wrong additional text edit when complete import statement #1944

Merged
merged 1 commit into from
Dec 15, 2021

Conversation

jdneo
Copy link
Contributor

@jdneo jdneo commented Nov 17, 2021

fix redhat-developer/vscode-java#1422.

No need to add import when user is complete on an import statement.

Signed-off-by: Sheng Chen [email protected]

@fbricon
Copy link
Contributor

fbricon commented Nov 17, 2021

I can't reproduce the issue with vscode. I see the extra textedit in the tests, if I remove your fix, but don't see that same textedit in vscode-java 1.0

@jdneo
Copy link
Contributor Author

jdneo commented Nov 17, 2021

@fbricon

Make sure the compilation unit does not have any import statement at first. and then write them like this:

import.mp4

@fbricon
Copy link
Contributor

fbricon commented Nov 17, 2021

still nope

Screen.Recording.2021-11-17.at.12.16.54.mov

@jdneo
Copy link
Contributor Author

jdneo commented Nov 17, 2021

Interesting, is that the power of M1 chip?

What if you hit enter just right after typing ja (and be quick)

@rgrunber
Copy link
Contributor

rgrunber commented Nov 30, 2021

I'm able to reproduce. Some observations :

  • In order to reproduce consistently, I need wait to ensure diagnostics are generated when the import statement contains no tokens to the right, and then select the completion fairly quickly as I type it out
  • If I trigger auto-completion without the document/AST being refreshed, then the issue doesn't occur for me.

import-text-edit-bug

Given the potential relation to the state of the AST, might this be related to #1918 ?

@jdneo
Copy link
Contributor Author

jdneo commented Dec 1, 2021

Thank you @rgrunber! I'll take some more investigation.

@jdneo jdneo marked this pull request as draft December 1, 2021 01:33
@jdneo
Copy link
Contributor Author

jdneo commented Dec 7, 2021

@fbricon Did you set the system property java.lsp.joinOnCompletion to true when you launch the JDT.LS?

@jdneo
Copy link
Contributor Author

jdneo commented Dec 7, 2021

After some investigation, I got some new findings.

In the code CompletionResolveHandler@line:114, if I added unit.makeConsistent(null);, then the issue get resolved.

So it looks like a race condition between the completion resolve request and the validation job in the document lifecycle.

Next step I'll think about how to solve it more 'elegantly'.

@jdneo jdneo marked this pull request as ready for review December 8, 2021 13:54
@jdneo
Copy link
Contributor Author

jdneo commented Dec 8, 2021

Reopen it since this could be a quick solution for this specific problem. But for long-term, will keep looking at the race condition issue.

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.

If I type import ja and trigger completion very fast, i can reproduce the issue. The adaptive debounce with validation can reduce the probability of race condition.

Also since we have already used similar logic to skip import rewriting from within the import section. See computeJavaTypeReplacementString. This PR is doing the same thing for additional import, it makes sense to me.

https://github.com/eclipse/eclipse.jdt.ls/blob/238b06f7632213e3be32dec53ee17d619b8d5a2c/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/contentassist/CompletionProposalReplacementProvider.java#L895-L902

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.

Import packages show the wrong result
4 participants