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 2022/02/22 01:33:52 UTC

[groovy] branch GROOVY_3_0_X updated: GROOVY-4320, GROOVY-4516: Delegate: apply default arguments w/o Verifier

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

emilles 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 0a5da03  GROOVY-4320, GROOVY-4516: Delegate: apply default arguments w/o Verifier
0a5da03 is described below

commit 0a5da031c1aa4b2bb194ac13b3e2eaee0d1d7525
Author: Eric Milles <er...@thomsonreuters.com>
AuthorDate: Mon May 17 19:20:46 2021 -0500

    GROOVY-4320, GROOVY-4516: Delegate: apply default arguments w/o Verifier
    
    3_0_X backport
    
    Conflicts:
    	src/main/java/org/codehaus/groovy/transform/DelegateASTTransformation.java
    	src/test/org/codehaus/groovy/transform/DelegateTransformTest.groovy
---
 .../transform/DelegateASTTransformation.java       | 248 +++++++++++++--------
 .../groovy/transform/DelegateTransformTest.groovy  | 200 ++++++++++++-----
 2 files changed, 302 insertions(+), 146 deletions(-)

diff --git a/src/main/java/org/codehaus/groovy/transform/DelegateASTTransformation.java b/src/main/java/org/codehaus/groovy/transform/DelegateASTTransformation.java
index 120d611..2015fb3 100644
--- a/src/main/java/org/codehaus/groovy/transform/DelegateASTTransformation.java
+++ b/src/main/java/org/codehaus/groovy/transform/DelegateASTTransformation.java
@@ -19,7 +19,6 @@
 package org.codehaus.groovy.transform;
 
 import groovy.lang.Delegate;
-import groovy.lang.GroovyObject;
 import groovy.lang.Lazy;
 import groovy.lang.Reference;
 import org.codehaus.groovy.ast.ASTNode;
@@ -37,7 +36,6 @@ import org.codehaus.groovy.ast.expr.Expression;
 import org.codehaus.groovy.ast.expr.MethodCallExpression;
 import org.codehaus.groovy.ast.tools.BeanUtils;
 import org.codehaus.groovy.ast.tools.GenericsUtils;
-import org.codehaus.groovy.classgen.Verifier;
 import org.codehaus.groovy.control.CompilePhase;
 import org.codehaus.groovy.control.SourceUnit;
 
@@ -45,12 +43,13 @@ import java.lang.reflect.Modifier;
 import java.util.ArrayList;
 import java.util.HashSet;
 import java.util.List;
+import java.util.ListIterator;
 import java.util.Map;
 import java.util.Set;
 
+import static java.util.stream.Collectors.toSet;
 import static org.apache.groovy.ast.tools.ClassNodeUtils.addGeneratedMethod;
 import static org.apache.groovy.util.BeanUtils.capitalize;
-import static org.codehaus.groovy.ast.ClassHelper.make;
 import static org.codehaus.groovy.ast.tools.GeneralUtils.assignS;
 import static org.codehaus.groovy.ast.tools.GeneralUtils.callThisX;
 import static org.codehaus.groovy.ast.tools.GeneralUtils.callX;
@@ -68,6 +67,7 @@ import static org.codehaus.groovy.ast.tools.GenericsUtils.correctToGenericsSpec;
 import static org.codehaus.groovy.ast.tools.GenericsUtils.correctToGenericsSpecRecurse;
 import static org.codehaus.groovy.ast.tools.GenericsUtils.createGenericsSpec;
 import static org.codehaus.groovy.ast.tools.GenericsUtils.extractSuperClassGenerics;
