You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@groovy.apache.org by em...@apache.org on 2019/08/29 13:43:04 UTC
[groovy] branch GROOVY-8815 created (now 04b2602)
This is an automated email from the ASF dual-hosted git repository.
emilles pushed a change to branch GROOVY-8815
in repository https://gitbox.apache.org/repos/asf/groovy.git.
at 04b2602 GROOVY-8815: preserve redirects for generics placeholders
This branch includes the following new commits:
new 04b2602 GROOVY-8815: preserve redirects for generics placeholders
The 1 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.
[groovy] 01/01: GROOVY-8815: preserve redirects for generics
placeholders
Posted by em...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
emilles pushed a commit to branch GROOVY-8815
in repository https://gitbox.apache.org/repos/asf/groovy.git
commit 04b26023436b9c805ace64eaaab930d2e3da6799
Author: Eric Milles <er...@thomsonreuters.com>
AuthorDate: Fri Aug 16 15:52:31 2019 -0500
GROOVY-8815: preserve redirects for generics placeholders
When resolving method with generics, placeholder may have bounds. Ex:
def <T extends Type> void doSomething(Predicate<Type> check) { ... }
If Type has generics, the resolved generics types must be retained:
T -> Type<?> -> Type<X>
---
.../java/org/codehaus/groovy/ast/ClassNode.java | 94 +++++++++++-----------
.../codehaus/groovy/ast/tools/GenericsUtils.java | 14 +---
.../groovy/transform/trait/TraitComposer.java | 71 +++++++---------
src/test/groovy/bugs/Groovy8815.groovy | 77 ++++++++++++++++++
4 files changed, 161 insertions(+), 95 deletions(-)
diff --git a/src/main/java/org/codehaus/groovy/ast/ClassNode.java b/src/main/java/org/codehaus/groovy/ast/ClassNode.java
index ead078e..68a2c59 100644
--- a/src/main/java/org/codehaus/groovy/ast/ClassNode.java
+++ b/src/main/java/org/codehaus/groovy/ast/ClassNode.java
@@ -47,6 +47,9 @@ import java.util.ListIterator;
import java.util.Map;
import java.util.Set;
+import static java.util.Arrays.stream;
+import static java.util.stream.Collectors.joining;
+
/**
* Represents a class in the AST.
* <p>
@@ -191,19 +194,23 @@ public class ClassNode extends AnnotatedNode implements Opcodes {
/**
* Returns the ClassNode this ClassNode is redirecting to.
*/
- public ClassNode redirect(){
- if (redirect==null) return this;
- return redirect.redirect();
+ public ClassNode redirect() {
+ return (redirect == null ? this : redirect.redirect());
+ }
+
+ public final boolean isRedirectNode() {
+ return (redirect != null);
}
/**
* Sets this instance as proxy for the given ClassNode.
- * @param cn the class to redirect to. If set to null the redirect will be removed
+ *
+ * @param cn the class to redirect to. If {@code null} the redirect will be removed.
*/
public void setRedirect(ClassNode cn) {
- if (isPrimaryNode) throw new GroovyBugError("tried to set a redirect for a primary ClassNode ("+getName()+"->"+cn.getName()+").");
- if (cn!=null) cn = cn.redirect();
- if (cn==this) return;
+ if (isPrimaryNode) throw new GroovyBugError("tried to set a redirect for a primary ClassNode (" + getName() + "->" + cn.getName() + ").");
+ if (cn != null && !isGenericsPlaceHolder()) cn = cn.redirect();
+ if (cn == this) return;
redirect = cn;
}
@@ -1239,53 +1246,45 @@ public class ClassNode extends AnnotatedNode implements Opcodes {
public String toString(boolean showRedirect) {
if (isArray()) {
- return componentType.toString(showRedirect)+"[]";
+ return getComponentType().toString(showRedirect) + "[]";
}
- StringBuilder ret = new StringBuilder(getName());
- if (placeholder) ret = new StringBuilder(getUnresolvedName());
- if (!placeholder && genericsTypes != null) {
+ boolean placeholder = isGenericsPlaceHolder();
+ StringBuilder ret = new StringBuilder(!placeholder ? getName() : getUnresolvedName());
+ GenericsType[] genericsTypes = getGenericsTypes();
+ if (!placeholder && genericsTypes != null && genericsTypes.length > 0) {
ret.append(" <");
- for (int i = 0; i < genericsTypes.length; i++) {
- if (i != 0) ret.append(", ");
- GenericsType genericsType = genericsTypes[i];
- ret.append(genericTypeAsString(genericsType));
- }
+ ret.append(stream(genericsTypes).map(this::genericTypeAsString).collect(joining(", ")));
ret.append(">");
}
- if (redirect != null && showRedirect) {
- ret.append(" -> ").append(redirect().toString());
+ if (isRedirectNode() && showRedirect) {
+ ret.append(" -> ").append(redirect.toString());
}
return ret.toString();
}
/**
- * This exists to avoid a recursive definition of toString. The default toString
- * in GenericsType calls ClassNode.toString(), which calls GenericsType.toString(), etc.
- * @param genericsType
- * @return the string representing the generic type
+ * Avoids a recursive definition of toString. The default {@code toString}
+ * in {@link GenericsType} calls {@code ClassNode.toString()}, which would
+ * call {@code GenericsType.toString()} without this method.
*/
private String genericTypeAsString(GenericsType genericsType) {
- StringBuilder ret = new StringBuilder(genericsType.getName());
+ String name = genericsType.getName();
if (genericsType.getUpperBounds() != null) {
- ret.append(" extends ");
- for (int i = 0; i < genericsType.getUpperBounds().length; i++) {
- ClassNode classNode = genericsType.getUpperBounds()[i];
- if (classNode.equals(this)) {
- ret.append(classNode.getName());
- } else {
- ret.append(classNode.toString(false));
- }
- if (i + 1 < genericsType.getUpperBounds().length) ret.append(" & ");
- }
- } else if (genericsType.getLowerBound() !=null) {
- ClassNode classNode = genericsType.getLowerBound();
- if (classNode.equals(this)) {
- ret.append(" super ").append(classNode.getName());
- } else {
- ret.append(" super ").append(classNode);
- }
+ return name + " extends " + stream(genericsType.getUpperBounds())
+ .map(this::toStringTerminal).collect(joining(" & "));
+ } else if (genericsType.getLowerBound() != null) {
+ return name + " super " + toStringTerminal(genericsType.getLowerBound());
+ } else {
+ return name;
+ }
+ }
+
+ private String toStringTerminal(ClassNode classNode) {
+ if (classNode.equals(this)) {
+ return classNode.getName();
+ } else {
+ return classNode.toString(false);
}
- return ret.toString();
}
/**
@@ -1456,6 +1455,15 @@ public class ClassNode extends AnnotatedNode implements Opcodes {
return this.annotated;
}
+ public GenericsType asGenericsType() {
+ if (!isGenericsPlaceHolder()) {
+ return new GenericsType(this);
+ } else {
+ ClassNode upper = (redirect != null ? redirect : this);
+ return new GenericsType(this, new ClassNode[]{upper}, null);
+ }
+ }
+
public GenericsType[] getGenericsTypes() {
return genericsTypes;
}
@@ -1563,10 +1571,6 @@ public class ClassNode extends AnnotatedNode implements Opcodes {
}
return transformInstances;
}
-
- public boolean isRedirectNode() {
- return redirect!=null;
- }
@Override
public String getText() {
diff --git a/src/main/java/org/codehaus/groovy/ast/tools/GenericsUtils.java b/src/main/java/org/codehaus/groovy/ast/tools/GenericsUtils.java
index f9beb91..082d4b1 100644
--- a/src/main/java/org/codehaus/groovy/ast/tools/GenericsUtils.java
+++ b/src/main/java/org/codehaus/groovy/ast/tools/GenericsUtils.java
@@ -466,8 +466,7 @@ public class GenericsUtils {
}
public static Map<String, ClassNode> addMethodGenerics(MethodNode current, Map<String, ClassNode> oldSpec) {
- Map<String, ClassNode> ret = new HashMap<>(oldSpec);
- // ret starts with the original type specs, now add gts for the current method if any
+ Map<String, ClassNode> newSpec = new HashMap<>(oldSpec);
GenericsType[] gts = current.getGenericsTypes();
if (gts != null) {
for (GenericsType gt : gts) {
@@ -490,10 +489,10 @@ public class GenericsUtils {
type.setRedirect(redirect);
}
}
- ret.put(name, type);
+ newSpec.put(name, type);
}
}
- return ret;
+ return newSpec;
}
public static void extractSuperClassGenerics(ClassNode type, ClassNode target, Map<String, ClassNode> spec) {
@@ -663,12 +662,7 @@ public class GenericsUtils {
throw new GroovyBugError("Given generics type " + old + " must be a placeholder!");
ClassNode fromSpec = genericsSpec.get(old.getName());
if (fromSpec != null) {
- if (fromSpec.isGenericsPlaceHolder()) {
- ClassNode[] upper = new ClassNode[]{fromSpec.redirect()};
- newTypes[i] = new GenericsType(fromSpec, upper, null);
- } else {
- newTypes[i] = new GenericsType(fromSpec);
- }
+ newTypes[i] = fromSpec.asGenericsType();
} else {
ClassNode[] upper = old.getUpperBounds();
ClassNode[] newUpper = upper;
diff --git a/src/main/java/org/codehaus/groovy/transform/trait/TraitComposer.java b/src/main/java/org/codehaus/groovy/transform/trait/TraitComposer.java
index f05e02b..f7dae0a 100644
--- a/src/main/java/org/codehaus/groovy/transform/trait/TraitComposer.java
+++ b/src/main/java/org/codehaus/groovy/transform/trait/TraitComposer.java
@@ -59,17 +59,15 @@ import org.codehaus.groovy.transform.ASTTransformationCollectorCodeVisitor;
import org.codehaus.groovy.transform.sc.StaticCompileTransformation;
import org.objectweb.asm.Opcodes;
-import java.lang.reflect.Modifier;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
-import java.util.HashMap;
import java.util.Iterator;
-import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
import static org.codehaus.groovy.ast.tools.GeneralUtils.args;
import static org.codehaus.groovy.ast.tools.GeneralUtils.assignX;
@@ -130,37 +128,34 @@ public abstract class TraitComposer {
ClassNode helperClassNode = helpers.getHelper();
ClassNode fieldHelperClassNode = helpers.getFieldHelper();
ClassNode staticFieldHelperClassNode = helpers.getStaticFieldHelper();
- Map<String,ClassNode> genericsSpec = GenericsUtils.createGenericsSpec(cNode);
- genericsSpec = GenericsUtils.createGenericsSpec(trait, genericsSpec);
+ Map<String, ClassNode> genericsSpec = GenericsUtils.createGenericsSpec(trait, GenericsUtils.createGenericsSpec(cNode));
for (MethodNode methodNode : helperClassNode.getAllDeclaredMethods()) {
String name = methodNode.getName();
Parameter[] helperMethodParams = methodNode.getParameters();
- boolean isAbstract = methodNode.isAbstract();
- if (!isAbstract && helperMethodParams.length > 0 && ((methodNode.getModifiers() & Opcodes.ACC_STATIC) == Opcodes.ACC_STATIC) && (!name.contains("$") || (methodNode.getModifiers() & Opcodes.ACC_SYNTHETIC) == 0)) {
+ int nParams = helperMethodParams.length;
+ if (nParams > 0 && !methodNode.isAbstract() && ((methodNode.getModifiers() & Opcodes.ACC_STATIC) != 0)
+ && (!name.contains("$") || (methodNode.getModifiers() & Opcodes.ACC_SYNTHETIC) == 0)) {
ArgumentListExpression argList = new ArgumentListExpression();
argList.addExpression(new VariableExpression("this"));
- Parameter[] origParams = new Parameter[helperMethodParams.length - 1];
- Parameter[] params = new Parameter[helperMethodParams.length - 1];
+ Parameter[] origParams = new Parameter[nParams - 1];
+ Parameter[] params = new Parameter[nParams - 1];
System.arraycopy(methodNode.getParameters(), 1, params, 0, params.length);
- Map<String,ClassNode> methodGenericsSpec = new LinkedHashMap<String, ClassNode>(genericsSpec);
MethodNode originalMethod = trait.getMethod(name, params);
- // Original method may be null for the case of private or static methods
- if (originalMethod!=null) {
- methodGenericsSpec = GenericsUtils.addMethodGenerics(originalMethod, methodGenericsSpec);
- }
- for (int i = 1; i < helperMethodParams.length; i++) {
+ Map<String, ClassNode> methodGenericsSpec = Optional.ofNullable(originalMethod)
+ .map(m -> GenericsUtils.addMethodGenerics(m, genericsSpec)).orElse(genericsSpec);
+ for (int i = 1; i < nParams; i += 1) {
Parameter parameter = helperMethodParams[i];
ClassNode originType = parameter.getOriginType();
ClassNode fixedType = correctToGenericsSpecRecurse(methodGenericsSpec, originType);
Parameter newParam = new Parameter(fixedType, parameter.getName());
- List<AnnotationNode> copied = new LinkedList<AnnotationNode>();
- List<AnnotationNode> notCopied = new LinkedList<AnnotationNode>();
+ List<AnnotationNode> copied = new LinkedList<>();
+ List<AnnotationNode> notCopied = new LinkedList<>();
GeneralUtils.copyAnnotatedNodeAnnotations(parameter, copied, notCopied);
newParam.addAnnotations(copied);
params[i - 1] = newParam;
- origParams[i-1] = parameter;
- argList.addExpression(new VariableExpression(params[i - 1]));
+ origParams[i - 1] = parameter;
+ argList.addExpression(new VariableExpression(newParam));
}
createForwarderMethod(trait, cNode, methodNode, originalMethod, helperClassNode, methodGenericsSpec, helperMethodParams, origParams, params, argList, unit);
}
@@ -181,7 +176,7 @@ public abstract class TraitComposer {
// we should implement the field helper interface too
cNode.addInterface(fieldHelperClassNode);
// implementation of methods
- List<MethodNode> declaredMethods = new LinkedList<MethodNode>();
+ List<MethodNode> declaredMethods = new LinkedList<>();
for (MethodNode declaredMethod : fieldHelperClassNode.getAllDeclaredMethods()) {
if (declaredMethod.getName().endsWith(Traits.DIRECT_GETTER_SUFFIX)) {
declaredMethods.add(0, declaredMethod);
@@ -228,17 +223,17 @@ public abstract class TraitComposer {
if (helperField == null) {
// look for possible legacy fields (trait compiled pre 2.4.8)
helperField = fieldHelperClassNode.getField(Traits.FIELD_PREFIX + Traits.PUBLIC_FIELD_PREFIX + fieldName);
- if (helperField==null) {
+ if (helperField == null) {
publicField = false;
helperField = fieldHelperClassNode.getField(Traits.FIELD_PREFIX + Traits.PRIVATE_FIELD_PREFIX + fieldName);
}
- if (helperField==null) {
+ if (helperField == null) {
publicField = true;
// try to find a static one
- helperField = fieldHelperClassNode.getField(Traits.STATIC_FIELD_PREFIX+Traits.PUBLIC_FIELD_PREFIX+fieldName);
- if (helperField==null) {
+ helperField = fieldHelperClassNode.getField(Traits.STATIC_FIELD_PREFIX+Traits.PUBLIC_FIELD_PREFIX + fieldName);
+ if (helperField == null) {
publicField = false;
- helperField = fieldHelperClassNode.getField(Traits.STATIC_FIELD_PREFIX+Traits.PRIVATE_FIELD_PREFIX +fieldName);
+ helperField = fieldHelperClassNode.getField(Traits.STATIC_FIELD_PREFIX+Traits.PRIVATE_FIELD_PREFIX + fieldName);
}
fieldMods = fieldMods | Opcodes.ACC_STATIC;
isStatic = Opcodes.ACC_STATIC;
@@ -248,8 +243,8 @@ public abstract class TraitComposer {
if (getter) {
// add field
if (helperField!=null) {
- List<AnnotationNode> copied = new LinkedList<AnnotationNode>();
- List<AnnotationNode> notCopied = new LinkedList<AnnotationNode>();
+ List<AnnotationNode> copied = new LinkedList<>();
+ List<AnnotationNode> notCopied = new LinkedList<>();
GeneralUtils.copyAnnotatedNodeAnnotations(helperField, copied, notCopied);
FieldNode fieldNode = cNode.addField(fieldName, fieldMods, returnType, null);
fieldNode.addAnnotations(copied);
@@ -334,33 +329,32 @@ public abstract class TraitComposer {
);
mce.setImplicitThis(false);
- genericsSpec = GenericsUtils.addMethodGenerics(helperMethod,genericsSpec);
+ genericsSpec = GenericsUtils.addMethodGenerics(helperMethod, genericsSpec);
ClassNode[] exceptionNodes = correctToGenericsSpecRecurse(genericsSpec, copyExceptions(helperMethod.getExceptions()));
ClassNode fixedReturnType = correctToGenericsSpecRecurse(genericsSpec, helperMethod.getReturnType());
boolean noCastRequired = genericsSpec.isEmpty() || fixedReturnType.getName().equals(ClassHelper.VOID_TYPE.getName());
Expression forwardExpression = noCastRequired ? mce : new CastExpression(fixedReturnType,mce);
- int access = helperMethod.getModifiers();
// we could rely on the first parameter name ($static$self) but that information is not
// guaranteed to be always present
boolean isHelperForStaticMethod = helperMethodParams[0].getOriginType().equals(ClassHelper.CLASS_Type);
- if (Modifier.isPrivate(access) && !isHelperForStaticMethod) {
- // do not create forwarder for private methods
- // see GROOVY-7213
+ if (helperMethod.isPrivate() && !isHelperForStaticMethod) {
+ // GROOVY-7213: do not create forwarder for private methods
return;
}
+ int modifiers = helperMethod.getModifiers();
if (!isHelperForStaticMethod) {
- access = access ^ Opcodes.ACC_STATIC;
+ modifiers ^= Opcodes.ACC_STATIC;
}
MethodNode forwarder = new MethodNode(
helperMethod.getName(),
- access,
+ modifiers,
fixedReturnType,
forwarderParams,
exceptionNodes,
new ExpressionStatement(forwardExpression)
);
- List<AnnotationNode> copied = new LinkedList<AnnotationNode>();
+ List<AnnotationNode> copied = new LinkedList<>();
List<AnnotationNode> notCopied = Collections.emptyList(); // at this point, should *always* stay empty
GeneralUtils.copyAnnotatedNodeAnnotations(helperMethod, copied, notCopied);
if (!copied.isEmpty()) {
@@ -374,8 +368,7 @@ public abstract class TraitComposer {
// null indicates a static method which may still need generics correction
GenericsType[] genericsTypes = helperMethod.getGenericsTypes();
if (genericsTypes != null) {
- Map<String, ClassNode> methodSpec = new HashMap<String, ClassNode>();
- methodSpec = GenericsUtils.addMethodGenerics(helperMethod, methodSpec);
+ Map<String, ClassNode> methodSpec = GenericsUtils.addMethodGenerics(helperMethod, Collections.emptyMap());
GenericsType[] newGt = GenericsUtils.applyGenericsContextToPlaceHolders(methodSpec, helperMethod.getGenericsTypes());
forwarder.setGenericsTypes(newGt);
}
@@ -384,9 +377,7 @@ public abstract class TraitComposer {
AnnotationNode bridgeAnnotation = new AnnotationNode(Traits.TRAITBRIDGE_CLASSNODE);
bridgeAnnotation.addMember("traitClass", new ClassExpression(trait));
bridgeAnnotation.addMember("desc", new ConstantExpression(BytecodeHelper.getMethodDescriptor(helperMethod.getReturnType(), traitMethodParams)));
- forwarder.addAnnotation(
- bridgeAnnotation
- );
+ forwarder.addAnnotation(bridgeAnnotation);
MethodNode existingMethod = findExistingMethod(targetNode, forwarder);
if (existingMethod != null) {
diff --git a/src/test/groovy/bugs/Groovy8815.groovy b/src/test/groovy/bugs/Groovy8815.groovy
new file mode 100644
index 0000000..8a8c9f4
--- /dev/null
+++ b/src/test/groovy/bugs/Groovy8815.groovy
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package groovy.bugs
+
+import org.codehaus.groovy.control.CompilerConfiguration
+import org.codehaus.groovy.tools.javac.JavaAwareCompilationUnit
+import org.junit.Test
+
+final class Groovy8815 {
+
+ @Test
+ void testGenerics() {
+ def config = new CompilerConfiguration(
+ targetDirectory: File.createTempDir(),
+ jointCompilationOptions: [memStub: true]
+ )
+
+ def parentDir = File.createTempDir()
+ try {
+ def a = new File(parentDir, 'Event.groovy')
+ a.write '''
+ class Event<T> {
+ Event(String id, T payload) {}
+ Event<T> setReplyTo(Object replyTo) {}
+ }
+ '''
+ def b = new File(parentDir, 'Events.groovy')
+ b.write '''
+ import groovy.transform.CompileStatic
+ import java.util.function.Consumer
+ @CompileStatic
+ trait Events {
+ def <E extends Event<?>> Registration<Object, Consumer<E>> on(Class key, Closure consumer) {}
+ }
+ '''
+ def c = new File(parentDir, 'Registration.java')
+ c.write '''
+ interface Registration<K, V> {
+ }
+ '''
+ def d = new File(parentDir, 'Service.groovy')
+ d.write '''
+ class Service implements Events {
+ }
+ '''
+
+ def loader = new GroovyClassLoader(this.class.classLoader)
+ def cu = new JavaAwareCompilationUnit(config, loader)
+ cu.addSources(a, b, c, d)
+ cu.compile()
+
+ def method = loader.loadClass('Service').getMethod('on', Class, Closure)
+
+ // should not contain unresolved type parameter 'T'
+ assert method.genericSignature == '<E:LEvent<*>;>(Ljava/lang/Class;Lgroovy/lang/Closure;)LRegistration<Ljava/lang/Object;Ljava/util/function/Consumer<TE;>;>;'
+ } finally {
+ parentDir.deleteDir()
+ config.targetDirectory.deleteDir()
+ }
+ }
+}