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

No completion on generic anonymous class instance objects #2508

Merged
merged 1 commit into from Mar 2, 2023

Conversation

snjeza
Copy link
Contributor

@snjeza snjeza commented Feb 28, 2023

Fixes #2505

Copy link
Contributor

@jdneo jdneo left a comment

Choose a reason for hiding this comment

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

What's the error thrown from the postfix side? Can we handle that error in postfix's handlers?

@snjeza
Copy link
Contributor Author

snjeza commented Mar 1, 2023

What's the error thrown from the postfix side?

[Error - 1:38:36 AM] Mar 1, 2023, 1:38:36 AM Problem with codeComplete for file:///home/snjeza/demo/src/main/java/foo/bar/Main.java
slave can only serve one master
java.lang.IllegalArgumentException: slave can only serve one master
	at org.eclipse.jdt.ls.core.internal.corext.template.java.MultiVariableGuess.addDependency(MultiVariableGuess.java:56)
	at org.eclipse.jdt.ls.core.internal.corext.template.java.JavaContext.addDependency(JavaContext.java:76)
	at org.eclipse.jdt.ls.core.internal.corext.template.java.TypeResolver.resolve(TypeResolver.java:54)
	at org.eclipse.jdt.ls.core.internal.corext.template.java.ActualTypeResolver.resolve(ActualTypeResolver.java:87)
	at org.eclipse.jface.text.templates.TemplateContextType.resolve(TemplateContextType.java:276)
	at org.eclipse.jface.text.templates.TemplateContextType.resolve(TemplateContextType.java:243)
	at org.eclipse.jdt.internal.corext.template.java.JavaContextCore.evaluate(JavaContextCore.java:157)
	at org.eclipse.jdt.ls.core.internal.corext.template.java.JavaPostfixContext.evaluate(JavaPostfixContext.java:422)
	at org.eclipse.jdt.ls.core.internal.corext.template.java.PostfixTemplateEngine.evaluateGenericTemplate(PostfixTemplateEngine.java:117)
	at org.eclipse.jdt.ls.core.internal.corext.template.java.PostfixTemplateEngine.complete(PostfixTemplateEngine.java:82)
	at org.eclipse.jdt.ls.core.internal.contentassist.SnippetCompletionProposal.getPostfixSnippets(SnippetCompletionProposal.java:231)
	at org.eclipse.jdt.ls.core.internal.contentassist.SnippetCompletionProposal.getSnippets(SnippetCompletionProposal.java:210)
	at org.eclipse.jdt.ls.core.internal.handlers.CompletionHandler.computeContentAssist(CompletionHandler.java:224)
	at org.eclipse.jdt.ls.core.internal.handlers.CompletionHandler.completion(CompletionHandler.java:93)
	at org.eclipse.jdt.ls.core.internal.handlers.JDTLanguageServer.lambda$5(JDTLanguageServer.java:582)
	at org.eclipse.jdt.ls.core.internal.BaseJDTLanguageServer.lambda$0(BaseJDTLanguageServer.java:87)
	at java.base/java.util.concurrent.CompletableFuture$UniApply.tryFire(Unknown Source)
	at java.base/java.util.concurrent.CompletableFuture$Completion.exec(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinPool.scan(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source)

Can we handle that error in postfix's handlers?

I don't know. The PR logs and ignore an exception in PostfixTemplateEngine.complete - 2965f0b#diff-37c4c6e9f8f50d2e98e4cde549406f090b08b05fb820f1f07883f8991b88c5c0L231

@snjeza snjeza changed the title No completion on generic anonymous class instance objects [WIP] No completion on generic anonymous class instance objects Mar 1, 2023
Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com>
@snjeza snjeza changed the title [WIP] No completion on generic anonymous class instance objects No completion on generic anonymous class instance objects Mar 1, 2023
@snjeza snjeza removed the in progress label Mar 1, 2023
@snjeza
Copy link
Contributor Author

snjeza commented Mar 1, 2023

@jdneo @rgrunber I have updated the PR.

@rgrunber
Copy link
Contributor

rgrunber commented Mar 1, 2023

Looks like this adds some missing code from

eclipse-jdt/eclipse.jdt.ui@36dbc24
eclipse-jdt/eclipse.jdt.ui@b170a2b

How does this fix the illegal argument error though ? I think I was seeing the failure in the "for" postfix template.

@snjeza
Copy link
Contributor Author

snjeza commented Mar 1, 2023

Looks like this adds some missing code from

You are right.

How does this fix the illegal argument error though ?

Java LS cancels the completion on an exception - https://github.com/eclipse/eclipse.jdt.ls/blob/master/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionHandler.java#L98
Now, we log an error and skip a proposal that throws an exception - 525953a#diff-37c4c6e9f8f50d2e98e4cde549406f090b08b05fb820f1f07883f8991b88c5c0L234

I think I was seeing the failure in the "for" postfix template.

Right.

@rgrunber
Copy link
Contributor

rgrunber commented Mar 2, 2023

Ah, I thought those additions were somehow related to fixing this. @jdneo , I'm fine with merging, but it might be good to figure out why this only happens in JDT-LS and not upstream. Either way, I don't think the for template would apply for the original snippet in the issue, so I don't think we're losing any functionality.

@snjeza
Copy link
Contributor Author

snjeza commented Mar 2, 2023

...out why this only happens in JDT-LS and not upstream

JDT UI doesn't cancel the completion if a snippet throws an exception. Now, JDT LS do the same.

JDT UI offers the for template and throws an exception when the user applies it.
2508

!ENTRY org.eclipse.ui 4 0 2023-03-02 02:13:50.678
!MESSAGE Unhandled event loop exception
!STACK 0
java.lang.IllegalArgumentException: slave can only serve one master
	at org.eclipse.jdt.internal.ui.text.template.contentassist.MultiVariableGuess.addDependency(MultiVariableGuess.java:280)
	at org.eclipse.jdt.internal.corext.template.java.JavaContext.addDependency(JavaContext.java:150)
	at org.eclipse.jdt.internal.corext.template.java.TypeResolver.resolve(TypeResolver.java:58)
	at org.eclipse.jdt.internal.corext.template.java.ActualTypeResolver.resolve(ActualTypeResolver.java:90)
	at org.eclipse.jface.text.templates.TemplateContextType.resolve(TemplateContextType.java:276)
	at org.eclipse.jface.text.templates.TemplateContextType.resolve(TemplateContextType.java:243)
	at org.eclipse.jdt.internal.corext.template.java.JavaContextCore.evaluate(JavaContextCore.java:157)
	at org.eclipse.jdt.internal.corext.template.java.JavaContext.evaluate(JavaContext.java:107)
	at org.eclipse.jdt.internal.corext.template.java.JavaPostfixContext.evaluate(JavaPostfixContext.java:412)
	at org.eclipse.jdt.internal.ui.text.template.contentassist.TemplateProposal.getAdditionalProposalInfo(TemplateProposal.java:447)
	at org.eclipse.jface.text.contentassist.AdditionalInfoController$Timer$4.lambda$0(AdditionalInfoController.java:183)
	at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:40)
	at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:132)
	at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:5040)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:4520)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$5.run(PartRenderingEngine.java:1155)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:338)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1046)
	at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:155)
	at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:643)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:338)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:550)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:171)
	at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:152)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:203)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:136)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:104)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:402)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:255)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:659)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:596)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1467)
	at org.eclipse.equinox.launcher.Main.main(Main.java:1440)

