[groovy] branch GROOVY-6882 created (now 2c2c82f)

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[groovy] branch GROOVY-6882 created (now 2c2c82f)

emilles
This is an automated email from the ASF dual-hosted git repository.

emilles pushed a change to branch GROOVY-6882
in repository https://gitbox.apache.org/repos/asf/groovy.git.


      at 2c2c82f  GROOVY-6882: STC: skip overridden methods when selecting best fit

This branch includes the following new commits:

     new 4bc126e  fix AbstractMap#put(K,V) to resolve generics just like HashMap#put(K,V)
     new 2c2c82f  GROOVY-6882: STC: skip overridden methods when selecting best fit

The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails.  The revisions
listed as "add" were already present in the repository and have only
been added to this reference.


Reply | Threaded
Open this post in threaded view
|

[groovy] 01/02: fix AbstractMap#put(K, V) to resolve generics just like HashMap#put(K, V)

emilles
This is an automated email from the ASF dual-hosted git repository.

emilles pushed a commit to branch GROOVY-6882
in repository https://gitbox.apache.org/repos/asf/groovy.git

commit 4bc126e5cbf3d11c7f17d917da4811156822f6b3
Author: Eric Milles <[hidden email]>
AuthorDate: Fri Nov 20 15:15:08 2020 -0600

    fix AbstractMap#put(K,V) to resolve generics just like HashMap#put(K,V)
   
    - chooseBestMethod rejects HashMap#put(K,V) but not AbstractMap#put(K,V)
---
 .../transform/stc/StaticTypeCheckingSupport.java   |  2 +-
 .../groovy/transform/stc/GenericsSTCTest.groovy    | 28 +++++++++++-----------
 2 files changed, 15 insertions(+), 15 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 488f3f6..87c0a2a 100644