+import static org.codehaus.groovy.ast.tools.ParameterUtils.parametersEqual;
 
 /**
  * Handles generation of code for the <code>@Delegate</code> annotation
@@ -75,12 +75,10 @@ import static org.codehaus.groovy.ast.tools.GenericsUtils.extractSuperClassGener
 @GroovyASTTransformation(phase = CompilePhase.CANONICALIZATION)
 public class DelegateASTTransformation extends AbstractASTTransformation {
 
-    private static final Class MY_CLASS = Delegate.class;
-    private static final ClassNode MY_TYPE = make(MY_CLASS);
+    private static final ClassNode MY_TYPE = ClassHelper.make(Delegate.class);
     private static final String MY_TYPE_NAME = "@" + MY_TYPE.getNameWithoutPackage();
-    private static final ClassNode DEPRECATED_TYPE = make(Deprecated.class);
-    private static final ClassNode GROOVYOBJECT_TYPE = make(GroovyObject.class);
-    private static final ClassNode LAZY_TYPE = make(Lazy.class);
+    private static final ClassNode DEPRECATED_TYPE = ClassHelper.make(Deprecated.class);
+    private static final ClassNode LAZY_TYPE = ClassHelper.make(Lazy.class);
 
     private static final String MEMBER_DEPRECATED = "deprecated";
     private static final String MEMBER_INTERFACES = "interfaces";
@@ -92,7 +90,8 @@ public class DelegateASTTransformation extends AbstractASTTransformation {
     private static final String MEMBER_METHOD_ANNOTATIONS = "methodAnnotations";
     private static final String MEMBER_ALL_NAMES = "allNames";
 
-    public void visit(ASTNode[] nodes, SourceUnit source) {
+    @Override
+    public void visit(final ASTNode[] nodes, final SourceUnit source) {
         init(nodes, source);
 
         AnnotatedNode parent = (AnnotatedNode) nodes[1];
@@ -131,7 +130,7 @@ public class DelegateASTTransformation extends AbstractASTTransformation {
         }
 
         if (delegate != null) {
-            if (delegate.type.equals(ClassHelper.OBJECT_TYPE) || delegate.type.equals(GROOVYOBJECT_TYPE)) {
+            if (delegate.type.equals(ClassHelper.OBJECT_TYPE) || delegate.type.equals(ClassHelper.GROOVY_OBJECT_TYPE)) {
                 addError(MY_TYPE_NAME + " " + delegate.origin + " '" + delegate.name + "' has an inappropriate type: " + delegate.type.getName() +
                         ". Please add an explicit type but not java.lang.Object or groovy.lang.GroovyObject.", parent);
                 return;
@@ -141,26 +140,24 @@ public class DelegateASTTransformation extends AbstractASTTransformation {
                         ". Delegation to own type not supported. Please use a different type.", parent);
                 return;
             }
-            final List<MethodNode> delegateMethods = getAllMethods(delegate.type);
-            for (ClassNode next : delegate.type.getAllInterfaces()) {
-                delegateMethods.addAll(getAllMethods(next));
-            }
 
-            final boolean skipInterfaces = memberHasValue(node, MEMBER_INTERFACES, false);
-            final boolean includeDeprecated = memberHasValue(node, MEMBER_DEPRECATED, true) || (delegate.type.isInterface() && !skipInterfaces);
-            final boolean allNames = memberHasValue(node, MEMBER_ALL_NAMES, true);
+            final boolean skipInterfaces = memberHasValue(node, MEMBER_INTERFACES, Boolean.FALSE);
+            final boolean includeDeprecated = memberHasValue(node, MEMBER_DEPRECATED, Boolean.TRUE) || (delegate.type.isInterface() && !skipInterfaces);
+            final boolean allNames = memberHasValue(node, MEMBER_ALL_NAMES, Boolean.TRUE);
             delegate.excludes = getMemberStringList(node, MEMBER_EXCLUDES);
             delegate.includes = getMemberStringList(node, MEMBER_INCLUDES);
             delegate.excludeTypes = getMemberClassList(node, MEMBER_EXCLUDE_TYPES);
             delegate.includeTypes = getMemberClassList(node, MEMBER_INCLUDE_TYPES);
             checkIncludeExcludeUndefinedAware(node, delegate.excludes, delegate.includes,
                                               delegate.excludeTypes, delegate.includeTypes, MY_TYPE_NAME);
-            if (!checkPropertyOrMethodList(delegate.type, delegate.includes, "includes", node, MY_TYPE_NAME)) return;
             if (!checkPropertyOrMethodList(delegate.type, delegate.excludes, "excludes", node, MY_TYPE_NAME)) return;
+            if (!checkPropertyOrMethodList(delegate.type, delegate.includes, "includes", node, MY_TYPE_NAME)) return;
 
             final List<MethodNode> ownerMethods = getAllMethods(delegate.owner);
+            final List<MethodNode> delegateMethods = filterMethods(collectMethods(delegate.type), delegate, allNames, includeDeprecated);
+
             for (MethodNode mn : delegateMethods) {
-                addDelegateMethod(delegate, ownerMethods, mn, includeDeprecated, allNames);
+                addDelegateMethod(mn, delegate, ownerMethods);
             }
 
             for (PropertyNode prop : getAllProperties(delegate.type)) {
@@ -170,15 +167,19 @@ public class DelegateASTTransformation extends AbstractASTTransformation {
                 addGetterIfNeeded(delegate, prop, name, allNames);
                 addSetterIfNeeded(delegate, prop, name, allNames);
             }
+
             if (delegate.type.isArray()) {
                 boolean skipLength = delegate.excludes != null && (delegate.excludes.contains("length") || delegate.excludes.contains("getLength"));
                 if (!skipLength) {
-                    addGeneratedMethod(delegate.owner, "getLength",
+                    addGeneratedMethod(
+                            delegate.owner,
+                            "getLength",
                             ACC_PUBLIC,
                             ClassHelper.int_TYPE,
                             Parameter.EMPTY_ARRAY,
                             null,
-                            returnS(propX(delegate.getOp, "length")));
+                            returnS(propX(delegate.getOp, "length"))
+                    );
                 }
             }
 
@@ -191,18 +192,83 @@ public class DelegateASTTransformation extends AbstractASTTransformation {
             for (ClassNode iface : allInterfaces) {
                 if (Modifier.isPublic(iface.getModifiers()) && !ownerIfaces.contains(iface)) {
                     final ClassNode[] ifaces = delegate.owner.getInterfaces();
-                    final ClassNode[] newIfaces = new ClassNode[ifaces.length + 1];
-                    for (int i = 0; i < ifaces.length; i++) {
+                    final int nFaces = ifaces.length;
+
+                    final ClassNode[] newIfaces = new ClassNode[nFaces + 1];
+                    for (int i = 0; i < nFaces; i += 1) {
                         newIfaces[i] = correctToGenericsSpecRecurse(genericsSpec, ifaces[i]);
                     }
-                    newIfaces[ifaces.length] = correctToGenericsSpecRecurse(genericsSpec, iface);
+                    newIfaces[nFaces] = correctToGenericsSpecRecurse(genericsSpec, iface);
                     delegate.owner.setInterfaces(newIfaces);
                 }
             }
         }
     }
 
-    private boolean checkPropertyOrMethodList(ClassNode cNode, List<String> propertyNameList, String listName, AnnotationNode anno, String typeName) {
+    private static List<MethodNode> collectMethods(final ClassNode type) {
+        List<MethodNode> methods = new java.util.LinkedList<>(getAllMethods(type));
+        // GROOVY-4320, GROOVY-4516
+        for (ListIterator<MethodNode> it = methods.listIterator(); it.hasNext();) {
+            MethodNode next = it.next();
+            if (next.isPublic() && !next.isAbstract() && next.hasDefaultValue()) {
+                int n = 0;
+                for (Parameter p : next.getParameters())
+                    if (p.hasInitialExpression()) n += 1;
+                for (int i = 1; i <= n; i += 1) { // from Verifier#addDefaultParameters
+                    Parameter[] params = new Parameter[next.getParameters().length - i];
+                    int index = 0, j = 1;
+                    for (Parameter parameter : next.getParameters()) {
+                        if (j > n - i && parameter.hasInitialExpression()) {
+                            j += 1;
+                        } else {
+                            params[index++] = parameter;
+                            if (parameter.hasInitialExpression()) j += 1;
+                        }
+                    }
+
+                    if (methods.stream().noneMatch(mn -> mn.getName().equals(next.getName()) && parametersEqual(mn.getParameters(), params))) {
+                        MethodNode mn = new MethodNode(next.getName(), next.getModifiers(), next.getReturnType(), params, next.getExceptions(), null);
+                        mn.setDeclaringClass(next.getDeclaringClass());
+                        mn.setGenericsTypes(next.getGenericsTypes());
+                        mn.addAnnotations(next.getAnnotations());
+                        it.add(mn);
+                    }
+                }
+            }
+        }
+
+        for (ClassNode face : type.getAllInterfaces()) {
+            methods.addAll(face.getMethods());
+        }
+
+        return methods;
+    }
+
+    private static List<MethodNode> filterMethods(final List<MethodNode> methods, final DelegateDescription delegate, final boolean allNames, final boolean includeDeprecated) {
+        Set<String> groovyObjectMethods = ClassHelper.GROOVY_OBJECT_TYPE.getMethods().stream().map(MethodNode::getTypeDescriptor).collect(toSet());
+        Set<String> javaObjectMethods = ClassHelper.OBJECT_TYPE.getMethods().stream().map(MethodNode::getTypeDescriptor).collect(toSet());
+        Set<String> ownClassMethods = delegate.owner.getMethods().stream().map(MethodNode::getTypeDescriptor).collect(toSet());
+
+        methods.removeIf(candidate -> {
+            if (!candidate.isPublic() || candidate.isStatic() || (candidate.getModifiers () & ACC_SYNTHETIC) != 0) return true;
+
+            if (shouldSkip(candidate.getName(), delegate.excludes, delegate.includes, allNames)) return true;
+
+            if (!includeDeprecated && !candidate.getAnnotations(DEPRECATED_TYPE).isEmpty()) return true;
+
+            if (groovyObjectMethods.contains(candidate.getTypeDescriptor())) return true;
+
+            if (javaObjectMethods.contains(candidate.getTypeDescriptor())) return true;
+
+            if (ownClassMethods.contains(candidate.getTypeDescriptor())) return true;
+
+            return false;
+        });
+
+        return methods;
+    }
+
+    private boolean checkPropertyOrMethodList(final ClassNode cNode, final List<String> propertyNameList, final String listName, final AnnotationNode anno, final String typeName) {
         if (propertyNameList == null || propertyNameList.isEmpty()) {
             return true;
         }
@@ -215,11 +281,10 @@ public class DelegateASTTransformation extends AbstractASTTransformation {
             if ((pNode.getModifiers() & ACC_FINAL) == 0) {
                 mNames.add(getSetterName(name));
             }
-            String capitalized = capitalize(name);
-            mNames.add("get" + capitalized);
-            boolean isPrimBool = pNode.getOriginType().equals(ClassHelper.boolean_TYPE);
-            if (isPrimBool) {
-                mNames.add("is" + capitalized);
+            String suffix = capitalize(name);
+            mNames.add("get" + suffix);
+            if (pNode.getOriginType().equals(ClassHelper.boolean_TYPE)) {
+                mNames.add("is" + suffix);
             }
         }
         for (MethodNode mNode : cNode.getAllDeclaredMethods()) {
@@ -235,12 +300,14 @@ public class DelegateASTTransformation extends AbstractASTTransformation {
         return result;
     }
 
-    private static void addSetterIfNeeded(DelegateDescription delegate, PropertyNode prop, String name, boolean allNames) {
-        String setterName = "set" + Verifier.capitalize(name);
+    private static void addSetterIfNeeded(final DelegateDescription delegate, final PropertyNode prop, final String name, final boolean allNames) {
+        String setterName = "set" + capitalize(name);
         if ((prop.getModifiers() & ACC_FINAL) == 0
                 && delegate.owner.getSetterMethod(setterName) == null && delegate.owner.getProperty(name) == null
                 && !shouldSkipPropertyMethod(name, setterName, delegate.excludes, delegate.includes, allNames)) {
-            addGeneratedMethod(delegate.owner, setterName,
+            addGeneratedMethod(
+                    delegate.owner,
+                    setterName,
                     ACC_PUBLIC,
                     ClassHelper.VOID_TYPE,
                     params(new Parameter(GenericsUtils.nonGeneric(prop.getType()), "value")),
@@ -250,64 +317,69 @@ public class DelegateASTTransformation extends AbstractASTTransformation {
         }
     }
 
-    private static void addGetterIfNeeded(DelegateDescription delegate, PropertyNode prop, String name, boolean allNames) {
+    private static void addGetterIfNeeded(final DelegateDescription delegate, final PropertyNode prop, final String name, final boolean allNames) {
         boolean isPrimBool = prop.getOriginType().equals(ClassHelper.boolean_TYPE);
         // do a little bit of pre-work since Groovy compiler hasn't added property accessors yet
         boolean willHaveGetAccessor = true;
         boolean willHaveIsAccessor = isPrimBool;
-        String suffix = Verifier.capitalize(name);
+        String suffix = capitalize(name);
+        String isserName = "is" + suffix;
+        String getterName = "get" + suffix;
         if (isPrimBool) {
             ClassNode cNode = prop.getDeclaringClass();
-            if (cNode.getGetterMethod("is" + suffix) != null && cNode.getGetterMethod("get" + suffix) == null)
+            if (cNode.getGetterMethod(isserName) != null && cNode.getGetterMethod(getterName) == null)
                 willHaveGetAccessor = false;
-            if (cNode.getGetterMethod("get" + suffix) != null && cNode.getGetterMethod("is" + suffix) == null)
+            if (cNode.getGetterMethod(getterName) != null && cNode.getGetterMethod(isserName) == null)
                 willHaveIsAccessor = false;
         }
         Reference<Boolean> ownerWillHaveGetAccessor = new Reference<Boolean>();
         Reference<Boolean> ownerWillHaveIsAccessor = new Reference<Boolean>();
         extractAccessorInfo(delegate.owner, name, ownerWillHaveGetAccessor, ownerWillHaveIsAccessor);
 
-        for (String prefix : new String[]{"get", "is"}) {
-            String getterName = prefix + suffix;
-            if ((prefix.equals("get") && willHaveGetAccessor && !ownerWillHaveGetAccessor.get()
-                    || prefix.equals("is") && willHaveIsAccessor && !ownerWillHaveIsAccessor.get())
-                    && !shouldSkipPropertyMethod(name, getterName, delegate.excludes, delegate.includes, allNames)) {
-                addGeneratedMethod(delegate.owner, getterName,
-                        ACC_PUBLIC,
-                        GenericsUtils.nonGeneric(prop.getType()),
-                        Parameter.EMPTY_ARRAY,
-                        null,
-                        returnS(propX(delegate.getOp, name)));
-            }
+        if (willHaveGetAccessor && !ownerWillHaveGetAccessor.get()
+                && !shouldSkipPropertyMethod(name, getterName, delegate.excludes, delegate.includes, allNames)) {
+            addGeneratedMethod(
+                    delegate.owner,
+                    getterName,
+                    ACC_PUBLIC,
+                    GenericsUtils.nonGeneric(prop.getType()),
+                    Parameter.EMPTY_ARRAY,
+                    null,
+                    returnS(propX(delegate.getOp, name))
+            );
+        }
+
+        if (willHaveIsAccessor && !ownerWillHaveIsAccessor.get()
+                && !shouldSkipPropertyMethod(name, getterName, delegate.excludes, delegate.includes, allNames)) {
+            addGeneratedMethod(
+                    delegate.owner,
+                    isserName,
+                    ACC_PUBLIC,
+                    GenericsUtils.nonGeneric(prop.getType()),
+                    Parameter.EMPTY_ARRAY,
+                    null,
+                    returnS(propX(delegate.getOp, name))
+            );
         }
     }
 
-    private static void extractAccessorInfo(ClassNode owner, String name, Reference<Boolean> willHaveGetAccessor, Reference<Boolean> willHaveIsAccessor) {
-        String suffix = Verifier.capitalize(name);
+    private static void extractAccessorInfo(final ClassNode owner, final String name, final Reference<Boolean> willHaveGetAccessor, final Reference<Boolean> willHaveIsAccessor) {
+        String suffix = capitalize(name);
         boolean hasGetAccessor = owner.getGetterMethod("get" + suffix) != null;
         boolean hasIsAccessor = owner.getGetterMethod("is" + suffix) != null;
         PropertyNode prop = owner.getProperty(name);
         willHaveGetAccessor.set(hasGetAccessor || (prop != null && !hasIsAccessor));
         willHaveIsAccessor.set(hasIsAccessor || (prop != null && !hasGetAccessor && prop.getOriginType().equals(ClassHelper.boolean_TYPE)));
     }
-    
-    private static boolean shouldSkipPropertyMethod(String propertyName, String methodName, List<String> excludes, List<String> includes, boolean allNames) {
+
+    private static boolean shouldSkipPropertyMethod(final String propertyName, final String methodName, final List<String> excludes, final List<String> includes, final boolean allNames) {
         return ((!allNames && deemedInternalName(propertyName))
-                    || excludes != null && (excludes.contains(propertyName) || excludes.contains(methodName)) 
+                    || excludes != null && (excludes.contains(propertyName) || excludes.contains(methodName))
                     || (includes != null && !includes.isEmpty() && !includes.contains(propertyName) && !includes.contains(methodName)));
     }
 
-    private void addDelegateMethod(DelegateDescription delegate, List<MethodNode> ownMethods, MethodNode candidate, boolean includeDeprecated, boolean allNames) {
-        if (!candidate.isPublic() || candidate.isStatic() || 0 != (candidate.getModifiers () & ACC_SYNTHETIC))
-            return;
-
-        if (!candidate.getAnnotations(DEPRECATED_TYPE).isEmpty() && !includeDeprecated)
-            return;
-
-        if (shouldSkip(candidate.getName(), delegate.excludes, delegate.includes, allNames)) return;
-
-        Map<String,ClassNode> genericsSpec = createGenericsSpec(delegate.owner);
-        genericsSpec = addMethodGenerics(candidate, genericsSpec);
+    private void addDelegateMethod(final MethodNode candidate, final DelegateDescription delegate, final Iterable<MethodNode> ownMethods) {
+        Map<String,ClassNode> genericsSpec = addMethodGenerics(candidate, createGenericsSpec(delegate.owner));
         extractSuperClassGenerics(delegate.type, candidate.getDeclaringClass(), genericsSpec);
 
         if ((delegate.excludeTypes != null && !delegate.excludeTypes.isEmpty()) || delegate.includeTypes != null) {
@@ -317,44 +389,26 @@ public class DelegateASTTransformation extends AbstractASTTransformation {
                 return;
         }
 
-        // ignore methods from GroovyObject
-        for (MethodNode mn : GROOVYOBJECT_TYPE.getMethods()) {
-            if (mn.getTypeDescriptor().equals(candidate.getTypeDescriptor())) {
-                return;
-            }
-        }
-
-        // ignore methods already in owner
-        for (MethodNode mn : delegate.owner.getMethods()) {
-            if (mn.getTypeDescriptor().equals(candidate.getTypeDescriptor())) {
-                return;
-            }
-        }
-
         // give precedence to methods of self (but not abstract or static superclass methods)
         // also allows abstract or static self methods to be selected for overriding but they are ignored later
         MethodNode existingNode = null;
         for (MethodNode mn : ownMethods) {
-            if (mn.getTypeDescriptor().equals(candidate.getTypeDescriptor()) && !mn.isAbstract() && !mn.isStatic()) {
+            if (!mn.isAbstract() && !mn.isStatic() && mn.getTypeDescriptor().equals(candidate.getTypeDescriptor())) {
                 existingNode = mn;
                 break;
             }
         }
         if (existingNode == null || existingNode.getCode() == null) {
-
             final ArgumentListExpression args = new ArgumentListExpression();
             final Parameter[] params = candidate.getParameters();
             final Parameter[] newParams = new Parameter[params.length];
-            List<String> currentMethodGenPlaceholders = genericPlaceholderNames(candidate);
-            for (int i = 0; i < newParams.length; i++) {
+            List<String> currentMethodGenPlaceholders = getGenericPlaceholderNames(candidate);
+            for (int i = 0, n = newParams.length; i < n; i += 1) {
                 ClassNode newParamType = correctToGenericsSpecRecurse(genericsSpec, params[i].getType(), currentMethodGenPlaceholders);
                 Parameter newParam = new Parameter(newParamType, getParamName(params, i, delegate.name));
-                newParam.setInitialExpression(params[i].getInitialExpression());
-
-                if (memberHasValue(delegate.annotation, MEMBER_PARAMETER_ANNOTATIONS, true)) {
+                if (memberHasValue(delegate.annotation, MEMBER_PARAMETER_ANNOTATIONS, Boolean.TRUE)) {
                     newParam.addAnnotations(copyAnnotatedNodeAnnotations(params[i], MY_TYPE_NAME));
                 }
-
                 newParams[i] = newParam;
                 args.addExpression(varX(newParam));
             }
@@ -368,23 +422,25 @@ public class DelegateASTTransformation extends AbstractASTTransformation {
             mce.setImplicitThis(false); // GROOVY-9938
             mce.setSourcePosition(delegate.delegate); // GROOVY-6542
             ClassNode returnType = correctToGenericsSpecRecurse(genericsSpec, candidate.getReturnType(), currentMethodGenPlaceholders);
-            MethodNode newMethod = addGeneratedMethod(delegate.owner, candidate.getName(),
+            MethodNode newMethod = addGeneratedMethod(
+                    delegate.owner,
+                    candidate.getName(),
                     candidate.getModifiers() & (~ACC_ABSTRACT) & (~ACC_NATIVE),
                     returnType,
                     newParams,
                     candidate.getExceptions(),
-                    stmt(mce));
+                    candidate.isVoidMethod() ? stmt(mce) : returnS(mce)
+            );
             newMethod.setGenericsTypes(candidate.getGenericsTypes());
-
-            if (memberHasValue(delegate.annotation, MEMBER_METHOD_ANNOTATIONS, true)) {
+            if (memberHasValue(delegate.annotation, MEMBER_METHOD_ANNOTATIONS, Boolean.TRUE)) {
                 newMethod.addAnnotations(copyAnnotatedNodeAnnotations(candidate, MY_TYPE_NAME, false));
             }
         }
     }
 
-    private static List<String> genericPlaceholderNames(MethodNode candidate) {
+    private static List<String> getGenericPlaceholderNames(final MethodNode candidate) {
         GenericsType[] candidateGenericsTypes = candidate.getGenericsTypes();
-        List<String> names = new ArrayList<String>();
+        List<String> names = new ArrayList<>();
         if (candidateGenericsTypes != null) {
             for (GenericsType gt : candidateGenericsTypes) {
                 names.add(gt.getName());
@@ -393,7 +449,7 @@ public class DelegateASTTransformation extends AbstractASTTransformation {
         return names;
     }
 
-    private static String getParamName(Parameter[] params, int i, String fieldName) {
+    private static String getParamName(final Parameter[] params, final int i, final String fieldName) {
         String name = params[i].getName();
         while(name.equals(fieldName) || clashesWithOtherParams(name, params, i)) {
             name = "_" + name;
@@ -401,8 +457,8 @@ public class DelegateASTTransformation extends AbstractASTTransformation {
         return name;
     }
 
-    private static boolean clashesWithOtherParams(String name, Parameter[] params, int i) {
-        for (int j = 0; j < params.length; j++) {
+    private static boolean clashesWithOtherParams(final String name, final Parameter[] params, final int i) {
+        for (int j = 0, n = params.length; j < n; j += 1) {
             if (i == j) continue;
             if (params[j].getName().equals(name)) return true;
         }
diff --git a/src/test/org/codehaus/groovy/transform/DelegateTransformTest.groovy b/src/test/org/codehaus/groovy/transform/DelegateTransformTest.groovy
index 8e7026d..b61c890 100644
--- a/src/test/org/codehaus/groovy/transform/DelegateTransformTest.groovy
+++ b/src/test/org/codehaus/groovy/transform/DelegateTransformTest.groovy
@@ -18,7 +18,9 @@
  */
 package org.codehaus.groovy.transform
 
