Repository: groovy Updated Branches: refs/heads/master b673737a6 -> ae73edbe4
GROOVY-8440: Groovy's @Immutable annotation should be re-vamped to be a meta-annotation (additional refactoring to remove dup and plumb in pre/post conditions) Project: http://git-wip-us.apache.org/repos/asf/groovy/repo Commit: http://git-wip-us.apache.org/repos/asf/groovy/commit/ae73edbe Tree: http://git-wip-us.apache.org/repos/asf/groovy/tree/ae73edbe Diff: http://git-wip-us.apache.org/repos/asf/groovy/diff/ae73edbe Branch: refs/heads/master Commit: ae73edbe4641e809969d90d0dec2771a97c85aef Parents: b673737 Author: paulk <[email protected]> Authored: Wed Jan 31 11:19:57 2018 +1000 Committer: paulk <[email protected]> Committed: Wed Jan 31 11:19:57 2018 +1000 ---------------------------------------------------------------------- .../groovy/classgen/EnumCompletionVisitor.java | 8 +- .../transform/ImmutableASTTransformation.java | 136 ++++++++++++------- .../MapConstructorASTTransformation.java | 25 ++-- .../TupleConstructorASTTransformation.java | 75 +++++----- 4 files changed, 141 insertions(+), 103 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/groovy/blob/ae73edbe/src/main/java/org/codehaus/groovy/classgen/EnumCompletionVisitor.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/codehaus/groovy/classgen/EnumCompletionVisitor.java b/src/main/java/org/codehaus/groovy/classgen/EnumCompletionVisitor.java index 30c6fab..7cc5375 100644 --- a/src/main/java/org/codehaus/groovy/classgen/EnumCompletionVisitor.java +++ b/src/main/java/org/codehaus/groovy/classgen/EnumCompletionVisitor.java @@ -82,7 +82,7 @@ public class EnumCompletionVisitor extends ClassCodeVisitorSupport { ClassNode sn = enumClass.getSuperClass(); List<ConstructorNode> sctors = new ArrayList<ConstructorNode>(sn.getDeclaredConstructors()); if (sctors.isEmpty()) { - addMapConstructors(enumClass, false); + addMapConstructors(enumClass); } else { for (ConstructorNode constructorNode : sctors) { ConstructorNode init = new ConstructorNode(Opcodes.ACC_PUBLIC, constructorNode.getParameters(), ClassNode.EMPTY_ARRAY, new BlockStatement()); @@ -90,7 +90,7 @@ public class EnumCompletionVisitor extends ClassCodeVisitorSupport { } } } else { - addMapConstructors(enumClass, false); + addMapConstructors(enumClass); } } @@ -143,8 +143,8 @@ public class EnumCompletionVisitor extends ClassCodeVisitorSupport { } } - public static void addMapConstructors(ClassNode enumClass, boolean hasNoArg) { - TupleConstructorASTTransformation.addMapConstructors(enumClass, hasNoArg, "One of the enum constants for enum " + enumClass.getName() + + private static void addMapConstructors(ClassNode enumClass) { + TupleConstructorASTTransformation.addMapConstructors(enumClass, false, "One of the enum constants for enum " + enumClass.getName() + " was initialized with null. Please use a non-null value or define your own constructor."); } http://git-wip-us.apache.org/repos/asf/groovy/blob/ae73edbe/src/main/java/org/codehaus/groovy/transform/ImmutableASTTransformation.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/codehaus/groovy/transform/ImmutableASTTransformation.java b/src/main/java/org/codehaus/groovy/transform/ImmutableASTTransformation.java index 5536783..0d271ed 100644 --- a/src/main/java/org/codehaus/groovy/transform/ImmutableASTTransformation.java +++ b/src/main/java/org/codehaus/groovy/transform/ImmutableASTTransformation.java @@ -75,7 +75,6 @@ import static org.codehaus.groovy.ast.tools.GeneralUtils.callX; import static org.codehaus.groovy.ast.tools.GeneralUtils.castX; import static org.codehaus.groovy.ast.tools.GeneralUtils.classList2args; import static org.codehaus.groovy.ast.tools.GeneralUtils.constX; -import static org.codehaus.groovy.ast.tools.GeneralUtils.createConstructorStatementDefault; import static org.codehaus.groovy.ast.tools.GeneralUtils.ctorX; import static org.codehaus.groovy.ast.tools.GeneralUtils.declS; import static org.codehaus.groovy.ast.tools.GeneralUtils.eqX; @@ -95,6 +94,7 @@ import static org.codehaus.groovy.ast.tools.GeneralUtils.neX; import static org.codehaus.groovy.ast.tools.GeneralUtils.notX; import static org.codehaus.groovy.ast.tools.GeneralUtils.orX; import static org.codehaus.groovy.ast.tools.GeneralUtils.params; +import static org.codehaus.groovy.ast.tools.GeneralUtils.propX; import static org.codehaus.groovy.ast.tools.GeneralUtils.returnS; import static org.codehaus.groovy.ast.tools.GeneralUtils.safeExpression; import static org.codehaus.groovy.ast.tools.GeneralUtils.stmt; @@ -138,6 +138,7 @@ public class ImmutableASTTransformation extends AbstractASTTransformation { private static final Class MY_CLASS = ImmutableBase.class; private static final Class<? extends Annotation> KNOWN_IMMUTABLE_CLASS = KnownImmutable.class; private static final Class<? extends Annotation> IMMUTABLE_BASE_CLASS = ImmutableBase.class; + private static final ClassNode IMMUTABLE_BASE_TYPE = makeWithoutCaching(IMMUTABLE_BASE_CLASS, false); public static final ClassNode MY_TYPE = make(MY_CLASS); static final String MY_TYPE_NAME = "@" + MY_TYPE.getNameWithoutPackage(); private static final String MEMBER_KNOWN_IMMUTABLE_CLASSES = "knownImmutableClasses"; @@ -173,7 +174,6 @@ public class ImmutableASTTransformation extends AbstractASTTransformation { private void doMakeImmutable(ClassNode cNode, AnnotationNode node) { List<PropertyNode> newProperties = new ArrayList<PropertyNode>(); -// final List<String> knownImmutableClasses = getKnownImmutableClasses(this, node); final List<String> knownImmutables = getKnownImmutables(this, node); String cName = cNode.getName(); @@ -250,7 +250,7 @@ public class ImmutableASTTransformation extends AbstractASTTransformation { } } - static List<String> getKnownImmutableClasses(AbstractASTTransformation xform, AnnotationNode node) { + private static List<String> getKnownImmutableClasses(AbstractASTTransformation xform, AnnotationNode node) { final List<String> immutableClasses = new ArrayList<String>(); if (node == null) return immutableClasses; @@ -272,7 +272,7 @@ public class ImmutableASTTransformation extends AbstractASTTransformation { return immutableClasses; } - static List<String> getKnownImmutables(AbstractASTTransformation xform, AnnotationNode node) { + private static List<String> getKnownImmutables(AbstractASTTransformation xform, AnnotationNode node) { final List<String> immutables = new ArrayList<String>(); if (node == null) return immutables; @@ -386,7 +386,7 @@ public class ImmutableASTTransformation extends AbstractASTTransformation { if (fNode.getName().contains("$") || fNode.isSynthetic()) continue; // internal field if (fNode.isFinal() && fNode.getInitialExpression() != null) body.addStatement(checkFinalArgNotOverridden(cNode, fNode)); - body.addStatement(createConstructorStatementDefault(fNode)); + body.addStatement(createConstructorStatementDefault(fNode, true)); } doAddConstructor(cNode, new ConstructorNode(ACC_PUBLIC, params(new Parameter(HASHMAP_TYPE, "args")), ClassNode.EMPTY_ARRAY, body)); } @@ -413,21 +413,18 @@ public class ImmutableASTTransformation extends AbstractASTTransformation { } Expression namedArgs = findArg(fNode.getName()); Expression baseArgs = varX("args"); - return ifElseS( - equalsNullX(baseArgs), - assignInit, + Statement assignStmt = ifElseS( + equalsNullX(namedArgs), ifElseS( - equalsNullX(namedArgs), - ifElseS( - isTrueX(callX(baseArgs, "containsKey", constX(fNode.getName()))), - assignS(fieldExpr, namedArgs), - assignS(fieldExpr, cloneCollectionExpr(baseArgs, fieldType))), - ifElseS( - isOneX(callX(baseArgs, "size")), - assignS(fieldExpr, cloneCollectionExpr(namedArgs, fieldType)), - assignS(fieldExpr, cloneCollectionExpr(baseArgs, fieldType))) - ) + isTrueX(callX(baseArgs, "containsKey", constX(fNode.getName()))), + assignS(fieldExpr, namedArgs), + assignS(fieldExpr, cloneCollectionExpr(baseArgs, fieldType))), + ifElseS( + isOneX(callX(baseArgs, "size")), + assignS(fieldExpr, cloneCollectionExpr(namedArgs, fieldType)), + assignS(fieldExpr, cloneCollectionExpr(baseArgs, fieldType))) ); + return ifElseS(equalsNullX(baseArgs), assignInit, assignStmt); } private static void ensureNotPublic(AbstractASTTransformation xform, String cNode, FieldNode fNode) { @@ -462,29 +459,66 @@ public class ImmutableASTTransformation extends AbstractASTTransformation { return true; } - static Statement createConstructorStatement(AbstractASTTransformation xform, ClassNode cNode, PropertyNode pNode, List<String> knownImmutables, List<String> knownImmutableClasses) { + static boolean makeImmutable(ClassNode cNode) { + List<AnnotationNode> annotations = cNode.getAnnotations(IMMUTABLE_BASE_TYPE); + AnnotationNode annoImmutable = annotations.isEmpty() ? null : annotations.get(0); + return annoImmutable != null; + } + + static Statement createConstructorStatement(AbstractASTTransformation xform, ClassNode cNode, PropertyNode pNode, boolean namedArgs) { + List<AnnotationNode> annotations = cNode.getAnnotations(IMMUTABLE_BASE_TYPE); + AnnotationNode annoImmutable = annotations.isEmpty() ? null : annotations.get(0); + final List<String> knownImmutableClasses = getKnownImmutableClasses(xform, annoImmutable); + final List<String> knownImmutables = getKnownImmutables(xform, annoImmutable); FieldNode fNode = pNode.getField(); final ClassNode fieldType = fNode.getType(); Statement statement; if (fieldType.isArray() || isOrImplements(fieldType, CLONEABLE_TYPE)) { - statement = createConstructorStatementArrayOrCloneable(fNode); + statement = createConstructorStatementArrayOrCloneable(fNode, namedArgs); } else if (isKnownImmutableClass(fieldType, knownImmutableClasses) || isKnownImmutable(pNode.getName(), knownImmutables)) { - statement = createConstructorStatementDefault(fNode); + statement = createConstructorStatementDefault(fNode, namedArgs); } else if (fieldType.isDerivedFrom(DATE_TYPE)) { - statement = createConstructorStatementDate(fNode); + statement = createConstructorStatementDate(fNode, namedArgs); } else if (isOrImplements(fieldType, COLLECTION_TYPE) || fieldType.isDerivedFrom(COLLECTION_TYPE) || isOrImplements(fieldType, MAP_TYPE) || fieldType.isDerivedFrom(MAP_TYPE)) { - statement = createConstructorStatementCollection(fNode); + statement = createConstructorStatementCollection(fNode, namedArgs); } else if (fieldType.isResolved()) { xform.addError(createErrorMessage(cNode.getName(), fNode.getName(), fieldType.getName(), "compiling"), fNode); statement = EmptyStatement.INSTANCE; } else { - statement = createConstructorStatementGuarded(cNode, fNode, knownImmutables, knownImmutableClasses); + statement = createConstructorStatementGuarded(cNode, fNode, namedArgs, knownImmutables, knownImmutableClasses); } return statement; } - private static Statement createConstructorStatementGuarded(ClassNode cNode, FieldNode fNode, List<String> knownImmutables, List<String> knownImmutableClasses) { - final Expression fieldExpr = varX(fNode); + private static Statement createConstructorStatementDefault(FieldNode fNode, boolean namedArgs) { + final ClassNode fType = fNode.getType(); + final Expression fieldExpr = propX(varX("this"), fNode.getName()); + Expression initExpr = fNode.getInitialValueExpression(); + Statement assignInit; + if (initExpr == null || (initExpr instanceof ConstantExpression && ((ConstantExpression)initExpr).isNullExpression())) { + if (ClassHelper.isPrimitiveType(fType)) { + assignInit = EmptyStatement.INSTANCE; + } else { + assignInit = assignS(fieldExpr, ConstantExpression.EMPTY_EXPRESSION); + } + } else { + assignInit = assignS(fieldExpr, initExpr); + } + fNode.setInitialValueExpression(null); + Expression param = getParam(fNode, namedArgs); + Statement assignStmt = assignS(fieldExpr, castX(fType, param)); + return assignWithDefault(namedArgs, assignInit, param, assignStmt); + } + + private static Statement assignWithDefault(boolean namedArgs, Statement assignInit, Expression param, Statement assignStmt) { + if (!namedArgs) { + return assignStmt; + } + return ifElseS(equalsNullX(param), assignInit, assignStmt); + } + + private static Statement createConstructorStatementGuarded(ClassNode cNode, FieldNode fNode, boolean namedArgs, List<String> knownImmutables, List<String> knownImmutableClasses) { + final Expression fieldExpr = propX(varX("this"), fNode.getName()); Expression initExpr = fNode.getInitialValueExpression(); final Statement assignInit; if (initExpr == null || (initExpr instanceof ConstantExpression && ((ConstantExpression) initExpr).isNullExpression())) { @@ -492,8 +526,9 @@ public class ImmutableASTTransformation extends AbstractASTTransformation { } else { assignInit = assignS(fieldExpr, checkUnresolved(fNode, initExpr, knownImmutables, knownImmutableClasses)); } - Expression unknown = findArg(fNode.getName()); - return ifElseS(equalsNullX(unknown), assignInit, assignS(fieldExpr, checkUnresolved(fNode, unknown, knownImmutables, knownImmutableClasses))); + Expression param = getParam(fNode, namedArgs); + Statement assignStmt = assignS(fieldExpr, checkUnresolved(fNode, param, knownImmutables, knownImmutableClasses)); + return assignWithDefault(namedArgs, assignInit, param, assignStmt); } private static Expression checkUnresolved(FieldNode fNode, Expression value, List<String> knownImmutables, List<String> knownImmutableClasses) { @@ -501,8 +536,8 @@ public class ImmutableASTTransformation extends AbstractASTTransformation { return callX(SELF_TYPE, "checkImmutable", args); } - private static Statement createConstructorStatementCollection(FieldNode fNode) { - final Expression fieldExpr = varX(fNode); + private static Statement createConstructorStatementCollection(FieldNode fNode, boolean namedArgs) { + final Expression fieldExpr = propX(varX("this"), fNode.getName()); ClassNode fieldType = fieldExpr.getType(); Expression initExpr = fNode.getInitialValueExpression(); final Statement assignInit; @@ -511,15 +546,12 @@ public class ImmutableASTTransformation extends AbstractASTTransformation { } else { assignInit = assignS(fieldExpr, cloneCollectionExpr(initExpr, fieldType)); } - Expression collection = findArg(fNode.getName()); - return ifElseS( - equalsNullX(collection), - assignInit, - ifElseS( - isInstanceOfX(collection, CLONEABLE_TYPE), - assignS(fieldExpr, cloneCollectionExpr(cloneArrayOrCloneableExpr(collection, fieldType), fieldType)), - assignS(fieldExpr, cloneCollectionExpr(collection, fieldType))) - ); + Expression param = getParam(fNode, namedArgs); + Statement assignStmt = ifElseS( + isInstanceOfX(param, CLONEABLE_TYPE), + assignS(fieldExpr, cloneCollectionExpr(cloneArrayOrCloneableExpr(param, fieldType), fieldType)), + assignS(fieldExpr, cloneCollectionExpr(param, fieldType))); + return assignWithDefault(namedArgs, assignInit, param, assignStmt); } private static boolean isKnownImmutableClass(ClassNode fieldType, List<String> knownImmutableClasses) { @@ -549,22 +581,27 @@ public class ImmutableASTTransformation extends AbstractASTTransformation { return immutableList.contains(typeName); } - private static Statement createConstructorStatementArrayOrCloneable(FieldNode fNode) { - final Expression fieldExpr = varX(fNode); - Expression initExpr = fNode.getInitialValueExpression(); - ClassNode fieldType = fNode.getType(); - final Expression array = findArg(fNode.getName()); + private static Statement createConstructorStatementArrayOrCloneable(FieldNode fNode, boolean namedArgs) { + final Expression fieldExpr = propX(varX("this"), fNode.getName()); + final Expression initExpr = fNode.getInitialValueExpression(); + final ClassNode fieldType = fNode.getType(); + final Expression param = getParam(fNode, namedArgs); final Statement assignInit; if (initExpr == null || (initExpr instanceof ConstantExpression && ((ConstantExpression) initExpr).isNullExpression())) { assignInit = assignS(fieldExpr, ConstantExpression.EMPTY_EXPRESSION); } else { assignInit = assignS(fieldExpr, cloneArrayOrCloneableExpr(initExpr, fieldType)); } - return ifElseS(equalsNullX(array), assignInit, assignS(fieldExpr, cloneArrayOrCloneableExpr(array, fieldType))); + Statement assignStmt = assignS(fieldExpr, cloneArrayOrCloneableExpr(param, fieldType)); + return assignWithDefault(namedArgs, assignInit, param, assignStmt); } - private static Statement createConstructorStatementDate(FieldNode fNode) { - final Expression fieldExpr = varX(fNode); + private static Expression getParam(FieldNode fNode, boolean namedArgs) { + return namedArgs ? findArg(fNode.getName()) : varX(fNode.getName(), fNode.getType()); + } + + private static Statement createConstructorStatementDate(FieldNode fNode, boolean namedArgs) { + final Expression fieldExpr = propX(varX("this"), fNode.getName()); Expression initExpr = fNode.getInitialValueExpression(); final Statement assignInit; if (initExpr == null || (initExpr instanceof ConstantExpression && ((ConstantExpression) initExpr).isNullExpression())) { @@ -572,8 +609,9 @@ public class ImmutableASTTransformation extends AbstractASTTransformation { } else { assignInit = assignS(fieldExpr, cloneDateExpr(initExpr)); } - final Expression date = findArg(fNode.getName()); - return ifElseS(equalsNullX(date), assignInit, assignS(fieldExpr, cloneDateExpr(date))); + final Expression param = getParam(fNode, namedArgs); + Statement assignStmt = assignS(fieldExpr, cloneDateExpr(param)); + return assignWithDefault(namedArgs, assignInit, param, assignStmt); } private static Expression cloneDateExpr(Expression origDate) { http://git-wip-us.apache.org/repos/asf/groovy/blob/ae73edbe/src/main/java/org/codehaus/groovy/transform/MapConstructorASTTransformation.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/codehaus/groovy/transform/MapConstructorASTTransformation.java b/src/main/java/org/codehaus/groovy/transform/MapConstructorASTTransformation.java index 8f24342..7718b27 100644 --- a/src/main/java/org/codehaus/groovy/transform/MapConstructorASTTransformation.java +++ b/src/main/java/org/codehaus/groovy/transform/MapConstructorASTTransformation.java @@ -18,7 +18,6 @@ */ package org.codehaus.groovy.transform; -import groovy.transform.ImmutableBase; import groovy.transform.MapConstructor; import org.codehaus.groovy.ast.ASTNode; import org.codehaus.groovy.ast.AnnotatedNode; @@ -40,7 +39,6 @@ import org.codehaus.groovy.ast.stmt.Statement; import org.codehaus.groovy.control.CompilePhase; import org.codehaus.groovy.control.SourceUnit; -import java.lang.annotation.Annotation; import java.util.ArrayList; import java.util.HashSet; import java.util.List; @@ -57,10 +55,10 @@ import static org.codehaus.groovy.ast.tools.GeneralUtils.callX; import static org.codehaus.groovy.ast.tools.GeneralUtils.constX; import static org.codehaus.groovy.ast.tools.GeneralUtils.copyStatementsWithSuperAdjustment; import static org.codehaus.groovy.ast.tools.GeneralUtils.ctorX; +import static org.codehaus.groovy.ast.tools.GeneralUtils.equalsNullX; import static org.codehaus.groovy.ast.tools.GeneralUtils.getAllProperties; import static org.codehaus.groovy.ast.tools.GeneralUtils.getSetterName; import static org.codehaus.groovy.ast.tools.GeneralUtils.ifS; -import static org.codehaus.groovy.ast.tools.GeneralUtils.notNullX; import static org.codehaus.groovy.ast.tools.GeneralUtils.param; import static org.codehaus.groovy.ast.tools.GeneralUtils.params; import static org.codehaus.groovy.ast.tools.GeneralUtils.propX; @@ -69,8 +67,6 @@ import static org.codehaus.groovy.ast.tools.GeneralUtils.varX; import static org.codehaus.groovy.transform.ImmutableASTTransformation.createConstructorMapCommon; import static org.codehaus.groovy.transform.ImmutableASTTransformation.createConstructorStatement; import static org.codehaus.groovy.transform.ImmutableASTTransformation.createConstructorStatementMapSpecial; -import static org.codehaus.groovy.transform.ImmutableASTTransformation.getKnownImmutableClasses; -import static org.codehaus.groovy.transform.ImmutableASTTransformation.getKnownImmutables; /** * Handles generation of code for the @MapConstructor annotation. @@ -82,8 +78,6 @@ public class MapConstructorASTTransformation extends AbstractASTTransformation { static final ClassNode MY_TYPE = make(MY_CLASS); static final String MY_TYPE_NAME = "@" + MY_TYPE.getNameWithoutPackage(); private static final ClassNode MAP_TYPE = makeWithoutCaching(Map.class, false); - private static final Class<? extends Annotation> IMMUTABLE_BASE_CLASS = ImmutableBase.class; - private static final ClassNode IMMUTABLE_BASE_TYPE = makeWithoutCaching(IMMUTABLE_BASE_CLASS, false); private static final ClassNode IMMUTABLE_XFORM_TYPE = make(ImmutableASTTransformation.class); public void visit(ASTNode[] nodes, SourceUnit source) { @@ -148,11 +142,7 @@ public class MapConstructorASTTransformation extends AbstractASTTransformation { } List<PropertyNode> list = getAllProperties(names, cNode, true, includeFields, allProperties, false, true); - List<AnnotationNode> annotations = cNode.getAnnotations(IMMUTABLE_BASE_TYPE); - AnnotationNode annoImmutable = annotations.isEmpty() ? null : annotations.get(0); - boolean makeImmutable = annoImmutable != null; - final List<String> knownImmutableClasses = getKnownImmutableClasses(xform, annoImmutable); - final List<String> knownImmutables = getKnownImmutables(xform, annoImmutable); + boolean makeImmutable = ImmutableASTTransformation.makeImmutable(cNode); Parameter map = param(MAP_TYPE, "args"); final BlockStatement body = new BlockStatement(); @@ -165,9 +155,10 @@ public class MapConstructorASTTransformation extends AbstractASTTransformation { superList.addAll(list); boolean specialHashMapCase = ImmutableASTTransformation.isSpecialHashMapCase(superList); if (!specialHashMapCase) { - processProps(xform, cNode, makeImmutable && !specialHashMapCase, useSetters, allNames, excludes, includes, superList, map, inner, knownImmutables, knownImmutableClasses); + processProps(xform, cNode, makeImmutable && !specialHashMapCase, useSetters, allNames, excludes, includes, superList, map, inner); + body.addStatement(ifS(equalsNullX(varX("args")), assignS(varX("args"), new MapExpression()))); } - body.addStatement(ifS(notNullX(varX("args")), inner)); + body.addStatement(inner); if (post != null) { ClosureExpression transformed = (ClosureExpression) transformer.transform(post); body.addStatement(transformed.getCode()); @@ -186,12 +177,12 @@ public class MapConstructorASTTransformation extends AbstractASTTransformation { } } - private static void processProps(AbstractASTTransformation xform, ClassNode cNode, boolean makeImmutable, boolean useSetters, boolean allNames, List<String> excludes, List<String> includes, List<PropertyNode> superList, Parameter map, BlockStatement inner, List<String> knownImmutables, List<String> knownImmutableClasses) { + private static void processProps(AbstractASTTransformation xform, ClassNode cNode, boolean makeImmutable, boolean useSetters, boolean allNames, List<String> excludes, List<String> includes, List<PropertyNode> superList, Parameter map, BlockStatement inner) { for (PropertyNode pNode : superList) { String name = pNode.getName(); - if (shouldSkip(name, excludes, includes, allNames)) continue; + if (shouldSkipUndefinedAware(name, excludes, includes, allNames)) continue; if (makeImmutable) { - inner.addStatement(createConstructorStatement(xform, cNode, pNode, knownImmutables, knownImmutableClasses)); + inner.addStatement(createConstructorStatement(xform, cNode, pNode, true)); } else { assignField(useSetters, map, inner, name); } http://git-wip-us.apache.org/repos/asf/groovy/blob/ae73edbe/src/main/java/org/codehaus/groovy/transform/TupleConstructorASTTransformation.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/codehaus/groovy/transform/TupleConstructorASTTransformation.java b/src/main/java/org/codehaus/groovy/transform/TupleConstructorASTTransformation.java index 5f0e381..c5068d5 100644 --- a/src/main/java/org/codehaus/groovy/transform/TupleConstructorASTTransformation.java +++ b/src/main/java/org/codehaus/groovy/transform/TupleConstructorASTTransformation.java @@ -18,7 +18,7 @@ */ package org.codehaus.groovy.transform; -import groovy.transform.ImmutableBase; +import groovy.transform.MapConstructor; import groovy.transform.TupleConstructor; import org.codehaus.groovy.ast.ASTNode; import org.codehaus.groovy.ast.AnnotatedNode; @@ -71,6 +71,8 @@ import static org.codehaus.groovy.ast.tools.GeneralUtils.propX; import static org.codehaus.groovy.ast.tools.GeneralUtils.stmt; import static org.codehaus.groovy.ast.tools.GeneralUtils.throwS; import static org.codehaus.groovy.ast.tools.GeneralUtils.varX; +import static org.codehaus.groovy.transform.ImmutableASTTransformation.createConstructorStatement; +import static org.codehaus.groovy.transform.ImmutableASTTransformation.makeImmutable; /** * Handles generation of code for the @TupleConstructor annotation. @@ -84,8 +86,7 @@ public class TupleConstructorASTTransformation extends AbstractASTTransformation private static final ClassNode LHMAP_TYPE = makeWithoutCaching(LinkedHashMap.class, false); private static final ClassNode HMAP_TYPE = makeWithoutCaching(HashMap.class, false); private static final ClassNode CHECK_METHOD_TYPE = make(ImmutableASTTransformation.class); - private static final Class<? extends Annotation> IMMUTABLE_BASE_CLASS = ImmutableBase.class; - private static final ClassNode IMMUTABLE_BASE_TYPE = makeWithoutCaching(IMMUTABLE_BASE_CLASS, false); + private static final Class<? extends Annotation> MAP_CONSTRUCTOR_CLASS = MapConstructor.class; private static final Map<Class<?>, Expression> primitivesInitialValues; static { @@ -186,21 +187,12 @@ public class TupleConstructorASTTransformation extends AbstractASTTransformation List<PropertyNode> list = getAllProperties(names, cNode, true, includeFields, allProperties, false, true); - List<AnnotationNode> annotations = cNode.getAnnotations(IMMUTABLE_BASE_TYPE); - AnnotationNode annoImmutable = annotations.isEmpty() ? null : annotations.get(0); - boolean makeImmutable = annoImmutable != null; - if (makeImmutable) { - boolean specialHashMapCase = (ImmutableASTTransformation.isSpecialHashMapCase(list) && superList.isEmpty()) || - (ImmutableASTTransformation.isSpecialHashMapCase(superList) && list.isEmpty()); - if (!specialHashMapCase) { - superList.addAll(list); - ImmutableASTTransformation.createConstructorOrdered(cNode, superList); - } - return; - } + boolean makeImmutable = makeImmutable(cNode); + boolean specialHashMapCase = (ImmutableASTTransformation.isSpecialHashMapCase(list) && superList.isEmpty()) || + (ImmutableASTTransformation.isSpecialHashMapCase(superList) && list.isEmpty()); - // no processing if existing constructors found - if (!cNode.getDeclaredConstructors().isEmpty() && !force) return; + // no processing if existing constructors found unless forced or ImmutableBase in play + if (!cNode.getDeclaredConstructors().isEmpty() && !force && !makeImmutable) return; final List<Parameter> params = new ArrayList<Parameter>(); final List<Expression> superParams = new ArrayList<Expression>(); @@ -215,18 +207,22 @@ public class TupleConstructorASTTransformation extends AbstractASTTransformation } final BlockStatement body = new BlockStatement(); for (PropertyNode pNode : superList) { - FieldNode fNode = pNode.getField(); String name = pNode.getName(); + FieldNode fNode = pNode.getField(); if (shouldSkipUndefinedAware(name, excludes, includes, allNames)) continue; - params.add(createParam(fNode, name, defaults, xform)); + params.add(createParam(fNode, name, defaults, xform, makeImmutable)); boolean hasSetter = cNode.getProperty(name) != null && !fNode.isFinal(); if (callSuper) { superParams.add(varX(name)); - } else if (!superInPre) { - if (useSetters && hasSetter) { - body.addStatement(stmt(callThisX(getSetterName(name), varX(name)))); + } else if (!superInPre && !specialHashMapCase) { + if (makeImmutable) { + body.addStatement(createConstructorStatement(xform, cNode, pNode, false)); } else { - body.addStatement(assignS(propX(varX("this"), name), varX(name))); + if (useSetters && hasSetter) { + body.addStatement(stmt(callThisX(getSetterName(name), varX(name)))); + } else { + body.addStatement(assignS(propX(varX("this"), name), varX(name))); + } } } } @@ -236,19 +232,25 @@ 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, defaults, xform); + Parameter nextParam = createParam(fNode, name, defaults, xform, makeImmutable); params.add(nextParam); boolean hasSetter = cNode.getProperty(name) != null && !fNode.isFinal(); - if (useSetters && hasSetter) { - body.addStatement(stmt(callThisX(getSetterName(name), varX(nextParam)))); + if (makeImmutable) { + body.addStatement(createConstructorStatement(xform, cNode, pNode, false)); } else { - body.addStatement(assignS(propX(varX("this"), name), varX(nextParam))); + if (useSetters && hasSetter) { + body.addStatement(stmt(callThisX(getSetterName(name), varX(nextParam)))); + } else { + body.addStatement(assignS(propX(varX("this"), name), varX(nextParam))); + } } } + if (post != null) { body.addStatement(post.getCode()); } @@ -262,14 +264,21 @@ public class TupleConstructorASTTransformation extends AbstractASTTransformation Collections.sort(params, includeComparator); } - cNode.addConstructor(new ConstructorNode(ACC_PUBLIC, params.toArray(new Parameter[params.size()]), ClassNode.EMPTY_ARRAY, body)); + boolean hasMapCons = hasAnnotation(cNode, MapConstructorASTTransformation.MY_TYPE); + if (!specialHashMapCase || !hasMapCons) { + cNode.addConstructor(new ConstructorNode(ACC_PUBLIC, params.toArray(new Parameter[params.size()]), ClassNode.EMPTY_ARRAY, body)); + } + if (sourceUnit != null && !body.isEmpty()) { VariableScopeVisitor scopeVisitor = new VariableScopeVisitor(sourceUnit); scopeVisitor.visitClass(cNode); } - // add map constructor if needed, don't do it for LinkedHashMap for now (would lead to duplicate signature) + + // 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) - if (!params.isEmpty() && defaults) { + // or if there is already a @MapConstructor annotation + if (!params.isEmpty() && defaults && !hasMapCons) { ClassNode firstParam = params.get(0).getType(); if (params.size() > 1 || firstParam.equals(ClassHelper.OBJECT_TYPE)) { String message = "The class " + cNode.getName() + " was incorrectly initialized via the map constructor with null."; @@ -289,15 +298,15 @@ public class TupleConstructorASTTransformation extends AbstractASTTransformation } } - private static Parameter createParam(FieldNode fNode, String name, boolean defaults, AbstractASTTransformation xform) { + private static Parameter createParam(FieldNode fNode, String name, boolean defaults, AbstractASTTransformation xform, boolean makeImmutable) { Parameter param = new Parameter(fNode.getType(), name); if (defaults) { param.setInitialExpression(providedOrDefaultInitialValue(fNode)); - } else { + } 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); } - } return param; }
