groovy git commit: GROOVY-8703: Unexpected behavior with @NamedVariant on constructor (closes #795)

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

groovy git commit: GROOVY-8703: Unexpected behavior with @NamedVariant on constructor (closes #795)

paulk
Repository: groovy
Updated Branches:
  refs/heads/master b255db420 -> d8c96ecc6


GROOVY-8703: Unexpected behavior with @NamedVariant on constructor (closes #795)


Project: http://git-wip-us.apache.org/repos/asf/groovy/repo
Commit: http://git-wip-us.apache.org/repos/asf/groovy/commit/d8c96ecc
Tree: http://git-wip-us.apache.org/repos/asf/groovy/tree/d8c96ecc
Diff: http://git-wip-us.apache.org/repos/asf/groovy/diff/d8c96ecc

Branch: refs/heads/master
Commit: d8c96ecc647c5f7fc736be8752b0434477e52fd1
Parents: b255db4
Author: Paul King <[hidden email]>
Authored: Wed Sep 12 21:05:47 2018 +1000
Committer: Paul King <[hidden email]>
Committed: Thu Sep 13 16:44:45 2018 +1000

----------------------------------------------------------------------
 .../groovy/groovy/transform/NamedVariant.java   |  53 +++++--
 .../NamedVariantASTTransformation.java          | 147 +++++++++++--------
 2 files changed, 123 insertions(+), 77 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/groovy/blob/d8c96ecc/src/main/groovy/groovy/transform/NamedVariant.java
----------------------------------------------------------------------
diff --git a/src/main/groovy/groovy/transform/NamedVariant.java b/src/main/groovy/groovy/transform/NamedVariant.java
index e36c64a..04faeb5 100644
--- a/src/main/groovy/groovy/transform/NamedVariant.java
+++ b/src/main/groovy/groovy/transform/NamedVariant.java
@@ -40,37 +40,51 @@ import java.lang.annotation.Target;
  * type information. The generated method however contains no business logic
  * so the chance of errors is minimal.
  *
- * Any arguments identified as named arguments will be supplied as
- * part of the map. Any additional arguments are supplied in the normal
- * tuple style.
+ * Any arguments identified as named arguments will be supplied as part of the map.
+ * Any additional arguments are supplied in the normal tuple style.
  *
- * Named arguments are identified in one of three ways:
+ * Named parameters are identified in one of three ways:
  * <ol>
- *     <li>Use one or more {@code @NamedParam} annotations to explicitly identify such arguments</li>
- *     <li>Use one or more {@code @NamedDelegate} annotations to explicitly identify such arguments as
- *     delegate arguments</li>
- *     <li>If no arguments with {@code @NamedParam} or {@code @NamedDelegate} annotations are found the
- *     first argument is assumed to be an implicit named delegate</li>
+ *     <li>Use one or more {@code @NamedParam} annotations to explicitly identify such parameters</li>
+ *     <li>Use one or more {@code @NamedDelegate} annotations to explicitly identify such parameters as
+ *     delegate parameters</li>
+ *     <li>If no parameters with {@code @NamedParam} or {@code @NamedDelegate} annotations are found then:
+ *     <ul>
+ *         <li>If {@code autoDelegate} is false (the default), all parameters are treated as if they were named parameters</li>
+ *         <li>If {@code autoDelegate} is true, the first parameters is treated as if it was a delegate parameter</li>
+ *     </ul>
+ *     </li>
  * </ol>
  * You can also mix and match the {@code @NamedParam} and {@code @NamedDelegate} annotations.
  *
  * Named arguments will be supplied via the map with their property name (configurable via
  * annotation attributes within {@code @NamedParam}) being the key and value being the argument value.
- * For named delegates, any properties of the delegate can become map keys. Duplicate keys across
- * delegates or named parameters are not allowed. Delegate arguments must be
- * compatible with Groovy's {@code as} cast operation from a {@code Map}.
+ * For named delegates, any properties of the delegate can become map keys.
+ * Duplicate keys across delegate properties or named parameters are not allowed.
+ * The type of delegate parameters must be compatible with Groovy's {@code as} cast operation from a {@code Map}.
  *
- * Here is an example using the implicit delegate approach.
+ * Here is an example using implicit named parameters.
  * <pre class="groovyTestCase">
  * import groovy.transform.*
  *
- * {@code @ToString(includeNames=true, includeFields=true)}
+ * {@code @NamedVariant}
+ * int makeSense(int dollars, int cents) {
+ *     100 * dollars + cents
+ * }
+ *
+ * assert makeSense(dollars: 2, cents: 50) == 250
+ * </pre>
+ * Here is an example using a delegate parameter.
+ * <pre class="groovyTestCase">
+ * import groovy.transform.*
+ *
+ * {@code @ToString(includeNames=true)}
  * class Color {
  *     Integer r, g, b
  * }
  *
  * {@code @NamedVariant}
- * String foo(Color shade) {
+ * String foo(@NamedDelegate Color shade) {
  *     shade
  * }
  *
@@ -103,4 +117,11 @@ public @interface NamedVariant {
      * If specified, must match the optional "id" attribute in an applicable {@code VisibilityOptions} annotation.
      */
     String visibilityId() default Undefined.STRING;
-}
\ No newline at end of file
+
+    /**
+     * If true, add an implicit @NamedDelegate to the first parameter if no @NamedDelegate or @NamedParam annotations are found on any parameter.
+     *
+     * @since 2.5.3
+     */
+    boolean autoDelegate() default false;
+}

http://git-wip-us.apache.org/repos/asf/groovy/blob/d8c96ecc/src/main/java/org/codehaus/groovy/transform/NamedVariantASTTransformation.java
----------------------------------------------------------------------
diff --git a/src/main/java/org/codehaus/groovy/transform/NamedVariantASTTransformation.java b/src/main/java/org/codehaus/groovy/transform/NamedVariantASTTransformation.java
index b5e85e7..9db1cf4 100644
--- a/src/main/java/org/codehaus/groovy/transform/NamedVariantASTTransformation.java
+++ b/src/main/java/org/codehaus/groovy/transform/NamedVariantASTTransformation.java
@@ -89,6 +89,7 @@ public class NamedVariantASTTransformation extends AbstractASTTransformation {
             return;
         }
 
+        boolean autoDelegate = memberHasValue(anno, "autoDelegate", true);
         Parameter mapParam = param(GenericsUtils.nonGeneric(ClassHelper.MAP_TYPE), "__namedArgs");
         List<Parameter> genParams = new ArrayList<Parameter>();
         genParams.add(mapParam);
@@ -105,35 +106,15 @@ public class NamedVariantASTTransformation extends AbstractASTTransformation {
             }
         }
 
-        if (!annoFound) {
+        if (!annoFound && autoDelegate) {
             // assume the first param is the delegate by default
             processDelegateParam(mNode, mapParam, args, propNames, fromParams[0]);
         } else {
             for (Parameter fromParam : fromParams) {
-                if (AnnotatedNodeUtils.hasAnnotation(fromParam, NAMED_PARAM_TYPE)) {
-                    AnnotationNode namedParam = fromParam.getAnnotations(NAMED_PARAM_TYPE).get(0);
-                    boolean required = memberHasValue(namedParam, "required", true);
-                    if (getMemberStringValue(namedParam, "value") == null) {
-                        namedParam.addMember("value", constX(fromParam.getName()));
-                    }
-                    String name = getMemberStringValue(namedParam, "value");
-                    if (getMemberValue(namedParam, "type") == null) {
-                        namedParam.addMember("type", classX(fromParam.getType()));
-                    }
-                    if (hasDuplicates(mNode, propNames, name)) return;
-                    // TODO check specified type is assignable from declared param type?
-                    // ClassNode type = getMemberClassValue(namedParam, "type");
-                    if (required) {
-                        if (fromParam.hasInitialExpression()) {
-                            addError("Error during " + MY_TYPE_NAME + " processing. A required parameter can't have an initial value.", mNode);
-                            return;
-                        }
-                        inner.addStatement(new AssertStatement(boolX(callX(varX(mapParam), "containsKey", args(constX(name)))),
-                                plusX(new ConstantExpression("Missing required named argument '" + name + "'. Keys found: "), callX(varX(mapParam), "keySet"))));
-                    }
-                    args.addExpression(propX(varX(mapParam), name));
-                    mapParam.addAnnotation(namedParam);
-                    fromParam.getAnnotations().remove(namedParam);
+                if (!annoFound) {
+                    if (!processImplicitNamedParam(mNode, mapParam, args, propNames, fromParam)) return;
+                } else if (AnnotatedNodeUtils.hasAnnotation(fromParam, NAMED_PARAM_TYPE)) {
+                    if (!processExplicitNamedParam(mNode, mapParam, inner, args, propNames, fromParam)) return;
                 } else if (AnnotatedNodeUtils.hasAnnotation(fromParam, NAMED_DELEGATE_TYPE)) {
                     if (!processDelegateParam(mNode, mapParam, args, propNames, fromParam)) return;
                 } else {
@@ -143,6 +124,86 @@ public class NamedVariantASTTransformation extends AbstractASTTransformation {
                 }
             }
         }
+        createMapVariant(mNode, anno, mapParam, genParams, cNode, inner, args, propNames);
+    }
+
+    private boolean processImplicitNamedParam(MethodNode mNode, Parameter mapParam, ArgumentListExpression args, List<String> propNames, Parameter fromParam) {
+        boolean required = fromParam.hasInitialExpression();
+        String name = fromParam.getName();
+        if (hasDuplicates(mNode, propNames, name)) return false;
+        AnnotationNode namedParam = new AnnotationNode(NAMED_PARAM_TYPE);
+        namedParam.addMember("value", constX(name));
+        namedParam.addMember("type", classX(fromParam.getType()));
+        namedParam.addMember("required", constX(required, true));
+        mapParam.addAnnotation(namedParam);
+        args.addExpression(propX(varX(mapParam), name));
+        return true;
+    }
+
+    private boolean processExplicitNamedParam(MethodNode mNode, Parameter mapParam, BlockStatement inner, ArgumentListExpression args, List<String> propNames, Parameter fromParam) {
+        AnnotationNode namedParam = fromParam.getAnnotations(NAMED_PARAM_TYPE).get(0);
+        boolean required = memberHasValue(namedParam, "required", true);
+        if (getMemberStringValue(namedParam, "value") == null) {
+            namedParam.addMember("value", constX(fromParam.getName()));
+        }
+        String name = getMemberStringValue(namedParam, "value");
+        if (getMemberValue(namedParam, "type") == null) {
+            namedParam.addMember("type", classX(fromParam.getType()));
+        }
+        if (hasDuplicates(mNode, propNames, name)) return false;
+        // TODO check specified type is assignable from declared param type?
+        // ClassNode type = getMemberClassValue(namedParam, "type");
+        if (required) {
+            if (fromParam.hasInitialExpression()) {
+                addError("Error during " + MY_TYPE_NAME + " processing. A required parameter can't have an initial value.", mNode);
+                return false;
+            }
+            inner.addStatement(new AssertStatement(boolX(callX(varX(mapParam), "containsKey", args(constX(name)))),
+                    plusX(new ConstantExpression("Missing required named argument '" + name + "'. Keys found: "), callX(varX(mapParam), "keySet"))));
+        }
+        args.addExpression(propX(varX(mapParam), name));
+        mapParam.addAnnotation(namedParam);
+        fromParam.getAnnotations().remove(namedParam);
+        return true;
+    }
+
+    private boolean processDelegateParam(MethodNode mNode, Parameter mapParam, ArgumentListExpression args, List<String> propNames, Parameter fromParam) {
+        if (isInnerClass(fromParam.getType())) {
+            if (mNode.isStatic()) {
+                addError("Error during " + MY_TYPE_NAME + " processing. Delegate type '" + fromParam.getType().getNameWithoutPackage() + "' is an inner class which is not supported.", mNode);
+                return false;
+            }
+        }
+
+        Set<String> names = new HashSet<String>();
+        List<PropertyNode> props = getAllProperties(names, fromParam.getType(), true, false, false, true, false, true);
+        for (String next : names) {
+            if (hasDuplicates(mNode, propNames, next)) return false;
+        }
+        List<MapEntryExpression> entries = new ArrayList<MapEntryExpression>();
+        for (PropertyNode pNode : props) {
+            String name = pNode.getName();
+            entries.add(new MapEntryExpression(constX(name), propX(varX(mapParam), name)));
+            AnnotationNode namedParam = new AnnotationNode(NAMED_PARAM_TYPE);
+            namedParam.addMember("value", constX(name));
+            namedParam.addMember("type", classX(pNode.getType()));
+            mapParam.addAnnotation(namedParam);
+        }
+        Expression delegateMap = new MapExpression(entries);
+        args.addExpression(castX(fromParam.getType(), delegateMap));
+        return true;
+    }
+
+    private boolean hasDuplicates(MethodNode mNode, List<String> propNames, String next) {
+        if (propNames.contains(next)) {
+            addError("Error during " + MY_TYPE_NAME + " processing. Duplicate property '" + next + "' found.", mNode);
+            return true;
+        }
+        propNames.add(next);
+        return false;
+    }
+
+    private void createMapVariant(MethodNode mNode, AnnotationNode anno, Parameter mapParam, List<Parameter> genParams, ClassNode cNode, BlockStatement inner, ArgumentListExpression args, List<String> propNames) {
         Parameter namedArgKey = param(STRING_TYPE, "namedArgKey");
         inner.addStatement(
                 new ForStatement(
@@ -185,40 +246,4 @@ public class NamedVariantASTTransformation extends AbstractASTTransformation {
             );
         }
     }
-
-    private boolean processDelegateParam(MethodNode mNode, Parameter mapParam, ArgumentListExpression args, List<String> propNames, Parameter fromParam) {
-        if (isInnerClass(fromParam.getType())) {
-            if (mNode.isStatic()) {
-                addError("Error during " + MY_TYPE_NAME + " processing. Delegate type '" + fromParam.getType().getNameWithoutPackage() + "' is an inner class which is not supported.", mNode);
-                return false;
-            }
-        }
-
-        Set<String> names = new HashSet<String>();
-        List<PropertyNode> props = getAllProperties(names, fromParam.getType(), true, false, false, true, false, true);
-        for (String next : names) {
-            if (hasDuplicates(mNode, propNames, next)) return false;
-        }
-        List<MapEntryExpression> entries = new ArrayList<MapEntryExpression>();
-        for (PropertyNode pNode : props) {
-            String name = pNode.getName();
-            entries.add(new MapEntryExpression(constX(name), propX(varX(mapParam), name)));
-            AnnotationNode namedParam = new AnnotationNode(NAMED_PARAM_TYPE);
-            namedParam.addMember("value", constX(name));
-            namedParam.addMember("type", classX(pNode.getType()));
-            mapParam.addAnnotation(namedParam);
-        }
-        Expression delegateMap = new MapExpression(entries);
-        args.addExpression(castX(fromParam.getType(), delegateMap));
-        return true;
-    }
-
-    private boolean hasDuplicates(MethodNode mNode, List<String> propNames, String next) {
-        if (propNames.contains(next)) {
-            addError("Error during " + MY_TYPE_NAME + " processing. Duplicate property '" + next + "' found.", mNode);
-            return true;
-        }
-        propNames.add(next);
-        return false;
-    }
 }
\ No newline at end of file