[
https://issues.apache.org/jira/browse/GROOVY-12466?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18130712#comment-18130712
]
ASF GitHub Bot commented on GROOVY-12466:
-----------------------------------------
codecov-commenter commented on PR #3011:
URL: https://github.com/apache/groovy/pull/3011#issuecomment-6358739920
##
[Codecov](https://app.codecov.io/gh/apache/groovy/pull/3011?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 `80.00000%` with `1 line` in your changes missing
coverage. Please review.
:white_check_mark: Project coverage is 72.4381%. Comparing base
([`8d2980f`](https://app.codecov.io/gh/apache/groovy/commit/8d2980fd1249cb24b22b91524a4ad71c2c25fb45?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
to head
([`e744b99`](https://app.codecov.io/gh/apache/groovy/commit/e744b99ec54829a3d873409118f588aa4da802bd?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on master.
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/3011?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Patch % | Lines |
|---|---|---|
|
[...us/groovy/classgen/asm/BinaryExpressionHelper.java](https://app.codecov.io/gh/apache/groovy/pull/3011?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2Fasm%2FBinaryExpressionHelper.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL2FzbS9CaW5hcnlFeHByZXNzaW9uSGVscGVyLmphdmE=)
| 0.0000% | [0 Missing and 1 partial :warning:
](https://app.codecov.io/gh/apache/groovy/pull/3011?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/3011?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #3011 +/- ##
==================================================
- Coverage 72.4417% 72.4381% -0.0036%
Complexity 40682 40682
==================================================
Files 1644 1644
Lines 142897 142900 +3
Branches 26877 26880 +3
==================================================
- Hits 103517 103514 -3
- Misses 29825 29826 +1
- Partials 9555 9560 +5
```
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/3011?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Coverage Δ | |
|---|---|---|
|
[...ehaus/groovy/classgen/ClassCompletionVerifier.java](https://app.codecov.io/gh/apache/groovy/pull/3011?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2FClassCompletionVerifier.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL0NsYXNzQ29tcGxldGlvblZlcmlmaWVyLmphdmE=)
| `85.0000% <100.0000%> (+0.0268%)` | :arrow_up: |
|
[...codehaus/groovy/classgen/VariableScopeVisitor.java](https://app.codecov.io/gh/apache/groovy/pull/3011?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2FVariableScopeVisitor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL1ZhcmlhYmxlU2NvcGVWaXNpdG9yLmphdmE=)
| `94.1781% <100.0000%> (ø)` | |
|
[...dehaus/groovy/classgen/asm/indy/IndyBinHelper.java](https://app.codecov.io/gh/apache/groovy/pull/3011?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2Fasm%2Findy%2FIndyBinHelper.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL2FzbS9pbmR5L0luZHlCaW5IZWxwZXIuamF2YQ==)
| `96.9697% <100.0000%> (+0.0947%)` | :arrow_up: |
|
[...us/groovy/classgen/asm/BinaryExpressionHelper.java](https://app.codecov.io/gh/apache/groovy/pull/3011?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2Fasm%2FBinaryExpressionHelper.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL2FzbS9CaW5hcnlFeHByZXNzaW9uSGVscGVyLmphdmE=)
| `89.0688% <0.0000%> (-0.1204%)` | :arrow_down: |
... and [12 files with indirect coverage
changes](https://app.codecov.io/gh/apache/groovy/pull/3011/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>
> GEP-15: compound assignment that resolves to a *Assign method is refused for
> final parameters and for final fields used inside their own class
> ----------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12466
> URL: https://issues.apache.org/jira/browse/GROOVY-12466
> Project: Groovy
> Issue Type: Bug
> Components: Compiler, Static Type Checker
> Affects Versions: 6.0.0
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> h2. Problem
> GEP-15 lets a class define {{plusAssign}}, {{minusAssign}},
> {{leftShiftAssign}} and the other {{*Assign}} methods. The 6.0 documentation
> (core-operators, "compound assignment") says that when the compiler resolves
> such a method statically, the receiver "is mutated in place, no reassignment
> occurs (so {{final}} fields and variables can use compound assignment in that
> case)". That holds for final local variables and for final fields reached
> from outside their class. It does not hold for final parameters, or for final
> fields used inside their own class:
> {code:groovy}
> @CompileStatic
> class Acc {
> int total
> void plusAssign(int v) { total += v }
> }
> @CompileStatic
> class H {
> final Acc acc = new Acc()
> int viaField() { acc += 1; acc.total } // error: cannot modify
> final field 'acc' outside of constructor.
> int viaThis() { this.acc += 1; acc.total } // error: cannot modify
> final field 'acc' outside of constructor.
> static int viaParam(final Acc a) { a += 1; a.total } // error: Cannot
> assign a value to final variable 'a'
> static int viaLocal(Acc x) { final Acc a = x; a += 1; a.total } //
> compiles and runs: 1
> }
> @CompileStatic
> int outside() { def h = new H(); h.acc += 1; h.acc.total } // compiles and
> runs: 1
> {code}
> Results on 6.0.0:
> ||Target of {{+= 1}}||@CompileStatic||@TypeChecked||dynamic||
> |final local variable|compiles, runs|compiles, runs|refused (expected)|
> |final field from outside ({{h.acc}})|compiles, runs|compiles, runs|n/a|
> |final field inside its class ({{acc}} or
> {{this.acc}})|*refused*|*refused*|refused (expected)|
> |final parameter|*refused*|*refused*|refused (expected)|
> The dynamic rows are expected. The documentation says the final check cannot
> be relaxed there, because the compiler doesn't know whether a {{*Assign}}
> method will respond at runtime. The {{CompoundAssignmentTest}} cases cover
> the final local and the outside-field forms, but not these two.
> h2. Cause
> Two checks reject the assignment without considering the {{*Assign}} target
> that static type checking records in
> {{StaticTypesMarker.COMPOUND_ASSIGN_TARGET}}:
> * *Final parameters:* {{VariableScopeVisitor.checkFinalFieldAccess}} reports
> "Cannot assign a value to final variable" for any assignment operator whose
> target is a final {{Parameter}}. It runs during semantic analysis, before
> type checking, so it cannot know whether a {{*Assign}} method will be found.
> The later check in {{Verifier}} (the {{FinalVariableAnalyzer}} callback)
> already skips expressions carrying {{COMPOUND_ASSIGN_TARGET}}, for parameters
> as well as locals, but is never reached.
> * *Final fields:* {{ClassCompletionVerifier.checkFinalFieldAccess}} reports
> "cannot modify final field ... outside of constructor" for any assignment
> operator whose target is a final field named directly or through {{this}}. It
> runs after type checking, so the marker is available, but it is not
> consulted. A field reached through another object ({{h.acc}}) passes only
> because this check ignores properties on anything other than {{this}}.
> h2. Proposed fix
> * In {{VariableScopeVisitor}}, report final parameters only for plain
> assignment ({{=}}). Leave compound operators on final parameters to the
> {{FinalVariableAnalyzer}} check in {{Verifier}}, which already reports "The
> parameter \[a\] is declared final but is reassigned" when no {{*Assign}}
> target was resolved, in dynamic code or when the type has no such method. The
> error is still reported in those cases, with that message instead of the
> current one.
> * In {{ClassCompletionVerifier}}, skip the final-field error when the
> assignment's {{BinaryExpression}} carries {{COMPOUND_ASSIGN_TARGET}}. The
> check currently receives only the left-hand expression, so it needs the
> binary expression passed in as well.
> * Confirm that code generation emits no field store or local store for these
> cases, in both static code generation and the classic code generation used by
> {{@TypeChecked}}. A store to a final field outside a constructor would fail
> bytecode verification. The final-local and outside-field cases show the
> call-only path already works.
> h2. Tests
> Extend {{CompoundAssignmentTest}}:
> * Final parameter with {{plusAssign}}, under {{@CompileStatic}} and
> {{@TypeChecked}}: compiles, mutates the argument in place, and does not
> rebind the parameter.
> * Final instance field and final static field, used by simple name and
> through {{this}} inside their own class: compiles and mutates in place.
> * {{leftShiftAssign}} and an {{@OperatorRename}}-renamed {{*Assign}} method,
> for the same targets.
> * Still refused: the same forms in dynamic code; a type with only {{plus}}
> and no {{plusAssign}}; plain {{=}} to a final parameter or field outside a
> constructor.
> * Still allowed: assignment to a final field inside a constructor.
> h2. Related
> Found while checking PurityChecker against GEP-15 operators. That is a
> separate ticket: PurityChecker does not treat {{a += v}} that resolves to
> {{plusAssign}} as a mutation of {{a}}.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
