This is an automated email from the ASF dual-hosted git repository. sunlan pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/groovy.git
commit c6b0442c5b08991422ac6f9650a747f2f624abf4 Author: Eric Milles <[email protected]> AuthorDate: Thu Jan 14 12:58:08 2021 -0600 GROOVY-9896, GROOVY-4727: add return for last case if no default present --- .../org/codehaus/groovy/classgen/ReturnAdder.java | 43 ++++----- src/test/groovy/SwitchTest.groovy | 106 +++++++++++++++++++-- src/test/groovy/bugs/Groovy3789Bug.groovy | 39 -------- 3 files changed, 117 insertions(+), 71 deletions(-) diff --git a/src/main/java/org/codehaus/groovy/classgen/ReturnAdder.java b/src/main/java/org/codehaus/groovy/classgen/ReturnAdder.java index 983339f..ad952e7 100644 --- a/src/main/java/org/codehaus/groovy/classgen/ReturnAdder.java +++ b/src/main/java/org/codehaus/groovy/classgen/ReturnAdder.java @@ -36,6 +36,7 @@ import org.codehaus.groovy.ast.stmt.ThrowStatement; import org.codehaus.groovy.ast.stmt.TryCatchStatement; import java.util.ArrayList; +import java.util.Iterator; import java.util.List; import java.util.Objects; @@ -139,11 +140,16 @@ public class ReturnAdder { if (statement instanceof SwitchStatement) { SwitchStatement switchStatement = (SwitchStatement) statement; - for (CaseStatement caseStatement : switchStatement.getCaseStatements()) { - Statement code = adjustSwitchCaseCode(caseStatement.getCode(), scope, false); + Statement defaultStatement = switchStatement.getDefaultStatement(); + List<CaseStatement> caseStatements = switchStatement.getCaseStatements(); + for (Iterator<CaseStatement> it = caseStatements.iterator(); it.hasNext(); ) { + CaseStatement caseStatement = it.next(); + Statement code = adjustSwitchCaseCode(caseStatement.getCode(), scope, + // GROOVY-9896: return if no default and last case lacks break + defaultStatement == EmptyStatement.INSTANCE && !it.hasNext()); if (doAdd) caseStatement.setCode(code); } - Statement defaultStatement = adjustSwitchCaseCode(switchStatement.getDefaultStatement(), scope, true); + defaultStatement = adjustSwitchCaseCode(defaultStatement, scope, true); if (doAdd) switchStatement.setDefaultStatement(defaultStatement); return switchStatement; } @@ -203,26 +209,19 @@ public class ReturnAdder { return blockStatement; } - private Statement adjustSwitchCaseCode(final Statement statement, final VariableScope scope, final boolean defaultCase) { - if (statement instanceof BlockStatement) { - List<Statement> statements = ((BlockStatement) statement).getStatements(); - if (!statements.isEmpty()) { - int lastIndex = statements.size() - 1; - Statement last = statements.get(lastIndex); - if (last instanceof BreakStatement) { - if (doAdd) { - statements.remove(lastIndex); - return addReturnsIfNeeded(statement, scope); - } else { - BlockStatement newBlock = new BlockStatement(); - for (int i = 0; i < lastIndex; i += 1) { - newBlock.addStatement(statements.get(i)); - } - return addReturnsIfNeeded(newBlock, scope); - } - } else if (defaultCase) { - return addReturnsIfNeeded(statement, scope); + private Statement adjustSwitchCaseCode(final Statement statement, final VariableScope scope, final boolean lastCase) { + if (!statement.isEmpty() && statement instanceof BlockStatement) { + BlockStatement block = (BlockStatement) statement; + int breakIndex = block.getStatements().size() - 1; + if (block.getStatements().get(breakIndex) instanceof BreakStatement) { + if (doAdd) { + block.getStatements().remove(breakIndex); + return addReturnsIfNeeded(block, scope); + } else { + addReturnsIfNeeded(new BlockStatement(block.getStatements().subList(0, breakIndex), null), scope); } + } else if (lastCase) { + return addReturnsIfNeeded(statement, scope); } } return statement; diff --git a/src/test/groovy/SwitchTest.groovy b/src/test/groovy/SwitchTest.groovy index 89cc170..36c9cb0 100644 --- a/src/test/groovy/SwitchTest.groovy +++ b/src/test/groovy/SwitchTest.groovy @@ -19,6 +19,7 @@ package groovy import groovy.test.GroovyTestCase +import groovy.test.NotYetImplemented class SwitchTest extends GroovyTestCase { @@ -215,16 +216,101 @@ class SwitchTest extends GroovyTestCase { } assertEquals 1, i } - - void testSwitchReturnFromDefaultCase() { - assert m3('a') == 'letter A' && m3('b') == 'letter B' && m3('z') == 'Unknown letter' + + void testSwitchReturn1() { + assertScript ''' + def test(x) { + switch (x) { + case 'a': 'letter A'; break + case 'b': 'letter B'; break + default : 'Unknown letter' + } + } + assert test('a') == 'letter A' + assert test('b') == 'letter B' + assert test('z') == 'Unknown letter' + ''' + } + + // GROOVY-3789 + void testSwitchReturn2() { + assertScript ''' + def test = { -> + if ( 0 ) { 10 } + else { 20 } + } + assert test() == 20 + ''' + assertScript ''' + def test = { -> + switch ( 0 ) { + case 0 : 10 ; break + default : 20 ; break + } + } + assert test() == 10 + ''' + } + + // GROOVY-4727 + void testSwitchReturn3() { + assertScript ''' + def test(x,y) { + switch (x) { + case 'x1': + switch (y) { + case 'y1': + 'r1' + break + case 'y2': + 'r2' + break + } + // no break + } + } + assert test('x1','y1') == 'r1' + ''' + } + + @NotYetImplemented // GROOVY-9880 + void testSwitchReturn4() { + assertScript ''' + def test(sb) { + switch ('value') { + case 'value': + sb.append('foo') + if (false) sb.append('X') + // implicit "else ;" + break + default: + sb.append('bar') + } + } + def sb = new StringBuilder() + test(sb); assert sb.toString() == 'foo' + ''' } - def m3(s) { - switch(s) { - case 'a': 'letter A'; break - case 'b': 'letter B'; break - default: 'Unknown letter' - } + // GROOVY-9896 + void testSwitchReturn5() { + assertScript ''' + def test(x) { + switch(x) { + case 1: + 'a' + break + case 2: + 'b' + break + case 3: + 'c' + } + } + assert test(1) == 'a' + assert test(2) == 'b' + assert test(3) == 'c' + assert test(4) == null + ''' } -} \ No newline at end of file +} diff --git a/src/test/groovy/bugs/Groovy3789Bug.groovy b/src/test/groovy/bugs/Groovy3789Bug.groovy deleted file mode 100644 index 405e9de..0000000 --- a/src/test/groovy/bugs/Groovy3789Bug.groovy +++ /dev/null @@ -1,39 +0,0 @@ -/* - * 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 groovy.test.GroovyTestCase - -class Groovy3789Bug extends GroovyTestCase { - void testAddReturnWhenLastStatementIsSwitch() { - def ifClosure = { -> - if ( 0 ) { 10 } - else { 20 } - } - def switchClosure = { -> - switch ( 0 ) { - case 0 : 10 ; break - default : 20 ; break - } - } - - assert ifClosure() == 20 - assert switchClosure() == 10 - } -}
