[
https://issues.apache.org/jira/browse/GROOVY-12689?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18137762#comment-18137762
]
ASF GitHub Bot commented on GROOVY-12689:
-----------------------------------------
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.
> FormatStringChecker fails inside the compiler when an argument's type is
> declared in the same source
> ----------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12689
> URL: https://issues.apache.org/jira/browse/GROOVY-12689
> Project: Groovy
> Issue Type: Bug
> Affects Versions: 6.0.0
> Reporter: Scott Murphy Heiberg
> Priority: Major
>
> 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 with a {{GroovyBugError}}. A plain class is
> enough, and so is an enum, a record, an anonymous class, an array of such a
> type, or {{this}} inside the checked class. The conversion does not matter:
> {{%s}} fails too.
> {{Show.groovy}} and {{ShowColor.groovy}}:
> {code:groovy}
> import groovy.transform.TypeChecked
> class Foo {}
> @TypeChecked(extensions = 'groovy.typecheckers.FormatStringChecker')
> String show() {
> String.format('%s', new Foo())
> }
> {code}
> {code:groovy}
> import groovy.transform.TypeChecked
> enum Color { RED, GREEN }
> @TypeChecked(extensions = 'groovy.typecheckers.FormatStringChecker')
> String show() {
> String.format('%s', Color.RED)
> }
> {code}
> {code:sh}
> groovyc --check --error-format=short Show.groovy
> groovyc --check --error-format=short ShowColor.groovy
> {code}
> Actual, for {{Show.groovy}}: exit 1 and
> {code:text}
> >>> a serious error occurred: BUG! exception in phase 'instruction selection'
> >>> in source unit 'Show.groovy' ClassNode#getTypeClass for Foo called before
> >>> the type class is set
> >>> stacktrace:
> BUG! exception in phase 'instruction selection' in source unit 'Show.groovy'
> ClassNode#getTypeClass for Foo called before the type class is set
> at org.codehaus.groovy.ast.ClassNode.getTypeClass(ClassNode.java:1987)
> at
> groovy.typecheckers.FormatStringChecker$1$_checkFormatStringTypes_closure5.doCall(FormatStringChecker.groovy:184)
> ...
> at
> groovy.typecheckers.FormatStringChecker$1.checkFormatStringTypes(FormatStringChecker.groovy:163)
> at
> groovy.typecheckers.FormatStringChecker$1.checkFormatterMethod(FormatStringChecker.groovy:156)
> at
> groovy.typecheckers.FormatStringChecker$1.visitMethodCallExpression(FormatStringChecker.groovy:122)
> ...
> at
> org.codehaus.groovy.transform.stc.GroovyTypeCheckingExtensionSupport.afterVisitMethod(GroovyTypeCheckingExtensionSupport.java:427)
> ...
> {code}
> {{ShowColor.groovy}} fails the same way with {{ClassNode#getTypeClass for
> Color called before the type class is set}}. The smallest form needs no
> second type:
> {code:groovy}
> @groovy.transform.TypeChecked(extensions =
> 'groovy.typecheckers.FormatStringChecker')
> class Main {
> String show() { String.format('%s', this) }
> }
> {code}
> It fails with {{ClassNode#getTypeClass for Main called before the type class
> is set}}. The same happens when the checker is applied through
> {{CompilerConfiguration}}:
> {code:groovy}
> import groovy.transform.TypeChecked
> import org.codehaus.groovy.control.CompilerConfiguration
> import org.codehaus.groovy.control.customizers.ASTTransformationCustomizer
> def config = new CompilerConfiguration()
> config.addCompilationCustomizers(new ASTTransformationCustomizer(
> [extensions: 'groovy.typecheckers.FormatStringChecker'], TypeChecked))
> new GroovyShell(config).evaluate('''
> class Foo {}
> String.format('%s', new Foo())
> ''')
> {code}
> {code:text}
> org.codehaus.groovy.GroovyBugError: BUG! exception in phase 'instruction
> selection' in source unit 'Script1.groovy' ClassNode#getTypeClass for Foo
> called before the type class is set
> {code}
> Expected: these sources compile. They do when {{Foo}} is compiled first and
> put on the class path: {{groovyc --check}} of the same checked method then
> exits 0. A real mismatch should still be reported, for example
> {{IllegalFormatConversion: d != Foo}} for {{String.format('%d', new Foo())}}.
> That is what the checker reports for a precompiled {{Foo}} or for
> {{java.util.UUID}}.
> When the checked code also declares an enum or a record, this error hides the
> one described in GROOVY-12688. The two have separate causes.
> Affected versions: 6.0.0, 6.0.0-beta-1, 5.0.8 and 5.0.0 fail the same way,
> and so does current master (7.0.0-SNAPSHOT at 5e9212ce1e).
> Cause: when a format call's arguments are not all constants,
> {{checkFormatStringTypes}} takes the runtime class of each argument's static
> type ({{getWrapper(getType(arg)).typeClass}}, line 184) and compares it with
> {{Class}} objects. A type compiled in the same compilation unit has no
> runtime class yet, so {{ClassNode#getTypeClass}} throws. The class is taken
> before the conversion is looked at, which is why {{%s}} fails as well.
> Because {{GroovyBugError}} is an {{Error}}, it escapes the extension's error
> handling and ends the compilation.
> A fix that works: compare the argument's {{ClassNode}} with the
> {{ClassHelper}} types, and use
> {{StaticTypeCheckingSupport.implementsInterfaceOrIsSubclassOf}} for the
> date/time check, so that types from the same source are checked like
> precompiled ones. A pull request with regression tests follows.
> Impact: FormatStringChecker was graduated to stable for 6.0.0 (GROOVY-12039).
> Checked code that passes an instance of one of its own types to a format
> method, including {{this}} or one of its own enum constants, cannot be
> compiled with it. Workaround: compile those types separately, or leave
> FormatStringChecker off for such sources.
> Environment: Groovy 6.0.0 on BellSoft JDK 21.0.12, macOS 26.5 (arm64);
> reproduced on master at 5e9212ce1e with JDK 21.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
