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();`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
