[
https://issues.apache.org/jira/browse/GROOVY-12464?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18130714#comment-18130714
]
ASF GitHub Bot commented on GROOVY-12464:
-----------------------------------------
codecov-commenter commented on PR #3012:
URL: https://github.com/apache/groovy/pull/3012#issuecomment-6358858763
##
[Codecov](https://app.codecov.io/gh/apache/groovy/pull/3012?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
Report
:x: Patch coverage is `61.90476%` with `24 lines` in your changes missing
coverage. Please review.
:white_check_mark: Project coverage is 72.4416%. Comparing base
([`fa53afa`](https://app.codecov.io/gh/apache/groovy/commit/fa53afab4ff811b7ed93560300cd626d0374842e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
to head
([`dd64a3e`](https://app.codecov.io/gh/apache/groovy/commit/dd64a3e1a60c9047142d977cb9875b467ecb4b71?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/3012?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Patch % | Lines |
|---|---|---|
|
[...in/groovy/groovy/typecheckers/PurityChecker.groovy](https://app.codecov.io/gh/apache/groovy/pull/3012?src=pr&el=tree&filepath=subprojects%2Fgroovy-typecheckers%2Fsrc%2Fmain%2Fgroovy%2Fgroovy%2Ftypecheckers%2FPurityChecker.groovy&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VicHJvamVjdHMvZ3Jvb3Z5LXR5cGVjaGVja2Vycy9zcmMvbWFpbi9ncm9vdnkvZ3Jvb3Z5L3R5cGVjaGVja2Vycy9QdXJpdHlDaGVja2VyLmdyb292eQ==)
| 61.9048% | [9 Missing and 15 partials :warning:
](https://app.codecov.io/gh/apache/groovy/pull/3012?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/3012?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #3012 +/- ##
==================================================
- Coverage 72.4452% 72.4416% -0.0036%
+ Complexity 40687 40683 -4
==================================================
Files 1644 1644
Lines 142897 142933 +36
Branches 26877 26896 +19
==================================================
+ Hits 103522 103543 +21
- Misses 29820 29823 +3
- Partials 9555 9567 +12
```
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/3012?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Coverage Δ | |
|---|---|---|
|
[...in/groovy/groovy/typecheckers/PurityChecker.groovy](https://app.codecov.io/gh/apache/groovy/pull/3012?src=pr&el=tree&filepath=subprojects%2Fgroovy-typecheckers%2Fsrc%2Fmain%2Fgroovy%2Fgroovy%2Ftypecheckers%2FPurityChecker.groovy&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VicHJvamVjdHMvZ3Jvb3Z5LXR5cGVjaGVja2Vycy9zcmMvbWFpbi9ncm9vdnkvZ3Jvb3Z5L3R5cGVjaGVja2Vycy9QdXJpdHlDaGVja2VyLmdyb292eQ==)
| `71.1864% <61.9048%> (+3.6189%)` | :arrow_up: |
... and [12 files with indirect coverage
changes](https://app.codecov.io/gh/apache/groovy/pull/3012/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test
Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests,
report on failures, and find test suite problems.
- :package: [JS Bundle
Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save
yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
> 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
> 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}}, 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
(v10.3.26#10030026)
