paulk-asert commented on PR #3169: URL: https://github.com/apache/groovy/pull/3169#issuecomment-6595793782
This spike explores the opposite direction to #3137. Instead of rejecting nest-mate private access under `@TypeChecked`, it makes dynamic dispatch reach a private field or method of a superclass from a subclass nested with it, as statically compiled code already does. The fallback in `MetaClassImpl` runs only when the caller's own view has no such member, and only if all of these hold: - the caller (for a closure, its enclosing class) is a subclass of the declaring class, and the receiver is an instance of the caller; - the caller and the declaring class are nest-mates; - the caller is not an inner class whose chain of enclosing instances leads to the declaring class. That case keeps resolving to the enclosing instance (GROOVY-12533). So a nest-mate outside the hierarchy still can't reach the member through a subclass instance, which keeps GROOVY-10198's "private members aren't inherited". A per-metaclass flag keeps the `methodMissing` / `propertyMissing` miss path at a field read; I measured no difference from master. The selected suites pass: `groovy.lang`, `o.c.g.runtime`, STC/SC, `gls`, `bugs`, `groovy.util`, `o.c.g.transform` and `vmplugin`, 14,232 tests in all. **Known gap:** the sender-less property entry points name the receiver's class as the caller. Those are `getProperty(Object, String)`, `getAttribute(Object, String)` and `InvokerHelper.getProperty`. So a caller outside the nest can still reach the field through them. Closing that properly needs the caller (ideally its `Lookup`) carried through the MOP, which fits with GEP-31's lexical, call-site-carried realm context. `Base` declares `private String z` and `private String pm()`. Java results are from `javac` (lambdas stand in for closures). "runtime failure" means `MissingPropertyException`, `MissingMethodException` or `MissingFieldException`. | Access | Java | master (dyn / TC) | spike (dyn / TC) | CS | |---|---|---|---|---| | **nested `static class Sub extends Base`** | | | | | | `this.z` | ✗ private access | runtime failure | ✓ | ✓ | | bare `z` | ✗ static context | runtime failure | ✓ | ✓ | | `super.z` | ✓ | runtime failure | ✓ | ✓ | | `((Base) this).z` | ✓ | runtime failure | ✓ | ✓ | | `that.z` (`Sub that`) | ✗ | runtime failure | ✓ | ✓ | | closure/lambda `this.z` / `super.z` | ✗ / ✓ | runtime failure | ✓ | ✓ | | write `this.z = 'y'` | ✗ | runtime failure | ✓ | ✓ | | `this.@z` | n/a | ✓ | ✓ | ✓ | | `this.pm()` | ✗ cannot find symbol | runtime failure | ✓ | ✓ | | `super.pm()` (also in a closure/lambda) | ✓ | runtime failure | ✓ | ✓ | | bare `pm()` in a closure/lambda | ✗ static context | runtime failure | ✓ | ✓ | | **`SubSub extends Sub`** | | | | | | `this.z` / `((Base) this).z` | ✗ / ✓ | runtime failure | ✓ | ✓ | | **`Other`, a nest-mate but not a subclass** | | | | | | `sub.z` / `sub.pm()` / `sub.@z` | ✗ | runtime failure | runtime failure | ✓ | | `((Base) sub).z` / `((Base) sub).pm()` | ✓ | runtime failure | runtime failure | ✓ | | **class outside the nest** | | | | | | `sub.z` | ✗ | runtime failure | runtime failure | compile error | | **`Sub` declares its own `getZ()` / `propertyMissing`** | n/a | `getZ` / `missing z` | `getZ` / `x` | `getZ` / `x` | | **inner (non-static) `Sub`** | | | | | | bare `z` | ✓ the enclosing instance's | enclosing instance | enclosing instance | enclosing instance | | `this.z` | ✗ | enclosing instance | enclosing instance | enclosing instance | Java decides by the static type of the qualifying expression: `super`, or a cast to `Base`, is allowed; a subclass-typed receiver is not. Dynamic dispatch only sees the runtime class and the caller, so it can't follow that rule exactly. Within the hierarchy the spike is more permissive than Java. The `Other` + cast rows are the one place it's stricter, and there only compile-time resolution (CS, or a direct-invocation tier in GEP-31) could give Java's answer. **Behaviour change to note:** a nested subclass with `propertyMissing` now reads its nest-mate superclass's private field instead of getting the `propertyMissing` call. That matches what CS does. **Possible paths forward** 1. **6.0.x:** keep the current runtime behaviour, and have type checking report the access instead of letting it fail at run time. That's #3137, ideally narrowed later to match this spike's rule (reject only callers outside the receiver's hierarchy) and extended to private methods. No dynamic semantics change in a patch line. 2. **Groovy 7, now:** turn this spike into a real change. Add the matrix as a regression test, close the property gap by giving the sender-less property entry points the same treatment as `invokeMethod(Object, String, Object[])`, and add a COMPATIBILITY.md entry for the `propertyMissing` precedence change. Then drop the #3137 rule on master, since TC's nest-mate leniency becomes correct. 3. **Groovy 7, via GEP-31:** leave dynamic semantics alone for now. Instead, propose that realm-aware call sites carry the caller's `Lookup` alongside the realm token. That gives a reliable caller identity, and nest-mate private access through the JVM's own access checks, without `setAccessible`. Direct-invocation tiers would also cover the cast cases dynamic dispatch can't see. The matrix above would be the acceptance table, and `GROOVY_DYN` would inherit whichever rule we settle on. My preference: (1) for 6.0.x regardless. For 7, (2) is a self-contained improvement available now, but it changes `GROOVY_DYN` semantics, which (3) might then revise again. I'd only take (2) if GEP-31 is unlikely to land in the 7 timeframe. -- 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]
