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/11/21 23:53:55 UTC
[groovy] branch GROOVY_4_0_X updated: GROOVY-10789: no constructor duplication for `@TupleConstructor`
This is an automated email from the ASF dual-hosted git repository.
emilles pushed a commit to branch GROOVY_4_0_X
in repository https://gitbox.apache.org/repos/asf/groovy.git
The following commit(s) were added to refs/heads/GROOVY_4_0_X by this push:
new 5bab55084a GROOVY-10789: no constructor duplication for `@TupleConstructor`
5bab55084a is described below
commit 5bab55084a12db698c99e1278c0e1485f22dcf9d
Author: Eric Milles <er...@thomsonreuters.com>
AuthorDate: Mon Nov 21 15:27:05 2022 -0600
GROOVY-10789: no constructor duplication for `@TupleConstructor`
---
.../java/org/codehaus/groovy/ast/ClassNode.java | 4 +-
.../TupleConstructorASTTransformation.java | 91 ++++++++++++----------
.../transform/TupleConstructorTransformTest.groovy | 26 ++++++-
3 files changed, 73 insertions(+), 48 deletions(-)
diff --git a/src/main/java/org/codehaus/groovy/ast/ClassNode.java b/src/main/java/org/codehaus/groovy/ast/ClassNode.java
index 3f71cb520c..daafc5f04c 100644
--- a/src/main/java/org/codehaus/groovy/ast/ClassNode.java
+++ b/src/main/java/org/codehaus/groovy/ast/ClassNode.java
@@ -606,8 +606,8 @@ public class ClassNode extends AnnotatedNode {
}
public void addConstructor(ConstructorNode node) {
- node.setDeclaringClass(this);
ClassNode r = redirect();
+ node.setDeclaringClass(r);
if (r.constructors == null)
r.constructors = new ArrayList<>();
r.constructors.add(node);
@@ -620,8 +620,8 @@ public class ClassNode extends AnnotatedNode {
}
public void addMethod(MethodNode node) {
- node.setDeclaringClass(this);
ClassNode r = redirect();
+ node.setDeclaringClass(r);
if (r.methodsList.isEmpty()) {
r.methodsList = new ArrayList<>();
}
diff --git a/src/main/java/org/codehaus/groovy/transform/TupleConstructorASTTransformation.java b/src/main/java/org/codehaus/groovy/transform/TupleConstructorASTTransformation.java
index 87bd6c5c26..396e8fdb1f 100644
--- a/src/main/java/org/codehaus/groovy/transform/TupleConstructorASTTransformation.java
+++ b/src/main/java/org/codehaus/groovy/transform/TupleConstructorASTTransformation.java
@@ -25,6 +25,7 @@ import groovy.transform.TupleConstructor;
import groovy.transform.options.PropertyHandler;
import groovy.transform.stc.POJO;
import org.apache.groovy.ast.tools.AnnotatedNodeUtils;
+import org.apache.groovy.ast.tools.ClassNodeUtils;
import org.apache.groovy.ast.tools.ExpressionUtils;
import org.codehaus.groovy.ast.ASTNode;
import org.codehaus.groovy.ast.AnnotatedNode;
@@ -62,6 +63,7 @@ import java.util.Set;
import static groovy.transform.DefaultsMode.OFF;
import static groovy.transform.DefaultsMode.ON;
+import static java.util.stream.Collectors.joining;
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;
@@ -240,24 +242,6 @@ public class TupleConstructorASTTransformation extends AbstractASTTransformation
if (!preBody.isEmpty()) {
body.addStatements(preBody.getStatements());
}
-
- for (PropertyNode pNode : list) {
- String name = pNode.getName();
- FieldNode fNode = pNode.getField();
- if (shouldSkipUndefinedAware(name, excludes, includes, allNames)) continue;
- Parameter nextParam = createParam(fNode, name, defaultsMode, xform, makeImmutable);
- if (cNode.getNodeMetaData("_RECORD_HEADER") != null) {
- nextParam.addAnnotations(pNode.getAnnotations());
- nextParam.putNodeMetaData("_SKIPPABLE_ANNOTATIONS", Boolean.TRUE);
- fNode.putNodeMetaData("_SKIPPABLE_ANNOTATIONS", Boolean.TRUE);
- }
- params.add(nextParam);
- }
-
- if (includes != null) {
- params.sort(Comparator.comparingInt(p -> includes.indexOf(p.getName())));
- }
-
for (PropertyNode pNode : list) {
String name = pNode.getName();
if (shouldSkipUndefinedAware(name, excludes, includes, allNames)) continue;
@@ -265,41 +249,62 @@ public class TupleConstructorASTTransformation extends AbstractASTTransformation
if (propInit != null) {
body.addStatement(propInit);
}
+ FieldNode fNode = pNode.getField();
+ Parameter param = createParam(fNode, name, defaultsMode, xform, makeImmutable);
+ if (cNode.getNodeMetaData("_RECORD_HEADER") != null) {
+ param.addAnnotations(pNode.getAnnotations());
+ param.putNodeMetaData("_SKIPPABLE_ANNOTATIONS", Boolean.TRUE);
+ fNode.putNodeMetaData("_SKIPPABLE_ANNOTATIONS", Boolean.TRUE);
+ }
+ params.add(param);
}
-
if (post != null) {
body.addStatement(post.getCode());
}
- int modifiers = getVisibility(anno, cNode, ConstructorNode.class, ACC_PUBLIC);
- // add main tuple constructor; if any parameters have default values then Verifier will generate the other variants
- ConstructorNode tupleCtor = addGeneratedConstructor(cNode, modifiers, params.toArray(Parameter.EMPTY_ARRAY), ClassNode.EMPTY_ARRAY, body);
- if (cNode.getNodeMetaData("_RECORD_HEADER") != null) {
- tupleCtor.addAnnotations(cNode.getAnnotations());
- tupleCtor.putNodeMetaData("_SKIPPABLE_ANNOTATIONS", Boolean.TRUE);
+ if (includes != null) {
+ params.sort(Comparator.comparingInt(p -> includes.indexOf(p.getName())));
}
- if (namedVariant) {
- BlockStatement inner = new BlockStatement();
- Parameter mapParam = param(ClassHelper.MAP_TYPE.getPlainNodeReference(), NAMED_ARGS);
- List<Parameter> genParams = new ArrayList<>();
- genParams.add(mapParam);
- ArgumentListExpression args = new ArgumentListExpression();
- List<String> propNames = new ArrayList<>();
- Map<Parameter, Expression> seen = new HashMap<>();
- for (Parameter p : params) {
- if (!processImplicitNamedParam(xform, tupleCtor, mapParam, inner, args, propNames, p, false, seen)) return;
+
+ int modifiers = getVisibility(anno, cNode, ConstructorNode.class, ACC_PUBLIC);
+ Parameter[] signature = params.toArray(Parameter.EMPTY_ARRAY);
+ if (cNode.getDeclaredConstructor(signature) != null) {
+ if (sourceUnit != null) {
+ String warning = String.format(
+ "%s specifies duplicate constructor: %s(%s)",
+ xform.getAnnotationName(), cNode.getNameWithoutPackage(),
+ params.stream().map(Parameter::getType).map(ClassNodeUtils::formatTypeName).collect(joining(",")));
+ sourceUnit.addWarning(warning, anno.getLineNumber() > 0 ? anno : cNode);
+ }
+ } else {
+ // add main tuple constructor; if any parameters have default values, then Verifier will generate the variants
+ ConstructorNode tupleCtor = addGeneratedConstructor(cNode, modifiers, signature, ClassNode.EMPTY_ARRAY, body);
+ if (cNode.getNodeMetaData("_RECORD_HEADER") != null) {
+ tupleCtor.addAnnotations(cNode.getAnnotations());
+ tupleCtor.putNodeMetaData("_SKIPPABLE_ANNOTATIONS", Boolean.TRUE);
+ }
+ if (namedVariant) {
+ BlockStatement inner = new BlockStatement();
+ Parameter mapParam = param(ClassHelper.MAP_TYPE.getPlainNodeReference(), NAMED_ARGS);
+ List<Parameter> genParams = new ArrayList<>();
+ genParams.add(mapParam);
+ ArgumentListExpression args = new ArgumentListExpression();
+ List<String> propNames = new ArrayList<>();
+ Map<Parameter, Expression> seen = new HashMap<>();
+ for (Parameter p : params) {
+ if (!processImplicitNamedParam(xform, tupleCtor, mapParam, inner, args, propNames, p, false, seen)) return;
+ }
+ NamedVariantASTTransformation.createMapVariant(xform, tupleCtor, anno, mapParam, genParams, cNode, inner, args, propNames);
}
- NamedVariantASTTransformation.createMapVariant(xform, tupleCtor, anno, mapParam, genParams, cNode, inner, args, propNames);
- }
- if (sourceUnit != null && !body.isEmpty()) {
- new VariableScopeVisitor(sourceUnit).visitClass(cNode);
- }
+ if (sourceUnit != null && !body.isEmpty()) {
+ new VariableScopeVisitor(sourceUnit).visitClass(cNode);
+ }
- if (body.isEmpty()) { // GROOVY-8868: retain empty constructor
- body.addStatement(stmt(ConstantExpression.EMPTY_EXPRESSION));
+ if (body.isEmpty()) { // GROOVY-8868: retain empty constructor
+ body.addStatement(stmt(ConstantExpression.EMPTY_EXPRESSION));
+ }
}
-
// If the first param is def or a Map, named args might not work as expected so we add a hard-coded map constructor in this case
// 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)
diff --git a/src/test/org/codehaus/groovy/transform/TupleConstructorTransformTest.groovy b/src/test/org/codehaus/groovy/transform/TupleConstructorTransformTest.groovy
index 228690b108..296d77cf33 100644
--- a/src/test/org/codehaus/groovy/transform/TupleConstructorTransformTest.groovy
+++ b/src/test/org/codehaus/groovy/transform/TupleConstructorTransformTest.groovy
@@ -137,7 +137,7 @@ final class TupleConstructorTransformTest {
}
assert new Cat("Mr. Bigglesworth").name == null
- assert Cat.declaredConstructors.size() == 1
+ assert Cat.declaredConstructors.length == 1
'''
assertScript shell, '''
@TupleConstructor(force=true)
@@ -150,7 +150,7 @@ final class TupleConstructorTransformTest {
assert new Cat().name == null
assert new Cat("Mr. Bigglesworth").name == null
assert new Cat("Mr. Bigglesworth", 42).name == "Mr. Bigglesworth"
- assert Cat.declaredConstructors.size() == 3 // (), (String) and (String,int)
+ assert Cat.declaredConstructors.length == 3 // (), (String) and (String,int)
'''
}
@@ -205,7 +205,7 @@ final class TupleConstructorTransformTest {
// GROOVY-7524
@Test
- void testCombiningWithInheritConstructors() {
+ void testWithInheritConstructors() {
assertScript shell, '''
@TupleConstructor
class NameId {
@@ -257,6 +257,26 @@ final class TupleConstructorTransformTest {
'''
}
+ // GROOVY-10789
+ @Test
+ void testMultipleUsage2() {
+ for (entry in [DUPLICATE:3, PREFER_COLLECTOR:1, PREFER_EXPLICIT:3, PREFER_EXPLICIT_MERGED:3]) {
+ assertScript shell, """import static groovy.transform.AnnotationCollectorMode.*
+ @TupleConstructor(defaults=false)
+ @AnnotationCollector(mode=$entry.key)
+ @interface Collector {
+ }
+
+ @Collector @TupleConstructor(defaults=true)
+ class Foo {
+ String bar, baz
+ }
+
+ assert Foo.declaredConstructors.length == $entry.value
+ """
+ }
+ }
+
// GROOVY-6454
@Test
void testInternalFieldsAreIncludedIfRequested() {