This is an automated email from the ASF dual-hosted git repository.
paulk pushed a commit to branch GROOVY_3_0_X
in repository https://gitbox.apache.org/repos/asf/groovy.git
The following commit(s) were added to refs/heads/GROOVY_3_0_X by this push:
new 4693a92 GROOVY-10472: @AutoImplement is failing when covariant
returns are involved (port to 3_0_X)
4693a92 is described below
commit 4693a92cdb0ed116e34afb66fd7715b981e23bcd
Author: Paul King <[email protected]>
AuthorDate: Fri Feb 11 14:41:19 2022 +1000
GROOVY-10472: @AutoImplement is failing when covariant returns are involved
(port to 3_0_X)
---
.../apache/groovy/ast/tools/ClassNodeUtils.java | 28 ++++---
.../transform/AutoImplementASTTransformation.java | 16 ++--
.../transform/AutoImplementTransformTest.groovy | 92 ++++++++++++++++++++++
3 files changed, 119 insertions(+), 17 deletions(-)
diff --git a/src/main/java/org/apache/groovy/ast/tools/ClassNodeUtils.java
b/src/main/java/org/apache/groovy/ast/tools/ClassNodeUtils.java
index b394909..8bf4a46 100644
--- a/src/main/java/org/apache/groovy/ast/tools/ClassNodeUtils.java
+++ b/src/main/java/org/apache/groovy/ast/tools/ClassNodeUtils.java
@@ -45,6 +45,7 @@ import java.util.Set;
import static org.apache.groovy.ast.tools.AnnotatedNodeUtils.isGenerated;
import static org.apache.groovy.ast.tools.AnnotatedNodeUtils.markAsGenerated;
import static org.codehaus.groovy.ast.ClassHelper.boolean_TYPE;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.isOrImplements;
import static org.objectweb.asm.Opcodes.ACC_SYNTHETIC;
/**
@@ -83,11 +84,11 @@ public class ClassNodeUtils {
* @see ClassNode#addMethod(String, int, ClassNode, Parameter[],
ClassNode[], Statement)
*/
public static MethodNode addGeneratedMethod(ClassNode cNode, String name,
- int modifiers,
- ClassNode returnType,
- Parameter[] parameters,
- ClassNode[] exceptions,
- Statement code) {
+ int modifiers,
+ ClassNode returnType,
+ Parameter[] parameters,
+ ClassNode[] exceptions,
+ Statement code) {
MethodNode existing = cNode.getDeclaredMethod(name, parameters);
if (existing != null) return existing;
MethodNode result = new MethodNode(name, modifiers, returnType,
parameters, exceptions, code);
@@ -165,7 +166,7 @@ public class ClassNodeUtils {
* take precedence. Methods from interfaces visited early take precedence
* over later ones.
*
- * @param cNode The ClassNode
+ * @param cNode The ClassNode
* @param methodsMap A map of existing methods to alter
*/
public static void addDeclaredMethodsFromInterfaces(ClassNode cNode,
Map<String, MethodNode> methodsMap) {
@@ -196,7 +197,7 @@ public class ClassNodeUtils {
* Adds methods from interfaces and parent interfaces. Existing entries in
the methods map take precedence.
* Methods from interfaces visited early take precedence over later ones.
*
- * @param cNode The ClassNode
+ * @param cNode The ClassNode
* @param methodsMap A map of existing methods to alter
*/
public static void addDeclaredMethodsFromAllInterfaces(ClassNode cNode,
Map<String, MethodNode> methodsMap) {
@@ -324,7 +325,7 @@ public class ClassNodeUtils {
* Detect whether a static property with the given name is within the class
* or a super class.
*
- * @param cNode the ClassNode of interest
+ * @param cNode the ClassNode of interest
* @param propName the property name
* @return the static property if found or else null
*/
@@ -350,7 +351,8 @@ public class ClassNodeUtils {
&& !Modifier.isStatic(cNode.getModifiers());
}
- private ClassNodeUtils() { }
+ private ClassNodeUtils() {
+ }
public static boolean hasNoArgConstructor(ClassNode cNode) {
List<ConstructorNode> constructors = cNode.getDeclaredConstructors();
@@ -389,7 +391,7 @@ public class ClassNodeUtils {
/**
* Determine if the given ClassNode values have the same package name.
*
- * @param first a ClassNode
+ * @param first a ClassNode
* @param second a ClassNode
* @return true if both given nodes have the same package name
* @throws NullPointerException if either of the given nodes are null
@@ -422,4 +424,10 @@ public class ClassNodeUtils {
}
return null;
}
+
+ public static boolean isSubtype(ClassNode maybeSuperclassOrInterface,
ClassNode candidateChild) {
+ return maybeSuperclassOrInterface.isInterface() ||
candidateChild.isInterface()
+ ? isOrImplements(candidateChild, maybeSuperclassOrInterface)
+ : candidateChild.isDerivedFrom(maybeSuperclassOrInterface);
+ }
}
diff --git
a/src/main/java/org/codehaus/groovy/transform/AutoImplementASTTransformation.java
b/src/main/java/org/codehaus/groovy/transform/AutoImplementASTTransformation.java
index 7a94f1b..d96c620 100644
---
a/src/main/java/org/codehaus/groovy/transform/AutoImplementASTTransformation.java
+++
b/src/main/java/org/codehaus/groovy/transform/AutoImplementASTTransformation.java
@@ -42,6 +42,7 @@ import java.util.List;
import java.util.Map;
import static org.apache.groovy.ast.tools.ClassNodeUtils.addGeneratedMethod;
+import static org.apache.groovy.ast.tools.ClassNodeUtils.isSubtype;
import static org.apache.groovy.ast.tools.MethodNodeUtils.getPropertyName;
import static
org.apache.groovy.ast.tools.MethodNodeUtils.methodDescriptorWithoutReturnType;
import static org.apache.groovy.util.BeanUtils.capitalize;
@@ -163,9 +164,7 @@ public class AutoImplementASTTransformation extends
AbstractASTTransformation {
MethodNode found =
getDeclaredMethodCorrected(genericsSpec, correctedMethod, correctedClass);
if (found != null) {
String td = methodDescriptorWithoutReturnType(found);
- if (result.containsKey(td) &&
!result.get(td).isAbstract()) {
- continue;
- }
+ if (result.containsKey(td) &&
isWeakerCandidate(result.get(td), found)) continue;
result.put(td, found);
}
}
@@ -184,9 +183,7 @@ public class AutoImplementASTTransformation extends
AbstractASTTransformation {
MethodNode found =
getDeclaredMethodCorrected(updatedGenericsSpec, correctedMethod,
correctedInterface);
if (found != null) {
String td =
methodDescriptorWithoutReturnType(found);
- if (result.containsKey(td) &&
!result.get(td).isAbstract()) {
- continue;
- }
+ if (result.containsKey(td) &&
isWeakerCandidate(result.get(td), found)) continue;
result.put(td, found);
}
}
@@ -221,10 +218,15 @@ public class AutoImplementASTTransformation extends
AbstractASTTransformation {
}
}
}
-
return result;
}
+ private static boolean isWeakerCandidate(MethodNode existing, MethodNode
found) {
+ return !(existing.isAbstract() && !found.isAbstract()) &&
+ // GROOVY-10472: prefer covariant method with more concrete
type
+ isSubtype(found.getReturnType(), existing.getReturnType());
+ }
+
private static MethodNode getDeclaredMethodCorrected(final Map<String,
ClassNode> genericsSpec, final MethodNode origMethod, final ClassNode
correctedClass) {
for (MethodNode nameMatch :
correctedClass.getDeclaredMethods(origMethod.getName())) {
MethodNode correctedMethod = correctToGenericsSpec(genericsSpec,
nameMatch);
diff --git
a/src/test/org/codehaus/groovy/transform/AutoImplementTransformTest.groovy
b/src/test/org/codehaus/groovy/transform/AutoImplementTransformTest.groovy
index ba86e77..05b108f 100644
--- a/src/test/org/codehaus/groovy/transform/AutoImplementTransformTest.groovy
+++ b/src/test/org/codehaus/groovy/transform/AutoImplementTransformTest.groovy
@@ -216,4 +216,96 @@ final class AutoImplementTransformTest {
assert new Foo().compare('bar', 'baz') == 0
'''
}
+
+ @Test // GROOVY-10472
+ void testCovariantReturnTypes() {
+ assertScript '''
+ import groovy.transform.AutoImplement
+
+ interface Super { List findAll() }
+ interface Sub extends Super { Iterable findAll() }
+
+ @AutoImplement
+ class ThisClassFails implements Sub{}
+
+ assert !(new ThisClassFails().findAll())
+ '''
+ assertScript '''
+ import groovy.transform.AutoImplement
+
+ interface Super { ArrayList findAll() }
+ interface Sub extends Super { Iterable findAll() }
+
+ @AutoImplement
+ class ThisClassFails implements Sub{}
+
+ assert !(new ThisClassFails().findAll())
+ '''
+ assertScript '''
+ import groovy.transform.AutoImplement
+
+ interface Super { Iterable findAll() }
+ interface Sub extends Super { List findAll() }
+
+ @AutoImplement
+ class ThisClassFails implements Sub{}
+
+ assert !(new ThisClassFails().findAll())
+ '''
+ assertScript '''
+ import groovy.transform.AutoImplement
+
+ interface Super { Iterable findAll() }
+ interface Sub extends Super { ArrayList findAll() }
+
+ @AutoImplement
+ class ThisClassFails implements Sub{}
+
+ assert !(new ThisClassFails().findAll())
+ '''
+ assertScript '''
+ import groovy.transform.AutoImplement
+
+ interface Super { AbstractList findAll() }
+ interface Sub extends Super { List findAll() }
+
+ @AutoImplement
+ class ThisClassFails implements Sub{}
+
+ assert !(new ThisClassFails().findAll())
+ '''
+ assertScript '''
+ import groovy.transform.AutoImplement
+
+ interface Super { List findAll() }
+ interface Sub extends Super { AbstractList findAll() }
+
+ @AutoImplement
+ class ThisClassFails implements Sub{}
+
+ assert !(new ThisClassFails().findAll())
+ '''
+ assertScript '''
+ import groovy.transform.AutoImplement
+
+ interface Super { AbstractList findAll() }
+ interface Sub extends Super { ArrayList findAll() }
+
+ @AutoImplement
+ class ThisClassFails implements Sub{}
+
+ assert !(new ThisClassFails().findAll())
+ '''
+ assertScript '''
+ import groovy.transform.AutoImplement
+
+ interface Super { ArrayList findAll() }
+ interface Sub extends Super { AbstractList findAll() }
+
+ @AutoImplement
+ class ThisClassFails implements Sub{}
+
+ assert !(new ThisClassFails().findAll())
+ '''
+ }
}
