This is an automated email from the ASF dual-hosted git repository.
emilles pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/groovy.git
The following commit(s) were added to refs/heads/master by this push:
new 11d8e65 GROOVY-10314: classgen: no extra finally if try or catch(es)
can't reach
11d8e65 is described below
commit 11d8e65751ae4b6556dd0da13e5997e5ff36e4d7
Author: Eric Milles <[email protected]>
AuthorDate: Sat Oct 16 15:33:22 2021 -0500
GROOVY-10314: classgen: no extra finally if try or catch(es) can't reach
---
.../codehaus/groovy/ast/tools/GeneralUtils.java | 33 ++++++++++++++++
.../groovy/classgen/AsmClassGenerator.java | 46 +++++++---------------
.../groovy/classgen/asm/StatementWriter.java | 36 ++++++++++-------
src/test/groovy/bugs/Groovy10314.groovy | 38 ++++++++++++++++++
4 files changed, 107 insertions(+), 46 deletions(-)
diff --git a/src/main/java/org/codehaus/groovy/ast/tools/GeneralUtils.java
b/src/main/java/org/codehaus/groovy/ast/tools/GeneralUtils.java
index 4b67b70..dd0b883 100644
--- a/src/main/java/org/codehaus/groovy/ast/tools/GeneralUtils.java
+++ b/src/main/java/org/codehaus/groovy/ast/tools/GeneralUtils.java
@@ -62,6 +62,8 @@ import org.codehaus.groovy.ast.stmt.ForStatement;
import org.codehaus.groovy.ast.stmt.IfStatement;
import org.codehaus.groovy.ast.stmt.ReturnStatement;
import org.codehaus.groovy.ast.stmt.Statement;
+import org.codehaus.groovy.ast.stmt.SwitchStatement;
+import org.codehaus.groovy.ast.stmt.SynchronizedStatement;
import org.codehaus.groovy.ast.stmt.ThrowStatement;
import org.codehaus.groovy.ast.stmt.TryCatchStatement;
import org.codehaus.groovy.classgen.BytecodeExpression;
@@ -1071,4 +1073,35 @@ public class GeneralUtils {
}
return sb.toString();
}
+
+ public static boolean maybeFallsThrough(final Statement statement) {
+ if (statement.isEmpty()) return true;
+
+ if (statement instanceof ReturnStatement) {
+ return false;
+ } else if (statement instanceof ThrowStatement) {
+ return false;
+ } else if (statement instanceof BlockStatement) {
+ List<Statement> list = ((BlockStatement)
statement).getStatements();
+ final int last = list.size() - 1;
+ if (!maybeFallsThrough(list.get(last))) return false;
+ for (int i = 0; i < last; i += 1)
+ if (!maybeFallsThrough(list.get(i))) return false;
+ } else if (statement instanceof IfStatement) {
+ return maybeFallsThrough(((IfStatement) statement).getElseBlock())
+ || maybeFallsThrough(((IfStatement) statement).getIfBlock());
+ } else if (statement instanceof SwitchStatement) {
+ // TODO
+ } else if (statement instanceof TryCatchStatement) {
+ TryCatchStatement tryCatch = (TryCatchStatement) statement;
+ if (!maybeFallsThrough(tryCatch.getFinallyStatement())) return
false;
+ for (CatchStatement cs : tryCatch.getCatchStatements())
+ if (maybeFallsThrough(cs.getCode())) return true;
+ return maybeFallsThrough(tryCatch.getTryStatement());
+ } else if (statement instanceof SynchronizedStatement) {
+ return maybeFallsThrough(((SynchronizedStatement)
statement).getCode());
+ }
+
+ return true;
+ }
}
diff --git a/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java
b/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java
index 1ba6a2a..616fcb0 100644
--- a/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java
+++ b/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java
@@ -152,6 +152,7 @@ import static
org.codehaus.groovy.ast.tools.GeneralUtils.classX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.fieldX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.getGetterName;
import static org.codehaus.groovy.ast.tools.GeneralUtils.getSetterName;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.maybeFallsThrough;
import static org.codehaus.groovy.ast.tools.GeneralUtils.propX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.thisPropX;
import static
org.codehaus.groovy.transform.sc.StaticCompilationMetadataKeys.PROPERTY_OWNER;
@@ -579,7 +580,7 @@ public class AsmClassGenerator extends ClassGenerator {
MethodVisitor mv = controller.getMethodVisitor();
if (isConstructor && (code == null || !((ConstructorNode)
node).firstStatementIsSpecialConstructorCall())) {
boolean hasCallToSuper = false;
- if (code != null && isInnerClass()) {
+ if (code != null && controller.getClassNode().getOuterClass() !=
null) {
// GROOVY-4471: if the class is an inner class node, there are
chances that
// the call to super is already added so we must ensure not to
add it twice
if (code instanceof BlockStatement) {
@@ -602,7 +603,7 @@ public class AsmClassGenerator extends ClassGenerator {
code.visit(this);
}
- if (!isLastStatementReturnOrThrow(code)) {
+ if (code == null || maybeFallsThrough(code)) {
if (code != null) { // GROOVY-7647, GROOVY-9373
controller.visitLineNumber(code.getLastLineNumber());
}
@@ -627,21 +628,6 @@ public class AsmClassGenerator extends ClassGenerator {
controller.getCompileStack().clear();
}
- private boolean isLastStatementReturnOrThrow(final Statement code) {
- if (code instanceof BlockStatement) {
- BlockStatement blockStatement = (BlockStatement) code;
- List<Statement> statementList = blockStatement.getStatements();
- int nStatements = statementList.size();
- if (nStatements > 0) {
- Statement lastStatement = statementList.get(nStatements - 1);
- if (lastStatement instanceof ReturnStatement || lastStatement
instanceof ThrowStatement) {
- return true;
- }
- }
- }
- return false;
- }
-
private void visitAnnotationDefaultExpression(final AnnotationVisitor av,
final ClassNode type, final Expression exp) {
if (exp instanceof ClosureExpression) {
ClassNode closureClass =
controller.getClosureWriter().getOrAddClosureClass((ClosureExpression) exp,
ACC_PUBLIC);
@@ -880,10 +866,11 @@ public class AsmClassGenerator extends ClassGenerator {
* Loads either this object or if we're inside a closure then load the top
level owner
*/
protected void loadThisOrOwner() {
- if (isInnerClass()) {
-
fieldX(controller.getClassNode().getDeclaredField("owner")).visit(this);
- } else {
+ ClassNode classNode = controller.getClassNode();
+ if (classNode.getOuterClass() == null) {
loadThis(VariableExpression.THIS_EXPRESSION);
+ } else {
+ fieldX(classNode.getDeclaredField("owner")).visit(this);
}
}
@@ -1348,7 +1335,6 @@ public class AsmClassGenerator extends ClassGenerator {
}
}
-
/**
* RHS instance field. should move most of the code in the BytecodeHelper
*/
@@ -2339,6 +2325,13 @@ public class AsmClassGenerator extends ClassGenerator {
// Implementation methods
//--------------------------------------------------------------------------
+ public boolean addInnerClass(final ClassNode innerClass) {
+ ModuleNode mn = controller.getClassNode().getModule();
+ innerClass.setModule(mn);
+ mn.getUnit().addGeneratedInnerClass((InnerClassNode) innerClass);
+ return innerClasses.add(innerClass);
+ }
+
public static int argumentSize(final Expression arguments) {
if (arguments instanceof TupleExpression) {
TupleExpression tupleExpression = (TupleExpression) arguments;
@@ -2378,10 +2371,6 @@ public class AsmClassGenerator extends ClassGenerator {
return false;
}
- private boolean isInnerClass() {
- return controller.getClassNode().getOuterClass() != null;
- }
-
@Deprecated
public static boolean isThisExpression(final Expression expression) {
return ExpressionUtils.isThisExpression(expression);
@@ -2401,13 +2390,6 @@ public class AsmClassGenerator extends ClassGenerator {
return (parameters.length > 0 && parameters[parameters.length -
1].getType().isArray());
}
- public boolean addInnerClass(final ClassNode innerClass) {
- ModuleNode mn = controller.getClassNode().getModule();
- innerClass.setModule(mn);
- mn.getUnit().addGeneratedInnerClass((InnerClassNode)innerClass);
- return innerClasses.add(innerClass);
- }
-
public void onLineNumber(final ASTNode statement, final String message) {
if (statement == null || statement instanceof BlockStatement) return;
diff --git
a/src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java
b/src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java
index 7e0222e..671e928 100644
--- a/src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java
+++ b/src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java
@@ -54,6 +54,7 @@ import java.util.Optional;
import java.util.function.Consumer;
import static org.codehaus.groovy.ast.ClassHelper.isPrimitiveVoid;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.maybeFallsThrough;
import static org.objectweb.asm.Opcodes.ALOAD;
import static org.objectweb.asm.Opcodes.ATHROW;
import static org.objectweb.asm.Opcodes.CHECKCAST;
@@ -332,8 +333,11 @@ public class StatementWriter {
// skip past catch block(s)
Label finallyStart = new Label();
- mv.visitJumpInsn(GOTO, finallyStart);
-
+ boolean fallthroughFinally = false;
+ if (maybeFallsThrough(tryStatement)) {
+ mv.visitJumpInsn(GOTO, finallyStart);
+ fallthroughFinally = true;
+ }
closeRange(tryBlock, mv);
// pop for BlockRecorder
compileStack.pop();
@@ -355,13 +359,16 @@ public class StatementWriter {
// end of catch
closeRange(catches, mv);
- mv.visitJumpInsn(GOTO, finallyStart);
+ if (maybeFallsThrough(catchStatement.getCode())) {
+ mv.visitJumpInsn(GOTO, finallyStart);
+ fallthroughFinally = true;
+ }
compileStack.writeExceptionTable(tryBlock, catchBlock,
BytecodeHelper.getClassInternalName(catchStatement.getExceptionType()));
}
// used to handle exceptions in catches and regularly visited finals
- Label catchAll = new Label();
+ Label catchAll = new Label(), afterCatchAll = new Label();
// add "catch all" block to exception table for try part; we do this
// after the exception blocks so they are not superseded by this one
@@ -372,26 +379,27 @@ public class StatementWriter {
// pop for BlockRecorder
compileStack.pop();
- // start finally
- mv.visitLabel(finallyStart);
- finallyStatement.visit(controller.getAcg());
+ if (fallthroughFinally) {
+ mv.visitLabel(finallyStart);
+ finallyStatement.visit(controller.getAcg());
- // skip past all-catching block
- Label afterCatchAll = new Label();
- mv.visitJumpInsn(GOTO, afterCatchAll);
+ // skip over the catch-finally-rethrow
+ mv.visitJumpInsn(GOTO, afterCatchAll);
+ }
mv.visitLabel(catchAll);
operandStack.push(ClassHelper.make(Throwable.class));
- int anyThrowableIndex =
compileStack.defineTemporaryVariable("throwable", true);
+ int anyThrowable = compileStack.defineTemporaryVariable("throwable",
true);
finallyStatement.visit(controller.getAcg());
// load the throwable and rethrow it
- mv.visitVarInsn(ALOAD, anyThrowableIndex);
+ mv.visitVarInsn(ALOAD, anyThrowable);
mv.visitInsn(ATHROW);
- mv.visitLabel(afterCatchAll);
- compileStack.removeVar(anyThrowableIndex);
+ if (fallthroughFinally)
+ mv.visitLabel(afterCatchAll);
+ compileStack.removeVar(anyThrowable);
}
private BlockRecorder makeBlockRecorder(final Statement finallyStatement) {
diff --git a/src/test/groovy/bugs/Groovy10314.groovy
b/src/test/groovy/bugs/Groovy10314.groovy
new file mode 100644
index 0000000..537a58e
--- /dev/null
+++ b/src/test/groovy/bugs/Groovy10314.groovy
@@ -0,0 +1,38 @@
+/*
+ * 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
+
+import org.codehaus.groovy.classgen.asm.AbstractBytecodeTestCase
+
+final class Groovy10314 extends AbstractBytecodeTestCase {
+
+ void testTryFinallyWhereAllPathsReturn() {
+ def result = compile method:'test', '''
+ def test() {
+ try {
+ print 'hello'
+ } finally {
+ print ' world'
+ }
+ }
+ '''
+
+ assert !result.hasStrictSequence(['NOP', 'NOP'])
+ }
+}
