search for references from IClassFile without source #1632

snjeza · 2020-12-15T20:16:22Z

Fixes redhat-developer/vscode-java#1665
Fixes #1634
Requires redhat-developer/vscode-java#1773

Signed-off-by: Snjezana Peco [email protected]

testforstephen · 2020-12-16T03:33:27Z

@snjeza Would you mind sharing the specific root cause to help us understand this fix?

snjeza · 2020-12-16T17:21:44Z

@snjeza Would you mind sharing the specific root cause to help us understand this fix?

There are several issues:

JDTUtils.findElementAtSelection(...) can't find an element in a class file
ReferencesHandler.findReferences(...) doesn't define the location of an element in a class file.
A similar issue also exists in the NavigateToDefinitionHandler class. If you call the "Go To Definition" action for an element without a source, Java LS will always open a class file at the (0,0). location.
I will create a separate issue for this.
Sometimes, Eclipse doesn't find all references in binary files without a source, but that is an upstream issue.

The PR improves both JDTUtils.findElementAtSelection() and ReferencesHandler.findReferences

rgrunber · 2021-01-04T19:33:13Z

Not the first time we've had issues with finding references (eg. https://github.com/eclipse/eclipse.jdt.ls/pull/1561/files#r501326693)

rgrunber · 2021-01-05T02:20:07Z

Some initial thoughts.

The code in ReferencesHandler seems to resolve the issue with finding references, so I'll take a closer look there. Calling AST parsing on the classfile contents of every match could be intensive but I don't see a way around it. Would this still work with setResolveBindings/setBindingsRecovery set to false ?

The additions to JDTUtils#findElementsAtSelection(..) seem to produce better resolutions (eg. hovers) for various tokens, which aren't that informative (in the example i'm using), but better than nothing. Instead of the search engine just looking for type declarations in classfiles, it can now find other types of elements as well.

rgrunber

Overall, looks good to me. Just a small refactoring of the visit(..) boolean check, and maybe avoiding binding resolution if possible.

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/ReferencesHandler.java

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/JDTUtils.java

snjeza · 2021-01-14T00:15:43Z

test this please

snjeza · 2021-01-14T01:00:55Z

@rgrunber I have updated the PR.

rgrunber · 2021-01-14T14:29:32Z

Great! @testforstephen , let me know if there's anything else you'd like to address here. I'll likely be merging this before the week is up.

testforstephen · 2021-01-14T15:01:23Z

Performance is my concern. Based on the results of profiling the code action handler I did before, findElementsAtSelection is not a cheap operation. Since code action handler is a high frequency operation, I don't want the new modifications to cause it to slow down.

Let me take a look at the performance side.

testforstephen

The code in ReferencesHandler seems to resolve the issue with finding references, so I'll take a closer look there. Calling AST parsing on the classfile contents of every match could be intensive but I don't see a way around it. Would this still work with setResolveBindings/setBindingsRecovery set to false ?

Yes, AST parser is expensive. An idea is to add a preference to control whether to include the decompiled sources when finding references.

The additions to JDTUtils#findElementsAtSelection(..) seem to produce better resolutions (eg. hovers) for various tokens, which aren't that informative (in the example i'm using), but better than nothing. Instead of the search engine just looking for type declarations in classfiles, it can now find other types of elements as well.

The search engine will look up for more pattern, it will take more time to return. But i don't see it will affect the code action performance yet. Because SourceAssistProcessor.java will preferentially use the unit.codeSelect branch of findElementsAtSelection method to find the currently selected element.

However, this pull request didn't fix the issue you mentioned at Issue with 'Go To Definition' #1634. 'Go to definition' still stops at 0,0 of the disassembled source contents of the classfile. We need use the same logic to convert position to the location of it's disassembled source. I'm ok to fix go to definition by using a new PR.

go-to-definition.mov

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/ReferencesHandler.java

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/JDTUtils.java

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/ReferencesHandler.java

snjeza · 2021-02-01T18:16:35Z

@testforstephen @rgrunber I have updated the PR.

testforstephen · 2021-02-03T07:39:27Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/JDTUtils.java

