paulk-asert commented on PR #2842:
URL: https://github.com/apache/groovy/pull/2842#issuecomment-5486439840
@blackdrag Response below:
> Good questions — taking them in turn.
>
> **When is `mp` not a CachedField but `rawMp` is?** Exactly when
`getEffectiveGetMetaProperty` skips a field it would otherwise have chosen.
Your guess is right that the GROOVY-12314 change to MetaClassImpl created the
main path here (a non-public field whose access reflection cannot force now
falls through to the generic property, while `getMetaProperty` still returns
the raw CachedField) — but there is also a pre-existing one, which is what the
condition is really guarding; see below.
>
> **On `cf.isAccessEstablishable()` being of no relevance:** agreed the name
was the problem — renamed to `isReflectiveAccessEstablishable` per your
suggestion (along with the other two comments) in <commit>. But the check isn't
deciding whether the lookup may access the field — the lookup alone decides
that, in `asAccessMethod`. It narrows the retry to "the metaclass chose this
field and dropped it *only* because reflection cannot force it". Without that
narrowing, the retry also fires when the metaclass deliberately preferred
something else over the field. Concrete case: a Map subclass with a
package-private field. `getEffectiveGetMetaProperty` returns the map-get
property *before* it ever considers the non-public field (the GROOVY-11367
ordering), which lands us in this else branch with `rawMp` the CachedField:
>
> ```groovy
> class M extends HashMap<String, String> {
> @PackageScope String value = 'field'
> }
> def m = new M()
> m.put('value', 'map')
> assert m.value == 'map'
> ```
>
> Drop the condition and a same-package caller's lookup unreflects the field
happily — field access silently wins over `map.get`. I verified both
directions: the assert passes today and fails with the condition removed. Happy
to add this to the PR as a test so the guard's purpose is pinned in code rather
than in this thread.
>
> **On the inverse logic:** I looked at it, but `getMetaProperty(name)` is a
bare index lookup — no sender/selectionBase (the GROOVY-5438 private-field
selection), no getter-over-field precedence, no visibility check, no
map/category ordering. "Handling the cases from that" in the Selector would
mean re-deriving policy the metaclass already encodes, which cuts against the
direction of the rest of this review: the metaclass decides *selection*, the
lookup decides *access*.
--
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]
