Copilot commented on code in PR #2755: URL: https://github.com/apache/groovy/pull/2755#discussion_r3704807536
########## src/test/groovy/org/codehaus/groovy/reflection/ClassLoaderForClassArtifactsTest.groovy: ########## @@ -0,0 +1,101 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.codehaus.groovy.reflection + +import org.apache.groovy.util.HiddenClassDefiner +import org.junit.jupiter.api.Test +import org.objectweb.asm.ClassWriter + +import static org.junit.jupiter.api.Assertions.assertEquals +import static org.junit.jupiter.api.Assertions.assertFalse +import static org.junit.jupiter.api.Assertions.assertNotNull +import static org.junit.jupiter.api.Assertions.assertNull +import static org.junit.jupiter.api.Assertions.assertTrue +import static org.objectweb.asm.Opcodes.ACC_PUBLIC +import static org.objectweb.asm.Opcodes.ALOAD +import static org.objectweb.asm.Opcodes.INVOKESPECIAL +import static org.objectweb.asm.Opcodes.RETURN +import static org.objectweb.asm.Opcodes.V17 + +/** + * Covers the hidden-class path in {@link ClassLoaderForClassArtifacts}. + */ +class ClassLoaderForClassArtifactsTest { + + static class Host { + // nest host for generated artifacts + } + + private static byte[] minimalBytes(String internalName) { + def cw = new ClassWriter(0) + cw.visit(V17, ACC_PUBLIC, internalName, null, 'java/lang/Object', null) + def mv = cw.visitMethod(ACC_PUBLIC, '<init>', '()V', null, null) + mv.visitCode() + mv.visitVarInsn(ALOAD, 0) + mv.visitMethodInsn(INVOKESPECIAL, 'java/lang/Object', '<init>', '()V', false) + mv.visitInsn(RETURN) + mv.visitMaxs(1, 1) + mv.visitEnd() + cw.visitEnd() + cw.toByteArray() + } + + @Test + void testDefinePrefersHiddenNestmateOfTarget() { + def loader = new ClassLoaderForClassArtifacts(Host) + String name = loader.createClassName('artifact') + Class<?> cls = loader.define(name, minimalBytes(name.replace('.', '/'))) + assertNotNull(cls) + if (HiddenClassDefiner.isEnabled()) { + assertTrue(cls.isHidden()) + assertEquals(Host.nestHost, cls.nestHost) + assertEquals(Host.packageName, cls.packageName) + } + assertNotNull(cls.getDeclaredConstructor().newInstance()) + } + + @Test + void testDefineClassAndGetConstructor() { + def loader = new ClassLoaderForClassArtifacts(Host) + String name = loader.createClassName('withCtor') + def ctor = loader.defineClassAndGetConstructor(name, minimalBytes(name.replace('.', '/'))) + assertNotNull(ctor) + assertNotNull(ctor.newInstance()) + // Missing constructor signature → null + assertNull(loader.defineClassAndGetConstructor( + loader.createClassName('missing'), + minimalBytes('org/codehaus/groovy/reflection/Missing'), + String)) + } + + @Test + void testCreateClassNameUniquenessAndJavaPrefix() { + def loader = new ClassLoaderForClassArtifacts(Host) + String first = loader.createClassName('m') + String second = loader.createClassName('m') + assertTrue(first.contains(Host.name)) + assertTrue(first != second || second.endsWith('$0') || second.contains('$')) + + def javaLoader = new ClassLoaderForClassArtifacts(String) Review Comment: The uniqueness assertion is effectively always true because `second.contains('$')` will always hold for these generated names (the base form always includes a `$` before the method name). This makes the test non-validating. Replace it with a strict uniqueness check (`assertNotEquals(first, second)`) and/or assert an expected suffix pattern for the second name (e.g., ends with `$0`). ########## src/main/java/org/codehaus/groovy/reflection/ClassLoaderForClassArtifacts.java: ########## @@ -18,71 +18,146 @@ */ package org.codehaus.groovy.reflection; +import org.apache.groovy.util.HiddenClassDefiner; + import java.lang.ref.SoftReference; import java.lang.reflect.Constructor; import java.lang.reflect.Method; import java.util.concurrent.atomic.AtomicInteger; +/** + * A specialized {@link ClassLoader} used to define per-class <em>artifact</em> + * classes such as generated meta-method dispatchers. + * + * <p>Since Groovy 6.0 this loader first attempts to define each artifact as a + * <em>hidden nestmate</em> of the target class ({@link HiddenClassDefiner}): + * <ul> + * <li>non-discoverable by name — no class-space pollution;</li> + * <li>same defining loader / package / protection domain as the target, so + * references to the target resolve correctly even under custom loaders;</li> + * <li>nestmate of the target (mutual private access);</li> + * <li>weak lifecycle — eligible for eager unloading once the {@link Class} + * object is unreachable, reducing metaspace pressure in long-running + * applications that generate many per-class artifacts.</li> + * </ul> + * + * <p>If hidden-class definition is disabled or fails (module not open, target + * is a hidden/primitive/array type, linkage error, …) the loader falls back to + * the traditional {@link ClassLoader#defineClass} path transparently. + */ public class ClassLoaderForClassArtifacts extends ClassLoader { + + /** Soft reference to the class for which artifacts are generated. */ public final SoftReference<Class> klazz; + + /** + * Counter used to ensure unique class names when multiple artifacts are + * generated for the same method name. + */ private final AtomicInteger classNamesCounter = new AtomicInteger(-1); - public ClassLoaderForClassArtifacts(Class klazz) { + /** + * Creates a new artifact class loader for the specified class. + * + * @param klazz the class whose artifact classes are to be defined via this loader + */ + public ClassLoaderForClassArtifacts(final Class klazz) { super(klazz.getClassLoader()); this.klazz = new SoftReference<>(klazz); } - public Class define(String name, byte[] bytes) { - Class cls = defineClass(name, bytes, 0, bytes.length, klazz.get().getProtectionDomain()); + // ------------------------------------------------------------------------- + // Class definition + // ------------------------------------------------------------------------- + + /** + * Defines a class from bytecode, preferring a hidden nestmate of the target. + * + * @param name the binary name used for the fallback (visible-class) path + * @param bytes the class-file bytes + * @return the defined class + */ + public Class define(final String name, final byte[] bytes) { + final Class<?> host = klazz.get(); + if (host != null) { + final Class<?> hidden = HiddenClassDefiner.tryDefineNestmate(host, bytes, false); + if (hidden != null) { + return hidden; + } + } + + // Fallback: visible class with the target's protection domain + final Class<?> cls = defineClass( + name, bytes, 0, bytes.length, + host != null ? host.getProtectionDomain() : null); Review Comment: When `klazz.get()` has been cleared, the fallback path defines the class with a `null` `ProtectionDomain`, which can change security behavior vs. defining with the target’s protection domain (and also differs from the previous behavior, which would have failed instead of silently changing domains). Consider capturing and storing the host’s `ProtectionDomain` (and likely the host’s name/package info used by `createClassName`) strongly in the constructor so artifact definition remains deterministic even if the `SoftReference` is cleared; alternatively, fail fast if the host was reclaimed. ########## src/test/groovy/groovy/util/ProxyGeneratorAdapterTest.groovy: ########## @@ -283,4 +285,130 @@ class ProxyGeneratorAdapterTest { proxy.run() assert calls == 2 } + + // ------------------------------------------------------------------------- + // Hidden-class-specific tests (since Groovy 6.0 / JEP 371) + // ------------------------------------------------------------------------- + + /** + * Concrete abstract superclasses only reference types visible from the + * host loader, so the hidden nestmate path must succeed when enabled. + */ + @Test + void testProxyIsDefinedAsHiddenClass() { + if (!HiddenClassDefiner.isEnabled()) return + + def map = ['bar': { }] + ProxyGeneratorAdapter adapter = new ProxyGeneratorAdapter(map, Bar, null, this.class.classLoader, false, null) + assertTrue(adapter.isProxyHidden(), + 'Concrete-super proxy must be a hidden class when hidden classes are enabled') + assert adapter.proxy(map) instanceof Bar + } + + /** + * Interface aggregates (Object super + user interfaces, no typed delegate) + * must stay <em>visible</em>: MockFor/StubFor re-wrap them and need a + * nameable binary type for the {@code $delegate} field. + */ + @Test + void testInterfaceAggregateIsNotHidden() { + if (!HiddenClassDefiner.isEnabled()) return + + def map = [:] + ProxyGeneratorAdapter adapter = new ProxyGeneratorAdapter( + map, Object, [Iterator] as Class[], this.class.classLoader, false, null) + assertFalse(adapter.isProxyHidden(), + 'Interface aggregates must remain nameable for MockFor re-wrapping') + def obj = adapter.proxy(map) + assert obj instanceof Iterator + assertFalse(obj.getClass().isHidden()) + } + + /** + * A proxy defined as a hidden class must report {@link Class#isHidden()} as + * {@code true} and must not be discoverable via {@code Class.forName()}. + */ + @Test + void testHiddenProxyIsNotDiscoverableByName() { + if (!HiddenClassDefiner.isEnabled()) return + + def map = ['bar': { }] + ProxyGeneratorAdapter adapter = new ProxyGeneratorAdapter(map, Bar, null, this.class.classLoader, false, null) + if (!adapter.isProxyHidden()) return + + Class<?> proxyCls = adapter.proxy(map).getClass() + assertTrue(proxyCls.isHidden(), 'Proxy class must report isHidden() == true') + assertThrows(ClassNotFoundException) { + Class.forName(proxyCls.getName()) + } Review Comment: `Class.forName(proxyCls.getName())` will throw for hidden classes largely because their `Class#getName()` contains a `/0x...` suffix and is not a valid binary name, so this doesn’t strongly validate “not discoverable by name.” A more robust assertion is to attempt loading the *binary-name prefix* (substring before `/`) via the relevant loader and assert CNFE; that checks actual non-discoverability rather than name format. ########## src/main/java/org/codehaus/groovy/reflection/ClassLoaderForClassArtifacts.java: ########## @@ -18,71 +18,146 @@ */ package org.codehaus.groovy.reflection; +import org.apache.groovy.util.HiddenClassDefiner; + import java.lang.ref.SoftReference; import java.lang.reflect.Constructor; import java.lang.reflect.Method; import java.util.concurrent.atomic.AtomicInteger; +/** + * A specialized {@link ClassLoader} used to define per-class <em>artifact</em> + * classes such as generated meta-method dispatchers. + * + * <p>Since Groovy 6.0 this loader first attempts to define each artifact as a + * <em>hidden nestmate</em> of the target class ({@link HiddenClassDefiner}): + * <ul> + * <li>non-discoverable by name — no class-space pollution;</li> + * <li>same defining loader / package / protection domain as the target, so + * references to the target resolve correctly even under custom loaders;</li> + * <li>nestmate of the target (mutual private access);</li> + * <li>weak lifecycle — eligible for eager unloading once the {@link Class} + * object is unreachable, reducing metaspace pressure in long-running + * applications that generate many per-class artifacts.</li> + * </ul> + * + * <p>If hidden-class definition is disabled or fails (module not open, target + * is a hidden/primitive/array type, linkage error, …) the loader falls back to + * the traditional {@link ClassLoader#defineClass} path transparently. + */ public class ClassLoaderForClassArtifacts extends ClassLoader { + + /** Soft reference to the class for which artifacts are generated. */ public final SoftReference<Class> klazz; + + /** + * Counter used to ensure unique class names when multiple artifacts are + * generated for the same method name. + */ private final AtomicInteger classNamesCounter = new AtomicInteger(-1); - public ClassLoaderForClassArtifacts(Class klazz) { + /** + * Creates a new artifact class loader for the specified class. + * + * @param klazz the class whose artifact classes are to be defined via this loader + */ + public ClassLoaderForClassArtifacts(final Class klazz) { super(klazz.getClassLoader()); this.klazz = new SoftReference<>(klazz); } - public Class define(String name, byte[] bytes) { - Class cls = defineClass(name, bytes, 0, bytes.length, klazz.get().getProtectionDomain()); + // ------------------------------------------------------------------------- + // Class definition + // ------------------------------------------------------------------------- + + /** + * Defines a class from bytecode, preferring a hidden nestmate of the target. + * + * @param name the binary name used for the fallback (visible-class) path + * @param bytes the class-file bytes + * @return the defined class + */ + public Class define(final String name, final byte[] bytes) { + final Class<?> host = klazz.get(); + if (host != null) { + final Class<?> hidden = HiddenClassDefiner.tryDefineNestmate(host, bytes, false); + if (hidden != null) { + return hidden; + } + } + + // Fallback: visible class with the target's protection domain + final Class<?> cls = defineClass( + name, bytes, 0, bytes.length, + host != null ? host.getProtectionDomain() : null); resolveClass(cls); return cls; } + /** + * Defines a class from bytecode and returns the constructor matching the + * given parameter types, or {@code null} if definition or lookup fails. + * + * @param name the binary name (for fallback visible-class definition) + * @param bytes the class-file bytes + * @param parameterTypes the constructor parameter types to look up + * @return the matching constructor, or {@code null} + */ + public Constructor defineClassAndGetConstructor( + final String name, + final byte[] bytes, + final Class<?>... parameterTypes) { + try { + final Class<?> cls = define(name, bytes); + return cls.getDeclaredConstructor(parameterTypes); + } catch (NoSuchMethodException e) { + return null; + } + } Review Comment: `defineClassAndGetConstructor` previously returned a *public* constructor (via `getConstructor` in the old implementation). Switching to `getDeclaredConstructor` can return non-public constructors, which may later fail at `newInstance()` with `IllegalAccessException` and changes the method’s effective contract. Prefer using `getConstructor(parameterTypes)` to preserve the previous semantics (or explicitly set accessibility if returning declared constructors is intended, and document that change). ########## src/main/java/org/apache/groovy/util/HiddenClassDefiner.java: ########## @@ -0,0 +1,315 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.groovy.util; + +import org.objectweb.asm.ClassReader; +import org.objectweb.asm.ClassWriter; +import org.objectweb.asm.commons.ClassRemapper; +import org.objectweb.asm.commons.SimpleRemapper; + +import java.lang.invoke.MethodHandles; +import java.lang.invoke.MethodHandles.Lookup; +import java.lang.reflect.Constructor; + +/** + * Central facility for defining <em>hidden classes</em> + * (<a href="https://openjdk.org/jeps/371";>JEP 371</a>). + * + * <h2>Why a single entry point</h2> + * <p>Every dynamic class generator in Groovy (proxies, reflectors, per-class + * meta-method artifacts, …) should obtain hidden classes through this type so + * that: + * <ul> + * <li>the {@code NESTMATE} / {@code STRONG} policy lives in one place;</li> + * <li>the package of the supplied class-file is automatically aligned with + * the lookup class (a hard requirement of + * {@link Lookup#defineHiddenClass});</li> + * <li>callers can use a soft {@code try*} API that never throws on the + * expected failure modes (module access denied, package mismatch after + * rewrite, linkage errors because the host class loader cannot see a + * referenced type) and simply returns {@code null} for transparent + * fall-back to {@link ClassLoader#defineClass}.</li> + * </ul> + * + * <h2>Preferred usage (host-based)</h2> + * <pre>{@code + * // host determines: defining loader, run-time package, protection domain, nest + * Class<?> hidden = HiddenClassDefiner.tryDefineNestmate(hostClass, bytecode, false); + * if (hidden == null) { + * // fall back to ClassLoader.defineClass(...) + * } + * }</pre> + * + * <p>The host is obtained via {@link MethodHandles#privateLookupIn(Class, Lookup)} + * using a full-privilege lookup captured inside this (Java) class. That makes + * the result independent of Groovy's indy / {@code $$InjectedInvoker} + * caller-sensitive quirks. + * + * <h2>Lifecycle</h2> + * <ul> + * <li><em>weak</em> (default for nestmates) — the JVM may unload the class as + * soon as its {@link Class} object becomes unreachable;</li> + * <li><em>strong</em> — lifetime is tied to the defining class loader.</li> + * </ul> + * + * <h2>Kill switch</h2> + * <p>{@code -Dgroovy.hidden.classes.disable=true} forces every {@code try*} + * method to return {@code null} (and makes {@link #isEnabled()} false) so + * diagnostics and legacy environments can fall back without code changes. + * + * @since 6.0.0 + * @see Lookup#defineHiddenClass(byte[], boolean, Lookup.ClassOption...) + */ +public final class HiddenClassDefiner { + + /** System property that disables hidden-class definitions. */ + public static final String PROPERTY_DISABLE = "groovy.hidden.classes.disable"; + + /** + * {@code true} when hidden-class definitions are globally disabled. + * Evaluated once at class-init so hot paths pay no property-lookup cost. + */ + public static final boolean HIDDEN_CLASSES_DISABLED = + SystemUtil.getBooleanSafe(PROPERTY_DISABLE, false); + + /** + * Full-privilege lookup for <em>this</em> Java class, captured during + * {@code <clinit>}. Used solely as the caller argument to + * {@link MethodHandles#privateLookupIn(Class, Lookup)}; it is never used + * as the nest host of user-generated classes. + */ + private static final Lookup TRUSTED_LOOKUP = MethodHandles.lookup(); + + // Pre-allocated option arrays — defineHiddenClass is on the meta-class hot path. + private static final Lookup.ClassOption[] OPT_NESTMATE = + new Lookup.ClassOption[]{Lookup.ClassOption.NESTMATE}; + private static final Lookup.ClassOption[] OPT_STRONG = + new Lookup.ClassOption[]{Lookup.ClassOption.STRONG}; + private static final Lookup.ClassOption[] OPT_NESTMATE_STRONG = + new Lookup.ClassOption[]{Lookup.ClassOption.NESTMATE, Lookup.ClassOption.STRONG}; + private static final Lookup.ClassOption[] OPT_NONE = new Lookup.ClassOption[0]; + + private HiddenClassDefiner() { + throw new AssertionError("HiddenClassDefiner is a utility class"); + } + + // ------------------------------------------------------------------------- + // Status + // ------------------------------------------------------------------------- + + /** + * @return {@code true} when hidden-class definition is enabled + * (the default unless {@value #PROPERTY_DISABLE} is set) + */ + public static boolean isEnabled() { + return !HIDDEN_CLASSES_DISABLED; + } + + // ------------------------------------------------------------------------- + // Soft (best-effort) API — preferred by production call sites + // ------------------------------------------------------------------------- + + /** + * Attempts to define {@code bytes} as a hidden <em>nestmate</em> of + * {@code host} with a weak (eager-unloading) lifecycle. + * + * <p>On success the returned class: + * <ul> + * <li>is non-discoverable by name ({@link Class#isHidden()} is true);</li> + * <li>shares the defining loader / package / protection domain of + * {@code host};</li> + * <li>is a nestmate of {@code host} (mutual private access).</li> + * </ul> + * + * <p>Returns {@code null} when hidden classes are disabled, {@code host} + * is unsuitable (null / primitive / array / hidden), private lookup is + * refused, the class file is invalid, or linkage fails (for example the + * host loader cannot resolve a supertype referenced by the bytecode). + * Callers are expected to fall back to {@link ClassLoader#defineClass}. + * + * @param host nest host and class-loader / package donor; must be a + * normal (non-hidden) reference type + * @param bytes class-file bytes; {@code this_class} is rewritten into + * {@code host}'s package when needed + * @param initialize {@code true} to run {@code <clinit>} immediately + * @return the hidden class, or {@code null} if definition is not possible + */ + public static Class<?> tryDefineNestmate( + final Class<?> host, + final byte[] bytes, + final boolean initialize) { + if (HIDDEN_CLASSES_DISABLED || !isUsableHost(host) || bytes == null) { + return null; + } + try { + final Lookup hostLookup = MethodHandles.privateLookupIn(host, TRUSTED_LOOKUP); + final byte[] aligned = alignPackage(bytes, host); + return hostLookup.defineHiddenClass(aligned, initialize, OPT_NESTMATE).lookupClass(); + } catch (IllegalAccessException | IllegalArgumentException | SecurityException | LinkageError e) { + return null; + } catch (RuntimeException e) { + // ASM rewrite failures, unexpected JVM checks, etc. + return null; + } + } + + /** + * Soft variant of {@link #define(Lookup, byte[], boolean, boolean, boolean)}. + * Returns {@code null} instead of throwing for the expected failure modes. + * + * <p>The bytecode package is aligned to {@code lookup.lookupClass()} before + * definition. The lookup itself is not replaced — callers that need a + * specific nest host should obtain it via + * {@link MethodHandles#privateLookupIn(Class, Lookup)} (or use + * {@link #tryDefineNestmate(Class, byte[], boolean)}). + */ + public static Class<?> tryDefine( + final Lookup lookup, + final byte[] bytes, + final boolean initialize, + final boolean nestmate, + final boolean strong) { + if (HIDDEN_CLASSES_DISABLED || lookup == null || bytes == null) { + return null; + } + try { + return define(lookup, bytes, initialize, nestmate, strong); + } catch (IllegalAccessException | IllegalArgumentException | SecurityException | LinkageError e) { + return null; + } catch (RuntimeException e) { + return null; + } + } + + // ------------------------------------------------------------------------- + // Strict API — for tests and callers that want the original exception + // ------------------------------------------------------------------------- + + /** + * Defines a hidden class with the given options. + * + * <p>The class-file's {@code this_class} package is rewritten to match + * {@code lookup.lookupClass()} when they differ — this is required by + * {@link Lookup#defineHiddenClass}. + * + * @param lookup full-privilege lookup whose lookup-class supplies the + * defining loader, package, protection domain and + * (when {@code nestmate}) nest + * @param bytes class-file bytes + * @param initialize whether to initialize the class immediately + * @param nestmate whether to inject the class into the lookup class's nest + * @param strong whether the class's lifetime is tied to the loader + * @return the defined hidden class + * @throws IllegalAccessException if the lookup lacks the required access + * @throws IllegalArgumentException if the bytes are not a valid class file + * (after package alignment) + * @throws LinkageError if a dependency of the new class cannot + * be resolved in the lookup class's loader + */ + public static Class<?> define( + final Lookup lookup, + final byte[] bytes, + final boolean initialize, + final boolean nestmate, + final boolean strong) throws IllegalAccessException { + final byte[] aligned = alignPackage(bytes, lookup.lookupClass()); + return lookup.defineHiddenClass(aligned, initialize, options(nestmate, strong)).lookupClass(); + } + + /** + * Strict nestmate + weak convenience overload. + * + * @see #define(Lookup, byte[], boolean, boolean, boolean) + */ + public static Class<?> defineNestmate( + final Lookup lookup, + final byte[] bytes, + final boolean initialize) throws IllegalAccessException { + return define(lookup, bytes, initialize, true, false); + } + + /** + * Strict non-nestmate + strong convenience overload. + * + * @see #define(Lookup, byte[], boolean, boolean, boolean) + */ + public static Class<?> defineStrong( + final Lookup lookup, + final byte[] bytes, + final boolean initialize) throws IllegalAccessException { + return define(lookup, bytes, initialize, false, true); + } + + // ------------------------------------------------------------------------- + // Helpers + // ------------------------------------------------------------------------- + + /** + * Returns a declared constructor of {@code type}, wrapping a missing + * constructor as {@link IllegalStateException} (bytecode-generation bug). + */ + @SuppressWarnings("unchecked") + public static <T> Constructor<T> findConstructor( + final Class<T> type, + final Class<?>... parameterTypes) { + try { + return type.getDeclaredConstructor(parameterTypes); + } catch (NoSuchMethodException e) { + throw new IllegalStateException( + "Class " + type.getName() + " is missing the expected constructor", e); + } + } Review Comment: `@SuppressWarnings("unchecked")` is unnecessary here (there is no unchecked cast). Removing it reduces noise and keeps warnings meaningful. ########## src/main/java/org/codehaus/groovy/runtime/metaclass/ReflectorLoader.java: ########## @@ -51,56 +66,70 @@ public class ReflectorLoader extends ClassLoader { * @throws ClassNotFoundException if the class cannot be found */ @Override - protected Class findClass(String name) throws ClassNotFoundException { - if (delegatationLoader==null) return super.findClass(name); + protected Class<?> findClass(String name) throws ClassNotFoundException { + if (delegatationLoader == null) return super.findClass(name); return delegatationLoader.loadClass(name); } /** - * Loads a class per name. Unlike a normal loadClass this version - * behaves different during a class definition. In that case it - * checks if the class we want to load is Reflector and returns - * class if the check is successful. If it is not during a class - * definition it just calls the super class version of loadClass. - * - * @param name of the class to load - * @param resolve is true if the class should be resolved + * Loads a class per name. Unlike a normal {@code loadClass} this version + * behaves differently during a class definition. In that case it checks + * if the class we want to load is {@link Reflector} and returns that + * class if the check is successful. If it is not during a class definition + * it just calls the super class version of {@code loadClass}. + * + * @param name of the class to load + * @param resolve is {@code true} if the class should be resolved * @see Reflector * @see ClassLoader#loadClass(String, boolean) */ @Override - protected synchronized Class loadClass(String name, boolean resolve) throws ClassNotFoundException { + protected synchronized Class<?> loadClass(String name, boolean resolve) throws ClassNotFoundException { if (inDefine) { if (name.equals(REFLECTOR)) return Reflector.class; } return super.loadClass(name, resolve); } /** - * Helper method to define Reflector classes. This method sets the inDefine flag to true - * during class definition to ensure Reflector is resolved correctly, then resolves the - * newly defined class and stores it in the loadedClasses cache. + * Helper method to define Reflector classes. * - * @param name the fully qualified name of the Reflector class + * <p>Prefers a hidden nestmate of {@link Reflector} and falls back to the + * classic {@link ClassLoader#defineClass} path when that is not possible. + * + * <p>This method sets the {@code inDefine} flag to {@code true} during + * class definition to ensure {@link Reflector} is resolved correctly. + * + * @param name the fully qualified binary name of the Reflector class * @param bytecode the bytecode of the Reflector class - * @param domain the protection domain for the class + * @param domain the protection domain for the fallback visible-class + * definition; not used when the hidden-class path succeeds Review Comment: With the hidden-class path, the JVM-level class name will not match the provided binary `name` (package alignment + hidden-class suffix), and the class is not loadable by that name. It would help to document explicitly that `name` is used for caching/lookup and for the *visible fallback definition*, not as a guaranteed runtime name when a hidden class is defined. -- 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]
