From fed8402f44e6ac29561e2e9040edf74559cc8fb3 Mon Sep 17 00:00:00 2001 From: Gayan Perera Date: Tue, 4 Apr 2023 21:10:52 +0200 Subject: [PATCH 1/6] Improve lambda method discovery Fixes: #477 --- .../internal/JdtSourceLookUpProvider.java | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/plugin/internal/JdtSourceLookUpProvider.java b/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/plugin/internal/JdtSourceLookUpProvider.java index c4d4fe318..e2e21e720 100644 --- a/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/plugin/internal/JdtSourceLookUpProvider.java +++ b/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/plugin/internal/JdtSourceLookUpProvider.java @@ -29,8 +29,8 @@ import java.util.jar.Manifest; import java.util.logging.Level; import java.util.logging.Logger; -import java.util.stream.Stream; import java.util.stream.Collectors; +import java.util.stream.Stream; import org.eclipse.core.resources.IResource; import org.eclipse.core.runtime.CoreException; @@ -50,10 +50,10 @@ import org.eclipse.jdt.core.dom.ASTParser; import org.eclipse.jdt.core.dom.ASTVisitor; import org.eclipse.jdt.core.dom.CompilationUnit; -import org.eclipse.jdt.core.dom.LambdaExpression; -import org.eclipse.jdt.core.manipulation.CoreASTProvider; import org.eclipse.jdt.core.dom.IMethodBinding; import org.eclipse.jdt.core.dom.ITypeBinding; +import org.eclipse.jdt.core.dom.LambdaExpression; +import org.eclipse.jdt.core.manipulation.CoreASTProvider; import org.eclipse.jdt.internal.core.JarPackageFragmentRoot; import org.eclipse.jdt.launching.IVMInstall; import org.eclipse.jdt.launching.JavaRuntime; @@ -157,7 +157,20 @@ public JavaBreakpointLocation[] getBreakpointLocations(String sourceUri, SourceB return new JavaBreakpointLocation[0]; } - CompilationUnit astUnit = asCompilationUnit(sourceUri); + boolean useCache = false; + CompilationUnit cachedUnit = CoreASTProvider.getInstance().getCachedAST(); + if (cachedUnit != null) { + ITypeRoot cachedElement = cachedUnit.getTypeRoot(); + if (cachedElement != null && isSameURI(JDTUtils.toUri(cachedElement), sourceUri)) { + useCache = true; + } + } + + final CompilationUnit astUnit = useCache ? cachedUnit : asCompilationUnit(sourceUri); + if (astUnit == null) { + return new JavaBreakpointLocation[0]; + } + JavaBreakpointLocation[] sourceLocations = Stream.of(sourceBreakpoints) .map(sourceBreakpoint -> new JavaBreakpointLocation(sourceBreakpoint.line, sourceBreakpoint.column)) .toArray(JavaBreakpointLocation[]::new); From 453fa03313a5a02806c1c2a0fd6f306a1db926f4 Mon Sep 17 00:00:00 2001 From: Gayan Perera Date: Mon, 17 Apr 2023 19:48:39 +0200 Subject: [PATCH 2/6] Improve the solution - Use classfile and compilationunit resolution as the first method before fallback to old strategy. - Switch to using jdt.debug lambda helper which resolves lambda method signature with outer local variables. --- .../microsoft/java/debug/BindingUtils.java | 41 ++++------------ .../java/debug/BreakpointLocationLocator.java | 2 +- .../java/debug/LambdaExpressionLocator.java | 2 +- .../internal/JdtSourceLookUpProvider.java | 49 ++++++++++++------- 4 files changed, 42 insertions(+), 52 deletions(-) diff --git a/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/BindingUtils.java b/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/BindingUtils.java index b696be049..1508bb7e7 100644 --- a/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/BindingUtils.java +++ b/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/BindingUtils.java @@ -12,10 +12,12 @@ package com.microsoft.java.debug; import org.eclipse.jdt.core.dom.IMethodBinding; +import org.eclipse.jdt.internal.debug.core.breakpoints.LambdaLocationLocatorHelper; /** * Utility methods around working with JDT Bindings. */ +@SuppressWarnings("restriction") public final class BindingUtils { private BindingUtils() { @@ -33,46 +35,21 @@ private BindingUtils() { */ public static String getMethodName(IMethodBinding binding, boolean fromKey) { if (fromKey) { - String key = binding.getKey(); - int dotAt = key.indexOf('.'); - int end = key.indexOf('<', dotAt); - if (end == -1) { - end = key.indexOf('('); - } else { - end = Math.min(end, key.indexOf('(')); - } - return key.substring(dotAt + 1, end); + return LambdaLocationLocatorHelper.toMethodName(binding); } else { return binding.getName(); } } /** - * Returns the method signature of the method represented by the binding. Since - * this implementation use the {@link IMethodBinding#getKey()} to extract the - * signature from, the method name must be passed in. + * Returns the method signature of the method represented by the binding + * including the synthetic outer locals. * * @param binding the binding which the signature must be resolved for. - * @param name the name of the method. - * @return the signature or null if the signature could not be resolved from the - * key. + * @return the signature or null if the signature could not be resolved. */ - public static String toSignature(IMethodBinding binding, String name) { - // use key for now until JDT core provides a public API for this. - // "Ljava/util/Arrays;.asList([TT;)Ljava/util/List;" - // "([Ljava/lang/String;)V|Ljava/lang/InterruptedException;" - String signatureString = binding.getKey(); - if (signatureString != null) { - name = "." + name; - int index = signatureString.indexOf(name); - if (index > -1) { - int exceptionIndex = signatureString.indexOf("|", signatureString.lastIndexOf(")")); - if (exceptionIndex > -1) { - return signatureString.substring(index + name.length(), exceptionIndex); - } - return signatureString.substring(index + name.length()); - } - } - return null; + public static String toSignature(IMethodBinding binding) { + return LambdaLocationLocatorHelper.toMethodSignature(binding); } + } diff --git a/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/BreakpointLocationLocator.java b/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/BreakpointLocationLocator.java index 68f593aff..b14afde83 100644 --- a/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/BreakpointLocationLocator.java +++ b/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/BreakpointLocationLocator.java @@ -46,7 +46,7 @@ public String getMethodSignature() { if (this.methodBinding == null) { return null; } - return BindingUtils.toSignature(this.methodBinding, getMethodName()); + return BindingUtils.toSignature(this.methodBinding); } /** diff --git a/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/LambdaExpressionLocator.java b/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/LambdaExpressionLocator.java index afffd9741..a5456809a 100644 --- a/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/LambdaExpressionLocator.java +++ b/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/LambdaExpressionLocator.java @@ -71,7 +71,7 @@ public String getMethodSignature() { if (!this.found) { return null; } - return BindingUtils.toSignature(this.lambdaMethodBinding, getMethodName()); + return BindingUtils.toSignature(this.lambdaMethodBinding); } /** diff --git a/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/plugin/internal/JdtSourceLookUpProvider.java b/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/plugin/internal/JdtSourceLookUpProvider.java index e2e21e720..3235b5eca 100644 --- a/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/plugin/internal/JdtSourceLookUpProvider.java +++ b/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/plugin/internal/JdtSourceLookUpProvider.java @@ -38,6 +38,7 @@ import org.eclipse.debug.core.sourcelookup.ISourceContainer; import org.eclipse.jdt.core.IBuffer; import org.eclipse.jdt.core.IClassFile; +import org.eclipse.jdt.core.ICompilationUnit; import org.eclipse.jdt.core.IJavaElement; import org.eclipse.jdt.core.IJavaProject; import org.eclipse.jdt.core.IPackageFragmentRoot; @@ -157,20 +158,7 @@ public JavaBreakpointLocation[] getBreakpointLocations(String sourceUri, SourceB return new JavaBreakpointLocation[0]; } - boolean useCache = false; - CompilationUnit cachedUnit = CoreASTProvider.getInstance().getCachedAST(); - if (cachedUnit != null) { - ITypeRoot cachedElement = cachedUnit.getTypeRoot(); - if (cachedElement != null && isSameURI(JDTUtils.toUri(cachedElement), sourceUri)) { - useCache = true; - } - } - - final CompilationUnit astUnit = useCache ? cachedUnit : asCompilationUnit(sourceUri); - if (astUnit == null) { - return new JavaBreakpointLocation[0]; - } - + CompilationUnit astUnit = asCompilationUnit(sourceUri); JavaBreakpointLocation[] sourceLocations = Stream.of(sourceBreakpoints) .map(sourceBreakpoint -> new JavaBreakpointLocation(sourceBreakpoint.line, sourceBreakpoint.column)) .toArray(JavaBreakpointLocation[]::new); @@ -270,7 +258,32 @@ private CompilationUnit asCompilationUnit(String uri) { parser.setResolveBindings(true); parser.setBindingsRecovery(true); parser.setStatementsRecovery(true); + + // try resolving compilation util or class file based on file extension from + // jdt.ls + boolean uriIsClassFile = uri.endsWith(".class"); + boolean needFallbackResolution = true; + if (uriIsClassFile) { + IClassFile resolveClassFile = JDTUtils.resolveClassFile(uri); + if (resolveClassFile != null) { + parser.setSource(resolveClassFile); + needFallbackResolution = false; + } + } else { + ICompilationUnit resolveCompilationUnit = JDTUtils.resolveCompilationUnit(uri); + if (resolveCompilationUnit != null) { + parser.setSource(resolveCompilationUnit); + needFallbackResolution = false; + } + } CompilationUnit astUnit = null; + if ((needFallbackResolution && configureFallbackResolution(uri, parser)) || !needFallbackResolution) { + astUnit = (CompilationUnit) parser.createAST(null); + } + return astUnit; + } + + private boolean configureFallbackResolution(String uri, final ASTParser parser) { String filePath = AdapterUtils.toPath(uri); // For file uri, read the file contents directly and pass them to the ast // parser. @@ -301,17 +314,17 @@ private CompilationUnit asCompilationUnit(String uri) { javaOptions.put(JavaCore.COMPILER_COMPLIANCE, this.latestJavaVersion); javaOptions.put(JavaCore.COMPILER_PB_ENABLE_PREVIEW_FEATURES, JavaCore.ENABLED); parser.setCompilerOptions(javaOptions); - astUnit = (CompilationUnit) parser.createAST(null); + return true; } else { // For non-file uri (e.g. jdt://contents/rt.jar/java.io/PrintStream.class), // leverage jdt to load the source contents. ITypeRoot typeRoot = resolveClassFile(uri); if (typeRoot != null) { parser.setSource(typeRoot); - astUnit = (CompilationUnit) parser.createAST(null); + return true; } } - return astUnit; + return false; } @Override @@ -505,7 +518,7 @@ public List findMethodInvocations(String uri, int line) { // Keep consistent with JDI since JDI uses binary class name invocation.declaringTypeName = binding.getDeclaringClass().getBinaryName(); } - invocation.methodGenericSignature = BindingUtils.toSignature(binding, BindingUtils.getMethodName(binding, true)); + invocation.methodGenericSignature = BindingUtils.toSignature(binding); invocation.methodSignature = Signature.getTypeErasure(invocation.methodGenericSignature); int startOffset = astNode.getStartPosition(); if (astNode instanceof org.eclipse.jdt.core.dom.MethodInvocation) { From a06f28b086211dfce5462d9f5eeb3d8b16512edb Mon Sep 17 00:00:00 2001 From: Gayan Perera Date: Mon, 22 May 2023 18:39:55 +0200 Subject: [PATCH 3/6] improve fix with suggestions --- .../internal/JdtSourceLookUpProvider.java | 44 ++++++------------- 1 file changed, 13 insertions(+), 31 deletions(-) diff --git a/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/plugin/internal/JdtSourceLookUpProvider.java b/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/plugin/internal/JdtSourceLookUpProvider.java index 3235b5eca..2165ebdc8 100644 --- a/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/plugin/internal/JdtSourceLookUpProvider.java +++ b/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/plugin/internal/JdtSourceLookUpProvider.java @@ -32,13 +32,14 @@ import java.util.stream.Collectors; import java.util.stream.Stream; +import org.eclipse.core.resources.IFile; import org.eclipse.core.resources.IResource; +import org.eclipse.core.resources.ResourcesPlugin; import org.eclipse.core.runtime.CoreException; import org.eclipse.core.runtime.URIUtil; import org.eclipse.debug.core.sourcelookup.ISourceContainer; import org.eclipse.jdt.core.IBuffer; import org.eclipse.jdt.core.IClassFile; -import org.eclipse.jdt.core.ICompilationUnit; import org.eclipse.jdt.core.IJavaElement; import org.eclipse.jdt.core.IJavaProject; import org.eclipse.jdt.core.IPackageFragmentRoot; @@ -258,32 +259,7 @@ private CompilationUnit asCompilationUnit(String uri) { parser.setResolveBindings(true); parser.setBindingsRecovery(true); parser.setStatementsRecovery(true); - - // try resolving compilation util or class file based on file extension from - // jdt.ls - boolean uriIsClassFile = uri.endsWith(".class"); - boolean needFallbackResolution = true; - if (uriIsClassFile) { - IClassFile resolveClassFile = JDTUtils.resolveClassFile(uri); - if (resolveClassFile != null) { - parser.setSource(resolveClassFile); - needFallbackResolution = false; - } - } else { - ICompilationUnit resolveCompilationUnit = JDTUtils.resolveCompilationUnit(uri); - if (resolveCompilationUnit != null) { - parser.setSource(resolveCompilationUnit); - needFallbackResolution = false; - } - } CompilationUnit astUnit = null; - if ((needFallbackResolution && configureFallbackResolution(uri, parser)) || !needFallbackResolution) { - astUnit = (CompilationUnit) parser.createAST(null); - } - return astUnit; - } - - private boolean configureFallbackResolution(String uri, final ASTParser parser) { String filePath = AdapterUtils.toPath(uri); // For file uri, read the file contents directly and pass them to the ast // parser. @@ -302,8 +278,14 @@ private boolean configureFallbackResolution(String uri, final ASTParser parser) * setEnvironment(String [], String [], String [], boolean) * and a unit name setUnitName(String). */ - parser.setEnvironment(new String[0], new String[0], null, true); - parser.setUnitName(Paths.get(filePath).getFileName().toString()); + IFile resource = (IFile) JDTUtils.findResource(JDTUtils.toURI(uri), + ResourcesPlugin.getWorkspace().getRoot()::findFilesForLocationURI); + if (resource != null && JdtUtils.isJavaProject(resource.getProject())) { + parser.setProject(JavaCore.create(resource.getProject())); + } else { + parser.setEnvironment(new String[0], new String[0], null, true); + parser.setUnitName(Paths.get(filePath).getFileName().toString()); + } /** * See the java doc for { @link ASTParser#setSource(char[]) }, * the user need specify the compiler options explicitly. @@ -314,17 +296,17 @@ private boolean configureFallbackResolution(String uri, final ASTParser parser) javaOptions.put(JavaCore.COMPILER_COMPLIANCE, this.latestJavaVersion); javaOptions.put(JavaCore.COMPILER_PB_ENABLE_PREVIEW_FEATURES, JavaCore.ENABLED); parser.setCompilerOptions(javaOptions); - return true; + astUnit = (CompilationUnit) parser.createAST(null); } else { // For non-file uri (e.g. jdt://contents/rt.jar/java.io/PrintStream.class), // leverage jdt to load the source contents. ITypeRoot typeRoot = resolveClassFile(uri); if (typeRoot != null) { parser.setSource(typeRoot); - return true; + astUnit = (CompilationUnit) parser.createAST(null); } } - return false; + return astUnit; } @Override From 7d7546ae4a9f8b5799a9518aba86437df46fd80c Mon Sep 17 00:00:00 2001 From: Gayan Perera Date: Tue, 23 May 2023 05:49:00 +0200 Subject: [PATCH 4/6] Move unit name for both scenarios --- .../java/debug/plugin/internal/JdtSourceLookUpProvider.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/plugin/internal/JdtSourceLookUpProvider.java b/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/plugin/internal/JdtSourceLookUpProvider.java index 2165ebdc8..bb518ed34 100644 --- a/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/plugin/internal/JdtSourceLookUpProvider.java +++ b/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/plugin/internal/JdtSourceLookUpProvider.java @@ -284,8 +284,9 @@ private CompilationUnit asCompilationUnit(String uri) { parser.setProject(JavaCore.create(resource.getProject())); } else { parser.setEnvironment(new String[0], new String[0], null, true); - parser.setUnitName(Paths.get(filePath).getFileName().toString()); } + parser.setUnitName(Paths.get(filePath).getFileName().toString()); + /** * See the java doc for { @link ASTParser#setSource(char[]) }, * the user need specify the compiler options explicitly. From 04fd45c531adba556cea072f7be06a22b85928b5 Mon Sep 17 00:00:00 2001 From: Gayan Perera Date: Tue, 23 May 2023 05:49:50 +0200 Subject: [PATCH 5/6] Fix format --- .../java/debug/plugin/internal/JdtSourceLookUpProvider.java | 1 - 1 file changed, 1 deletion(-) diff --git a/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/plugin/internal/JdtSourceLookUpProvider.java b/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/plugin/internal/JdtSourceLookUpProvider.java index bb518ed34..6d42572e4 100644 --- a/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/plugin/internal/JdtSourceLookUpProvider.java +++ b/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/plugin/internal/JdtSourceLookUpProvider.java @@ -286,7 +286,6 @@ private CompilationUnit asCompilationUnit(String uri) { parser.setEnvironment(new String[0], new String[0], null, true); } parser.setUnitName(Paths.get(filePath).getFileName().toString()); - /** * See the java doc for { @link ASTParser#setSource(char[]) }, * the user need specify the compiler options explicitly. From de8b81109c459aff7b9587a4eae9b848c97b2ab3 Mon Sep 17 00:00:00 2001 From: Gayan Perera Date: Tue, 23 May 2023 19:38:01 +0200 Subject: [PATCH 6/6] revert back the method name logic since its better than JDT --- .../java/com/microsoft/java/debug/BindingUtils.java | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/BindingUtils.java b/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/BindingUtils.java index 1508bb7e7..085bf0d4c 100644 --- a/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/BindingUtils.java +++ b/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/BindingUtils.java @@ -35,7 +35,15 @@ private BindingUtils() { */ public static String getMethodName(IMethodBinding binding, boolean fromKey) { if (fromKey) { - return LambdaLocationLocatorHelper.toMethodName(binding); + String key = binding.getKey(); + int dotAt = key.indexOf('.'); + int end = key.indexOf('<', dotAt); + if (end == -1) { + end = key.indexOf('('); + } else { + end = Math.min(end, key.indexOf('(')); + } + return key.substring(dotAt + 1, end); } else { return binding.getName(); }