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 | | --- | --- | --- | | `TenOnly.groovy`, `--check --error-format=short` | `StackOverflowError` | `10 errors` | | `TenOnly.groovy`, `--tolerance=3` | `StackOverflowError` | `3 errors` | | 11 dereferences, no annotation, `--configscript`, default format | `StackOverflowError` | `10 errors` | | 12 dereferences, no annotation, `--configscript`, short format | `StackOverflowError` | `10 errors` | | `TenOnly.groovy`, `--tolerance=0` | `10 errors` | `10 errors` | | 9 dereferences | `9 errors` | `9 errors` | | plain `@TypeChecked`, 12 `int a = 'x'` | `10 errors` | `10 errors` | | extension calling `sourceUnit.addFatalError` | `StackOverflowError` | `1 error` | | extension parsing broken source with its own `GroovyShell` | `1 error` (`General error ...`) | same | ## Affected versions - 6.0.0: affected (reproduced with the binary distribution on JDK 21). - 6.0.1 (unreleased): `GROOVY_6_0_X` (caef1fcb4e) has the same `safeCall`, `ErrorCollector` and `StaticTypeCheckingVisitor.addError`, so it is affected. Checked by source only, not built. - master (7.0.0-SNAPSHOT): affected, reproduced on 9a1a0d0ccb. - 6.0.0-beta-1: the tolerance case is not affected (prints `10 errors`), but the fatal-error case is. - 5.0.8: the fatal-error case is affected. Before GROOVY-12306 the tolerance did not apply to type checking errors. ## Backport Suggested target: `GROOVY_6_0_X`. Both touched files are identical there and on master, and the commit applies cleanly to caef1fcb4e (`git apply --check`). The 5.x branches (`GROOVY_5_0_X`, `GROOVY_5_1_X`) have the same `safeCall`, so they could take the change for the fatal-error case. They target Java 11, so the pattern variable would need to become a cast there. --- -- 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]