+import org.codehaus.groovy.control.CompilerConfiguration
 import org.codehaus.groovy.control.MultipleCompilationErrorsException
+import org.codehaus.groovy.tools.javac.JavaAwareCompilationUnit
 import org.junit.Test
 
 import java.util.concurrent.locks.Lock
@@ -65,19 +67,19 @@ final class DelegateTransformTest {
     void testDelegateExcludes() {
         assertScript '''
             class MapSet {
-                @Delegate(interfaces=false, excludes=["remove","clear"]) Map m = [a: 1]
+                @Delegate(interfaces=false, excludes=['remove','clear']) Map m = [a: 1]
                 @Delegate Set s = new LinkedHashSet([2, 3, 4] as Set) // HashSet not good enough in JDK 1.5
-                String toString() { m.toString() + " " + s }
+                String toString() { m.toString() + ' ' + s }
             }
 
             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] []'
         '''
     }
 
@@ -86,19 +88,19 @@ final class DelegateTransformTest {
         assertScript '''
             @groovy.transform.CompileStatic
             class MapSet {
-                @Delegate(interfaces=false, excludes=["remove","clear"]) Map m = [a: 1]
+                @Delegate(interfaces=false, excludes=['remove','clear']) Map m = [a: 1]
                 @Delegate Set s = new LinkedHashSet([2, 3, 4] as Set)
-                String toString() { m.toString() + " " + s }
+                String toString() { m.toString() + ' ' + s }
             }
 
             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} []'
         '''
     }
 
@@ -250,7 +252,7 @@ final class DelegateTransformTest {
             class C {
                 def foo(){"C->foo()"}
             }
-            assert new B().foo() == "C->foo()"
+            assert new B().foo() == 'C->foo()'
         '''
     }
 
@@ -320,19 +322,19 @@ final class DelegateTransformTest {
                 @Delegate private I5729 delegate
             }
             assert I5729.isAssignableFrom(Delegator1)
-            assert Delegator1.methods*.name.contains("aMethod")
+            assert Delegator1.methods*.name.contains('aMethod')
 
             class Delegator2 {
                 @Delegate(interfaces=false) private I5729 delegate
             }
             assert !I5729.isAssignableFrom(Delegator2)
-            assert !Delegator2.methods*.name.contains("aMethod")
+            assert !Delegator2.methods*.name.contains('aMethod')
 
             class Delegator3 {
                 @Delegate(interfaces=false, deprecated=true) private I5729 delegate
             }
             assert !I5729.isAssignableFrom(Delegator3)
-            assert Delegator3.methods*.name.contains("aMethod")
+            assert Delegator3.methods*.name.contains('aMethod')
         '''
     }
 
@@ -355,10 +357,10 @@ final class DelegateTransformTest {
                 A a = new A()
             }
 
-            def originalMethod = A.getMethod("method", [Object.class] as Class[])
+            def originalMethod = A.getMethod('method', [Object.class] as Class[])
             def originalAnno = originalMethod.parameterAnnotations[0][0]
 
-            def delegateMethod = A_Delegate.getMethod("method", [Object.class] as Class[])
+            def delegateMethod = A_Delegate.getMethod('method', [Object.class] as Class[])
             def delegateAnno = delegateMethod.parameterAnnotations[0][0]
             println delegateMethod.parameterAnnotations
 
@@ -387,10 +389,10 @@ final class DelegateTransformTest {
                 A a = new A()
             }
 
-            def originalMethod = A.getMethod("method", [Object.class] as Class[])
+            def originalMethod = A.getMethod('method', [Object.class] as Class[])
             def originalAnno = originalMethod.declaredAnnotations[0]
 
-            def delegateMethod = A_Delegate.getMethod("method", [Object.class] as Class[])
+            def delegateMethod = A_Delegate.getMethod('method', [Object.class] as Class[])
             def delegateAnno = delegateMethod.declaredAnnotations[1]
 
             assert delegateAnno == originalAnno
@@ -419,10 +421,10 @@ final class DelegateTransformTest {
                 A a = new A()
             }
 
-            def originalMethod = A.getMethod("method", [Object.class] as Class[])
+            def originalMethod = A.getMethod('method', [Object.class] as Class[])
             def originalAnno = originalMethod.parameterAnnotations[0][0]
 
-            def delegateMethod = A_Delegate.getMethod("method", [Object.class] as Class[])
+            def delegateMethod = A_Delegate.getMethod('method', [Object.class] as Class[])
             assert delegateMethod.parameterAnnotations[0].length == 0
         '''
     }
@@ -499,7 +501,7 @@ final class DelegateTransformTest {
 
             def f = new Foo()
             f.foo()
-            assert f.bar + f.baz == "barbaz"
+            assert f.bar + f.baz == 'barbaz'
         '''
     }
 