+		return location;
+	}
+
+	public static ICompilationUnit getWorkingCopy(IClassFile classFile, String contents, IProgressMonitor monitor) throws JavaModelException {


Why not to use ITypeRoot.getWorkingCopy() method directly? What's the difference between this one and ITypeRoot.getWorkingCopy()?

We associate a decompiled source with a class file and configure a ICompilationUnit.

But in your new findElementAtSelection() method, you're using ClassFile.getWorkingCopy() to attach a decompile source, it looks more concise to me.

ICompilationUnit workingCopy = classFile.getWorkingCopy(new WorkingCopyOwner() { }, monitor); workingCopy.getBuffer().setContents(contents); workingCopy.becomeWorkingCopy(monitor); workingCopy.makeConsistent(monitor);

testforstephen · 2021-02-03T07:51:56Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/HoverInfoProvider.java

 		try {
 			if (monitor.isCanceled()) {
 				return cancelled(res);
 			}
-			IJavaElement[] elements = JDTUtils.findElementsAtSelection(unit, line, column, this.preferenceManager, monitor);
+			ITypeRoot typeRoot = unit;
+			if (unit instanceof IClassFile && preferenceManager.getPreferences().isIncludeDecompiledSources()) {


i would add check to bypass those class file with source jar attached to avoid creating workingCopy for them.

testforstephen · 2021-02-03T08:34:55Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/JDTUtils.java

+						String name = contents.substring(region.getOffset(), region.getOffset() + region.getLength());
+						List<IJavaElement> elements = search(unit, name, element, preferenceManager, monitor);
+						if (monitor != null && monitor.isCanceled()) {
+							return null;
+						}
+						if (elements.isEmpty()) {
+							elements = search(unit.getJavaProject(), name, element, preferenceManager, monitor);
+						}


The codeSelect above already returns a JavaElement, it it possible to make other places to use this result directly? I didn't get the point to use search engine to search the element again.

The codeselect finds a java element in a ICompilationUnit we have created using JDTUtils.getWorkingCopy(). We can't return a java element from ICompilationUnit which is created temporarily and destroyed. We have to search a class file.

We can't return a java element from ICompilationUnit which is created temporarily and destroyed.

If this is the only concern, then you can have the outermost caller to create the workingcopy, and destroy the workingcopy until the LSP response returns.

We need to search three times to find the currently selected element, which seems a bit too complicated.

I have updated the PR. Could, you, please, review it.

snjeza · 2021-02-06T18:40:16Z

@rgrunber @testforstephen I have updated the PR. It also fixes #1634.

testforstephen

I tried the latest commit, it works better than before. But i find it's not pretty stable with go-to-definition. For example, in the decompiled StringUtils library, sometimes go to definition can jump to the right position of the decompiled source, sometimes not.

gtd.mov

BTW, i will be off for two weeks. @rgrunber, you can merge it if you test it to be OK.

testforstephen · 2021-02-07T07:34:04Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/JDTUtils.java

+	 *
+	 * @param uri
+	 * @param returnCompilationUnit
+	 * @param monitor2


monitor2 -> monitor

testforstephen · 2021-02-07T07:40:06Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/JDTUtils.java

 		}
 		return resolveCompilationUnit(uri);
 	}

+	public static void discardWorkingCopy(ITypeRoot unit) {


how about naming it as discardClassFileWorkingCopy?

testforstephen · 2021-02-07T07:45:54Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/JDTUtils.java

@@ -937,6 +1100,20 @@ public static IJavaSearchScope createSearchScope(IJavaProject project, Preferenc
 		return SearchEngine.createJavaSearchScope(elements, scope);
 	}

+	public static IJavaSearchScope createSearchScope(IJavaElement element, PreferenceManager preferenceManager) {


I just searched the code and this new helper method is unused. You can remove it.

I have removed it.

testforstephen · 2021-02-07T07:50:05Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/JDTUtils.java

+			ICompilationUnit workingCopy = null;
+			workingCopy = getWorkingCopy(classFile, contents, monitor);


merge them to one line.

ICompilationUnit workingCopy = getWorkingCopy(classFile, contents, monitor);

snjeza · 2021-02-07T19:07:25Z

I tried the latest commit, it works better than before. But i find it's not pretty stable with go-to-definition. For example, in the decompiled StringUtils library, sometimes go to definition can jump to the right position of the decompiled source, sometimes not.

I have updated the PR.
Sometimes a decompiler throws an exception like the following:

java.lang.NullPointerException
	at org.jetbrains.java.decompiler.util.VBStyleCollection.removeWithKey(VBStyleCollection.java:84)
	at org.jetbrains.java.decompiler.modules.decompiler.stats.Statement.collapseNodesToStatement(Statement.java:209)
	at org.jetbrains.java.decompiler.modules.decompiler.DomHelper.findSimpleStatements(DomHelper.java:600)
	at org.jetbrains.java.decompiler.modules.decompiler.DomHelper.processStatement(DomHelper.java:356)
	at org.jetbrains.java.decompiler.modules.decompiler.DomHelper.processStatement(DomHelper.java:305)
	at org.jetbrains.java.decompiler.modules.decompiler.DomHelper.parseGraph(DomHelper.java:196)
	at org.jetbrains.java.decompiler.main.rels.MethodProcessorRunnable.codeToJava(MethodProcessorRunnable.java:101)
	at org.jetbrains.java.decompiler.main.rels.ClassWrapper.init(ClassWrapper.java:62)
	at org.jetbrains.java.decompiler.main.ClassesProcessor.initWrappers(ClassesProcessor.java:280)
	at org.jetbrains.java.decompiler.main.ClassesProcessor.writeClass(ClassesProcessor.java:227)
	at org.jetbrains.java.decompiler.main.Fernflower.getClassContent(Fernflower.java:90)
	at org.jetbrains.java.decompiler.struct.ContextUnit.save(ContextUnit.java:97)
	at org.jetbrains.java.decompiler.struct.StructContext.saveContext(StructContext.java:58)
	at org.jetbrains.java.decompiler.main.Fernflower.decompileContext(Fernflower.java:47)
	at org.jetbrains.java.decompiler.main.decompiler.BaseDecompiler.decompileContext(BaseDecompiler.java:26)
	at dg.jdt.ls.decompiler.fernflower.FernflowerDecompiler.getContent(FernflowerDecompiler.java:82)
	at dg.jdt.ls.decompiler.fernflower.FernflowerDecompiler.decompileContent(FernflowerDecompiler.java:73)
	at dg.jdt.ls.decompiler.common.CachingDecompiler.getSource(CachingDecompiler.java:47)
	at org.eclipse.jdt.ls.core.internal.managers.ContentProviderManager.getContent(ContentProviderManager.java:112)
	at org.eclipse.jdt.ls.core.internal.managers.ContentProviderManager.getSource(ContentProviderManager.java:65)
	at org.eclipse.jdt.ls.core.internal.JDTUtils.searchDecompiledSources(JDTUtils.java:1577)
	at org.eclipse.jdt.ls.core.internal.handlers.NavigateToDefinitionHandler.computeDefinitionNavigation(NavigateToDefinitionHandler.java:187)
	at org.eclipse.jdt.ls.core.internal.handlers.NavigateToDefinitionHandler.computeDefinitionNavigation(NavigateToDefinitionHandler.java:90)
	at org.eclipse.jdt.ls.core.internal.handlers.NavigateToDefinitionHandler.definition(NavigateToDefinitionHandler.java:73)
	at org.eclipse.jdt.ls.core.internal.handlers.JDTLanguageServer.lambda$8(JDTLanguageServer.java:574)
	at org.eclipse.jdt.ls.core.internal.BaseJDTLanguageServer.lambda$0(BaseJDTLanguageServer.java:75)
	at java.base/java.util.concurrent.CompletableFuture$UniApply.tryFire(CompletableFuture.java:642)
	at java.base/java.util.concurrent.CompletableFuture$Completion.exec(CompletableFuture.java:479)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1020)
	at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1656)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1594)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:183)

