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 715ca1d528059907d2936af070335448bf95d2ea 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 --- .../transform/stc/StaticTypeCheckingSupport.java | 106 +++++++++++---------- .../transform/stc/StaticTypeCheckingVisitor.java | 13 ++- src/test/groovy/bugs/Groovy8629Bug.groovy | 5 +- .../stc/ArraysAndCollectionsSTCTest.groovy | 12 +-- .../stc/FieldsAndPropertiesSTCTest.groovy | 33 ++++--- .../groovy/transform/stc/GenericsSTCTest.groovy | 35 ++++--- .../groovy/transform/stc/STCAssignmentTest.groovy | 15 +-- .../transform/stc/TypeInferenceSTCTest.groovy | 12 --- .../classgen/asm/sc/bugs/Groovy7325Bug.groovy | 41 ++++---- .../groovy/transform/DelegateTransformTest.groovy | 6 +- 10 files changed, 137 insertions(+), 141 deletions(-) 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..6a0d23553d 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,38 @@ 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(); } + // GROOVY-8965: type disjunction + boolean duckType = receiver instanceof UnionTypeClassNode; + if (methods.size() > 1 && !methods.iterator().next().isConstructor()) + methods = removeCovariantsAndInterfaceEquivalents(methods, duckType); + + Set<MethodNode> bestMethods = new HashSet<>(); // choose best method(s) for each possible receiver + for (ClassNode rcvr : duckType ? ((UnionTypeClassNode) receiver).getDelegates() : new ClassNode[]{receiver}) { + bestMethods.addAll(chooseBestMethods(rcvr, methods, argumentTypes)); + } + return new LinkedList<>(bestMethods); // assumes caller wants remove to be inexpensive + } + + private static List<MethodNode> chooseBestMethods(final ClassNode receiver, Collection<MethodNode> methods, final ClassNode[] argumentTypes) { 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 ArrayList<>(); + + // 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 +1071,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); } } - 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 2: receiver-provider distance classifier + if (bestMethods.size() > 1 && receiver != null) { + methods = bestMethods; + bestDist = Integer.MAX_VALUE; + bestMethods = new ArrayList<>(); + for (MethodNode method : methods) { + int dist = getClassDistance(method.getDeclaringClass(), receiver); + if (dist <= bestDist) { + if (dist < bestDist) { + bestDist = dist; + bestMethods.clear(); + } + bestMethods.add(method); } } - if (onlyExtensionMethods.size() == 1) { - return onlyExtensionMethods; + } + + // phase 3: prefer extension method in case of tie + if (bestMethods.size() > 1) { + List<MethodNode> extensionMethods = new ArrayList<>(); + for (MethodNode method : bestMethods) { + if (method instanceof ExtensionMethodNode) { + extensionMethods.add(method); + } + } + if (extensionMethods.size() == 1) { + bestMethods = extensionMethods; } } - return bestChoices; + + return bestMethods; } private static int measureParametersAndArgumentsDistance(final Parameter[] parameters, final ClassNode[] argumentTypes) { @@ -1169,10 +1175,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/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java index 35efb8b896..d791fddc1c 100644 --- a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java +++ b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java @@ -827,11 +827,11 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport { enclosingExpressionRHS.visit(this); } ClassNode[] arguments = {rType, getType(enclosingExpressionRHS)}; - List<MethodNode> nodes = findMethod(lType.redirect(), "putAt", arguments); - if (nodes.size() == 1) { - typeCheckMethodsWithGenericsOrFail(lType, arguments, nodes.get(0), enclosingExpressionRHS); - } else if (nodes.isEmpty()) { + List<MethodNode> methods = findMethod(lType, "putAt", arguments); + if (methods.isEmpty()) { addNoMatchingMethodError(lType, "putAt", arguments, enclosingBinaryExpression); + } else if (methods.size() == 1) { + typeCheckMethodsWithGenericsOrFail(lType, arguments, methods.get(0), enclosingExpressionRHS); } } } @@ -4520,6 +4520,11 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport { } if (isArrayOp(op)) { + if (isOrImplements(left, MAP_TYPE) && isStringType(right)) { // GROOVY-5700, GROOVY-8788 + PropertyExpression prop = propX(leftExpression, rightExpression); // m['xx'] -> m.xx + return existsProperty(prop, !typeCheckingContext.isTargetOfEnclosingAssignment(expr)) + ? getType(prop) : getTypeForMapPropertyExpression(left, prop); + } Expression copy = binX(leftExpression, expr.getOperation(), rightExpression); copy.setSourcePosition(expr); // do not propagate BINARY_EXP_TARGET, etc. MethodNode method = findMethodOrFail(copy, left, "getAt", rightRedirect); 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/ArraysAndCollectionsSTCTest.groovy b/src/test/groovy/transform/stc/ArraysAndCollectionsSTCTest.groovy index c39f0df451..d80b4559ad 100644 --- a/src/test/groovy/transform/stc/ArraysAndCollectionsSTCTest.groovy +++ b/src/test/groovy/transform/stc/ArraysAndCollectionsSTCTest.groovy @@ -803,14 +803,12 @@ class ArraysAndCollectionsSTCTest extends StaticTypeCheckingTestCase { } // GROOVY-6131 - void testArraySetShouldGenerateBytecode() { + void testCollectionPutAt() { shouldFailWithMessages ''' - void addToCollection(Collection coll, int index, val) { - coll[index] = val + void addToCollection(Collection coll, int index, value) { + coll[index] = value } - def list = ['a'] - addToCollection(list, 0, 'b') - assert list == ['b'] + addToCollection(['a'], 0, 'b') ''', 'Cannot find matching method java.util.Collection#putAt(int, java.lang.Object)' } @@ -818,7 +816,7 @@ class ArraysAndCollectionsSTCTest extends StaticTypeCheckingTestCase { // GROOVY-6266 void testMapKeyGenerics() { assertScript ''' - HashMap<String,List<List>> map = new HashMap<String,List<List>>() + Map<String,List<List>> map = new HashMap<String,List<List>>() map.get('key',[['val1'],['val2']]) assert map.'key'[0] == ['val1'] ''' diff --git a/src/test/groovy/transform/stc/FieldsAndPropertiesSTCTest.groovy b/src/test/groovy/transform/stc/FieldsAndPropertiesSTCTest.groovy index ed471189ce..2f0ac89dee 100644 --- a/src/test/groovy/transform/stc/FieldsAndPropertiesSTCTest.groovy +++ b/src/test/groovy/transform/stc/FieldsAndPropertiesSTCTest.groovy @@ -576,33 +576,40 @@ class FieldsAndPropertiesSTCTest extends StaticTypeCheckingTestCase { ''' } - // GROOVY-5700 - void testInferenceOfMapDotProperty() { + // GROOVY-5700, GROOVY-8788 + void testInferenceOfMapSubProperty() { assertScript ''' - def m = [retries: 10] + def map = [key: 123] @ASTTest(phase=INSTRUCTION_SELECTION, value={ assert node.getNodeMetaData(INFERRED_TYPE) == Integer_TYPE }) - def r1 = m['retries'] + def val = map['key'] + assert val == 123 + ''' + } + // GROOVY-5700 + void testInferenceOfMapDotProperty() { + assertScript ''' + def map = [key: 123] @ASTTest(phase=INSTRUCTION_SELECTION, value={ assert node.getNodeMetaData(INFERRED_TYPE) == Integer_TYPE }) - def r2 = m.retries + def val = map.key + assert val == 123 ''' } void testInferenceOfListDotProperty() { - assertScript '''class Foo { int x } - def list = [new Foo(x:1), new Foo(x:2)] + assertScript ''' + class C { int x } + def list = [new C(x:1), new C(x:2)] @ASTTest(phase=INSTRUCTION_SELECTION, value={ - def iType = node.getNodeMetaData(INFERRED_TYPE) - assert iType == make(List) - assert iType.isUsingGenerics() - assert iType.genericsTypes[0].type == Integer_TYPE + def type = node.getNodeMetaData(INFERRED_TYPE) + assert type.toString(false) == 'java.util.List<java.lang.Integer>' }) - def r2 = list.x - assert r2 == [ 1,2 ] + def x = list.x + assert x == [1,2] ''' } diff --git a/src/test/groovy/transform/stc/GenericsSTCTest.groovy b/src/test/groovy/transform/stc/GenericsSTCTest.groovy index d6c7788f1c..9727aa89e5 100644 --- a/src/test/groovy/transform/stc/GenericsSTCTest.groovy +++ b/src/test/groovy/transform/stc/GenericsSTCTest.groovy @@ -2004,18 +2004,26 @@ class GenericsSTCTest extends StaticTypeCheckingTestCase { } void testPutAtWithWrongValueType() { + shouldFailWithMessages ''' + def map = new HashMap<Integer, Integer>() + map[123] = new Object() + ''', + 'Cannot call <K,V> org.codehaus.groovy.runtime.DefaultGroovyMethods#putAt(java.util.Map<K, V>, K, V) with arguments [java.util.HashMap<java.lang.Integer, java.lang.Integer>, int, java.lang.Object]' + } + + // GROOVY-8788 + void testPutAtWithWrongValueType2() { shouldFailWithMessages ''' def map = new HashMap<String, Integer>() - map['hello'] = new Object() + map['x'] = new Object() ''', - 'Cannot call <K,V> org.codehaus.groovy.runtime.DefaultGroovyMethods#putAt(java.util.Map<K, V>, K, V) with arguments [java.util.HashMap<java.lang.String, java.lang.Integer>, java.lang.String, java.lang.Object]' + 'Cannot assign value of type java.lang.Object to variable of type java.lang.Integer' } // GROOVY-9069 - void testPutAtWithWrongValueType2() { + void testPutAtWithWrongValueType3() { shouldFailWithMessages ''' - class ConfigAttribute { - } + class ConfigAttribute { } void test(Map<String, Map<String, List<String>>> maps) { maps.each { String key, Map<String, List<String>> map -> Map<String, List<ConfigAttribute>> value = [:] @@ -2024,18 +2032,15 @@ class GenericsSTCTest extends StaticTypeCheckingTestCase { } } ''', - 'Cannot find matching method java.util.Map#put(java.lang.String, java.util.LinkedHashMap<java.lang.String, java.util.List<ConfigAttribute>>). Please check if the declared type is correct and if the method exists.', - 'Cannot call <K,V> org.codehaus.groovy.runtime.DefaultGroovyMethods#putAt(java.util.Map<K, V>, K, V) with arguments [java.util.Map<java.lang.String, java.util.Map<java.lang.String, java.util.List<java.lang.String>>>, java.lang.String, java.util.LinkedHashMap<java.lang.String, java.util.List<ConfigAttribute>>]' - } + 'Cannot find matching method java.util.Map#put(java.lang.String, java.util.LinkedHashMap<java.lang.String, java.util.List<ConfigAttribute>>)', + 'Cannot assign java.util.LinkedHashMap<java.lang.String, java.util.List<ConfigAttribute>> to: java.util.Map<java.lang.String, java.util.List<java.lang.String>>' - void testPutAtWithWrongValueType3() { assertScript ''' void test(Map<String, Map<String, List<String>>> maps) { maps.each { String key, Map<String, List<String>> map -> Map<String, List<String>> value = [:] - // populate value - maps[key] = value maps.put(key, value) + maps[key] = value } } ''' @@ -2802,9 +2807,9 @@ class GenericsSTCTest extends StaticTypeCheckingTestCase { assert type.genericsTypes[1].type == DATE }) Map<Date, Date> map = new HashMap<>() - map.put('foo', new Date()) + map.put(123, new Date()) ''', - 'Cannot find matching method java.util.HashMap#put(java.lang.String, java.util.Date). Please check if the declared type is correct and if the method exists.' + 'Cannot find matching method java.util.HashMap#put(int, java.util.Date). Please check if the declared type is correct and if the method exists.' } void testInferDiamondForAssignmentWithDatesAndIllegalKeyUsingSquareBracket() { shouldFailWithMessages ''' @@ -2822,9 +2827,9 @@ class GenericsSTCTest extends StaticTypeCheckingTestCase { assert type.genericsTypes[1].type == DATE }) Map<Date, Date> map = new HashMap<>() - map['foo'] = new Date() + map[123] = new Date() ''', - 'Cannot call <K,V> org.codehaus.groovy.runtime.DefaultGroovyMethods#putAt(java.util.Map<K, V>, K, V) with arguments [java.util.HashMap<java.util.Date, java.util.Date>, java.lang.String, java.util.Date]' + 'Cannot call <K,V> org.codehaus.groovy.runtime.DefaultGroovyMethods#putAt(java.util.Map<K, V>, K, V) with arguments [java.util.HashMap<java.util.Date, java.util.Date>, int, java.util.Date]' } void testInferDiamondForAssignmentWithDatesAndIllegalValueUsingPut() { shouldFailWithMessages ''' diff --git a/src/test/groovy/transform/stc/STCAssignmentTest.groovy b/src/test/groovy/transform/stc/STCAssignmentTest.groovy index c573582bcc..9df4022ee1 100644 --- a/src/test/groovy/transform/stc/STCAssignmentTest.groovy +++ b/src/test/groovy/transform/stc/STCAssignmentTest.groovy @@ -1241,21 +1241,14 @@ class STCAssignmentTest extends StaticTypeCheckingTestCase { void setProperty(String name, value) { if (value instanceof File) { value = new File(value, 'bar.txt') - } - else if (value instanceof URL) { + } else if (value instanceof URL) { value = value.toURI() - } - else if (value instanceof InputStream) { + } else if (value instanceof InputStream) { value = new BufferedInputStream(value) - } - else if (value instanceof GString) { + } else if (value instanceof GString) { value = value.toString() } - if (mvm[name]) { - mvm[name].add value - } else { - mvm.put(name, [value]) - } + mvm.computeIfAbsent(name, k -> []).add(value) } } new M().setProperty('foo', 'bar') diff --git a/src/test/groovy/transform/stc/TypeInferenceSTCTest.groovy b/src/test/groovy/transform/stc/TypeInferenceSTCTest.groovy index af7526fcb9..ef48abf961 100644 --- a/src/test/groovy/transform/stc/TypeInferenceSTCTest.groovy +++ b/src/test/groovy/transform/stc/TypeInferenceSTCTest.groovy @@ -1018,18 +1018,6 @@ class TypeInferenceSTCTest extends StaticTypeCheckingTestCase { ''' } - void testInferMapValueType() { - assertScript ''' - Map<String, Integer> map = new HashMap<String,Integer>() - map['foo'] = 123 - map['bar'] = 246 - Integer foo = map['foo'] - assert foo == 123 - Integer bar = map.get('bar') - assert bar == 246 - ''' - } - // GROOVY-5522 void testTypeInferenceWithArrayAndFind() { assertScript ''' 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..d12cdd665a 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 @@ -16,43 +16,42 @@ * specific language governing permissions and limitations * under the License. */ - - package org.codehaus.groovy.classgen.asm.sc.bugs import groovy.transform.stc.StaticTypeCheckingTestCase import org.codehaus.groovy.classgen.asm.sc.StaticCompilationTestSupport -class Groovy7325Bug extends StaticTypeCheckingTestCase implements StaticCompilationTestSupport { +final class Groovy7325Bug extends StaticTypeCheckingTestCase implements StaticCompilationTestSupport { 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 + final Set<String> HISTORY = [] as HashSet - Set<String> getHistory() { - return HISTORY.clone() as HashSet<String> - } + Set<String> getHistory() { + return HISTORY.clone() as HashSet<String> + } } 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] []' ''' }
