Repository: groovy Updated Branches: refs/heads/GROOVY_2_4_X a8fcbd684 -> 96b0351de
GROOVY-7291: Declaration of double without assignment null in closure (closes #446) Project: http://git-wip-us.apache.org/repos/asf/groovy/repo Commit: http://git-wip-us.apache.org/repos/asf/groovy/commit/96b0351d Tree: http://git-wip-us.apache.org/repos/asf/groovy/tree/96b0351d Diff: http://git-wip-us.apache.org/repos/asf/groovy/diff/96b0351d Branch: refs/heads/GROOVY_2_4_X Commit: 96b0351de2b25a4f32c75361ba789356067e682e Parents: a8fcbd6 Author: zhangbo <zhan...@nanchao.org> Authored: Mon Oct 10 17:53:36 2016 +0800 Committer: John Wagenleitner <jwagenleit...@apache.org> Committed: Sat Oct 15 23:14:44 2016 -0700 ---------------------------------------------------------------------- .../classgen/asm/OptimizingStatementWriter.java | 190 ++++++++++--------- src/test/groovy/bugs/Groovy7291Bug.groovy | 41 ++++ 2 files changed, 145 insertions(+), 86 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/groovy/blob/96b0351d/src/main/org/codehaus/groovy/classgen/asm/OptimizingStatementWriter.java ---------------------------------------------------------------------- diff --git a/src/main/org/codehaus/groovy/classgen/asm/OptimizingStatementWriter.java b/src/main/org/codehaus/groovy/classgen/asm/OptimizingStatementWriter.java index 9e660fa..e72b112 100644 --- a/src/main/org/codehaus/groovy/classgen/asm/OptimizingStatementWriter.java +++ b/src/main/org/codehaus/groovy/classgen/asm/OptimizingStatementWriter.java @@ -43,14 +43,14 @@ import static org.codehaus.groovy.ast.tools.WideningCategories.*; * A class to write out the optimized statements */ public class OptimizingStatementWriter extends StatementWriter { - + private static class FastPathData { private Label pathStart = new Label(); private Label afterPath = new Label(); } - + public static class ClassNodeSkip{} - + public static class StatementMeta { private boolean optimize=false; protected MethodNode target; @@ -83,7 +83,7 @@ public class OptimizingStatementWriter extends StatementWriter { MethodCaller.newStatic(BytecodeInterface8.class, "isOrigF"), MethodCaller.newStatic(BytecodeInterface8.class, "isOrigZ"), }; - + private static final MethodCaller disabledStandardMetaClass = MethodCaller.newStatic(BytecodeInterface8.class, "disabledStandardMetaClass"); private boolean fastPathBlocked = false; private WriterController controller; @@ -92,26 +92,26 @@ public class OptimizingStatementWriter extends StatementWriter { super(controller); this.controller = controller; } - + private boolean notEnableFastPath(StatementMeta meta) { // return false if cannot do fast path and if are already on the path return fastPathBlocked || meta==null || !meta.optimize || controller.isFastPath(); } - + private FastPathData writeGuards(StatementMeta meta, Statement statement) { if (notEnableFastPath(meta)) return null; controller.getAcg().onLineNumber(statement, null); MethodVisitor mv = controller.getMethodVisitor(); FastPathData fastPathData = new FastPathData(); Label slowPath = new Label(); - + for (int i=0; i<guards.length; i++) { if (meta.involvedTypes[i]) { guards[i].call(mv); mv.visitJumpInsn(IFEQ, slowPath); } } - + // meta class check with boolean holder String owner = BytecodeHelper.getClassInternalName(controller.getClassNode()); MethodNode mn = controller.getMethodNode(); @@ -119,37 +119,37 @@ public class OptimizingStatementWriter extends StatementWriter { mv.visitFieldInsn(GETSTATIC, owner, Verifier.STATIC_METACLASS_BOOL, "Z"); mv.visitJumpInsn(IFNE, slowPath); } - + //standard metaclass check disabledStandardMetaClass.call(mv); mv.visitJumpInsn(IFNE, slowPath); - + // other guards here - + mv.visitJumpInsn(GOTO, fastPathData.pathStart); mv.visitLabel(slowPath); - + return fastPathData; } - + private void writeFastPathPrelude(FastPathData meta) { MethodVisitor mv = controller.getMethodVisitor(); mv.visitJumpInsn(GOTO, meta.afterPath); mv.visitLabel(meta.pathStart); controller.switchToFastPath(); } - + private void writeFastPathEpilogue(FastPathData meta) { MethodVisitor mv = controller.getMethodVisitor(); mv.visitLabel(meta.afterPath); controller.switchToSlowPath(); } - + @Override public void writeBlockStatement(BlockStatement statement) { StatementMeta meta = (StatementMeta) statement.getNodeMetaData(StatementMeta.class); FastPathData fastPathData = writeGuards(meta, statement); - + if (fastPathData==null) { // normal mode with different paths // important is to not to have a fastpathblock here, @@ -162,13 +162,13 @@ public class OptimizingStatementWriter extends StatementWriter { fastPathBlocked = true; super.writeBlockStatement(statement); fastPathBlocked = oldFastPathBlock; - + writeFastPathPrelude(fastPathData); super.writeBlockStatement(statement); writeFastPathEpilogue(fastPathData); } } - + @Override public void writeDoWhileLoop(DoWhileStatement statement) { if (controller.isFastPath()) { @@ -176,19 +176,19 @@ public class OptimizingStatementWriter extends StatementWriter { } else { StatementMeta meta = (StatementMeta) statement.getNodeMetaData(StatementMeta.class); FastPathData fastPathData = writeGuards(meta, statement); - + boolean oldFastPathBlock = fastPathBlocked; fastPathBlocked = true; super.writeDoWhileLoop(statement); fastPathBlocked = oldFastPathBlock; - + if (fastPathData==null) return; writeFastPathPrelude(fastPathData); super.writeDoWhileLoop(statement); writeFastPathEpilogue(fastPathData); } } - + @Override protected void writeIteratorHasNext(MethodVisitor mv) { if (controller.isFastPath()) { @@ -197,7 +197,7 @@ public class OptimizingStatementWriter extends StatementWriter { super.writeIteratorHasNext(mv); } } - + @Override protected void writeIteratorNext(MethodVisitor mv) { if (controller.isFastPath()) { @@ -206,7 +206,7 @@ public class OptimizingStatementWriter extends StatementWriter { super.writeIteratorNext(mv); } } - + @Override protected void writeForInLoop(ForStatement statement) { if (controller.isFastPath()) { @@ -214,12 +214,12 @@ public class OptimizingStatementWriter extends StatementWriter { } else { StatementMeta meta = (StatementMeta) statement.getNodeMetaData(StatementMeta.class); FastPathData fastPathData = writeGuards(meta, statement); - + boolean oldFastPathBlock = fastPathBlocked; fastPathBlocked = true; super.writeForInLoop(statement); fastPathBlocked = oldFastPathBlock; - + if (fastPathData==null) return; writeFastPathPrelude(fastPathData); super.writeForInLoop(statement); @@ -234,12 +234,12 @@ public class OptimizingStatementWriter extends StatementWriter { } else { StatementMeta meta = (StatementMeta) statement.getNodeMetaData(StatementMeta.class); FastPathData fastPathData = writeGuards(meta, statement); - + boolean oldFastPathBlock = fastPathBlocked; fastPathBlocked = true; super.writeForLoopWithClosureList(statement); fastPathBlocked = oldFastPathBlock; - + if (fastPathData==null) return; writeFastPathPrelude(fastPathData); super.writeForLoopWithClosureList(statement); @@ -254,12 +254,12 @@ public class OptimizingStatementWriter extends StatementWriter { } else { StatementMeta meta = (StatementMeta) statement.getNodeMetaData(StatementMeta.class); FastPathData fastPathData = writeGuards(meta, statement); - + boolean oldFastPathBlock = fastPathBlocked; fastPathBlocked = true; super.writeWhileLoop(statement); fastPathBlocked = oldFastPathBlock; - + if (fastPathData==null) return; writeFastPathPrelude(fastPathData); super.writeWhileLoop(statement); @@ -274,19 +274,19 @@ public class OptimizingStatementWriter extends StatementWriter { } else { StatementMeta meta = (StatementMeta) statement.getNodeMetaData(StatementMeta.class); FastPathData fastPathData = writeGuards(meta, statement); - + boolean oldFastPathBlock = fastPathBlocked; fastPathBlocked = true; super.writeIfElse(statement); fastPathBlocked = oldFastPathBlock; - + if (fastPathData==null) return; writeFastPathPrelude(fastPathData); super.writeIfElse(statement); writeFastPathEpilogue(fastPathData); } } - + private boolean isNewPathFork(StatementMeta meta) { // meta.optimize -> can do fast path if (meta==null || meta.optimize==false) return false; @@ -296,7 +296,7 @@ public class OptimizingStatementWriter extends StatementWriter { if (controller.isFastPath()) return false; return true; } - + @Override public void writeReturn(ReturnStatement statement) { if (controller.isFastPath()) { @@ -305,16 +305,16 @@ public class OptimizingStatementWriter extends StatementWriter { StatementMeta meta = (StatementMeta) statement.getNodeMetaData(StatementMeta.class); if (isNewPathFork(meta) && writeDeclarationExtraction(statement)) { FastPathData fastPathData = writeGuards(meta, statement); - + boolean oldFastPathBlock = fastPathBlocked; fastPathBlocked = true; super.writeReturn(statement); fastPathBlocked = oldFastPathBlock; - + if (fastPathData==null) return; writeFastPathPrelude(fastPathData); super.writeReturn(statement); - writeFastPathEpilogue(fastPathData); + writeFastPathEpilogue(fastPathData); } else { super.writeReturn(statement); } @@ -340,19 +340,19 @@ public class OptimizingStatementWriter extends StatementWriter { // (3) fast path possible and in slow or fastPath. Nothing to do here. // // the only case we need to handle is then (2). - + if (isNewPathFork(meta) && writeDeclarationExtraction(statement)) { FastPathData fastPathData = writeGuards(meta, statement); - + boolean oldFastPathBlock = fastPathBlocked; fastPathBlocked = true; super.writeExpressionStatement(statement); fastPathBlocked = oldFastPathBlock; - + if (fastPathData==null) return; writeFastPathPrelude(fastPathData); super.writeExpressionStatement(statement); - writeFastPathEpilogue(fastPathData); + writeFastPathEpilogue(fastPathData); } else { super.writeExpressionStatement(statement); } @@ -366,15 +366,15 @@ public class OptimizingStatementWriter extends StatementWriter { ex = rs.getExpression(); } else if (statement instanceof ExpressionStatement) { ExpressionStatement es = (ExpressionStatement) statement; - ex = es.getExpression(); + ex = es.getExpression(); } else { throw new GroovyBugError("unknown statement type :"+statement.getClass()); - } + } if (!(ex instanceof DeclarationExpression)) return true; DeclarationExpression declaration = (DeclarationExpression) ex; ex = declaration.getLeftExpression(); if (ex instanceof TupleExpression) return false; - + // do declaration controller.getCompileStack().defineVariable(declaration.getVariableExpression(), false); // change statement to do assignment only @@ -390,18 +390,18 @@ public class OptimizingStatementWriter extends StatementWriter { rs.setExpression(assignment); } else if (statement instanceof ExpressionStatement) { ExpressionStatement es = (ExpressionStatement) statement; - es.setExpression(assignment); + es.setExpression(assignment); } else { throw new GroovyBugError("unknown statement type :"+statement.getClass()); } return true; } - + public static void setNodeMeta(TypeChooser chooser, ClassNode classNode) { if (classNode.getNodeMetaData(ClassNodeSkip.class)!=null) return; new OptVisitor(chooser).visitClass(classNode); } - + private static StatementMeta addMeta(ASTNode node) { StatementMeta metaOld = (StatementMeta) node.getNodeMetaData(StatementMeta.class); StatementMeta meta = metaOld; @@ -410,13 +410,13 @@ public class OptimizingStatementWriter extends StatementWriter { if (metaOld==null) node.setNodeMetaData(StatementMeta.class, meta); return meta; } - + private static StatementMeta addMeta(ASTNode node, OptimizeFlagsCollector opt) { StatementMeta meta = addMeta(node); meta.chainInvolvedTypes(opt); return meta; } - + private static class OptimizeFlagsCollector { private static class OptimizeFlagsEntry { private boolean canOptimize = false; @@ -488,7 +488,7 @@ public class OptimizingStatementWriter extends StatementWriter { current.involvedTypes = new boolean[typeMapKeyNames.length]; } } - + private static class OptVisitor extends ClassCodeVisitorSupport { private final TypeChooser typeChooser; @@ -503,7 +503,7 @@ public class OptimizingStatementWriter extends StatementWriter { private boolean optimizeMethodCall = true; private VariableScope scope; private static final VariableScope nonStaticScope = new VariableScope(); - + @Override public void visitClass(ClassNode node) { this.optimizeMethodCall = !node.implementsInterface(GROOVY_INTERCEPTABLE_TYPE); @@ -513,20 +513,20 @@ public class OptimizingStatementWriter extends StatementWriter { this.scope=null; this.node=null; } - + @Override public void visitMethod(MethodNode node) { scope = node.getVariableScope(); super.visitMethod(node); opt.reset(); } - + @Override public void visitConstructor(ConstructorNode node) { scope = node.getVariableScope(); super.visitConstructor(node); } - + @Override public void visitReturnStatement(ReturnStatement statement) { opt.push(); @@ -534,7 +534,7 @@ public class OptimizingStatementWriter extends StatementWriter { if (opt.shouldOptimize()) addMeta(statement,opt); opt.pop(opt.shouldOptimize()); } - + @Override public void visitUnaryMinusExpression(UnaryMinusExpression expression) { //TODO: implement int operations for this @@ -542,7 +542,7 @@ public class OptimizingStatementWriter extends StatementWriter { StatementMeta meta = addMeta(expression); meta.type = OBJECT_TYPE; } - + @Override public void visitUnaryPlusExpression(UnaryPlusExpression expression) { //TODO: implement int operations for this @@ -550,7 +550,7 @@ public class OptimizingStatementWriter extends StatementWriter { StatementMeta meta = addMeta(expression); meta.type = OBJECT_TYPE; } - + @Override public void visitBitwiseNegationExpression(BitwiseNegationExpression expression) { //TODO: implement int operations for this @@ -558,7 +558,7 @@ public class OptimizingStatementWriter extends StatementWriter { StatementMeta meta = addMeta(expression); meta.type = OBJECT_TYPE; } - + private void addTypeInformation(Expression expression, Expression orig) { ClassNode type = typeChooser.resolveType(expression, node); if (isPrimitiveType(type)) { @@ -568,32 +568,50 @@ public class OptimizingStatementWriter extends StatementWriter { opt.chainInvolvedType(type); } } - + @Override public void visitPrefixExpression(PrefixExpression expression) { super.visitPrefixExpression(expression); addTypeInformation(expression.getExpression(),expression); } - + @Override public void visitPostfixExpression(PostfixExpression expression) { super.visitPostfixExpression(expression); addTypeInformation(expression.getExpression(),expression); - } - + } + + private void replaceEmptyToConstantZeroIfNecessary(DeclarationExpression expression) { + // GROOVY-7291 and GROOVY-5570, a variable referenced by closure cannot be primitive type + // So here's a trick: in this case replace EmptyExpression on the right side to a ConstantExpression + Expression leftExpression = expression.getLeftExpression(); + Expression rightExpression=expression.getRightExpression(); + if (leftExpression instanceof VariableExpression + && rightExpression instanceof EmptyExpression) { + VariableExpression leftVariableExpression = (VariableExpression) leftExpression; + + if (isPrimitiveType(leftVariableExpression.getOriginType()) + && leftVariableExpression.isClosureSharedVariable()) { + expression.setRightExpression(new ConstantExpression(0)); + } + } + } + @Override public void visitDeclarationExpression(DeclarationExpression expression) { - Expression right = expression.getRightExpression(); - right.visit(this); - - ClassNode leftType = typeChooser.resolveType(expression.getLeftExpression(), node); + replaceEmptyToConstantZeroIfNecessary(expression); + Expression rightExpression = expression.getRightExpression(); + rightExpression.visit(this); + + ClassNode leftType = typeChooser.resolveType(expression.getLeftExpression(), node); ClassNode rightType = optimizeDivWithIntOrLongTarget(rightExpression, leftType); - if (rightType==null) rightType = typeChooser.resolveType(expression.getRightExpression(), node); + + if (rightType==null) rightType = typeChooser.resolveType(rightExpression, node); if (isPrimitiveType(leftType) && isPrimitiveType(rightType)) { // if right is a constant, then we optimize only if it makes // a block complete, so we set a maybe - if (right instanceof ConstantExpression) { + if (rightExpression instanceof ConstantExpression) { opt.chainCanOptimize(true); } else { opt.chainShouldOptimize(true); @@ -605,23 +623,23 @@ public class OptimizingStatementWriter extends StatementWriter { opt.chainInvolvedType(rightType); } } - + @Override public void visitBinaryExpression(BinaryExpression expression) { if (expression.getNodeMetaData(StatementMeta.class)!=null) return; super.visitBinaryExpression(expression); - + ClassNode leftType = typeChooser.resolveType(expression.getLeftExpression(), node); ClassNode rightType = typeChooser.resolveType(expression.getRightExpression(), node); ClassNode resultType = null; int operation = expression.getOperation().getType(); - + if (operation==Types.LEFT_SQUARE_BRACKET && leftType.isArray()) { opt.chainShouldOptimize(true); resultType = leftType.getComponentType(); } else { switch (operation) { - case Types.COMPARE_EQUAL: + case Types.COMPARE_EQUAL: case Types.COMPARE_LESS_THAN: case Types.COMPARE_LESS_THAN_EQUAL: case Types.COMPARE_GREATER_THAN: @@ -683,7 +701,7 @@ public class OptimizingStatementWriter extends StatementWriter { } } } - + if (resultType!=null) { StatementMeta meta = addMeta(expression); meta.type = resultType; @@ -706,7 +724,7 @@ public class OptimizingStatementWriter extends StatementWriter { ClassNode originalResultType = typeChooser.resolveType(binExp, node); if ( !originalResultType.equals(BigDecimal_TYPE) || - !(isLongCategory(assignmentTartgetType) || isFloatingCategory(assignmentTartgetType)) + !(isLongCategory(assignmentTartgetType) || isFloatingCategory(assignmentTartgetType)) ) { return null; } @@ -740,7 +758,7 @@ public class OptimizingStatementWriter extends StatementWriter { if (opt.shouldOptimize()) addMeta(statement,opt); opt.pop(opt.shouldOptimize()); } - + @Override public void visitBlockStatement(BlockStatement block) { opt.push(); @@ -755,12 +773,12 @@ public class OptimizingStatementWriter extends StatementWriter { opt.chainCanOptimize(true); opt.pop(true); } else { - opt.chainShouldOptimize(optAll); + opt.chainShouldOptimize(optAll); if (optAll) addMeta(block,opt); opt.pop(optAll); } } - + @Override public void visitIfElse(IfStatement statement) { opt.push(); @@ -768,7 +786,7 @@ public class OptimizingStatementWriter extends StatementWriter { if (opt.shouldOptimize()) addMeta(statement,opt); opt.pop(opt.shouldOptimize()); } - + @Override public void visitStaticMethodCallExpression(StaticMethodCallExpression expression) { if (expression.getNodeMetaData(StatementMeta.class)!=null) return; @@ -776,23 +794,23 @@ public class OptimizingStatementWriter extends StatementWriter { setMethodTarget(expression,expression.getMethod(), expression.getArguments(), true); } - + @Override public void visitMethodCallExpression(MethodCallExpression expression) { if (expression.getNodeMetaData(StatementMeta.class)!=null) return; super.visitMethodCallExpression(expression); - + Expression object = expression.getObjectExpression(); boolean setTarget = AsmClassGenerator.isThisExpression(object); if (!setTarget) { if (!(object instanceof ClassExpression)) return; setTarget = object.equals(node); } - + if (!setTarget) return; setMethodTarget(expression, expression.getMethodAsString(), expression.getArguments(), true); } - + @Override public void visitConstructorCallExpression(ConstructorCallExpression call) { if (call.getNodeMetaData(StatementMeta.class)!=null) return; @@ -802,7 +820,7 @@ public class OptimizingStatementWriter extends StatementWriter { // check the meta class of the other class // setMethodTarget(call, "<init>", call.getArguments(), false); } - + private void setMethodTarget(Expression expression, String name, Expression callArgs, boolean isMethod) { if (name==null) return; if (!optimizeMethodCall) return; @@ -839,13 +857,13 @@ public class OptimizingStatementWriter extends StatementWriter { target = selectConstructor(type, paraTypes); if (target==null) return; } - + StatementMeta meta = addMeta(expression); meta.target = target; meta.type = type; opt.chainShouldOptimize(true); } - + private static MethodNode selectConstructor(ClassNode node, Parameter[] paraTypes) { List<ConstructorNode> cl = node.getDeclaredConstructors(); MethodNode res = null; @@ -870,7 +888,7 @@ public class OptimizingStatementWriter extends StatementWriter { public void visitClosureExpression(ClosureExpression expression) { return; } - + @Override public void visitForLoop(ForStatement statement) { opt.push(); @@ -879,6 +897,6 @@ public class OptimizingStatementWriter extends StatementWriter { opt.pop(opt.shouldOptimize()); } } - + } http://git-wip-us.apache.org/repos/asf/groovy/blob/96b0351d/src/test/groovy/bugs/Groovy7291Bug.groovy ---------------------------------------------------------------------- diff --git a/src/test/groovy/bugs/Groovy7291Bug.groovy b/src/test/groovy/bugs/Groovy7291Bug.groovy new file mode 100644 index 0000000..afac56d --- /dev/null +++ b/src/test/groovy/bugs/Groovy7291Bug.groovy @@ -0,0 +1,41 @@ +/* + * 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 Groovy7291Bug extends GroovyShellTestCase { + void testPrimitiveDouble() { + evaluate(''' +double a; +def b = { + a = a + 1; +} +b(); + '''); + } + + void testDouble() { + shouldFail(''' +Double a; +def b = { + a = a + 1; +} +b(); + '''); + } +}