Skip to content

Commit d5b22aa

Browse files
eric-milleseric-milles
committed
GROOVY-11565: empty block handling
3_0_X backport
1 parent d96c59c commit d5b22aa

File tree

5 files changed

+89
-97
lines changed

5 files changed

+89
-97
lines changed

‎src/main/antlr2/org/codehaus/groovy/antlr/groovy.g

+14-18
Original file line numberDiff line numberDiff line change
@@ -1718,35 +1718,31 @@ statement[int prevToken]
17181718
// side-effects.
17191719
// The prevToken is used to check for dumb expressions like +1.
17201720
| es:expressionStatement[prevToken]
1721-
//{#statement = #(create(EXPR,"EXPR",first,LT(1)), es);}
17221721

1723-
// If-else statement
1724-
| "if"! LPAREN! ale:assignmentLessExpression! RPAREN! nlsWarn! ifCbs:compatibleBodyStatement!
1722+
// If-then-else statement
1723+
| "if"! LPAREN! condExpr:assignmentLessExpression! RPAREN! nlsWarn! (skipStmt:SEMI! | thenStmt:compatibleBodyStatement!)
17251724
(
1726-
// CONFLICT: the old "dangling-else" problem...
1727-
// ANTLR generates proper code matching
1728-
// as soon as possible. Hush warning.
1729-
options {
1730-
warnWhenFollowAmbig = false;
1731-
}
1732-
: // lookahead to check if we're entering an 'else' clause
17331725
( (sep!)? "else"! )=>
1734-
(sep!)? // allow SEMI here for compatibility with Java
1735-
"else"! nlsWarn! elseCbs:compatibleBodyStatement!
1726+
(sep!)? // "if (cond) 1; else 2" -- dangling SEMI!
1727+
"else"! nlsWarn! elseStmt:compatibleBodyStatement!
17361728
)?
1737-
{#statement = #(create(LITERAL_if,"if",first,LT(1)), ale, ifCbs, elseCbs);}
1729+
{
1730+
if (#skipStmt != null)
1731+
#statement = #(create(LITERAL_if,"if",first,LT(1)),condExpr,skipStmt,elseStmt);
1732+
else
1733+
#statement = #(create(LITERAL_if,"if",first,LT(1)),condExpr,thenStmt,elseStmt);
1734+
}
17381735

17391736
// For statement
17401737
| forStatement
17411738

17421739
// While statement
1743-
| "while"! LPAREN! sce=while_sce:strictContextExpression[false]! RPAREN! nlsWarn!
1744-
(s:SEMI! | while_cbs:compatibleBodyStatement!)
1740+
| "while"! LPAREN! sce=whileExpr:strictContextExpression[false]! RPAREN! nlsWarn! (emptyStmt:SEMI! | whileStmt:compatibleBodyStatement!)
17451741
{
1746-
if (#s != null)
1747-
#statement = #(create(LITERAL_while,"Literal_while",first,LT(1)), while_sce, s);
1742+
if (#emptyStmt != null)
1743+
#statement = #(create(LITERAL_while,"Literal_while",first,LT(1)),whileExpr,emptyStmt);
17481744
else
1749-
#statement = #(create(LITERAL_while,"Literal_while",first,LT(1)), while_sce, while_cbs);
1745+
#statement = #(create(LITERAL_while,"Literal_while",first,LT(1)),whileExpr,whileStmt);
17501746
}
17511747

17521748
// Import statement. Can be used in any scope. Has "import x as y" also.

‎src/main/java/org/codehaus/groovy/antlr/AntlrParserPlugin.java

+5-8
Original file line numberDiff line numberDiff line change
@@ -1486,19 +1486,16 @@ protected Statement forStatement(AST forNode) {
14861486
}
14871487

14881488
protected Statement ifStatement(AST ifNode) {
1489-
AST node = ifNode.getFirstChild();
1490-
assertNodeType(EXPR, node);
1489+
AST node = ifNode.getFirstChild(); assertNodeType(EXPR, node);
14911490
BooleanExpression booleanExpression = booleanExpression(node);
14921491

14931492
node = node.getNextSibling();
1494-
Statement ifBlock = statement(node);
1493+
Statement thenBlock = isType(SEMI, node) ? EmptyStatement.INSTANCE : statement(node); // GROOVY-11565
14951494

1496-
Statement elseBlock = EmptyStatement.INSTANCE;
14971495
node = node.getNextSibling();
1498-
if (node != null) {
1499-
elseBlock = statement(node);
1500-
}
1501-
IfStatement ifStatement = new IfStatement(booleanExpression, ifBlock, elseBlock);
1496+
Statement elseBlock = (node == null || isType(SEMI, node)) ? EmptyStatement.INSTANCE : statement(node);
1497+
1498+
IfStatement ifStatement = new IfStatement(booleanExpression, thenBlock, elseBlock);
15021499
configureAST(ifStatement, ifNode);
15031500
return ifStatement;
15041501
}

‎subprojects/parser-antlr4/src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java

+21-62
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@
3535
import org.antlr.v4.runtime.misc.ParseCancellationException;
3636
import org.antlr.v4.runtime.tree.ParseTree;
3737
import org.antlr.v4.runtime.tree.TerminalNode;
38-
import org.apache.groovy.parser.antlr4.GroovyParser.*;
3938
import org.apache.groovy.parser.antlr4.internal.DescriptiveErrorStrategy;
4039
import org.apache.groovy.parser.antlr4.internal.atnmanager.AtnManager;
4140
import org.apache.groovy.parser.antlr4.util.StringUtils;
@@ -146,25 +145,7 @@
146145
import java.util.stream.Stream;
147146

148147
import static groovy.lang.Tuple.tuple;
149-
import static org.apache.groovy.parser.antlr4.GroovyParser.ADD;
150-
import static org.apache.groovy.parser.antlr4.GroovyParser.AS;
151-
import static org.apache.groovy.parser.antlr4.GroovyParser.CASE;
152-
import static org.apache.groovy.parser.antlr4.GroovyParser.DEC;
153-
import static org.apache.groovy.parser.antlr4.GroovyParser.DEF;
154-
import static org.apache.groovy.parser.antlr4.GroovyParser.DEFAULT;
155-
import static org.apache.groovy.parser.antlr4.GroovyParser.GE;
156-
import static org.apache.groovy.parser.antlr4.GroovyParser.GT;
157-
import static org.apache.groovy.parser.antlr4.GroovyParser.IN;
158-
import static org.apache.groovy.parser.antlr4.GroovyParser.INC;
159-
import static org.apache.groovy.parser.antlr4.GroovyParser.INSTANCEOF;
160-
import static org.apache.groovy.parser.antlr4.GroovyParser.LE;
161-
import static org.apache.groovy.parser.antlr4.GroovyParser.LT;
162-
import static org.apache.groovy.parser.antlr4.GroovyParser.NOT_IN;
163-
import static org.apache.groovy.parser.antlr4.GroovyParser.NOT_INSTANCEOF;
164-
import static org.apache.groovy.parser.antlr4.GroovyParser.PRIVATE;
165-
import static org.apache.groovy.parser.antlr4.GroovyParser.STATIC;
166-
import static org.apache.groovy.parser.antlr4.GroovyParser.SUB;
167-
import static org.apache.groovy.parser.antlr4.GroovyParser.VAR;
148+
import static org.apache.groovy.parser.antlr4.GroovyParser.*;
168149
import static org.apache.groovy.parser.antlr4.util.PositionConfigureUtils.configureAST;
169150
import static org.codehaus.groovy.classgen.asm.util.TypeUtil.isPrimitiveType;
170151
import static org.codehaus.groovy.runtime.DefaultGroovyMethods.asBoolean;
@@ -445,29 +426,22 @@ public Statement visitConditionalStatement(final ConditionalStatementContext ctx
445426
@Override
446427
public IfStatement visitIfElseStatement(final IfElseStatementContext ctx) {
447428
Expression conditionExpression = this.visitExpressionInPar(ctx.expressionInPar());
448-
BooleanExpression booleanExpression =
449-
configureAST(
450-
new BooleanExpression(conditionExpression), conditionExpression);
429+
BooleanExpression booleanExpression = configureAST(new BooleanExpression(conditionExpression), conditionExpression);
451430

452-
Statement ifBlock =
453-
this.unpackStatement(
454-
(Statement) this.visit(ctx.tb));
455-
Statement elseBlock =
456-
this.unpackStatement(
457-
asBoolean(ctx.ELSE())
458-
? (Statement) this.visit(ctx.fb)
459-
: EmptyStatement.INSTANCE);
431+
Statement thenStatement = this.unpackStatement((Statement) this.visit(ctx.tb)) ;
432+
Statement elseStatement = ctx.ELSE() != null ? this.unpackStatement((Statement) this.visit(ctx.fb)) : EmptyStatement.INSTANCE;
460433

461-
return configureAST(new IfStatement(booleanExpression, ifBlock, elseBlock), ctx);
434+
return configureAST(new IfStatement(booleanExpression, thenStatement, elseStatement), ctx);
462435
}
463436

464437
@Override
465438
public Statement visitLoopStmtAlt(final LoopStmtAltContext ctx) {
466439
visitingLoopStatementCount += 1;
467-
Statement result = configureAST((Statement) this.visit(ctx.loopStatement()), ctx);
468-
visitingLoopStatementCount -= 1;
469-
470-
return result;
440+
try {
441+
return configureAST((Statement) this.visit(ctx.loopStatement()), ctx);
442+
} finally {
443+
visitingLoopStatementCount -= 1;
444+
}
471445
}
472446

473447
@Override
@@ -476,9 +450,7 @@ public ForStatement visitForStmtAlt(final ForStmtAltContext ctx) {
476450

477451
Statement loopBlock = this.unpackStatement((Statement) this.visit(ctx.statement()));
478452

479-
return configureAST(
480-
new ForStatement(controlTuple.getV1(), controlTuple.getV2(), asBoolean(loopBlock) ? loopBlock : EmptyStatement.INSTANCE),
481-
ctx);
453+
return configureAST(new ForStatement(controlTuple.getV1(), controlTuple.getV2(), loopBlock), ctx);
482454
}
483455

484456
@Override
@@ -502,12 +474,12 @@ public Expression visitForInit(final ForInitContext ctx) {
502474

503475
if (asBoolean(ctx.localVariableDeclaration())) {
504476
DeclarationListStatement declarationListStatement = this.visitLocalVariableDeclaration(ctx.localVariableDeclaration());
505-
List<DeclarationExpression> declarationExpressions = declarationListStatement.getDeclarationExpressions();
477+
List<? extends Expression> declarationExpressions = declarationListStatement.getDeclarationExpressions();
506478

507479
if (declarationExpressions.size() == 1) {
508-
return configureAST((Expression) declarationExpressions.get(0), ctx);
480+
return configureAST(declarationExpressions.get(0), ctx);
509481
} else {
510-
return configureAST(new ClosureListExpression((List) declarationExpressions), ctx);
482+
return configureAST(new ClosureListExpression((List<Expression>) declarationExpressions), ctx);
511483
}
512484
}
513485

@@ -563,28 +535,19 @@ public Tuple2<Parameter, Expression> visitClassicalForControl(final ClassicalFor
563535
public WhileStatement visitWhileStmtAlt(final WhileStmtAltContext ctx) {
564536
Tuple2<BooleanExpression, Statement> conditionAndBlock = createLoopConditionExpressionAndBlock(ctx.expressionInPar(), ctx.statement());
565537

566-
return configureAST(
567-
new WhileStatement(conditionAndBlock.getV1(), asBoolean(conditionAndBlock.getV2()) ? conditionAndBlock.getV2() : EmptyStatement.INSTANCE),
568-
ctx);
538+
return configureAST(new WhileStatement(conditionAndBlock.getV1(), conditionAndBlock.getV2()), ctx);
569539
}
570540

571541
@Override
572542
public DoWhileStatement visitDoWhileStmtAlt(final DoWhileStmtAltContext ctx) {
573543
Tuple2<BooleanExpression, Statement> conditionAndBlock = createLoopConditionExpressionAndBlock(ctx.expressionInPar(), ctx.statement());
574544

575-
return configureAST(
576-
new DoWhileStatement(conditionAndBlock.getV1(), asBoolean(conditionAndBlock.getV2()) ? conditionAndBlock.getV2() : EmptyStatement.INSTANCE),
577-
ctx);
545+
return configureAST(new DoWhileStatement(conditionAndBlock.getV1(), conditionAndBlock.getV2()), ctx);
578546
}
579547

580548
private Tuple2<BooleanExpression, Statement> createLoopConditionExpressionAndBlock(final ExpressionInParContext eipc, final StatementContext sc) {
581549
Expression conditionExpression = this.visitExpressionInPar(eipc);
582-
583-
BooleanExpression booleanExpression =
584-
configureAST(
585-
new BooleanExpression(conditionExpression),
586-
conditionExpression
587-
);
550+
BooleanExpression booleanExpression = configureAST(new BooleanExpression(conditionExpression), conditionExpression);
588551

589552
Statement loopBlock = this.unpackStatement((Statement) this.visit(sc));
590553

@@ -4080,16 +4043,12 @@ private BinaryExpression createBinaryExpression(final ExpressionContext left, fi
40804043

40814044
private Statement unpackStatement(final Statement statement) {
40824045
if (statement instanceof DeclarationListStatement) {
4083-
List<ExpressionStatement> expressionStatementList = ((DeclarationListStatement) statement).getDeclarationStatements();
4084-
4085-
if (1 == expressionStatementList.size()) {
4086-
return expressionStatementList.get(0);
4087-
}
4088-
4089-
return configureAST(this.createBlockStatement(statement), statement); // if DeclarationListStatement contains more than 1 declarations, maybe it's better to create a block to hold them
4046+
// if DeclarationListStatement contains more than 1 declarations, maybe it's better to create a block to hold them
4047+
List<ExpressionStatement> expressionStatements = ((DeclarationListStatement) statement).getDeclarationStatements();
4048+
return expressionStatements.size() == 1 ? expressionStatements.get(0) : configureAST(this.createBlockStatement(statement), statement);
40904049
}
40914050

4092-
return statement;
4051+
return Optional.ofNullable(statement).orElse(EmptyStatement.INSTANCE);
40934052
}
40944053

40954054
BlockStatement createBlockStatement(final Statement... statements) {

‎subprojects/parser-antlr4/src/test/groovy/org/apache/groovy/parser/antlr4/GroovyParserTest.groovy

+2-9
Original file line numberDiff line numberDiff line change
@@ -237,7 +237,8 @@ final class GroovyParserTest extends GroovyTestCase {
237237
}
238238

239239
void "test groovy core - IfElse"() {
240-
doTest('core/IfElse_01.groovy', [AssertStatement])
240+
doTest('core/IfElse_01.groovy')
241+
doTest('core/IfElse_02.groovy')
241242
}
242243

243244
void "test groovy core - For"() {
@@ -264,7 +265,6 @@ final class GroovyParserTest extends GroovyTestCase {
264265
doRunAndTestAntlr4('core/DoWhile_04x.groovy')
265266
}
266267

267-
268268
void "test groovy core - TryCatch"() {
269269
doTest('core/TryCatch_01.groovy')
270270
}
@@ -291,7 +291,6 @@ final class GroovyParserTest extends GroovyTestCase {
291291
doRunAndTestAntlr4('core/DefaultMethod_02x.groovy')
292292
}
293293

294-
295294
void "test groovy core - Switch"() {
296295
doTest('core/Switch_01.groovy')
297296
}
@@ -373,12 +372,6 @@ final class GroovyParserTest extends GroovyTestCase {
373372
doRunAndTestAntlr4('core/Command_07x.groovy')
374373
}
375374

376-
/*
377-
void "test groovy core - Unicode"() {
378-
doTest('core/Unicode_01.groovy')
379-
}
380-
*/
381-
382375
void "test groovy core - BreakingChanges"() {
383376
doRunAndTestAntlr4('core/BreakingChange_01x.groovy')
384377
doRunAndTestAntlr4('core/BreakingChange_02x.groovy')

‎subprojects/parser-antlr4/src/test/resources/core/IfElse_02.groovy

+47
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
// GROOVY-11565: empty block handling
21+
22+
if (true);
23+
if (true) ;
24+
if (true)
25+
;
26+
27+
if (true){}
28+
if (true) {}
29+
if (true)
30+
{
31+
}
32+
33+
if (true);else 0
34+
if (true) ; else 0
35+
if (true)
36+
;
37+
else
38+
0
39+
40+
if (true){}else{}
41+
if (true) {} else {}
42+
if (true)
43+
{
44+
}
45+
else
46+
{
47+
}

0 commit comments

Comments
 (0)