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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
