[
https://issues.apache.org/jira/browse/GROOVY-12466?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18130681#comment-18130681
]
ASF GitHub Bot commented on GROOVY-12466:
-----------------------------------------
paulk-asert opened a new pull request, #3011:
URL: https://github.com/apache/groovy/pull/3011
…method is refused for final parameters and for final fields used inside
their own class
When static type checking resolves a compound assignment to a *Assign method
the receiver is mutated in place, so a final target is not reassigned. Two
checks refused it anyway. VariableScopeVisitor reported any assignment operator
on a final parameter; it runs before type checking, so it now reports plain
assignment only and leaves compound operators to the existing
FinalVariableAnalyzer check in Verifier, which already skips a resolved *Assign
target. ClassCompletionVerifier reported any assignment operator on a final
field named directly or through this; it now skips assignments that carry
COMPOUND_ASSIGN_TARGET.
The classic code generation used by @TypeChecked still stored the result
back into the left-hand side, which would update a final field outside its
constructor (rejected by the JVM) and, for a final property reached from
outside its class, failed with ReadOnlyPropertyException. Both the indy and the
ScriptBytecodeAdapter paths now skip the store when the *Assign target is
resolved, matching static compilation. As documented, the setter is then not
invoked for a property target under @TypeChecked either.
Dynamic code is unchanged apart from the message for a final parameter,
which is now "The parameter [x] is declared final but is reassigned".
> 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)
