[
https://issues.apache.org/jira/browse/GROOVY-9192?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18099079#comment-18099079
]
ASF GitHub Bot commented on GROOVY-9192:
----------------------------------------
daniellansun commented on PR #2724:
URL: https://github.com/apache/groovy/pull/2724#issuecomment-5076818766
> Approved with some minor potential cleanup possible. AI read:
>
> In short: well-scoped, well-tested, default behavior preserved, opt-in
semantics correct.
>
> Minor notes (none blocking):
>
> * Sonar reported 7 new minor issues (gate still passed) — worth a glance
before or after merge, likely trivia.
> * Theoretical edge: under recovery, a CompilationFailedException thrown
without any error having been recorded would surface as silent success — every
current throw path records first, so this is hypothetical, but a cheap
belt-and-braces guard (if collector empty, rethrow) could be suggested in
review comments if you want.
Thanks for the review and the careful notes.
1) Sonar (7 minor):
Addressed in a follow-up: assertSame for reference equality, single-throw
assertThrows
lambdas, assertion on recoveringStrategySyncDoesNotThrow, and the
unused-recognizer
hook on createFailedPredicateErrorMessage (kept for parity with the other
create*
hooks; documented + requireNonNull). The AstBuilder "brain method" INFO is
pre-existing
size on an unrelated visitor method and left alone.
2) Empty-collector + recovery → silent success:
Agreed — cheap and worth doing. buildAST now returns the partial module only
when
errorRecovery && errorCollector.hasErrors(); otherwise the CFE is rethrown.
Covered by
regression tests in Groovy9192 (empty collector rethrows; recorded
diagnostics still
return the partial module; fail-fast unchanged).
> Implement optional parser error recovery for Parrot parser
> ----------------------------------------------------------
>
> Key: GROOVY-9192
> URL: https://issues.apache.org/jira/browse/GROOVY-9192
> Project: Groovy
> Issue Type: Improvement
> Components: parser-antlr4
> Reporter: Daniel Sun
> Priority: Major
> Time Spent: 1h
> Remaining Estimate: 0h
>
> h2. Summary
> Add *optional* ANTLR error recovery to the Parrot (antlr4) parser so IDE
> hosts can collect *multiple* syntax diagnostics in one pass. Default remains
> fail-fast (current {{groovyc}} behaviour).
> h2. Motivation
> * Groovy Eclipse and similar tools reuse the Parrot parser while editing.
> * Recognition failures today fail-fast: the first syntax error stops parsing,
> so the editor often shows only one diagnostic per rebuild.
> * Editors need resynchronization after a fault so further errors in the same
> file can be reported without redesigning the grammar.
> h2. Goals
> # *Opt-in only* — recovery is *disabled by default*; production compilation
> stays fail-fast.
> # *Configuration switch* — {{CompilerConfiguration.ERROR_RECOVERY}}
> ({{errorRecovery}}) and optional system property
> {{groovy.parser.error.recovery}}.
> # *Multi-error collection* — with recovery on, recognition errors accumulate
> in {{ErrorCollector}} instead of aborting on the first fatal parse error.
> # *Friendly diagnostics preserved* — keep messages such as {{Missing ')'}} /
> {{Missing ']'}} / {{Missing '}'}} via existing {{MissingDelimiterDiagnostic}}.
> # *Fail-fast path unchanged* — SLL→LL two-stage parse and bail-style cancel
> remain the default cost model.
> # *No dual throwable hierarchy* — recovery is owned by the error strategy and
> {{ErrorCollector}}, not a second exception type tree.
> h2. Design
> h3. Configuration
> * Constant: {{CompilerConfiguration.ERROR_RECOVERY}} = {{"errorRecovery"}},
> stored in {{getOptimizationOptions()}} (same map as other feature flags, e.g.
> groovydoc).
> * Query: {{isErrorRecoveryEnabled()}} — true only when the option is
> explicitly {{Boolean.TRUE}}.
> * Bootstrap: system property {{groovy.parser.error.recovery}}.
> {code:java}
> CompilerConfiguration config = new CompilerConfiguration();
> config.getOptimizationOptions().put(CompilerConfiguration.ERROR_RECOVERY,
> Boolean.TRUE);
> // or: -Dgroovy.parser.error.recovery=true
> {code}
> h3. Error strategies
> * Shared reporting: package-private {{AbstractFriendlyErrorStrategy}}
> ({{MissingDelimiterDiagnostic}} + NVAE/IME messaging).
> * Fail-fast (default): {{DescriptiveErrorStrategy}} — {{recover}} /
> {{recoverInline}} cancel via {{ParseCancellationException}}; {{sync}} is a
> no-op (SLL-friendly).
> * Recovery: {{RecoveringDescriptiveErrorStrategy}} — ANTLR default resync /
> single-token repair.
> * Factory: {{DescriptiveErrorStrategy.create(CharStream, boolean)}}.
> h3. AstBuilder / pipeline
> * Select strategy from {{isErrorRecoveryEnabled()}}.
> * Recovery parse uses *LL only* with error listeners (skip the SLL probe so
> recovery cannot "succeed" on a degraded tree without re-running LL).
> * {{collectSyntaxError}}: recovery → {{ErrorCollector.addErrorAndContinue}};
> fail-fast → {{addFatalError}}.
> * Under recovery, {{AstBuilder.buildAST}} returns a (possibly partial)
> {{ModuleNode}} when diagnostics are already recorded; it does *not* require
> {{SourceUnit}} to swallow {{CompilationFailedException}}.
> * Compile still fails: {{CompilationUnit}} /
> {{ErrorCollector.failIfErrors()}} owns phase failure with the full diagnostic
> list.
> h3. Host contract
> * *Source of truth for multi-error UI:* {{ErrorCollector}} (not the partial
> AST).
> * Recovery may still yield an incomplete tree; later phases can add further
> diagnostics. IDEs should display all collector errors.
> h2. Compatibility
> * Default off: existing scripts, CLI, and fail-fast tests keep current
> behaviour and cost model.
> * Public additive API only: {{ERROR_RECOVERY}}, {{isErrorRecoveryEnabled()}}
> (no removals).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
