From 3f2344c85ba255dcdc73e1e0a34c3d00985e5c64 Mon Sep 17 00:00:00 2001 From: Daniel Sun Date: Mon, 1 Jan 2024 02:43:29 +0800 Subject: [PATCH 1/6] GROOVY-11263: Analyze dead code --- .../groovy/classgen/DeadCodeAnalyzer.java | 68 +++++ .../codehaus/groovy/classgen/Verifier.java | 17 ++ .../groovy/control/CompilationUnit.java | 2 +- src/test/gls/statements/ReturnTest.groovy | 14 +- src/test/groovy/BreakContinueLabelTest.groovy | 150 ++++++----- src/test/groovy/bugs/Groovy11263.groovy | 239 ++++++++++++++++++ 6 files changed, 422 insertions(+), 68 deletions(-) create mode 100644 src/main/java/org/codehaus/groovy/classgen/DeadCodeAnalyzer.java create mode 100644 src/test/groovy/bugs/Groovy11263.groovy diff --git a/src/main/java/org/codehaus/groovy/classgen/DeadCodeAnalyzer.java b/src/main/java/org/codehaus/groovy/classgen/DeadCodeAnalyzer.java new file mode 100644 index 00000000000..fcc6016f6eb --- /dev/null +++ b/src/main/java/org/codehaus/groovy/classgen/DeadCodeAnalyzer.java @@ -0,0 +1,68 @@ +/* + * 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.classgen; + +import org.codehaus.groovy.ast.ClassCodeVisitorSupport; +import org.codehaus.groovy.ast.stmt.BlockStatement; +import org.codehaus.groovy.ast.stmt.BreakStatement; +import org.codehaus.groovy.ast.stmt.ContinueStatement; +import org.codehaus.groovy.ast.stmt.ReturnStatement; +import org.codehaus.groovy.ast.stmt.Statement; +import org.codehaus.groovy.control.SourceUnit; + +/** + * Analyze AST for dead code + * + * @since 5.0.0 + */ +public class DeadCodeAnalyzer extends ClassCodeVisitorSupport { + + private final SourceUnit sourceUnit; + + public DeadCodeAnalyzer(final SourceUnit sourceUnit) { + this.sourceUnit = sourceUnit; + } + + @Override + protected SourceUnit getSourceUnit() { + return sourceUnit; + } + @Override + public void visitBlockStatement(BlockStatement statement) { + analyzeDeadCode(statement); + super.visitBlockStatement(statement); + } + + private void analyzeDeadCode(BlockStatement block) { + int foundCnt = 0; + for (Statement statement : block.getStatements()) { + if (statement instanceof ReturnStatement + || statement instanceof BreakStatement + || statement instanceof ContinueStatement + ) { + foundCnt++; + if (1 == foundCnt) continue; + } + + if (foundCnt > 0) { + addError("Unreachable statement found", statement); + } + } + } +} diff --git a/src/main/java/org/codehaus/groovy/classgen/Verifier.java b/src/main/java/org/codehaus/groovy/classgen/Verifier.java index b93b9e24c0c..5298c39f89f 100644 --- a/src/main/java/org/codehaus/groovy/classgen/Verifier.java +++ b/src/main/java/org/codehaus/groovy/classgen/Verifier.java @@ -64,6 +64,7 @@ import org.codehaus.groovy.classgen.asm.MopWriter; import org.codehaus.groovy.classgen.asm.OptimizingStatementWriter.ClassNodeSkip; import org.codehaus.groovy.classgen.asm.WriterController; +import org.codehaus.groovy.control.SourceUnit; import org.codehaus.groovy.reflection.ClassInfo; import org.codehaus.groovy.syntax.RuntimeParserException; import org.codehaus.groovy.syntax.Token; @@ -167,6 +168,15 @@ public class Verifier implements GroovyClassVisitor, Opcodes { private ClassNode classNode; private MethodNode methodNode; + private SourceUnit source; + + public Verifier() { + } + + public Verifier(SourceUnit source) { + this.source = source; + } + public ClassNode getClassNode() { return classNode; } @@ -267,6 +277,7 @@ public void visitClass(final ClassNode node) { addCovariantMethods(node); detectNonSealedClasses(node); checkFinalVariables(node); + checkDeadCode(node); } private static final String[] INVALID_COMPONENTS = {"clone", "finalize", "getClass", "hashCode", "notify", "notifyAll", "toString", "wait"}; @@ -302,6 +313,12 @@ private void checkFinalVariables(final ClassNode node) { visitor.visitClass(node); } + private void checkDeadCode(final ClassNode node) { + if (null == source) return; + GroovyClassVisitor visitor = new DeadCodeAnalyzer(source); + visitor.visitClass(node); + } + protected FinalVariableAnalyzer.VariableNotFinalCallback getFinalVariablesCallback() { return new FinalVariableAnalyzer.VariableNotFinalCallback() { @Override diff --git a/src/main/java/org/codehaus/groovy/control/CompilationUnit.java b/src/main/java/org/codehaus/groovy/control/CompilationUnit.java index 071f6e3a0cb..1d3be4e3f7e 100644 --- a/src/main/java/org/codehaus/groovy/control/CompilationUnit.java +++ b/src/main/java/org/codehaus/groovy/control/CompilationUnit.java @@ -750,7 +750,7 @@ protected boolean dequeued() throws CompilationFailedException { public void call(final SourceUnit source, final GeneratorContext context, final ClassNode classNode) throws CompilationFailedException { new OptimizerVisitor(CompilationUnit.this).visitClass(classNode, source); // GROOVY-4272: repositioned from static import visitor - GroovyClassVisitor visitor = new Verifier(); + GroovyClassVisitor visitor = new Verifier(source); try { visitor.visitClass(classNode); } catch (RuntimeParserException rpe) { diff --git a/src/test/gls/statements/ReturnTest.groovy b/src/test/gls/statements/ReturnTest.groovy index 66effd7b765..5495a3d4908 100644 --- a/src/test/gls/statements/ReturnTest.groovy +++ b/src/test/gls/statements/ReturnTest.groovy @@ -26,18 +26,22 @@ class ReturnTest extends CompilableTestSupport { shouldNotCompile """ class A { {return} - } + } """ } void testStaticInitializer() { - assertScript """ + def err = shouldFail """\ class A { static foo=2 - static { return; foo=1 } + static { + return; + foo=1 + } } assert A.foo==2 - """ + """ + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 5, column 17\.\s+foo=1\s+(.+)/ } void testReturnAdditionInFinally() { @@ -59,4 +63,4 @@ class ReturnTest extends CompilableTestSupport { assert finalCountDown().counter == 9 """ } -} \ No newline at end of file +} diff --git a/src/test/groovy/BreakContinueLabelTest.groovy b/src/test/groovy/BreakContinueLabelTest.groovy index a993f203826..e76fa006eaa 100644 --- a/src/test/groovy/BreakContinueLabelTest.groovy +++ b/src/test/groovy/BreakContinueLabelTest.groovy @@ -31,70 +31,89 @@ class BreakContinueLabelTest extends CompilableTestSupport { assert true } void testBreakLabelInSimpleForLoop() { - label_1: for (i in [1]) { - break label_1 - assert false - } + def err = shouldFail '''\ + label_1: for (i in [1]) { + break label_1 + assert false + } + ''' + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 3, column 17\.\s+assert false\s+(.+)/ } void testBreakLabelInNestedForLoop() { - label: for (i in [1]) { - for (j in [1]){ - break label - assert false, 'did not break inner loop' + def err = shouldFail '''\ + label: for (i in [1]) { + for (j in [1]){ + break label + assert false, 'did not break inner loop' + } + assert false, 'did not break outer loop' } - assert false, 'did not break outer loop' - } + ''' + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 4, column 21\.\s+assert false, 'did not break inner loop'\s+(.+)/ } void testUnlabelledBreakInNestedForLoop() { - def reached = false - for (i in [1]) { - for (j in [1]){ - break - assert false, 'did not break inner loop' + def err = shouldFail '''\ + def reached = false + for (i in [1]) { + for (j in [1]){ + break + assert false, 'did not break inner loop' + } + reached = true } - reached = true - } - assert reached, 'must not break outer loop' + assert reached, 'must not break outer loop' + ''' + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 5, column 21\.\s+assert false, 'did not break inner loop'\s+(.+)/ } void testBreakLabelInSimpleWhileLoop() { - label_1: while (true) { - break label_1 - assert false - } + def err = shouldFail '''\ + label_1: while (true) { + break label_1 + assert false + } + ''' + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 3, column 17\.\s+assert false\s+(.+)/ } void testBreakLabelInNestedWhileLoop() { - def count = 0 - label: while (count < 1) { - count++ - while (true){ - break label - assert false, 'did not break inner loop' + def err = shouldFail '''\ + def count = 0 + label: while (count < 1) { + count++ + while (true){ + break label + assert false, 'did not break inner loop' + } + assert false, 'did not break outer loop' } - assert false, 'did not break outer loop' - } + ''' + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 6, column 21\.\s+assert false, 'did not break inner loop'\s+(.+)/ } void testBreakLabelInNestedMixedForAndWhileLoop() { - def count = 0 - label_1: while (count < 1) { - count++ - for (i in [1]){ - break label_1 - assert false, 'did not break inner loop' + def err = shouldFail '''\ + def count = 0 + label_1: while (count < 1) { + count++ + for (i in [1]){ + break label_1 + assert false, 'did not break inner loop' + } + assert false, 'did not break outer loop' } - assert false, 'did not break outer loop' - } - label_2: for (i in [1]) { - while (true){ - break label_2 - assert false, 'did not break inner loop' + label_2: for (i in [1]) { + while (true){ + break label_2 + assert false, 'did not break inner loop' + } + assert false, 'did not break outer loop' } - assert false, 'did not break outer loop' - } + ''' + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 6, column 21\.\s+assert false, 'did not break inner loop'\s+(.+)/ + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 13, column 21\.\s+assert false, 'did not break inner loop'\s+(.+)/ } void testUnlabelledContinueInNestedForLoop() { @@ -123,13 +142,16 @@ class BreakContinueLabelTest extends CompilableTestSupport { } void testBreakToLastLabelSucceeds() { - one: - two: - three: - for (i in 1..2) { - break three - fail() - } + def err = shouldFail '''\ + one: + two: + three: + for (i in 1..2) { + break three + assert false + } + ''' + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 6, column 17\.\s+assert false\s+(.+)/ } void testMultipleLabelSupport() { @@ -152,15 +174,19 @@ class BreakContinueLabelTest extends CompilableTestSupport { // this is in accordance with Java; Spock Framework relies on this void testLabelCanOccurMultipleTimesInSameScope() { - one: - for (i in 1..2) { - break one - fail() - } - one: - for (i in 1..2) { - break one - fail() - } + def err = shouldFail '''\ + one: + for (i in 1..2) { + break one + assert false + } + one: + for (i in 1..2) { + break one + assert false + } + ''' + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 4, column 17\.\s+assert false\s+(.+)/ + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 9, column 17\.\s+assert false\s+(.+)/ } -} \ No newline at end of file +} diff --git a/src/test/groovy/bugs/Groovy11263.groovy b/src/test/groovy/bugs/Groovy11263.groovy new file mode 100644 index 00000000000..fe2a5a9ec60 --- /dev/null +++ b/src/test/groovy/bugs/Groovy11263.groovy @@ -0,0 +1,239 @@ +/* + * 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 bugs + +import org.junit.Test + +import static groovy.test.GroovyAssert.assertScript +import static groovy.test.GroovyAssert.shouldFail + +final class Groovy11263 { + + @Test + void testUnreachableStatementAfterReturn1() { + def err = shouldFail '''\ + def m() { + return + def a = 1 + } + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 3, column 17\.\s+def a = 1\s+(.+)/ + } + + @Test + void testUnreachableStatementAfterReturn2() { + def err = shouldFail '''\ + class X { + X() { + return + def a = 1 + } + } + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 4, column 21\.\s+def a = 1\s+(.+)/ + } + + @Test + void testUnreachableStatementAfterReturn3() { + def err = shouldFail '''\ + { + return + def a = 1 + } + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 3, column 17\.\s+def a = 1\s+(.+)/ + } + + @Test + void testUnreachableStatementAfterReturn4() { + def err = shouldFail '''\ + for (var i in [1, 2, 3]) { + return + def a = 1 + } + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 3, column 17\.\s+def a = 1\s+(.+)/ + } + + @Test + void testUnreachableStatementAfterReturn5() { + def err = shouldFail '''\ + while (true) { + return + def a = 1 + } + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 3, column 17\.\s+def a = 1\s+(.+)/ + } + + @Test + void testUnreachableStatementAfterReturn6() { + def err = shouldFail '''\ + do { + return + def a = 1 + } while (true) + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 3, column 17\.\s+def a = 1\s+(.+)/ + } + + @Test + void testUnreachableStatementAfterReturn7() { + def err = shouldFail '''\ + if (true) { + return + def a = 1 + } + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 3, column 17\.\s+def a = 1\s+(.+)/ + } + + @Test + void testUnreachableStatementAfterReturn8() { + def err = shouldFail '''\ + if (true) { + } else { + return + def a = 1 + } + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 4, column 17\.\s+def a = 1\s+(.+)/ + } + + @Test + void testUnreachableStatementAfterReturn9() { + def err = shouldFail '''\ + try { + return + def a = 1 + } catch (e) { + } + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 3, column 17\.\s+def a = 1\s+(.+)/ + } + + @Test + void testUnreachableStatementAfterReturn10() { + def err = shouldFail '''\ + try { + } catch (e) { + return + def a = 1 + } + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 4, column 17\.\s+def a = 1\s+(.+)/ + } + + @Test + void testUnreachableStatementAfterReturn11() { + def err = shouldFail '''\ + try { + } finally { + return + def a = 1 + } + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 4, column 17\.\s+def a = 1\s+(.+)/ + } + + @Test + void testUnreachableStatementAfterReturn12() { + def err = shouldFail '''\ + switch(1) { + case 1: + return + def a = 1 + } + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 4, column 21\.\s+def a = 1\s+(.+)/ + } + + @Test + void testUnreachableStatementAfterReturn13() { + def err = shouldFail '''\ + switch(1) { + default: + return + def a = 1 + } + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 4, column 21\.\s+def a = 1\s+(.+)/ + } + + @Test + void testUnreachableStatementAfterReturn14() { + def err = shouldFail '''\ + [1, 2, 3].each { + return + def a = 1 + } + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 3, column 17\.\s+def a = 1\s+(.+)/ + } + + @Test + void testUnreachableStatementAfterReturn15() { + def err = shouldFail '''\ + [1, 2, 3].each(e -> { + return + def a = 1 + }) + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 3, column 17\.\s+def a = 1\s+(.+)/ + } + + @Test + void testUnreachableStatementAfterBreak() { + def err = shouldFail '''\ + for (var i in [1, 2, 3]) { + break + def a = 1 + } + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 3, column 17\.\s+def a = 1\s+(.+)/ + } + + @Test + void testUnreachableStatementAfterContinue() { + def err = shouldFail '''\ + for (var i in [1, 2, 3]) { + continue + def a = 1 + } + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 3, column 17\.\s+def a = 1\s+(.+)/ + } +} From c37ac8adc0177557a8de4fb6ea99ff90ddf650ac Mon Sep 17 00:00:00 2001 From: Daniel Sun Date: Mon, 1 Jan 2024 10:16:02 +0800 Subject: [PATCH 2/6] Set source unit via setter --- .../org/codehaus/groovy/classgen/Verifier.java | 17 +++++++---------- .../groovy/control/CompilationUnit.java | 5 ++++- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/main/java/org/codehaus/groovy/classgen/Verifier.java b/src/main/java/org/codehaus/groovy/classgen/Verifier.java index 5298c39f89f..7073f29b175 100644 --- a/src/main/java/org/codehaus/groovy/classgen/Verifier.java +++ b/src/main/java/org/codehaus/groovy/classgen/Verifier.java @@ -168,14 +168,7 @@ public class Verifier implements GroovyClassVisitor, Opcodes { private ClassNode classNode; private MethodNode methodNode; - private SourceUnit source; - - public Verifier() { - } - - public Verifier(SourceUnit source) { - this.source = source; - } + private SourceUnit sourceUnit; public ClassNode getClassNode() { return classNode; @@ -189,6 +182,10 @@ public MethodNode getMethodNode() { return methodNode; } + public void setSourceUnit(SourceUnit sourceUnit) { + this.sourceUnit = sourceUnit; + } + private static FieldNode getMetaClassField(final ClassNode node) { FieldNode metaClassField = node.getDeclaredField("metaClass"); if (metaClassField != null) { @@ -314,8 +311,8 @@ private void checkFinalVariables(final ClassNode node) { } private void checkDeadCode(final ClassNode node) { - if (null == source) return; - GroovyClassVisitor visitor = new DeadCodeAnalyzer(source); + if (null == sourceUnit) return; + GroovyClassVisitor visitor = new DeadCodeAnalyzer(sourceUnit); visitor.visitClass(node); } diff --git a/src/main/java/org/codehaus/groovy/control/CompilationUnit.java b/src/main/java/org/codehaus/groovy/control/CompilationUnit.java index 1d3be4e3f7e..1fa4bf9a76e 100644 --- a/src/main/java/org/codehaus/groovy/control/CompilationUnit.java +++ b/src/main/java/org/codehaus/groovy/control/CompilationUnit.java @@ -750,8 +750,11 @@ protected boolean dequeued() throws CompilationFailedException { public void call(final SourceUnit source, final GeneratorContext context, final ClassNode classNode) throws CompilationFailedException { new OptimizerVisitor(CompilationUnit.this).visitClass(classNode, source); // GROOVY-4272: repositioned from static import visitor - GroovyClassVisitor visitor = new Verifier(source); + GroovyClassVisitor visitor; try { + Verifier verifier = new Verifier(); + verifier.setSourceUnit(source); + visitor = verifier; visitor.visitClass(classNode); } catch (RuntimeParserException rpe) { getErrorCollector().addError(new SyntaxException(rpe.getMessage(), rpe.getNode()), source); From 23af892fb1e6b28fecd6ef82aa7cf45849f121ba Mon Sep 17 00:00:00 2001 From: Daniel Sun Date: Mon, 1 Jan 2024 12:37:18 +0800 Subject: [PATCH 3/6] Add more test cases --- src/test/groovy/bugs/Groovy11263.groovy | 120 +++++++++++++++++++++++- 1 file changed, 118 insertions(+), 2 deletions(-) diff --git a/src/test/groovy/bugs/Groovy11263.groovy b/src/test/groovy/bugs/Groovy11263.groovy index fe2a5a9ec60..7c58e8d3922 100644 --- a/src/test/groovy/bugs/Groovy11263.groovy +++ b/src/test/groovy/bugs/Groovy11263.groovy @@ -214,7 +214,51 @@ final class Groovy11263 { } @Test - void testUnreachableStatementAfterBreak() { + void testUnreachableStatementAfterReturn16() { + def err = shouldFail '''\ + return + def a = 1 + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 2, column 13\.\s+def a = 1\s+(.+)/ + } + + @Test + void testUnreachableStatementAfterReturn17() { + def err = shouldFail '''\ + return + return + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 2, column 13\.\s+return\s+(.+)/ + } + + @Test + void testUnreachableStatementAfterReturn19() { + def err = shouldFail '''\ + for (var i in [1, 2, 3]) { + return + break + } + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 3, column 17\.\s+break\s+(.+)/ + } + + @Test + void testUnreachableStatementAfterReturn20() { + def err = shouldFail '''\ + for (var i in [1, 2, 3]) { + return + continue + } + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 3, column 17\.\s+continue\s+(.+)/ + } + + @Test + void testUnreachableStatementAfterBreak1() { def err = shouldFail '''\ for (var i in [1, 2, 3]) { break @@ -226,7 +270,43 @@ final class Groovy11263 { } @Test - void testUnreachableStatementAfterContinue() { + void testUnreachableStatementAfterBreak2() { + def err = shouldFail '''\ + for (var i in [1, 2, 3]) { + break + break + } + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 3, column 17\.\s+break\s+(.+)/ + } + + @Test + void testUnreachableStatementAfterBreak3() { + def err = shouldFail '''\ + for (var i in [1, 2, 3]) { + break + continue + } + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 3, column 17\.\s+continue\s+(.+)/ + } + + @Test + void testUnreachableStatementAfterBreak4() { + def err = shouldFail '''\ + for (var i in [1, 2, 3]) { + break + return + } + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 3, column 17\.\s+return\s+(.+)/ + } + + @Test + void testUnreachableStatementAfterContinue1() { def err = shouldFail '''\ for (var i in [1, 2, 3]) { continue @@ -236,4 +316,40 @@ final class Groovy11263 { assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 3, column 17\.\s+def a = 1\s+(.+)/ } + + @Test + void testUnreachableStatementAfterContinue2() { + def err = shouldFail '''\ + for (var i in [1, 2, 3]) { + continue + continue + } + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 3, column 17\.\s+continue\s+(.+)/ + } + + @Test + void testUnreachableStatementAfterContinue3() { + def err = shouldFail '''\ + for (var i in [1, 2, 3]) { + continue + break + } + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 3, column 17\.\s+break\s+(.+)/ + } + + @Test + void testUnreachableStatementAfterContinue4() { + def err = shouldFail '''\ + for (var i in [1, 2, 3]) { + continue + return + } + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 3, column 17\.\s+return\s+(.+)/ + } } From 0f313a813e69d76542232310b8cbc8c93e69e53a Mon Sep 17 00:00:00 2001 From: Daniel Sun Date: Mon, 1 Jan 2024 18:48:15 +0800 Subject: [PATCH 4/6] Support analyzing dead code after `throw` statement --- .../groovy/classgen/DeadCodeAnalyzer.java | 3 +- src/test/groovy/ThrowTest.groovy | 17 ++++--- src/test/groovy/bugs/Groovy11263.groovy | 49 ++++++++++++++++++- .../post/SimplePostconditionTests.groovy | 4 +- 4 files changed, 62 insertions(+), 11 deletions(-) diff --git a/src/main/java/org/codehaus/groovy/classgen/DeadCodeAnalyzer.java b/src/main/java/org/codehaus/groovy/classgen/DeadCodeAnalyzer.java index fcc6016f6eb..c856b618614 100644 --- a/src/main/java/org/codehaus/groovy/classgen/DeadCodeAnalyzer.java +++ b/src/main/java/org/codehaus/groovy/classgen/DeadCodeAnalyzer.java @@ -24,6 +24,7 @@ import org.codehaus.groovy.ast.stmt.ContinueStatement; import org.codehaus.groovy.ast.stmt.ReturnStatement; import org.codehaus.groovy.ast.stmt.Statement; +import org.codehaus.groovy.ast.stmt.ThrowStatement; import org.codehaus.groovy.control.SourceUnit; /** @@ -55,7 +56,7 @@ private void analyzeDeadCode(BlockStatement block) { if (statement instanceof ReturnStatement || statement instanceof BreakStatement || statement instanceof ContinueStatement - ) { + || statement instanceof ThrowStatement) { foundCnt++; if (1 == foundCnt) continue; } diff --git a/src/test/groovy/ThrowTest.groovy b/src/test/groovy/ThrowTest.groovy index e0d6ae0b7a0..093c9375a3a 100644 --- a/src/test/groovy/ThrowTest.groovy +++ b/src/test/groovy/ThrowTest.groovy @@ -22,12 +22,15 @@ import groovy.test.GroovyTestCase class ThrowTest extends GroovyTestCase { void testThrow() { - try { - throw new Exception("abcd") - fail("Should have thrown an exception by now") - } - catch (Exception e) { - assert e.message == "abcd" - } + def err = shouldFail '''\ + try { + throw new Exception("abcd") + assert false: "Should have thrown an exception by now" + } + catch (Exception e) { + assert e.message == "abcd" + } + ''' + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 3, column 17\.\s+assert false: "Should have thrown an exception by now"\s+(.+)/ } } diff --git a/src/test/groovy/bugs/Groovy11263.groovy b/src/test/groovy/bugs/Groovy11263.groovy index 7c58e8d3922..0eb10a6b7ab 100644 --- a/src/test/groovy/bugs/Groovy11263.groovy +++ b/src/test/groovy/bugs/Groovy11263.groovy @@ -20,7 +20,6 @@ package bugs import org.junit.Test -import static groovy.test.GroovyAssert.assertScript import static groovy.test.GroovyAssert.shouldFail final class Groovy11263 { @@ -352,4 +351,52 @@ final class Groovy11263 { assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 3, column 17\.\s+return\s+(.+)/ } + + @Test + void testUnreachableStatementAfterThrow1() { + def err = shouldFail '''\ + for (var i in [1, 2, 3]) { + throw new RuntimeException("just for test") + throw new RuntimeException("just for test.") + } + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 3, column 17\.\s+throw new RuntimeException\("just for test\."\)\s+(.+)/ + } + + @Test + void testUnreachableStatementAfterThrow2() { + def err = shouldFail '''\ + for (var i in [1, 2, 3]) { + throw new RuntimeException("just for test") + return + } + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 3, column 17\.\s+return\s+(.+)/ + } + + @Test + void testUnreachableStatementAfterThrow3() { + def err = shouldFail '''\ + for (var i in [1, 2, 3]) { + throw new RuntimeException("just for test") + break + } + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 3, column 17\.\s+break\s+(.+)/ + } + + @Test + void testUnreachableStatementAfterThrow4() { + def err = shouldFail '''\ + for (var i in [1, 2, 3]) { + throw new RuntimeException("just for test") + continue + } + ''' + + assert err ==~ /(?s)(.+)\s+Unreachable statement found\s+@ line 3, column 17\.\s+continue\s+(.+)/ + } } diff --git a/subprojects/groovy-contracts/src/test/groovy/org/apache/groovy/contracts/tests/post/SimplePostconditionTests.groovy b/subprojects/groovy-contracts/src/test/groovy/org/apache/groovy/contracts/tests/post/SimplePostconditionTests.groovy index 0da74a6a691..85fc678bcbc 100644 --- a/subprojects/groovy-contracts/src/test/groovy/org/apache/groovy/contracts/tests/post/SimplePostconditionTests.groovy +++ b/subprojects/groovy-contracts/src/test/groovy/org/apache/groovy/contracts/tests/post/SimplePostconditionTests.groovy @@ -120,7 +120,7 @@ class Account { if (true) { try { throw new Exception ('test') - return 1 + // return 1 // GROOVY-11263 } finally { return 3 } @@ -148,7 +148,7 @@ class Account { if (true) { try { throw new Exception ('test') - return 1 + // return 1 // GROOVY-11263 } finally { return 3 } From 103cfa04a9dd8da2c4f0e64ee64f1e78185eca67 Mon Sep 17 00:00:00 2001 From: Daniel Sun Date: Tue, 2 Jan 2024 20:12:58 +0800 Subject: [PATCH 5/6] Keep `Verifier` as it was --- .../org/codehaus/groovy/classgen/Verifier.java | 14 -------------- .../codehaus/groovy/control/CompilationUnit.java | 8 +++++--- 2 files changed, 5 insertions(+), 17 deletions(-) diff --git a/src/main/java/org/codehaus/groovy/classgen/Verifier.java b/src/main/java/org/codehaus/groovy/classgen/Verifier.java index 7073f29b175..b93b9e24c0c 100644 --- a/src/main/java/org/codehaus/groovy/classgen/Verifier.java +++ b/src/main/java/org/codehaus/groovy/classgen/Verifier.java @@ -64,7 +64,6 @@ import org.codehaus.groovy.classgen.asm.MopWriter; import org.codehaus.groovy.classgen.asm.OptimizingStatementWriter.ClassNodeSkip; import org.codehaus.groovy.classgen.asm.WriterController; -import org.codehaus.groovy.control.SourceUnit; import org.codehaus.groovy.reflection.ClassInfo; import org.codehaus.groovy.syntax.RuntimeParserException; import org.codehaus.groovy.syntax.Token; @@ -168,8 +167,6 @@ public class Verifier implements GroovyClassVisitor, Opcodes { private ClassNode classNode; private MethodNode methodNode; - private SourceUnit sourceUnit; - public ClassNode getClassNode() { return classNode; } @@ -182,10 +179,6 @@ public MethodNode getMethodNode() { return methodNode; } - public void setSourceUnit(SourceUnit sourceUnit) { - this.sourceUnit = sourceUnit; - } - private static FieldNode getMetaClassField(final ClassNode node) { FieldNode metaClassField = node.getDeclaredField("metaClass"); if (metaClassField != null) { @@ -274,7 +267,6 @@ public void visitClass(final ClassNode node) { addCovariantMethods(node); detectNonSealedClasses(node); checkFinalVariables(node); - checkDeadCode(node); } private static final String[] INVALID_COMPONENTS = {"clone", "finalize", "getClass", "hashCode", "notify", "notifyAll", "toString", "wait"}; @@ -310,12 +302,6 @@ private void checkFinalVariables(final ClassNode node) { visitor.visitClass(node); } - private void checkDeadCode(final ClassNode node) { - if (null == sourceUnit) return; - GroovyClassVisitor visitor = new DeadCodeAnalyzer(sourceUnit); - visitor.visitClass(node); - } - protected FinalVariableAnalyzer.VariableNotFinalCallback getFinalVariablesCallback() { return new FinalVariableAnalyzer.VariableNotFinalCallback() { @Override diff --git a/src/main/java/org/codehaus/groovy/control/CompilationUnit.java b/src/main/java/org/codehaus/groovy/control/CompilationUnit.java index 1fa4bf9a76e..bf5783a5056 100644 --- a/src/main/java/org/codehaus/groovy/control/CompilationUnit.java +++ b/src/main/java/org/codehaus/groovy/control/CompilationUnit.java @@ -34,6 +34,7 @@ import org.codehaus.groovy.ast.expr.VariableExpression; import org.codehaus.groovy.classgen.AsmClassGenerator; import org.codehaus.groovy.classgen.ClassCompletionVerifier; +import org.codehaus.groovy.classgen.DeadCodeAnalyzer; import org.codehaus.groovy.classgen.EnumCompletionVisitor; import org.codehaus.groovy.classgen.EnumVisitor; import org.codehaus.groovy.classgen.ExtendedVerifier; @@ -752,14 +753,15 @@ public void call(final SourceUnit source, final GeneratorContext context, final GroovyClassVisitor visitor; try { - Verifier verifier = new Verifier(); - verifier.setSourceUnit(source); - visitor = verifier; + visitor = new Verifier(); visitor.visitClass(classNode); } catch (RuntimeParserException rpe) { getErrorCollector().addError(new SyntaxException(rpe.getMessage(), rpe.getNode()), source); } + visitor = new DeadCodeAnalyzer(source); + visitor.visitClass(classNode); + visitor = new LabelVerifier(source); visitor.visitClass(classNode); From 66bc06f380a0928c8f6e544c5df573570d2d8a29 Mon Sep 17 00:00:00 2001 From: Daniel Sun Date: Thu, 4 Jan 2024 23:52:52 +0800 Subject: [PATCH 6/6] Add an option to switch off dead code analysis --- .../java/org/codehaus/groovy/control/CompilationUnit.java | 6 ++++-- .../org/codehaus/groovy/control/CompilerConfiguration.java | 5 +++++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/codehaus/groovy/control/CompilationUnit.java b/src/main/java/org/codehaus/groovy/control/CompilationUnit.java index bf5783a5056..598a741be5b 100644 --- a/src/main/java/org/codehaus/groovy/control/CompilationUnit.java +++ b/src/main/java/org/codehaus/groovy/control/CompilationUnit.java @@ -759,8 +759,10 @@ public void call(final SourceUnit source, final GeneratorContext context, final getErrorCollector().addError(new SyntaxException(rpe.getMessage(), rpe.getNode()), source); } - visitor = new DeadCodeAnalyzer(source); - visitor.visitClass(classNode); + if (Boolean.TRUE.equals(configuration.getOptimizationOptions().get(CompilerConfiguration.ANALYZE_DEAD_CODE))) { + visitor = new DeadCodeAnalyzer(source); + visitor.visitClass(classNode); + } visitor = new LabelVerifier(source); visitor.visitClass(classNode); diff --git a/src/main/java/org/codehaus/groovy/control/CompilerConfiguration.java b/src/main/java/org/codehaus/groovy/control/CompilerConfiguration.java index 6d9522acd54..8b662fff30d 100644 --- a/src/main/java/org/codehaus/groovy/control/CompilerConfiguration.java +++ b/src/main/java/org/codehaus/groovy/control/CompilerConfiguration.java @@ -66,6 +66,9 @@ public class CompilerConfiguration { /** Joint Compilation Option for enabling generating stubs in memory. */ public static final String MEM_STUB = "memStub"; + /** Optimization Option for enabling dead code analysis */ + public static final String ANALYZE_DEAD_CODE = "analyzeDeadCode"; + /** This ("1.4") is the value for targetBytecode to compile for a JDK 1.4. */ @Deprecated public static final String JDK4 = "1.4"; /** This ("1.5") is the value for targetBytecode to compile for a JDK 1.5. */ @@ -469,6 +472,8 @@ public CompilerConfiguration() { handleOptimizationOption(GROOVYDOC, getSystemPropertySafe("groovy.attach.groovydoc")); handleOptimizationOption(RUNTIME_GROOVYDOC, getSystemPropertySafe("groovy.attach.runtime.groovydoc")); handleOptimizationOption(PARALLEL_PARSE, getSystemPropertySafe("groovy.parallel.parse", "true")); + handleOptimizationOption(ANALYZE_DEAD_CODE, getSystemPropertySafe("groovy.analyze.deadcode", "true")); + if (getBooleanSafe("groovy.mem.stub")) { jointCompilationOptions = new HashMap<>(2);