[
https://issues.apache.org/jira/browse/GROOVY-12314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18109656#comment-18109656
]
ASF GitHub Bot commented on GROOVY-12314:
-----------------------------------------
blackdrag commented on code in PR #2842:
URL: https://github.com/apache/groovy/pull/2842#discussion_r3889223204
##########
src/main/java/org/codehaus/groovy/reflection/CachedField.java:
##########
@@ -47,9 +47,29 @@ public CachedField(final Field field) {
private final Field field;
private boolean madeAccessible;
+ private Boolean accessEstablishable;
private void makeAccessible() {
- ReflectionUtils.makeAccessibleInPrivilegedAction(field);
- madeAccessible = true;
+ // GROOVY-12314: only record success; a failed attempt (strongly
encapsulated
+ // declaring class) must not make isAccessEstablishable() report the
field reachable
+ madeAccessible =
ReflectionUtils.makeAccessibleInPrivilegedAction(field).isPresent();
+ }
+
+ /**
+ * Determines if reflective access to the underlying field can actually be
+ * established: accessibility has already been forced, or forcing it can
+ * succeed (the declaring class lives in an open package or module). A
+ * non-public field of a strongly encapsulated class — e.g. a JDK class
+ * without {@code --add-opens} — reports {@code false}, which lets the MOP
+ * treat the field as absent instead of failing when it is read or written.
+ *
+ * @since 6.0.0
+ */
+ public boolean isAccessEstablishable() {
Review Comment:
I think we should change the name as it gives a wrong impression. I suggest
(be aware I am bad at naming) isReflectiveAccessEstablishable because this is
really only and solely for reflective access and maybe made access by
refection... but that again should maybe also be reflected in the name, so
"isReflectiveAccessible". I am sure AI can suggest better names.
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/Selector.java:
##########
@@ -438,14 +438,29 @@ public void chooseMeta(MetaClassImpl mci) {
insertName = true; // pass "name" field as argument
} else if (mp instanceof CachedField && !mp.isStatic()) {
try {
- // GROOVY-9144, GROOVY-9596: get lookup for sender and
unreflect before forcing access
- @SuppressWarnings("removal")
- MethodHandles.Lookup lookup = ((Java8)
VMPluginFactory.getPlugin()).newLookup(sender);
- handle = ((CachedField) mp).asAccessMethod(lookup);
+ // GROOVY-9144, GROOVY-9596: unreflect against the
call-site lookup before forcing access
+ handle = ((CachedField)
mp).asAccessMethod(callSite.getLookup());
} catch (IllegalAccessException e) {
- throw new GroovyBugError(e);
+ // GROOVY-12314: refusal is an access-control outcome, not
an internal
+ // error; invoke the MetaProperty generically like any
other property
+ handle = META_PROPERTY_GETTER.bindTo(mp);
}
} else {
+ // GROOVY-12314: the effective lookup skips fields whose
access reflection
+ // cannot force, but that is a property of the reflective
(Field.get) path;
+ // the call-site lookup carries the caller's own access
rights, exactly as
+ // the bytecode a Java compiler would emit, so it may still
reach e.g. an
+ // inherited protected field (FilterReader#in from a
subclass). The lookup
+ // decides: no access rules are re-implemented here.
+ MetaProperty rawMp = mci.getMetaProperty(name);
+ if (rawMp instanceof CachedField cf && !cf.isStatic() &&
!cf.isAccessEstablishable()) {
Review Comment:
you can access the field only with an `asAccessMethod`, if the lookup allows
it. What `cf.isAccessEstablishable()` says to it is of no relevance. Also, when
is mp no CachedField, but rawMp is? I guess that is what the comment is for and
I guess this is now needed because of the changes to MetaClassImpl. But then
maybe this here should have the inverse logic. first get the property via
mci.getMetaProperty(name); handle the cases from that and then only get the
effective property to do the generic fallback. Frankly I think we should get
rid of the effective* stuff in indy, but that is beyond the scope of this PR.
##########
src/main/java/groovy/lang/MetaClassImpl.java:
##########
@@ -3267,6 +3283,9 @@ public void setAttribute(final Class sender, final Object
object, final String a
if (mp instanceof MetaBeanProperty mbp) {
mp = mbp.getField();
}
+ if (mp != null && !isAccessEstablishable(mp)) { // GROOVY-12314:
unreachable field reports missing
Review Comment:
you could consider moving the null check inside isAccessEstablishable, but
that is minor
##########
src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java:
##########
@@ -883,8 +877,14 @@ private boolean setField(final PropertyExpression
expression, final Expression o
}*/
private void addPropertyAccessError(final Expression receiver, final
String propertyName, final ClassNode receiverType) {
- String receiverName = (receiver instanceof ClassExpression ?
receiver.getType() : receiverType).toString(false);
- String message = "Access to " + receiverName + "#" + propertyName + "
is forbidden";
- controller.getSourceUnit().addError(new SyntaxException(message,
receiver));
+ ClassNode receiverNode = (receiver instanceof ClassExpression ?
receiver.getType() : receiverType);
+ if (receiverNode.isGenericsPlaceHolder()) receiverNode =
receiverNode.redirect(); // GROOVY-12314: report the erasure, not "E"
Review Comment:
why not simply always do `receiverNode = receiverNode.redirect();`?
> 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)
