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.
--
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]
