[
https://issues.apache.org/jira/browse/GROOVY-12464?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Paul King updated GROOVY-12464:
-------------------------------
Description:
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}} and not
widely used, so I think we can add this in a 6.0.x patch release. However
builds can newly fail, so worth a note in the release notes.
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.
was:
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.
> 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
> Assignee: Paul King
> Priority: Major
>
> 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}} and not
> widely used, so I think we can add this in a 6.0.x patch release. However
> builds can newly fail, so worth a note in the release notes.
> 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
(v10.3.26#10030026)
