[
https://issues.apache.org/jira/browse/GROOVY-12687?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18137753#comment-18137753
]
ASF GitHub Bot commented on GROOVY-12687:
-----------------------------------------
codeconsole opened a new pull request, #3206:
URL: https://github.com/apache/groovy/pull/3206
JIRA: https://issues.apache.org/jira/browse/GROOVY-12687
## The bug
When an error reported by a type checking extension reaches the compiler's
error tolerance
(10 by default), compilation ends in a `StackOverflowError` and none of the
collected errors is
reported. Ten errors from `groovy.typecheckers.NullChecker` in one method
are enough. Any
tolerance above zero is affected, not only the default (`--tolerance=3`
fails the same way), and
so is a fatal error reported from an extension
(`sourceUnit.addFatalError(...)`), whatever the
tolerance.
## Reproducer
`TenOnly.groovy`:
```groovy
import groovy.transform.TypeChecked
@interface Nullable {}
@TypeChecked(extensions = 'groovy.typecheckers.NullChecker')
class TenOnly {
void touch(@Nullable String a) {
a.length(); a.length(); a.length(); a.length(); a.length()
a.length(); a.length(); a.length(); a.length(); a.length()
}
}
```
```sh
groovyc --check --error-format=short TenOnly.groovy
```
With 6.0.0 and with master (9a1a0d0ccb) it exits 1 after printing 1,030
lines of stack trace
and no diagnostic:
```text
Caused by: java.lang.StackOverflowError
...
at
org.codehaus.groovy.control.MultipleCompilationErrorsException.getMessage(MultipleCompilationErrorsException.java:76)
at
org.codehaus.groovy.control.messages.ExceptionMessage.toDiagnostic(ExceptionMessage.java:68)
at
org.codehaus.groovy.control.ErrorFormat.writeShort(ErrorFormat.java:116)
at org.codehaus.groovy.control.ErrorFormat$2.write(ErrorFormat.java:71)
at
org.codehaus.groovy.control.ErrorCollector.write(ErrorCollector.java:362)
(repeating: 203 ErrorFormat.writeShort frames in the printed trace)
```
Expected, and printed with this change: the ten `Potential null dereference:
'a' is @Nullable`
diagnostics and `10 errors`, as for ordinary type checking errors.
Neither `--check` nor the annotation is needed. The same class without
`@TypeChecked`, compiled
with `--configscript` applying
`ast(groovy.transform.TypeChecked, extensions:
['groovy.typecheckers.NullChecker'])`, fails the
same way with the default error format (that cycle goes through
`ErrorFormat.writeFull` instead).
Nine errors, `--tolerance=0`, and twelve ordinary `@TypeChecked` errors with
no extension were
already reported correctly, and their output is unchanged.
## Cause
An extension's event handlers (`afterVisitMethod`, `onMethodSelection` and
the rest) are run by
`AbstractTypeCheckingExtension.safeCall`, which catches any `Exception` and
records it with
`SourceUnit.addException`. When an error the extension reports reaches the
tolerance,
`ErrorCollector.addError` throws a `MultipleCompilationErrorsException` that
carries the
collector itself, and `safeCall` records that exception, as an
`ExceptionMessage`, in the same
collector. Rendering the collector renders the exception's message, which
renders the collector
again.
The path is old: a fatal error reported from an extension overflows the same
way on 5.0.8 and
6.0.0-beta-1. GROOVY-12306 (6.0.0-RC-1), which made type checking errors
count toward the
tolerance, is what lets ordinary extension errors reach it.
## Fix
`safeCall` now rethrows a `MultipleCompilationErrorsException` that carries
the source unit's
own error collector, because its errors are already recorded there. It then
propagates the way
it does when the type checker itself reaches the tolerance. Any other
exception is recorded as
before.
The check compares collectors rather than only exception types.
`CompilationUnit`'s phase loop
discards a `CompilationFailedException` on the assumption that its errors
are already collected,
so rethrowing every `CompilationFailedException` would lose a failure from a
separate compilation
that an extension runs. This was checked with an extension that calls
`new GroovyShell().parse('class {')`. If every such exception is rethrown,
the class compiles
with exit 0 and is written out. With this change it still reports
`General error during canonicalization: startup failed: ... Missing '}'`,
exactly as before.
## Test
`ErrorToleranceTest.testExtensionErrorsHonourTolerance` compiles a class
with 14 methods, using a
precompiled extension that reports one error per method from
`afterVisitMethod`, at the default
tolerance. It checks four things:
- the failure's message renders and contains the extension's error;
- exactly `DEFAULT_TOLERANCE` errors are collected;
- all of them are `SyntaxErrorMessage`s;
- with tolerance 0, all 14 are reported.
Without the fix, the test fails with `StackOverflowError` in
`MultipleCompilationErrorsException.getMessage`.
Commands and results (JDK 21.0.12, macOS, `--max-workers=2`):
- `./gradlew :test --tests org.codehaus.groovy.control.ErrorToleranceTest`:
7 tests. Without the
fix, 1 failed (the new test, with `java.lang.StackOverflowError`). With
the fix, all 7 pass.
- `./gradlew :test :groovy-typecheckers:test`: `BUILD SUCCESSFUL`. Core ran
19,317 tests with
0 failures, 0 errors and 68 skipped; groovy-typecheckers ran 386 tests
with 0 failures.
- `./gradlew :checkstyleMain`: no new violation. The class's existing
fan-out warning goes from
26 to 27 (limit 22) because of the one added import. The total for core
main is unchanged
at 3,337.
- CodeNarc 4.0.0 with `config/codenarc/codenarc.groovy`, run directly on
`ErrorToleranceTest.groovy`: one P3 (`UnnecessaryDotClass`) both before
and after, on a line
the change only moves. The whole `:codenarcTest` task ran out of heap on
this machine, so it
was not used.
- `./gradlew :groovy-binary:installGroovy`, then the installed `groovyc`
before and after the
change. On master, the installed `groovyc` fails at startup without JavaFX
(GROOVY-12683), so
these runs used a copy of the installation without `lib/groovy-fx-*.jar`.
| Case | Before | After |
| -
> groovyc: StackOverflowError when the error tolerance is reached inside a
> type-checking extension
> ------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12687
> URL: https://issues.apache.org/jira/browse/GROOVY-12687
> Project: Groovy
> Issue Type: Bug
> Affects Versions: 6.0.0
> Reporter: Scott Murphy Heiberg
> Priority: Major
>
> When a type-checking extension reports the error that reaches the compiler's
> error tolerance (10 by default), {{groovyc}} (with or without {{--check}})
> prints no diagnostics and ends with a {{StackOverflowError}}. Ten reports
> from NullChecker in one method are enough.
> {{TenOnly.groovy}}:
> {code:groovy}
> import groovy.transform.TypeChecked
> @interface Nullable {}
> @TypeChecked(extensions = 'groovy.typecheckers.NullChecker')
> class TenOnly {
> void touch(@Nullable String a) {
> a.length(); a.length(); a.length(); a.length(); a.length()
> a.length(); a.length(); a.length(); a.length(); a.length()
> }
> }
> {code}
> {code:sh}
> groovyc --check --error-format=short TenOnly.groovy
> {code}
> Actual: exit 1 and about 1,000 lines on stderr, no diagnostic:
> {code:text}
> java.lang.reflect.InvocationTargetException
> at
> org.codehaus.groovy.tools.GroovyStarter.rootLoader(GroovyStarter.java:150)
> at org.codehaus.groovy.tools.GroovyStarter.main(GroovyStarter.java:52)
> Caused by: java.lang.StackOverflowError
> ...
> at
> org.codehaus.groovy.control.ErrorFormat.writeShort(ErrorFormat.java:116)
> at org.codehaus.groovy.control.ErrorFormat$2.write(ErrorFormat.java:71)
> at
> org.codehaus.groovy.control.ErrorCollector.write(ErrorCollector.java:349)
> at
> org.codehaus.groovy.control.MultipleCompilationErrorsException.getMessage(MultipleCompilationErrorsException.java:79)
> at
> org.codehaus.groovy.control.messages.ExceptionMessage.toDiagnostic(ExceptionMessage.java:68)
> at
> org.codehaus.groovy.control.ErrorFormat.writeShort(ErrorFormat.java:116)
> (the last five frames repeat; 203 times in the printed trace)
> {code}
> The same file without {{--error-format=short}} cycles through
> {{ErrorFormat.writeFull}}, {{ExceptionMessage.write}} and the same
> {{ErrorCollector.write}} / {{MultipleCompilationErrorsException.getMessage}}
> pair. Neither {{--check}} nor the annotation is required: the same class with
> eleven dereferences and no {{@TypeChecked}}, compiled with {{groovyc -d out
> --configscript nullchecker.groovy Many.groovy}} where {{nullchecker.groovy}}
> is {{withConfig(configuration) { ast(groovy.transform.TypeChecked,
> extensions: ['groovy.typecheckers.NullChecker']) } }}, fails the same way.
> Expected: the ten diagnostics and {{10 errors}}, as for ordinary type errors.
> Controls:
> * The same class with nine dereferences prints nine {{Potential null
> dereference: 'a' is @Nullable}} lines and {{9 errors}}.
> * Twelve {{int a = 'x'}} assignments under plain {{@TypeChecked}} (no
> extension) print the first ten {{Cannot assign value of type java.lang.String
> to variable of type int}} diagnostics and {{10 errors}}.
> * 6.0.0-beta-1 on {{TenOnly.groovy}} prints the ten diagnostics and {{10
> errors}} (no crash), so the tolerance case is a regression between
> 6.0.0-beta-1 and 6.0.0: GROOVY-12306 (fixed in 6.0.0-RC-1) made it reachable.
> The underlying defect is older: an extension that reports a fatal error
> overflows the same way on 6.0.0-beta-1 and 5.0.8. Current master is affected
> too.
> * {{groovyc --check --error-format=short --tolerance=0 TenOnly.groovy}}
> (unlimited tolerance) prints all ten diagnostics and {{10 errors}}; this is a
> workaround.
> Analysis. A script that compiles {{TenOnly.groovy}} through
> {{CompilationUnit}} and inspects the thrown
> {{MultipleCompilationErrorsException}} without rendering it shows 11 messages
> in the collector: ten {{SyntaxErrorMessage}} and one {{ExceptionMessage}}
> whose cause is a {{MultipleCompilationErrorsException}} carrying the same
> collector, raised from {{ErrorCollector.failIfErrors}} via
> {{StaticTypeCheckingVisitor.addStaticTypeError}} inside the extension's
> {{afterVisitMethod}}. So the tenth {{addStaticTypeError}} makes
> {{failIfErrors}} throw; {{AbstractTypeCheckingExtension.safeCall}} catches
> {{Exception}} and calls {{SourceUnit.addException}}, which records that
> exception as an {{ExceptionMessage}} in the same collector. Rendering the
> collector renders the exception's message, which renders the collector again,
> without end.
> A fix that works: {{safeCall}} rethrows a
> {{MultipleCompilationErrorsException}} that carries the source unit's own
> error collector instead of recording it; other exceptions are recorded as
> before. With that change the file above reports its ten diagnostics and {{10
> errors}}, {{--tolerance=3}} reports three, an extension reporting a fatal
> error reports one error, and {{--tolerance=0}} and plain {{@TypeChecked}} are
> unchanged. A pull request with a regression test ({{ErrorToleranceTest}})
> follows.
> Impact: any build or tool that compiles with an extension (the Groovy 6
> checkers, or a custom {{TypeCheckingDSL}}) and hits ten reports loses every
> diagnostic.
> Environment: Groovy 6.0.0 on BellSoft JDK 21.0.12, macOS 26.5 (arm64);
> reproduced on master at 9a1a0d0ccb with JDK 21.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
