[
https://issues.apache.org/jira/browse/GROOVY-12314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18109567#comment-18109567
]
ASF GitHub Bot commented on GROOVY-12314:
-----------------------------------------
codecov-commenter commented on PR #2842:
URL: https://github.com/apache/groovy/pull/2842#issuecomment-5466342472
##
[Codecov](https://app.codecov.io/gh/apache/groovy/pull/2842?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 `80.00000%` with `8 lines` in your changes missing
coverage. Please review.
:white_check_mark: Project coverage is 70.6683%. Comparing base
([`83ecaa0`](https://app.codecov.io/gh/apache/groovy/commit/83ecaa04ddaa2ff6f8ef2f90228c1ecdd82549d2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
to head
([`4e7b0d1`](https://app.codecov.io/gh/apache/groovy/commit/4e7b0d11adeb37ac3e9690c0ca2a2fedb00a6680?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on master.
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/2842?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Patch % | Lines |
|---|---|---|
|
[...ovy/classgen/asm/sc/StaticTypesCallSiteWriter.java](https://app.codecov.io/gh/apache/groovy/pull/2842?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2Fasm%2Fsc%2FStaticTypesCallSiteWriter.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL2FzbS9zYy9TdGF0aWNUeXBlc0NhbGxTaXRlV3JpdGVyLmphdmE=)
| 37.5000% | [2 Missing and 3 partials :warning:
](https://app.codecov.io/gh/apache/groovy/pull/2842?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
|
[...java/org/codehaus/groovy/vmplugin/v8/Selector.java](https://app.codecov.io/gh/apache/groovy/pull/2842?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fvmplugin%2Fv8%2FSelector.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3ZtcGx1Z2luL3Y4L1NlbGVjdG9yLmphdmE=)
| 77.7778% | [2 Missing :warning:
](https://app.codecov.io/gh/apache/groovy/pull/2842?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
|
[...roovy/transform/stc/StaticTypeCheckingVisitor.java](https://app.codecov.io/gh/apache/groovy/pull/2842?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Ftransform%2Fstc%2FStaticTypeCheckingVisitor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3RyYW5zZm9ybS9zdGMvU3RhdGljVHlwZUNoZWNraW5nVmlzaXRvci5qYXZh)
| 88.8889% | [0 Missing and 1 partial :warning:
](https://app.codecov.io/gh/apache/groovy/pull/2842?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/2842?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #2842 +/- ##
==================================================
+ Coverage 70.6561% 70.6683% +0.0123%
- Complexity 36541 36584 +43
==================================================
Files 1571 1571
Lines 133997 134070 +73
Branches 24697 24728 +31
==================================================
+ Hits 94677 94745 +68
+ Misses 30803 30798 -5
- Partials 8517 8527 +10
```
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/2842?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Coverage Δ | |
|---|---|---|
|
[src/main/java/groovy/lang/MetaClassImpl.java](https://app.codecov.io/gh/apache/groovy/pull/2842?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fgroovy%2Flang%2FMetaClassImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9ncm9vdnkvbGFuZy9NZXRhQ2xhc3NJbXBsLmphdmE=)
| `79.3296% <100.0000%> (+0.0695%)` | :arrow_up: |
|
[...va/org/codehaus/groovy/reflection/CachedField.java](https://app.codecov.io/gh/apache/groovy/pull/2842?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Freflection%2FCachedField.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3JlZmxlY3Rpb24vQ2FjaGVkRmllbGQuamF2YQ==)
| `65.9574% <100.0000%> (+9.1393%)` | :arrow_up: |
|
[...roovy/transform/stc/StaticTypeCheckingVisitor.java](https://app.codecov.io/gh/apache/groovy/pull/2842?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Ftransform%2Fstc%2FStaticTypeCheckingVisitor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3RyYW5zZm9ybS9zdGMvU3RhdGljVHlwZUNoZWNraW5nVmlzaXRvci5qYXZh)
| `87.2821% <88.8889%> (+0.0219%)` | :arrow_up: |
|
[...java/org/codehaus/groovy/vmplugin/v8/Selector.java](https://app.codecov.io/gh/apache/groovy/pull/2842?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fvmplugin%2Fv8%2FSelector.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3ZtcGx1Z2luL3Y4L1NlbGVjdG9yLmphdmE=)
| `81.0811% <77.7778%> (+0.0964%)` | :arrow_up: |
|
[...ovy/classgen/asm/sc/StaticTypesCallSiteWriter.java](https://app.codecov.io/gh/apache/groovy/pull/2842?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2Fasm%2Fsc%2FStaticTypesCallSiteWriter.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL2FzbS9zYy9TdGF0aWNUeXBlc0NhbGxTaXRlV3JpdGVyLmphdmE=)
| `76.3044% <37.5000%> (-0.3695%)` | :arrow_down: |
... and [9 files with indirect coverage
changes](https://app.codecov.io/gh/apache/groovy/pull/2842/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>
> Align field-backed property access across compilation modes
> -----------------------------------------------------------
>
> Key: GROOVY-12314
> URL: https://issues.apache.org/jira/browse/GROOVY-12314
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
> Labels: breaking
>
> Found while investigating GROOVY-12305 (see also GROOVY-12290, GROOVY-9967):
> a family of edge cases where property access that resolves to a *field*
> behaved differently under dynamic Groovy, {{@TypeChecked}} and
> {{@CompileStatic}} — different values, different failure modes, or silently
> wrong values. Each item is individually minor; taken together they justify
> aligning the three modes even though some behavior changes result.
> h3. Behavior before this ticket (measured on 4.0.33, 5.1.1, 6.0.0-beta-3 and
> master)
> *A. Non-public fields of foreign (JDK) classes*
> {code:groovy}
> def list = [1, 2]
> println list.modCount // protected field inherited from
> java.util.AbstractList
> println list.elementData // package-private field of java.util.ArrayList
> println list.@modCount // attribute access
> {code}
> ||access (dynamic, no add-opens)||4.0.33||5.1.1||beta-3 / master||
> |{{list.modCount}} (protected)|MissingPropertyException|*GroovyBugError*
> "BUG! UNCAUGHT EXCEPTION: member is protected"|*GroovyBugError*|
> |{{list.elementData}}
> (package-private)|MissingPropertyException|MissingPropertyException|MissingPropertyException|
> |{{list.@modCount}}|MissingFieldException|raw IllegalAccessException|raw
> IllegalAccessException|
> The GroovyBugError was a 4→5 regression:
> {{Selector$PropertySelector.chooseMeta}} wrapped the refused
> {{MethodHandles}} lookup in GroovyBugError (vmplugin/v8/Selector.java,
> GROOVY-9144/9596 code). It was not catchable as MissingPropertyException and
> presented as an internal bug. The {{.@}} error-shape change
> (MissingFieldException → raw IllegalAccessException escaping from
> {{CachedField.getProperty}}) was likewise a 4→5 regression.
> The static modes for the same expressions: {{@TypeChecked}} compiled the
> property reads ({{storeField}} admitted the fields — {{isFieldAccessible}}'s
> exact-receiver leniency still covered package-private, and {{storeField}}
> deliberately proceeded for inaccessible protected fields), then failed at
> runtime as above. {{@CompileStatic}} failed during class generation with
> {{Access to E#modCount is forbidden @ line -1, column -1}} — an unresolved
> type parameter as the receiver name and no source position.
> With {{--add-opens java.base/java.util=ALL-UNNAMED}}, every spelling above
> works and prints the field value, on all versions.
> *B. Collection {{size}}/{{length}} classgen shortcut*
> {{StaticTypesCallSiteWriter#makeGetPropertySite}} rewrote {{size}}/{{length}}
> on Collection receivers to {{size()}} *before* getter/map-rule/field lookup.
> Measured under {{@CompileStatic}} (the dynamic column is identical on all
> four versions):
> {code:groovy}
> class C { int getSize() { 999 } } // Groovy class
> // JColl: Java class extends ArrayList<Object> with public int size = 42,
> public int length = 99
> {code}
> ||scenario (CS)||4.0.33||5.1.1||beta-3 / master||dynamic (all versions)||
> |{{List l = \[1,2\]; l.size}}|2|2|STC error|MissingPropertyException|
> |{{l.length}}|STC error|STC error|STC error|MissingPropertyException|
> |Groovy class with {{getSize()}}: {{c.size}}|999|999|999|999|
> |Groovy class, public field {{size=42}}: {{d.size}}|42|42|42|42|
> |{{List<C> l; l.size}} (element {{getSize()}})|*2*|*2*|\[999, 999\]|\[999,
> 999\]|
> |Java class, public field {{size=42}}: {{j.size}}|*1*|*1*|*1*|42|
> |Java class, public field {{length=99}}: {{j.length}}|*1*|*1*|*1*|99|
> Notes:
> * The shortcut's only mainstream feeder was STC resolving {{l.size}} to
> ArrayList's *private* {{int size}} field via the exact-receiver leniency —
> closed by GROOVY-12290 — which is why rows 1 and 5 changed in 6.0.0-beta-3
> (aligning with dynamic semantics).
> * Row 5 on 4.x/5.x was a three-way divergence: dynamic gives \[999, 999\];
> {{@TypeChecked}} returned \[999, 999\] at runtime while statically typing the
> expression {{int}} (so {{int n = l.size}} type-checked cleanly then threw
> GroovyCastException); {{@CompileStatic}} gave 2.
> * Rows 6–7 were silently wrong values on *every* version: STC legitimately
> admits the public field, but the shortcut hijacked the access to {{size()}}.
> Groovy-class receivers were immune because
> {{makeGroovyObjectGetPropertySite}} has no such shortcut — the result
> differed depending on whether the receiver class was written in Java or
> Groovy.
> h3. Changes made (one commit each)
> # *MOP*: a field whose reflective access cannot be established (new
> {{CachedField#isAccessEstablishable}}, backed by {{checkCanSetAccessible}})
> is treated as absent during meta-property selection — property get/set and
> attribute get/set — so the normal missing-member handling applies. The indy
> selectors degrade to the generic MetaProperty or the sender-aware adapter
> path instead of throwing GroovyBugError. Forceable access (open modules,
> class-path classes, {{--add-opens}}) is unaffected. This commit alone fixes
> the 5.x GroovyBugError/IllegalAccessException regressions and is a back-port
> candidate for GROOVY_5_0_X.
> # *STC*: the GROOVY-12290 rule extended — the exact-receiver leniency no
> longer admits plain property syntax to *any* field of a foreign nest that
> Java access rules reject (was: private only), and an inaccessible protected
> field no longer backs a property at all. Resolution falls through to
> accessors, extensions or the map/list handling, so the checker rejects at
> compile time what the dynamic MOP reports as missing at run time. Escape
> hatches unchanged: attribute access, closure bodies, delegate-resolved
> access, nest-mates, and everything Java admits (same package, protected from
> a subclass).
> # *Classgen*: the Collection {{size}}/{{length}}-to-{{size()}} rewrite
> removed. Post-GROOVY-12290 it was vestigial, and the cases still reaching it
> produced wrong values; a public {{size}}/{{length}} field now resolves
> through the normal field handling.
> # *Tests*: the two {{DifferentPackageTest}} scenarios now expect the
> positioned type-checking error ("No such property") instead of class
> generation's "Access to ... is forbidden".
> # *Classgen*: the safety-net "Access to ... is forbidden" error now reports
> the placeholder's erasure (was "E") and falls back to the current statement's
> position (was line -1, column -1).
> # *indy*: the metaclass skips fields reflection cannot force, but that
> constraint belongs to the classic ({{Field.get}}) access path only — a sender
> that passes Java's access rules (e.g. a {{FilterReader}} subclass reading the
> protected {{in}} field) still reaches the field through its own
> {{MethodHandles}} lookup, exactly like javac-emitted bytecode. When the
> effective meta property comes back as a fallback, the property-get selector
> retries the raw field via the sender lookup before binding the fallback.
> (Selection itself must stay sender-blind: classic API calls pass the receiver
> class as the sender, which would otherwise grant phantom privileges.)
> h3. Resulting behavior (verified)
> ||scenario (no add-opens)||dynamic||@TypeChecked||@CompileStatic||
> |{{list.size}} / {{list.length}}|MissingPropertyException|STC error|STC error|
> |{{list.modCount}} read (protected, foreign sender)|MissingPropertyException
> (was BUG!)|STC error (was compiles→BUG!)|STC error (was line -1 classgen
> error)|
> |{{list.elementData}} read (package-private)|MissingPropertyException|STC
> error (was compiles→runtime MPE)|STC error (was line -1 classgen error)|
> |{{list.modCount}} write|ReadOnlyPropertyException (was BUG!-adjacent; 4.x
> threw IllegalArgumentException)|STC error|STC error|
> |{{list.@modCount}} read / write|MissingFieldException (was raw IAE)|STC
> error "Cannot access field" (unchanged)|STC error (unchanged)|
> |protected field of super class from a *subclass* (e.g.
> {{FilterReader#in}})|works (sender lookup)|works|works|
> |Groovy {{getSize()}} / Groovy public field / element spread|999 / 42 /
> \[999, 999\]|999 / 42 / \[999, 999\]|999 / 42 / \[999, 999\]|
> |Java public field {{size}}/{{length}}|42 / 99|42 / 99|42 / 99 (was 1)|
> ||with --add-opens||dynamic||@TypeChecked||@CompileStatic||
> |{{list.modCount}}|field value (kept)|STC error|STC error|
> |{{list.@modCount}}|field value|field value|field value|
> All three modes agree on every row: the same value, or the static modes
> rejecting at compile time exactly what dynamic reports as missing at run
> time. All failures are well-formed — catchable
> MissingPropertyException/MissingFieldException/ReadOnlyPropertyException at
> runtime, positioned STC errors at compile time; no GroovyBugError, no "line
> -1" classgen errors. The one deliberate asymmetry: with {{--add-opens}},
> dynamic property syntax can still read a protected field, while the static
> modes require the explicit {{.@}} spelling.
> h3. Behavior changes (accepted as the price of alignment)
> * {{@TypeChecked}}/{{@CompileStatic}} code reading package-private/protected
> foreign fields via property syntax stops compiling (previously it failed at
> runtime or with a malformed classgen error; {{.@}} remains rejected
> statically as before, and dynamic {{.@}} works under {{--add-opens}}).
> * Cross-package {{@PackageScope}} field misuse now fails during type checking
> with "No such property" instead of during class generation with "Access to
> ... is forbidden".
> * {{@CompileStatic}} on a Java Collection class with a public
> {{size}}/{{length}} field changes from the element count to the field value
> (bug fix, but observable).
> * A dynamic property *write* to a strongly encapsulated field now throws
> ReadOnlyPropertyException (accurate: the field exists but cannot be written
> that way; previously a raw IllegalAccessException-based failure,
> IllegalArgumentException on 4.x).
> * Already shipped in 6.0.0-beta-3 via GROOVY-12290, noted here for the
> migration notes: {{list.size}} under {{@CompileStatic}} is now a compile
> error; on 4.x/5.x it compiled and returned the element count.
> Validated with the full core test suite (17,244 tests) plus the complete
> scenario matrix above run against 4.0.33, 5.1.1, 6.0.0-beta-3 and the patched
> build, with and without {{--add-opens}}.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