@@ -522,27 +524,27 @@ final class DelegateTransformTest {
             class OwnedBook {
                 String owner
 
-                @Delegate(includes=["author", "getTitleAndAuthor"])
+                @Delegate(includes=['author', 'getTitleAndAuthor'])
                 Book book
             }
 
-            Book book = new Book(title: "Ulysses", author: "James Joyce")
-            OwnedBook ownedBook = new OwnedBook(owner: "John Smith", book: book)
+            Book book = new Book(title: 'Ulysses', author: 'James Joyce')
+            OwnedBook ownedBook = new OwnedBook(owner: 'John Smith', book: book)
 
-            ownedBook.author = "John Smith"
-            assert book.author == "John Smith"
+            ownedBook.author = 'John Smith'
+            assert book.author == 'John Smith'
 
-            assert ownedBook.getTitleAndAuthor() == "Ulysses : John Smith"
+            assert ownedBook.getTitleAndAuthor() == 'Ulysses : John Smith'
 
             try {
                 ownedBook.getAuthorAndTitle()
-                assert false : "Non-included methods should not be delegated"
+                assert false, 'Non-included methods should not be delegated'
             } catch(groovy.lang.MissingMethodException expected) {
             }
 
             try {
-                ownedBook.title = "Finnegans Wake"
-                assert false : "Non-included properties should not be delegated"
+                ownedBook.title = 'Finnegans Wake'
+                assert false, 'Non-included properties should not be delegated'
             } catch(groovy.lang.MissingPropertyException expected) {
             }
         '''
@@ -622,8 +624,8 @@ final class DelegateTransformTest {
     void testLineNumberInStackTrace() {
         def err = shouldFail '''\
             @groovy.transform.ASTTest(phase=CANONICALIZATION, value={
-                def property = node.getDeclaredField("thingie")
-                def method = node.getDeclaredMethod("blowup")
+                def property = node.getDeclaredField('thingie')
+                def method = node.getDeclaredMethod('blowup')
                 def call = method.code.expression
 
                 assert property.lineNumber > 0
@@ -668,7 +670,7 @@ final class DelegateTransformTest {
                 DelegateMap dm = new DelegateMap()
             }
             def foo = new Foo()
-            assert foo.dm.x == "123"
+            assert foo.dm.x == '123'
         '''
     }
 
@@ -709,7 +711,7 @@ final class DelegateTransformTest {
     void testShouldWorkWithLazyTransform() {
         assertScript '''
             class Foo {
-                private @Delegate @Lazy ArrayList list = ["bar", "baz"]
+                private @Delegate @Lazy ArrayList list = ['bar', 'baz']
                 // fragile: $list is an internal implementation detail that may change
                 def getInternalDelegate() { $list }
             }
@@ -717,7 +719,7 @@ final class DelegateTransformTest {
             def f = new Foo()
             assert f.internalDelegate == null
             assert f.size() == 2
-            assert f.internalDelegate == ["bar", "baz"]
+            assert f.internalDelegate == ['bar', 'baz']
         '''
     }
 
@@ -823,7 +825,7 @@ final class DelegateTransformTest {
             class Bar {
                 String pls
             }
-            assert new Foo(pls: "ok").pls == "ok"
+            assert new Foo(pls: 'ok').pls == 'ok'
         '''
     }
 
@@ -831,15 +833,15 @@ final class DelegateTransformTest {
     void testOwnerMethodPreferredToDelegateMethod() {
         assertScript '''
             class Foo {
-                String pls() { "foo pls" }
+                String pls() { 'foo pls' }
                 @groovy.lang.Delegate
                 Bar bar
             }
 
             class Bar {
-                String pls() { "bar pls" }
+                String pls() { 'bar pls' }
             }
-            assert new Foo(bar: new Bar()).pls() == "foo pls"
+            assert new Foo(bar: new Bar()).pls() == 'foo pls'
         '''
     }
 
@@ -848,10 +850,10 @@ final class DelegateTransformTest {
         assertScript '''
             class BugsMe {
                 @Delegate
-                String[] content = ["foo", "bar"]
+                String[] content = ['foo', 'bar']
             }
 
-            assert new BugsMe().content.join() == "foobar"
+            assert new BugsMe().content.join() == 'foobar'
             assert new BugsMe().content.length == 2
             assert new BugsMe().length == 2
         '''
@@ -871,7 +873,7 @@ final class DelegateTransformTest {
                 }
             }
 
-            new WMap("example", [name: "weird"])
+            new WMap('example', [name: 'weird'])
         '''
 
         assert err.message.contains("Error during @Delegate processing: 'excludes' property or method 'name' does not exist.")
@@ -885,7 +887,7 @@ final class DelegateTransformTest {
                 @Delegate(methodAnnotations = true)
                 private final CompiledClass8825 delegate = new CompiledClass8825()
             }
-            assert new B().s == "456"
+            assert new B().s == '456'
         '''
     }
 
@@ -896,9 +898,9 @@ final class DelegateTransformTest {
                 String name
             }
             class BarDelegate {
-                @Delegate(includes = "getName") Bar bar = new Bar(name: "Baz")
+                @Delegate(includes = 'getName') Bar bar = new Bar(name: 'Baz')
             }
-            assert new BarDelegate().name == "Baz"
+            assert new BarDelegate().name == 'Baz'
         '''
     }
 
@@ -923,19 +925,117 @@ final class DelegateTransformTest {
     void testParameterWithDefaultArgument2() {
         assertScript '''
             class C {
-                def m(x = "x", y = "y", int z) {
-                    "" + x + y + z
+                def m(x = 'x', y = 'y', int z) {
+                    '' + x + y + z
                 }
             }
             class D {
                 @Delegate private C c = new C()
             }
 
-            assert new D().m(1) == "xy1"
-            assert new D().m(1,2) == "1y2"
-            assert new D().m(1,2,3) == "123"
+            assert new D().m(1) == 'xy1'
+            assert new D().m(1,2) == '1y2'
+            assert new D().m(1,2,3) == '123'
         '''
     }
+
+    @Test
+    void testParameterWithDefaultArgument3() {
+        assertScript '''
+            class C {
+                def m(x = 'x', y = why(), int z) {
+                    '' + x + y + z
+                }
+                private why() {
+                    'y'
+                }
+            }
+            class D {
+                @Delegate private C c = new C()
+            }
+
+            assert new D().m(1) == 'xy1'
+            assert new D().m(1,2) == '1y2'
+            assert new D().m(1,2,3) == '123'
+        '''
+    }
+
+    @Test // GROOVY-4320
+    void testParameterWithDefaultArgument4() {
+        assertScript '''
+            interface I {
+                String event(String name)
+                String event(String name, List values)
+            }
+            class C implements I {
+                String event(String name, List values = []) {
+                    return "$name:$values"
+                }
+            }
+            class D {
+                @Delegate private C c = new C() // The method with default parameters "..." defines a method "event(java.lang.String)" that is already defined.
+            }
+
+            String result = new D().event('x')
+            assert result == 'x:[]'
+        '''
+    }
+
+    @Test // GROOVY-4320
+    void testParameterWithDefaultArgument5() {
+        def config = new CompilerConfiguration(
+            targetDirectory: File.createTempDir(),
+            jointCompilationOptions: [memStub: true]
+        )
+        def parentDir = File.createTempDir()
+        try {
+            new File(parentDir, 'p').mkdir()
+
+            def a = new File(parentDir, 'p/I.java')
+            a.write '''
+                package p;
+                import java.util.List;
+                public interface I {
+                    String event(String name);
+                    String event(String name, List values);
+                }
+            '''
+            def b = new File(parentDir, 'p/C.groovy')
+            b.write '''
+                package p
+                class C implements I {
+                    private final I i
+                    C(I i) { this.i = i }
+
+                    String event(String name, List values = []) {
+                        return "$name:$values"
+                    }
+                }
+            '''
+            def c = new File(parentDir, 'p/Main.groovy')
+            c.write '''
+                package p
+                class Main {
+                    @Delegate private C c = new C(this)
+
+                    void test() {
+                        String result = this.event('x')
+                        assert result == 'x:[]'
+                    }
+                }
+            '''
+
+            def loader = new GroovyClassLoader(this.class.classLoader)
+            def cu = new JavaAwareCompilationUnit(config, loader)
+            cu.addSources(a, b, c)
+            cu.compile()
+
+            loader.loadClass('p.Main').newInstance().test()
+        } finally {
+            config.targetDirectory.deleteDir()
+            parentDir.deleteDir()
+        }
+    }
 }
 
 //------------------------------------------------------------------------------