You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@groovy.apache.org by pa...@apache.org on 2021/11/15 23:02:05 UTC
[groovy] branch master updated: GROOVY-10361: TupleConstructor could be improved to have a smarter mode for handling default values
This is an automated email from the ASF dual-hosted git repository.
paulk pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/groovy.git
The following commit(s) were added to refs/heads/master by this push:
new cdcdc5e GROOVY-10361: TupleConstructor could be improved to have a smarter mode for handling default values
cdcdc5e is described below
commit cdcdc5eacf2abb3801d97679d8ffebae50181779
Author: Paul King <pa...@asert.com.au>
AuthorDate: Mon Nov 15 22:50:50 2021 +1000
GROOVY-10361: TupleConstructor could be improved to have a smarter mode for handling default values
---
src/main/groovy/groovy/transform/RecordType.groovy | 2 +-
src/main/java/groovy/transform/DefaultsMode.java | 44 +++++++++++
.../java/groovy/transform/TupleConstructor.java | 31 ++++++--
.../groovy/classgen/AnnotationVisitor.java | 2 +
.../transform/AutoCloneASTTransformation.java | 14 +++-
.../TupleConstructorASTTransformation.java | 87 +++++++++++++++++-----
.../transform/TupleConstructorTransformTest.groovy | 52 +++++++++++++
7 files changed, 206 insertions(+), 26 deletions(-)
diff --git a/src/main/groovy/groovy/transform/RecordType.groovy b/src/main/groovy/groovy/transform/RecordType.groovy
index 3e50833..a164156 100644
--- a/src/main/groovy/groovy/transform/RecordType.groovy
+++ b/src/main/groovy/groovy/transform/RecordType.groovy
@@ -77,7 +77,7 @@ import java.lang.annotation.Target
*/
@RecordBase
@RecordOptions
-@TupleConstructor(namedVariant = true, force = true)
+@TupleConstructor(namedVariant = true, force = true, defaultsMode = DefaultsMode.AUTO)
@PropertyOptions
@KnownImmutable
@POJO
diff --git a/src/main/java/groovy/transform/DefaultsMode.java b/src/main/java/groovy/transform/DefaultsMode.java
new file mode 100644
index 0000000..c75dfa2
--- /dev/null
+++ b/src/main/java/groovy/transform/DefaultsMode.java
@@ -0,0 +1,44 @@
+/*
+ * 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;
+
+/**
+ * Intended mode to use when generating constructors to emulate default parameter values when using the {@link @TupleConstructor} annotation.
+ *
+ * @since 4.0.0
+ * @see TupleConstructor
+ */
+public enum DefaultsMode {
+ /**
+ * Produce a single constructor corresponding to the complete list of properties/fields of the class being compiled.
+ */
+ OFF,
+
+ /**
+ * Produce multiple constructors as required to handle any parameters with explicit initial values.
+ * Stops at the first parameter from the right which has no such explicit value.
+ */
+ AUTO,
+
+ /**
+ * Produce multiple constructors as required from all parameters through to the no-arg constructor.
+ * Defaults will be set according to any explicit initial values or the default value otherwise.
+ */
+ ON
+}
diff --git a/src/main/java/groovy/transform/TupleConstructor.java b/src/main/java/groovy/transform/TupleConstructor.java
index e0527da..dfc3f65 100644
--- a/src/main/java/groovy/transform/TupleConstructor.java
+++ b/src/main/java/groovy/transform/TupleConstructor.java
@@ -276,24 +276,43 @@ public @interface TupleConstructor {
/**
* Used to set whether default value processing is enabled (the default) or disabled.
+ * Ignored if an explicit value is given for {@code defaultsMode()}.
*
- * By default, every constructor parameter is given a default value. This value will
- * be Java's default for primitive types (zero or false) and null for Objects, unless
- * an initial value is given when declaring the property or field. A consequence of
+ * By default, every constructor parameter is given a default value.
+ * This is the equivalent of {@link DefaultsMode#ON}.
+ *
+ * When set to false, default values are not allowed for properties and fields.
+ * This is the equivalent of {@link DefaultsMode#OFF}.
+ *
+ * @since 2.5.0
+ */
+ boolean defaults() default true;
+
+ /**
+ * Used to set the mode for default value processing.
+ *
+ * When set to {@code ON} (the default value), every constructor parameter is given a default value.
+ * This value will be Java's default for primitive types (zero or false) and null for Objects,
+ * unless an initial value is given when declaring the property or field. A consequence of
* this design is that you can leave off parameters from the right if the default
* value will suffice. As far as Java interoperability is concerned, Groovy will
* create additional constructors under the covers representing the constructors
* with parameters left off, all the way from the constructor with all arguments
* to the no-arg constructor.
*
- * However, when set to false, default values are not allowed for properties and fields.
+ * When set to {@code AUTO}, default values are catered for where explicit
+ * default values are given for the respective property/field.
+ * Default values are processed from the right and processing stops when the
+ * first property/field is found without an explicit deault value.
+ *
+ * When set to {@code OFF}, default values are not allowed for properties and fields.
* Only the constructor containing all arguments will be provided.
* In particular, a no-arg constructor won't be provided and since this is currently
* used by Groovy when using named-arguments, the named-argument style won't be available.
*
- * @since 2.5.0
+ * @since 4.0.0
*/
- boolean defaults() default true;
+ DefaultsMode defaultsMode() default DefaultsMode.ON;
/**
* By default, properties are set directly using their respective field.
diff --git a/src/main/java/org/codehaus/groovy/classgen/AnnotationVisitor.java b/src/main/java/org/codehaus/groovy/classgen/AnnotationVisitor.java
index a768271..de0d332 100644
--- a/src/main/java/org/codehaus/groovy/classgen/AnnotationVisitor.java
+++ b/src/main/java/org/codehaus/groovy/classgen/AnnotationVisitor.java
@@ -195,6 +195,8 @@ public class AnnotationVisitor {
} else if (attrType.isDerivedFrom(ClassHelper.Enum_Type)) {
if (attrExp instanceof PropertyExpression) {
visitEnumExpression(attrName, (PropertyExpression) attrExp, attrType);
+ } else if (attrExp instanceof ConstantExpression) {
+ visitConstantExpression(attrName, getConstantExpression(attrExp, attrType), attrType);
} else {
addError("Expected enum value for attribute " + attrName, attrExp);
}
diff --git a/src/main/java/org/codehaus/groovy/transform/AutoCloneASTTransformation.java b/src/main/java/org/codehaus/groovy/transform/AutoCloneASTTransformation.java
index 1bcb9c6..fe318a0 100644
--- a/src/main/java/org/codehaus/groovy/transform/AutoCloneASTTransformation.java
+++ b/src/main/java/org/codehaus/groovy/transform/AutoCloneASTTransformation.java
@@ -20,6 +20,7 @@ package org.codehaus.groovy.transform;
import groovy.transform.AutoClone;
import groovy.transform.AutoCloneStyle;
+import org.apache.groovy.ast.tools.AnnotatedNodeUtils;
import org.codehaus.groovy.ast.ASTNode;
import org.codehaus.groovy.ast.AnnotatedNode;
import org.codehaus.groovy.ast.AnnotationNode;
@@ -27,10 +28,12 @@ import org.codehaus.groovy.ast.ClassHelper;
import org.codehaus.groovy.ast.ClassNode;
import org.codehaus.groovy.ast.ConstructorNode;
import org.codehaus.groovy.ast.FieldNode;
+import org.codehaus.groovy.ast.MethodNode;
import org.codehaus.groovy.ast.Parameter;
import org.codehaus.groovy.ast.VariableScope;
import org.codehaus.groovy.ast.expr.ClassExpression;
import org.codehaus.groovy.ast.expr.ClosureExpression;
+import org.codehaus.groovy.ast.expr.ConstructorCallExpression;
import org.codehaus.groovy.ast.expr.Expression;
import org.codehaus.groovy.ast.expr.MethodCallExpression;
import org.codehaus.groovy.ast.expr.PropertyExpression;
@@ -81,6 +84,8 @@ import static org.codehaus.groovy.ast.tools.GeneralUtils.stmt;
import static org.codehaus.groovy.ast.tools.GeneralUtils.ternaryX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.varX;
import static org.codehaus.groovy.ast.ClassHelper.isObjectType;
+import static org.codehaus.groovy.transform.sc.StaticCompilationVisitor.COMPILESTATIC_CLASSNODE;
+import static org.codehaus.groovy.transform.stc.StaticTypesMarker.DIRECT_METHOD_CALL_TARGET;
import static org.objectweb.asm.Opcodes.ACC_FINAL;
import static org.objectweb.asm.Opcodes.ACC_PROTECTED;
import static org.objectweb.asm.Opcodes.ACC_PUBLIC;
@@ -236,8 +241,15 @@ public class AutoCloneASTTransformation extends AbstractASTTransformation {
addSimpleCloneHelperMethod(cNode, fieldNodes, excludes);
final Expression result = localVarX("_result");
ClassNode[] exceptions = {make(CloneNotSupportedException.class)};
+ ConstructorCallExpression init = ctorX(cNode);
+ if (AnnotatedNodeUtils.hasAnnotation(cNode, COMPILESTATIC_CLASSNODE) && cNode.getDeclaredConstructor(Parameter.EMPTY_ARRAY) == null) {
+ // constructor may not be added yet which confuses type checking, so add manually
+ MethodNode cons = new ConstructorNode(ACC_PUBLIC, Parameter.EMPTY_ARRAY, ClassNode.EMPTY_ARRAY, EmptyStatement.INSTANCE);
+ cons.setDeclaringClass(cNode);
+ init.putNodeMetaData(DIRECT_METHOD_CALL_TARGET, cons);
+ }
addGeneratedMethod(cNode, "clone", ACC_PUBLIC, GenericsUtils.nonGeneric(cNode), Parameter.EMPTY_ARRAY, exceptions, block(
- declS(result, ctorX(cNode)),
+ declS(result, init),
stmt(callThisX("cloneOrCopyMembers", args(result))),
returnS(result)));
}
diff --git a/src/main/java/org/codehaus/groovy/transform/TupleConstructorASTTransformation.java b/src/main/java/org/codehaus/groovy/transform/TupleConstructorASTTransformation.java
index 97d8058..c87922c 100644
--- a/src/main/java/org/codehaus/groovy/transform/TupleConstructorASTTransformation.java
+++ b/src/main/java/org/codehaus/groovy/transform/TupleConstructorASTTransformation.java
@@ -20,6 +20,7 @@ package org.codehaus.groovy.transform;
import groovy.lang.GroovyClassLoader;
import groovy.transform.CompilationUnitAware;
+import groovy.transform.DefaultsMode;
import groovy.transform.TupleConstructor;
import groovy.transform.options.PropertyHandler;
import groovy.transform.stc.POJO;
@@ -35,10 +36,12 @@ import org.codehaus.groovy.ast.FieldNode;
import org.codehaus.groovy.ast.Parameter;
import org.codehaus.groovy.ast.PropertyNode;
import org.codehaus.groovy.ast.expr.ArgumentListExpression;
+import org.codehaus.groovy.ast.expr.ClassExpression;
import org.codehaus.groovy.ast.expr.ClosureExpression;
import org.codehaus.groovy.ast.expr.ConstantExpression;
import org.codehaus.groovy.ast.expr.Expression;
import org.codehaus.groovy.ast.expr.MethodCallExpression;
+import org.codehaus.groovy.ast.expr.PropertyExpression;
import org.codehaus.groovy.ast.expr.VariableExpression;
import org.codehaus.groovy.ast.stmt.BlockStatement;
import org.codehaus.groovy.ast.stmt.EmptyStatement;
@@ -56,6 +59,9 @@ import java.util.LinkedHashMap;
import java.util.List;
import java.util.Set;
+import static groovy.transform.DefaultsMode.AUTO;
+import static groovy.transform.DefaultsMode.OFF;
+import static groovy.transform.DefaultsMode.ON;
import static org.apache.groovy.ast.tools.ClassNodeUtils.addGeneratedConstructor;
import static org.apache.groovy.ast.tools.ClassNodeUtils.hasExplicitConstructor;
import static org.apache.groovy.ast.tools.ConstructorNodeUtils.checkPropNamesS;
@@ -165,9 +171,18 @@ public class TupleConstructorASTTransformation extends AbstractASTTransformation
final List<String> excludes, final List<String> includes, final boolean allNames, final boolean allProperties,
final SourceUnit sourceUnit, final PropertyHandler handler, final ClosureExpression pre, final ClosureExpression post) {
boolean callSuper = xform.memberHasValue(anno, "callSuper", true);
- boolean defaults = !xform.memberHasValue(anno, "defaults", false);
boolean force = xform.memberHasValue(anno, "force", true);
+ DefaultsMode defaultsMode = maybeDefaultsMode(anno, "defaultsMode");
+ if (defaultsMode == null) {
+ if (anno.getMember("defaults") == null) {
+ defaultsMode = ON;
+ } else {
+ boolean defaults = !xform.memberHasValue(anno, "defaults", false);
+ defaultsMode = defaults ? ON : OFF;
+ }
+ }
boolean namedVariant = xform.memberHasValue(anno, "namedVariant", true);
+
Set<String> names = new HashSet<>();
List<PropertyNode> superList;
if (includeSuperProperties || includeSuperFields) {
@@ -179,8 +194,8 @@ public class TupleConstructorASTTransformation extends AbstractASTTransformation
List<PropertyNode> list = getAllProperties(names, cNode, includeProperties, includeFields, false, allProperties, false, true);
boolean makeImmutable = makeImmutable(cNode);
- boolean specialNamedArgCase = (ImmutableASTTransformation.isSpecialNamedArgCase(list, !defaults) && superList.isEmpty()) ||
- (ImmutableASTTransformation.isSpecialNamedArgCase(superList, !defaults) && list.isEmpty());
+ boolean specialNamedArgCase = (ImmutableASTTransformation.isSpecialNamedArgCase(list, defaultsMode == OFF) && superList.isEmpty()) ||
+ (ImmutableASTTransformation.isSpecialNamedArgCase(superList, defaultsMode == OFF) && list.isEmpty());
// no processing if existing constructors found unless forced or ImmutableBase in play
if (hasExplicitConstructor(null, cNode) && !force && !makeImmutable) return;
@@ -209,7 +224,7 @@ public class TupleConstructorASTTransformation extends AbstractASTTransformation
String name = pNode.getName();
FieldNode fNode = pNode.getField();
if (shouldSkipUndefinedAware(name, excludes, includes, allNames)) continue;
- params.add(createParam(fNode, name, defaults, xform, makeImmutable));
+ params.add(createParam(fNode, name, defaultsMode, xform, makeImmutable));
if (callSuper) {
superParams.add(varX(name));
} else if (!superInPre && !specialNamedArgCase) {
@@ -230,8 +245,31 @@ public class TupleConstructorASTTransformation extends AbstractASTTransformation
String name = pNode.getName();
FieldNode fNode = pNode.getField();
if (shouldSkipUndefinedAware(name, excludes, includes, allNames)) continue;
- Parameter nextParam = createParam(fNode, name, defaults, xform, makeImmutable);
+ Parameter nextParam = createParam(fNode, name, defaultsMode, xform, makeImmutable);
params.add(nextParam);
+ }
+
+ if (includes != null) {
+ Comparator<Parameter> includeComparator = Comparator.comparingInt(p -> includes.indexOf(p.getName()));
+ params.sort(includeComparator);
+ }
+
+ if (defaultsMode == AUTO) {
+ boolean foundNoDefault = false;
+ for (int i = params.size() - 1; i >= 0; i--) {
+ Parameter p = params.get(i);
+ if (!p.hasInitialExpression()) {
+ foundNoDefault = true;
+ }
+ if (foundNoDefault) {
+ p.setInitialExpression(null);
+ }
+ }
+ }
+
+ for (PropertyNode pNode : list) {
+ String name = pNode.getName();
+ if (shouldSkipUndefinedAware(name, excludes, includes, allNames)) continue;
Statement propInit = handler.createPropInit(xform, anno, cNode, pNode, null);
if (propInit != null) {
body.addStatement(propInit);
@@ -242,10 +280,7 @@ public class TupleConstructorASTTransformation extends AbstractASTTransformation
body.addStatement(post.getCode());
}
- if (includes != null) {
- params.sort(Comparator.comparingInt(p -> includes.indexOf(p.getName())));
- }
-
+ boolean hasMapCons = AnnotatedNodeUtils.hasAnnotation(cNode, MapConstructorASTTransformation.MY_TYPE);
int modifiers = getVisibility(anno, cNode, ConstructorNode.class, ACC_PUBLIC);
ConstructorNode consNode = addGeneratedConstructor(cNode, modifiers, params.toArray(Parameter.EMPTY_ARRAY), ClassNode.EMPTY_ARRAY, body);
if (namedVariant) {
@@ -275,8 +310,7 @@ public class TupleConstructorASTTransformation extends AbstractASTTransformation
// we don't do it for LinkedHashMap for now (would lead to duplicate signature)
// or if there is only one Map property (for backwards compatibility)
// or if there is already a @MapConstructor annotation
- if (defaults && specialNamedArgCase && !params.isEmpty()
- && !AnnotatedNodeUtils.hasAnnotation(cNode, MapConstructorASTTransformation.MY_TYPE)) {
+ if (!params.isEmpty() && defaultsMode != OFF && !hasMapCons && specialNamedArgCase) {
ClassNode firstParamType = params.get(0).getType();
if (params.size() > 1 || ClassHelper.isObjectType(firstParamType)) {
String message = "The class " + cNode.getName() + " was incorrectly initialized via the map constructor with null.";
@@ -285,15 +319,15 @@ public class TupleConstructorASTTransformation extends AbstractASTTransformation
}
}
- private static Parameter createParam(final FieldNode fNode, final String name, final boolean defaults, final AbstractASTTransformation xform, final boolean makeImmutable) {
+ private static Parameter createParam(FieldNode fNode, String name, DefaultsMode defaultsMode, AbstractASTTransformation xform, boolean makeImmutable) {
Parameter param = new Parameter(fNode.getType(), name);
- if (defaults) {
+ if (defaultsMode == ON) {
param.setInitialExpression(providedOrDefaultInitialValue(fNode));
- } else if (!makeImmutable) {
- // TODO we could support some default vals provided they were listed last
- if (fNode.getInitialExpression() != null) {
- xform.addError("Error during " + MY_TYPE_NAME + " processing, default value processing disabled but default value found for '" + fNode.getName() + "'", fNode);
- }
+ } else if (defaultsMode == AUTO && fNode.hasInitialExpression()) {
+ param.setInitialExpression(fNode.getInitialExpression());
+ fNode.setInitialValueExpression(null);
+ } else if (!makeImmutable && fNode.hasInitialExpression()) {
+ xform.addError("Error during " + MY_TYPE_NAME + " processing, default value processing disabled but default value found for '" + fNode.getName() + "'", fNode);
}
return param;
}
@@ -345,4 +379,21 @@ public class TupleConstructorASTTransformation extends AbstractASTTransformation
block.addStatement(checkPropNamesS(namedArgs, pojo, props));
return block;
}
+
+ private static DefaultsMode maybeDefaultsMode(AnnotationNode node, String name) {
+ if (node != null) {
+ final Expression member = node.getMember(name);
+ if (member instanceof PropertyExpression) {
+ PropertyExpression prop = (PropertyExpression) member;
+ Expression oe = prop.getObjectExpression();
+ if (oe instanceof ClassExpression) {
+ ClassExpression ce = (ClassExpression) oe;
+ if (ce.getType().getName().equals("groovy.transform.DefaultsMode")) {
+ return DefaultsMode.valueOf(prop.getPropertyAsString());
+ }
+ }
+ }
+ }
+ return null;
+ }
}
diff --git a/src/test/org/codehaus/groovy/transform/TupleConstructorTransformTest.groovy b/src/test/org/codehaus/groovy/transform/TupleConstructorTransformTest.groovy
index 55973e1..ddfca66 100644
--- a/src/test/org/codehaus/groovy/transform/TupleConstructorTransformTest.groovy
+++ b/src/test/org/codehaus/groovy/transform/TupleConstructorTransformTest.groovy
@@ -369,4 +369,56 @@ class TupleConstructorTransformTest extends GroovyShellTestCase {
'public Baz(Basep,Basepp,Basepubf,java.lang.Byte,Foop,Foopubf,java.lang.Short,Barp,Barpp,Barpubf,java.lang.Integer,Bazp,Bazpp,Bazpubf,java.lang.Long)'
'''
}
+
+ // GROOVY-10361
+ void testTupleConstructorDefaultsModes() {
+ assertScript '''
+ import groovy.transform.*
+ import static groovy.test.GroovyAssert.shouldFail
+
+ @TupleConstructor(defaultsMode = DefaultsMode.OFF, includeFields = true)
+ class A {
+ String won
+ private int too
+ }
+ assert A.declaredConstructors.toString() == '[public A(java.lang.String,int)]'
+
+ shouldFail """
+ @TupleConstructor(defaultsMode = DefaultsMode.OFF, includeFields = true)
+ class B {
+ String won = 'one'
+ private int too = 2
+ }
+ """
+
+ @TupleConstructor(defaultsMode = DefaultsMode.AUTO, includeFields = true)
+ class C {
+ String one = 'won'
+ int too = 2
+ private int three
+ private String four = 'for'
+ }
+
+ assert C.declaredConstructors*.toString().toSet() == [
+ 'public C(java.lang.String,int,int,java.lang.String)',
+ 'public C(java.lang.String,int,int)'
+ ].toSet()
+
+ @TupleConstructor(defaultsMode = DefaultsMode.ON, includeFields = true)
+ class D {
+ String one = 'won'
+ int too = 2
+ private int three
+ private String four = 'for'
+ }
+
+ assert D.declaredConstructors*.toString().toSet() == [
+ 'public D(java.lang.String,int,int,java.lang.String)',
+ 'public D(java.lang.String,int,int)',
+ 'public D(java.lang.String,int)',
+ 'public D(java.lang.String)',
+ 'public D()'
+ ].toSet()
+ '''
+ }
}