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/15 23:44:17 UTC
[groovy] branch GROOVY-10484 created (now 8e4d7d8)
This is an automated email from the ASF dual-hosted git repository.
emilles pushed a change to branch GROOVY-10484
in repository https://gitbox.apache.org/repos/asf/groovy.git.
at 8e4d7d8 GROOVY-9158, GROOVY-10176, GROOVY-10484: NamedParam checking enhancement
This branch includes the following new commits:
new 8e4d7d8 GROOVY-9158, GROOVY-10176, GROOVY-10484: NamedParam checking enhancement
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-9158, GROOVY-10176, GROOVY-10484: NamedParam checking enhancement
Posted by em...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
emilles pushed a commit to branch GROOVY-10484
in repository https://gitbox.apache.org/repos/asf/groovy.git
commit 8e4d7d826e6c08614a894b79f00e304796774aef
Author: Eric Milles <er...@thomsonreuters.com>
AuthorDate: Tue Feb 15 17:44:01 2022 -0600
GROOVY-9158, GROOVY-10176, GROOVY-10484: NamedParam checking enhancement
---
.../transform/NamedVariantASTTransformation.java | 95 +++++++++------
src/test/groovy/transform/NamedVariantTest.groovy | 86 --------------
.../transform/NamedVariantTransformTest.groovy | 132 +++++++++++++++++----
3 files changed, 171 insertions(+), 142 deletions(-)
diff --git a/src/main/java/org/codehaus/groovy/transform/NamedVariantASTTransformation.java b/src/main/java/org/codehaus/groovy/transform/NamedVariantASTTransformation.java
index 7cda805..31d9c00 100644
--- a/src/main/java/org/codehaus/groovy/transform/NamedVariantASTTransformation.java
+++ b/src/main/java/org/codehaus/groovy/transform/NamedVariantASTTransformation.java
@@ -20,6 +20,7 @@ package org.codehaus.groovy.transform;
import groovy.transform.NamedDelegate;
import groovy.transform.NamedParam;
+import groovy.transform.NamedParams;
import groovy.transform.NamedVariant;
import org.apache.groovy.ast.tools.AnnotatedNodeUtils;
import org.codehaus.groovy.ast.ASTNode;
@@ -29,16 +30,16 @@ import org.codehaus.groovy.ast.ConstructorNode;
import org.codehaus.groovy.ast.MethodNode;
import org.codehaus.groovy.ast.Parameter;
import org.codehaus.groovy.ast.PropertyNode;
+import org.codehaus.groovy.ast.Variable;
+import org.codehaus.groovy.ast.expr.AnnotationConstantExpression;
import org.codehaus.groovy.ast.expr.ArgumentListExpression;
import org.codehaus.groovy.ast.expr.Expression;
import org.codehaus.groovy.ast.expr.MapEntryExpression;
-import org.codehaus.groovy.ast.expr.PropertyExpression;
-import org.codehaus.groovy.ast.expr.VariableExpression;
+import org.codehaus.groovy.ast.expr.MethodCallExpression;
import org.codehaus.groovy.ast.stmt.AssertStatement;
import org.codehaus.groovy.ast.stmt.BlockStatement;
import org.codehaus.groovy.ast.stmt.ForStatement;
import org.codehaus.groovy.ast.tools.GenericsUtils;
-import org.codehaus.groovy.classgen.asm.util.TypeUtil;
import org.codehaus.groovy.control.CompilePhase;
import org.codehaus.groovy.control.SourceUnit;
@@ -48,12 +49,14 @@ import java.util.List;
import java.util.Optional;
import java.util.Set;
+import static java.util.stream.Collectors.toList;
import static org.apache.groovy.ast.tools.ClassNodeUtils.addGeneratedConstructor;
import static org.apache.groovy.ast.tools.ClassNodeUtils.addGeneratedMethod;
import static org.apache.groovy.ast.tools.ClassNodeUtils.isInnerClass;
import static org.apache.groovy.ast.tools.VisibilityUtils.getVisibility;
import static org.codehaus.groovy.ast.ClassHelper.MAP_TYPE;
import static org.codehaus.groovy.ast.ClassHelper.STRING_TYPE;
+import static org.codehaus.groovy.ast.ClassHelper.isPrimitiveType;
import static org.codehaus.groovy.ast.ClassHelper.make;
import static org.codehaus.groovy.ast.ClassHelper.makeWithoutCaching;
import static org.codehaus.groovy.ast.tools.GeneralUtils.args;
@@ -70,6 +73,7 @@ import static org.codehaus.groovy.ast.tools.GeneralUtils.elvisX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.entryX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.getAllProperties;
import static org.codehaus.groovy.ast.tools.GeneralUtils.list2args;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.listX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.mapX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.param;
import static org.codehaus.groovy.ast.tools.GeneralUtils.plusX;
@@ -99,8 +103,8 @@ public class NamedVariantASTTransformation extends AbstractASTTransformation {
return;
}
- boolean autoDelegate = memberHasValue(anno, "autoDelegate", true);
boolean coerce = memberHasValue(anno, "coerce", true);
+ boolean autoDelegate = memberHasValue(anno, "autoDelegate", true);
Parameter mapParam = param(GenericsUtils.nonGeneric(MAP_TYPE), "__namedArgs");
List<Parameter> genParams = new ArrayList<>();
genParams.add(mapParam);
@@ -109,11 +113,12 @@ public class NamedVariantASTTransformation extends AbstractASTTransformation {
ArgumentListExpression args = new ArgumentListExpression();
List<String> propNames = new ArrayList<>();
- // first pass, just check for absence of annotations of interest
+ // first pass, just check for annotations of interest
boolean annoFound = false;
for (Parameter fromParam : fromParams) {
if (AnnotatedNodeUtils.hasAnnotation(fromParam, NAMED_PARAM_TYPE) || AnnotatedNodeUtils.hasAnnotation(fromParam, NAMED_DELEGATE_TYPE)) {
annoFound = true;
+ break;
}
}
@@ -129,39 +134,47 @@ public class NamedVariantASTTransformation extends AbstractASTTransformation {
} else if (AnnotatedNodeUtils.hasAnnotation(fromParam, NAMED_DELEGATE_TYPE)) {
if (!processDelegateParam(mNode, mapParam, args, propNames, fromParam, coerce)) return;
} else {
- VariableExpression arg = varX(fromParam);
- Expression argOrDefault = fromParam.hasInitialExpression() ? elvisX(arg, fromParam.getDefaultValue()) : arg;
- args.addExpression(asType(argOrDefault, fromParam.getType(), coerce));
+ Expression optionalValue = fromParam.hasInitialExpression() ? elvisX(varX(fromParam), fromParam.getInitialExpression()) : varX(fromParam);
+ args.addExpression(asType(optionalValue, fromParam.getType(), coerce));
if (hasDuplicates(this, mNode, propNames, fromParam.getName())) return;
genParams.add(fromParam);
}
}
}
+ collateNamedParamAnnotations(mapParam); // GROOVY-10484
createMapVariant(this, mNode, anno, mapParam, genParams, cNode, inner, args, propNames);
}
- static boolean processImplicitNamedParam(final ErrorCollecting xform, final MethodNode mNode, final Parameter mapParam, final BlockStatement inner, final ArgumentListExpression args, final List<String> propNames, final Parameter fromParam, boolean coerce) {
- boolean required = !fromParam.hasInitialExpression();
+ private static void collateNamedParamAnnotations(final Parameter mapParam) {
+ AnnotationNode namedParams = new AnnotationNode(make(NamedParams.class));
+ List<AnnotationNode> mapParamAnnotations = mapParam.getAnnotations();
+ namedParams.addMember("value", listX(mapParamAnnotations.stream()
+ .map(AnnotationConstantExpression::new).collect(toList())));
+ mapParamAnnotations.clear();
+ mapParamAnnotations.add(namedParams);
+ }
+
+ static boolean processImplicitNamedParam(final ErrorCollecting xform, final MethodNode mNode, final Parameter mapParam, final BlockStatement inner, final ArgumentListExpression args, final List<String> propNames, final Parameter fromParam, final boolean coerce) {
String name = fromParam.getName();
+ ClassNode type = fromParam.getType();
+ boolean required = !fromParam.hasInitialExpression();
if (hasDuplicates(xform, mNode, propNames, name)) return false;
+
AnnotationNode namedParam = new AnnotationNode(NAMED_PARAM_TYPE);
- ClassNode type = fromParam.getType();
- namedParam.addMember("value", constX(name));
namedParam.addMember("type", classX(type));
+ namedParam.addMember("value", constX(name));
namedParam.addMember("required", constX(required, true));
mapParam.addAnnotation(namedParam);
- PropertyExpression arg = propX(varX(mapParam), name);
- Expression fallback = fromParam.getDefaultValue() == null && TypeUtil.isPrimitiveType(type) ? defaultValueX(type) : fromParam.getDefaultValue();
- Expression argOrDefault = required && !TypeUtil.isPrimitiveType(type) ? arg : ternaryX(arg, arg, fallback);
- args.addExpression(asType(argOrDefault, type, coerce));
+
if (required) {
- inner.addStatement(new AssertStatement(boolX(callX(varX(mapParam), "containsKey", args(constX(name)))),
+ inner.addStatement(new AssertStatement(boolX(containsKey(mapParam, name)),
plusX(constX("Missing required named argument '" + name + "'. Keys found: "), callX(varX(mapParam), "keySet"))));
}
+ args.addExpression(namedParamValue(mapParam, name, type, coerce, fromParam.getInitialExpression()));
return true;
}
- private boolean processExplicitNamedParam(final MethodNode mNode, final Parameter mapParam, final BlockStatement inner, final ArgumentListExpression args, final List<String> propNames, final Parameter fromParam, boolean coerce) {
+ private boolean processExplicitNamedParam(final MethodNode mNode, final Parameter mapParam, final BlockStatement inner, final ArgumentListExpression args, final List<String> propNames, final Parameter fromParam, final boolean coerce) {
AnnotationNode namedParam = fromParam.getAnnotations(NAMED_PARAM_TYPE).get(0);
boolean required = memberHasValue(namedParam, "required", true);
if (getMemberStringValue(namedParam, "value") == null) {
@@ -173,29 +186,25 @@ public class NamedVariantASTTransformation extends AbstractASTTransformation {
}
if (hasDuplicates(this, mNode, propNames, name)) return false;
// TODO: Check specified type is assignable from declared param type?
- //ClassNode type = getMemberClassValue(namedParam, "type");
+ ClassNode type = getMemberClassValue(namedParam, "type", fromParam.getType());
if (required) {
if (fromParam.hasInitialExpression()) {
- addError("Error during " + NAMED_VARIANT + " processing. A required parameter can't have an initial value.", mNode);
+ addError("Error during " + NAMED_VARIANT + " processing. A required parameter can't have an initial value.", fromParam);
return false;
}
- inner.addStatement(new AssertStatement(boolX(callX(varX(mapParam), "containsKey", args(constX(name)))),
+ inner.addStatement(new AssertStatement(boolX(containsKey(mapParam, name)),
plusX(constX("Missing required named argument '" + name + "'. Keys found: "), callX(varX(mapParam), "keySet"))));
}
- PropertyExpression arg = propX(varX(mapParam), name);
- Expression argOrDefault = fromParam.hasInitialExpression() ? elvisX(arg, fromParam.getDefaultValue()) : arg;
- args.addExpression(asType(argOrDefault, fromParam.getType(), coerce));
+ args.addExpression(namedParamValue(mapParam, name, type, coerce, fromParam.getInitialExpression()));
mapParam.addAnnotation(namedParam);
fromParam.getAnnotations().remove(namedParam);
return true;
}
- private boolean processDelegateParam(final MethodNode mNode, final Parameter mapParam, final ArgumentListExpression args, final List<String> propNames, final Parameter fromParam, boolean coerce) {
- if (isInnerClass(fromParam.getType())) {
- if (mNode.isStatic()) {
- addError("Error during " + NAMED_VARIANT + " processing. Delegate type '" + fromParam.getType().getNameWithoutPackage() + "' is an inner class which is not supported.", mNode);
- return false;
- }
+ private boolean processDelegateParam(final MethodNode mNode, final Parameter mapParam, final ArgumentListExpression args, final List<String> propNames, final Parameter fromParam, final boolean coerce) {
+ if (isInnerClass(fromParam.getType()) && mNode.isStatic()) {
+ addError("Error during " + NAMED_VARIANT + " processing. Delegate type '" + fromParam.getType().getNameWithoutPackage() + "' is an inner class which is not supported.", mNode);
+ return false;
}
Set<String> names = new HashSet<>();
@@ -211,8 +220,8 @@ public class NamedVariantASTTransformation extends AbstractASTTransformation {
entries.add(entryX(constX(name), asType(callX(varX(mapParam), "getOrDefault", args(constX(name), defaultValue)), pNode.getType(), coerce)));
// create annotation @NamedParam(value='name', type=DelegateType)
AnnotationNode namedParam = new AnnotationNode(NAMED_PARAM_TYPE);
- namedParam.addMember("value", constX(name));
namedParam.addMember("type", classX(pNode.getType()));
+ namedParam.addMember("value", constX(name));
mapParam.addAnnotation(namedParam);
}
Expression delegateMap = mapX(entries);
@@ -272,11 +281,25 @@ public class NamedVariantASTTransformation extends AbstractASTTransformation {
}
}
- private static Expression asType(Expression expression, ClassNode classNode, boolean coerce) {
- if (coerce) {
- return asX(classNode, expression);
- } else {
- return expression;
+ private static Expression namedParamValue(final Variable map, final String name, final ClassNode type, final boolean coerce, Expression defaultValue) {
+ Expression value = propX(varX(map), name); // TODO: "map.get(name)"
+ if (defaultValue == null && isPrimitiveType(type)) {
+ defaultValue = defaultValueX(type);
}
+ if (defaultValue != null) {
+ value = ternaryX(containsKey(map, name), value, defaultValue);
+ }
+ return asType(value, type, coerce);
+ }
+
+ private static Expression containsKey(final Variable map, final String name) {
+ MethodCallExpression call = callX(varX(map), "containsKey", constX(name));
+ call.setImplicitThis(false); // required for use before super ctor call
+ call.setMethodTarget(MAP_TYPE.getMethods("containsKey").get(0));
+ return call;
+ }
+
+ private static Expression asType(final Expression exp, final ClassNode type, final boolean coerce) {
+ return coerce ? asX(type, exp) : exp;
}
}
diff --git a/src/test/groovy/transform/NamedVariantTest.groovy b/src/test/groovy/transform/NamedVariantTest.groovy
deleted file mode 100644
index 3696ec0..0000000
--- a/src/test/groovy/transform/NamedVariantTest.groovy
+++ /dev/null
@@ -1,86 +0,0 @@
-/*
- * 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.transform
-
-import java.lang.reflect.Modifier
-import groovy.test.GroovyTestCase
-
-/**
- * Unit tests for the NamedVariant annotation
- */
-class NamedVariantTest extends GroovyTestCase {
- void testMethod() {
- def tester = new GroovyClassLoader().parseClass(
- '''class MyClass {
- | @groovy.transform.NamedVariant
- | void run(int number) {
- | }
- |}'''.stripMargin())
- // Should have such method `void run(Map)`
- def method = tester.getDeclaredMethod("run", Map)
- assert method
- assert Modifier.isPublic(method.modifiers)
- assert method.returnType == void.class
- }
-
- void testMethodCall() {
- def tester = new GroovyClassLoader().parseClass(
- '''class MyClass {
- | @groovy.transform.NamedVariant
- | int run(int number) {
- | number
- | }
- |}'''.stripMargin()).getConstructor().newInstance()
-
- assert tester.run(number: 123) == 123
- try {
- tester.run(number: "123")
- } catch (MissingMethodException ignored) {
- return
- }
- fail("Should have thrown MissingMethodException")
- }
-
- void testCoerceMethodCall() {
- def tester = new GroovyClassLoader().parseClass(
- '''class MyClass {
- | @groovy.transform.NamedVariant(coerce = true)
- | int run(int number) {
- | number
- | }
- |}'''.stripMargin()).getConstructor().newInstance()
-
- assert tester.run(number: 123) == 123
- assert tester.run(number: "123") == 123
- }
-
- void testStaticCoerceMethodCall() {
- def tester = new GroovyClassLoader().parseClass(
- '''@groovy.transform.CompileStatic
- |class MyClass {
- | @groovy.transform.NamedVariant(coerce = true)
- | int run(int number) {
- | number
- | }
- |}'''.stripMargin()).getConstructor().newInstance()
-
- assert tester.run(number: 123) == 123
- assert tester.run(number: "123") == 123
- }
-}
diff --git a/src/test/org/codehaus/groovy/transform/NamedVariantTransformTest.groovy b/src/test/org/codehaus/groovy/transform/NamedVariantTransformTest.groovy
index edb22d6..25afbd6 100644
--- a/src/test/org/codehaus/groovy/transform/NamedVariantTransformTest.groovy
+++ b/src/test/org/codehaus/groovy/transform/NamedVariantTransformTest.groovy
@@ -22,6 +22,7 @@ import groovy.transform.CompileStatic
import org.junit.Test
import static groovy.test.GroovyAssert.assertScript
+import static groovy.test.GroovyAssert.shouldFail
/**
* Tests for the {@code @NamedVariant} transformation.
@@ -30,6 +31,30 @@ import static groovy.test.GroovyAssert.assertScript
final class NamedVariantTransformTest {
@Test
+ void testMethod() {
+ assertScript '''
+ import groovy.transform.*
+ import org.codehaus.groovy.ast.*
+
+ @ASTTest(phase=CANONICALIZATION, value={
+ def method = node.getMethod('m', new Parameter(ClassHelper.MAP_TYPE, 'map'))
+ use(org.apache.groovy.ast.tools.AnnotatedNodeUtils) {
+ assert method.isPublic()
+ assert method.isGenerated()
+ assert method.returnType == ClassHelper.int_TYPE
+ }
+ })
+ class C {
+ @NamedVariant
+ int m(int n) {
+ return n
+ }
+ }
+ assert new C().m(n:42) == 42
+ '''
+ }
+
+ @Test
void testNamedParam() {
assertScript '''
import groovy.transform.*
@@ -77,7 +102,7 @@ final class NamedVariantTransformTest {
@Test
void testNamedParamConstructor() {
- assertScript """
+ assertScript '''
import groovy.transform.*
@ToString(includeNames=true, includeFields=true)
@@ -92,20 +117,19 @@ final class NamedVariantTransformTest {
}
assert new Color(r: 10, g: 20, b: 30).toString() == 'Color(r:10, g:20, b:30)'
- """
+ '''
}
@Test
- void testNamedParamConstructorVisibility() {
- assertScript """
+ void testConstructorVisibility() {
+ assertScript '''
import groovy.transform.*
import static groovy.transform.options.Visibility.*
class Color {
private int r, g, b
- @VisibilityOptions(PUBLIC)
- @NamedVariant
+ @NamedVariant @VisibilityOptions(PUBLIC)
private Color(@NamedParam int r, @NamedParam int g, @NamedParam int b) {
this.r = r
this.g = g
@@ -116,7 +140,7 @@ final class NamedVariantTransformTest {
def pubCons = Color.constructors
assert pubCons.size() == 1
assert pubCons[0].parameterTypes[0] == Map
- """
+ '''
}
@Test
@@ -158,14 +182,80 @@ final class NamedVariantTransformTest {
void testGeneratedMethodsSkipped() {
assertScript '''
import groovy.transform.*
- import static org.codehaus.groovy.transform.NamedVariantTransformTest.*
+
+ class Storm { String front }
+ class Switch { String back }
@NamedVariant
- def baz(@NamedDelegate Storm storm_, @NamedDelegate Switch switch_) { storm_.front + switch_.back }
- assert baz(front: 'Hello', back: 'World') == 'HelloWorld'
+ def foo(@NamedDelegate Storm storm_, @NamedDelegate Switch switch_) { storm_.front + switch_.back }
+ assert foo(front: 'Hello', back: 'World') == 'HelloWorld'
+ '''
+ }
+
+ @Test // GROOVY-10176
+ void testNamedParamWithPrimitiveValues() {
+ assertScript '''
+ import groovy.transform.*
+
+ @ToString(includeNames=true)
+ class Color {
+ int r, g, b
+ }
+
+ @NamedVariant
+ String m(Color color, int alpha = 0) {
+ return [color, alpha].join(' ')
+ }
+
+ @TypeChecked
+ def test() {
+ m(color: new Color(r:1,g:2,b:3))
+ }
+ test()
+
+ String result = m(color: new Color(r:1,g:2,b:3))
+ assert result == 'Color(r:1, g:2, b:3) 0'
'''
}
+ @Test // GROOVY-10484
+ void testNamedParamRequiredVersusOptional() {
+ def err = shouldFail '''
+ import groovy.transform.*
+
+ class Color {
+ int r, g, b
+ }
+
+ @NamedVariant
+ String m(Color color, int alpha = 0) {
+ return [color, alpha].join(' ')
+ }
+
+ m(alpha: 123)
+ '''
+ assert err =~ /Missing required named argument 'color'/
+
+ err = shouldFail '''
+ import groovy.transform.*
+
+ class Color {
+ int r, g, b
+ }
+
+ @NamedVariant
+ String m(Color color, int alpha = 0) {
+ return [color, alpha].join(' ')
+ }
+
+ @TypeChecked
+ void test() {
+ m(alpha: 123)
+ }
+ '''
+ assert err =~ /required named param 'color' not found/
+ }
+
@Test // GROOVY-9183
void testNamedDelegateWithPrimitiveValues() {
assertScript '''
@@ -187,21 +277,26 @@ final class NamedVariantTransformTest {
'''
}
- @Test // GROOVY-GROOVY-10261
- void testNamedDelegateWithDefaultValues() {
+ @Test // GROOVY-10261
+ void testNamedDelegateWithDefaultArguments() {
assertScript '''
import groovy.transform.*
- import java.awt.Color
+
+ @TupleConstructor(defaults=false)
+ @ToString(includeNames=true)
+ class Color {
+ int r, g, b
+ }
@NamedVariant
Color makeColor(int r=10, int g=20, int b=30) {
new Color(r, g, b)
}
- assert makeColor(r: 128, g: 128, b: 5).toString() == 'java.awt.Color[r=128,g=128,b=5]'
- assert makeColor(r: 128, g: 128).toString() == 'java.awt.Color[r=128,g=128,b=30]'
- assert makeColor(r: 128).toString() == 'java.awt.Color[r=128,g=20,b=30]'
- assert makeColor().toString() == 'java.awt.Color[r=10,g=20,b=30]'
+ assert makeColor(r: 128, g: 128, b: 5).toString() == 'Color(r:128, g:128, b:5)'
+ assert makeColor(r: 128, g: 128).toString() == 'Color(r:128, g:128, b:30)'
+ assert makeColor(r: 128).toString() == 'Color(r:128, g:20, b:30)'
+ assert makeColor().toString() == 'Color(r:10, g:20, b:30)'
'''
}
@@ -245,7 +340,4 @@ final class NamedVariantTransformTest {
assert mapper.settings.firstDataRow == 1
'''
}
-
- static class Storm { String front }
- static class Switch { String back }
}