[
https://issues.apache.org/jira/browse/GROOVY-12700?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18138570#comment-18138570
]
ASF GitHub Bot commented on GROOVY-12700:
-----------------------------------------
paulk-asert opened a new pull request, #3220:
URL: https://github.com/apache/groovy/pull/3220
…t-hand side reads the wrong variable and has the wrong value
Static compilation rewrites a multiple assignment whose right-hand side is a
list literal into single assignments. For a declaration such as def (a, b) =
[1, a], the second value then read the variable declared for the first one
instead of the field or property "a", and the value of the rewritten expression
was its last assignment instead of the list, so a multiple assignment used as a
value, for example as the last statement of a closure, produced a wrong value
or a GroovyCastException.
Keep the rewrite only where the value of the multiple assignment is not
used, that is for an expression statement that is not the implicit return value
of a method or closure; elsewhere the list is evaluated and assigned from, as
dynamic Groovy does. For a declaration whose later values name a variable
declared for an earlier value, evaluate every value into a temporary before
declaring the variables, as the rewrite already did for plain assignments.
> SC: multiple assignment with a list literal on the right-hand side reads the
> wrong variable and has the wrong value
> -------------------------------------------------------------------------------------------------------------------
>
> 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)
