[
https://issues.apache.org/jira/browse/GROOVY-12353?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18111794#comment-18111794
]
ASF GitHub Bot commented on GROOVY-12353:
-----------------------------------------
Copilot commented on code in PR #2877:
URL: https://github.com/apache/groovy/pull/2877#discussion_r3939441663
##########
src/main/java/org/apache/groovy/parser/antlr4/GroovySyntaxError.java:
##########
@@ -19,7 +19,14 @@
package org.apache.groovy.parser.antlr4;
/**
- * Represents a syntax error of groovy program
+ * Represents a syntax error of a Groovy program, raised by the lexer or
parser.
+ * <p>
+ * The message is the diagnostic as produced by the recogniser (for example
+ * {@code Unclosed string literal} or {@code Unexpected character:
'\u005cu200b'}).
+ * Line and column are 1-based; they are the caret location, not part of the
+ * message string — {@link org.codehaus.groovy.syntax.SyntaxException} appends
+ * {@code @ line N, column M} when the error is reported through the compiler.
Review Comment:
This class-wide statement conflicts with the public
`SyntaxErrorReportable.throwSyntaxError(..., true)` path, which deliberately
appends `PositionInfo` to the `GroovySyntaxError` message. Please qualify this
as the compiler's normal lexer path rather than claiming the location can never
be part of the message.
##########
src/main/java/org/apache/groovy/parser/antlr4/internal/AbstractFriendlyErrorStrategy.java:
##########
@@ -49,24 +77,234 @@ abstract class AbstractFriendlyErrorStrategy extends
DefaultErrorStrategy {
}
/**
- * Prefer a precise "Missing …" delimiter diagnostic when the token stream
- * clearly indicates an unclosed / incomplete construct; otherwise fall
- * back to the generic message.
+ * Prefer a relocated "Missing …" closer when the token stream supports it;
+ * otherwise refine the generic {@code Unexpected input: ...} fallback
+ * (reserved keyword, sole expected punctuation, unexpected EOF).
+ * Locate/refine run in a defensive try; a single listener dispatch is
+ * outside so a listener {@link IllegalArgumentException} /
+ * {@link IndexOutOfBoundsException} is not mistaken for a lookup failure
+ * and re-dispatched.
*/
private void reportFriendlyError(final Parser recognizer, final
RecognitionException e, final String fallbackMessage) {
- MissingDelimiterDiagnostic.Hit hit = null;
+ Token at = e.getOffendingToken();
+ String message = fallbackMessage;
try {
- // Incomplete / synthetic contexts can leave token indices out of
range.
- hit =
MissingDelimiterDiagnostic.locate(recognizer.getInputStream(), e);
+ // Incomplete / synthetic contexts can leave token indices out of
range,
+ // and getExpectedTokens() can reject an invalid ATN state number.
+ MissingDelimiterDiagnostic.Hit hit =
MissingDelimiterDiagnostic.locate(recognizer.getInputStream(), e);
+ if (hit != null) {
+ at = hit.at;
+ message = hit.message;
+ } else {
+ message = refineFallbackMessage(e, fallbackMessage);
+ }
} catch (IndexOutOfBoundsException | IllegalArgumentException ignored)
{
- // Fall through to the generic message. Catch only locate()'s known
- // defensive failures — never listener-side fatals (e.g.
addFatalError).
+ message = fallbackMessage;
+ }
+ recognizer.notifyErrorListeners(at, message, e);
+ }
+
+ /**
+ * Improve a generic mismatch/NVAE sentence without moving the caret.
+ * Precedence: reserved/misplaced keyword, then a singleton expected
+ * punctuation token, then unexpected EOF, then {@code generic}.
+ */
+ static String refineFallbackMessage(final RecognitionException e, final
String generic) {
+ if (e == null) {
+ return generic;
+ }
+ Token offending = e.getOffendingToken();
+ if (offending != null) {
+ String keyword = keywordMessage(offending.getType());
+ if (keyword != null) {
+ return keyword;
+ }
+ String misplacedDefault = misplacedDefaultClause(e, offending);
+ if (misplacedDefault != null) {
+ return misplacedDefault;
+ }
+ String safeIndex = unexpectedSafeIndex(e, offending);
+ if (safeIndex != null) {
+ return safeIndex;
+ }
+ }
+ String sole = soleExpectedMessage(e);
+ if (sole != null) {
+ return sole;
+ }
+ if (offending != null && offending.getType() == Token.EOF) {
+ return "Unexpected end of input";
}
- if (hit != null) {
- recognizer.notifyErrorListeners(hit.at, hit.message, e);
- return;
+ return generic;
+ }
+
+ /**
+ * javac wording for reserved keywords Groovy tokenises but does not
+ * implement, and for control-flow keywords that appear as the offending
+ * token. Like javac, the sentence names the keyword even when a related
+ * construct is nearby but incomplete ({@code if (x) else {}} is
+ * {@code 'else' without 'if'} because the then-branch is missing).
+ * {@code const} is reserved and unused in Java (JLS 3.9); the replacements
+ * are {@code val} (locals; preferred over {@code final} in Groovy 6) and
+ * {@code static final} (class constants). {@code threadsafe} is reserved
+ * and unused in Groovy. {@code default} itself is <em>not</em> mapped
+ * here: it is a valid interface-method and annotation-element keyword, so
+ * an offending {@code default} (for example {@code def m() default {1}})
+ * is not "outside of switch". {@code default:} / {@code default ->}
+ * outside a switch is recognised via {@link #misplacedDefaultClause}.
+ */
+ static String keywordMessage(final int tokenType) {
+ return switch (tokenType) {
+ case CONST -> "'const' is not supported; use 'val' or 'static
final' instead";
+ case GOTO -> "'goto' is not supported";
+ case THREADSAFE -> "'threadsafe' is not supported";
+ case ELSE -> "'else' without 'if'";
+ case CATCH -> "'catch' without 'try'";
+ case FINALLY -> "'finally' without 'try'";
+ case CASE -> "'case' outside of switch";
+ default -> null;
+ };
+ }
+
+ /**
+ * {@code default: x} / {@code default -> x} at script or method scope:
+ * ANTLR's offending token is the {@code :} or {@code ->}, so
+ * {@link #keywordMessage(int)} on the offender is silent. Look one
+ * default-channel token back; skip {@code NL} so a line break between
+ * {@code default} and the clause marker still names {@code default}.
+ * Restricted to those two markers so {@code interface I { default }}
+ * (offender {@code }}) is not mislabelled.
+ */
+ static String misplacedDefaultClause(final RecognitionException e, final
Token offending) {
+ if (e == null || offending == null) {
+ return null;
+ }
+ int type = offending.getType();
+ if (type != COLON && type != ARROW) {
+ return null;
+ }
+ if (!(e.getInputStream() instanceof TokenStream tokens)) {
+ return null;
+ }
+ int index = offending.getTokenIndex();
+ if (index < 1) {
+ return null;
+ }
+ try {
+ for (int i = index - 1; i >= 0; i--) {
+ Token prev = tokens.get(i);
+ int prevType = prev.getType();
+ if (prevType == NL || prev.getChannel() !=
Token.DEFAULT_CHANNEL) {
+ continue;
+ }
+ return prevType == DEFAULT ? "'default' outside of switch" :
null;
+ }
+ } catch (IndexOutOfBoundsException | IllegalArgumentException ignored)
{
+ return null;
}
- notifyErrorListeners(recognizer, fallbackMessage, e);
+ return null;
+ }
+
+ /**
+ * {@code ?[} is a single token (Groovy 4 safe index) and must follow an
+ * expression. A leading {@code ?[0]} or {@code a??[0]} (ternary {@code ?}
+ * then {@code ?[}) has {@code ?[} as the offender; name it rather than
+ * dumping a generic {@code Unexpected input} span.
+ */
+ static String unexpectedSafeIndex(final RecognitionException e, final
Token offending) {
+ if (e == null || offending == null || offending.getType() !=
SAFE_INDEX) {
+ return null;
+ }
+ if (!(e.getInputStream() instanceof TokenStream tokens)) {
+ return "'?[' requires an expression before it";
+ }
+ int index = offending.getTokenIndex();
+ if (index < 1) {
+ return "'?[' requires an expression before it";
+ }
+ try {
+ for (int i = index - 1; i >= 0; i--) {
+ Token prev = tokens.get(i);
+ int prevType = prev.getType();
+ if (prevType == NL || prev.getChannel() !=
Token.DEFAULT_CHANNEL) {
+ continue;
+ }
+ // postfix-able previous token: this '?[' is in a
pairing/content
+ // failure, not a missing receiver — leave the generic
sentence.
+ if (prevType == QUESTION) {
+ return "'?[' requires an expression before it";
+ }
+ return null;
Review Comment:
This treats every preceding token other than `QUESTION` as if it could end
an expression. For example, in `foo(?[0])` or `x = ?[0]`, the previous token is
`LPAREN` or `ASSIGN`; `SAFE_INDEX` cannot start a primary (it only appears as
an `indexPropertyArgs` suffix), but this returns `null` and leaves the generic
diagnostic. Please return the missing-receiver message unless the previous
token can actually terminate an expression, and cover at least one
operator/delimiter case end to end.
> Improve remaining common syntax error messages (unclosed literals, unexpected
> characters, missing punctuation, reserved keywords)
> ---------------------------------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12353
> URL: https://issues.apache.org/jira/browse/GROOVY-12353
> Project: Groovy
> Issue Type: Improvement
> Reporter: Daniel Sun
> Priority: Major
>
> h3. Problem
> After GROOVY-12169 and GROOVY-12171, several everyday syntax mistakes still
> produce a generic *Unexpected input* or *Unexpected character* message that
> does not name the actual problem.
> Invisible or look-alike characters (zero-width space, BOM, NUL, no-break
> space, curly quotes, non-ASCII dashes) render as an empty or misleading glyph
> between quotes. Unclosed quotes and comments are reported as an unexpected
> quote or slash rather than as an unclosed literal. {{if true}} without
> parentheses, {{x ? y}}, and {{const x = 1}} look like random unexpected
> tokens instead of a missing {{(}} / {{:}} or an unimplemented keyword. Groovy
> 4 safe index is a single token that pairs with a closing bracket, but an
> unclosed index inside parentheses was reported as {{Missing ')'}}.
> h3. Examples (before)
> * Unclosed string:
> {noformat}
> println 'Hello
> {noformat}
> reports:
> {noformat}
> Unexpected character: ''' @ line 1, column 9.
> {noformat}
> (caret on the opening quote)
> * Unclosed block comment:
> {noformat}
> /* comment
> {noformat}
> reports:
> {noformat}
> Unexpected input: '/'
> {noformat}
> * Invisible character ({{def}} + U+200B + {{name = null}}):
> {noformat}
> Unexpected character: ''
> {noformat}
> * Missing punctuation:
> {noformat}
> if true { x = 1 }
> {noformat}
> reports:
> {noformat}
> Unexpected input: 'true'
> {noformat}
> * Reserved keyword:
> {noformat}
> const x = 1
> {noformat}
> reports:
> {noformat}
> Unexpected input: 'const'
> {noformat}
> * Duplicate location:
> {noformat}
> def n = 1_
> {noformat}
> reports:
> {noformat}
> Number ending with underscores is invalid @ line 1, column 10 @ line 1,
> column 10.
> {noformat}
> * Hard-coded varargs name:
> {noformat}
> def m(int... a, int b) {}
> {noformat}
> reports:
> {noformat}
> The var-arg parameter strs must be the last parameter
> {noformat}
> * Unclosed safe index inside parentheses:
> {noformat}
> (a?[0
> {noformat}
> reports:
> {noformat}
> Missing ')'
> {noformat}
> h3. Root cause
> * Lexer {{UNEXPECTED_CHAR}} inlined the raw character with only a
> quote-escape, so control, format, and look-alike characters vanish or
> mislead. An unexpected quote is almost always an unclosed string, but the
> message never said so.
> * Unclosed block comments failed the comment rule and were retokenized as
> {{/}}, so the parser saw an unexpected slash.
> * Parser fallback wording was still ANTLR's *Unexpected input*, even when the
> expected set was a single punctuation token, or the offender was a reserved
> or misplaced keyword ({{const}}, {{goto}}, {{threadsafe}}, {{else}},
> {{catch}}, {{finally}}, {{case}}) or EOF. For {{default:}} / {{default ->}}
> outside a switch, the offender is {{:}} or {{->}}, not {{default}}.
> * The missing-closer diagnostic trusted a sole expected closer. For an
> unclosed safe index inside parentheses the parser expected {{)}}, so the
> inner unclosed index was reported as {{Missing ')'}}. The safe-index token
> was not treated as the same opener family as a normal index.
> * Lexer {{require(..., true)}} appended {{@ line N, column M}} to
> {{GroovySyntaxError}}, and {{SyntaxException}} appended the same location
> again.
> * {{AstBuilder}} used a hard-coded parameter name {{strs}} in the
> varargs-not-last diagnostic.
> Grammar-level parser error alternatives are not an option: GROOVY-9588 showed
> they enlarge the ATN and slow successful parses. An EOF closer on slashy
> strings is also not an option: {{/}} after an expression with newlines is
> division, not an unclosed slashy string.
> h3. Goal
> Give javac-aligned, developer-facing sentences for these common mistakes,
> with an accurate caret, without reintroducing parser error alternatives on
> the hot path.
> h3. Approach
> Error-path-only, two layers. Successful parses never enter these helpers.
> * Lexer ({{GroovyLexer.g4}} / {{AbstractLexer}}): an unexpected quote becomes
> *Unclosed string literal*; an unclosed block comment becomes *Unclosed
> comment* at the opener (one non-greedy loop, then the closer or EOF — not a
> second lexer rule, which would win by longest match and swallow trailing
> source). Other unexpected characters go through {{getCharErrorDisplay}}
> (shared with the GString {{$}} path from GROOVY-12171). Unicode spaces other
> than U+0020, curly quotes, and non-ASCII dashes are named as a Unicode escape
> (for example {{'\u200b'}}) so they do not vanish into the caret line. Stop
> attaching position text on lexer {{require}} calls so {{SyntaxException}} is
> the only source of {{@ line N, column M}}.
> * Parser ({{AbstractFriendlyErrorStrategy}}): {{MissingDelimiterDiagnostic}}
> still relocates the caret for a missing closer (GROOVY-12169). Safe index is
> the same opener family as a normal index; if the innermost opener is not the
> family of the sole expected closer, defer so an unclosed index inside
> parentheses reports *Missing ']'*. Everything else only refines the fallback
> sentence and keeps ANTLR's offending token: reserved/misplaced keyword, then
> {{default:}} / {{default ->}} lookback (skip newlines; do not label an
> incomplete interface {{default}} method as outside switch), then a leading
> safe-index token without a receiver, then a singleton expected punctuation
> token ({{Missing '('}}, {{Missing ':'}}, {{Missing '>'}}, ...), then
> *Unexpected end of input* for EOF. Locate/refine stay in a defensive try;
> listener dispatch is a single call afterwards.
> {{AstBuilder}} reports the actual varargs parameter name.
> h3. Expected result (after)
> * unclosed single-, double-, or triple-quoted string becomes *Unclosed string
> literal*
> * unclosed block comment becomes *Unclosed comment* (caret on the opener)
> * zero-width space, no-break space, curly quote, em dash become {{Unexpected
> character: '\u200b'}} (and the matching escape)
> * {{if true}} / {{while true}} / {{for int i in ...}} without parentheses
> becomes {{Missing '('}}
> * {{x ? y}} becomes {{Missing ':'}}
> * a generic type missing {{>}} before {{(}} becomes {{Missing '>'}}
> * {{const x = 1}} becomes {{'const' is not supported; use 'val' or 'static
> final' instead}}
> * {{goto label}} becomes {{'goto' is not supported}}
> * {{threadsafe}} as a modifier becomes {{'threadsafe' is not supported}}
> * stray {{else}} / {{catch}} / {{case}} become {{'else' without 'if'}} /
> {{'catch' without 'try'}} / {{'case' outside of switch}}
> * {{default: x}} / {{default -> x}} at script scope becomes {{'default'
> outside of switch}}; an incomplete interface {{default}} method is not that
> message
> * unclosed or mismatched safe index, including inside parentheses, becomes
> *Missing ']'*
> * a safe index with no receiver, or {{?}} immediately before one, becomes:
> {noformat}
> '?[' requires an expression before it
> {noformat}
> * a space between the question mark and the opening bracket is an incomplete
> ternary ({{Missing ':'}}); safe index is one token:
> {noformat}
> a? [0]
> {noformat}
> * {{throw}} at EOF becomes *Unexpected end of input*
> * {{def n = 1_}} becomes *Number ending with underscores is invalid*
> (position once)
> * {{def m(int... a, int b)}} becomes {{The var-arg parameter a must be the
> last parameter}}
> Valid programs are unchanged.
> h3. Related
> GROOVY-12169, GROOVY-12171, GROOVY-10146
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
