GROOVY-12169: Improve syntax error messages and caret positions for missing ')', ']', and '}'

[daniellansun]

https://issues.apache.org/jira/browse/GROOVY-12169

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.

Benchmark suite	Current: 1f18b8a	Previous: 2d57704	Ratio
org.apache.groovy.bench.GeneratedHashCodeBench.generated_hashcode_on_instance_with_null_properties	205941.4328302444 ops/ms	131692.1645815392 ops/ms	1.56
org.apache.groovy.bench.NonCapturingLambdaBench.capturingLambdaApply	114645.36234527593 ops/ms	74918.8989046163 ops/ms	1.53
org.apache.groovy.bench.NonCapturingLambdaBench.streamMapNonCapturing	26926.346940213174 ops/ms	16198.983030455269 ops/ms	1.66
org.apache.groovy.bench.StaticMethodCallIndyBench.instanceChain_groovy	184.39943806614167 ops/ms	113.37893198200717 ops/ms	1.63
org.apache.groovy.bench.StaticMethodCallIndyBench.instanceFib_groovy	2.0494772002017547 ops/ms	1.2369449950715008 ops/ms	1.66
org.apache.groovy.bench.StaticMethodCallIndyBench.instanceSum_groovy	246.6410632977486 ops/ms	144.6242693431618 ops/ms	1.71
org.apache.groovy.bench.StaticMethodCallIndyBench.staticChain_groovy	1105.3171503771462 ops/ms	720.8843282126909 ops/ms	1.53
org.apache.groovy.bench.StaticMethodCallIndyBench.staticFib_groovy	4.774938319086619 ops/ms	2.6040608472851217 ops/ms	1.83
org.apache.groovy.bench.StaticMethodCallIndyBench.staticSum_groovy	1415.173333355066 ops/ms	723.089090154398 ops/ms	1.96
org.apache.groovy.bench.dispatch.CallsiteBench.dispatch_1_monomorphic_groovy	7352.034281553356 ops/ms	4570.034697549628 ops/ms	1.61
org.apache.groovy.bench.dispatch.CallsiteBench.dispatch_1_monomorphic_groovyColdReflect	7356.6867454913545 ops/ms	4567.336211382841 ops/ms	1.61
org.apache.groovy.bench.dispatch.CallsiteBench.dispatch_1_monomorphic_java	73394.11449588262 ops/ms	45902.69843927495 ops/ms	1.60
org.apache.groovy.bench.dispatch.CallsiteBench.dispatch_3_polymorphic_java	8514.09000642501 ops/ms	2737.3048365888867 ops/ms	3.11
org.apache.groovy.bench.dispatch.CallsiteBench.dispatch_8_megamorphic_groovyCS	25926.473707471458 ops/ms	16983.073673062565 ops/ms	1.53
org.apache.groovy.bench.dispatch.CallsiteBench.dispatch_8_megamorphic_java	8638.987861365998 ops/ms	1885.9895489711587 ops/ms	4.58

This comment was automatically generated by workflow using github-action-benchmark.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 94.17040% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.1956%. Comparing base (b9a3822) to head (1f18b8a).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...er/antlr4/internal/MissingDelimiterDiagnostic.java	94.3925%	0 Missing and 12 partials ⚠️
...rser/antlr4/internal/DescriptiveErrorStrategy.java	88.8889%	1 Missing ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##               master      #2718        +/-   ##
==================================================
+ Coverage     69.1438%   69.1956%   +0.0518%     
- Complexity      34327      34454       +127     
==================================================
  Files            1538       1539         +1     
  Lines          129611     129832       +221     
  Branches        23580      23650        +70     
==================================================
+ Hits            89618      89838       +220     
+ Misses          31950      31944         -6     
- Partials         8043       8050         +7     

Files with missing lines	Coverage Δ
...rser/antlr4/internal/DescriptiveErrorStrategy.java	62.1622% <88.8889%> (+8.8288%)	⬆️
...er/antlr4/internal/MissingDelimiterDiagnostic.java	94.3925% <94.3925%> (ø)

... and 9 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

JMH summary — classic (commit e6c995b)

Speedup vs trailing 90-day baseline on gh-pages. Higher = faster.
1.00 = in line with history. Per-benchmark ratio, geomean within group.
Time-per-op units inverted so direction is consistent. The calibrated
column divides out this runner's speed vs the baseline hardware, as
measured by Groovy-independent pure-Java ruler benchmarks.

Group	Speedup	Calibrated	n
bench	1.235 ×	0.989 ×	84
core	1.071 ×	1.102 ×	77
grails	0.993 ×	0.945 ×	80

⚠️ Runner speed differs ≥15% from the historical baseline hardware for: bench. Raw speedups are not meaningful for those parts — use the calibrated column.

_{Runner calibration (this run vs baseline hardware): bench 1.25× (26 rulers) · core-ag 0.94× (3 rulers) · core-hz 1.01× (3 rulers) · grails-ad 1.11× (3 rulers) · grails-ez 1.00× (3 rulers)}

_{Baseline: dev/bench/jmh/<part>/classic/data.js on gh-pages, trailing 90 days. Daily dashboard · Per-suite raw data}

[github-actions]

JMH summary — indy (commit e6c995b)

