[
https://issues.apache.org/jira/browse/GROOVY-12314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18110274#comment-18110274
]
ASF GitHub Bot commented on GROOVY-12314:
-----------------------------------------
paulk-asert commented on PR #2842:
URL: https://github.com/apache/groovy/pull/2842#issuecomment-5494204819
**Summary of what this PR now includes** (after the review rework; the
branch is squashed to a single commit covering GROOVY-12314 plus a
directly-related GROOVY-12310 fix that builds on the same machinery).
**Runtime / MOP (GROOVY-12314).** Property or attribute access to a field
whose reflective access cannot be forced — e.g. a non-public field of a
strongly encapsulated JDK class, absent `--add-opens` — now degrades to the
normal missing-member handling (`MissingPropertyException` /
`MissingFieldException`, and `ReadOnlyPropertyException` on the write side), as
in 4.x, instead of `GroovyBugError` or a raw `IllegalAccessException`.
Following the review discussion, this ended up with **no cached,
caller-independent accessibility verdict anywhere**: selection in
`MetaClassImpl` no longer predicts what reflection can do, and the reflective
path reports its own failure at the point of use (conversions are gated on an
`IllegalAccessException` cause, so genuine errors still propagate). The
`isAccessEstablishable` API that earlier iterations added to `CachedField` is
gone. `makeAccessible` is attempted at most once, race-safely (the attempt
completes before being recorded — a stress-test-caught fix; the once-only latch
briefly made a concurrent first access observable as a spurious missing
property).
**Runtime / indy.** Per the `ARCHITECTURE.md` note in #2843, the call-site
lookup is the sole access authority: field read and write handles unreflect
against `callSite.getLookup()` (never a lookup fabricated from Groovy's own
module), so e.g. a `FilterReader` subclass reaches the inherited protected `in`
field through its own access rights — the case that motivated the ticket. On
refusal, the handle is left unset and the full MOP adapter path applies; the
earlier retry block that reached around the metaclass is gone. Map semantics
are unaffected by construction: map-entry-over-non-public-field precedence
(GROOVY-5001/5491/11367) is enforced solely by the metaclass ordering, and is
now pinned by an additional `MapTest` case for `HashMap` subclass property
*reads*.
**STC.** Two accessibility gaps behind property syntax are now compile-time
errors, matching javac:
- *GROOVY-12314:* an inaccessible package-private or protected field
declared by the receiver's own class reports `Cannot access field: ... of
class: ... from class: ...` instead of "No such property" (resolution still
falls through to accessor/extension/map/list first). A private field stays
hidden (GROOVY-12290), and an inaccessible inherited field still reports
missing (JLS 8.2; GROOVY-9093, GROOVY-9293).
- *GROOVY-12310:* a member reached through an inaccessible **qualifying
type** — the reported shape is `UIInput.PropertyKeys.localValueSet`, a public
enum constant of a package-private nested enum — previously compiled to a
direct reference that failed at runtime with `IllegalAccessError`; it is now
rejected by the type checker (JLS 6.6.1, via a new `hasAccessToClass` check).
The qualifying type is checked rather than the declaring class, so an inherited
public field remains reachable through an accessible subtype. Dynamic access,
class references, and subscript access are unaffected, and a type-checking
extension can still resolve such a reference dynamically via
`unresolvedProperty`/`makeDynamic`. The `valueOf(...)` method-call variant
needs the same check in method selection and is left for a follow-up.
**Classgen.** The property-access safety-net error is now well-formed: a
synthetic receiver no longer yields `@ line -1, column -1`, and a
type-parameter receiver reports its erasure rather than `E` (while a
parameterized receiver keeps its type arguments — `List<String>`, not
`List<E>`). Separately, the `size`/`length`-to-`size()` rewrite for
`Collection` receivers is dropped: STC no longer admits either name unless a
genuine member backs it, so `list.size` under `@CompileStatic` is now a compile
error rather than a silently rewritten value — a deliberate source-breaking
alignment with dynamic Groovy, which already throws for it (tests using a
genuine `size` field member pin that real members still win).
**Tests.** New `Groovy12314` suite (the JDK-field cases guard themselves
with an assumption and skip under `--add-opens`; permissive dynamic read *and*
write to open classes pinned; public-field-of-package-private-class pinned
through the forced-access retry), new `Groovy12310` suite, the `MapTest`
read-precedence pin, an STC extension resource for the parameterized-receiver
classgen message, and updated package-scope/STC expectations for the new
compile-time errors.
**Behaviour changes to call out:** cross-package field misuse behind
property syntax under STC is now a compile error; `list.size`/`list.length`
under `@CompileStatic` no longer compile; strongly encapsulated JDK fields
report missing instead of crashing. Dynamic Groovy's permissive access to
open/class-path classes is unchanged throughout.
**Follow-ups deliberately out of scope:** the method-path twins of the field
fix (`CachedMethod` forcing access before `Lookup.unreflect` can check it, and
`Java9.checkAccessible` hand-rolling module rules during linkage), the
`valueOf` case of 12310, and removing the `effective*` machinery from indy —
the latter expected to be subsumed by the realm-aware invokedynamic work under
GEP-31 in Groovy 7.
> 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)
