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.
--
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]
