[
https://issues.apache.org/jira/browse/GROOVY-12700?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18139267#comment-18139267
]
ASF GitHub Bot commented on GROOVY-12700:
-----------------------------------------
paulk-asert merged PR #3233:
URL: https://github.com/apache/groovy/pull/3233
> 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)