Could you try again?

rgrunber · 2021-02-09T00:23:21Z

I think this is working pretty well. The case I've been testing from before seem to be working. Occasionally I got the following stacktrace when selecting the StringWriter type, from line 291 in Gson.class, from the eclipse-sample project :

null
org.eclipse.jface.text.BadLocationException
    at org.eclipse.jface.text.ListLineTracker.getLineOffset(ListLineTracker.java:197)
    at org.eclipse.jface.text.AbstractLineTracker.getLineOffset(AbstractLineTracker.java:161)
    at org.eclipse.jface.text.AbstractDocument.getLineOffset(AbstractDocument.java:877)
    at org.eclipse.jdt.ls.core.internal.handlers.JsonRpcHelpers.toOffset(JsonRpcHelpers.java:85)
    at org.eclipse.jdt.ls.core.internal.handlers.JsonRpcHelpers.toOffset(JsonRpcHelpers.java:69)
    at org.eclipse.jdt.ls.core.internal.handlers.NavigateToDefinitionHandler.computeBreakContinue(NavigateToDefinitionHandler.java:99)
    at org.eclipse.jdt.ls.core.internal.handlers.NavigateToDefinitionHandler.computeDefinitionNavigation(NavigateToDefinitionHandler.java:88)
    at org.eclipse.jdt.ls.core.internal.handlers.NavigateToDefinitionHandler.definition(NavigateToDefinitionHandler.java:73)
    at org.eclipse.jdt.ls.core.internal.handlers.JDTLanguageServer.lambda$8(JDTLanguageServer.java:574)
    at org.eclipse.jdt.ls.core.internal.BaseJDTLanguageServer.lambda$0(BaseJDTLanguageServer.java:75)
    at java.base/java.util.concurrent.CompletableFuture$UniApply.tryFire(CompletableFuture.java:642)
    at java.base/java.util.concurrent.CompletableFuture$Completion.exec(CompletableFuture.java:479)
    at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290)
    at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1016)
    at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1665)
    at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1598)
    at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:183)

