GROOVY-7584: transient fields in trait are not transient in implementing class (closes #463)
Project: http://git-wip-us.apache.org/repos/asf/groovy/repo Commit: http://git-wip-us.apache.org/repos/asf/groovy/commit/c00a75a8 Tree: http://git-wip-us.apache.org/repos/asf/groovy/tree/c00a75a8 Diff: http://git-wip-us.apache.org/repos/asf/groovy/diff/c00a75a8 Branch: refs/heads/GROOVY_2_4_X Commit: c00a75a89a15e46c2afacf6e72256cd4484bd5f1 Parents: 67c5fa1 Author: paulk <[email protected]> Authored: Thu Nov 24 04:42:29 2016 +1000 Committer: paulk <[email protected]> Committed: Tue Nov 29 05:24:19 2016 +1000 ---------------------------------------------------------------------- .../transform/trait/TraitASTTransformation.java | 27 +++++----- .../groovy/transform/trait/TraitComposer.java | 55 +++++++++++++------ .../codehaus/groovy/transform/trait/Traits.java | 9 ++++ src/test/groovy/bugs/Groovy7584Bug.groovy | 56 ++++++++++++++++++++ 4 files changed, 117 insertions(+), 30 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/groovy/blob/c00a75a8/src/main/org/codehaus/groovy/transform/trait/TraitASTTransformation.java ---------------------------------------------------------------------- diff --git a/src/main/org/codehaus/groovy/transform/trait/TraitASTTransformation.java b/src/main/org/codehaus/groovy/transform/trait/TraitASTTransformation.java index 6479e07..1ced372 100644 --- a/src/main/org/codehaus/groovy/transform/trait/TraitASTTransformation.java +++ b/src/main/org/codehaus/groovy/transform/trait/TraitASTTransformation.java @@ -388,7 +388,7 @@ public class TraitASTTransformation extends AbstractASTTransformation implements private void processField(final FieldNode field, final MethodNode initializer, final MethodNode staticInitializer, final ClassNode fieldHelper, final ClassNode trait, final Set<String> knownFields) { Expression initialExpression = field.getInitialExpression(); MethodNode selectedMethod = field.isStatic()?staticInitializer:initializer; - if (initialExpression != null) { + if (initialExpression != null && !field.isFinal()) { VariableExpression thisObject = new VariableExpression(selectedMethod.getParameters()[0]); ExpressionStatement initCode = new ExpressionStatement(initialExpression); processBody(thisObject, selectedMethod, initCode, trait, fieldHelper, knownFields); @@ -416,14 +416,16 @@ public class TraitASTTransformation extends AbstractASTTransformation implements code.addStatement(new ExpressionStatement(mce)); } // define setter/getter helper methods - fieldHelper.addMethod( - Traits.helperSetterName(field), - ACC_PUBLIC | ACC_ABSTRACT, - field.getOriginType(), - new Parameter[]{new Parameter(field.getOriginType(), "val")}, - ClassNode.EMPTY_ARRAY, - null - ); + if (!Modifier.isFinal(field.getModifiers())) { + fieldHelper.addMethod( + Traits.helperSetterName(field), + ACC_PUBLIC | ACC_ABSTRACT, + field.getOriginType(), + new Parameter[]{new Parameter(field.getOriginType(), "val")}, + ClassNode.EMPTY_ARRAY, + null + ); + } fieldHelper.addMethod( Traits.helperGetterName(field), ACC_PUBLIC | ACC_ABSTRACT, @@ -435,15 +437,14 @@ public class TraitASTTransformation extends AbstractASTTransformation implements // dummy fields are only used to carry annotations if instance field // and to differentiate from static fields otherwise - String dummyFieldName = (field.isStatic() ? Traits.STATIC_FIELD_PREFIX : Traits.FIELD_PREFIX) + - (field.isPublic()? Traits.PUBLIC_FIELD_PREFIX : Traits.PRIVATE_FIELD_PREFIX)+ - Traits.remappedFieldName(field.getOwner(), field.getName()); + int mods = field.getModifiers() & Traits.FIELD_PREFIX_MASK; + String dummyFieldName = String.format("$0x%04x", mods) + Traits.remappedFieldName(field.getOwner(), field.getName()); FieldNode dummyField = new FieldNode( dummyFieldName, ACC_STATIC | ACC_PUBLIC | ACC_FINAL | ACC_SYNTHETIC, field.getOriginType(), fieldHelper, - null + field.isFinal() ? initialExpression : null ); // copy annotations from field to dummy field List<AnnotationNode> copied = new LinkedList<AnnotationNode>(); http://git-wip-us.apache.org/repos/asf/groovy/blob/c00a75a8/src/main/org/codehaus/groovy/transform/trait/TraitComposer.java ---------------------------------------------------------------------- diff --git a/src/main/org/codehaus/groovy/transform/trait/TraitComposer.java b/src/main/org/codehaus/groovy/transform/trait/TraitComposer.java index 27bbe98..763dc75 100644 --- a/src/main/org/codehaus/groovy/transform/trait/TraitComposer.java +++ b/src/main/org/codehaus/groovy/transform/trait/TraitComposer.java @@ -70,6 +70,9 @@ import java.util.LinkedList; import java.util.List; import java.util.Map; +import static org.codehaus.groovy.ast.tools.GeneralUtils.returnS; +import static org.codehaus.groovy.ast.tools.GeneralUtils.stmt; +import static org.codehaus.groovy.ast.tools.GeneralUtils.varX; import static org.codehaus.groovy.ast.tools.GenericsUtils.correctToGenericsSpecRecurse; /** @@ -208,6 +211,7 @@ public abstract class TraitComposer { String operation = methodNode.getName().substring(suffixIdx + 1); boolean getter = "get".equals(operation); ClassNode returnType = correctToGenericsSpecRecurse(genericsSpec, methodNode.getReturnType()); + int fieldMods = 0; int isStatic = 0; boolean publicField = true; FieldNode helperField = fieldHelperClassNode.getField(Traits.FIELD_PREFIX + Traits.PUBLIC_FIELD_PREFIX + fieldName); @@ -223,15 +227,30 @@ public abstract class TraitComposer { publicField = false; helperField = fieldHelperClassNode.getField(Traits.STATIC_FIELD_PREFIX+Traits.PRIVATE_FIELD_PREFIX +fieldName); } + fieldMods = fieldMods | Opcodes.ACC_STATIC; isStatic = Opcodes.ACC_STATIC; } + if (helperField == null) { + fieldMods = 0; + isStatic = 0; + for (Integer mod : Traits.FIELD_PREFIXES) { + helperField = fieldHelperClassNode.getField(String.format("$0x%04x", mod) + fieldName); + if (helperField != null) { + if ((mod & Opcodes.ACC_STATIC) != 0) isStatic = Opcodes.ACC_STATIC; + fieldMods = fieldMods | mod; + break; + } + } + } else { + fieldMods = fieldMods | (publicField?Opcodes.ACC_PUBLIC:Opcodes.ACC_PRIVATE); + } if (getter) { // add field if (helperField!=null) { List<AnnotationNode> copied = new LinkedList<AnnotationNode>(); List<AnnotationNode> notCopied = new LinkedList<AnnotationNode>(); GeneralUtils.copyAnnotatedNodeAnnotations(helperField, copied, notCopied); - FieldNode fieldNode = cNode.addField(fieldName, (publicField?Opcodes.ACC_PUBLIC:Opcodes.ACC_PRIVATE) | isStatic, returnType, null); + FieldNode fieldNode = cNode.addField(fieldName, fieldMods, returnType, (fieldMods & Opcodes.ACC_FINAL) == 0 ? null : helperField.getInitialExpression()); fieldNode.addAnnotations(copied); } } @@ -244,28 +263,30 @@ public abstract class TraitComposer { newParams = new Parameter[]{new Parameter(fixedType, "val")}; } - Expression fieldExpr = new VariableExpression(cNode.getField(fieldName)); + Expression fieldExpr = varX(cNode.getField(fieldName)); Statement body = - getter ? new ReturnStatement(fieldExpr) : - new ExpressionStatement( + getter ? returnS(fieldExpr) : + stmt( new BinaryExpression( fieldExpr, Token.newSymbol(Types.EQUAL, 0, 0), - new VariableExpression(newParams[0]) + varX(newParams[0]) ) ); - MethodNode impl = new MethodNode( - methodNode.getName(), - Opcodes.ACC_PUBLIC | isStatic, - returnType, - newParams, - ClassNode.EMPTY_ARRAY, - body - ); - AnnotationNode an = new AnnotationNode(COMPILESTATIC_CLASSNODE); - impl.addAnnotation(an); - cNode.addTransform(StaticCompileTransformation.class, an); - cNode.addMethod(impl); + if (getter || (fieldMods & Opcodes.ACC_FINAL) == 0) { + MethodNode impl = new MethodNode( + methodNode.getName(), + Opcodes.ACC_PUBLIC | isStatic, + returnType, + newParams, + ClassNode.EMPTY_ARRAY, + body + ); + AnnotationNode an = new AnnotationNode(COMPILESTATIC_CLASSNODE); + impl.addAnnotation(an); + cNode.addTransform(StaticCompileTransformation.class, an); + cNode.addMethod(impl); + } } } } http://git-wip-us.apache.org/repos/asf/groovy/blob/c00a75a8/src/main/org/codehaus/groovy/transform/trait/Traits.java ---------------------------------------------------------------------- diff --git a/src/main/org/codehaus/groovy/transform/trait/Traits.java b/src/main/org/codehaus/groovy/transform/trait/Traits.java index 61d602f..4a0ac91 100644 --- a/src/main/org/codehaus/groovy/transform/trait/Traits.java +++ b/src/main/org/codehaus/groovy/transform/trait/Traits.java @@ -34,12 +34,14 @@ import org.codehaus.groovy.ast.expr.ListExpression; import org.codehaus.groovy.ast.tools.GenericsUtils; import org.codehaus.groovy.classgen.asm.BytecodeHelper; import org.codehaus.groovy.runtime.DefaultGroovyMethods; +import org.objectweb.asm.Opcodes; import java.lang.annotation.ElementType; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; import java.lang.reflect.Method; +import java.util.Arrays; import java.util.Iterator; import java.util.LinkedHashSet; import java.util.List; @@ -71,6 +73,13 @@ public abstract class Traits { static final String FIELD_PREFIX = "$ins"; static final String PUBLIC_FIELD_PREFIX = "$0"; static final String PRIVATE_FIELD_PREFIX = "$1"; + // TODO decide if we should support VOLATILE +// def hex(s) {new BigInteger(s, 16).intValue()} +// def optionals = [[0, 1], [0, 1], [0, 1], [0, 1]].combinations{ a, b, c, d -> +// (a ? hex('80') : 0) + (b ? hex('10') : 0) + (c ? hex('8') : 0) + (d ? hex('2') : hex('1')) +// }.sort() + static final List<Integer> FIELD_PREFIXES = Arrays.asList(1, 2, 9, 10, 17, 18, 25, 26, 129, 130, 137, 138, 145, 146, 153, 154); + static final int FIELD_PREFIX_MASK = Opcodes.ACC_PRIVATE | Opcodes.ACC_PUBLIC | Opcodes.ACC_STATIC | Opcodes.ACC_FINAL | Opcodes.ACC_TRANSIENT; static final String SUPER_TRAIT_METHOD_PREFIX = "trait$super$"; static String fieldHelperClassName(final ClassNode traitNode) { http://git-wip-us.apache.org/repos/asf/groovy/blob/c00a75a8/src/test/groovy/bugs/Groovy7584Bug.groovy ---------------------------------------------------------------------- diff --git a/src/test/groovy/bugs/Groovy7584Bug.groovy b/src/test/groovy/bugs/Groovy7584Bug.groovy new file mode 100644 index 0000000..6f4772f --- /dev/null +++ b/src/test/groovy/bugs/Groovy7584Bug.groovy @@ -0,0 +1,56 @@ +/* + * 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.bugs + +class Groovy7584Bug extends GroovyTestCase { + void testTraitFieldModifiersAreRetained() { + assertScript """ + import static java.lang.reflect.Modifier.* + + trait User { + final String name = 'Foo' + public static final boolean loggedIn = true + private transient final int ANSWER = 42 + } + + @groovy.transform.ToString(includeFields=true, includeNames=true) + class Person implements User { } + + def tos = new Person().toString() + assert tos.contains('name:Foo') + assert tos.contains('User__ANSWER:42') + assert tos.contains('User__name:Foo') + + def loggedInField = Person.getDeclaredFields().find { + it.name.contains('loggedIn') + } + assert isPublic(loggedInField.modifiers) + assert isFinal(loggedInField.modifiers) + assert isStatic(loggedInField.modifiers) + assert Person.User__loggedIn + + def answerField = Person.getDeclaredFields().find { + it.name.contains('ANSWER') + } + assert isPrivate(answerField.modifiers) + assert isTransient(answerField.modifiers) + assert isFinal(answerField.modifiers) + """ + } +}
