paulk-asert commented on PR #3216:
URL: https://github.com/apache/groovy/pull/3216#issuecomment-6679128071
Thanks Scott, the diagnosis is right and one message for every form is the
right goal. Two things before this goes in.
**Terminology.** `long` isn't a keyword. The docs separate *reserved
keywords*, *contextual keywords* and *other reserved words* (the primitive type
names plus `null`, `true` and `false`), and the parser keeps a `keywords` rule
precisely so these words still work as map keys, property names and quoted
method names (`[long: 1]`, `x.long`, `def "long"() {}`). So the message, the
method names (`keywordAsVariableName`, `isPrimitiveTypeKeyword`) and the
Javadoc should say "primitive type name" or "reserved word" rather than
"keyword". `void` is a type name but not a primitive.
**Approach.** The `AstBuilder` check in `visitAssignmentExprAlt` is good:
it's the usual "accept it, then reject it with a precise message" pattern, and
as a bonus it fixes `(long) = [1]`, which on master compiles and then dies with
a `VerifyError`. The `AbstractFriendlyErrorStrategy` half I'd rather not take.
It's 177 lines of token look-back that has to enumerate modifiers and
declarator-ending tokens, moves the caret based on where ANTLR happened to
stop, and still can't reach the same mistake in other positions: `def m(long)
{}`, `{ long -> }`, `for (long in x)`, `catch (long)`, and the `def long` at
end-of-input cases your tests list as "keeps its own diagnostic".
The same pattern as your `AstBuilder` check covers all of those if the
grammar accepts the name:
```antlr
variableDeclaratorId
: identifier
| builtInType // accepted here so that AstBuilder can name the mistake
;
with visitVariableDeclaratorId throwing the message, and
enhancedForControl/catchClause using variableDeclaratorId instead of
identifier. That one rule feeds variable declarators, def (a, long), method,
closure and lambda parameters, for-loop and catch variables, so every form gets
the same message with an exact position and ordinary recovery, and var long = 1
parses as a declaration rather than falling back to var(long = 1). I've put
that together on #3241: 3 grammar lines, 13 lines in AstBuilder, and tests
covering your cases plus the parameter/loop/catch ones. On the GROOVY-9588
concern: the new alternative is only reachable on a primitive-name token, which
no valid program has in that position; the repo's 2,733 Groovy sources parse
identically and parse timing is unchanged (7.2–7.4 s per pass on master vs
6.9–7.4 s), and the parser, gls, bugs, STC and classgen suites pass.
Two things that branch doesn't do, same as yours: def def = 1 / def class =
1 keep the generic message (extending the rule to keywords would cover them,
but I'd want to measure that separately), and int long() now reports on the (
rather than on long.
Happy to take it either way: you rebase this PR onto that approach, or I
merge the branch and we close this one against it. Either way the
visitAssignmentExprAlt check and your negative tests are what I'd keep from
here.
--
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]