It seemed to only happen the first few attempts.

Otherwise, I'll probably have a look at the new additions, but I think this is in better shape.

snjeza · 2021-02-09T01:27:31Z

I think this is working pretty well. The case I've been testing from before seem to be working. Occasionally I got the following stacktrace when selecting the StringWriter type, from line 291 in Gson.class, from the eclipse-sample project :

@rgrunber It is an issue related to the fernflower decompiler. Could you, please, test the latest PR? - aec27b7

rgrunber

Overall, this change works. I think it's in better shape than before. I still had some cases where go-to-definition would fail the first or 2nd time (eg. From within Gson.class on StringWriter with the java sources installed, "No definition found") but maybe we can address these issues separately as this patch is getting large.

rgrunber · 2021-02-09T18:35:00Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/JDTUtils.java

 		}
 		return resolveCompilationUnit(uri);
 	}

+	public static void discardClassFileWorkingCopy(ITypeRoot unit) {


I don't think unit is ever a ClassFileWorkingCopy for me. Is there a specific case where this should happen ?

you are right. Fixed.

rgrunber

I think we should prepare to merge this after the 0.75.0 release just so that we at least have weeks leading up to 0.76.0 to fix up any regressions that may come up.

I'll still try a few of the use-cases and maybe look at the code more thoroughly, and make suggestions. However, to me the functionality seems pretty solid now, and I think merging + fixing any subsequent issues is the best option.

Signed-off-by: Snjezana Peco <[email protected]>

snjeza · 2021-02-14T14:50:42Z

I still had some cases where go-to-definition would fail the first or 2nd time (eg. From within Gson.class on StringWriter with the java sources installed, "No definition found") but maybe we can address these issues separately as this patch is getting large.

@rgrunber The issue you have faced is an upstream fernflower issue.
I have updated fernflower.jar to the latest version. You can check it at https://github.com/snjeza/vscode-test/raw/master/java-decompiler-0.0.3.vsix.

rgrunber · 2021-02-17T19:56:31Z

I still had some cases where go-to-definition would fail the first or 2nd time (eg. From within Gson.class on StringWriter with the java sources installed, "No definition found") but maybe we can address these issues separately as this patch is getting large.

@rgrunber The issue you have faced is an upstream fernflower issue.
I have updated fernflower.jar to the latest version. You can check it at https://github.com/snjeza/vscode-test/raw/master/java-decompiler-0.0.3.vsix.

If that's the case, then let's merge this along with its dependent change and we can address any additional issues that might arise in separate changes.

testforstephen · 2021-02-22T02:49:43Z

The issue you have faced is an upstream fernflower issue.
I have updated fernflower.jar to the latest version. You can check it at https://github.com/snjeza/vscode-test/raw/master/java-decompiler-0.0.3.vsix.

@snjeza since you have figured out the cause of this failure, how about sending a PR to Java Decompiler extension?https://github.com/dgileadi/dg.jdt.ls.decompiler/tree/master/dg.jdt.ls.decompiler.fernflower/lib

snjeza · 2021-02-22T22:05:50Z

since you have figured out the cause of this failure, how about sending a PR to Java Decompiler extension?https://github.com/dgileadi/dg.jdt.ls.decompiler/tree/master/dg.jdt.ls.decompiler.fernflower/lib

I will try.

snjeza requested a review from fbricon December 15, 2020 20:16

snjeza assigned gorkem, rgrunber and testforstephen Dec 15, 2020

snjeza force-pushed the issue-1665 branch 2 times, most recently from 319a904 to 90650bb Compare December 15, 2020 21:41

snjeza unassigned gorkem, rgrunber and testforstephen Dec 16, 2020

snjeza requested review from gorkem, rgrunber and testforstephen December 16, 2020 04:34

snjeza mentioned this pull request Dec 16, 2020

Issue with 'Go To Definition' #1634

Closed

rgrunber reviewed Jan 5, 2021

View reviewed changes

snjeza force-pushed the issue-1665 branch 2 times, most recently from 3558bc6 to 9ce4d13 Compare January 14, 2021 00:00

testforstephen reviewed Jan 15, 2021

View reviewed changes

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/ReferencesHandler.java Outdated Show resolved Hide resolved

testforstephen reviewed Jan 15, 2021

View reviewed changes

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/JDTUtils.java Outdated Show resolved Hide resolved

testforstephen changed the title ~~search for enum reference without source~~ search for references from IClassFile without source Jan 15, 2021

testforstephen reviewed Jan 15, 2021

View reviewed changes

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/ReferencesHandler.java Outdated Show resolved Hide resolved

snjeza force-pushed the issue-1665 branch from 9ce4d13 to f60e209 Compare January 18, 2021 20:45

snjeza mentioned this pull request Jan 18, 2021

search for references from IClassFile without source redhat-developer/vscode-java#1773

Merged

snjeza added in progress and removed in progress labels Jan 20, 2021

snjeza force-pushed the issue-1665 branch from 2c977c2 to 894cb03 Compare January 28, 2021 19:24

snjeza added in progress and removed in progress labels Jan 28, 2021

testforstephen mentioned this pull request Feb 1, 2021

Support debugging the decompiled classes microsoft/vscode-java-debug#945

Closed

snjeza force-pushed the issue-1665 branch from 894cb03 to ab42a9c Compare February 1, 2021 17:56

testforstephen reviewed Feb 3, 2021

View reviewed changes

snjeza force-pushed the issue-1665 branch 3 times, most recently from 34aa411 to a7ba70d Compare February 6, 2021 18:34

snjeza removed the in progress label Feb 6, 2021

testforstephen reviewed Feb 7, 2021

View reviewed changes

snjeza force-pushed the issue-1665 branch from a7ba70d to aec27b7 Compare February 7, 2021 19:00

rgrunber reviewed Feb 9, 2021

View reviewed changes

snjeza force-pushed the issue-1665 branch from aec27b7 to 70bf4f5 Compare February 9, 2021 20:12

rgrunber approved these changes Feb 10, 2021

View reviewed changes

search for references from IClassFile without source

14394cb

Signed-off-by: Snjezana Peco <[email protected]>

snjeza force-pushed the issue-1665 branch from 70bf4f5 to 14394cb Compare February 14, 2021 14:41

snjeza merged commit e350670 into eclipse-jdtls:master Feb 17, 2021

rgrunber mentioned this pull request Mar 3, 2021

Add changelog for 0.70.0 #1671

Merged

		ICompilationUnit workingCopy = null;
		workingCopy = getWorkingCopy(classFile, contents, monitor);

search for references from IClassFile without source #1632

search for references from IClassFile without source #1632

Conversation

snjeza commented Dec 15, 2020 • edited Loading

testforstephen commented Dec 16, 2020

snjeza commented Dec 16, 2020

rgrunber commented Jan 4, 2021

rgrunber commented Jan 5, 2021

rgrunber left a comment

Choose a reason for hiding this comment

snjeza commented Jan 14, 2021

snjeza commented Jan 14, 2021

rgrunber commented Jan 14, 2021

testforstephen commented Jan 14, 2021

testforstephen left a comment • edited Loading

Choose a reason for hiding this comment

snjeza commented Feb 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snjeza commented Feb 6, 2021

testforstephen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snjeza commented Feb 7, 2021

rgrunber commented Feb 9, 2021 • edited Loading

snjeza commented Feb 9, 2021 • edited Loading

rgrunber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgrunber left a comment

Choose a reason for hiding this comment

snjeza commented Feb 14, 2021

rgrunber commented Feb 17, 2021

testforstephen commented Feb 22, 2021

snjeza commented Feb 22, 2021

snjeza commented Dec 15, 2020 •

edited

Loading

testforstephen left a comment •

edited

Loading

rgrunber commented Feb 9, 2021 •

edited

Loading

snjeza commented Feb 9, 2021 •

edited

Loading