codeconsole opened a new pull request, #3208: URL: https://github.com/apache/groovy/pull/3208
JIRA: [GROOVY-12689](https://issues.apache.org/jira/browse/GROOVY-12689) ### The bug With `groovy.typecheckers.FormatStringChecker` enabled, a format call with a non-constant argument whose static type is declared in the same compilation unit aborts the compilation: ```text BUG! exception in phase 'instruction selection' in source unit 'Show.groovy' ClassNode#getTypeClass for Foo called before the type class is set ``` The type can be a class, an enum, a record, an anonymous class, an array of one of these, or `this` inside the checked class. The conversion does not matter: `%s` fails too. The same code compiles when the type is compiled first and put on the class path. ### Reproducer ```groovy import groovy.transform.TypeChecked class Foo {} @TypeChecked(extensions = 'groovy.typecheckers.FormatStringChecker') String show() { String.format('%s', new Foo()) } ``` ```groovy import groovy.transform.TypeChecked enum Color { RED, GREEN } @TypeChecked(extensions = 'groovy.typecheckers.FormatStringChecker') String show() { String.format('%s', Color.RED) } ``` `groovyc --check Show.groovy` (or `ShowColor.groovy`) exits 1 with the error above, for `Foo` or `Color`. The smallest form is `String.format('%s', this)` in a class annotated `@TypeChecked(extensions = 'groovy.typecheckers.FormatStringChecker')`. The checker fails the same way when it is applied through an `ASTTransformationCustomizer` in a `CompilerConfiguration`. ### Cause When a format call's arguments are not all constants, `checkFormatStringTypes` took the runtime class of each argument's static type (`getWrapper(getType(arg)).typeClass`) and compared it with `Class` objects. A type compiled in the same compilation unit has no runtime class yet, so `ClassNode#getTypeClass` throws `GroovyBugError`. That is an `Error`, so it escapes the extension's error handling and ends the compilation. The class was taken before the conversion was looked at, which is why `%s` failed as well. When the checked code also declares an enum or a record, this error hides the one fixed by #3207 (GROOVY-12688). The two are independent, and this change does not depend on #3207. ### Fix `checkFormatStringTypes` now works on the argument's `ClassNode`: - The character, integral and floating-point checks compare it with `ClassHelper.Character_TYPE`, `Integer_TYPE`, `BigDecimal_TYPE` and the rest. This is the same equality check as before, now on `ClassNode`s. - The date/time check uses `StaticTypeCheckingSupport.implementsInterfaceOrIsSubclassOf` against `Long`, `Calendar`, `Date` and `TemporalAccessor`, in place of `Class#isAssignableFrom`. Types from the same source are now checked like precompiled ones. `String.format('%d', new Foo())` reports `IllegalFormatConversion: d != Foo`, as it already did for a precompiled `Foo`. A same-source subclass of `Date`, or an implementation of `TemporalAccessor`, is accepted for `%tY`. For types that have a class, I compared the checker's results before and after the change. The comparison covered 53 argument types (primitives, wrappers, `BigInteger`/`BigDecimal`, `String`/`GString`, date/time types and interfaces, arrays, generic collections, type parameters, and conditional expressions with mixed types) against 12 conversions, 636 cases in all. The verdict changed in two cases, and a message changed in seven: - `%tY` and `%TH` on `flag ? LocalDate.now() : LocalDateTime.now()` were reported as `Y != java.io.Serializable` and are now accepted. The common type implements `Temporal`, so it is a `TemporalAccessor`, and `Formatter` accepts either value. - For a `Date[]` argument, the message now reads `d != java.util.Date[]` instead of `d != [Ljava.util.Date;`, because it uses the `ClassNode` name. Whether the call is reported is unchanged, and `int[]` and `String[]` messages are unchanged. The other checkers in the module do not take runtime classes from `ClassNode`s. `CheckingVisitor.findConstExp` uses `Class#isAssignableFrom` only on the value of a constant. I compiled code that uses same-source classes, enums, records, arrays and anonymous classes under `RegexChecker`, `NullChecker`, `SqlInjectionChecker`, `PurityChecker`, `ModifiesChecker`, `CombinerChecker`, `MonadicChecker` and `MonadicShapeChecker`. None of them fails, before or after this change. ### Tests Two new tests in `FormatStringCheckerTest`: - `testArgsOfTypesFromSameSource`: at script level, with the extension applied by the test shell's customizer, an instance of a same-source class compiles and formats correctly. In a class annotated `@TypeChecked(extensions = 'groovy.typecheckers.FormatStringChecker')`, a same-source class, an enum constant, an array, `this`, and a same-source `Date` subclass with `%tY` all compile and format correctly. - `testIllegalConversionForArgsOfTypesFromSameSource`: `%d` on a same-source class, `%c` on a same-source enum constant and `%tY` on a same-source class are all reported as `IllegalFormatConversion`, and no `BUG!` occurs. Both tests failed on master with the `getTypeClass` error and pass with the change. The existing mismatch tests are unchanged and pass. ```text ./gradlew :groovy-typecheckers:test --rerun 388 tests, 0 failures ./gradlew :groovy-typecheckers:lintGate :groovy-typecheckers:rat BUILD SUCCESSFUL ``` With #3207 applied on top as well, the module has 392 tests and none fails. Sources that combine an enum or record in the checked code with a same-source format argument then compile. ### Affected versions Reproduced with JDK 21 on the Groovy 6.0.0 and 6.0.0-beta-1 distributions and on master (7.0.0-SNAPSHOT at 5e9212ce1e). 5.0.0 and 5.0.8 fail the same way. The affected code dates from GROOVY-11261. ### Backport Suggested target: `GROOVY_6_0_X`. `FormatStringChecker.groovy` and `FormatStringCheckerTest.groovy` are the same there as on master, and the commit applies without conflicts. If a 5.0.x backport is wanted, `GROOVY_5_0_X` has the same five lines, but the imports and fields around them differ, so the edits would need to be made by hand. -- 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]
