Paul King created GROOVY-12464:
----------------------------------
Summary: PurityChecker: mutation through operators (<<, subscript
assignment, GEP-15 compound-assignment methods such as plusAssign) is not
detected, while the equivalent method calls are
Key: GROOVY-12464
URL: https://issues.apache.org/jira/browse/GROOVY-12464
Project: Groovy
Issue Type: Bug
Components: groovy-typecheckers
Affects Versions: 6.0.0
Reporter: Paul King
h2. Problem
Inside a {{@Pure}} method, PurityChecker refuses a mutating method call on a
parameter or field, but accepts the same mutation written as an operator.
Probed on 6.0.0 with {{@TypeChecked(extensions =
'groovy.typecheckers.PurityChecker')}}:
||Inside a @Pure method||Result||
|{{s.add(v)}} on a {{List}} parameter|refused: "call to 's.add()' — purity
cannot be verified"|
|{{s.leftShift(v)}} on a {{List}} parameter|refused|
|{{s << v}} on a {{List}} parameter|*passes*|
|{{sb.append(v)}} on a {{StringBuilder}} parameter|refused|
|{{sb << v}} on a {{StringBuilder}} parameter|*passes*|
|{{s.putAt(0, v)}} on a {{List}} parameter|refused|
|{{s\[0\] = v}} on a {{List}} parameter|*passes*|
|{{m\["k"\] = v}} on a {{Map}} parameter|*passes*|
|{{m.k = v}} on a {{Map}} parameter|refused: "property assignment to 'm.k'"|
|{{s\[0\]++}} on a {{List}} parameter|*passes*|
|{{s\[0\] += v}} on a {{List}} parameter|*passes*|
|{{items.add(v)}} on a static {{List}} field|refused|
|{{items << v}} on a static {{List}} field|*passes*|
|{{items += v}} on a static {{List}} field|refused: "field assignment to
'items'"|
|{{a.plusAssign(v)}} on a parameter whose class defines {{plusAssign}}|refused|
|{{a += v}} on that parameter (GEP-15: calls {{plusAssign}}, mutating in
place)|*passes*|
|{{a <<= v}} on a parameter whose class defines {{leftShiftAssign}}|*passes*|
|{{a += v}} with {{@OperatorRename(plusAssign = "addInPlace")}}|*passes*|
Each row marked *passes* changes an object the method does not own: a caller's
argument or a field. So a method that the checker accepts as {{@Pure}} can
still have side effects, and anything relying on the annotation, such as
{{@Memoized}}, parallel evaluation or a reader of the code, is misled.
{code:groovy}
@TypeChecked(extensions = 'groovy.typecheckers.PurityChecker')
class Example {
@Pure
static int record(List<Integer> sink, int v) {
sink << v // accepted today; sink.add(v) is refused
v
}
}
{code}
The GEP-15 rows matter because the same source means different things depending
on the receiver's type. Since Groovy 6.0, when static type checking resolves a
{{*Assign}} method ({{plusAssign}}, {{minusAssign}}, {{leftShiftAssign}} and
the rest), {{a += v}} calls it and mutates {{a}} in place. A probe confirmed
that the caller's object changes. Only when no {{*Assign}} method exists does
{{a += v}} mean {{a = a.plus(v)}}, which merely rebinds the local variable.
Not affected, and correct as is:
* {{s += v}} on a {{List}} parameter: the GDK defines no {{*Assign}} methods,
so this builds a new list and rebinds the local parameter variable, and the
caller's list is unchanged. If a {{*Assign}} extension for {{List}} were added
later, the fix below would cover it automatically.
* Pure operators such as {{a + b}} on {{BigDecimal}} parameters.
h2. Cause
{{PurityChecker}}'s visitor only looks at a {{BinaryExpression}} when its
operator is an assignment, and then only to report a write to a field or a
property ({{checkFieldWrite}}).
* A subscript target ({{s\[0\] = v}}) is neither a {{PropertyExpression}} nor a
{{VariableExpression}}, so nothing is reported. The postfix and prefix visitors
have the same gap for {{s\[0\]++}}.
* {{<<}} is not an assignment, so the expression is ignored, even though static
type checking resolves it to {{leftShift}}. Operator forms never reach
{{checkInstanceCall}}, which is what refuses the named calls.
* For a compound assignment whose target is a parameter or local variable,
{{checkFieldWrite}} finds no field, so nothing is reported, even when static
type checking has recorded a {{*Assign}} target
({{StaticTypesMarker.COMPOUND_ASSIGN_TARGET}}) and the code will mutate the
receiver in place. For a field or property target the existing "field
assignment" message fires, which is the right outcome even though the wording
describes a reassignment.
h2. Proposed fix
Treat the mutating operator forms like the method calls they stand for:
* {{a << b}}: check it as the call {{a.leftShift(b)}}, using the target static
type checking resolved for the expression.
* Subscript assignment {{a\[i\] = v}}, compound subscript assignment ({{a\[i\]
+= v}} and similar) and subscript {{++}}/{{--}}: check them as {{a.putAt(i,
...)}} on the receiver {{a}}.
* A compound assignment carrying {{COMPOUND_ASSIGN_TARGET}} metadata: check it
as a call to the recorded method on the receiver. That covers every GEP-15
operator, extension-method {{*Assign}} implementations and {{@OperatorRename}}
without listing names.
Other operators ({{+}}, {{-}}, comparisons and so on) stay as they are, so this
adds no new reports for pure arithmetic.
h3. Local variables
Today {{s << v}}, {{s\[0\] = v}} and a GEP-15 {{a += v}} pass on an object
created inside the method, while {{s.add(v)}} on the same local list is
refused. Building a result in a local list is a common idiom in pure code.
Applying the fix to locals would make it fail, so the minimal fix applies only
when the receiver is a parameter, a field, a property or {{this}}, and leaves
locals as they are.
That leaves one gap: a local that aliases a parameter ({{def t = s; t << v}}).
Closing it properly needs a freshness rule, accepting mutation only of locals
initialised with a literal or a constructor call and never reassigned. That
rule should then also apply to the named calls, so {{s.add(v)}} on a fresh
local list would be accepted too. It is suggested as a follow-up rather than
part of this fix.
h2. Compatibility
The fix reports problems in code that compiles today, so builds using
PurityChecker may start failing. PurityChecker is {{@Incubating}}, but because
builds can newly fail, 6.1.0 seems the right target rather than a 6.0.x patch
release.
h2. Tests
* Each *passes* row in the table above is refused after the fix, with a message
naming the operator and its receiver.
* The refused rows stay refused, and {{s += v}} on a list parameter and
{{BigDecimal}} arithmetic still pass.
* The local-list forms ({{s << v}}, {{s\[0\] = v}} on a list created in the
method) still pass.
* GEP-15: {{a += v}} and {{a <<= v}} on a parameter whose class defines
{{plusAssign}}/{{leftShiftAssign}} are refused, including with
{{@OperatorRename}}; {{a += v}} on a type without {{*Assign}} still passes.
* The same cases with {{@Pure}} graded to allow some effects behave
consistently with the named-call forms.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