--- a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingSupport.java
+++ b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingSupport.java
@@ -1038,7 +1038,7 @@ public abstract class StaticTypeCheckingSupport {
                 Person p = foo(b)
              */
 
-            Map<GenericsType, GenericsType> declaringAndActualGenericsTypeMap = GenericsUtils.makeDeclaringAndActualGenericsTypeMap(declaringClassForDistance, actualReceiverForDistance);
+            Map<GenericsType, GenericsType> declaringAndActualGenericsTypeMap = GenericsUtils.makeDeclaringAndActualGenericsTypeMapOfExactType(declaringClassForDistance, actualReceiverForDistance);
             Parameter[] params = makeRawTypes(safeNode.getParameters(), declaringAndActualGenericsTypeMap);
             int dist = measureParametersAndArgumentsDistance(params, safeArgs);
             if (dist >= 0) {
diff --git a/src/test/groovy/transform/stc/GenericsSTCTest.groovy b/src/test/groovy/transform/stc/GenericsSTCTest.groovy
index 5db067d..469a0dc 100644
--- a/src/test/groovy/transform/stc/GenericsSTCTest.groovy
+++ b/src/test/groovy/transform/stc/GenericsSTCTest.groovy
@@ -122,7 +122,7 @@ class GenericsSTCTest extends StaticTypeCheckingTestCase {
         shouldFailWithMessages '''
             List<Integer> list = new LinkedList<>()
             list.add 'Hello'
-        ''', '[Static type checking] - Cannot call java.util.LinkedList <java.lang.Integer>#add(java.lang.Integer) with arguments [java.lang.String]'
+        ''', '[Static type checking] - Cannot find matching method java.util.LinkedList#add(java.lang.String). Please check if the declared type is correct and if the method exists.'
     }
 
     void testAddOnListWithDiamondAndWrongTypeUsingLeftShift() {
@@ -537,27 +537,27 @@ class GenericsSTCTest extends StaticTypeCheckingTestCase {
         '''
     }
 
-    void testPutMethodWithPrimitiveValue() {
+    void testPutMethodWithPrimitiveValue1() {
         assertScript '''
-            Map<String, Integer> map = new HashMap<String,Integer>()
+            def map = new HashMap<String, Integer>()
             map.put('hello', 1)
         '''
     }
 
-    void testPutMethodWithWrongValueType() {
-        shouldFailWithMessages '''
-            Map<String, Integer> map = new HashMap<String,Integer>()
-            map.put('hello', new Object())
-        ''', '[Static type checking] - Cannot call java.util.HashMap <String, Integer>#put(java.lang.String, java.lang.Integer) with arguments [java.lang.String, java.lang.Object]'
-    }
-
-    void testPutMethodWithPrimitiveValueAndArrayPut() {
+    void testPutMethodWithPrimitiveValue2() {
         assertScript '''
-            Map<String, Integer> map = new HashMap<String,Integer>()
+            def map = new HashMap<String, Integer>()
             map['hello'] = 1
         '''
     }
 
+    void testPutMethodWithWrongValueType() {
+        shouldFailWithMessages '''
+            def map = new HashMap<String, Integer>()
+            map.put('hello', new Object())
+        ''', '[Static type checking] - Cannot find matching method java.util.HashMap#put(java.lang.String, java.lang.Object). Please check if the declared type is correct and if the method exists.'
+    }
+
     void testShouldComplainAboutToInteger() {
         String code = '''
             class Test {
@@ -1020,7 +1020,7 @@ class GenericsSTCTest extends StaticTypeCheckingTestCase {
             })
             Map<Date, Date> map = new HashMap<>()
             map.put('foo', new Date())
-        ''', '[Static type checking] - Cannot call java.util.HashMap <java.util.Date, java.util.Date>#put(java.util.Date, java.util.Date) with arguments [java.lang.String, java.util.Date]'
+        ''', '[Static type checking] - 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.'
     }
     void testInferDiamondForAssignmentWithDatesAndIllegalKeyUsingSquareBracket() {
         shouldFailWithMessages '''
@@ -1058,7 +1058,7 @@ class GenericsSTCTest extends StaticTypeCheckingTestCase {
             })
             Map<Date, Date> map = new HashMap<>()
             map.put(new Date(), 'foo')
-        ''', '[Static type checking] - Cannot call java.util.HashMap <java.util.Date, java.util.Date>#put(java.util.Date, java.util.Date) with arguments [java.util.Date, java.lang.String]'
+        ''', '[Static type checking] - Cannot find matching method java.util.HashMap#put(java.util.Date, java.lang.String). Please check if the declared type is correct and if the method exists.'
     }
     void testInferDiamondForAssignmentWithDatesAndIllegalValueUsingSquareBracket() {
         shouldFailWithMessages '''

Reply | Threaded
Open this post in threaded view
|

[groovy] 02/02: GROOVY-6882: STC: skip overridden methods when selecting best fit

emilles
In reply to this post by emilles
This is an automated email from the ASF dual-hosted git repository.

emilles pushed a commit to branch GROOVY-6882
in repository https://gitbox.apache.org/repos/asf/groovy.git

commit 2c2c82ffd289f202c5588ebf4f2b11909953c08b
Author: Eric Milles <[hidden email]>
AuthorDate: Fri Nov 20 16:36:57 2020 -0600

    GROOVY-6882: STC: skip overridden methods when selecting best fit
---
 .../transform/stc/StaticTypeCheckingSupport.java   | 164 +++++++++------------
 .../stc/AnonymousInnerClassSTCTest.groovy          |  35 ++++-
 2 files changed, 102 insertions(+), 97 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 87c0a2a..15edfca 100644
--- a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingSupport.java
+++ b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingSupport.java
@@ -976,12 +976,8 @@ public abstract class StaticTypeCheckingSupport {
     }
 
     public static List<MethodNode> findDGMMethodsByNameAndArguments(final ClassLoader loader, final ClassNode receiver, final String name, final ClassNode[] args, final List<MethodNode> methods) {
-        final List<MethodNode> chosen;
         methods.addAll(findDGMMethodsForClassNode(loader, receiver, name));
-        if (methods.isEmpty()) return methods;
-
-        chosen = chooseBestMethod(receiver, methods, args);
-        return chosen;
+        return methods.isEmpty() ? methods : chooseBestMethod(receiver, methods, args);
     }
 
     /**
@@ -997,64 +993,64 @@ public abstract class StaticTypeCheckingSupport {
     }
 
     /**
-     * Given a list of candidate methods, returns the one which best matches the argument types
+     * Returns the method(s) which best fit the argument types.
      *
-     * @param receiver
-     * @param methods  candidate methods
-     * @param argumentTypes     argument types
-     * @return the list of methods which best matches the argument types. It is still possible that multiple
-     * methods match the argument types.
+     * @return zero or more results
      */
     public static List<MethodNode> chooseBestMethod(final ClassNode receiver, final Collection<MethodNode> methods, final ClassNode... argumentTypes) {
-        if (methods.isEmpty()) return Collections.emptyList();
+        if (!asBoolean(methods)) {
+            return Collections.emptyList();
+        }
         if (isUsingUncheckedGenerics(receiver)) {
-            ClassNode raw = makeRawType(receiver);
-            return chooseBestMethod(raw, methods, argumentTypes);
+            return chooseBestMethod(makeRawType(receiver), methods, argumentTypes);
         }
-        List<MethodNode> bestChoices = new LinkedList<>();
+
         int bestDist = Integer.MAX_VALUE;
-        Collection<MethodNode> choicesLeft = removeCovariantsAndInterfaceEquivalents(methods);
-        for (MethodNode candidateNode : choicesLeft) {
-            ClassNode declaringClassForDistance = candidateNode.getDeclaringClass();
-            ClassNode actualReceiverForDistance = receiver != null ? receiver : candidateNode.getDeclaringClass();
-            MethodNode safeNode = candidateNode;
+        List<MethodNode> bestChoices = new LinkedList<>();
+        boolean noCulling = methods.size() <= 1 || "<init>".equals(methods.iterator().next().getName());
+        Iterable<MethodNode> candidates = noCulling ? methods : removeCovariantsAndInterfaceEquivalents(methods);
+
+        for (MethodNode candidate : candidates) {
+            MethodNode safeNode = candidate;
             ClassNode[] safeArgs = argumentTypes;
-            boolean isExtensionMethodNode = candidateNode instanceof ExtensionMethodNode;
-            if (isExtensionMethodNode) {
-                safeArgs = new ClassNode[argumentTypes.length + 1];
-                System.arraycopy(argumentTypes, 0, safeArgs, 1, argumentTypes.length);
-                safeArgs[0] = receiver;
-                safeNode = ((ExtensionMethodNode) candidateNode).getExtensionMethodNode();
+            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
-            /*
+            /* TODO: corner case
                 class B extends A {}
+                Animal foo(A a) {}
+                Person foo(B b) {}
 
-                Animal foo(A o) {...}
-                Person foo(B i){...}
-
-                B  a = new B()
+                B b = new B()
                 Person p = foo(b)
-             */
+            */
 
+            ClassNode declaringClassForDistance = candidate.getDeclaringClass();
+            ClassNode actualReceiverForDistance = receiver != null ? receiver : declaringClassForDistance;
             Map<GenericsType, GenericsType> declaringAndActualGenericsTypeMap = GenericsUtils.makeDeclaringAndActualGenericsTypeMapOfExactType(declaringClassForDistance, actualReceiverForDistance);
+
             Parameter[] params = makeRawTypes(safeNode.getParameters(), declaringAndActualGenericsTypeMap);
             int dist = measureParametersAndArgumentsDistance(params, safeArgs);
             if (dist >= 0) {
                 dist += getClassDistance(declaringClassForDistance, actualReceiverForDistance);
-                dist += getExtensionDistance(isExtensionMethodNode);
+                dist += getExtensionDistance(isExtensionMethod);
                 if (dist < bestDist) {
-                    bestChoices.clear();
-                    bestChoices.add(candidateNode);
                     bestDist = dist;
+                    bestChoices.clear();
+                    bestChoices.add(candidate);
                 } else if (dist == bestDist) {
-                    bestChoices.add(candidateNode);
+                    bestChoices.add(candidate);
                 }
             }
         }
         if (bestChoices.size() > 1) {
-            // GROOVY-6849: prefer extension methods in case of ambiguity
+            // GROOVY-6849: prefer extension method in case of ambiguity
             List<MethodNode> onlyExtensionMethods = new LinkedList<>();
             for (MethodNode choice : bestChoices) {
                 if (choice instanceof ExtensionMethodNode) {
@@ -1150,10 +1146,9 @@ public abstract class StaticTypeCheckingSupport {
         return raw;
     }
 
-    private static Collection<MethodNode> removeCovariantsAndInterfaceEquivalents(final Collection<MethodNode> collection) {
-        if (collection.size() <= 1) return collection;
-        List<MethodNode> toBeRemoved = new LinkedList<>();
-        List<MethodNode> list = new LinkedList<>(new LinkedHashSet<>(collection));
+    private static List<MethodNode> removeCovariantsAndInterfaceEquivalents(final Collection<MethodNode> collection) {
+        List<MethodNode> toBeRemoved = new ArrayList<>();
+        List<MethodNode> list = new ArrayList<>(new LinkedHashSet<>(collection));
         for (int i = 0, n = list.size(); i < n - 1; i += 1) {
             MethodNode one = list.get(i);
             if (toBeRemoved.contains(one)) continue;
@@ -1161,72 +1156,53 @@ public abstract class StaticTypeCheckingSupport {
                 MethodNode two = list.get(j);
                 if (toBeRemoved.contains(two)) continue;
                 if (one.getParameters().length == two.getParameters().length) {
-                    if (areOverloadMethodsInSameClass(one, two)) {
+                    ClassNode oneDC = one.getDeclaringClass(), twoDC = two.getDeclaringClass();
+                    if (oneDC == twoDC) {
                         if (ParameterUtils.parametersEqual(one.getParameters(), two.getParameters())) {
-                            removeMethodWithSuperReturnType(toBeRemoved, one, two);
+                            ClassNode oneRT = one.getReturnType(), twoRT = two.getReturnType();
+                            if (isCovariant(oneRT, twoRT)) {
+                                toBeRemoved.add(two);
+                            } else if (isCovariant(twoRT, oneRT)) {
+                                toBeRemoved.add(one);
+                            }
                         } else {
-                            // this is an imperfect solution to determining if two methods are
-                            // equivalent, for example String#compareTo(Object) and String#compareTo(String)
-                            // in that case, Java marks the Object version as synthetic
-                            removeSyntheticMethodIfOne(toBeRemoved, one, two);
+                            // imperfect solution to determining if two methods are
+                            // equivalent, for example String#compareTo(Object) and
+                            // String#compareTo(String) -- in that case, the Object
+                            // version is marked as synthetic
+                            if (one.isSynthetic() && !two.isSynthetic()) {
+                                toBeRemoved.add(one);
+                            } else if (two.isSynthetic() && !one.isSynthetic()) {
+                                toBeRemoved.add(two);
+                            }
+                        }
+                    } else if (!oneDC.equals(twoDC)) {
+                        if (ParameterUtils.parametersEqual(one.getParameters(), two.getParameters())) {
+                            // GROOVY-6882, GROOVY-6970: drop overridden or interface equivalent method
+                            if (twoDC.isInterface() ? oneDC.implementsInterface(twoDC)
+                                    : oneDC.isDerivedFrom(twoDC)) {
+                                toBeRemoved.add(two);
+                            } else if (oneDC.isInterface() ? twoDC.isInterface()
+                                    : twoDC.isDerivedFrom(oneDC)) {
+                                toBeRemoved.add(one);
+                            }
                         }
-                    } else if (areEquivalentInterfaceMethods(one, two)) {
-                        // GROOVY-6970: choose between equivalent interface methods
-                        removeMethodInSuperInterface(toBeRemoved, one, two);
                     }
                 }
             }
         }
         if (toBeRemoved.isEmpty()) return list;
+
         List<MethodNode> result = new LinkedList<>(list);
         result.removeAll(toBeRemoved);
         return result;
     }
 
-    private static void removeMethodInSuperInterface(final List<MethodNode> toBeRemoved, final MethodNode one, final MethodNode two) {
-        ClassNode oneDC = one.getDeclaringClass();
-        ClassNode twoDC = two.getDeclaringClass();
-        if (oneDC.implementsInterface(twoDC)) {
-            toBeRemoved.add(two);
-        } else {
-            toBeRemoved.add(one);
-        }
-    }
-
-    private static boolean areEquivalentInterfaceMethods(final MethodNode one, final MethodNode two) {
-        return (one.getName().equals(two.getName())
-                && one.getDeclaringClass().isInterface()
-                && two.getDeclaringClass().isInterface()
-                && ParameterUtils.parametersEqual(one.getParameters(), two.getParameters()));
-    }
-
-    private static void removeSyntheticMethodIfOne(final List<MethodNode> toBeRemoved, final MethodNode one, final MethodNode two) {
-        if (one.isSynthetic() && !two.isSynthetic()) {
-            toBeRemoved.add(one);
-        } else if (two.isSynthetic() && !one.isSynthetic()) {
-            toBeRemoved.add(two);
+    private static boolean isCovariant(final ClassNode one, final ClassNode two) {
+        if (one.isArray() && two.isArray()) {
+            return isCovariant(one.getComponentType(), two.getComponentType());
         }
-    }
-
-    private static void removeMethodWithSuperReturnType(final List<MethodNode> toBeRemoved, final MethodNode one, final MethodNode two) {
-        ClassNode oneRT = one.getReturnType();
-        ClassNode twoRT = two.getReturnType();
-        if (isCovariant(oneRT, twoRT)) {
-            toBeRemoved.add(two);
-        } else if (isCovariant(twoRT, oneRT)) {
-            toBeRemoved.add(one);
-        }
-    }
-
-    private static boolean isCovariant(final ClassNode left, final ClassNode right) {
-        if (left.isArray() && right.isArray()) {
-            return isCovariant(left.getComponentType(), right.getComponentType());
-        }
-        return (left.isDerivedFrom(right) || left.implementsInterface(right));
-    }
-
-    private static boolean areOverloadMethodsInSameClass(final MethodNode one, final MethodNode two) {
-        return (one.getName().equals(two.getName()) && one.getDeclaringClass() == two.getDeclaringClass());
+        return (one.isDerivedFrom(two) || one.implementsInterface(two));
     }
 
     /**
diff --git a/src/test/groovy/transform/stc/AnonymousInnerClassSTCTest.groovy b/src/test/groovy/transform/stc/AnonymousInnerClassSTCTest.groovy
index 5ad7019..1fb7aac 100644
--- a/src/test/groovy/transform/stc/AnonymousInnerClassSTCTest.groovy
+++ b/src/test/groovy/transform/stc/AnonymousInnerClassSTCTest.groovy
@@ -68,7 +68,8 @@ class AnonymousInnerClassSTCTest extends StaticTypeCheckingTestCase {
     }
 
     void testCallMethodUsingAIC() {
-        assertScript '''abstract class Foo { abstract String item() }
+        assertScript '''
+            abstract class Foo { abstract String item() }
             boolean valid(Foo foo) {
                 foo.item() == 'ok'
             }
@@ -80,7 +81,8 @@ class AnonymousInnerClassSTCTest extends StaticTypeCheckingTestCase {
     }
 
     void testCallMethodUsingAICImplementingInterface() {
-        assertScript '''abstract class Foo implements Runnable { abstract String item() }
+        assertScript '''
+            abstract class Foo implements Runnable { abstract String item() }
             boolean valid(Foo foo) {
                 foo.item() == 'ok'
             }
@@ -93,7 +95,7 @@ class AnonymousInnerClassSTCTest extends StaticTypeCheckingTestCase {
         '''
     }
 
-    void testAICIntoClass() {
+    void testAICReferencingOuterMethod() {
         assertScript '''
             class Outer {
                 int outer() { 1 }
@@ -111,6 +113,33 @@ class AnonymousInnerClassSTCTest extends StaticTypeCheckingTestCase {
         '''
     }
 
+    // GROOVY-6882
+    void testAICReferencingOuterMethodOverride() {
+        assertScript '''
+            class B {
+                def m() { 'B' }
+            }
+
+            class C extends B {
+                @Override
+                def m() { 'C' }
+
+                void test() {
+                    def aic = new Runnable() {
+                        void run() {
+                            assert m() == 'C' // Cannot choose between [C#m(), B#m()]
+                        }
+                    }
+                    aic.run()
+
+                    assert m() == 'C'
+                }
+            }
+
+            new C().test()
+        '''
+    }
+
     // GROOVY-5566
     void testAICReferencingOuterLocalVariable() {
         assertScript '''