Skip to content

Commit

Permalink
Fix for #1143: mimic runtime selection algorithm for category methods
Browse files Browse the repository at this point in the history
  • Loading branch information
@eric-milles
eric-milles committed Jul 20, 2020
1 parent 70655e8 commit f822985
Show file tree
Hide file tree
Showing 4 changed files with 90 additions and 36 deletions.
17 changes: 13 additions & 4 deletions ...e.tests.builder/src/org/eclipse/jdt/core/groovy/tests/search/GenericInferencingTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -448,21 +448,30 @@ public void testMapOfList5() {
@Test
public void testMapOfList6() {
String contents =
"Map<String, Map<Integer, List<Date>>> m\n" +
"def xxx = m.get('foo').get(5).get(2)\n";
"Map<Integer, Map<Integer, List<Date>>> m\n" +
"def xxx = m.get(1).get(2).get(3)\n";

assertType(contents, "xxx", "java.util.Date");
}

@Test // GRECLIPSE-941
public void testMapOfList7() {
String contents =
"Map<String, Map<Integer, List<Date>>> m\n" +
"def xxx = m['foo'][5][2]\n";
"Map<Integer, Map<Integer, List<Date>>> m\n" +
"def xxx = m[1][2][3]\n";

assertType(contents, "xxx", "java.util.Date");
}

@Test // GROOVY-9420
public void testMapOfList8() {
String contents =
"Map<String, List<Date>> m\n" +
"def xxx = m['a'][1]\n";

assertType(contents, "xxx", "java.lang.Object"); // TODO: "java.util.Date" requires new getAt DGM
}

@Test
public void testMapOfMaps() {
String contents =
Expand Down
78 changes: 50 additions & 28 deletions base/org.eclipse.jdt.groovy.core/src/org/eclipse/jdt/groovy/search/CategoryTypeLookup.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,11 @@
*/
package org.eclipse.jdt.groovy.search;

import static org.codehaus.groovy.runtime.DefaultGroovyMethods.tail;

import java.util.ArrayList;
import java.util.List;
import java.util.Map;

import org.codehaus.groovy.ast.ClassNode;
import org.codehaus.groovy.ast.MethodNode;
Expand Down Expand Up @@ -93,7 +96,7 @@ public TypeLookupResult lookupType(final Expression node, final VariableScope sc
return null;
}

protected static boolean isCompatibleConstantExpression(final Expression node, final VariableScope scope, final ClassNode selfType) {
private static boolean isCompatibleConstantExpression(final Expression node, final VariableScope scope, final ClassNode selfType) {
if (node instanceof ConstantExpression && !scope.isTopLevel() && !VariableScope.VOID_CLASS_NODE.equals(selfType)) {
org.codehaus.groovy.ast.ASTNode enclosingNode = scope.getEnclosingNode();
if (!(enclosingNode instanceof AttributeExpression || (enclosingNode instanceof MethodPointerExpression &&
Expand All @@ -104,7 +107,7 @@ protected static boolean isCompatibleConstantExpression(final Expression node, f
return false;
}

protected static boolean isCompatibleCategoryMethod(final MethodNode method, final ClassNode firstArgumentType, final VariableScope scope) {
private static boolean isCompatibleCategoryMethod(final MethodNode method, final ClassNode firstArgumentType, final VariableScope scope) {
if (method.isStatic()) {
Parameter[] paramters = method.getParameters();
if (paramters != null && paramters.length > 0) {
Expand All @@ -120,7 +123,7 @@ protected static boolean isCompatibleCategoryMethod(final MethodNode method, fin
return false;
}

protected static boolean isSelfTypeCompatible(final ClassNode source, final ClassNode target) {
private static boolean isSelfTypeCompatible(final ClassNode source, final ClassNode target) {
if (SimpleTypeLookup.isTypeCompatible(source, target) != Boolean.FALSE) {
if (!(VariableScope.CLASS_CLASS_NODE.equals(source) && source.isUsingGenerics()) ||
VariableScope.OBJECT_CLASS_NODE.equals(target) || !target.isUsingGenerics()) {
Expand All @@ -136,15 +139,15 @@ protected static boolean isSelfTypeCompatible(final ClassNode source, final Clas
return false;
}

protected static boolean isDefaultGroovyMethod(final MethodNode method, final VariableScope scope) {
private static boolean isDefaultGroovyMethod(final MethodNode method, final VariableScope scope) {
return (VariableScope.DGM_CLASS_NODE.equals(method.getDeclaringClass()) || scope.isDefaultCategory(method.getDeclaringClass()));
}

protected static boolean isDefaultGroovyStaticMethod(final MethodNode method, final VariableScope scope) {
private static boolean isDefaultGroovyStaticMethod(final MethodNode method, final VariableScope scope) {
return (VariableScope.DGSM_CLASS_NODE.equals(method.getDeclaringClass()) || scope.isDefaultStaticCategory(method.getDeclaringClass()));
}

protected static MethodNode selectBestMatch(final List<MethodNode> candidates, final List<ClassNode> argumentTypes, final VariableScope scope) {
private static MethodNode selectBestMatch(final List<MethodNode> candidates, final List<ClassNode> argumentTypes, final VariableScope scope) {
java.util.function.Function<MethodNode, List<ClassNode>> argsWithSelfTypeFix;
if (!VariableScope.CLASS_CLASS_NODE.equals(argumentTypes.get(0))) {
argsWithSelfTypeFix = x -> argumentTypes;
Expand All @@ -159,56 +162,75 @@ protected static MethodNode selectBestMatch(final List<MethodNode> candidates, f
};
}

MethodNode method = null;
// Phase 1: find best self-type match for each set of parameters
Map<String, MethodNode> m = new java.util.LinkedHashMap<>();
for (MethodNode candidate : candidates) {
StringBuilder signature = new StringBuilder();
Parameter[] parameters = candidate.getParameters();
for (int i = 1, n = parameters.length; i < n; i += 1) {
signature.append(parameters[i].getType().getName()).append(',');
}
MethodNode previous = m.put(signature.toString(), candidate);
if (previous != null) {
long d1 = selfParameterDistance(argsWithSelfTypeFix.apply(previous), previous.getParameters());
long d2 = selfParameterDistance(argsWithSelfTypeFix.apply(candidate), candidate.getParameters());

if (d1 <= d2) m.put(signature.toString(), previous);
}
}

// Phase 2: find best set of parameters for given call arguments
MethodNode method = null;
for (MethodNode candidate : m.values()) {
if (argumentTypes.size() == candidate.getParameters().length) {
Boolean compatible = SimpleTypeLookup.isTypeCompatible(argsWithSelfTypeFix.apply(candidate), candidate.getParameters());
Boolean compatible = SimpleTypeLookup.isTypeCompatible(argumentTypes.subList(1, argumentTypes.size()), tail(candidate.getParameters()));
if (compatible == Boolean.TRUE) { // exact match
method = candidate;
break;
} else if (compatible != Boolean.FALSE) { // fuzzy match
if (method != null) {
long d1 = calculateParameterDistance(argsWithSelfTypeFix.apply(method), method.getParameters());
long d2 = calculateParameterDistance(argsWithSelfTypeFix.apply(candidate), candidate.getParameters());
long d1 = tailParameterDistance(argumentTypes, method.getParameters());
long d2 = tailParameterDistance(argumentTypes, candidate.getParameters());

if (d1 <= d2) continue; // stick with current selection
}
method = candidate;
}
}
}

return method;
}

protected static long calculateParameterDistance(final List<ClassNode> arguments, final Parameter[] parameters) {
private static long selfParameterDistance(final List<ClassNode> arguments, final Parameter[] parameters) {
try {
if (arguments.size() == 1 && parameters.length == 1) {
Class<?>[] args = {arguments.get(0).getTypeClass()};
Class<?>[] prms = {parameters[0].getType().getTypeClass()};
return MetaClassHelper.calculateParameterDistance(args, new ParameterTypes(prms));
}

// weight self type higher to prevent considering getAt(Map, Object)
// and getAt(Object, String) equally for the arguments (Map, String)
Class<?>[] a = {arguments.get(0).getTypeClass()};
Class<?>[] p = {parameters[0].getType().getTypeClass()};
// TODO: This can fail in a lot of cases; is there a better way to call it?
return MetaClassHelper.calculateParameterDistance(a, new ParameterTypes(p));
} catch (Throwable t) {
return Long.MAX_VALUE - (VariableScope.isVoidOrObject(parameters[0].getType()) ? 0 : 1);
}
}

int n = 1 + arguments.size();
private static long tailParameterDistance(final List<ClassNode> arguments, final Parameter[] parameters) {
try {
int n = arguments.size() - 1;
Class<?>[] args = new Class[n];
for (int i = 1; i < n; i += 1) {
args[i] = arguments.get(i - 1).getTypeClass();
for (int i = 0; i < n; i += 1) {
args[i] = arguments.get(i + 1).getTypeClass();
}
args[0] = args[1]; // repeat the self type for effect

n = 1 + parameters.length;
n = parameters.length - 1;
Class<?>[] prms = new Class[n];
for (int i = 1; i < n; i += 1) {
prms[i] = parameters[i - 1].getType().getTypeClass();
for (int i = 0; i < n; i += 1) {
prms[i] = parameters[i + 1].getType().getTypeClass();
}
prms[0] = prms[1]; // repeat the self type for effect

// TODO: This can fail in a lot of cases; is there a better way to call it?
return MetaClassHelper.calculateParameterDistance(args, new ParameterTypes(prms));
} catch (Throwable t) {
return Long.MAX_VALUE - (VariableScope.isVoidOrObject(parameters[0].getType()) ? 0 : 1);
return Long.MAX_VALUE;
}
}
}
27 changes: 25 additions & 2 deletions base/org.eclipse.jdt.groovy.core/src/org/eclipse/jdt/groovy/search/SimpleTypeLookup.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@
import org.codehaus.groovy.ast.expr.StaticMethodCallExpression;
import org.codehaus.groovy.ast.expr.VariableExpression;
import org.codehaus.groovy.classgen.asm.OptimizingStatementWriter.StatementMeta;
import org.codehaus.groovy.reflection.ParameterTypes;
import org.codehaus.groovy.runtime.MetaClassHelper;
import org.codehaus.groovy.syntax.Types;
import org.codehaus.groovy.transform.trait.Traits;
import org.codehaus.jdt.groovy.model.GroovyCompilationUnit;
Expand Down Expand Up @@ -808,14 +810,35 @@ protected static Optional<MethodNode> findPropertyAccessorMethod(final String pr

protected static MethodNode closer(final MethodNode next, final MethodNode last, final List<ClassNode> args) {
if (last != null) {
long d1 = CategoryTypeLookup.calculateParameterDistance(args, last.getParameters());
long d2 = CategoryTypeLookup.calculateParameterDistance(args, next.getParameters());
long d1 = calculateParameterDistance(args, last.getParameters());
long d2 = calculateParameterDistance(args, next.getParameters());
if (d1 <= d2)
return last;
}
return next;
}

protected static long calculateParameterDistance(final List<ClassNode> arguments, final Parameter[] parameters) {
try {
int n = arguments.size();
Class<?>[] args = new Class[n];
for (int i = 0; i < n; i += 1) {
args[i] = arguments.get(i).getTypeClass();
}

n = parameters.length;
Class<?>[] prms = new Class[n];
for (int i = 0; i < n; i += 1) {
prms[i] = parameters[i].getType().getTypeClass();
}

// TODO: This can fail in a lot of cases; is there a better way to call it?
return MetaClassHelper.calculateParameterDistance(args, new ParameterTypes(prms));
} catch (Throwable t) {
return Long.MAX_VALUE - (VariableScope.isVoidOrObject(parameters[0].getType()) ? 0 : 1);
}
}

protected static FieldNode createLengthField(final ClassNode declaringType) {
FieldNode lengthField = new FieldNode("length", Flags.AccPublic, VariableScope.INTEGER_CLASS_NODE, declaringType, null);
lengthField.setDeclaringClass(declaringType);
Expand Down
4 changes: 2 additions & 2 deletions ...ng.test/src/org/codehaus/groovy/eclipse/codebrowsing/tests/CodeSelectGenericsTests.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,8 @@ final class CodeSelectGenericsTests extends BrowsingTestSuite {
}

@Test
void testCodeSelectGenericCategoryMethod3() {
String contents = '[a: Number].getAt("a")'
void testCodeSelectGenericCategoryMethod() {
String contents = '[1: Number].getAt(1)'
IJavaElement elem = assertCodeSelect([contents], 'getAt')
MethodNode method = ((GroovyResolvedBinaryMethod) elem).inferredElement
assert method.returnType.toString(false) == 'java.lang.Class <java.lang.Number>'
Expand Down

0 comments on commit f822985

Please sign in to comment.