@snjeza
Copy link
Contributor Author

snjeza commented Mar 2, 2023

Ah, I thought those additions were somehow related to fixing this...

I have added them just in case. The fix is at 525953a#diff-37c4c6e9f8f50d2e98e4cde549406f090b08b05fb820f1f07883f8991b88c5c0L234

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.

@gayanper , @snjeza , so then eclipse-jdt/eclipse.jdt.core#802 should be re-opened with respect to the stacktrace ?

@jdneo
Copy link
Contributor

jdneo commented Mar 2, 2023

One question: If jdt.ui also has the same behavior. Do we need to include those checks about the new templates? (switch, record)

Currently we don't have those postfix template.

@snjeza
Copy link
Contributor Author

snjeza commented Mar 2, 2023

Do we need to include those checks about the new templates? (switch, record)

@jdneo No, we don't need them. See #2508 (comment)

@snjeza
Copy link
Contributor Author

snjeza commented Mar 2, 2023

so then eclipse-jdt/eclipse.jdt.core#802 should be re-opened with respect to the stacktrace ?

cc @gayanper

@snjeza snjeza merged commit 4898e8b into eclipse-jdtls:master Mar 2, 2023
4 checks passed
@gayanper
Copy link
Contributor

gayanper commented Mar 2, 2023

@snjeza can you add these details you found out in the jdt issue and open it. I can have a look at it.

@snjeza
Copy link
Contributor Author

snjeza commented Mar 2, 2023

@gayanper I have added eclipse-jdt/eclipse.jdt.core#802 (comment)
I can't reopen eclipse-jdt/eclipse.jdt.core#802

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.

No completion on generic anonymous class instance objects
4 participants