[jira] [Created] (GROOVY-12480) 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

["Paul King (Jira)" <ji...@apache.org>]

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)