Paul King created GROOVY-12466:
----------------------------------
Summary: 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
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
(v8.20.10#820010)
