paulk-asert commented on PR #2924:
URL: https://github.com/apache/groovy/pull/2924#issuecomment-5662336534
AI read:
> I went through this in some detail, including building the branch merged
with current master. Summary: good work, and the parts that looked risky turn
out to be safe. There is one defect I think should be fixed before merge.
>
> ## The blocking one: `unmatchedTypeArgument` misfires on ordinary
comparisons
>
> The helper decides "missing `>`" from the *absence* of a closing `>` in a
token window. A comparison never has one, so any comparison whose left operand
starts with a capital letter gets rewritten when an unrelated syntax error
follows:
>
> | Source | master | this PR |
> |---|---|---|
> | `int MAX = 10`<br>`if (MAX < x) { println 1 +` | `Unexpected input: '{
println 1 +\n}'` | **`Missing '>'`** |
> | `if (Integer.MAX_VALUE < x) { println 1 +` | `Unexpected input: '{
println 1 +\n}'` | **`Missing '>'`** |
> | `def X = 1`<br>`def y = X << 2 +` | `Unexpected input: '+'` | **`Missing
'>'`** |
> | `int max = 10`<br>`if (max < x) { println 1 +` | `Unexpected input: ...`
| `Unexpected '}'` — declines correctly |
>
> Uppercase-initial receivers are common in exactly this position: constants
(`MAX`, `LIMIT`, `SIZE`) and class references. The old message was unhelpful,
but the new one is confidently wrong and points the reader at generics that are
not there, which I think is a step back.
>
> Two things make it harder to reason about than it first appears:
>
> - **Distance does not save it.** With six statements between the
comparison and the error, the message still changes to `Missing '>'`.
> - **An unrelated later token cancels it.** Adding `&& 4 > 5` to the
condition closes the imagined type argument and the message reverts. So for the
same class of mistake the advice varies for reasons the author cannot see.
>
> ### The `<<` case is an off-by-one in the guard written for it
>
> ```java
> if (type == LT) {
> // `<<` is two LT tokens (shift), not nested type arguments.
> if (prev != null && prev.getType() == LT) {
> prev = t;
> continue; // the second '<' declines ...
> }
> if (prev != null && prev.getType() == CapitalizedIdentifier) {
> depth++; // ... but the first already
counted
> typeOpener = true;
> }
> ```
>
> By the time the second `<` is seen and skipped, the first has already
raised `depth` and set `typeOpener`, so the method still returns `Missing '>'`.
The comment describes the intended behaviour; the code does not implement it.
Deciding on lookahead — if the next default-channel token is also `LT`, treat
the pair as a shift and skip both — would match the comment. `>>` is unaffected
because `depth` is zero there.
>
> ### Suggested narrowing
>
> Require positive evidence of a type-argument list rather than mere absence
of `>`: for example that the token after the opening `<` is itself a type name
and the construct sits in a declaration position, or that the offending token
is the identifier immediately following the would-be type argument (the
`List<Integer name` shape that motivated the helper). The genuine cases in the
PR's own tests all have that shape; the false positives do not. Worth adding
the three inputs above to `ErrorStrategyTest`.
>
> The separate `AstBuilder` path is sound, for what it's worth — it fires
only when *both* operands look like type names (`A < B foo`), and only inside a
branch that was already going to throw.
>
> ## What I checked that came out clean
>
> **No new rejections.** The diff adds 23 `createParsingFailedException`
calls. Twenty-one replace an existing call one for one. The two genuine
additions are both guarded by a condition that already threw:
>
> - `Anonymous classes cannot declare constructors` replaces the more
general `Invalid method declaration: Foo` in the same branch.
> - `Missing '>'` in the command-expression path precedes that branch's
existing `Unexpected input: ...` throw.
>
> So no source that compiled before this PR fails after it. That was my main
concern with a change of this shape.
>
> **The position change is strictly additive.** `asBoolean(stop)` →
`hasEndPosition(node)` is the real fix here, and a good one: Groovy truth on an
`ASTNode` is just non-null, so a synthetic `ConstantExpression` with −1
coordinates was passing the test and writing −1 over an authored parent's end.
I dumped every position from 483 files under both builds and diffed:
>
> | Outcome | Nodes |
> |---|---|
> | −1 replaced by a real position | 871 |
> | Real position replaced by −1 | 0 |
> | Real position moved | 0 |
> | Unchanged | 256,384 |
>
> The gains are almost all argument lists for empty parentheses, plus the
for-control. Confirmed directly for the two shapes the JIRA names:
>
> ```
> for (int i = 0; i < 3; i++) { }
> for-control master: -1,-1 to -1,-1 PR: 4,19 to 4,40
>
> new StringBuilder()
> arguments master: -1,-1 to -1,-1 PR: 5,30 to 5,32
> ```
>
> One consequence worth stating: `SecureASTCustomizer` tells authored nodes
from generated ones by `lineNumber > 0`, so nodes that gain a position become
visible to it. The direction is toward checking more rather than less, and its
60 tests pass on the merge, but that is the mechanism by which a position
change can reach behaviour rather than only tooling.
>
> **Merged with current master it is green.** The base is 20 commits behind,
and master has since changed switch handling in the same `AstBuilder`, so the
combination is worth exercising. It merges cleanly; 905 tests pass across the
parser, generics, switch, the four updated bug tests and `SecureASTCustomizer`;
`lintGate` passes. Please rebase so CI sees that combination too.
>
> **The new diagnostics land as claimed.** Of the twelve shapes the JIRA
lists, nine improve and the other three are valid Groovy that still parses:
>
> | Input | master | this PR |
> |---|---|---|
> | `def a = new int[]` | `Missing '{'` | `Array dimension missing; specify
a size or add an initializer '{}'` |
> | `def a = new int[3]{1,2}` | `Unexpected input: '{'` | `Cannot combine an
array size with an array initializer` |
> | `List<Integer name = null` | `Unexpected input: 'List<Integer'` |
`Missing '>'` |
> | `def m() { import java.util.List; 1 }` | `Unexpected input: 'import'` |
`'import' is only allowed at the beginning of a compilation unit` |
> | `do { println 1 } (true)` | `Unexpected end of input` | `Missing
'while'` |
> | `do println 1; println 2 while (false)` | `Unexpected input: '1;'` |
`do-while body must be a single statement; wrap multiple statements in '{ }'` |
> | `interface I { def m() default {1} }` | `Unexpected input: 'default'` |
`'default' cannot follow a method header; put 'default' before the method name`
|
> | `def c = { -> def say(String m) { } }()` | `Unexpected input: '('` |
`Method definition not expected here` |
> | `def m() { 1 } }` and the `)`, `]`, `,` variants | `Unexpected input:
'}'` | `Unexpected '}'` |
>
> Moving the duplicate-`default` caret from the first branch's body to the
offending `default` keyword is the right target.
>
> ## Two smaller points
>
> **The hedged generics assertions.** Five cases in `GenericsUsageTest`
moved to `shouldFailCompilationWithAnyMessage`, which passes if *either* the
new or the old message appears. Run repeatedly against this branch, four of the
five produce the new `Missing '>'` every time and the fifth (the `for
(Iterator<String i = ...)` case) produces the old message every time — nothing
I could observe varies. An assertion that accepts either will not notice if the
improvement silently stops working. If the hedge is there because the message
is known to differ under some configuration, could you note that condition in a
comment? Otherwise the four deterministic cases could assert the exact message
and the fifth assert the old one.
>
> **The rewordings are broad and user-visible.** About fifteen long-standing
messages change text. The new wording is better and closer to javac in each
case, and nothing in the tree still refers to the old text (I checked). The
exposure is external: build tools, IDE plugins and user test suites that match
on message substrings. `Numbers.java` is a runtime class, so its
`NumberFormatException` text can surface outside compilation. Normal for a
major release and no change needed in the PR, but it should go in the release
notes rather than being found by downstream projects.
>
> ## Recommendation
>
> Merge after narrowing `unmatchedTypeArgument`, plus a rebase. Everything
else is a genuine improvement, and the test coverage the PR adds is substantial
— `ErrorStrategyTest` goes from 46 to 118 cases, and
`PositionConfigureUtilsTest` from 2 to 21, including the null,
`EmptyExpression.INSTANCE`, synthetic-constant and zero-last-line paths.
--
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]
