[
https://issues.apache.org/jira/browse/GROOVY-12425?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18128648#comment-18128648
]
ASF GitHub Bot commented on GROOVY-12425:
-----------------------------------------
paulk-asert commented on PR #2944:
URL: https://github.com/apache/groovy/pull/2944#issuecomment-6262890990
This looks good with some minor suggested improvements:
> **Verdict: Careful, well-tested, and the reasoning is documented in the
code itself rather than in a report. One genuine false-positive risk in the
lexer that I'd want closed before merge, plus one scope gap and one bit of
unrelated scope creep.**
>
> ## What it changes
>
> Two unrelated diagnostics, plus a drive-by:
>
> 1. **Unclosed slashy string.** `s = /hello` reported `Unexpected input:
'/'` because `/` is also `DIV`, so an unclosed `/.../` fell through to the
division token. A new EOF alternative in the `StringLiteral` lexer rule now
yields `Unclosed string literal` with the caret on the opening `/`.
> 2. **Unclosed type argument.** `List<Integer name` already said `Missing
'>'`, but the caret sat on `List` — often many tokens from the actual problem.
It now points at the first token of the argument list.
> 3. `Objects.requireNonNull` in `transformRecordHeaderToProperties` —
unrelated to this JIRA.
>
> ## The lexer change — what I verified
>
> This is the part that could break working code, so I traced it against the
grammar on master.
>
> ```
> | Slash { this.isRegexAllowed() && _input.LA(1) != '*' }?
> ( SlashyStringCharacter+ Slash
> | { this.isUnclosedSlashy() }? SlashyStringCharacter* EOF {
requireUnclosedString(errorIgnored); }
> )
> ```
>
> - **`a / b` is untouched.** The outer predicate `isRegexAllowed()` already
rejects the whole slashy alternative when the preceding token is a division
operand (`Identifier` is in `REGEX_CHECK_SET`), so the new alternative is never
even reached.
> - **`9 \n / \n 3` stays division — and this is the subtle part the PR gets
right.** `NL` is *not* in `REGEX_CHECK_SET`, so `isRegexAllowed()` returns true
after a newline and the slashy alternative *is* attempted. Without more, that
input would become a bogus "unclosed string literal". That's exactly why
`lastRealTokenType` exists: `isUnclosedSlashy()` looks *through* `NL` to the
last real token (`9`, an `IntegerLiteral`, which is an operand) and rejects the
alternative. The deliberate asymmetry — `isRegexAllowed()` keeps using
`lastTokenType` so a *closed* regex can still start a line, while
`isUnclosedSlashy()` is stricter — is correct and is the right way round.
> - **`//` and `/*` can't match.** `/` is not a `SlashyStringCharacter`
(only `\/` via `SlashEscape`), so an empty body followed by a second `/` fails
the `EOF` requirement; `/*` is excluded by the existing `LA(1) != '*'` guard.
> - **A real closer is never swallowed.** Since the body excludes `/`,
alternative 1 always wins when a closing `/` exists on the same token.
> - **Slashy GStrings are not broken.** `SlashyStringCharacter` admits `$`
only when *not* followed by a Java letter, so the new alternative cannot
consume `/hello $name` and `SlashyGStringBegin` still wins.
> - **`Token.EOF` is guarded twice** — in `emit()` and in
`isDivisionOperand()` — because `BitSet.get(-1)` throws. `Token.INVALID_TYPE`
is 0, identical to the old implicit default, so making it explicit changes
nothing.
> - **The `reset()` override is a latent bug fix.** `Lexer.setInputStream`
calls `reset()`, and previously a reused lexer carried `lastTokenType` across
inputs, so a leading `/` in a second input was classified using the *first*
input's last token. There's a test for it
(`reusedLexerForgetsThePreviousDivisionOperand`). Good catch, and worth calling
out separately in the JIRA.
> - **Predicate placement is correct.** `isUnclosedSlashy()` is on the left
edge of the alternative, which is where ANTLR can reliably evaluate a lexer
predicate during ATN simulation.
>
> ## The one real risk
>
> **`REGEX_CHECK_SET` is a permissive heuristic, and this change promotes it
into the justification for a hard error.**
>
> The set contains: `DEC, INC, THIS, RBRACE, RBRACK, RPAREN, GStringEnd,
NullLiteral, StringLiteral, BooleanLiteral, IntegerLiteral,
FloatingPointLiteral, Identifier, CapitalizedIdentifier`.
>
> It was built to answer "could a regex plausibly start here?" — and a gap
in it previously meant only a mislex requiring *two* slashes. The new EOF
alternative uses the complement of that same set to assert "this definitely
cannot be division, so emit an error." Any value-yielding token missing from
the set now turns **valid code into a compile error**.
>
> Notably absent are the keyword tokens that can terminate a value
expression via property access. `GroovyParser.g4:1305+` lists `CLASS`,
`DEFAULT`, `IN`, `TRAIT`, `RECORD`, `VAR`, `YIELD` and others as usable name
parts, and the lexer still emits them as keyword tokens. So:
>
> ```groovy
> def n = size.class / 2 // last '/' in the file
> ```
>
> `lastTokenType` is `CLASS`, which is not in the set, so `isRegexAllowed()`
passes, `isUnclosedSlashy()` passes, and the EOF alternative matches `/ 2<EOF>`
→ `Unclosed string literal`. Today that same input lexes `/` as `DIV` and
compiles.
>
> Three things bound the exposure, and they bound it a lot:
> 1. The `/` must be the **last `/` in the file** (the body excludes `/`, so
EOF must arrive first).
> 2. The preceding token must be outside `REGEX_CHECK_SET`.
> 3. It must be genuine division.
>
> So this is narrow and my examples are contrived — dividing a `Class` isn't
meaningful. But the failure mode is the worst kind for a diagnostics PR:
previously-compiling code now fails to compile, and the error message actively
misdirects. I derived this from reading the grammar rather than running it, so
the right next step is a test, not a redesign.
>
> The fix is cheap and strictly improves things: add the value-terminating
keyword tokens to `REGEX_CHECK_SET`. That also fixes the *pre-existing*
two-slash misparse (`a.class /2/ b` is already mislexed as a regex on master).
Alternatively, gate the EOF alternative on a stricter positive whitelist rather
than the complement of a permissive set.
>
> ## Scope gap: slashy GStrings
>
> An unclosed *interpolated* slashy — `s = /hello $name` — is not covered.
`SlashyGStringBegin` matches `/hello $`, enters `SLASHY_GSTRING_MODE`, never
finds a closer, and falls back to the old error. The user-facing mistake is
identical to the plain case, so the message will now be inconsistent between
`/hello` and `/hello $name`.
>
> The JIRA explicitly documents and justifies excluding `$/...$/` (an EOF
alternative there would reject `($/2)`, `$/=`, `$//`) — that reasoning is
sound. But it doesn't mention the slashy-GString case, and there's no test
pinning the behaviour either way. I'd want it documented as a known limitation
with a test asserting whatever it currently does, so the gap is explicit rather
than accidental.
>
> ## Scope creep
>
> ```java
> Parameter[] parameters =
Objects.requireNonNull(this.visitFormalParameters(ctx.formalParameters()));
> ```
>
> This is in `transformRecordHeaderToProperties` — records, nothing to do
with error messages — and the comment concedes the value can never be null
(`visitFormalParameters` returns `Parameter.EMPTY_ARRAY`). It exists to silence
`javabugs:S2259`. Adding a runtime check that provably cannot fire, to appease
a static analyser, in an unrelated PR, is three small negatives at once. I'd
drop it or split it into its own commit.
>
> ## Consumer-visible behaviour changes
>
> Both are expected for this kind of change, but worth being deliberate
about:
>
> - **The `Missing '>'` caret moves** (column 1 → column 16 for `List <
Integer name`). Anything asserting on full error output — IDE integration
tests, tooling that parses compiler output — will see a diff. The existing
tests were updated accordingly.
> - **In `errorIgnored` mode the token stream changes.** An unclosed `/...`
now lexes as a single `StringLiteral` instead of `DIV` plus identifiers. That's
better for syntax highlighting, but Groovy-Eclipse consumes this lexer, so it's
a real downstream change. There is a test
(`errorIgnoredUnclosedSlashyTokenizesWithoutThrowing`) confirming tokenisation
completes and reaches EOF.
>
> ## Test quality — the strongest part
>
> The tests map one-to-one onto the risks, which is unusual and good.
Specifically covered: `a / b` stays division; `$/ab/$` still works; `//` is not
an empty slashy; a closed slashy after a newline is still a string; `9 \n / \n
3` stays division (the `lastRealTokenType` path); a leading newline *without*
an operand *is* unclosed; a reused lexer forgets the previous operand (the
`reset()` path); `errorIgnored` doesn't throw; and caret positions at
start-of-file, mid-file, after a line break, and at bare `/`.
>
> Gaps: no symmetric "valid division survives" case for a token *outside*
`REGEX_CHECK_SET` (the risk above), and nothing pinning slashy-GString
behaviour.
>
> ## Recommendation
>
> Merge once the `REGEX_CHECK_SET` gap is closed — add the value-terminating
keyword tokens plus a test asserting `x.class / 2` at end-of-file still
compiles. Add a test pinning unclosed-slashy-GString behaviour and note it as a
limitation in the JIRA. Drop or split the `Objects.requireNonNull` change. The
lexer reasoning and the `reset()` fix are both good work and the latter
deserves its own mention in the JIRA rather than riding along silently.
> 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)
