[
https://issues.apache.org/jira/browse/GROOVY-12223?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18101427#comment-18101427
]
ASF GitHub Bot commented on GROOVY-12223:
-----------------------------------------
daniellansun commented on PR #2755:
URL: https://github.com/apache/groovy/pull/2755#issuecomment-5168731102
@blackdrag Thanks for the careful review — those points were well taken.
Here is how the current design addresses them.
## Test-only API on `HiddenClassDefiner`
Agreed. The extra helpers that existed only for tests (`findConstructor`,
strict multi-option `define*` overloads, etc.) have been removed. Production
surface is now essentially:
- `isEnabled()` / the disable property
- `tryDefineNestmate(Lookup, bytes, initialize)` — preferred
- `tryDefineNestmate(Class, bytes, initialize)` — best-effort for foreign
hosts only
## Duplication with `ProxyGeneratorAdapter` / `ReflectorLoader`
Also agreed that define policy should not be reimplemented at each call
site. Class definition (package alignment, `NESTMATE` + weak lifecycle,
soft-fail → `null`) lives only in `HiddenClassDefiner`.
- `ReflectorLoader` / `ClassLoaderForClassArtifacts` are thin: one
`tryDefineNestmate` + visible `defineClass` fallback.
- Proxy-specific *policy* (when a proxy may be hidden, which host to try,
MockFor-style “must stay nameable” aggregates) was pulled out of
`ProxyGeneratorAdapter` into package-private `ProxyClassDefiner`, so the
adapter stays focused on bytecode generation and no longer reimplements the
define path.
## `MethodHandles.lookup()` and private module access
Your reading of the javadoc matches how we now structure the API.
A `Lookup` only carries full privilege for the class that actually called
`MethodHandles.lookup()`. Capturing a lookup inside `HiddenClassDefiner` (or
any other utility) cannot magically grant full access into arbitrary hosts such
as `java.lang.String`. Relying on that for private/module access would be the
wrong model.
So the intended split is:
1. **Preferred:** `tryDefineNestmate(Lookup, …)` where the **nest-host
class** owns the lookup, e.g.
`private static final Lookup LOOKUP = MethodHandles.lookup();`
on `ReflectorLoader` / `ProxyGeneratorAdapter`. That is the correct
caller-sensitive pattern.
2. **Best-effort only:** `tryDefineNestmate(Class, …)` for **foreign** hosts
(user classes we do not control). It uses `privateLookupIn` from
`HiddenClassDefiner` and is documented to succeed only when the host package is
accessible to Groovy’s module (typical for unnamed application classes; not a
general substitute for a host-created `Lookup`). Callers must always handle
`null` and fall back to `ClassLoader.defineClass`.
We no longer expose a public `HiddenClassDefiner.privateLookupIn` that would
suggest “full access for any class from a utility.” That intent and usability
are no longer conflated.
Happy to adjust naming or docs further if anything still feels misleading.
> Introduce hidden class support
> ------------------------------
>
> Key: GROOVY-12223
> URL: https://issues.apache.org/jira/browse/GROOVY-12223
> Project: Groovy
> Issue Type: Improvement
> Reporter: Daniel Sun
> Priority: Major
>
> h2. Background
> Groovy generates many short-lived synthetic classes at runtime, including:
> * map/interface proxies ({{ProxyGeneratorAdapter}})
> * reflection dispatch helpers ({{Reflector}} / {{ReflectorLoader}})
> * per-class meta-method artifacts ({{ClassLoaderForClassArtifacts}})
> Today these are defined with {{ClassLoader#defineClass}} as ordinary *named*
> classes. That has three practical downsides:
> # *Name pollution* — the synthetic types are discoverable via
> {{Class.forName}} / {{ClassLoader#loadClass}}.
> # *Metaspace pressure* — their lifetime is tied to the defining class loader;
> long-running applications that generate many artifacts retain them until the
> loader itself is collected.
> # *Access friction* — without nest membership, generated code cannot share
> private access with the host class the way a true nestmate can.
> JDK 15 introduced *hidden classes* ([JEP 371|https://openjdk.org/jeps/371]):
> classes defined through {{Lookup#defineHiddenClass}} that are
> non-discoverable by name, may join an access-control nest ({{NESTMATE}}), and
> may be unloaded independently of the defining loader when not marked
> {{STRONG}}.
> Groovy 6 requires JDK 17+, so the API is always present on supported runtimes.
> h2. Proposal
> Centralise hidden-class definition behind a single utility and prefer it for
> the dynamic class-generation sites listed above, with a transparent fallback
> to the existing {{ClassLoader#defineClass}} path.
> h3. New API
> {{org.apache.groovy.util.HiddenClassDefiner}} — the only call-site that
> invokes {{Lookup#defineHiddenClass}}:
> * {{defineHiddenClass(lookup, bytes, initialize, nestmate, strong)}} — full
> control
> * {{defineNestmateClass(lookup, bytes, initialize)}} — nestmate + weak
> lifecycle (default for proxies / reflectors / artifacts)
> * {{defineStrongHiddenClass(lookup, bytes, initialize)}} — non-discoverable,
> loader-tied lifetime
> * helpers: {{privateLookupIn(hostClass)}}, {{findConstructor(hiddenClass,
> ...parameterTypes)}}
> Kill-switch (evaluated once at class-init for hot-path cost):
> {noformat}
> -Dgroovy.hidden.classes.disable=true
> {noformat}
> When disabled (or when private lookup / definition fails), callers fall back
> to defining a normal visible class.
> h3. Integration points
> || Site || Nest host || Preferred options || Fallback ||
> | {{ClassLoaderForClassArtifacts#define}} | target (klazz) | nestmate, weak |
> {{ClassLoader#defineClass}} + protection domain |
> | {{ProxyGeneratorAdapter}} | non-{{Object}} superclass if present; else
> {{ProxyGeneratorAdapter}} | nestmate, weak | {{InnerLoader#defineClass}} |
> | {{ReflectorLoader#defineClass}} | {{Reflector}} | nestmate, weak |
> {{ClassLoader#defineClass}} + protection domain |
> Behaviour for callers of these generators is unchanged: proxies still
> implement the requested interfaces, reflectors still dispatch, artifacts
> still construct. The only observable differences when the hidden path
> succeeds are the synthetic name form (contains {{/}}) and {{Class#isHidden()
> == true}}.
> h2. Benefits
> * Non-discoverable synthetic types (cleaner class-space / tooling view).
> * Nestmate private access where the nest host can be opened for private
> lookup.
> * Eager unloading of weak hidden classes reduces long-run metaspace retention
> for short-lived proxies and artifacts.
> * One policy / upgrade point if future JDKs add further
> {{Lookup.ClassOption}} values.
> h2. Compatibility
> * Default-on when the JVM can obtain a full-privilege lookup for the chosen
> nest host; silent fallback otherwise (e.g. sealed / unopened module packages).
> * Opt-out: {{-Dgroovy.hidden.classes.disable=true}}.
> * No public language-surface change; no change to successful proxy /
> reflector / artifact *behaviour*, only to how the {{Class}} is defined.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
