Repository: groovy Updated Branches: refs/heads/GROOVY_2_4_X ddf523e92 -> 69ace4d8c
GROOVY-7969: Incorrect modifers on setter for volatile property with @Bindable/@Vetoable (closes #448) Project: http://git-wip-us.apache.org/repos/asf/groovy/repo Commit: http://git-wip-us.apache.org/repos/asf/groovy/commit/69ace4d8 Tree: http://git-wip-us.apache.org/repos/asf/groovy/tree/69ace4d8 Diff: http://git-wip-us.apache.org/repos/asf/groovy/diff/69ace4d8 Branch: refs/heads/GROOVY_2_4_X Commit: 69ace4d8c7d3963e1843df8e35806d40082b6889 Parents: ddf523e Author: paulk <pa...@asert.com.au> Authored: Wed Oct 12 21:42:17 2016 +1000 Committer: paulk <pa...@asert.com.au> Committed: Thu Oct 13 17:28:12 2016 +1000 ---------------------------------------------------------------------- .../groovy/beans/BindableASTTransformation.java | 3 +- .../groovy/beans/VetoableASTTransformation.java | 3 +- .../groovy/ast/tools/PropertyNodeUtils.java | 43 ++++ .../org/codehaus/groovy/classgen/Verifier.java | 235 ++++++++++--------- src/test/groovy/bugs/Groovy7969Bug.groovy | 33 +++ 5 files changed, 204 insertions(+), 113 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/groovy/blob/69ace4d8/src/main/groovy/beans/BindableASTTransformation.java ---------------------------------------------------------------------- diff --git a/src/main/groovy/beans/BindableASTTransformation.java b/src/main/groovy/beans/BindableASTTransformation.java index 31ac190..f987295 100644 --- a/src/main/groovy/beans/BindableASTTransformation.java +++ b/src/main/groovy/beans/BindableASTTransformation.java @@ -30,6 +30,7 @@ import org.codehaus.groovy.ast.PropertyNode; import org.codehaus.groovy.ast.expr.Expression; import org.codehaus.groovy.ast.stmt.BlockStatement; import org.codehaus.groovy.ast.stmt.Statement; +import org.codehaus.groovy.ast.tools.PropertyNodeUtils; import org.codehaus.groovy.control.CompilePhase; import org.codehaus.groovy.control.SourceUnit; import org.codehaus.groovy.control.messages.SimpleMessage; @@ -247,7 +248,7 @@ public class BindableASTTransformation implements ASTTransformation, Opcodes { protected void createSetterMethod(ClassNode declaringClass, PropertyNode propertyNode, String setterName, Statement setterBlock) { MethodNode setter = new MethodNode( setterName, - propertyNode.getModifiers(), + PropertyNodeUtils.adjustPropertyModifiersForMethod(propertyNode), ClassHelper.VOID_TYPE, params(param(propertyNode.getType(), "value")), ClassNode.EMPTY_ARRAY, http://git-wip-us.apache.org/repos/asf/groovy/blob/69ace4d8/src/main/groovy/beans/VetoableASTTransformation.java ---------------------------------------------------------------------- diff --git a/src/main/groovy/beans/VetoableASTTransformation.java b/src/main/groovy/beans/VetoableASTTransformation.java index 67a1617..d7353fd 100644 --- a/src/main/groovy/beans/VetoableASTTransformation.java +++ b/src/main/groovy/beans/VetoableASTTransformation.java @@ -30,6 +30,7 @@ import org.codehaus.groovy.ast.PropertyNode; import org.codehaus.groovy.ast.expr.Expression; import org.codehaus.groovy.ast.stmt.BlockStatement; import org.codehaus.groovy.ast.stmt.Statement; +import org.codehaus.groovy.ast.tools.PropertyNodeUtils; import org.codehaus.groovy.control.CompilePhase; import org.codehaus.groovy.control.SourceUnit; import org.codehaus.groovy.control.messages.SimpleMessage; @@ -312,7 +313,7 @@ public class VetoableASTTransformation extends BindableASTTransformation { ClassNode[] exceptions = {ClassHelper.make(PropertyVetoException.class)}; MethodNode setter = new MethodNode( setterName, - propertyNode.getModifiers(), + PropertyNodeUtils.adjustPropertyModifiersForMethod(propertyNode), ClassHelper.VOID_TYPE, params(param(propertyNode.getType(), "value")), exceptions, http://git-wip-us.apache.org/repos/asf/groovy/blob/69ace4d8/src/main/org/codehaus/groovy/ast/tools/PropertyNodeUtils.java ---------------------------------------------------------------------- diff --git a/src/main/org/codehaus/groovy/ast/tools/PropertyNodeUtils.java b/src/main/org/codehaus/groovy/ast/tools/PropertyNodeUtils.java new file mode 100644 index 0000000..513144a --- /dev/null +++ b/src/main/org/codehaus/groovy/ast/tools/PropertyNodeUtils.java @@ -0,0 +1,43 @@ +/* + * 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 org.codehaus.groovy.ast.tools; + +import org.codehaus.groovy.ast.PropertyNode; + +import java.lang.reflect.Modifier; + +public class PropertyNodeUtils { + /** + * Fields within the AST that have no explicit visibility are deemed to be properties + * and represented by a PropertyNode. The Groovy compiler creates accessor methods and + * a backing field for such property nodes. During this process, all modifiers + * from the property are carried over to the backing field (so a property marked as + * {@code transient} will have a {@code transient} backing field) but when creating + * the accessor methods we don't carry over modifier values which don't make sense for + * methods (this includes VOLATILE and TRANSIENT) but other modifiers are carried over, + * for example {@code static}. + * + * @param propNode the original property node + * @return the modifiers which make sense for an accessor method + */ + public static int adjustPropertyModifiersForMethod(PropertyNode propNode) { + // GROOVY-3726: clear volatile, transient modifiers so that they don't get applied to methods + return ~(Modifier.TRANSIENT | Modifier.VOLATILE) & propNode.getModifiers(); + } +} http://git-wip-us.apache.org/repos/asf/groovy/blob/69ace4d8/src/main/org/codehaus/groovy/classgen/Verifier.java ---------------------------------------------------------------------- diff --git a/src/main/org/codehaus/groovy/classgen/Verifier.java b/src/main/org/codehaus/groovy/classgen/Verifier.java index c3d01e6..78e53d0 100644 --- a/src/main/org/codehaus/groovy/classgen/Verifier.java +++ b/src/main/org/codehaus/groovy/classgen/Verifier.java @@ -21,6 +21,7 @@ package org.codehaus.groovy.classgen; import groovy.lang.GroovyClassLoader; import groovy.lang.GroovyObject; import groovy.lang.MetaClass; +import org.codehaus.groovy.GroovyBugError; import org.codehaus.groovy.ast.*; import org.codehaus.groovy.ast.expr.ArgumentListExpression; import org.codehaus.groovy.ast.expr.BinaryExpression; @@ -39,6 +40,7 @@ import org.codehaus.groovy.ast.stmt.ReturnStatement; import org.codehaus.groovy.ast.stmt.Statement; import org.codehaus.groovy.ast.tools.ClassNodeUtils; import org.codehaus.groovy.ast.tools.GenericsUtils; +import org.codehaus.groovy.ast.tools.PropertyNodeUtils; import org.codehaus.groovy.classgen.asm.BytecodeHelper; import org.codehaus.groovy.classgen.asm.MopWriter; import org.codehaus.groovy.classgen.asm.OptimizingStatementWriter.ClassNodeSkip; @@ -66,15 +68,35 @@ import java.util.List; import java.util.Map; import java.util.Set; +import static java.lang.reflect.Modifier.isAbstract; +import static java.lang.reflect.Modifier.isFinal; +import static java.lang.reflect.Modifier.isPrivate; +import static java.lang.reflect.Modifier.isPublic; +import static java.lang.reflect.Modifier.isStatic; import static org.codehaus.groovy.ast.tools.GeneralUtils.makeDescriptorWithoutReturnType; import static org.codehaus.groovy.ast.tools.GenericsUtils.correctToGenericsSpec; import static org.codehaus.groovy.ast.tools.GenericsUtils.createGenericsSpec; /** - * Verifies the AST node and adds any defaulted AST code before - * bytecode generation occurs. + * Verifies the AST node and adds any default AST code before bytecode generation occurs. * - * @author <a href="mailto:ja...@coredevelopers.net">James Strachan</a> + * Checks include: + * <ul> + * <li>Methods with duplicate signatures</li> + * <li>Duplicate interfaces</li> + * <li>Reassigned final variables/parameters</li> + * <li>Uninitialized variables</li> + * <li>Bad code in object initializers or constructors</li> + * <li>Mismatches in modifiers or return types between implementations and interfaces/abstract classes</li> + * </ul> + * + * Added code includes: + * <ul> + * <li>Methods needed to implement GroovyObject</li> + * <li>Property accessor methods</li> + * <li>Covariant methods</li> + * <li>Additional methods/constructors as needed for default parameters</li> + * </ul> */ public class Verifier implements GroovyClassVisitor, Opcodes { @@ -121,12 +143,13 @@ public class Verifier implements GroovyClassVisitor, Opcodes { if (metaClassField != null) return metaClassField; final String classInternalName = BytecodeHelper.getClassInternalName(node); metaClassField = - node.addField("metaClass", ACC_PRIVATE | ACC_TRANSIENT | ACC_SYNTHETIC, ClassHelper.METACLASS_TYPE, - new BytecodeExpression(ClassHelper.METACLASS_TYPE) { - public void visit(MethodVisitor mv) { - mv.visitVarInsn(ALOAD, 0); - mv.visitMethodInsn(INVOKEVIRTUAL, classInternalName, "$getStaticMetaClass", "()Lgroovy/lang/MetaClass;", false); - }}); + node.addField("metaClass", ACC_PRIVATE | ACC_TRANSIENT | ACC_SYNTHETIC, ClassHelper.METACLASS_TYPE, + new BytecodeExpression(ClassHelper.METACLASS_TYPE) { + public void visit(MethodVisitor mv) { + mv.visitVarInsn(ALOAD, 0); + mv.visitMethodInsn(INVOKEVIRTUAL, classInternalName, "$getStaticMetaClass", "()Lgroovy/lang/MetaClass;", false); + } + }); metaClassField.setSynthetic(true); return metaClassField; } @@ -148,29 +171,29 @@ public class Verifier implements GroovyClassVisitor, Opcodes { if (current == null) break; ret = current.getDeclaredField("metaClass"); if (ret == null) continue; - if (Modifier.isPrivate(ret.getModifiers())) continue; + if (isPrivate(ret.getModifiers())) continue; return ret; } return null; } /** - * add code to implement GroovyObject + * walk the class * - * @param node + * @param node the node to visit */ public void visitClass(final ClassNode node) { this.classNode = node; if (Traits.isTrait(node) // maybe possible to have this true in joint compilation mode - || ((classNode.getModifiers() & Opcodes.ACC_INTERFACE) > 0)) { + || classNode.isInterface()) { //interfaces have no constructors, but this code expects one, //so create a dummy and don't add it to the class node ConstructorNode dummy = new ConstructorNode(0, null); addInitialization(node, dummy); node.visitContents(this); - if (classNode.getNodeMetaData(ClassNodeSkip.class)==null) { - classNode.setNodeMetaData(ClassNodeSkip.class,true); + if (classNode.getNodeMetaData(ClassNodeSkip.class) == null) { + classNode.setNodeMetaData(ClassNodeSkip.class, true); } return; } @@ -237,21 +260,21 @@ public class Verifier implements GroovyClassVisitor, Opcodes { private static FieldNode checkFieldDoesNotExist(ClassNode node, String fieldName) { FieldNode ret = node.getDeclaredField(fieldName); if (ret != null) { - if ( Modifier.isPublic(ret.getModifiers()) && - ret.getType().redirect()==ClassHelper.boolean_TYPE) { + if (isPublic(ret.getModifiers()) && + ret.getType().redirect() == ClassHelper.boolean_TYPE) { return ret; } throw new RuntimeParserException("The class " + node.getName() + - " cannot declare field '"+fieldName+"' as this" + + " cannot declare field '" + fieldName + "' as this" + " field is needed for internal groovy purposes", ret); } return null; } private static void addFastPathHelperFieldsAndHelperMethod(ClassNode node, final String classInternalName, boolean knownSpecialCase) { - if (node.getNodeMetaData(ClassNodeSkip.class)!=null) return; - FieldNode stMCB = checkFieldDoesNotExist(node,STATIC_METACLASS_BOOL); - if (stMCB==null) { + if (node.getNodeMetaData(ClassNodeSkip.class) != null) return; + FieldNode stMCB = checkFieldDoesNotExist(node, STATIC_METACLASS_BOOL); + if (stMCB == null) { stMCB = node.addField( STATIC_METACLASS_BOOL, ACC_PUBLIC | ACC_STATIC | ACC_SYNTHETIC | ACC_TRANSIENT, @@ -291,7 +314,7 @@ public class Verifier implements GroovyClassVisitor, Opcodes { mv.visitVarInsn(ALOAD, 0); mv.visitMethodInsn(INVOKEVIRTUAL, "java/lang/Object", "getClass", "()Ljava/lang/Class;", false); if (BytecodeHelper.isClassLiteralPossible(node) || BytecodeHelper.isSameCompilationUnit(classNode, node)) { - BytecodeHelper.visitClassLiteral(mv,node); + BytecodeHelper.visitClassLiteral(mv, node); } else { mv.visitMethodInsn(INVOKESTATIC, classInternalName, "$get$$class$" + classInternalName.replaceAll("\\/", "\\$"), "()Ljava/lang/Class;", false); } @@ -334,7 +357,7 @@ public class Verifier implements GroovyClassVisitor, Opcodes { if (!node.hasMethod("getMetaClass", Parameter.EMPTY_ARRAY)) { metaClassField = setMetaClassFieldIfNotExists(node, metaClassField); - addMethod(node, !Modifier.isAbstract(node.getModifiers()), + addMethod(node, !isAbstract(node.getModifiers()), "getMetaClass", ACC_PUBLIC, ClassHelper.METACLASS_TYPE, @@ -379,7 +402,7 @@ public class Verifier implements GroovyClassVisitor, Opcodes { if (!node.hasMethod("setMetaClass", parameters)) { metaClassField = setMetaClassFieldIfNotExists(node, metaClassField); Statement setMetaClassCode; - if (Modifier.isFinal(metaClassField.getModifiers())) { + if (isFinal(metaClassField.getModifiers())) { ConstantExpression text = new ConstantExpression("cannot set read-only meta class"); ConstructorCallExpression cce = new ConstructorCallExpression(ClassHelper.make(IllegalArgumentException.class), text); setMetaClassCode = new ExpressionStatement(cce); @@ -401,7 +424,7 @@ public class Verifier implements GroovyClassVisitor, Opcodes { setMetaClassCode = new BytecodeSequence(list); } - addMethod(node, !Modifier.isAbstract(node.getModifiers()), + addMethod(node, !isAbstract(node.getModifiers()), "setMetaClass", ACC_PUBLIC, ClassHelper.VOID_TYPE, SET_METACLASS_PARAMS, ClassNode.EMPTY_ARRAY, @@ -416,7 +439,7 @@ public class Verifier implements GroovyClassVisitor, Opcodes { blockScope.putReferencedLocalVariable(vMethods); blockScope.putReferencedLocalVariable(vArguments); - addMethod(node, !Modifier.isAbstract(node.getModifiers()), + addMethod(node, !isAbstract(node.getModifiers()), "invokeMethod", ACC_PUBLIC, ClassHelper.OBJECT_TYPE, INVOKE_METHOD_PARAMS, @@ -436,7 +459,7 @@ public class Verifier implements GroovyClassVisitor, Opcodes { } if (!node.hasMethod("getProperty", GET_PROPERTY_PARAMS)) { - addMethod(node, !Modifier.isAbstract(node.getModifiers()), + addMethod(node, !isAbstract(node.getModifiers()), "getProperty", ACC_PUBLIC, ClassHelper.OBJECT_TYPE, @@ -456,7 +479,7 @@ public class Verifier implements GroovyClassVisitor, Opcodes { } if (!node.hasMethod("setProperty", SET_PROPERTY_PARAMS)) { - addMethod(node, !Modifier.isAbstract(node.getModifiers()), + addMethod(node, !isAbstract(node.getModifiers()), "setProperty", ACC_PUBLIC, ClassHelper.VOID_TYPE, @@ -495,7 +518,7 @@ public class Verifier implements GroovyClassVisitor, Opcodes { protected void addTimeStamp(ClassNode node) { } - private static void checkReturnInObjectInitializer(List init) { + private static void checkReturnInObjectInitializer(List<Statement> init) { CodeVisitorSupport cvs = new CodeVisitorSupport() { @Override public void visitClosureExpression(ClosureExpression expression) { @@ -506,8 +529,7 @@ public class Verifier implements GroovyClassVisitor, Opcodes { throw new RuntimeParserException("'return' is not allowed in object initializer", statement); } }; - for (Iterator iterator = init.iterator(); iterator.hasNext();) { - Statement stm = (Statement) iterator.next(); + for (Statement stm : init) { stm.visit(cvs); } } @@ -554,7 +576,7 @@ public class Verifier implements GroovyClassVisitor, Opcodes { public void visitMethod(MethodNode node) { //GROOVY-3712 - if it's an MOP method, it's an error as they aren't supposed to exist before ACG is invoked - if(MopWriter.isMopMethod(node.getName())) { + if (MopWriter.isMopMethod(node.getName())) { throw new RuntimeParserException("Found unexpected MOP methods in the class node for " + classNode.getName() + "(" + node.getName() + ")", classNode); } @@ -596,25 +618,18 @@ public class Verifier implements GroovyClassVisitor, Opcodes { // method is in current class, nothing to be done if (m.getDeclaringClass() == this.getClassNode()) return false; // do not overwrite final - if ((m.getModifiers() & ACC_FINAL) != 0) return false; + if (isFinal(m.getModifiers())) return false; return true; } public void visitProperty(PropertyNode node) { String name = node.getName(); FieldNode field = node.getField(); - int propNodeModifiers = node.getModifiers(); String getterName = "get" + capitalize(name); String setterName = "set" + capitalize(name); - // GROOVY-3726: clear volatile, transient modifiers so that they don't get applied to methods - if ((propNodeModifiers & Modifier.VOLATILE) != 0) { - propNodeModifiers -= Modifier.VOLATILE; - } - if ((propNodeModifiers & Modifier.TRANSIENT) != 0) { - propNodeModifiers -= Modifier.TRANSIENT; - } + int accessorModifiers = PropertyNodeUtils.adjustPropertyModifiersForMethod(node); Statement getterBlock = node.getGetterBlock(); if (getterBlock == null) { @@ -631,17 +646,15 @@ public class Verifier implements GroovyClassVisitor, Opcodes { if (setterBlock == null) { // 2nd arg false below: though not usual, allow setter with non-void return type MethodNode setter = classNode.getSetterMethod(setterName, false); - if (!node.isPrivate() && - (propNodeModifiers & ACC_FINAL) == 0 && - methodNeedsReplacement(setter)) { + if (!node.isPrivate() && !isFinal(accessorModifiers) && methodNeedsReplacement(setter)) { setterBlock = createSetterBlock(node, field); } } - int getterModifiers = propNodeModifiers; + int getterModifiers = accessorModifiers; // don't make static accessors final - if (node.isStatic() && (propNodeModifiers & Modifier.FINAL) != 0) { - getterModifiers -= Modifier.FINAL; + if (node.isStatic()) { + getterModifiers = ~Modifier.FINAL & getterModifiers; } if (getterBlock != null) { MethodNode getter = @@ -662,7 +675,7 @@ public class Verifier implements GroovyClassVisitor, Opcodes { if (setterBlock != null) { Parameter[] setterParameterTypes = {new Parameter(node.getType(), "value")}; MethodNode setter = - new MethodNode(setterName, propNodeModifiers, ClassHelper.VOID_TYPE, setterParameterTypes, ClassNode.EMPTY_ARRAY, setterBlock); + new MethodNode(setterName, accessorModifiers, ClassHelper.VOID_TYPE, setterParameterTypes, ClassNode.EMPTY_ARRAY, setterBlock); setter.setSynthetic(true); addPropertyMethod(setter); visitMethod(setter); @@ -673,15 +686,14 @@ public class Verifier implements GroovyClassVisitor, Opcodes { classNode.addMethod(method); // GROOVY-4415 / GROOVY-4645: check that there's no abstract method which corresponds to this one List<MethodNode> abstractMethods = classNode.getAbstractMethods(); - if (abstractMethods==null) return; + if (abstractMethods == null) return; String methodName = method.getName(); Parameter[] parameters = method.getParameters(); ClassNode methodReturnType = method.getReturnType(); for (MethodNode node : abstractMethods) { if (!node.getDeclaringClass().equals(classNode)) continue; - if (node.getName().equals(methodName) - && node.getParameters().length==parameters.length) { - if (parameters.length==1) { + if (node.getName().equals(methodName) && node.getParameters().length == parameters.length) { + if (parameters.length == 1) { // setter ClassNode abstractMethodParameterType = node.getParameters()[0].getType(); ClassNode methodParameterType = parameters[0].getType(); @@ -700,9 +712,6 @@ public class Verifier implements GroovyClassVisitor, Opcodes { } } - // Implementation methods - //------------------------------------------------------------------------- - public interface DefaultArgsAction { void call(ArgumentListExpression arguments, Parameter[] newParams, MethodNode method); } @@ -731,7 +740,7 @@ public class Verifier implements GroovyClassVisitor, Opcodes { } // check whether closure shared variables refer to params with default values (GROOVY-5632) - if (argument instanceof ClosureExpression) { + if (argument instanceof ClosureExpression) { final List<Parameter> newMethodNodeParameters = Arrays.asList(newParams); CodeVisitorSupport visitor = new CodeVisitorSupport() { @@ -741,7 +750,7 @@ public class Verifier implements GroovyClassVisitor, Opcodes { if (!(v instanceof Parameter)) return; Parameter param = (Parameter) v; - if (param.hasInitialExpression() && code.getVariableScope().getDeclaredVariable(param.getName()) == null && !newMethodNodeParameters.contains(param)) { + if (param.hasInitialExpression() && code.getVariableScope().getDeclaredVariable(param.getName()) == null && !newMethodNodeParameters.contains(param)) { VariableExpression localVariable = new VariableExpression(param.getName(), ClassHelper.makeReference()); DeclarationExpression declarationExpression = new DeclarationExpression(localVariable, Token.newSymbol(Types.EQUAL, -1, -1), new ConstructorCallExpression(ClassHelper.makeReference(), param.getInitialExpression())); @@ -767,7 +776,7 @@ public class Verifier implements GroovyClassVisitor, Opcodes { } List<AnnotationNode> annotations = method.getAnnotations(); - if(annotations != null) { + if (annotations != null) { newMethod.addAnnotations(annotations); } MethodNode oldMethod = node.getDeclaredMethod(method.getName(), newParams); @@ -805,8 +814,8 @@ public class Verifier implements GroovyClassVisitor, Opcodes { * Creates a new helper method for each combination of default parameter expressions */ protected void addDefaultParameters(List methods, DefaultArgsAction action) { - for (Iterator iter = methods.iterator(); iter.hasNext();) { - MethodNode method = (MethodNode) iter.next(); + for (Object next : methods) { + MethodNode method = (MethodNode) next; if (method.hasDefaultValue()) { addDefaultParameters(action, method); } @@ -821,7 +830,7 @@ public class Verifier implements GroovyClassVisitor, Opcodes { for (int i = size - 1; i >= 0; i--) { Parameter parameter = parameters[i]; if (parameter != null && parameter.hasInitialExpression()) { - paramValues.add(Integer.valueOf(i)); + paramValues.add(i); paramValues.add( new CastExpression( parameter.getType(), @@ -837,32 +846,36 @@ public class Verifier implements GroovyClassVisitor, Opcodes { ArgumentListExpression arguments = new ArgumentListExpression(); int index = 0; int k = 1; - for (int i = 0; i < parameters.length; i++) { - if (k > counter - j && parameters[i] != null && parameters[i].hasInitialExpression()) { - arguments.addExpression( - new CastExpression( - parameters[i].getType(), - parameters[i].getInitialExpression() - ) - ); - k++; - } else if (parameters[i] != null && parameters[i].hasInitialExpression()) { - newParams[index++] = parameters[i]; - arguments.addExpression( - new CastExpression( - parameters[i].getType(), - new VariableExpression(parameters[i].getName()) - ) - ); - k++; + for (Parameter parameter : parameters) { + if (parameter == null) { + throw new GroovyBugError("Parameter should not be null for method " + methodNode.getName()); } else { - newParams[index++] = parameters[i]; - arguments.addExpression( - new CastExpression( - parameters[i].getType(), - new VariableExpression(parameters[i].getName()) - ) - ); + if (k > counter - j && parameter.hasInitialExpression()) { + arguments.addExpression( + new CastExpression( + parameter.getType(), + parameter.getInitialExpression() + ) + ); + k++; + } else if (parameter.hasInitialExpression()) { + newParams[index++] = parameter; + arguments.addExpression( + new CastExpression( + parameter.getType(), + new VariableExpression(parameter.getName()) + ) + ); + k++; + } else { + newParams[index++] = parameter; + arguments.addExpression( + new CastExpression( + parameter.getType(), + new VariableExpression(parameter.getName()) + ) + ); + } } } action.call(arguments, newParams, method); @@ -895,7 +908,7 @@ public class Verifier implements GroovyClassVisitor, Opcodes { } }); - List<Statement> swapCall= new ArrayList<Statement>(1); + List<Statement> swapCall = new ArrayList<Statement>(1); swapCall.add(seq); node.addStaticInitializerStatements(swapCall, true); } @@ -967,7 +980,6 @@ public class Verifier implements GroovyClassVisitor, Opcodes { constructorNode.setCode(newBlock); - if (!staticStatements.isEmpty()) { if (isEnum) { /* @@ -1122,7 +1134,7 @@ public class Verifier implements GroovyClassVisitor, Opcodes { final Field[] fields = clazz.getFields(); for (int i = 0; i != fields.length; ++i) { - if (Modifier.isStatic(fields[i].getModifiers())) { + if (isStatic(fields[i].getModifiers())) { final String name = fields[i].getName(); if (name.startsWith(__TIMESTAMP__)) { try { @@ -1147,7 +1159,7 @@ public class Verifier implements GroovyClassVisitor, Opcodes { List<MethodNode> declaredMethods = new ArrayList<MethodNode>(classNode.getMethods()); // remove all static, private and package private methods - for (Iterator methodsIterator = declaredMethods.iterator(); methodsIterator.hasNext();) { + for (Iterator methodsIterator = declaredMethods.iterator(); methodsIterator.hasNext(); ) { MethodNode m = (MethodNode) methodsIterator.next(); abstractMethods.remove(m.getTypeDescriptor()); if (m.isStatic() || !(m.isPublic() || m.isProtected())) { @@ -1272,7 +1284,7 @@ public class Verifier implements GroovyClassVisitor, Opcodes { boolean oldM = ClassHelper.isPrimitiveType(oldMethod.getReturnType()); boolean newM = ClassHelper.isPrimitiveType(overridingMethod.getReturnType()); if (oldM || newM) { - String message=""; + String message = ""; if (oldM && newM) { message = " with old and new method having different primitive return types"; } else if (newM) { @@ -1282,9 +1294,9 @@ public class Verifier implements GroovyClassVisitor, Opcodes { } throw new RuntimeParserException( "Cannot override method " + - oldMethod.getTypeDescriptor() + - " in " + oldMethod.getDeclaringClass().getName() + - message, + oldMethod.getTypeDescriptor() + + " in " + oldMethod.getDeclaringClass().getName() + + message, overridingMethod); } } @@ -1303,20 +1315,19 @@ public class Verifier implements GroovyClassVisitor, Opcodes { instructions.add( new BytecodeInstruction() { public void visit(MethodVisitor mv) { - mv.visitVarInsn(ALOAD,0); + mv.visitVarInsn(ALOAD, 0); Parameter[] para = oldMethod.getParameters(); Parameter[] goal = overridingMethod.getParameters(); int doubleSlotOffset = 0; for (int i = 0; i < para.length; i++) { ClassNode type = para[i].getType(); - BytecodeHelper.load(mv, type, i+1+doubleSlotOffset); - if (type.redirect()==ClassHelper.double_TYPE || - type.redirect()==ClassHelper.long_TYPE) - { + BytecodeHelper.load(mv, type, i + 1 + doubleSlotOffset); + if (type.redirect() == ClassHelper.double_TYPE || + type.redirect() == ClassHelper.long_TYPE) { doubleSlotOffset++; } if (!type.equals(goal[i].getType())) { - BytecodeHelper.doCast(mv,goal[i].getType()); + BytecodeHelper.doCast(mv, goal[i].getType()); } } mv.visitMethodInsn(INVOKEVIRTUAL, BytecodeHelper.getClassInternalName(classNode), overridingMethod.getName(), BytecodeHelper.getMethodDescriptor(overridingMethod.getReturnType(), overridingMethod.getParameters()), false); @@ -1341,7 +1352,9 @@ public class Verifier implements GroovyClassVisitor, Opcodes { } private boolean isArrayAssignable(ClassNode node, ClassNode testNode) { - if (node.isArray() && testNode.isArray()) { return isArrayAssignable(node.getComponentType(), testNode.getComponentType()); } + if (node.isArray() && testNode.isArray()) { + return isArrayAssignable(node.getComponentType(), testNode.getComponentType()); + } return isAssignable(node, testNode); } @@ -1360,8 +1373,8 @@ public class Verifier implements GroovyClassVisitor, Opcodes { } private void storeMissingCovariantMethods(Collection methods, MethodNode method, Map methodsToAdd, Map genericsSpec, boolean ignoreError) { - for (Object method1 : methods) { - MethodNode toOverride = (MethodNode) method1; + for (Object next : methods) { + MethodNode toOverride = (MethodNode) next; MethodNode bridgeMethod = getCovariantImplementation(toOverride, method, genericsSpec, ignoreError); if (bridgeMethod == null) continue; methodsToAdd.put(bridgeMethod.getTypeDescriptor(), bridgeMethod); @@ -1397,7 +1410,7 @@ public class Verifier implements GroovyClassVisitor, Opcodes { private static boolean moveOptimizedConstantsInitialization(final ClassNode node) { if (node.isInterface() && !Traits.isTrait(node)) return false; - final int mods = Opcodes.ACC_STATIC|Opcodes.ACC_SYNTHETIC| Opcodes.ACC_PUBLIC; + final int mods = Opcodes.ACC_STATIC | Opcodes.ACC_SYNTHETIC | Opcodes.ACC_PUBLIC; String name = SWAP_INIT; BlockStatement methodCode = new BlockStatement(); @@ -1405,7 +1418,7 @@ public class Verifier implements GroovyClassVisitor, Opcodes { boolean swapInitRequired = false; for (FieldNode fn : node.getFields()) { if (!fn.isStatic() || !fn.isSynthetic() || !fn.getName().startsWith("$const$")) continue; - if (fn.getInitialExpression()==null) continue; + if (fn.getInitialExpression() == null) continue; final FieldExpression fe = new FieldExpression(fn); if (fn.getType().equals(ClassHelper.REFERENCE_TYPE)) fe.setUseReferenceDirectly(true); ConstantExpression init = (ConstantExpression) fn.getInitialExpression(); @@ -1435,8 +1448,8 @@ public class Verifier implements GroovyClassVisitor, Opcodes { * Some constant expressions are optimized to return primitive types, but not all primitives are * handled. This method guarantees to return a similar constant expression but with a primitive type * instead of a boxed type. - * - * Additionaly, single char strings are converted to 'char' types. + * <p/> + * Additionally, single char strings are converted to 'char' types. * * @param constantExpression a constant expression * @return the same instance of constant expression if the type is already primitive, or a primitive @@ -1444,12 +1457,12 @@ public class Verifier implements GroovyClassVisitor, Opcodes { */ public static ConstantExpression transformToPrimitiveConstantIfPossible(ConstantExpression constantExpression) { Object value = constantExpression.getValue(); - if (value ==null) return constantExpression; + if (value == null) return constantExpression; ConstantExpression result; ClassNode type = constantExpression.getType(); if (ClassHelper.isPrimitiveType(type)) return constantExpression; - if (value instanceof String && ((String)value).length()==1) { - result = new ConstantExpression(((String)value).charAt(0)); + if (value instanceof String && ((String) value).length() == 1) { + result = new ConstantExpression(((String) value).charAt(0)); result.setType(ClassHelper.char_TYPE); } else { type = ClassHelper.getUnwrapper(type); @@ -1465,7 +1478,7 @@ public class Verifier implements GroovyClassVisitor, Opcodes { public SwapInitStatement() { super(new SwapInitInstruction()); - ((SwapInitInstruction)getInstructions().get(0)).statement = this; + ((SwapInitInstruction) getInstructions().get(0)).statement = this; } @Override http://git-wip-us.apache.org/repos/asf/groovy/blob/69ace4d8/src/test/groovy/bugs/Groovy7969Bug.groovy ---------------------------------------------------------------------- diff --git a/src/test/groovy/bugs/Groovy7969Bug.groovy b/src/test/groovy/bugs/Groovy7969Bug.groovy new file mode 100644 index 0000000..edcaed4 --- /dev/null +++ b/src/test/groovy/bugs/Groovy7969Bug.groovy @@ -0,0 +1,33 @@ +/* + * 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 Groovy7969Bug extends GroovyTestCase { + void testBindablePropertySettersHaveValidModifiersForMethod() { + assertScript """ + import groovy.beans.Bindable + + @Bindable class Bar { + volatile Date then + } + + assert Bar.getMethod('setThen', Date).modifiers == 1 + """ + } +}