This is an automated email from the ASF dual-hosted git repository. emilles pushed a commit to branch GROOVY-8788-take2 in repository https://gitbox.apache.org/repos/asf/groovy.git
commit f9ef470f321c0770aab97e918f28123a810327d6 Author: Eric Milles <[email protected]> AuthorDate: Sun Aug 21 19:36:17 2022 -0500 GROOVY-8788: STC: prefer closer parameter match over receiver-type match --- src/main/groovy/groovy/grape/GrapeIvy.groovy | 2 +- .../transform/stc/StaticTypeCheckingSupport.java | 96 ++++++++++------------ src/test/groovy/bugs/Groovy8629Bug.groovy | 5 +- .../groovy/transform/stc/STCAssignmentTest.groovy | 4 +- .../classgen/asm/sc/bugs/Groovy7325Bug.groovy | 29 +++---- .../groovy/transform/DelegateTransformTest.groovy | 6 +- 6 files changed, 67 insertions(+), 75 deletions(-) diff --git a/src/main/groovy/groovy/grape/GrapeIvy.groovy b/src/main/groovy/groovy/grape/GrapeIvy.groovy index bfc00258f1..8a192b35b4 100644 --- a/src/main/groovy/groovy/grape/GrapeIvy.groovy +++ b/src/main/groovy/groovy/grape/GrapeIvy.groovy @@ -596,7 +596,7 @@ class GrapeIvy implements GrapeEngine { // check for mutually exclusive arguments Set<String> keys = (Set<String>) args.keySet() keys.each { key -> - Set<String> badArgs = MUTUALLY_EXCLUSIVE_KEYS[key] + Set<String> badArgs = MUTUALLY_EXCLUSIVE_KEYS.get(key) if (badArgs && !badArgs.disjoint(keys)) { throw new RuntimeException("Mutually exclusive arguments passed into grab: ${keys.intersect(badArgs) + key}") } diff --git a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingSupport.java b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingSupport.java index 4d4090fc82..6024b92e61 100644 --- a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingSupport.java +++ b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingSupport.java @@ -1031,47 +1031,28 @@ public abstract class StaticTypeCheckingSupport { * * @return zero or more results */ - public static List<MethodNode> chooseBestMethod(final ClassNode receiver, final Collection<MethodNode> methods, final ClassNode... argumentTypes) { + public static List<MethodNode> chooseBestMethod(final ClassNode receiver, Collection<MethodNode> methods, final ClassNode... argumentTypes) { if (!asBoolean(methods)) { return Collections.emptyList(); } int bestDist = Integer.MAX_VALUE; - List<MethodNode> bestChoices = new LinkedList<>(); - boolean duckType = receiver instanceof UnionTypeClassNode; // GROOVY-8965: type disjunction - boolean noCulling = methods.size() <= 1 || "<init>".equals(methods.iterator().next().getName()); - Iterable<MethodNode> candidates = noCulling ? methods : removeCovariantsAndInterfaceEquivalents(methods, duckType); - - for (MethodNode candidate : candidates) { - MethodNode safeNode = candidate; - ClassNode[] safeArgs = argumentTypes; - boolean isExtensionMethod = candidate instanceof ExtensionMethodNode; - if (isExtensionMethod) { - int nArgs = argumentTypes.length; - safeArgs = new ClassNode[nArgs + 1]; - System.arraycopy(argumentTypes, 0, safeArgs, 1, nArgs); - safeArgs[0] = receiver; // prepend self-type as first argument - safeNode = ((ExtensionMethodNode) candidate).getExtensionMethodNode(); - } - - /* TODO: corner case - class B extends A {} - Animal foo(A a) {} - Person foo(B b) {} - - B b = new B() - Person p = foo(b) - */ - - ClassNode declaringClass = candidate.getDeclaringClass(); + List<MethodNode> bestMethods = new LinkedList<>(); + boolean duckType = receiver instanceof UnionTypeClassNode; + if (methods.size() > 1 && !methods.iterator().next().isConstructor()) + methods = removeCovariantsAndInterfaceEquivalents(methods, duckType); + + // phase 1: argument-parameter distance classifier + for (MethodNode method : methods) { + ClassNode declaringClass = method.getDeclaringClass(); ClassNode actualReceiver = receiver != null ? receiver : declaringClass; Map<GenericsType, GenericsType> spec; - if (candidate.isStatic()) { + if (method.isStatic()) { spec = Collections.emptyMap(); // none visible } else { spec = GenericsUtils.makeDeclaringAndActualGenericsTypeMapOfExactType(declaringClass, actualReceiver); - GenericsType[] methodGenerics = candidate.getGenericsTypes(); + GenericsType[] methodGenerics = method.getGenericsTypes(); if (methodGenerics != null) { // GROOVY-10322: remove hidden type parameters for (int i = 0, n = methodGenerics.length; i < n && !spec.isEmpty(); i += 1) { for (Iterator<GenericsType> it = spec.keySet().iterator(); it.hasNext(); ) { @@ -1080,34 +1061,49 @@ public abstract class StaticTypeCheckingSupport { } } } + Parameter[] parameters = makeRawTypes(method.getParameters(), spec); - Parameter[] params = makeRawTypes(safeNode.getParameters(), spec); - int dist = measureParametersAndArgumentsDistance(params, safeArgs); - if (dist >= 0) { - dist += getClassDistance(declaringClass, actualReceiver); - dist += getExtensionDistance(isExtensionMethod); + int dist = measureParametersAndArgumentsDistance(parameters, argumentTypes); + if (dist >= 0 && dist <= bestDist) { if (dist < bestDist) { bestDist = dist; - bestChoices.clear(); - bestChoices.add(candidate); - } else if (dist == bestDist) { - bestChoices.add(candidate); + bestMethods.clear(); + } + bestMethods.add(method); + } + } + + // phase 2: receiver-provider distance classifier + if (bestMethods.size() > 1 && receiver != null) { + methods = bestMethods; + bestDist = Integer.MAX_VALUE; + bestMethods = new LinkedList<>(); + for (MethodNode method : methods) { + int dist = getClassDistance(method.getDeclaringClass(), receiver); + if (dist <= bestDist) { + if (dist < bestDist) { + bestDist = dist; + bestMethods.clear(); + } + bestMethods.add(method); } } } - if (bestChoices.size() > 1 && !duckType) { - // GROOVY-6849: prefer extension method in case of ambiguity - List<MethodNode> onlyExtensionMethods = new LinkedList<>(); - for (MethodNode choice : bestChoices) { - if (choice instanceof ExtensionMethodNode) { - onlyExtensionMethods.add(choice); + + // phase 3: prefer extension method in case of tie + if (bestMethods.size() > 1 && !duckType) { + List<MethodNode> extensionMethods = new LinkedList<>(); + for (MethodNode method : bestMethods) { + if (method instanceof ExtensionMethodNode) { + extensionMethods.add(method); } } - if (onlyExtensionMethods.size() == 1) { - return onlyExtensionMethods; + if (extensionMethods.size() == 1) { + bestMethods = extensionMethods; } } - return bestChoices; + + return bestMethods; } private static int measureParametersAndArgumentsDistance(final Parameter[] parameters, final ClassNode[] argumentTypes) { @@ -1169,10 +1165,6 @@ public abstract class StaticTypeCheckingSupport { return getDistance(actualReceiverForDistance, declaringClassForDistance); } - private static int getExtensionDistance(final boolean isExtensionMethodNode) { - return isExtensionMethodNode ? 0 : 1; - } - private static Parameter[] makeRawTypes(final Parameter[] parameters, final Map<GenericsType, GenericsType> genericsPlaceholderAndTypeMap) { return Arrays.stream(parameters).map(param -> { String name = param.getType().getUnresolvedName(); diff --git a/src/test/groovy/bugs/Groovy8629Bug.groovy b/src/test/groovy/bugs/Groovy8629Bug.groovy index 3b56d81bc1..643cca4219 100644 --- a/src/test/groovy/bugs/Groovy8629Bug.groovy +++ b/src/test/groovy/bugs/Groovy8629Bug.groovy @@ -62,8 +62,8 @@ public class Groovy8629Bug extends GroovyTestCase { @Override IntegerPair next() { String key = keyIterator.next() - IntegerPair comp = new IntegerPair(m1[key], m2[key]) - return comp + IntegerPair pair = new IntegerPair(m1.get(key), m2.get(key)) + return pair } @Override @@ -87,5 +87,4 @@ public class Groovy8629Bug extends GroovyTestCase { ''' } - } diff --git a/src/test/groovy/transform/stc/STCAssignmentTest.groovy b/src/test/groovy/transform/stc/STCAssignmentTest.groovy index c573582bcc..e537f2fd1c 100644 --- a/src/test/groovy/transform/stc/STCAssignmentTest.groovy +++ b/src/test/groovy/transform/stc/STCAssignmentTest.groovy @@ -1251,8 +1251,8 @@ class STCAssignmentTest extends StaticTypeCheckingTestCase { else if (value instanceof GString) { value = value.toString() } - if (mvm[name]) { - mvm[name].add value + if (mvm.containsKey(name)) { + mvm.get(name).add(value) } else { mvm.put(name, [value]) } diff --git a/src/test/org/codehaus/groovy/classgen/asm/sc/bugs/Groovy7325Bug.groovy b/src/test/org/codehaus/groovy/classgen/asm/sc/bugs/Groovy7325Bug.groovy index 49ea6ed365..a50ae36ece 100644 --- a/src/test/org/codehaus/groovy/classgen/asm/sc/bugs/Groovy7325Bug.groovy +++ b/src/test/org/codehaus/groovy/classgen/asm/sc/bugs/Groovy7325Bug.groovy @@ -27,22 +27,22 @@ class Groovy7325Bug extends StaticTypeCheckingTestCase implements StaticCompilat void testGenericIdentityWithClosure() { assertScript ''' -public static <T> T identity(T self) { self } - -@ASTTest(phase=INSTRUCTION_SELECTION,value={ - assert node.rightExpression.getNodeMetaData(INFERRED_TYPE) == Integer_TYPE -}) -Integer i = identity(2) - -@ASTTest(phase=INSTRUCTION_SELECTION,value={ - assert node.rightExpression.getNodeMetaData(INFERRED_TYPE) == CLOSURE_TYPE -}) -Closure c = identity {'foo'} -''' + static <T> T itself(T self) { self } + + @ASTTest(phase=INSTRUCTION_SELECTION,value={ + assert node.rightExpression.getNodeMetaData(INFERRED_TYPE) == Integer_TYPE + }) + Integer i = itself(2) + + @ASTTest(phase=INSTRUCTION_SELECTION,value={ + assert node.rightExpression.getNodeMetaData(INFERRED_TYPE) == CLOSURE_TYPE + }) + Closure c = itself {'foo'} + ''' } void testShouldNotThrowIllegalAccessToProtectedData() { - shouldFailWithMessages(''' + shouldFailWithMessages ''' class Test { final Set<String> HISTORY = [] as HashSet @@ -53,6 +53,7 @@ Closure c = identity {'foo'} Test test = new Test() println test.history - ''', 'Method clone is protected in java.lang.Object') + ''', + 'Method clone is protected in java.lang.Object' } } diff --git a/src/test/org/codehaus/groovy/transform/DelegateTransformTest.groovy b/src/test/org/codehaus/groovy/transform/DelegateTransformTest.groovy index 7ad281e6ce..1e20c733f9 100644 --- a/src/test/org/codehaus/groovy/transform/DelegateTransformTest.groovy +++ b/src/test/org/codehaus/groovy/transform/DelegateTransformTest.groovy @@ -112,12 +112,12 @@ final class DelegateTransformTest { def ms = new MapSet() assert ms.size() == 1 - assert ms.toString() == '{a=1} [2, 3, 4]' + assert ms.toString() == '[a:1] [2, 3, 4]' ms.remove(3) assert ms.size() == 1 - assert ms.toString() == '{a=1} [2, 4]' + assert ms.toString() == '[a:1] [2, 4]' ms.clear() - assert ms.toString() == '{a=1} []' + assert ms.toString() == '[a:1] []' ''' }
