This is an automated email from the ASF dual-hosted git repository. emilles pushed a commit to branch GROOVY_4_0_X in repository https://gitbox.apache.org/repos/asf/groovy.git
commit ae983c4301d99fd1ff761e149a5d19f53c9d8995 Author: Eric Milles <[email protected]> AuthorDate: Wed Oct 12 11:29:54 2022 -0500 GROOVY-10717: proxy method if abstract or no abstract overload exists --- src/main/java/groovy/util/ProxyGenerator.java | 68 +++++++---------- .../groovy/runtime/ProxyGeneratorAdapter.java | 85 ++++++++++------------ src/spec/test/CoercionTest.groovy | 33 ++++++++- 3 files changed, 93 insertions(+), 93 deletions(-) diff --git a/src/main/java/groovy/util/ProxyGenerator.java b/src/main/java/groovy/util/ProxyGenerator.java index 0e92ae07fa..7887168230 100644 --- a/src/main/java/groovy/util/ProxyGenerator.java +++ b/src/main/java/groovy/util/ProxyGenerator.java @@ -24,7 +24,6 @@ import groovy.lang.GroovyObject; import groovy.lang.GroovySystem; import groovy.lang.MetaClass; import org.codehaus.groovy.runtime.InvokerHelper; -import org.codehaus.groovy.runtime.MetaClassHelper; import org.codehaus.groovy.runtime.ProxyGeneratorAdapter; import org.codehaus.groovy.runtime.memoize.LRUCache; import org.codehaus.groovy.runtime.typehandling.GroovyCastException; @@ -43,27 +42,22 @@ import java.util.Map; import java.util.Objects; import java.util.Set; +import static org.codehaus.groovy.runtime.MetaClassHelper.EMPTY_CLASS_ARRAY; + /** - * Classes to generate 'Proxy' objects which implement interfaces, - * maps of closures and/or extend classes/delegates. + * Generates 'Proxy' objects which implement interfaces, maps of closures and/or + * extend classes/delegates. */ +@SuppressWarnings("rawtypes") public class ProxyGenerator { - private static final Class[] EMPTY_INTERFACE_ARRAY = MetaClassHelper.EMPTY_TYPE_ARRAY; private static final Map<Object,Object> EMPTY_CLOSURE_MAP = Collections.emptyMap(); - private static final Set<String> EMPTY_KEYSET = Collections.emptySet(); static { // wrap the standard MetaClass with the delegate setMetaClass(GroovySystem.getMetaClassRegistry().getMetaClass(ProxyGenerator.class)); } - private ClassLoader override = null; - private boolean debug = false; - private boolean emptyMethods = false; - - private static final Integer GROOVY_ADAPTER_CACHE_DEFAULT_SIZE = Integer.getInteger("groovy.adapter.cache.default.size", 64); - - public static final ProxyGenerator INSTANCE = new ProxyGenerator(); // TODO should we make ProxyGenerator singleton? + public static final ProxyGenerator INSTANCE = new ProxyGenerator(); // TODO: Should we make ProxyGenerator singleton? /** * The adapter cache is used to cache proxy classes. When, for example, a call like: @@ -71,7 +65,11 @@ public class ProxyGenerator { * adapter already exists. If so, then the class is reused, instead of generating a * new class. */ - private final LRUCache adapterCache = new LRUCache(GROOVY_ADAPTER_CACHE_DEFAULT_SIZE); + private final LRUCache<CacheKey,ProxyGeneratorAdapter> adapterCache = new LRUCache<>(Integer.getInteger("groovy.adapter.cache.default.size", 64)); + + private boolean debug; + private boolean emptyMethods; + private ClassLoader override; public boolean getDebug() { return debug; @@ -158,7 +156,6 @@ public class ProxyGenerator { return instantiateAggregate(closureMap, interfaces, clazz, null); } - @SuppressWarnings("unchecked") public GroovyObject instantiateAggregate(Map closureMap, List<Class> interfaces, Class clazz, Object[] constructorArgs) { if (clazz != null && Modifier.isFinal(clazz.getModifiers())) { throw new GroovyCastException("Cannot coerce a map to class " + clazz.getName() + " because it is a final class"); @@ -199,7 +196,6 @@ public class ProxyGenerator { * @param name the name of the proxy, unused, but kept for compatibility with previous versions of Groovy. * @return a proxy object implementing the specified interfaces, and delegating to the provided object */ - @SuppressWarnings("unchecked") public GroovyObject instantiateDelegateWithBaseClass(Map closureMap, List<Class> interfaces, Object delegate, Class baseClass, String name) { Map<Object,Object> map = closureMap != null ? closureMap : EMPTY_CLOSURE_MAP; ProxyGeneratorAdapter adapter = createAdapter(map, interfaces, delegate.getClass(), baseClass); @@ -207,33 +203,20 @@ public class ProxyGenerator { return adapter.delegatingProxy(delegate, map, (Object[])null); } - private ProxyGeneratorAdapter createAdapter(Map closureMap, List<Class> interfaces, Class delegateClass, Class baseClass) { - // According to https://shipilev.net/blog/2016/arrays-wisdom-ancients/#_conclusion - // toArray(new T[0]) seems faster, safer, and contractually cleaner, and therefore should be the default choice now. - Class[] intfs = interfaces != null ? interfaces.toArray(EMPTY_INTERFACE_ARRAY) : EMPTY_INTERFACE_ARRAY; - Class base = baseClass; - if (base == null) { - if (intfs.length > 0) { - base = intfs[0]; - } else { - base = Object.class; - } + private ProxyGeneratorAdapter createAdapter(final Map<Object,Object> closureMap, final List<Class> interfaceList, final Class delegateClass, final Class baseClass) { + Class[] interfaces = interfaceList != null ? interfaceList.toArray(EMPTY_CLASS_ARRAY) : EMPTY_CLASS_ARRAY; + final Class base = baseClass != null ? baseClass : (interfaces.length > 0 ? interfaces[0] : Object.class); + Set<String> methodNames = closureMap.isEmpty() ? Collections.emptySet() : new HashSet<>(); + for (Object key : closureMap.keySet()) { + methodNames.add(key.toString()); } - Set<String> keys = closureMap == EMPTY_CLOSURE_MAP ? EMPTY_KEYSET : new HashSet<String>(); - for (Object o : closureMap.keySet()) { - keys.add(o.toString()); - } - boolean useDelegate = null != delegateClass; - CacheKey key = new CacheKey(base, useDelegate ? delegateClass : Object.class, keys, intfs, emptyMethods, useDelegate); - final Class b = base; - - return (ProxyGeneratorAdapter) adapterCache.getAndPut( - key, - k -> new ProxyGeneratorAdapter(closureMap, b, intfs, useDelegate - ? delegateClass.getClassLoader() - : b.getClassLoader(), emptyMethods, useDelegate ? delegateClass : null - ) - ); + boolean useDelegate = (delegateClass != null); + CacheKey key = new CacheKey(base, useDelegate ? delegateClass : Object.class, methodNames, interfaces, emptyMethods, useDelegate); + + return adapterCache.getAndPut(key, k -> { + ClassLoader classLoader = useDelegate ? delegateClass.getClassLoader() : base.getClassLoader(); + return new ProxyGeneratorAdapter(closureMap, base, interfaces, classLoader, emptyMethods, useDelegate ? delegateClass : null); + }); } private static void setMetaClass(final MetaClass metaClass) { @@ -246,6 +229,8 @@ public class ProxyGenerator { GroovySystem.getMetaClassRegistry().setMetaClass(ProxyGenerator.class, newMetaClass); } + //-------------------------------------------------------------------------- + private static final class CacheKey { private static final Comparator<Class> INTERFACE_COMPARATOR = (o1, o2) -> { // Traits order *must* be preserved @@ -334,5 +319,4 @@ public class ProxyGenerator { } } } - } diff --git a/src/main/java/org/codehaus/groovy/runtime/ProxyGeneratorAdapter.java b/src/main/java/org/codehaus/groovy/runtime/ProxyGeneratorAdapter.java index 9e24d1b3d2..337662cdfe 100644 --- a/src/main/java/org/codehaus/groovy/runtime/ProxyGeneratorAdapter.java +++ b/src/main/java/org/codehaus/groovy/runtime/ProxyGeneratorAdapter.java @@ -44,7 +44,6 @@ import java.lang.reflect.Constructor; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.lang.reflect.Modifier; -import java.security.PrivilegedAction; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -117,45 +116,40 @@ import static org.objectweb.asm.Opcodes.RETURN; * * @since 2.0.0 */ +@SuppressWarnings("rawtypes") public class ProxyGeneratorAdapter extends ClassVisitor { private static final Map<String, Boolean> EMPTY_DELEGATECLOSURE_MAP = Collections.emptyMap(); - private static final Set<String> EMPTY_STRING_SET = Collections.emptySet(); + private static final Object[] EMPTY_ARGS = new Object[0]; private static final String CLOSURES_MAP_FIELD = "$closures$delegate$map"; private static final String DELEGATE_OBJECT_FIELD = "$delegate"; + private static final AtomicLong proxyCounter = new AtomicLong(); private static List<Method> OBJECT_METHODS = getInheritedMethods(Object.class, new ArrayList<>()); private static List<Method> GROOVYOBJECT_METHODS = getInheritedMethods(GroovyObject.class, new ArrayList<>()); - - private static final AtomicLong pxyCounter = new AtomicLong(); - private static final Set<String> GROOVYOBJECT_METHOD_NAMESS; - private static final Object[] EMPTY_ARGS = new Object[0]; - private static final String[] EMPTY_STRING_ARRAY = new String[0]; - + private static final Set<String> GROOVYOBJECT_METHOD_NAMES; static { - List<String> names = new ArrayList<>(); + Set<String> names = new HashSet<>(); for (Method method : GroovyObject.class.getMethods()) { names.add(method.getName()); } - GROOVYOBJECT_METHOD_NAMESS = new HashSet<>(names); + GROOVYOBJECT_METHOD_NAMES = Collections.unmodifiableSet(names); } - private final Class<?> superClass; - private final Class<?> delegateClass; - private final InnerLoader loader; private final String proxyName; - private final LinkedHashSet<Class> classList; + private final Class superClass; + private final Class delegateClass; + private final InnerLoader innerLoader; + private final Set<Class > implClasses; + private final Set<Object> visitedMethods; + private final Set<String> objectDelegateMethods; private final Map<String, Boolean> delegatedClosures; - // if emptyBody == true, then we generate an empty body instead throwing error on unimplemented methods + // if emptyBody is true, then we generate an empty body instead throwing error on unimplemented methods private final boolean emptyBody; private final boolean hasWildcard; private final boolean generateDelegateField; - private final Set<String> objectDelegateMethods; - - private final Set<Object> visitedMethods; - // cached class private final Class cachedClass; private final Constructor cachedNoArgConstructor; @@ -179,7 +173,7 @@ public class ProxyGeneratorAdapter extends ClassVisitor { final boolean emptyBody, final Class delegateClass) { super(ASM_API_VERSION, new ClassWriter(ClassWriter.COMPUTE_MAXS | ClassWriter.COMPUTE_FRAMES)); - this.loader = proxyLoader != null ? createInnerLoader(proxyLoader, interfaces) : findClassLoader(superClass, interfaces); + this.innerLoader = proxyLoader != null ? createInnerLoader(proxyLoader, interfaces) : findClassLoader(superClass, interfaces); this.visitedMethods = new LinkedHashSet<>(); this.delegatedClosures = closureMap.isEmpty() ? EMPTY_DELEGATECLOSURE_MAP : new HashMap<>(); boolean wildcard = false; @@ -196,7 +190,7 @@ public class ProxyGeneratorAdapter extends ClassVisitor { // if we have to delegate to another object, generate the appropriate delegate field // and collect the name of the methods for which delegation is active this.generateDelegateField = delegateClass != null; - this.objectDelegateMethods = generateDelegateField ? createDelegateMethodList(fixedSuperClass, delegateClass, interfaces) : EMPTY_STRING_SET; + this.objectDelegateMethods = generateDelegateField ? createDelegateMethodList(fixedSuperClass, delegateClass, interfaces) : Collections.emptySet(); this.delegateClass = delegateClass; // a proxy is supposed to be a concrete class, so it cannot extend an interface. @@ -204,15 +198,15 @@ public class ProxyGeneratorAdapter extends ClassVisitor { // and add this interface to the list of implemented interfaces this.superClass = fixedSuperClass; - // create the base list of classes which have possible methods to be overloaded - this.classList = new LinkedHashSet<>(); - this.classList.add(superClass); + // collect classes which have possible methods to be overloaded + implClasses = new LinkedHashSet<>(); + implClasses.add(superClass); if (generateDelegateField) { - classList.add(delegateClass); - Collections.addAll(this.classList, Arrays.stream(delegateClass.getInterfaces()).filter(c -> !isSealed(c)).toArray(Class[]::new)); + implClasses.add(delegateClass); + Arrays.stream(delegateClass.getInterfaces()).filter(i -> !isSealed(i)).forEach(implClasses::add); } if (interfaces != null) { - Collections.addAll(this.classList, interfaces); + Collections.addAll(implClasses, interfaces); } this.proxyName = proxyName(); this.emptyBody = emptyBody; @@ -221,8 +215,7 @@ public class ProxyGeneratorAdapter extends ClassVisitor { ClassWriter writer = (ClassWriter) cv; this.visit(CompilerConfiguration.DEFAULT.getBytecodeVersion(), ACC_PUBLIC, proxyName, null, null, null); byte[] b = writer.toByteArray(); -// CheckClassAdapter.verify(new ClassReader(b), true, new PrintWriter(System.err)); - cachedClass = loader.defineClass(proxyName.replace('/', '.'), b); + cachedClass = innerLoader.defineClass(proxyName.replace('/', '.'), b); // cache no-arg constructor Class<?>[] args = generateDelegateField ? new Class[]{Map.class, delegateClass} : new Class[]{Map.class}; Constructor<?> constructor; @@ -247,9 +240,9 @@ public class ProxyGeneratorAdapter extends ClassVisitor { if (!traits.isEmpty()) { String name = superClass.getName() + "$TraitAdapter"; ClassNode cn = new ClassNode(name, ACC_PUBLIC | ACC_ABSTRACT, ClassHelper.OBJECT_TYPE, traits.toArray(ClassNode.EMPTY_ARRAY), null); - CompilationUnit cu = new CompilationUnit(loader); + CompilationUnit cu = new CompilationUnit(innerLoader); CompilerConfiguration config = new CompilerConfiguration(); - SourceUnit su = new SourceUnit(name + "wrapper", "", config, loader, new ErrorCollector(config)); + SourceUnit su = new SourceUnit(name + "wrapper", "", config, innerLoader, new ErrorCollector(config)); cu.addSource(su); cu.compile(Phases.CONVERSION); su.getAST().addClass(cn); @@ -257,7 +250,7 @@ public class ProxyGeneratorAdapter extends ClassVisitor { List<GroovyClass> classes = cu.getClasses(); for (GroovyClass groovyClass : classes) { if (groovyClass.getName().equals(name)) { - return loader.defineClass(name, groovyClass.getBytes()); + return innerLoader.defineClass(name, groovyClass.getBytes()); } } } @@ -285,14 +278,9 @@ public class ProxyGeneratorAdapter extends ClassVisitor { return traits; } - @SuppressWarnings("removal") // TODO a future Groovy version should create the loader not as a privileged action - private GroovyClassLoader createClassLoader(ClassLoader parentLoader) { - return java.security.AccessController.doPrivileged((PrivilegedAction<GroovyClassLoader>) () -> new GroovyClassLoader(parentLoader)); - } - @SuppressWarnings("removal") // TODO a future Groovy version should create the loader not as a privileged action private static InnerLoader createInnerLoader(final ClassLoader parent, final Class<?>[] interfaces) { - return java.security.AccessController.doPrivileged((PrivilegedAction<InnerLoader>) () -> new InnerLoader(parent, interfaces)); + return java.security.AccessController.doPrivileged((java.security.PrivilegedAction<InnerLoader>) () -> new InnerLoader(parent, interfaces)); } private InnerLoader findClassLoader(final Class<?> clazz, final Class<?>[] interfaces) { @@ -370,22 +358,22 @@ public class ProxyGeneratorAdapter extends ClassVisitor { public void visit(final int version, final int access, final String name, final String signature, final String superName, final String[] interfaces) { Set<String> interfacesSet = new LinkedHashSet<>(); if (interfaces != null) Collections.addAll(interfacesSet, interfaces); - for (Class<?> extraInterface : classList) { + for (Class<?> extraInterface : implClasses) { if (extraInterface.isInterface()) interfacesSet.add(BytecodeHelper.getClassInternalName(extraInterface)); } final boolean addGroovyObjectSupport = !GroovyObject.class.isAssignableFrom(superClass); if (addGroovyObjectSupport) interfacesSet.add("groovy/lang/GroovyObject"); if (generateDelegateField) { - classList.add(GeneratedGroovyProxy.class); + implClasses.add(GeneratedGroovyProxy.class); interfacesSet.add("groovy/lang/GeneratedGroovyProxy"); } - super.visit(CompilerConfiguration.DEFAULT.getBytecodeVersion(), ACC_PUBLIC, proxyName, signature, BytecodeHelper.getClassInternalName(superClass), interfacesSet.toArray(EMPTY_STRING_ARRAY)); + super.visit(CompilerConfiguration.DEFAULT.getBytecodeVersion(), ACC_PUBLIC, proxyName, signature, BytecodeHelper.getClassInternalName(superClass), interfacesSet.toArray(new String[0])); visitMethod(ACC_PUBLIC, "<init>", "()V", null, null); addDelegateFields(); if (addGroovyObjectSupport) { createGroovyObjectSupport(); } - for (Class<?> clazz : classList) { + for (Class<?> clazz : implClasses) { visitClass(clazz); } } @@ -514,8 +502,8 @@ public class ProxyGeneratorAdapter extends ClassVisitor { name = name.substring(1, name.length() - 1) + "_array"; } int index = name.lastIndexOf('.'); - if (index == -1) return name + pxyCounter.incrementAndGet() + "_groovyProxy"; - return name.substring(index + 1) + pxyCounter.incrementAndGet() + "_groovyProxy"; + if (index == -1) return name + proxyCounter.incrementAndGet() + "_groovyProxy"; + return name.substring(index + 1) + proxyCounter.incrementAndGet() + "_groovyProxy"; } private static boolean isImplemented(final Class<?> clazz, final String name, final String desc) { @@ -544,9 +532,10 @@ public class ProxyGeneratorAdapter extends ClassVisitor { boolean wildcardDelegate = hasWildcard && !"<init>".equals(name); if ((objectDelegate || closureDelegate || wildcardDelegate) && !Modifier.isStatic(access)) { - if (!GROOVYOBJECT_METHOD_NAMESS.contains(name) - // GROOVY-8244: proxy for abstract class/trait/interface only overrides abstract method(s) - && (!Modifier.isAbstract(superClass.getModifiers()) || !isImplemented(superClass, name, desc))) { + if (!GROOVYOBJECT_METHOD_NAMES.contains(name) + // GROOVY-8244, GROOVY-10717: replace if abstract or no abstract overload exists + && (!Modifier.isAbstract(superClass.getModifiers()) || !isImplemented(superClass, name, desc) + || ((objectDelegate || closureDelegate) && Arrays.stream(superClass.getMethods()).filter(m -> m.getName().equals(name)).mapToInt(Method::getModifiers).noneMatch(Modifier::isAbstract)))) { if (closureDelegate || wildcardDelegate || !(objectDelegate && generateDelegateField)) { delegatedClosures.put(name, Boolean.TRUE); @@ -560,7 +549,7 @@ public class ProxyGeneratorAdapter extends ClassVisitor { } else if ("<init>".equals(name) && (Modifier.isPublic(access) || Modifier.isProtected(access))) { return createConstructor(access, name, desc, signature, exceptions); - } else if (Modifier.isAbstract(access) && !GROOVYOBJECT_METHOD_NAMESS.contains(name) && !isImplemented(superClass, name, desc)) { + } else if (Modifier.isAbstract(access) && !GROOVYOBJECT_METHOD_NAMES.contains(name) && !isImplemented(superClass, name, desc)) { MethodVisitor mv = super.visitMethod(access & ~ACC_ABSTRACT, name, desc, signature, exceptions); mv.visitCode(); Type[] args = Type.getArgumentTypes(desc); diff --git a/src/spec/test/CoercionTest.groovy b/src/spec/test/CoercionTest.groovy index ff44750557..c0dd0e9688 100644 --- a/src/spec/test/CoercionTest.groovy +++ b/src/spec/test/CoercionTest.groovy @@ -1,5 +1,3 @@ -import groovy.test.GroovyTestCase - /* * Licensed to the Apache Software Foundation (ASF) under one * or more contributor license agreements. See the NOTICE file @@ -18,7 +16,8 @@ import groovy.test.GroovyTestCase * specific language governing permissions and limitations * under the License. */ -class CoercionTest extends GroovyTestCase { + +final class CoercionTest extends groovy.test.GroovyTestCase { void testStringToEnumValue() { assertScript ''' @@ -299,6 +298,34 @@ println iter.next() assert map.i==0 // end::use_coerced_iterator[] ''' + // "remove()" is an interface default method + assertScript '''import static groovy.test.GroovyAssert.shouldFail + + def iter = [next: {->null}, hasNext: {->true}] as Iterator + shouldFail(UnsupportedOperationException) { + iter.remove() + } + + iter = [next: {->null}, hasNext: {->true}, remove: {->}] as Iterator + iter.remove() + ''' + } + + // GROOVY-10717 + void testCoerceMapToAbstract() { + assertScript '''import static groovy.test.GroovyAssert.shouldFail + + abstract class A { + String b,c + } + + def a = [getB: {->'bb'}, getX: {->'xx'}] as A + assert a.b == 'bb' // overrides accessor + assert a.c == null // generated accessor + shouldFail(MissingPropertyException) { + a.x + } + ''' } void testCoerceThrowsNPE() {
