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/16 22:14:30 UTC
[groovy] branch GROOVY_3_0_X updated: GROOVY-9158, GROOVY-10176: `@NamedVariant`: refacor and add test cases
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 8572c70 GROOVY-9158, GROOVY-10176: `@NamedVariant`: refacor and add test cases
8572c70 is described below
commit 8572c70843bbdb2f004348a3d77e60604a1dfe65
Author: Eric Milles <er...@thomsonreuters.com>
AuthorDate: Wed Feb 16 16:11:10 2022 -0600
GROOVY-9158, GROOVY-10176: `@NamedVariant`: refacor and add test cases
3_0_X backport
---
.../transform/NamedVariantASTTransformation.java | 107 +++++++++------
src/test/groovy/transform/NamedVariantTest.groovy | 86 ------------
.../transform/NamedVariantTransformTest.groovy | 149 ++++++++++++++++++---
3 files changed, 194 insertions(+), 148 deletions(-)
diff --git a/src/main/java/org/codehaus/groovy/transform/NamedVariantASTTransformation.java b/src/main/java/org/codehaus/groovy/transform/NamedVariantASTTransformation.java
index b25bf78..28e0cbc 100644
--- a/src/main/java/org/codehaus/groovy/transform/NamedVariantASTTransformation.java
+++ b/src/main/java/org/codehaus/groovy/transform/NamedVariantASTTransformation.java
@@ -32,8 +32,7 @@ import org.codehaus.groovy.ast.PropertyNode;
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;
@@ -54,6 +53,7 @@ import static org.apache.groovy.ast.tools.VisibilityUtils.getVisibility;
import static org.codehaus.groovy.antlr.PrimitiveHelper.getDefaultValueForPrimitive;
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;
@@ -108,11 +108,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;
}
}
@@ -122,13 +123,13 @@ public class NamedVariantASTTransformation extends AbstractASTTransformation {
} else {
for (Parameter fromParam : fromParams) {
if (!annoFound) {
- if (!processImplicitNamedParam(mNode, mapParam, args, propNames, fromParam, coerce)) return;
+ if (!processImplicitNamedParam(mNode, mapParam, inner, args, propNames, fromParam, coerce)) return;
} else if (AnnotatedNodeUtils.hasAnnotation(fromParam, NAMED_PARAM_TYPE)) {
if (!processExplicitNamedParam(mNode, mapParam, inner, args, propNames, fromParam, coerce)) return;
} else if (AnnotatedNodeUtils.hasAnnotation(fromParam, NAMED_DELEGATE_TYPE)) {
if (!processDelegateParam(mNode, mapParam, args, propNames, fromParam, coerce)) return;
} else {
- VariableExpression arg = varX(fromParam);
+ Expression arg = varX(fromParam);
Expression argOrDefault = fromParam.hasInitialExpression() ? elvisX(arg, fromParam.getDefaultValue()) : arg;
args.addExpression(asType(argOrDefault, fromParam.getType(), coerce));
if (hasDuplicates(mNode, propNames, fromParam.getName())) return;
@@ -139,56 +140,63 @@ public class NamedVariantASTTransformation extends AbstractASTTransformation {
createMapVariant(mNode, anno, mapParam, genParams, cNode, inner, args, propNames);
}
- private boolean processImplicitNamedParam(final MethodNode mNode, final Parameter mapParam, final ArgumentListExpression args, final List<String> propNames, final Parameter fromParam, boolean coerce) {
- boolean required = fromParam.hasInitialExpression();
+ private boolean processImplicitNamedParam(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(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("type", classX(type));
namedParam.addMember("required", constX(required, true));
mapParam.addAnnotation(namedParam);
- PropertyExpression arg = propX(varX(mapParam), name);
- Expression argOrDefault = fromParam.hasInitialExpression() ? elvisX(arg, fromParam.getDefaultValue()) : arg;
- args.addExpression(asType(argOrDefault, fromParam.getType(), coerce));
+
+ if (required) {
+ 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) {
- namedParam.addMember("value", constX(fromParam.getName()));
- }
+
String name = getMemberStringValue(namedParam, "value");
- if (getMemberValue(namedParam, "type") == null) {
- namedParam.addMember("type", classX(fromParam.getType()));
+ if (name == null) {
+ name = fromParam.getName();
+ namedParam.addMember("value", constX(name));
}
if (hasDuplicates(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");
+ if (type == null) {
+ type = fromParam.getType();
+ namedParam.addMember("type", classX(type));
+ } else {
+ // TODO: Check attribute type is assignable to declared param type?
+ }
+
+ boolean required = memberHasValue(namedParam, "required", true);
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<>();
@@ -200,7 +208,7 @@ public class NamedVariantASTTransformation extends AbstractASTTransformation {
for (PropertyNode pNode : props) {
String name = pNode.getName();
// create entry [name: __namedArgs.getOrDefault('name', initialValue)]
- Expression defaultValue = Optional.ofNullable(pNode.getInitialExpression()).orElseGet(() -> getDefaultExpression(pNode.getType()));
+ Expression defaultValue = Optional.ofNullable(pNode.getInitialExpression()).orElseGet(() -> defaultValueX(pNode.getType()));
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);
@@ -213,10 +221,6 @@ public class NamedVariantASTTransformation extends AbstractASTTransformation {
return true;
}
- private Expression getDefaultExpression(ClassNode pType) {
- return Optional.ofNullable(getDefaultValueForPrimitive(pType)).orElse(nullX());
- }
-
private boolean hasDuplicates(final MethodNode mNode, final List<String> propNames, final String next) {
if (propNames.contains(next)) {
addError("Error during " + NAMED_VARIANT + " processing. Duplicate property '" + next + "' found.", mNode);
@@ -236,7 +240,7 @@ public class NamedVariantASTTransformation extends AbstractASTTransformation {
));
Parameter[] genParamsArray = genParams.toArray(Parameter.EMPTY_ARRAY);
- // TODO account for default params giving multiple signatures
+ // TODO: account for default params giving multiple signatures
if (cNode.hasMethod(mNode.getName(), genParamsArray)) {
addError("Error during " + NAMED_VARIANT + " processing. Class " + cNode.getNameWithoutPackage() +
" already has a named-arg " + (mNode instanceof ConstructorNode ? "constructor" : "method") +
@@ -269,11 +273,30 @@ public class NamedVariantASTTransformation extends AbstractASTTransformation {
}
}
- private Expression asType(Expression expression, ClassNode classNode, boolean coerce) {
- if (coerce) {
- return asX(classNode, expression);
- } else {
- return expression;
+ //--------------------------------------------------------------------------
+
+ private static Expression namedParamValue(final Parameter mapParam, final String name, final ClassNode type, final boolean coerce, Expression defaultValue) {
+ Expression value = propX(varX(mapParam), name); // TODO: "map.get(name)"
+ if (defaultValue == null && isPrimitiveType(type)) {
+ defaultValue = defaultValueX(type);
+ }
+ if (defaultValue != null) {
+ value = elvisX(value, defaultValue);
}
+ return asType(value, type, coerce);
+ }
+
+ private static Expression defaultValueX(final ClassNode type) {
+ return Optional.ofNullable(getDefaultValueForPrimitive(type)).orElse(nullX());
+ }
+
+ private static Expression containsKey(final Parameter mapParam, final String name) {
+ MethodCallExpression call = callX(varX(mapParam), "containsKey", constX(name));
+ call.setMethodTarget(MAP_TYPE.getMethods("containsKey").get(0));
+ return call;
+ }
+
+ private static Expression asType(final Expression value, final ClassNode type, final boolean coerce) {
+ return coerce ? asX(type, value) : /*castX(*/value/*)*/;
}
}
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..2ec9135 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,97 @@ 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-9158
+ void testNamedParamWithDefaultArgument() {
+ assertScript '''
+ import groovy.transform.*
+ import static groovy.test.GroovyAssert.shouldFail
+
+ @NamedVariant(coerce=true)
+ Map m(@NamedParam(required=true) String one, @NamedParam String two = 'X') {
+ [one: one, two: two]
+ }
+
+ def map = m('1')
+ assert map.one == '1'
+ assert map.two == 'X'
+
+ map = m('1', '2')
+ assert map.one == '1'
+ assert map.two == '2'
+
+ map = m(one: '1')
+ assert map.one == '1'
+ assert map.two == 'X'
+
+ map = m(one: '1', two: 2)
+ assert map.one == '1'
+ assert map.two == '2'
+
+ shouldFail(AssertionError) {
+ m([:])
+ }
+ shouldFail {
+ m()
+ }
'''
}
+ @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
+ 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'/
+ }
+
@Test // GROOVY-9183
void testNamedDelegateWithPrimitiveValues() {
assertScript '''
@@ -187,21 +294,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 +357,4 @@ final class NamedVariantTransformTest {
assert mapper.settings.firstDataRow == 1
'''
}
-
- static class Storm { String front }
- static class Switch { String back }
}