Painless: modify grammar to allow more statement delimiters (#29566)

This allows the grammar to determine when and what delimiters statements will use by
splitting up the statements into regular statements and delimited statements, those that do
not require a delimiter versus those that do. This allows consumers of the statements to
determine what delimiters the statements will use so that in certain cases semicolons are
not necessary like when there's a closing right bracket.

This change removes the need for semicolon insertion in the lexer, simplifying the existing
lexer quite a bit. It also ensures that there isn't a need to track semicolons being inserted
into places that aren't necessary such as array initializers.

modules/lang-painless/src/main/antlr/PainlessParser.g4

@@ -22,7 +22,7 @@ parser grammar PainlessParser;
 options { tokenVocab=PainlessLexer; }
 
 source
-    : function* statement* EOF
+    : function* statement* dstatement? EOF
     ;
 
 function
@@ -33,23 +33,31 @@ parameters
     : LP ( decltype ID ( COMMA decltype ID )* )? RP
     ;
 
+statement
+    : rstatement
+    | dstatement SEMICOLON
+    ;
+
 // Note we use a predicate on the if/else case here to prevent the
 // "dangling-else" ambiguity by forcing the 'else' token to be consumed
 // as soon as one is found.  See (https://en.wikipedia.org/wiki/Dangling_else).
-statement
+rstatement
     : IF LP expression RP trailer ( ELSE trailer | { _input.LA(1) != ELSE }? )                 # if
     | WHILE LP expression RP ( trailer | empty )                                               # while
-    | DO block WHILE LP expression RP delimiter                                                # do
     | FOR LP initializer? SEMICOLON expression? SEMICOLON afterthought? RP ( trailer | empty ) # for
     | FOR LP decltype ID COLON expression RP trailer                                           # each
     | FOR LP ID IN expression RP trailer                                                       # ineach
-    | declaration delimiter                                                                    # decl
-    | CONTINUE delimiter                                                                       # continue
-    | BREAK delimiter                                                                          # break
-    | RETURN expression delimiter                                                              # return
     | TRY block trap+                                                                          # try
-    | THROW expression delimiter                                                               # throw
-    | expression delimiter                                                                     # expr
+    ;
+
+dstatement
+    : DO block WHILE LP expression RP # do
+    | declaration                     # decl
+    | CONTINUE                        # continue
+    | BREAK                           # break
+    | RETURN expression               # return
+    | THROW expression                # throw
+    | expression                      # expr
     ;
 
 trailer
@@ -58,7 +66,7 @@ trailer
     ;
 
 block
-    : LBRACK statement* RBRACK
+    : LBRACK statement* dstatement? RBRACK
     ;
 
 empty
@@ -90,11 +98,6 @@ trap
     : CATCH LP TYPE ID RP block
     ;
 
-delimiter
-    : SEMICOLON
-    | EOF
-    ;
-
 expression
     :               unary                                                 # single
     |               expression ( MUL | DIV | REM ) expression             # binary
@@ -169,8 +172,8 @@ braceaccess
     ;
 
 arrayinitializer
-    : NEW TYPE ( LBRACE expression RBRACE )+ ( postdot postfix* )?                                   # newstandardarray
-    | NEW TYPE LBRACE RBRACE LBRACK ( expression ( COMMA expression )* )? SEMICOLON? RBRACK postfix* # newinitializedarray
+    : NEW TYPE ( LBRACE expression RBRACE )+ ( postdot postfix* )?                        # newstandardarray
+    | NEW TYPE LBRACE RBRACE LBRACK ( expression ( COMMA expression )* )? RBRACK postfix* # newinitializedarray
     ;
 
 listinitializer
@@ -206,10 +209,8 @@ lamtype
     ;
 
 funcref
-    : TYPE REF ID      # classfuncref       // reference to a static or instance method,
-                                            // e.g. ArrayList::size or Integer::compare
-    | decltype REF NEW # constructorfuncref // reference to a constructor, e.g. ArrayList::new
-    | ID REF ID        # capturingfuncref   // reference to an instance method, e.g. object::toString
-                                            // currently limited to capture of a simple variable (id).
-    | THIS REF ID      # localfuncref       // reference to a local function, e.g. this::myfunc
+    : TYPE REF ID      # classfuncref
+    | decltype REF NEW # constructorfuncref
+    | ID REF ID        # capturingfuncref
+    | THIS REF ID      # localfuncref
     ;

modules/lang-painless/src/main/java/org/elasticsearch/painless/antlr/EnhancedPainlessLexer.java

@@ -44,8 +44,7 @@ final class EnhancedPainlessLexer extends PainlessLexer {
     private final String sourceName;
     private final Definition definition;
 
-    private Token stashedNext = null;
-    private Token previous = null;
+    private Token current = null;
 
     EnhancedPainlessLexer(CharStream charStream, String sourceName, Definition definition) {
         super(charStream);
@@ -53,27 +52,10 @@ final class EnhancedPainlessLexer extends PainlessLexer {
         this.definition = definition;
     }
 
-    public Token getPreviousToken() {
-        return previous;
-    }
-
     @Override
     public Token nextToken() {
-        if (stashedNext != null) {
-            previous = stashedNext;
-            stashedNext = null;
-            return previous;
-        }
-        Token next = super.nextToken();
-        if (insertSemicolon(previous, next)) {
-            stashedNext = next;
-            previous = _factory.create(new Pair<TokenSource, CharStream>(this, _input), PainlessLexer.SEMICOLON, ";",
-                    Lexer.DEFAULT_TOKEN_CHANNEL, next.getStartIndex(), next.getStopIndex(), next.getLine(), next.getCharPositionInLine());
-            return previous;
-        } else {
-            previous = next;
-            return next;
-        }
+        current = super.nextToken();
+        return current;
     }
 
     @Override
@@ -101,7 +83,7 @@ final class EnhancedPainlessLexer extends PainlessLexer {
 
     @Override
     protected boolean slashIsRegex() {
-        Token lastToken = getPreviousToken();
+        Token lastToken = current;
         if (lastToken == null) {
             return true;
         }
@@ -120,18 +102,4 @@ final class EnhancedPainlessLexer extends PainlessLexer {
             return true;
         }
     }
-
-    private static boolean insertSemicolon(Token previous, Token next) {
-        if (previous == null || next.getType() != PainlessLexer.RBRACK) {
-            return false;
-        }
-        switch (previous.getType()) {
-        case PainlessLexer.RBRACK:     // };} would be weird!
-        case PainlessLexer.SEMICOLON:  // already have a semicolon, no need to add one
-        case PainlessLexer.LBRACK:     // empty blocks don't need a semicolon
-            return false;
-        default:
-            return true;
-        }
-    }
 }

modules/lang-painless/src/main/java/org/elasticsearch/painless/antlr/PainlessParserBaseVisitor.java

@@ -38,21 +38,21 @@ class PainlessParserBaseVisitor<T> extends AbstractParseTreeVisitor<T> implement
    * <p>The default implementation returns the result of calling
    * {@link #visitChildren} on {@code ctx}.</p>
    */
-  @Override public T visitIf(PainlessParser.IfContext ctx) { return visitChildren(ctx); }
+  @Override public T visitStatement(PainlessParser.StatementContext ctx) { return visitChildren(ctx); }
   /**
    * {@inheritDoc}
    *
    * <p>The default implementation returns the result of calling
    * {@link #visitChildren} on {@code ctx}.</p>
    */
-  @Override public T visitWhile(PainlessParser.WhileContext ctx) { return visitChildren(ctx); }
+  @Override public T visitIf(PainlessParser.IfContext ctx) { return visitChildren(ctx); }
   /**
    * {@inheritDoc}
    *
    * <p>The default implementation returns the result of calling
    * {@link #visitChildren} on {@code ctx}.</p>
    */
-  @Override public T visitDo(PainlessParser.DoContext ctx) { return visitChildren(ctx); }
+  @Override public T visitWhile(PainlessParser.WhileContext ctx) { return visitChildren(ctx); }
   /**
    * {@inheritDoc}
    *
@@ -80,35 +80,42 @@ class PainlessParserBaseVisitor<T> extends AbstractParseTreeVisitor<T> implement
    * <p>The default implementation returns the result of calling
    * {@link #visitChildren} on {@code ctx}.</p>
    */
-  @Override public T visitDecl(PainlessParser.DeclContext ctx) { return visitChildren(ctx); }
+  @Override public T visitTry(PainlessParser.TryContext ctx) { return visitChildren(ctx); }
   /**
    * {@inheritDoc}
    *
    * <p>The default implementation returns the result of calling
    * {@link #visitChildren} on {@code ctx}.</p>
    */
-  @Override public T visitContinue(PainlessParser.ContinueContext ctx) { return visitChildren(ctx); }
+  @Override public T visitDo(PainlessParser.DoContext ctx) { return visitChildren(ctx); }
   /**
    * {@inheritDoc}
    *
    * <p>The default implementation returns the result of calling
    * {@link #visitChildren} on {@code ctx}.</p>
    */
-  @Override public T visitBreak(PainlessParser.BreakContext ctx) { return visitChildren(ctx); }
+  @Override public T visitDecl(PainlessParser.DeclContext ctx) { return visitChildren(ctx); }
   /**
    * {@inheritDoc}
    *
    * <p>The default implementation returns the result of calling
    * {@link #visitChildren} on {@code ctx}.</p>
    */
-  @Override public T visitReturn(PainlessParser.ReturnContext ctx) { return visitChildren(ctx); }
+  @Override public T visitContinue(PainlessParser.ContinueContext ctx) { return visitChildren(ctx); }
   /**
    * {@inheritDoc}
    *
    * <p>The default implementation returns the result of calling
    * {@link #visitChildren} on {@code ctx}.</p>
    */
-  @Override public T visitTry(PainlessParser.TryContext ctx) { return visitChildren(ctx); }
+  @Override public T visitBreak(PainlessParser.BreakContext ctx) { return visitChildren(ctx); }
+  /**
+   * {@inheritDoc}
+   *
+   * <p>The default implementation returns the result of calling
+   * {@link #visitChildren} on {@code ctx}.</p>
+   */
+  @Override public T visitReturn(PainlessParser.ReturnContext ctx) { return visitChildren(ctx); }
   /**
    * {@inheritDoc}
    *
@@ -186,13 +193,6 @@ class PainlessParserBaseVisitor<T> extends AbstractParseTreeVisitor<T> implement
    * {@link #visitChildren} on {@code ctx}.</p>
    */
   @Override public T visitTrap(PainlessParser.TrapContext ctx) { return visitChildren(ctx); }
-  /**
-   * {@inheritDoc}
-   *
-   * <p>The default implementation returns the result of calling
-   * {@link #visitChildren} on {@code ctx}.</p>
-   */
-  @Override public T visitDelimiter(PainlessParser.DelimiterContext ctx) { return visitChildren(ctx); }
   /**
    * {@inheritDoc}
    *

modules/lang-painless/src/main/java/org/elasticsearch/painless/antlr/PainlessParserVisitor.java

@@ -29,92 +29,98 @@ interface PainlessParserVisitor<T> extends ParseTreeVisitor<T> {
    */
   T visitParameters(PainlessParser.ParametersContext ctx);
   /**
-   * Visit a parse tree produced by the {@code if}
-   * labeled alternative in {@link PainlessParser#statement}.
+   * Visit a parse tree produced by {@link PainlessParser#statement}.
    * @param ctx the parse tree
    * @return the visitor result
    */
-  T visitIf(PainlessParser.IfContext ctx);
+  T visitStatement(PainlessParser.StatementContext ctx);
   /**
-   * Visit a parse tree produced by the {@code while}
-   * labeled alternative in {@link PainlessParser#statement}.
+   * Visit a parse tree produced by the {@code if}
+   * labeled alternative in {@link PainlessParser#rstatement}.
    * @param ctx the parse tree
    * @return the visitor result
    */
-  T visitWhile(PainlessParser.WhileContext ctx);
+  T visitIf(PainlessParser.IfContext ctx);
   /**
-   * Visit a parse tree produced by the {@code do}
-   * labeled alternative in {@link PainlessParser#statement}.
+   * Visit a parse tree produced by the {@code while}
+   * labeled alternative in {@link PainlessParser#rstatement}.
    * @param ctx the parse tree
    * @return the visitor result
    */
-  T visitDo(PainlessParser.DoContext ctx);
+  T visitWhile(PainlessParser.WhileContext ctx);
   /**
    * Visit a parse tree produced by the {@code for}
-   * labeled alternative in {@link PainlessParser#statement}.
+   * labeled alternative in {@link PainlessParser#rstatement}.
    * @param ctx the parse tree
    * @return the visitor result
    */
   T visitFor(PainlessParser.ForContext ctx);
   /**
    * Visit a parse tree produced by the {@code each}
-   * labeled alternative in {@link PainlessParser#statement}.
+   * labeled alternative in {@link PainlessParser#rstatement}.
    * @param ctx the parse tree
    * @return the visitor result
    */
   T visitEach(PainlessParser.EachContext ctx);
   /**
    * Visit a parse tree produced by the {@code ineach}
-   * labeled alternative in {@link PainlessParser#statement}.
+   * labeled alternative in {@link PainlessParser#rstatement}.
    * @param ctx the parse tree
    * @return the visitor result
    */
   T visitIneach(PainlessParser.IneachContext ctx);
+  /**
+   * Visit a parse tree produced by the {@code try}
+   * labeled alternative in {@link PainlessParser#rstatement}.
+   * @param ctx the parse tree
+   * @return the visitor result
+   */
+  T visitTry(PainlessParser.TryContext ctx);
+  /**
+   * Visit a parse tree produced by the {@code do}
+   * labeled alternative in {@link PainlessParser#dstatement}.
+   * @param ctx the parse tree
+   * @return the visitor result
+   */
+  T visitDo(PainlessParser.DoContext ctx);
   /**
    * Visit a parse tree produced by the {@code decl}
-   * labeled alternative in {@link PainlessParser#statement}.
+   * labeled alternative in {@link PainlessParser#dstatement}.
    * @param ctx the parse tree
    * @return the visitor result
    */
   T visitDecl(PainlessParser.DeclContext ctx);
   /**
    * Visit a parse tree produced by the {@code continue}
-   * labeled alternative in {@link PainlessParser#statement}.
+   * labeled alternative in {@link PainlessParser#dstatement}.
    * @param ctx the parse tree
    * @return the visitor result
    */
   T visitContinue(PainlessParser.ContinueContext ctx);
   /**
    * Visit a parse tree produced by the {@code break}
-   * labeled alternative in {@link PainlessParser#statement}.
+   * labeled alternative in {@link PainlessParser#dstatement}.
    * @param ctx the parse tree
    * @return the visitor result
    */
   T visitBreak(PainlessParser.BreakContext ctx);
   /**
    * Visit a parse tree produced by the {@code return}
-   * labeled alternative in {@link PainlessParser#statement}.
+   * labeled alternative in {@link PainlessParser#dstatement}.
    * @param ctx the parse tree
    * @return the visitor result
    */
   T visitReturn(PainlessParser.ReturnContext ctx);
-  /**
-   * Visit a parse tree produced by the {@code try}
-   * labeled alternative in {@link PainlessParser#statement}.
-   * @param ctx the parse tree
-   * @return the visitor result
-   */
-  T visitTry(PainlessParser.TryContext ctx);
   /**
    * Visit a parse tree produced by the {@code throw}
-   * labeled alternative in {@link PainlessParser#statement}.
+   * labeled alternative in {@link PainlessParser#dstatement}.
    * @param ctx the parse tree
    * @return the visitor result
    */
   T visitThrow(PainlessParser.ThrowContext ctx);
   /**
    * Visit a parse tree produced by the {@code expr}
-   * labeled alternative in {@link PainlessParser#statement}.
+   * labeled alternative in {@link PainlessParser#dstatement}.
    * @param ctx the parse tree
    * @return the visitor result
    */
@@ -173,12 +179,6 @@ interface PainlessParserVisitor<T> extends ParseTreeVisitor<T> {
    * @return the visitor result
    */
   T visitTrap(PainlessParser.TrapContext ctx);
-  /**
-   * Visit a parse tree produced by {@link PainlessParser#delimiter}.
-   * @param ctx the parse tree
-   * @return the visitor result
-   */
-  T visitDelimiter(PainlessParser.DelimiterContext ctx);
   /**
    * Visit a parse tree produced by the {@code single}
    * labeled alternative in {@link PainlessParser#expression}.

modules/lang-painless/src/main/java/org/elasticsearch/painless/antlr/Walker.java

@@ -56,7 +56,6 @@ import org.elasticsearch.painless.antlr.PainlessParser.DeclContext;
 import org.elasticsearch.painless.antlr.PainlessParser.DeclarationContext;
 import org.elasticsearch.painless.antlr.PainlessParser.DecltypeContext;
 import org.elasticsearch.painless.antlr.PainlessParser.DeclvarContext;
-import org.elasticsearch.painless.antlr.PainlessParser.DelimiterContext;
 import org.elasticsearch.painless.antlr.PainlessParser.DoContext;
 import org.elasticsearch.painless.antlr.PainlessParser.DynamicContext;
 import org.elasticsearch.painless.antlr.PainlessParser.EachContext;
@@ -264,6 +263,10 @@ public final class Walker extends PainlessParserBaseVisitor<ANode> {
             statements.add((AStatement)visit(statement));
         }
 
+        if (ctx.dstatement() != null) {
+            statements.add((AStatement)visit(ctx.dstatement()));
+        }
+
         return new SSource(scriptClassInfo, settings, sourceName, sourceText, debugStream, (MainMethodReserved)reserved.pop(),
                            location(ctx), functions, globals, statements);
     }
@@ -290,6 +293,10 @@ public final class Walker extends PainlessParserBaseVisitor<ANode> {
             statements.add((AStatement)visit(statement));
         }
 
+        if (ctx.block().dstatement() != null) {
+            statements.add((AStatement)visit(ctx.block().dstatement()));
+        }
+
         return new SFunction((FunctionReserved)reserved.pop(), location(ctx), rtnType, name,
                              paramTypes, paramNames, statements, false);
     }
@@ -299,6 +306,17 @@ public final class Walker extends PainlessParserBaseVisitor<ANode> {
         throw location(ctx).createError(new IllegalStateException("Illegal tree structure."));
     }
 
+    @Override
+    public ANode visitStatement(StatementContext ctx) {
+        if (ctx.rstatement() != null) {
+            return visit(ctx.rstatement());
+        } else if (ctx.dstatement() != null) {
+            return visit(ctx.dstatement());
+        } else {
+            throw location(ctx).createError(new IllegalStateException("Illegal tree structure."));
+        }
+    }
+
     @Override
     public ANode visitIf(IfContext ctx) {
         AExpression expression = (AExpression)visit(ctx.expression());
@@ -446,7 +464,7 @@ public final class Walker extends PainlessParserBaseVisitor<ANode> {
 
     @Override
     public ANode visitBlock(BlockContext ctx) {
-        if (ctx.statement().isEmpty()) {
+        if (ctx.statement().isEmpty() && ctx.dstatement() == null) {
             return null;
         } else {
             List<AStatement> statements = new ArrayList<>();
@@ -455,6 +473,10 @@ public final class Walker extends PainlessParserBaseVisitor<ANode> {
                 statements.add((AStatement)visit(statement));
             }
 
+            if (ctx.dstatement() != null) {
+                statements.add((AStatement)visit(ctx.dstatement()));
+            }
+
             return new SBlock(location(ctx), statements);
         }
     }
@@ -514,11 +536,6 @@ public final class Walker extends PainlessParserBaseVisitor<ANode> {
         return new SCatch(location(ctx), type, name, block);
     }
 
-    @Override
-    public ANode visitDelimiter(DelimiterContext ctx) {
-        throw location(ctx).createError(new IllegalStateException("Illegal tree structure."));
-    }
-
     @Override
     public ANode visitSingle(SingleContext ctx) {
         return visit(ctx.unary());
@@ -1074,6 +1091,10 @@ public final class Walker extends PainlessParserBaseVisitor<ANode> {
             for (StatementContext statement : ctx.block().statement()) {
                 statements.add((AStatement)visit(statement));
             }
+
+            if (ctx.block().dstatement() != null) {
+                statements.add((AStatement)visit(ctx.block().dstatement()));
+            }
         }
 
         FunctionReserved lambdaReserved = (FunctionReserved)reserved.pop();

modules/lang-painless/src/test/java/org/elasticsearch/painless/RegexTests.java

@@ -278,6 +278,6 @@ public class RegexTests extends ScriptTestCase {
         IllegalArgumentException e = expectScriptThrows(IllegalArgumentException.class, () -> {
             exec("/asdf/b", false); // Not picky so we get a non-assertion error
         });
-        assertEquals("unexpected token ['b'] was expecting one of [{<EOF>, ';'}].", e.getMessage());
+        assertEquals("invalid sequence of tokens near ['b'].", e.getMessage());
     }
 }

modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java

@@ -256,7 +256,7 @@ public class WhenThingsGoWrongTests extends ScriptTestCase {
             // We don't want PICKY here so we get the normal error message
             exec("def i = 1} return 1", emptyMap(), emptyMap(), null, false);
         });
-        assertEquals("unexpected token ['}'] was expecting one of [<EOF>].", e.getMessage());
+        assertEquals("invalid sequence of tokens near ['}'].", e.getMessage());
     }
 
     public void testBadBoxingCast() {