You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@groovy.apache.org by su...@apache.org on 2019/09/03 08:39:26 UTC
[groovy] branch master updated: GROOVY-8815: preserve redirects for
generics placeholders
This is an automated email from the ASF dual-hosted git repository.
sunlan pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/groovy.git
The following commit(s) were added to refs/heads/master by this push:
new 0c2bdb9 GROOVY-8815: preserve redirects for generics placeholders
0c2bdb9 is described below
commit 0c2bdb903800d528746579a9385100a24020c186
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()
+ }
+ }
+}