From 6b18c46bc1945b32235c6a14d99013056469f4c5 Mon Sep 17 00:00:00 2001 From: Daniel Sun Date: Mon, 1 Jan 2024 02:43:29 +0800 Subject: [PATCH] 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 | 215 ++++++++++++++++++ 6 files changed, 398 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 000000000000..fcc6016f6ebc --- /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 b93b9e24c0c2..5298c39f89f5 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 071f6e3a0cb0..1d3be4e3f7e7 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 66effd7b765e..5495a3d49087 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 a993f2038265..e76fa006eaa1 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 000000000000..67dd8f2182b2 --- /dev/null +++ b/src/test/groovy/bugs/Groovy11263.groovy @@ -0,0 +1,215 @@ +/* + * 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+(.+)/ + } +}