[
https://issues.apache.org/jira/browse/GROOVY-12166?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Ben Sherman updated GROOVY-12166:
---------------------------------
Description:
(Found while upgrading Nextflow to Groovy 5)
h2. Summary
Under Groovy 5, when a `@CompileStatic` class contains a method whose body is
`return someField instanceof SomeSubtype`, the static type checker narrows the
*{*}declared type of the field itself{*}* to `SomeSubtype` for the rest of the
class.
The static compiler then emits a spurious `checkcast SomeSubtype` before
{*}other{*},
unrelated calls to that field elsewhere in the class. At runtime those methods
throw `ClassCastException` whenever the field holds a different subtype of its
declared type.
The field's declared type is a common base class; the `instanceof` is only a
runtime test and must not change the field's static type outside its own
expression. This worked correctly in Groovy 4.
h2. Affected / unaffected versions
|Groovy version|Result|
|—|—|
|4.0.29|OK — prints `base`|
|4.0.31|OK — prints `base`|
|5.0.7|*{*}BUG{*}* — `ClassCastException`|
This is a *{*}regression{*}* introduced somewhere between 4.0.x and 5.0.7.
h2. Minimal reproducible example
`Bug.groovy`:
```groovy
import groovy.transform.CompileStatic
abstract class Base {
String describe()
{ 'base' }
abstract void unrelated()
}
class Alpha extends Base \{ void unrelated() {} }
class Beta extends Base \{ void unrelated() {} }
@CompileStatic
class Holder {
private Base field
Holder(Base f)
{ this.field = f }
// (1) `instanceof` applied directly to the FIELD, with an explicit `return`.
// This is what poisons the field's inferred type.
boolean isAlpha()
{ return field instanceof Alpha }
// (2) An UNRELATED method that merely calls a Base method on the field.
// Under Groovy 5 the static compiler wrongly narrows `field` to `Alpha`
// here and emits `checkcast Alpha` before the call.
String use()
{ field.describe() }
}
// `field` holds a Beta and is never an Alpha
def h = new Holder(new Beta())
assert h.isAlpha() == false
println h.use() // Groovy 4 -> "base" | Groovy 5 -> ClassCastException
```
h3. How to run
```bash
# run with a given Groovy version
groovy Bug.groovy
```
h3. Expected output
```
base
```
h3. Actual output (Groovy 5.0.7)
```
Exception in thread "main" java.lang.ClassCastException: class Beta cannot be
cast to class Alpha (Beta and Alpha are in unnamed module of loader 'app')
at Holder.use(Bug.groovy)
at Bug.run(Bug.groovy)
```
h2. Bytecode evidence
Disassembly of `Holder.use()` (`javap -p -c Holder`), which contains *{*}no{*}*
`instanceof` and only calls `Base.describe()`:
*{*}Groovy 4.0.29 / 4.0.31 (correct):{*}*
```
1: getfield // Field field:LBase;
4: invokevirtual // Method Base.describe:()Ljava/lang/String;
```
*{*}Groovy 5.0.7 (incorrect — bogus cast injected):{*}*
```
1: getfield // Field field:LBase;
4: checkcast // class Alpha <-- injected; field is declared `Base`
7: invokevirtual // Method Alpha.describe:()Ljava/lang/String;
```
Note that `Alpha` does not even override `describe()` — the method is inherited
from `Base` — so narrowing the receiver to `Alpha` is both unnecessary and
wrong.
h2. Trigger conditions
>From bisecting the example, all of the following are required to reproduce:
1. The enclosing class is `@CompileStatic`.
2. A field (declared with a base/abstract type) is tested with `instanceof` a
subtype *{*}directly on the field reference{*}* (not via a local copy).
3. That `instanceof` expression is returned with an *{*}explicit `return`{*}*
keyword: `return field instanceof Sub`. Using Groovy's implicit return
(`field instanceof Sub` as the last expression) does *{*}not{*}* trigger it.
4. Another method in the same class calls a method on the same field.
Only the `return` on the `instanceof` method matters; the second method may use
either explicit or implicit return.
h2. Workaround
Copy the field into a local variable before the `instanceof` check so the
narrowing applies to the local, not the field:
```groovy
boolean isAlpha() {
final f = field
return f instanceof Alpha
}
```
was:
## Summary
Under Groovy 5, when a `@CompileStatic` class contains a method whose body is
`return someField instanceof SomeSubtype`, the static type checker narrows the
**declared type of the field itself** to `SomeSubtype` for the rest of the
class.
The static compiler then emits a spurious `checkcast SomeSubtype` before
*other*,
unrelated calls to that field elsewhere in the class. At runtime those methods
throw `ClassCastException` whenever the field holds a different subtype of its
declared type.
The field's declared type is a common base class; the `instanceof` is only a
runtime test and must not change the field's static type outside its own
expression. This worked correctly in Groovy 4.
## Affected / unaffected versions
| Groovy version | Result |
| --- | --- |
| 4.0.29 | OK — prints `base` |
| 4.0.31 | OK — prints `base` |
| 5.0.7 | **BUG** — `ClassCastException` |
This is a **regression** introduced somewhere between 4.0.x and 5.0.7.
## Minimal reproducible example
`Bug.groovy`:
```groovy
import groovy.transform.CompileStatic
abstract class Base {
String describe() { 'base' }
abstract void unrelated()
}
class Alpha extends Base { void unrelated() {} }
class Beta extends Base { void unrelated() {} }
@CompileStatic
class Holder {
private Base field
Holder(Base f) { this.field = f }
// (1) `instanceof` applied directly to the FIELD, with an explicit
`return`.
// This is what poisons the field's inferred type.
boolean isAlpha() { return field instanceof Alpha }
// (2) An UNRELATED method that merely calls a Base method on the field.
// Under Groovy 5 the static compiler wrongly narrows `field` to `Alpha`
// here and emits `checkcast Alpha` before the call.
String use() { field.describe() }
}
// `field` holds a Beta and is never an Alpha
def h = new Holder(new Beta())
assert h.isAlpha() == false
println h.use() // Groovy 4 -> "base" | Groovy 5 -> ClassCastException
```
### How to run
```bash
# run with a given Groovy version
groovy Bug.groovy
```
### Expected output
```
base
```
### Actual output (Groovy 5.0.7)
```
Exception in thread "main" java.lang.ClassCastException: class Beta cannot be
cast to class Alpha (Beta and Alpha are in unnamed module of loader 'app')
at Holder.use(Bug.groovy)
at Bug.run(Bug.groovy)
```
## Bytecode evidence
Disassembly of `Holder.use()` (`javap -p -c Holder`), which contains **no**
`instanceof` and only calls `Base.describe()`:
**Groovy 4.0.29 / 4.0.31 (correct):**
```
1: getfield // Field field:LBase;
4: invokevirtual // Method Base.describe:()Ljava/lang/String;
```
**Groovy 5.0.7 (incorrect — bogus cast injected):**
```
1: getfield // Field field:LBase;
4: checkcast // class Alpha <-- injected; field is declared `Base`
7: invokevirtual // Method Alpha.describe:()Ljava/lang/String;
```
Note that `Alpha` does not even override `describe()` — the method is inherited
from `Base` — so narrowing the receiver to `Alpha` is both unnecessary and
wrong.
## Trigger conditions
>From bisecting the example, all of the following are required to reproduce:
1. The enclosing class is `@CompileStatic`.
2. A field (declared with a base/abstract type) is tested with `instanceof` a
subtype **directly on the field reference** (not via a local copy).
3. That `instanceof` expression is returned with an **explicit `return`**
keyword: `return field instanceof Sub`. Using Groovy's implicit return
(`field instanceof Sub` as the last expression) does **not** trigger it.
4. Another method in the same class calls a method on the same field.
Only the `return` on the `instanceof` method matters; the second method may use
either explicit or implicit return.
## Workaround
Copy the field into a local variable before the `instanceof` check so the
narrowing applies to the local, not the field:
```groovy
boolean isAlpha() {
final f = field
return f instanceof Alpha
}
```
## Real-world impact
Found while upgrading [Nextflow](https://github.com/nextflow-io/nextflow) to
Groovy 5.
> STC: `return field instanceof Subtype` narrows the field's type class-wide,
> causing `ClassCastException` -- Groovy 5 regression
> -------------------------------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12166
> URL: https://issues.apache.org/jira/browse/GROOVY-12166
> Project: Groovy
> Issue Type: Bug
> Components: Static compilation
> Affects Versions: 5.0.7
> Environment: Ubuntu, JDK 17
> Reporter: Ben Sherman
> Priority: Minor
>
> (Found while upgrading Nextflow to Groovy 5)
> h2. Summary
> Under Groovy 5, when a `@CompileStatic` class contains a method whose body is
> `return someField instanceof SomeSubtype`, the static type checker narrows the
> *{*}declared type of the field itself{*}* to `SomeSubtype` for the rest of
> the class.
> The static compiler then emits a spurious `checkcast SomeSubtype` before
> {*}other{*},
> unrelated calls to that field elsewhere in the class. At runtime those methods
> throw `ClassCastException` whenever the field holds a different subtype of its
> declared type.
> The field's declared type is a common base class; the `instanceof` is only a
> runtime test and must not change the field's static type outside its own
> expression. This worked correctly in Groovy 4.
> h2. Affected / unaffected versions
> |Groovy version|Result|
> |—|—|
> |4.0.29|OK — prints `base`|
> |4.0.31|OK — prints `base`|
> |5.0.7|*{*}BUG{*}* — `ClassCastException`|
> This is a *{*}regression{*}* introduced somewhere between 4.0.x and 5.0.7.
> h2. Minimal reproducible example
> `Bug.groovy`:
> ```groovy
> import groovy.transform.CompileStatic
> abstract class Base {
> String describe()
> { 'base' }
> abstract void unrelated()
> }
> class Alpha extends Base \{ void unrelated() {} }
> class Beta extends Base \{ void unrelated() {} }
> @CompileStatic
> class Holder {
> private Base field
> Holder(Base f)
> { this.field = f }
> // (1) `instanceof` applied directly to the FIELD, with an explicit `return`.
> // This is what poisons the field's inferred type.
> boolean isAlpha()
> { return field instanceof Alpha }
> // (2) An UNRELATED method that merely calls a Base method on the field.
> // Under Groovy 5 the static compiler wrongly narrows `field` to `Alpha`
> // here and emits `checkcast Alpha` before the call.
> String use()
> { field.describe() }
> }
> // `field` holds a Beta and is never an Alpha
> def h = new Holder(new Beta())
> assert h.isAlpha() == false
> println h.use() // Groovy 4 -> "base" | Groovy 5 -> ClassCastException
> ```
> h3. How to run
> ```bash
> # run with a given Groovy version
> groovy Bug.groovy
> ```
> h3. Expected output
> ```
> base
> ```
> h3. Actual output (Groovy 5.0.7)
> ```
> Exception in thread "main" java.lang.ClassCastException: class Beta cannot be
> cast to class Alpha (Beta and Alpha are in unnamed module of loader 'app')
> at Holder.use(Bug.groovy)
> at Bug.run(Bug.groovy)
> ```
> h2. Bytecode evidence
> Disassembly of `Holder.use()` (`javap -p -c Holder`), which contains
> *{*}no{*}*
> `instanceof` and only calls `Base.describe()`:
> *{*}Groovy 4.0.29 / 4.0.31 (correct):{*}*
> ```
> 1: getfield // Field field:LBase;
> 4: invokevirtual // Method Base.describe:()Ljava/lang/String;
> ```
> *{*}Groovy 5.0.7 (incorrect — bogus cast injected):{*}*
> ```
> 1: getfield // Field field:LBase;
> 4: checkcast // class Alpha <-- injected; field is declared `Base`
> 7: invokevirtual // Method Alpha.describe:()Ljava/lang/String;
> ```
> Note that `Alpha` does not even override `describe()` — the method is
> inherited
> from `Base` — so narrowing the receiver to `Alpha` is both unnecessary and
> wrong.
> h2. Trigger conditions
> From bisecting the example, all of the following are required to reproduce:
> 1. The enclosing class is `@CompileStatic`.
> 2. A field (declared with a base/abstract type) is tested with `instanceof` a
> subtype *{*}directly on the field reference{*}* (not via a local copy).
> 3. That `instanceof` expression is returned with an *{*}explicit `return`{*}*
> keyword: `return field instanceof Sub`. Using Groovy's implicit return
> (`field instanceof Sub` as the last expression) does *{*}not{*}* trigger it.
> 4. Another method in the same class calls a method on the same field.
> Only the `return` on the `instanceof` method matters; the second method may
> use
> either explicit or implicit return.
> h2. Workaround
> Copy the field into a local variable before the `instanceof` check so the
> narrowing applies to the local, not the field:
> ```groovy
> boolean isAlpha() {
> final f = field
> return f instanceof Alpha
> }
> ```
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