Speedup vs trailing 90-day baseline on gh-pages. Higher = faster.
1.00 = in line with history. Per-benchmark ratio, geomean within group.
Time-per-op units inverted so direction is consistent. The calibrated
column divides out this runner's speed vs the baseline hardware, as
measured by Groovy-independent pure-Java ruler benchmarks.

Group	Speedup	Calibrated	n
bench	1.784 ×	0.902 ×	84
core	3.670 ×	3.487 ×	77
grails	2.229 ×	2.428 ×	80

⚠️ Runner speed differs ≥15% from the historical baseline hardware for: bench. Raw speedups are not meaningful for those parts — use the calibrated column.

_{Runner calibration (this run vs baseline hardware): bench 1.90× (26 rulers) · core-ag 0.98× (3 rulers) · core-hz 1.14× (3 rulers) · grails-ad 0.95× (3 rulers) · grails-ez 0.89× (3 rulers)}

_{Baseline: dev/bench/jmh/<part>/indy/data.js on gh-pages, trailing 90 days. Daily dashboard · Per-suite raw data}

[Copilot]

Pull request overview

Improves Groovy’s ANTLR4 parse error reporting for missing closing delimiters (), ], }) by adding a post-failure token-stream diagnostic that computes a more accurate “Missing …” message and caret position, and wiring it into the existing DescriptiveErrorStrategy.

Changes:

• Add MissingDelimiterDiagnostic to detect likely missing closers using expected-token hints, cast-pattern heuristics, and a delimiter stack scan.
• Integrate the diagnostic into DescriptiveErrorStrategy so missing-delimiter messages are preferred over generic “Unexpected input” when confidently detected.
• Expand end-to-end and branch-level tests covering missing delimiter messages and caret positioning.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/main/java/org/apache/groovy/parser/antlr4/internal/MissingDelimiterDiagnostic.java	New internal diagnostic that scans tokens after recognition failure to locate missing ), ], } with better caret placement.
src/main/java/org/apache/groovy/parser/antlr4/internal/DescriptiveErrorStrategy.java	Routes NoViableAlt/InputMismatch reports through the new missing-delimiter diagnostic before falling back to generic messages.
src/test/groovy/org/apache/groovy/parser/antlr4/SyntaxErrorTest.groovy	Enables and adds end-to-end syntax error expectations for missing delimiters and caret positions.
src/test/groovy/org/apache/groovy/parser/antlr4/internal/MissingDelimiterDiagnosticTest.groovy	New focused unit tests for MissingDelimiterDiagnostic, including defensive/edge branches.
src/test/java/org/apache/groovy/parser/antlr4/internal/ThrowingTokenStream.java	Test helper token stream intended to force error paths during token collection.

[paulk-asert]

Non-blocking nits (optional follow-ups, worth a PR comment)

1. insertionPointAfter adds the token's codepoint length to its start column — a multi-line anchor token (triple-quoted string/GString before a missing }) overshoots the caret. Message stays correct; caret only. Rare.
2. fromCastPattern reports the earliest high-confidence cast site independent of where ANTLR actually failed — intentional and well-guarded, but in a multi-error file the location could point at a different construct.
3. The broad catch (IndexOutOfBounds | IllegalArgumentException) in collectDefaultChannelTokens silently proceeds with partial tokens — reasonable for non-buffered streams (and there's a ThrowingTokenStream test), already commented.

[daniellansun]

Non-blocking nits (optional follow-ups, worth a PR comment)

1. insertionPointAfter adds the token's codepoint length to its start column — a multi-line anchor token (triple-quoted string/GString before a missing }) overshoots the caret. Message stays correct; caret only. Rare.
2. fromCastPattern reports the earliest high-confidence cast site independent of where ANTLR actually failed — intentional and well-guarded, but in a multi-error file the location could point at a different construct.
3. The broad catch (IndexOutOfBounds | IllegalArgumentException) in collectDefaultChannelTokens silently proceeds with partial tokens — reasonable for non-buffered streams (and there's a ThrowingTokenStream test), already commented.

Thanks for the careful review and the approval — notes below on each optional nit.

1. Multi-line insertion point / caret overshoot
   Agreed. insertionPointAfter now reuses PositionConfigureUtils.endPosition, so a multi-line anchor (e.g. triple-quoted string before a missing }) puts the caret on the last line of that token, after its final content, instead of overshooting on the start line. Covered with unit tests and end-to-end cases in SyntaxErrorTest. Related: lastNonEof also skips trailing NL so EOF-based carets stay on the last meaningful line.

2. fromCastPattern reports the earliest high-confidence cast site
   Yes — intentional. With bail-on-error we stop at the first recognition failure, so the earliest incomplete cast is almost always the right one. Biasing toward ANTLR’s offending token was tried; it regressed good cases such as println ((int 123) (caret moved off the cast operand). Left as earliest-site, with the multi-error trade-off documented on the method.

3. Broad catch in collectDefaultChannelTokens
   Agreed it’s the right shape for non-buffered / partial streams. Comment tightened slightly, and the IllegalArgumentException branch is now exercised alongside the existing ThrowingTokenStream (IndexOutOfBoundsException) coverage.

[sonarqubecloud]

Quality Gate passed

Issues
10 New issues
0 Accepted issues

Measures
0 Security Hotspots
96.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[testlens-app]

✅ All tests passed ✅

🏷️ Commit: 1f18b8a
▶️ Tests: 104718 executed
⚪️ Checks: 31/31 completed

---

Learn more about TestLens at testlens.app.