[
https://issues.apache.org/jira/browse/GROOVY-12425?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18125064#comment-18125064
]
ASF GitHub Bot commented on GROOVY-12425:
-----------------------------------------
Copilot commented on code in PR #2944:
URL: https://github.com/apache/groovy/pull/2944#discussion_r4220037277
##########
src/antlr/GroovyLexer.g4:
##########
@@ -289,13 +304,24 @@ options {
// §3.10.5 String Literals
+// A quote that never closes is UNEXPECTED_CHAR: nothing else matches it.
+// `/` is also DIV, so an unclosed `/.../` is an EOF alternative of this rule.
+// isUnclosedSlashy() is the first edge of that alternative, before any body
+// character is read. `*` is safe because the follower is EOF, not `/`:
+// `//` is not an empty slashy. EOF stays here so a second rule cannot
+// swallow source after a real closer.
Review Comment:
This comment reads as if unclosed quoted strings are handled purely as
`UNEXPECTED_CHAR`, while the PR adds/extends tests expecting `Unclosed string
literal` messaging. If unclosed quotes are intentionally diagnosed via
`UNEXPECTED_CHAR` (but mapped to `Unclosed string literal` elsewhere), consider
clarifying that mapping here to avoid misleading future maintainers.
##########
src/antlr/GroovyLexer.g4:
##########
@@ -48,6 +48,8 @@ options {
private boolean errorIgnored;
private long tokenIndex;
private int lastTokenType;
+ /** Last default-channel token whose type is not {@code NL}. */
+ private int lastRealTokenType;
Review Comment:
`lastRealTokenType` is used to make lexing decisions but is not initialized
here. That makes behavior depend on the Java default (0 / `Token.INVALID_TYPE`)
or any prior state if the lexer instance is reused. Initializing (and resetting
alongside `lastTokenType`) to a well-defined value like `Token.EOF` would make
`isUnclosedSlashy()` deterministic in leading-newline / fresh-input scenarios.
##########
src/antlr/GroovyLexer.g4:
##########
@@ -109,6 +114,16 @@ options {
return !REGEX_CHECK_SET.get(this.lastTokenType);
}
+ /**
+ * A {@code /} that reaches EOF is an unclosed slashy string only when the
+ * previous real token (ignoring {@code NL}) could not be the left operand
+ * of division. {@code 9 \n / \n 3} stays division; {@code s = /hello}
does not.
+ */
+ private boolean isUnclosedSlashy() {
+ int decision = this.lastTokenType == NL ? this.lastRealTokenType :
this.lastTokenType;
+ return !REGEX_CHECK_SET.get(decision);
+ }
Review Comment:
`lastRealTokenType` is used to make lexing decisions but is not initialized
here. That makes behavior depend on the Java default (0 / `Token.INVALID_TYPE`)
or any prior state if the lexer instance is reused. Initializing (and resetting
alongside `lastTokenType`) to a well-defined value like `Token.EOF` would make
`isUnclosedSlashy()` deterministic in leading-newline / fresh-input scenarios.
##########
src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java:
##########
@@ -2594,11 +2594,13 @@ public Expression visitCommandExpression(final
CommandExpressionContext ctx) {
&& baseExpr instanceof BinaryExpression bin
&& !"[".equals(bin.getOperation().getText())) {
// `List<Integer name` is parsed as the comparison `List <
Integer` plus a
- // command argument. javac: `'>' expected`.
+ // following argument list. javac: `'>' expected`. Unmatched `>`
in the
+ // error strategy keeps ANTLR's offender; point at that list, not
at `List`.
+ // An identifier after `T < U` is argumentList, not
commandArgument.
if ("<".equals(bin.getOperation().getText())
&& looksLikeTypeName(bin.getLeftExpression())
&& looksLikeTypeName(bin.getRightExpression())) {
- throw createParsingFailedException("Missing '>'", bin);
+ throw createParsingFailedException("Missing '>'",
ctx.enhancedArgumentListInPar());
Review Comment:
`ctx.enhancedArgumentListInPar()` may be null for some command-expression
shapes, which would risk an NPE (or a mis-positioned error) inside
`createParsingFailedException`. Consider guarding and falling back to a
non-null anchor (e.g., `ctx.expression()` or `bin`) when
`enhancedArgumentListInPar()` is absent, while still preferring the
argument-list node when it exists.
> Refine error message for unclosed type argument and unclosed slashy string
> --------------------------------------------------------------------------
>
> Key: GROOVY-12425
> URL: https://issues.apache.org/jira/browse/GROOVY-12425
> Project: Groovy
> Issue Type: Improvement
> Reporter: Daniel Sun
> Priority: Major
>
> h2. Description
> Two parser diagnostics name a missing closer but point at the wrong token, or
> name a different mistake. Both should match the caret already used for an
> unclosed quote: one sentence, on the token that is wrong.
> h3. Unclosed type argument
> {{List<Integer name}} is parsed as the comparison {{List < Integer}} plus a
> following argument list. The message was already {{Missing '>'}}, which is
> the same advice as javac's {{> or ',' expected}}. The caret sat on {{List}},
> often many tokens away from the missing {{>}}.
> The caret now sits on the first token of that argument list. An identifier
> after {{T < U}} is an argument list, not a command argument, so the caret is
> the start of the list. Extra spaces do not pull it back onto the space after
> the type argument.
> * {{List<Integer list2}} — caret on {{list2}}
> * {{List<List<Integer> list2}} — caret on {{list2}}, after the inner {{>}}
> * {{List<Integer name}} — two spaces; caret on {{name}}, not on the first
> space
> * {{List < Integer 1, 2}} — caret on {{1}}
> * {{List < Integer name foo}} — caret on {{name}}, not on {{foo}}
> A comparison that is not two type names stays {{Unexpected input}}. {{list <
> Integer name}} is one of those.
> {noformat}
> Missing '>' @ line 1, column 14.
> List<Integer list2 = new ArrayList<Integer>()
> ^
> {noformat}
> h3. Unclosed slashy string
> An unclosed {{/.../}} was reported as {{Unexpected input: '/'}}. {{/}} is
> also the division operator, so a slashy string that never closes fell through
> to that token.
> It is now {{Unclosed string literal}}, with the caret on the opening {{/}}.
> That is the same sentence and the same opener caret as an unclosed {{'}} or
> {{"}}, and the same place javac marks {{unclosed string literal}}.
> {noformat}
> Unclosed string literal @ line 1, column 5.
> s = /hello
> ^
> {noformat}
> The same message is used when the slashy string is only {{/}} at end of
> input, and when the opener is on the line after {{=}}.
> Division is unchanged:
> * {{a / b}} stays division.
> * A value, a line break, then {{/}}, then another value stays division. A
> newline is not a left operand. {{9}} split across lines from {{3}} by {{/}}
> still divides.
> * A closed {{/ab/}} is still a string. {{//}} and {{/*}} are still comments.
> {{$/}} is not treated as an unclosed string. {{$}} is an identifier, so
> {{($/2)}}, {{$/=}}, and {{$//}} are division. An end-of-input alternative on
> {{$/}} would reject those programs. An unclosed {{$/...}} therefore still
> surfaces as a later error.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
