[
https://issues.apache.org/jira/browse/GROOVY-12695?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18139421#comment-18139421
]
ASF GitHub Bot commented on GROOVY-12695:
-----------------------------------------
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.
> Primitive type keyword used as a variable name gets inconsistent and
> misleading diagnostics
> -------------------------------------------------------------------------------------------
>
> Key: GROOVY-12695
> URL: https://issues.apache.org/jira/browse/GROOVY-12695
> Project: Groovy
> Issue Type: Bug
> Components: parser
> Affects Versions: 6.0.0
> Reporter: Scott Murphy Heiberg
> Priority: Major
>
> *Environment:* macOS 26.5, BellSoft Liberica JDK 21.0.12; reproduced with the
> {{groovy}} command of each distribution
> h3. Problem
> Declaring a local variable named after a primitive type keyword is rejected
> (as it should be), but the message depends on which declaration keyword was
> used, and neither message says what is wrong.
> {code}
> var long = 1
> val long = 1
> def long = 1
> final long = 1
> {code}
> h3. Actual
> {{var}} and {{val}} report a resolve-phase message that talks about assigning
> to a class, with the caret on the keyword:
> {noformat}
> var_long.groovy: 1: you tried to assign a value to the class 'long'
> @ line 1, column 5.
> var long = 1
> ^
> {noformat}
> {noformat}
> val_long.groovy: 1: you tried to assign a value to the class 'long'
> @ line 1, column 5.
> val long = 1
> ^
> {noformat}
> {{def}} and {{final}} report a generic syntax error with the caret on the
> {{=}}, one token past the mistake:
> {noformat}
> def_long.groovy: 1: Unexpected input: 'def long =' @ line 1, column 10.
> def long = 1
> ^
> {noformat}
> {noformat}
> final_long.groovy: 1: Unexpected input: 'final long =' @ line 1, column 12.
> final long = 1
> ^
> {noformat}
> Other primitive names and {{void}} behave the same way ({{var int = 1}}
> reports {{you tried to assign a value to the class 'int'}}, {{def int = 1}}
> reports {{Unexpected input: 'def int ='}}). Inside a method body {{def long =
> 1}} reports {{Unexpected input: '='}} on the {{=}}. The same two shapes also
> appear for a typed declaration ({{String long = "x"}} and {{int long = 1}}
> report the class message, a field {{class C { int long = 1 }}} reports
> {{Unexpected input: 'int long'}}) and for a bare {{long = 1}}.
> h3. Cause
> {{var}} and {{val}} are soft keywords, so when {{var long = 1}} fails to
> parse as a declaration the parser accepts it as the command expression
> {{var(long = 1)}}; {{AstBuilder}} builds an assignment whose left-hand side
> is the built-in type name, and {{ResolveVisitor}} later reports it as an
> assignment to a class. ({{var long}} without an initializer actually runs, as
> a call to a method named {{var}}.) {{def}} and {{final}} are reserved, so the
> parser commits to a declaration, consumes {{long}} as the declared type and
> fails on the {{=}} with the generic {{Unexpected input}} sentence.
> h3. Expected
> One diagnostic for every form, naming the keyword, with the caret on the
> keyword:
> {noformat}
> 'long' is a keyword and cannot be used as a variable name @ line 1, column 5.
> var long = 1
> ^
> {noformat}
> This is in line with the other developer-facing parser sentences added in
> GROOVY-12353 and GROOVY-12401.
> A pull request with regression tests follows.
> h3. Versions
> Reproduced on 6.0.0, 6.0.1-SNAPSHOT (GROOVY_6_0_X at 7c0dcf17dd) and master
> (d86ea7eb85).
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
