This is an automated email from the ASF dual-hosted git repository. asf-gitbox-commits pushed a commit to branch GROOVY-12169 in repository https://gitbox.apache.org/repos/asf/groovy.git
commit 2ae01a3e703031668c45481556642d4768fe1fc1 Author: Daniel Sun <[email protected]> AuthorDate: Sat Jul 18 05:21:34 2026 +0900 GROOVY-12169: Improve syntax error message and caret position for missing ')' --- .../antlr4/internal/DescriptiveErrorStrategy.java | 26 +- .../antlr4/internal/MissingParenDiagnostic.java | 427 +++++++++++++++++++++ .../groovy/parser/antlr4/SyntaxErrorTest.groovy | 45 ++- 3 files changed, 492 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/apache/groovy/parser/antlr4/internal/DescriptiveErrorStrategy.java b/src/main/java/org/apache/groovy/parser/antlr4/internal/DescriptiveErrorStrategy.java index 1fbb41350d..212101df93 100644 --- a/src/main/java/org/apache/groovy/parser/antlr4/internal/DescriptiveErrorStrategy.java +++ b/src/main/java/org/apache/groovy/parser/antlr4/internal/DescriptiveErrorStrategy.java @@ -34,9 +34,15 @@ import org.antlr.v4.runtime.misc.ParseCancellationException; /** * Provide friendly error messages when parsing errors occurred. + * <p> + * Extends {@link BailErrorStrategy} so the successful-parse path stays + * allocation-free and ATN-light (no grammar-level error alternatives — + * those were removed after GROOVY-9588). Missing {@code ')'} diagnostics + * run only after a recognition failure via {@link MissingParenDiagnostic}. + * </p> */ public class DescriptiveErrorStrategy extends BailErrorStrategy { - private CharStream charStream; + private final CharStream charStream; public DescriptiveErrorStrategy(CharStream charStream) { this.charStream = charStream; @@ -69,6 +75,20 @@ public class DescriptiveErrorStrategy extends BailErrorStrategy { return null; } + /** + * Prefer a precise "Missing ')'" diagnostic when the token stream + * clearly indicates an unclosed / incomplete parenthesised construct; + * otherwise fall back to the generic "Unexpected input" message. + */ + private void reportError(Parser recognizer, RecognitionException e, String fallbackMessage) { + Token missingParenAt = MissingParenDiagnostic.locate(recognizer.getInputStream(), e); + if (missingParenAt != null) { + recognizer.notifyErrorListeners(missingParenAt, MissingParenDiagnostic.MESSAGE, e); + return; + } + notifyErrorListeners(recognizer, fallbackMessage, e); + } + protected String createNoViableAlternativeErrorMessage(Parser recognizer, NoViableAltException e) { TokenStream tokens = recognizer.getInputStream(); String input; @@ -89,7 +109,7 @@ public class DescriptiveErrorStrategy extends BailErrorStrategy { protected void reportNoViableAlternative(Parser recognizer, NoViableAltException e) { - notifyErrorListeners(recognizer, this.createNoViableAlternativeErrorMessage(recognizer, e), e); + reportError(recognizer, e, this.createNoViableAlternativeErrorMessage(recognizer, e)); } protected String createInputMismatchErrorMessage(Parser recognizer, @@ -101,7 +121,7 @@ public class DescriptiveErrorStrategy extends BailErrorStrategy { protected void reportInputMismatch(Parser recognizer, InputMismatchException e) { - notifyErrorListeners(recognizer, this.createInputMismatchErrorMessage(recognizer, e), e); + reportError(recognizer, e, this.createInputMismatchErrorMessage(recognizer, e)); } protected String createFailedPredicateErrorMessage(Parser recognizer, diff --git a/src/main/java/org/apache/groovy/parser/antlr4/internal/MissingParenDiagnostic.java b/src/main/java/org/apache/groovy/parser/antlr4/internal/MissingParenDiagnostic.java new file mode 100644 index 0000000000..08d3656326 --- /dev/null +++ b/src/main/java/org/apache/groovy/parser/antlr4/internal/MissingParenDiagnostic.java @@ -0,0 +1,427 @@ +/* + * 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.apache.groovy.parser.antlr4.internal; + +import org.antlr.v4.runtime.BufferedTokenStream; +import org.antlr.v4.runtime.CommonToken; +import org.antlr.v4.runtime.RecognitionException; +import org.antlr.v4.runtime.Token; +import org.antlr.v4.runtime.TokenStream; +import org.antlr.v4.runtime.misc.IntervalSet; + +import java.util.ArrayDeque; +import java.util.ArrayList; +import java.util.Deque; +import java.util.List; + +import static org.apache.groovy.parser.antlr4.GroovyParser.AT; +import static org.apache.groovy.parser.antlr4.GroovyParser.BITAND; +import static org.apache.groovy.parser.antlr4.GroovyParser.BooleanLiteral; +import static org.apache.groovy.parser.antlr4.GroovyParser.BuiltInPrimitiveType; +import static org.apache.groovy.parser.antlr4.GroovyParser.CapitalizedIdentifier; +import static org.apache.groovy.parser.antlr4.GroovyParser.DOT; +import static org.apache.groovy.parser.antlr4.GroovyParser.FloatingPointLiteral; +import static org.apache.groovy.parser.antlr4.GroovyParser.GT; +import static org.apache.groovy.parser.antlr4.GroovyParser.GStringBegin; +import static org.apache.groovy.parser.antlr4.GroovyParser.Identifier; +import static org.apache.groovy.parser.antlr4.GroovyParser.IntegerLiteral; +import static org.apache.groovy.parser.antlr4.GroovyParser.LBRACK; +import static org.apache.groovy.parser.antlr4.GroovyParser.LPAREN; +import static org.apache.groovy.parser.antlr4.GroovyParser.LT; +import static org.apache.groovy.parser.antlr4.GroovyParser.RBRACE; +import static org.apache.groovy.parser.antlr4.GroovyParser.RBRACK; +import static org.apache.groovy.parser.antlr4.GroovyParser.RPAREN; +import static org.apache.groovy.parser.antlr4.GroovyParser.StringLiteral; +import static org.apache.groovy.parser.antlr4.GroovyParser.VOID; + +/** + * Error-path-only diagnostic for a missing {@code ')'}. + * <p> + * Successful parses never call into this class, so there is no steady-state + * cost. When a parse has already failed, a single linear scan of the token + * stream is used to recover a precise "Missing ')'" location — something + * ANTLR4's default mismatch reporting cannot do without grammar-level error + * alternatives (which were removed because they hurt parsing performance; + * see GROOVY-9588). + * </p> + * <p> + * Detection is ordered from most specific to least specific: + * <ol> + * <li>RecognitionException whose sole expected token is {@code RPAREN}</li> + * <li>Cast / parenthesised-type pattern: + * {@code '(' type <expr-start>} without the closing {@code ')'} of the cast</li> + * <li>Unclosed {@code '('} at EOF or at a structural hard-stop + * ({@code '}'}, etc.)</li> + * </ol> + */ +final class MissingParenDiagnostic { + + static final String MESSAGE = "Missing ')'"; + + private MissingParenDiagnostic() { + } + + /** + * @return a token whose line / charPositionInLine mark the caret location + * for {@link #MESSAGE}, or {@code null} if this failure does not + * look like a missing {@code ')'} + */ + static Token locate(TokenStream tokens, RecognitionException e) { + if (tokens == null) { + return null; + } + + List<Token> defaultChannel = collectDefaultChannelTokens(tokens); + if (defaultChannel.isEmpty()) { + return null; + } + + // (1) Parser already knows it wanted only ')' + Token fromExpected = fromSoleExpectedRparen(e, defaultChannel); + if (fromExpected != null) { + return fromExpected; + } + + // (2) Cast form: (type <expr> — ')' of the cast is missing + Token fromCast = fromCastPattern(defaultChannel); + if (fromCast != null) { + return fromCast; + } + + // (3) Unclosed '(' + return fromUnclosedParen(defaultChannel); + } + + //-------------------------------------------------------------------------- + // Strategy 1 — sole expected token is RPAREN + //-------------------------------------------------------------------------- + + private static Token fromSoleExpectedRparen(RecognitionException e, List<Token> defaultChannel) { + if (e == null) { + return null; + } + IntervalSet expected = e.getExpectedTokens(); + if (expected == null || expected.size() != 1 || !expected.contains(RPAREN)) { + return null; + } + // If '(' / ')' are already balanced in the stream, the failure is an + // unexpected intermediate token (e.g. `foo(1;2;3)` using ';' for ','), + // not a missing ')' — leave the generic error message alone. + if (parenDepth(defaultChannel) == 0) { + return null; + } + Token offending = e.getOffendingToken(); + if (offending == null) { + return null; + } + // EOF's line/column often sit on the following empty line after a + // trailing newline; point just past the previous real token instead. + if (offending.getType() == Token.EOF) { + Token lastReal = lastNonEof(defaultChannel); + if (lastReal != null) { + return insertionPointAfter(lastReal); + } + } + return offending; + } + + /** Net '('…')' depth at end of stream; 0 means balanced (or never opened). */ + private static int parenDepth(List<Token> tokens) { + int depth = 0; + for (Token t : tokens) { + int type = t.getType(); + if (type == LPAREN) { + depth++; + } else if (type == RPAREN && depth > 0) { + depth--; + } + } + return depth; + } + + private static Token lastNonEof(List<Token> tokens) { + for (int i = tokens.size() - 1; i >= 0; i--) { + Token t = tokens.get(i); + if (t.getType() != Token.EOF) { + return t; + } + } + return null; + } + + //-------------------------------------------------------------------------- + // Strategy 2 — cast / parenthesised-type pattern + //-------------------------------------------------------------------------- + + /** + * Scan for {@code '(' type <token>} where the {@code ')'} of a cast is + * missing. The earliest high-confidence site is reported. + * <p> + * Confidence rules avoid flagging parenthesised command expressions + * such as {@code (foo bar)}: + * <ul> + * <li>type is a built-in primitive / {@code void} (e.g. {@code (int 123)}), or</li> + * <li>the token after the type is a literal or {@code '('} + * (e.g. {@code (Foo)1}, {@code (Foo)(x)})</li> + * </ul> + */ + private static Token fromCastPattern(List<Token> tokens) { + final int n = tokens.size(); + for (int i = 0; i < n; i++) { + if (tokens.get(i).getType() != LPAREN) { + continue; + } + int typeStart = skipAnnotations(tokens, i + 1); + if (typeStart < 0 || typeStart >= n) { + continue; + } + boolean primitiveType = isPrimitiveOrVoid(tokens.get(typeStart).getType()); + int afterType = matchType(tokens, i + 1); + if (afterType <= i + 1) { + continue; // no type after '(' + } + if (afterType >= n) { + // '(type' at EOF — point just past the type + return insertionPointAfter(tokens.get(afterType - 1)); + } + Token next = tokens.get(afterType); + int nt = next.getType(); + if (nt == RPAREN || nt == BITAND) { + continue; // well-formed cast or intersection type + } + // Type continuators that matchType should already have consumed; + // if they remain, skip rather than false-positive. + if (nt == DOT || nt == LT || nt == LBRACK) { + continue; + } + if (primitiveType || isHighConfidenceCastOperand(nt)) { + return next; + } + } + return null; + } + + /** + * A following literal or nested {@code '('} is a high-confidence cast + * operand; an identifier is not (it may be a command-expression argument). + */ + private static boolean isHighConfidenceCastOperand(int tokenType) { + return isLiteral(tokenType) || tokenType == LPAREN; + } + + //-------------------------------------------------------------------------- + // Strategy 3 — unclosed '(' + //-------------------------------------------------------------------------- + + private static Token fromUnclosedParen(List<Token> tokens) { + Deque<Token> openParens = new ArrayDeque<>(); + Token lastInside = null; + + for (Token t : tokens) { + int type = t.getType(); + if (type == Token.EOF) { + break; + } + if (type == LPAREN) { + openParens.push(t); + lastInside = t; + continue; + } + if (type == RPAREN) { + if (!openParens.isEmpty()) { + openParens.pop(); + } + lastInside = t; + continue; + } + // Hard-stop: a '}' that is not matched by an in-stream '{' while + // parens are still open almost always means the ')' was omitted + // before the closing brace of an enclosing block + // (e.g. `def x() { println((int) 123 <HERE> }`). + if (type == RBRACE && !openParens.isEmpty()) { + return lastInside != null ? insertionPointAfter(lastInside) + : t; + } + if (!openParens.isEmpty()) { + lastInside = t; + } + } + + if (!openParens.isEmpty()) { + return lastInside != null ? insertionPointAfter(lastInside) + : openParens.peek(); + } + return null; + } + + //-------------------------------------------------------------------------- + // Type matching (conservative, token-level) + //-------------------------------------------------------------------------- + + /** + * @return index after any leading annotations, or {@code -1} on a + * malformed annotation prefix + */ + private static int skipAnnotations(List<Token> tokens, int start) { + int i = start; + final int n = tokens.size(); + while (i < n && tokens.get(i).getType() == AT) { + i++; + if (i >= n || !isIdentifierLike(tokens.get(i).getType())) { + return -1; + } + i++; + if (i < n && tokens.get(i).getType() == LPAREN) { + i = skipBalanced(tokens, i, LPAREN, RPAREN); + if (i < 0) { + return -1; + } + } + } + return i; + } + + /** + * @return index of the first token <em>after</em> a matched type, or + * {@code start} if no type starts at {@code start} + */ + private static int matchType(List<Token> tokens, int start) { + int i = skipAnnotations(tokens, start); + if (i < 0) { + return start; + } + final int n = tokens.size(); + if (i >= n) { + return start; + } + + int head = tokens.get(i).getType(); + if (isPrimitiveOrVoid(head)) { + i++; + } else if (isIdentifierLike(head)) { + i++; + // qualified name: Foo.Bar.Baz + while (i + 1 < n + && tokens.get(i).getType() == DOT + && isIdentifierLike(tokens.get(i + 1).getType())) { + i += 2; + } + // type arguments: <...> + if (i < n && tokens.get(i).getType() == LT) { + i = skipBalanced(tokens, i, LT, GT); + if (i < 0) { + return start; + } + } + } else { + return start; + } + + // array dimensions: [][] + while (i + 1 < n + && tokens.get(i).getType() == LBRACK + && tokens.get(i + 1).getType() == RBRACK) { + i += 2; + } + + return i; + } + + /** + * Skip a balanced pair starting at {@code openIdx} (which must be the + * opening token). Returns the index after the matching closer, or + * {@code -1} if unbalanced. + */ + private static int skipBalanced(List<Token> tokens, int openIdx, int openType, int closeType) { + int depth = 0; + for (int i = openIdx; i < tokens.size(); i++) { + int t = tokens.get(i).getType(); + if (t == openType) { + depth++; + } else if (t == closeType) { + depth--; + if (depth == 0) { + return i + 1; + } + } else if (t == Token.EOF) { + return -1; + } + } + return -1; + } + + //-------------------------------------------------------------------------- + // Token helpers + //-------------------------------------------------------------------------- + + private static List<Token> collectDefaultChannelTokens(TokenStream tokens) { + // Must fill through EOF so trailing ')' are visible — otherwise an + // early failure inside `(...)` looks like an unclosed paren + // (e.g. `foo(1;2;3)` fails at ';' before ')' has been pulled). + if (tokens instanceof BufferedTokenStream) { + ((BufferedTokenStream) tokens).fill(); + } + + List<Token> result = new ArrayList<>(); + try { + for (int i = 0; ; i++) { + Token t = tokens.get(i); + if (t.getType() == Token.EOF) { + result.add(t); + break; + } + if (t.getChannel() == Token.DEFAULT_CHANNEL) { + result.add(t); + } + } + } catch (IndexOutOfBoundsException | IllegalArgumentException ignored) { + // Non-buffered stream: keep whatever was collected + } + return result; + } + + private static boolean isPrimitiveOrVoid(int tokenType) { + return tokenType == BuiltInPrimitiveType || tokenType == VOID; + } + + private static boolean isIdentifierLike(int tokenType) { + return tokenType == Identifier || tokenType == CapitalizedIdentifier; + } + + private static boolean isLiteral(int tokenType) { + return tokenType == IntegerLiteral + || tokenType == FloatingPointLiteral + || tokenType == StringLiteral + || tokenType == GStringBegin + || tokenType == BooleanLiteral; + } + + /** + * Caret sits just past {@code token} (the usual "insert ')' here" point). + */ + private static Token insertionPointAfter(Token token) { + String text = token.getText(); + int length = text != null ? text.codePointCount(0, text.length()) : 0; + CommonToken point = new CommonToken(Token.INVALID_TYPE); + point.setLine(token.getLine()); + point.setCharPositionInLine(token.getCharPositionInLine() + length); + point.setText(""); + return point; + } +} diff --git a/src/test/groovy/org/apache/groovy/parser/antlr4/SyntaxErrorTest.groovy b/src/test/groovy/org/apache/groovy/parser/antlr4/SyntaxErrorTest.groovy index bbe0175f72..06c55d7316 100644 --- a/src/test/groovy/org/apache/groovy/parser/antlr4/SyntaxErrorTest.groovy +++ b/src/test/groovy/org/apache/groovy/parser/antlr4/SyntaxErrorTest.groovy @@ -567,7 +567,7 @@ final class SyntaxErrorTest { TestUtils.doRunAndShouldFail('fail/SwitchExpression_10x.groovy') } - @NotYetImplemented @Test + @Test void 'error alternative - Missing ")" 1'() { expectParseError '''\ |println ((int 123) @@ -580,7 +580,7 @@ final class SyntaxErrorTest { |'''.stripMargin() } - @NotYetImplemented @Test + @Test void 'error alternative - Missing ")" 2'() { expectParseError '''\ |def x() { @@ -595,7 +595,7 @@ final class SyntaxErrorTest { |'''.stripMargin() } - @NotYetImplemented @Test + @Test void 'error alternative - Missing ")" 3'() { expectParseError '''\ |def m( { @@ -609,6 +609,45 @@ final class SyntaxErrorTest { |'''.stripMargin() } + @Test + void 'error alternative - Missing ")" 4'() { + expectParseError '''\ + |foo(1, 2 + |'''.stripMargin(), '''\ + |Missing ')' @ line 1, column 9. + | foo(1, 2 + | ^ + | + |1 error + |'''.stripMargin() + } + + @Test + void 'error alternative - Missing ")" 5'() { + expectParseError '''\ + |def f(int x + |'''.stripMargin(), '''\ + |Missing ')' @ line 1, column 12. + | def f(int x + | ^ + | + |1 error + |'''.stripMargin() + } + + @Test + void 'error alternative - Missing ")" 6'() { + expectParseError '''\ + |println ((int) 123 + |'''.stripMargin(), '''\ + |Missing ')' @ line 1, column 19. + | println ((int) 123 + | ^ + | + |1 error + |'''.stripMargin() + } + @NotYetImplemented @Test void 'CompilerErrorTest_001'() { unzipScriptAndShouldFail('scripts/CompilerErrorTest_001.groovy', [])
