This is an automated email from the ASF dual-hosted git repository. emilles pushed a commit to branch GROOVY_3_0_X in repository https://gitbox.apache.org/repos/asf/groovy.git
commit 3380c7aca4d4d514126105f24b2c3b916163b57d Author: Eric Milles <[email protected]> AuthorDate: Wed Feb 15 12:04:32 2023 -0600 GROOVY-10929: method closure swallows `IllegalArgumentException` 3_0_X backport --- src/main/java/groovy/lang/MetaClassImpl.java | 117 ++++++++++----------- src/main/java/groovy/lang/MetaProperty.java | 6 +- .../codehaus/groovy/reflection/CachedField.java | 16 ++- src/test/groovy/bugs/MethodClosureTest.groovy | 68 +++++++----- 4 files changed, 107 insertions(+), 100 deletions(-) diff --git a/src/main/java/groovy/lang/MetaClassImpl.java b/src/main/java/groovy/lang/MetaClassImpl.java index 763a941ebf..8ecf81eeca 100644 --- a/src/main/java/groovy/lang/MetaClassImpl.java +++ b/src/main/java/groovy/lang/MetaClassImpl.java @@ -26,7 +26,6 @@ import org.codehaus.groovy.ast.ClassNode; import org.codehaus.groovy.classgen.asm.BytecodeHelper; import org.codehaus.groovy.control.CompilationUnit; import org.codehaus.groovy.control.Phases; -import org.codehaus.groovy.reflection.CacheAccessControlException; import org.codehaus.groovy.reflection.CachedClass; import org.codehaus.groovy.reflection.CachedConstructor; import org.codehaus.groovy.reflection.CachedField; @@ -245,7 +244,7 @@ public class MetaClassImpl implements MetaClass, MutableMetaClass { /** * Returns the registry for this metaclass * - * @return The resgistry + * @return The registry */ public MetaClassRegistry getRegistry() { return registry; @@ -334,6 +333,7 @@ public class MetaClassImpl implements MetaClass, MutableMetaClass { * * @return The class contained by this metaclass */ + @Override public Class getTheClass() { return this.theClass; } @@ -1035,70 +1035,65 @@ public class MetaClassImpl implements MetaClass, MutableMetaClass { return invokeMethod(theClass, object, methodName, originalArguments, false, false); } - private Object invokeMethodClosure(Object object, Object[] arguments) { - final MethodClosure mc = (MethodClosure) object; - final Object owner = mc.getOwner(); - - String methodName = mc.getMethod(); - final Class ownerClass = owner instanceof Class ? (Class) owner : owner.getClass(); + private Object invokeMethodClosure(final MethodClosure object, final Object[] arguments) { + Object owner = object.getOwner(); + String method = object.getMethod(); + boolean ownerIsClass = (owner instanceof Class); + Class ownerClass = ownerIsClass ? (Class) owner : owner.getClass(); final MetaClass ownerMetaClass = registry.getMetaClass(ownerClass); - // To conform to "Least Surprise" principle, try to invoke method with original arguments first, which can match most of use cases + // To conform to "Least Surprise" principle, try to invoke method with original arguments first, which can match most use cases try { - return ownerMetaClass.invokeMethod(ownerClass, owner, methodName, arguments, false, false); - } catch (MissingMethodExceptionNoStack | InvokerInvocationException e) { - // CONSTRUCTOR REFERENCE - if (owner instanceof Class && MethodClosure.NEW.equals(methodName)) { - if (ownerClass.isArray()) { - if (0 == arguments.length) { - throw new GroovyRuntimeException("The arguments(specifying size) are required to create array[" + ownerClass.getCanonicalName() + "]"); - } - - int arrayDimension = ArrayTypeUtils.dimension(ownerClass); - - if (arguments.length > arrayDimension) { - throw new GroovyRuntimeException("The length[" + arguments.length + "] of arguments should not be greater than the dimensions[" + arrayDimension + "] of array[" + ownerClass.getCanonicalName() + "]"); - } + return ownerMetaClass.invokeMethod(ownerClass, owner, method, arguments, false, false); + } catch (GroovyRuntimeException e) { // GROOVY-10929: GroovyRuntimeException(cause:IllegalArgumentException) thrown for final fields + if (!(ownerIsClass && (e instanceof MissingMethodExceptionNoStack || e instanceof InvokerInvocationException || e.getCause() instanceof IllegalArgumentException))) { + throw e; + } + if (MethodClosure.NEW.equals(method)) { + // CONSTRUCTOR REFERENCE - int[] sizeArray = new int[arguments.length]; + if (!ownerClass.isArray()) + return ownerMetaClass.invokeConstructor(arguments); - for (int i = 0, n = sizeArray.length; i < n; i++) { - Object argument = arguments[i]; + int nArguments = arguments.length; + if (nArguments == 0) { + throw new GroovyRuntimeException("The arguments(specifying size) are required to create array[" + ownerClass.getCanonicalName() + "]"); + } - if (argument instanceof Integer) { - sizeArray[i] = (Integer) argument; - } else { - sizeArray[i] = Integer.parseInt(String.valueOf(argument)); - } + int arrayDimension = ArrayTypeUtils.dimension(ownerClass); + if (nArguments > arrayDimension) { + throw new GroovyRuntimeException("The length[" + nArguments + "] of arguments should not be greater than the dimensions[" + arrayDimension + "] of array[" + ownerClass.getCanonicalName() + "]"); + } + int[] sizeArray = new int[nArguments]; + for (int i = 0; i < nArguments; i += 1) { + Object argument = arguments[i]; + if (argument instanceof Integer) { + sizeArray[i] = (Integer) argument; + } else { + sizeArray[i] = Integer.parseInt(String.valueOf(argument)); } - - Class arrayType = - arguments.length == arrayDimension - ? ArrayTypeUtils.elementType(ownerClass) // Just for better performance, though we can use reduceDimension only - : ArrayTypeUtils.elementType(ownerClass, (arrayDimension - arguments.length)); - return Array.newInstance(arrayType, sizeArray); + } + Class elementType = + nArguments == arrayDimension + ? ArrayTypeUtils.elementType(ownerClass) // Just for better performance, though we can use reduceDimension only + : ArrayTypeUtils.elementType(ownerClass, (arrayDimension - nArguments)); + return Array.newInstance(elementType, sizeArray); + } else { + // METHOD REFERENCE + + // if the owner is a class and the method closure can be related to some instance method(s) + // try to invoke method with adjusted arguments -- first argument is instance of owner type + if (arguments.length > 0 && ownerClass.isAssignableFrom(arguments[0].getClass()) + && (Boolean) object.getProperty(MethodClosure.ANY_INSTANCE_METHOD_EXISTS)) { + try { + Object newReceiver = arguments[0]; + Object[] newArguments = Arrays.copyOfRange(arguments, 1, arguments.length); + return ownerMetaClass.invokeMethod(ownerClass, newReceiver, method, newArguments, false, false); + } catch (MissingMethodException ignore) {} } - return ownerMetaClass.invokeConstructor(arguments); - } - - // METHOD REFERENCE - // if and only if the owner is a class and the method closure can be related to some instance methods, - // try to invoke method with adjusted arguments(first argument is the actual owner) again. - // otherwise throw the MissingMethodExceptionNoStack. - if (!(owner instanceof Class - && (Boolean) mc.getProperty(MethodClosure.ANY_INSTANCE_METHOD_EXISTS))) { - - throw e; - } - - if (arguments.length < 1 || !ownerClass.isAssignableFrom(arguments[0].getClass())) { - return invokeMissingMethod(object, methodName, arguments); + return invokeMissingMethod(object, method, arguments); } - - Object newReceiver = arguments[0]; - Object[] newArguments = Arrays.copyOfRange(arguments, 1, arguments.length); - return ownerMetaClass.invokeMethod(ownerClass, newReceiver, methodName, newArguments, false, false); } } @@ -1134,7 +1129,7 @@ public class MetaClassImpl implements MetaClass, MutableMetaClass { if (CALL_METHOD.equals(methodName) || DO_CALL_METHOD.equals(methodName)) { final Class objectClass = object.getClass(); if (objectClass == MethodClosure.class) { - return this.invokeMethodClosure(object, arguments); + return this.invokeMethodClosure((MethodClosure) object, arguments); } else if (objectClass == CurriedClosure.class) { final CurriedClosure cc = (CurriedClosure) object; // change the arguments for an uncurried call @@ -1893,7 +1888,7 @@ public class MetaClassImpl implements MetaClass, MutableMetaClass { if (method == null && mp != null) { try { return mp.getProperty(object); - } catch (IllegalArgumentException | CacheAccessControlException e) { + } catch (GroovyRuntimeException e) { // can't access the field directly but there may be a getter mp = null; } @@ -2015,12 +2010,6 @@ public class MetaClassImpl implements MetaClass, MutableMetaClass { if (mp != null) { return mp; -// try { -// return mp.getProperty(object); -// } catch (IllegalArgumentException e) { -// // can't access the field directly but there may be a getter -// mp = null; -// } } //---------------------------------------------------------------------- diff --git a/src/main/java/groovy/lang/MetaProperty.java b/src/main/java/groovy/lang/MetaProperty.java index 5318b47b38..f4d52a4f2d 100644 --- a/src/main/java/groovy/lang/MetaProperty.java +++ b/src/main/java/groovy/lang/MetaProperty.java @@ -41,13 +41,13 @@ public abstract class MetaProperty { /** * @return the property of the given object - * @throws Exception if the property could not be evaluated + * @throws RuntimeException if the property could not be evaluated */ public abstract Object getProperty(Object object); /** - * Sets the property on the given object to the new value - * + * Sets the property on the given object to the new value. + * * @param object on which to set the property * @param newValue the new value of the property * @throws RuntimeException if the property could not be set diff --git a/src/main/java/org/codehaus/groovy/reflection/CachedField.java b/src/main/java/org/codehaus/groovy/reflection/CachedField.java index 6c666aac71..f7ccc80634 100644 --- a/src/main/java/org/codehaus/groovy/reflection/CachedField.java +++ b/src/main/java/org/codehaus/groovy/reflection/CachedField.java @@ -44,6 +44,9 @@ public class CachedField extends MetaProperty { return field.getDeclaringClass(); } + /** + * {@inheritDoc} + */ @Override public int getModifiers() { return field.getModifiers(); @@ -58,25 +61,20 @@ public class CachedField extends MetaProperty { } /** - * @return the property of the given object - * @throws RuntimeException if the property could not be evaluated + * {@inheritDoc} */ @Override public Object getProperty(final Object object) { makeAccessibleIfNecessary(); try { return field.get(object); - } catch (IllegalAccessException e) { + } catch (IllegalAccessException | IllegalArgumentException e) { throw new GroovyRuntimeException("Cannot get the property '" + name + "'.", e); } } /** - * Sets the property on the given object to the new value - * - * @param object on which to set the property - * @param newValue the new value of the property - * @throws RuntimeException if the property could not be set + * {@inheritDoc} */ @Override public void setProperty(final Object object, final Object newValue) { @@ -87,7 +85,7 @@ public class CachedField extends MetaProperty { Object goalValue = DefaultTypeTransformation.castToType(newValue, field.getType()); try { field.set(object, goalValue); - } catch (IllegalAccessException e) { + } catch (IllegalAccessException | IllegalArgumentException e) { throw new GroovyRuntimeException("Cannot set the property '" + name + "'.", e); } } diff --git a/src/test/groovy/bugs/MethodClosureTest.groovy b/src/test/groovy/bugs/MethodClosureTest.groovy index 03599e483c..76b0047c67 100644 --- a/src/test/groovy/bugs/MethodClosureTest.groovy +++ b/src/test/groovy/bugs/MethodClosureTest.groovy @@ -18,47 +18,54 @@ */ package groovy.bugs -import groovy.test.GroovyTestCase import org.codehaus.groovy.runtime.MethodClosure +import org.junit.Test -class MethodClosureTest extends GroovyTestCase { +import static groovy.test.GroovyAssert.assertScript +import static groovy.test.GroovyAssert.shouldFail + +final class MethodClosureTest { def aa(x) { x } - - static bb(it) { it} - - void testMethodClosure() { - def cl2 = String.&toUpperCase // Class.instanceMethod - assert cl2 instanceof Closure - assert cl2 instanceof MethodClosure - - assert ["xx", "yy"].collect(cl2) == ["XX","YY"] - Class[] c1 = [ Exception.class, Throwable.class ] - Class[] c2 = [ IllegalStateException.class ] + static bb(x) { + x + } - def cl = this.&aa // instance.instanceMethod + @Test + void testMethodClosure() { + def closure = this.&aa // instance.instanceMethod + assert closure instanceof Closure + assert closure instanceof MethodClosure - assert cl instanceof Closure - assert cl instanceof MethodClosure + Class[] c1 = [ Exception, Throwable ] + Class[] c2 = [ IllegalStateException ] + assert [c1, c2].collect(closure) == [c1,c2] + } - assert [c1, c2].collect(cl) == [c1,c2] + @Test + void testMethodClosure2() { + def closure = String.&toUpperCase // Class.instanceMethod + assert closure instanceof Closure + assert closure instanceof MethodClosure + assert ["xx", "yy"].collect(closure) == ["XX","YY"] } - - void testStaticMethodAccess() { - def list = [1].collect (this.&bb) + + @Test + void testStaticMethodClosure() { + def list = [1].collect(this.&bb) assert list == [1] - list = [1].collect (MethodClosureTest.&bb) // Class.staticMethod + list = [1].collect(MethodClosureTest.&bb) // Class.staticMethod assert list == [1] - def mct = new MethodClosureTest() - list = [1].collect (mct.&bb) // instance.staticMethod + list = [1].collect(new MethodClosureTest().&bb) // instance.staticMethod assert list == [1] } - void testShellVariable() { + @Test + void testShellVariableAccess() { def shell = new GroovyShell() assert shell.evaluate("x = String.&toUpperCase; x('abc')") == "ABC" assert shell.evaluate("x = 'abc'.&toUpperCase; x()") == "ABC" @@ -66,6 +73,7 @@ class MethodClosureTest extends GroovyTestCase { assert shell.evaluate("x = 3.&parseInt; x('123')") == 123 } + @Test void testMethodClosureWithCategory() { assertScript ''' class Bar { @@ -103,4 +111,16 @@ class MethodClosureTest extends GroovyTestCase { } ''' } + + // GROOVY-10929 + @Test + void testMethodClosureIllegalArgumentException() { + shouldFail IllegalArgumentException, ''' + static create(Class type) { + throw new IllegalArgumentException("Class ${type.name} does not ...") + } + def closure = this.class.&create + closure(Object) + ''' + } }
