[
https://issues.apache.org/jira/browse/GROOVY-12440?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
James Daugherty updated GROOVY-12440:
-------------------------------------
Description:
*Affects versions:* 5.0.0 – 5.1.3 (every 5.x release tested)
*Works in:* 4.0.33 (no `NestMembers` emitted), 6.0.0
*Related:* GROOVY-10687, GROOVY-11780
Under `@CompileStatic`, a closure on the right-hand side of a property
assignment made inside another closure is recorded twice in the enclosing
class's `NestMembers` attribute. Examples: `holder.value = list.collect \{ …
}`, or `value = …` inside `with \{ }`. One entry names the real class; the
other names a class that is never written. Every nested closure compiled after
it in the same class is then listed under the wrong name, one higher than the
class actually written.
Those closures name the outer class as their `NestHost`, but the outer class
doesn't list them, so the JVM rejects their direct calls to its private methods:
{code:java}
java.lang.IllegalAccessError: class Repro$_callPrivate_closure2$_closure4 tried
to access private method
'int Repro.secret(java.lang.Integer)' ... (Type
Repro$_callPrivate_closure2$_closure4 (loader: 'app') is not a
nest member of type Repro (loader: 'app'): current type is not listed as a nest
member){code}
The failing closure can be in any later method of the class, and the method
containing the assignment never has to run. We hit this in a Grails 8
application (Groovy 5.1.3): a `@CompileStatic` service failed at startup
because a closure in an unrelated method used
{code:java}
obj.with { prop = list.collect
{ … } } {code}
h3. Reproducer
Standalone, no build tool:
[https://github.com/jdaugherty/groovy5-issue-nested-closures]
{code:java}
./reproduce.sh 5.1.3 # fails
./reproduce.sh 6.0.0 # passes {code}
{code:java}
import groovy.transform.CompileStatic
@CompileStatic
class Repro {
static class Holder { String value }
// Never called. Compiling it is enough to corrupt the NestMembers
attribute of Repro.
void assign(Holder holder) {
Closure assignment = { -> holder.value = ['a'].collect { it }.join(',')
}
assignment.call()
}
List<Integer> callPrivate() {
Closure<List<Integer>> outer = { -> [1, 2].collect { Integer i ->
secret(i) } }
outer.call()
}
private int secret(Integer i) { i * 10 }
static void main(String[] args) { println new Repro().callPrivate()
}
} {code}
{*}Expected:{*}* prints `[10, 20]`, and `NestMembers` lists exactly the closure
classes written.
{*}Actual (5.1.3):{*}* the `IllegalAccessError` above. Classes written versus
the `NestMembers` attribute:
{code:java}
Classes written NestMembers of Repro
Repro$_assign_closure1 Repro$_assign_closure1
Repro$_assign_closure1$_closure3 Repro$_assign_closure1$_closure3
Repro$_assign_closure1$_closure4
<- never written
Repro$_callPrivate_closure2 Repro$_callPrivate_closure2
Repro$_callPrivate_closure2$_closure4 Repro$_callPrivate_closure2$_closure5
<- written as ...$_closure4 {code}
h3. Scope
On 5.1.3:
- {*}{{*}}Fails:{{*}}{*}
- `\{ -> holder.value = list.collect { … }
}`
- `holder.with { value = list.collect
{ … }
}`
- `holder.with { it.value = list.collect
{ … }
}`
- Whether or not the assignment is the closure's last statement.
- Each such assignment adds one phantom entry; two assignments shift later
names by two.
- {*}{{*}}Passes:{{*}}{*}
- `holder.setValue(list.collect \{ … })` inside a closure.
- Assigning to a local variable, or to a field of the enclosing class, inside
a closure.
- The same property assignment written directly in the method rather than in
a closure.
- A right-hand side with no closure.
- The failing source compiled without `@CompileStatic`, which also produces a
correct `NestMembers`.
Reproduced on JDK 21.0.12 and 25.0.4.
#
## Analysis
`AsmClassGenerator.moreNestmateEntries` doesn't record the closure classes that
are actually generated. It predicts the names of closures nested in closures by
walking each closure's `doCall` body with a `CodeVisitorSupport` and a running
counter (`name + "$_closure" + n++`). The prediction holds only if the walk
visits each `ClosureExpression` exactly once.
For an assignment whose value is kept, static compilation rewrites `a.x = val`
into `def tmp = val; a.setX(tmp); tmp`.
`StaticPropertyAccessHelper.transformToSetterCall` builds that as
`PoppingListOfExpressionsExpression(tmp, setX(tmp))`, so the same
`TemporaryVariableExpression` appears twice in the tree.
`TemporaryVariableExpression.visit` evaluates the wrapped expression only once
when the visitor is an `AsmClassGenerator`. For any other visitor it delegates
straight to the wrapped expression. The nest-member walker therefore counts the
right-hand-side closure twice, while code generation writes it once.
GROOVY-11780 (backported to 5.0.x) doesn't cover this case.
`moreNestmateEntries` and `TemporaryVariableExpression` look unchanged on
master, so we haven't identified the change that makes 6.0.0 pass. Possible
fixes:
- Emit `NestMembers` from the closure classes actually generated rather than
predicted names.
- Have the walker skip `ClosureExpression` nodes it has already counted
(identity-based).
#
## Workaround
Compute the value outside the closure, or call the setter explicitly:
{code:java}
String joined = ['a'].collect { it }.join(',')
holder.with { value = joined }
holder.with { setValue(['a'].collect
{ it }
.join(',')) } {code}
was:
*Affects versions:* 5.0.0 – 5.1.3 (every 5.x release tested)
*Works in:* 4.0.33 (no `NestMembers` emitted), 6.0.0
*Related:* GROOVY-10687, GROOVY-11780
Under `@CompileStatic`, a closure on the right-hand side of a property
assignment made inside another closure is recorded twice in the enclosing
class's `NestMembers` attribute. Examples: `holder.value = list.collect \{ …
}`, or `value = …` inside `with \{ }`. One entry names the real class; the
other names a class that is never written. Every nested closure compiled after
it in the same class is then listed under the wrong name, one higher than the
class actually written.
Those closures name the outer class as their `NestHost`, but the outer class
doesn't list them, so the JVM rejects their direct calls to its private methods:
{code:java}
java.lang.IllegalAccessError: class Repro$_callPrivate_closure2$_closure4 tried
to access private method
'int Repro.secret(java.lang.Integer)' ... (Type
Repro$_callPrivate_closure2$_closure4 (loader: 'app') is not a
nest member of type Repro (loader: 'app'): current type is not listed as a nest
member){code}
The failing closure can be in any later method of the class, and the method
containing the assignment never has to run. We hit this in a Grails 8
application (Groovy 5.1.3): a `@CompileStatic` service failed at startup
because a closure in an unrelated method used
{code:java}
obj.with { prop = list.collect
{ … } } {code}
## Reproducer
Standalone, no build tool:
[https://github.com/jdaugherty/groovy5-issue-nested-closures]
{code:java}
./reproduce.sh 5.1.3 # fails
./reproduce.sh 6.0.0 # passes {code}
{code:java}
import groovy.transform.CompileStatic
@CompileStatic
class Repro {
static class Holder { String value }
// Never called. Compiling it is enough to corrupt the NestMembers
attribute of Repro.
void assign(Holder holder) {
Closure assignment = { -> holder.value = ['a'].collect { it }.join(',')
}
assignment.call()
}
List<Integer> callPrivate() {
Closure<List<Integer>> outer = { -> [1, 2].collect { Integer i ->
secret(i) } }
outer.call()
}
private int secret(Integer i) { i * 10 }
static void main(String[] args) { println new Repro().callPrivate()
}
} {code}
{*}Expected:{*}* prints `[10, 20]`, and `NestMembers` lists exactly the closure
classes written.
{*}Actual (5.1.3):{*}* the `IllegalAccessError` above. Classes written versus
the `NestMembers` attribute:
{code:java}
Classes written NestMembers of Repro
Repro$_assign_closure1 Repro$_assign_closure1
Repro$_assign_closure1$_closure3 Repro$_assign_closure1$_closure3
Repro$_assign_closure1$_closure4
<- never written
Repro$_callPrivate_closure2 Repro$_callPrivate_closure2
Repro$_callPrivate_closure2$_closure4 Repro$_callPrivate_closure2$_closure5
<- written as ...$_closure4 {code}
## Scope
On 5.1.3:
- *{*}Fails:{*}*
- `\{ -> holder.value = list.collect { … }
}`
- `holder.with { value = list.collect
{ … } }`
- `holder.with \{ it.value = list.collect { … }
}`
- Whether or not the assignment is the closure's last statement.
- Each such assignment adds one phantom entry; two assignments shift later
names by two.
- *{*}Passes:{*}*
- `holder.setValue(list.collect \{ … })` inside a closure.
- Assigning to a local variable, or to a field of the enclosing class, inside
a closure.
- The same property assignment written directly in the method rather than in
a closure.
- A right-hand side with no closure.
- The failing source compiled without `@CompileStatic`, which also produces a
correct `NestMembers`.
Reproduced on JDK 21.0.12 and 25.0.4.
#
## Analysis
`AsmClassGenerator.moreNestmateEntries` doesn't record the closure classes that
are actually generated. It predicts the names of closures nested in closures by
walking each closure's `doCall` body with a `CodeVisitorSupport` and a running
counter (`name + "$_closure" + n++`). The prediction holds only if the walk
visits each `ClosureExpression` exactly once.
For an assignment whose value is kept, static compilation rewrites `a.x = val`
into `def tmp = val; a.setX(tmp); tmp`.
`StaticPropertyAccessHelper.transformToSetterCall` builds that as
`PoppingListOfExpressionsExpression(tmp, setX(tmp))`, so the same
`TemporaryVariableExpression` appears twice in the tree.
`TemporaryVariableExpression.visit` evaluates the wrapped expression only once
when the visitor is an `AsmClassGenerator`. For any other visitor it delegates
straight to the wrapped expression. The nest-member walker therefore counts the
right-hand-side closure twice, while code generation writes it once.
GROOVY-11780 (backported to 5.0.x) doesn't cover this case.
`moreNestmateEntries` and `TemporaryVariableExpression` look unchanged on
master, so we haven't identified the change that makes 6.0.0 pass. Possible
fixes:
- Emit `NestMembers` from the closure classes actually generated rather than
predicted names.
- Have the walker skip `ClosureExpression` nodes it has already counted
(identity-based).
#
## Workaround
Compute the value outside the closure, or call the setter explicitly:
```groovy
String joined = ['a'].collect \{ it }.join(',')
holder.with \{ value = joined }
holder.with { setValue(['a'].collect
{ it }
.join(',')) }
```
> closure on the right-hand side of a property assignment inside a closure
> corrupts NestMembers causing IllegalAccessError
> -------------------------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12440
> URL: https://issues.apache.org/jira/browse/GROOVY-12440
> Project: Groovy
> Issue Type: Bug
> Components: class generator, Static compilation
> Affects Versions: 5.0.0
> Reporter: James Daugherty
> Priority: Major
>
> *Affects versions:* 5.0.0 – 5.1.3 (every 5.x release tested)
> *Works in:* 4.0.33 (no `NestMembers` emitted), 6.0.0
> *Related:* GROOVY-10687, GROOVY-11780
> Under `@CompileStatic`, a closure on the right-hand side of a property
> assignment made inside another closure is recorded twice in the enclosing
> class's `NestMembers` attribute. Examples: `holder.value = list.collect \{ …
> }`, or `value = …` inside `with \{ }`. One entry names the real class; the
> other names a class that is never written. Every nested closure compiled
> after it in the same class is then listed under the wrong name, one higher
> than the class actually written.
> Those closures name the outer class as their `NestHost`, but the outer class
> doesn't list them, so the JVM rejects their direct calls to its private
> methods:
>
> {code:java}
> java.lang.IllegalAccessError: class Repro$_callPrivate_closure2$_closure4
> tried to access private method
> 'int Repro.secret(java.lang.Integer)' ... (Type
> Repro$_callPrivate_closure2$_closure4 (loader: 'app') is not a
> nest member of type Repro (loader: 'app'): current type is not listed as a
> nest member){code}
>
> The failing closure can be in any later method of the class, and the method
> containing the assignment never has to run. We hit this in a Grails 8
> application (Groovy 5.1.3): a `@CompileStatic` service failed at startup
> because a closure in an unrelated method used
> {code:java}
> obj.with { prop = list.collect
> { … } } {code}
>
> h3. Reproducer
> Standalone, no build tool:
> [https://github.com/jdaugherty/groovy5-issue-nested-closures]
> {code:java}
> ./reproduce.sh 5.1.3 # fails
> ./reproduce.sh 6.0.0 # passes {code}
>
> {code:java}
> import groovy.transform.CompileStatic
> @CompileStatic
> class Repro {
> static class Holder { String value }
> // Never called. Compiling it is enough to corrupt the NestMembers
> attribute of Repro.
> void assign(Holder holder) {
> Closure assignment = { -> holder.value = ['a'].collect { it
> }.join(',') }
> assignment.call()
> }
> List<Integer> callPrivate() {
> Closure<List<Integer>> outer = { -> [1, 2].collect { Integer i ->
> secret(i) } }
> outer.call()
> }
> private int secret(Integer i) { i * 10 }
> static void main(String[] args) { println new
> Repro().callPrivate() }
> } {code}
> {*}Expected:{*}* prints `[10, 20]`, and `NestMembers` lists exactly the
> closure classes written.
> {*}Actual (5.1.3):{*}* the `IllegalAccessError` above. Classes written versus
> the `NestMembers` attribute:
> {code:java}
> Classes written NestMembers of Repro
> Repro$_assign_closure1 Repro$_assign_closure1
> Repro$_assign_closure1$_closure3 Repro$_assign_closure1$_closure3
> Repro$_assign_closure1$_closure4
> <- never written
> Repro$_callPrivate_closure2 Repro$_callPrivate_closure2
> Repro$_callPrivate_closure2$_closure4
> Repro$_callPrivate_closure2$_closure5 <- written as ...$_closure4 {code}
> h3. Scope
> On 5.1.3:
> - {*}{{*}}Fails:{{*}}{*}
> - `\{ -> holder.value = list.collect { … }
>
> }`
> - `holder.with { value = list.collect
> { … }
> }`
> - `holder.with { it.value = list.collect
> { … }
> }`
> - Whether or not the assignment is the closure's last statement.
> - Each such assignment adds one phantom entry; two assignments shift later
> names by two.
> - {*}{{*}}Passes:{{*}}{*}
> - `holder.setValue(list.collect \{ … })` inside a closure.
> - Assigning to a local variable, or to a field of the enclosing class,
> inside a closure.
> - The same property assignment written directly in the method rather than
> in a closure.
> - A right-hand side with no closure.
> - The failing source compiled without `@CompileStatic`, which also produces
> a correct `NestMembers`.
> Reproduced on JDK 21.0.12 and 25.0.4.
> #
> ## Analysis
> `AsmClassGenerator.moreNestmateEntries` doesn't record the closure classes
> that are actually generated. It predicts the names of closures nested in
> closures by walking each closure's `doCall` body with a `CodeVisitorSupport`
> and a running counter (`name + "$_closure" + n++`). The prediction holds only
> if the walk visits each `ClosureExpression` exactly once.
> For an assignment whose value is kept, static compilation rewrites `a.x =
> val` into `def tmp = val; a.setX(tmp); tmp`.
> `StaticPropertyAccessHelper.transformToSetterCall` builds that as
> `PoppingListOfExpressionsExpression(tmp, setX(tmp))`, so the same
> `TemporaryVariableExpression` appears twice in the tree.
> `TemporaryVariableExpression.visit` evaluates the wrapped expression only
> once when the visitor is an `AsmClassGenerator`. For any other visitor it
> delegates straight to the wrapped expression. The nest-member walker
> therefore counts the right-hand-side closure twice, while code generation
> writes it once.
> GROOVY-11780 (backported to 5.0.x) doesn't cover this case.
> `moreNestmateEntries` and `TemporaryVariableExpression` look unchanged on
> master, so we haven't identified the change that makes 6.0.0 pass. Possible
> fixes:
> - Emit `NestMembers` from the closure classes actually generated rather than
> predicted names.
> - Have the walker skip `ClosureExpression` nodes it has already counted
> (identity-based).
> #
> ## Workaround
> Compute the value outside the closure, or call the setter explicitly:
> {code:java}
> String joined = ['a'].collect { it }.join(',')
> holder.with { value = joined }
> holder.with { setValue(['a'].collect
> { it }
> .join(',')) } {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
