Paul King created GROOVY-12480:
----------------------------------
Summary: PurityChecker: mutation through a local that aliases a
parameter or field is not detected, and mutating a value built locally is
refused when written as a named call
Key: GROOVY-12480
URL: https://issues.apache.org/jira/browse/GROOVY-12480
Project: Groovy
Issue Type: Improvement
Components: groovy-typecheckers
Reporter: Paul King
h2. Problem
GROOVY-12464 makes PurityChecker treat mutating operators ({{<<}}, subscript
stores, GEP-15 {{*Assign}} methods) like the method calls they stand for, but
only when the receiver is a parameter, a field or property, {{this}}, or a
variable captured by a checked closure. Locals were left alone so that building
up a result in a local list, a common idiom in pure code, keeps passing. That
leaves two inconsistencies, probed with GROOVY-12464 applied:
||Inside a @Pure method||Result||
|{{def t = s; t << v}} with {{s}} a {{List}} parameter|*passes*, although the
caller's list changes|
|{{def t = items; t << v}} with {{items}} a static field|*passes*, although the
field's list changes|
|{{def t = s; t.add(v)}}|refused: "call to 't.add()' — purity cannot be
verified"|
|{{List<Integer> s = []; s << v}}|passes|
|{{List<Integer> s = []; s.add(v)}}|*refused*, although only a list the method
created changes|
|{{def sb = new StringBuilder(); sb.append(v)}}|*refused*|
|{{def r = xs.collect \{ ... \}; r << 0}}|passes|
|{{def r = xs.collect \{ ... \}; r.add(0)}}|*refused*|
So the operator form misses mutation through an alias, while the named-call
form refuses mutation that is harmless. Each verdict depends on how the
mutation is written, not on whose object changes.
h2. Proposed fix
Decide by whether the local is *fresh*: it holds an object the method created,
and no other reference to that object can exist.
* A pre-pass over each checked method (and each checked closure) marks a local
as fresh when it is initialised with a fresh value and every later assignment
to it is also a fresh value, including assignments inside closures and multiple
assignment.
* Operators and named calls then use the same rule:
** On a fresh local, a mutating operator or an otherwise-unverified call is
accepted.
** On a local that is not fresh, it is checked as it would be on a parameter,
so {{def t = s; t << v}} is refused.
* The receiver-type checks are unchanged and still apply to fresh locals:
logging, metrics, I/O and non-deterministic types report unless allowed, and
constructing an I/O or non-deterministic type is still reported where it
happens.
h2. Open questions
# *Which types can be mutated through a fresh local?* Freshness proves the
object is not shared, not that its methods have no effects outside it: {{def t
= new Thread(r); t.start()}} is fresh. Proposed: accept unverified calls on a
fresh local only when its type is a JDK collection or map ({{java.util}}),
{{StringBuilder}}/{{StringBuffer}}, or an array. Keep the warning for other
types, unless the callee is {{@Pure}}.
# *Which initialisers are fresh?* Literals ({{[]}}, {{[:]}}, array literals)
and constructor calls are fresh. If only those count, {{def r = xs.collect \{
... \}; r << 0}}, which passes today, would be refused, because a method's
return value is not known to be fresh. Proposed: also treat as fresh the
results of the GDK and stream methods that build a new collection without
changing their receiver ({{collect}}, {{findAll}}, {{toList}}, {{toSorted}},
{{toUnique}}, {{collectEntries}}, {{groupBy}} and similar), reusing the
existing {{CLOSURE_PURE_METHODS}} list where it applies.
# *Reassignment.* Proposed: any reassignment to a value that is not fresh makes
the local not fresh. A reassignment to a fresh value ({{acc = []}}) keeps it
fresh. Results of {{+}} on collections are not treated as fresh in a first
version.
h2. Compatibility
The change goes in both directions: mutation through an alias is newly refused,
and named calls on fresh locals are newly accepted. With a narrow answer to
question 2, code that mutates a local initialised from a method call would also
be newly refused. PurityChecker is {{@Incubating}}; like GROOVY-12464 this
suits 6.1.0 rather than a 6.0.x release.
h2. Tests
* Refused: {{def t = s; t << v}}, {{t\[0\] = v}} and {{t += v}} (with
{{plusAssign}}) for a parameter {{s}}; the same through an alias of a field; an
alias assigned later ({{def t = []; t = s; t << v}}); an alias assigned inside
a closure.
* Accepted: {{s.add(v)}}, {{s << v}}, {{s\[0\] = v}} on a list literal or {{new
ArrayList<>()}}; {{sb.append(v)}} on a new {{StringBuilder}}; the same after
reassigning to a fresh value.
* Depending on question 1: {{t.start()}} on {{new Thread(r)}} still warns.
* Depending on question 2: {{r << 0}} and {{r.add(0)}} on the result of
{{xs.collect \{ ... \}}}.
* Effect categories still apply to fresh locals, e.g. {{new StringWriter()}}
followed by a write is judged by the existing I/O rules.
* The docs list "mutation through aliases" as not detected; update them.
h2. Related
Follow-up to GROOVY-12464, whose description suggests this rule.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
