[
https://issues.apache.org/jira/browse/GROOVY-12700?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Paul King updated GROOVY-12700:
-------------------------------
Description:
h3. Description
Under {{@CompileStatic}}, a multiple assignment whose right-hand side is a list
literal, such as {{def (a, b) = [1, 2]}} or {{(a, b) = [b, a]}}, is compiled as
a sequence of single assignments. This differs from dynamic Groovy and
{{@TypeChecked}} in two ways.
h4. 1. A later element on the right-hand side can read a variable declared by
the same statement
When an element on the right-hand side, other than the first, uses a name that
the left-hand side declares, the compiled code reads the newly declared local
variable instead of the field or property the name refers to at that point.
Dynamic Groovy and {{@TypeChecked}} read the field or property.
{code:groovy}
class Foo {
String a = 'field a'
static d = 'static d'
//@groovy.transform.CompileStatic
def method() {
def (a, b) = [1, a]
def (c, d, e) = [1, 2, d]
println([a, b, c, d, e])
}
}
def f = new Foo()
f.method()
println f.a
println f.d
{code}
||Mode||Output of {{method()}}||
|dynamic|{{[1, field a, 1, 2, static d]}}|
|{{@TypeChecked}}|{{[1, field a, 1, 2, static d]}}|
|{{@CompileStatic}}|{{[1, 1, 1, 2, 2]}}|
The fields themselves are unchanged in every mode ({{field a}} and {{static d}}
are printed afterwards), so only the values read on the right-hand side differ.
The same happens with typed declarations. With {{String a = 'field a'}}, {{def
(int a, String b) = [1, a]}} returns {{[1, field a]}} dynamically and {{[1,
1]}} under {{@CompileStatic}}: the type checker checks {{String b}} against the
field, but the generated code reads the local {{int a}}.
The first element is not affected ({{def (a, b) = [a, 1]}} reads the field in
every mode), nor is a plain assignment such as {{(a, b) = [b, a]}}, which swaps
correctly in every mode. Ordinary declarators ({{def a = 1, b = a}}) are
evaluated in order in every mode, so {{b}} correctly sees the new local there.
h4. 2. The value of the multiple assignment is the last element, not the list
In dynamic Groovy and {{@TypeChecked}}, the value of a multiple assignment is
the list on the right-hand side. Under {{@CompileStatic}} it is the value of
the last single assignment, while the type checker still gives the expression
the type of the list. Depending on how the value is used, this gives a wrong
value or a {{GroovyCastException}}:
{code:groovy}
@groovy.transform.CompileStatic
class Bar {
def swapLast() { def a = 1, b = 2; (a, b) = [b, a] } // dynamic
[2, 1], SC 1
def declLast() { def (a, b) = [1, 2] } // dynamic
[1, 2], SC 2
def swapUsed() { def a = 1, b = 2; def r = ((a, b) = [b, a]); r }
def fib() { def n = 1, r = 1; 3.times { (n, r) = [r, n + r] }; [n, r] }
}
{code}
||Method||dynamic, {{@TypeChecked}}||{{@CompileStatic}}||
|{{swapLast()}}|{{[2, 1]}}|{{1}}|
|{{declLast()}}|{{[1, 2]}}|{{2}}|
|{{swapUsed()}}|{{[2, 1]}}|{{GroovyCastException: Cannot cast object '1' with
class 'java.lang.Integer' to class 'java.util.ArrayList'}}|
|{{fib()}}|{{[3, 5]}}|{{GroovyCastException}}|
The last case is the easiest to run into: a multiple assignment as the last
statement of a closure is the closure's return value, so a swap inside
{{times}}, {{each}}, {{with}} and similar fails at run time, even though the
return value is not used. Adding a statement after the multiple assignment, or
using a right-hand side that is not a list literal (for example {{[r, n +
r].toList()}}), avoids it.
Reproduced with 4.0.32, 5.1.3, 6.0.0 and master.
h3. Cause
{{BinaryExpressionTransformer#transformMultipleAssignment}} (static
compilation, instruction selection phase) rewrites a multiple assignment with a
list literal on the right-hand side into a {{ListOfExpressionsExpression}}:
* for a declaration, {{def (x, y) = [e1, e2]}} becomes {{def x = e1; def y =
e2}};
* for an assignment, {{(x, y) = [e1, e2]}} becomes {{def t1 = e1; def t2 = e2;
x = t1; y = t2}}.
For a declaration the single declarations are emitted in order, so by the time
{{e2}} is evaluated, {{x}} is already a local variable in scope, and a
{{VariableExpression}} {{x}} in {{e2}} is generated as a read of it (problem
1). The assignment form avoids this by evaluating every element into a
temporary first.
In both forms the value of a {{ListOfExpressionsExpression}} is the value of
its last expression, so the rewritten code no longer produces the list (problem
2).
When the rewrite is skipped (a right-hand side that is not a list literal, or a
left-hand side with a rest or map-style binder), static compilation evaluates
the right-hand side as one value and assigns from it, as dynamic Groovy does,
and neither problem occurs. With the rewrite turned off for list literals as
well, every case above matches dynamic Groovy. That path is slower, though: it
allocates a list, boxes primitive values and reads each element back (for {{int
a, b}}, a swap's method grows from 34 to 180 bytes of bytecode), which is
presumably why the rewrite exists.
h3. Suggested fix
* For declarations, evaluate the right-hand elements into temporaries before
declaring the variables, as the assignment form already does. Primitives stay
unboxed and no list is allocated, so the common case keeps the current
performance.
* Where the value of the multiple assignment can be observed, produce the list:
either fall back to the unsplit path, or end the rewritten expression with a
list of the temporaries. Implicit returns are only made explicit later (by
{{ReturnAdder}} during class generation), so at instruction selection the
transformer has to recognise these value positions itself:
** the multiple assignment is part of a larger expression (for example {{def r
= ((a, b) = [b, a])}}, or an argument);
** it is the last statement of a closure, or of a method whose return type is
not {{void}}.
A multiple assignment used as a statement anywhere else, including as the last
statement of a {{void}} method, cannot have its value observed and can keep the
rewritten form.
was:
h3. Description
Under {{@CompileStatic}}, a multiple-assignment declaration whose right-hand
side is a list literal is compiled as a sequence of single declarations. When
an element on the right-hand side uses a name that the left-hand side declares,
the compiled code reads the newly declared local variable rather than the field
or property that the name refers to at that point. Dynamic Groovy and
{{@TypeChecked}} read the field or property.
{code:groovy}
class Foo {
String a = 'field a'
static d = 'static d'
//@groovy.transform.CompileStatic
def method() {
def (a, b) = [1, a]
def (c, d, e) = [1, 2, d]
println([a, b, c, d, e])
}
}
def f = new Foo()
f.method()
println f.a
println f.d
{code}
||Mode||Output of {{method()}}||
|dynamic|{{[1, field a, 1, 2, static d]}}|
|{{@TypeChecked}}|{{[1, field a, 1, 2, static d]}}|
|{{@CompileStatic}}|{{[1, 1, 1, 2, 2]}}|
The fields themselves are unchanged in every mode ({{field a}} and {{static d}}
are printed afterwards), so only the values read on the right-hand side differ.
The same happens with typed declarations. With {{String a = 'field a'}}, {{def
(int a, String b) = [1, a]}} returns {{[1, field a]}} dynamically and {{[1,
1]}} under {{@CompileStatic}}: the type checker checks {{String b}} against the
field, but the generated code reads the local {{int a}}. In every case the code
compiles without warnings and produces a different value at run time.
Reproduced with 4.0.32, 5.1.3, 6.0.0 and master.
h3. Cause
{{BinaryExpressionTransformer#transformMultipleAssignment}} (static
compilation, instruction selection phase) rewrites {{def (x, y) = [e1, e2]}}
into
{code:groovy}
def x = e1
def y = e2
{code}
For a declaration it emits the single declarations in order, so by the time
{{e2}} is evaluated, {{x}} is already a local variable in scope, and a
{{VariableExpression}} {{x}} in {{e2}} is generated as a read of that local. In
the source, and in the dynamic and type-checked code, every element of the list
is evaluated before any of the variables is declared.
The non-declaration branch of the same method ({{(x, y) = [y, x]}}) already
avoids the corresponding problem for plain assignments by first storing every
right-hand value in a temporary and then assigning the temporaries.
h3. Suggested fix
For declarations, evaluate the right-hand elements into temporaries before
declaring the variables, as the assignment branch does. This could be done only
when an element refers to one of the names being declared, to keep the current
bytecode for the common case.
> SC: multiple-assignment declaration resolves a name on the right-hand side to
> the variable being declared instead of the field or property
> ------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12700
> URL: https://issues.apache.org/jira/browse/GROOVY-12700
> Project: Groovy
> Issue Type: Bug
> Components: Static compilation
> Affects Versions: 4.0.33, 5.1.3, 6.0.0
> Reporter: Paul King
> Priority: Major
>
> h3. Description
> Under {{@CompileStatic}}, a multiple assignment whose right-hand side is a
> list literal, such as {{def (a, b) = [1, 2]}} or {{(a, b) = [b, a]}}, is
> compiled as a sequence of single assignments. This differs from dynamic
> Groovy and {{@TypeChecked}} in two ways.
> h4. 1. A later element on the right-hand side can read a variable declared by
> the same statement
> When an element on the right-hand side, other than the first, uses a name
> that the left-hand side declares, the compiled code reads the newly declared
> local variable instead of the field or property the name refers to at that
> point. Dynamic Groovy and {{@TypeChecked}} read the field or property.
> {code:groovy}
> class Foo {
> String a = 'field a'
> static d = 'static d'
> //@groovy.transform.CompileStatic
> def method() {
> def (a, b) = [1, a]
> def (c, d, e) = [1, 2, d]
> println([a, b, c, d, e])
> }
> }
> def f = new Foo()
> f.method()
> println f.a
> println f.d
> {code}
> ||Mode||Output of {{method()}}||
> |dynamic|{{[1, field a, 1, 2, static d]}}|
> |{{@TypeChecked}}|{{[1, field a, 1, 2, static d]}}|
> |{{@CompileStatic}}|{{[1, 1, 1, 2, 2]}}|
> The fields themselves are unchanged in every mode ({{field a}} and {{static
> d}} are printed afterwards), so only the values read on the right-hand side
> differ.
> The same happens with typed declarations. With {{String a = 'field a'}},
> {{def (int a, String b) = [1, a]}} returns {{[1, field a]}} dynamically and
> {{[1, 1]}} under {{@CompileStatic}}: the type checker checks {{String b}}
> against the field, but the generated code reads the local {{int a}}.
> The first element is not affected ({{def (a, b) = [a, 1]}} reads the field in
> every mode), nor is a plain assignment such as {{(a, b) = [b, a]}}, which
> swaps correctly in every mode. Ordinary declarators ({{def a = 1, b = a}})
> are evaluated in order in every mode, so {{b}} correctly sees the new local
> there.
> h4. 2. The value of the multiple assignment is the last element, not the list
> In dynamic Groovy and {{@TypeChecked}}, the value of a multiple assignment is
> the list on the right-hand side. Under {{@CompileStatic}} it is the value of
> the last single assignment, while the type checker still gives the expression
> the type of the list. Depending on how the value is used, this gives a wrong
> value or a {{GroovyCastException}}:
> {code:groovy}
> @groovy.transform.CompileStatic
> class Bar {
> def swapLast() { def a = 1, b = 2; (a, b) = [b, a] } // dynamic
> [2, 1], SC 1
> def declLast() { def (a, b) = [1, 2] } // dynamic
> [1, 2], SC 2
> def swapUsed() { def a = 1, b = 2; def r = ((a, b) = [b, a]); r }
> def fib() { def n = 1, r = 1; 3.times { (n, r) = [r, n + r] }; [n, r] }
> }
> {code}
> ||Method||dynamic, {{@TypeChecked}}||{{@CompileStatic}}||
> |{{swapLast()}}|{{[2, 1]}}|{{1}}|
> |{{declLast()}}|{{[1, 2]}}|{{2}}|
> |{{swapUsed()}}|{{[2, 1]}}|{{GroovyCastException: Cannot cast object '1' with
> class 'java.lang.Integer' to class 'java.util.ArrayList'}}|
> |{{fib()}}|{{[3, 5]}}|{{GroovyCastException}}|
> The last case is the easiest to run into: a multiple assignment as the last
> statement of a closure is the closure's return value, so a swap inside
> {{times}}, {{each}}, {{with}} and similar fails at run time, even though the
> return value is not used. Adding a statement after the multiple assignment,
> or using a right-hand side that is not a list literal (for example {{[r, n +
> r].toList()}}), avoids it.
> Reproduced with 4.0.32, 5.1.3, 6.0.0 and master.
> h3. Cause
> {{BinaryExpressionTransformer#transformMultipleAssignment}} (static
> compilation, instruction selection phase) rewrites a multiple assignment with
> a list literal on the right-hand side into a {{ListOfExpressionsExpression}}:
> * for a declaration, {{def (x, y) = [e1, e2]}} becomes {{def x = e1; def y =
> e2}};
> * for an assignment, {{(x, y) = [e1, e2]}} becomes {{def t1 = e1; def t2 =
> e2; x = t1; y = t2}}.
> For a declaration the single declarations are emitted in order, so by the
> time {{e2}} is evaluated, {{x}} is already a local variable in scope, and a
> {{VariableExpression}} {{x}} in {{e2}} is generated as a read of it (problem
> 1). The assignment form avoids this by evaluating every element into a
> temporary first.
> In both forms the value of a {{ListOfExpressionsExpression}} is the value of
> its last expression, so the rewritten code no longer produces the list
> (problem 2).
> When the rewrite is skipped (a right-hand side that is not a list literal, or
> a left-hand side with a rest or map-style binder), static compilation
> evaluates the right-hand side as one value and assigns from it, as dynamic
> Groovy does, and neither problem occurs. With the rewrite turned off for list
> literals as well, every case above matches dynamic Groovy. That path is
> slower, though: it allocates a list, boxes primitive values and reads each
> element back (for {{int a, b}}, a swap's method grows from 34 to 180 bytes of
> bytecode), which is presumably why the rewrite exists.
> h3. Suggested fix
> * For declarations, evaluate the right-hand elements into temporaries before
> declaring the variables, as the assignment form already does. Primitives stay
> unboxed and no list is allocated, so the common case keeps the current
> performance.
> * Where the value of the multiple assignment can be observed, produce the
> list: either fall back to the unsplit path, or end the rewritten expression
> with a list of the temporaries. Implicit returns are only made explicit later
> (by {{ReturnAdder}} during class generation), so at instruction selection the
> transformer has to recognise these value positions itself:
> ** the multiple assignment is part of a larger expression (for example {{def
> r = ((a, b) = [b, a])}}, or an argument);
> ** it is the last statement of a closure, or of a method whose return type is
> not {{void}}.
> A multiple assignment used as a statement anywhere else, including as the
> last statement of a {{void}} method, cannot have its value observed and can
> keep the rewritten form.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
