[
https://issues.apache.org/jira/browse/GROOVY-12700?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18139248#comment-18139248
]
ASF GitHub Bot commented on GROOVY-12700:
-----------------------------------------
codecov-commenter commented on PR #3233:
URL: https://github.com/apache/groovy/pull/3233#issuecomment-6672150716
##
[Codecov](https://app.codecov.io/gh/apache/groovy/pull/3233?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
Report
:x: Patch coverage is `93.33333%` with `4 lines` in your changes missing
coverage. Please review.
:white_check_mark: Project coverage is 74.1691%. Comparing base
([`fe4032d`](https://app.codecov.io/gh/apache/groovy/commit/fe4032db75d0e071961bb15d34fe1cf635c856bd?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
to head
([`9b2d8d2`](https://app.codecov.io/gh/apache/groovy/commit/9b2d8d2e22101d7813bee6f81348f7c3b9b44b53?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/3233?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Patch % | Lines |
|---|---|---|
|
[...m/sc/transformers/BinaryExpressionTransformer.java](https://app.codecov.io/gh/apache/groovy/pull/3233?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Ftransform%2Fsc%2Ftransformers%2FBinaryExpressionTransformer.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3RyYW5zZm9ybS9zYy90cmFuc2Zvcm1lcnMvQmluYXJ5RXhwcmVzc2lvblRyYW5zZm9ybWVyLmphdmE=)
| 89.2857% | [0 Missing and 3 partials :warning:
](https://app.codecov.io/gh/apache/groovy/pull/3233?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
|
[.../sc/transformers/StaticCompilationTransformer.java](https://app.codecov.io/gh/apache/groovy/pull/3233?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Ftransform%2Fsc%2Ftransformers%2FStaticCompilationTransformer.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3RyYW5zZm9ybS9zYy90cmFuc2Zvcm1lcnMvU3RhdGljQ29tcGlsYXRpb25UcmFuc2Zvcm1lci5qYXZh)
| 93.3333% | [0 Missing and 1 partial :warning:
](https://app.codecov.io/gh/apache/groovy/pull/3233?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/3233?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #3233 +/- ##
==================================================
+ Coverage 74.1631% 74.1691% +0.0060%
- Complexity 43783 43799 +16
==================================================
Files 1692 1692
Lines 149693 149747 +54
Branches 28406 28417 +11
==================================================
+ Hits 111017 111066 +49
+ Misses 28466 28463 -3
- Partials 10210 10218 +8
```
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/3233?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Coverage Δ | |
|---|---|---|
|
[...org/codehaus/groovy/classgen/ExtendedVerifier.java](https://app.codecov.io/gh/apache/groovy/pull/3233?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2FExtendedVerifier.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL0V4dGVuZGVkVmVyaWZpZXIuamF2YQ==)
| `94.7183% <100.0000%> (+0.9683%)` | :arrow_up: |
|
[.../sc/transformers/ClosureExpressionTransformer.java](https://app.codecov.io/gh/apache/groovy/pull/3233?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Ftransform%2Fsc%2Ftransformers%2FClosureExpressionTransformer.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3RyYW5zZm9ybS9zYy90cmFuc2Zvcm1lcnMvQ2xvc3VyZUV4cHJlc3Npb25UcmFuc2Zvcm1lci5qYXZh)
| `100.0000% <100.0000%> (ø)` | |
|
[.../sc/transformers/StaticCompilationTransformer.java](https://app.codecov.io/gh/apache/groovy/pull/3233?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Ftransform%2Fsc%2Ftransformers%2FStaticCompilationTransformer.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3RyYW5zZm9ybS9zYy90cmFuc2Zvcm1lcnMvU3RhdGljQ29tcGlsYXRpb25UcmFuc2Zvcm1lci5qYXZh)
| `95.1923% <93.3333%> (-0.3133%)` | :arrow_down: |
|
[...m/sc/transformers/BinaryExpressionTransformer.java](https://app.codecov.io/gh/apache/groovy/pull/3233?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Ftransform%2Fsc%2Ftransformers%2FBinaryExpressionTransformer.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3RyYW5zZm9ybS9zYy90cmFuc2Zvcm1lcnMvQmluYXJ5RXhwcmVzc2lvblRyYW5zZm9ybWVyLmphdmE=)
| `93.6345% <89.2857%> (-0.2917%)` | :arrow_down: |
... and [11 files with indirect coverage
changes](https://app.codecov.io/gh/apache/groovy/pull/3233/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test
Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests,
report on failures, and find test suite problems.
- :package: [JS Bundle
Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save
yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
> 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)
