ReflectiveMethodResolver applies useDistance mode by default (with fine-tuned varargs handling)

Issue: SPR-12803
Issue: SPR-12808

spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectionHelper.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2014 the original author or authors.
+ * Copyright 2002-2015 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -40,13 +40,14 @@ import org.springframework.util.MethodInvoker;
 public class ReflectionHelper {
 
 	/**
-	 * Compare argument arrays and return information about whether they match. A supplied
+	 * Compare argument arrays and return information about whether they match.
-	 * type converter and conversionAllowed flag allow for matches to take into account
+	 * A supplied type converter and conversionAllowed flag allow for matches to take
-	 * that a type may be transformed into a different type by the converter.
+	 * into account that a type may be transformed into a different type by the converter.
-	 * @param expectedArgTypes the array of types the method/constructor is expecting
+	 * @param expectedArgTypes the types the method/constructor is expecting
-	 * @param suppliedArgTypes the array of types that are being supplied at the point of invocation
+	 * @param suppliedArgTypes the types that are being supplied at the point of invocation
 	 * @param typeConverter a registered type converter
-	 * @return a MatchInfo object indicating what kind of match it was or null if it was not a match
+	 * @return a MatchInfo object indicating what kind of match it was,
+	 * or {@code null} if it was not a match
 	 */
 	static ArgumentsMatchInfo compareArguments(
 			List<TypeDescriptor> expectedArgTypes, List<TypeDescriptor> suppliedArgTypes, TypeConverter typeConverter) {
@@ -90,7 +91,7 @@ public class ReflectionHelper {
 		int result = 0;
 		for (int i = 0; i < paramTypes.size(); i++) {
 			TypeDescriptor paramType = paramTypes.get(i);
-			TypeDescriptor argType = argTypes.get(i);
+			TypeDescriptor argType = (i < argTypes.size() ? argTypes.get(i) : null);
 			if (argType == null) {
 				if (paramType.isPrimitive()) {
 					return Integer.MAX_VALUE;
@@ -127,13 +128,15 @@ public class ReflectionHelper {
 	}
 
 	/**
-	 * Compare argument arrays and return information about whether they match. A supplied type converter and
+	 * Compare argument arrays and return information about whether they match.
-	 * conversionAllowed flag allow for matches to take into account that a type may be transformed into a different
+	 * A supplied type converter and conversionAllowed flag allow for matches to
-	 * type by the converter. This variant of compareArguments also allows for a varargs match.
+	 * take into account that a type may be transformed into a different type by the
-	 * @param expectedArgTypes the array of types the method/constructor is expecting
+	 * converter. This variant of compareArguments also allows for a varargs match.
-	 * @param suppliedArgTypes the array of types that are being supplied at the point of invocation
+	 * @param expectedArgTypes the types the method/constructor is expecting
+	 * @param suppliedArgTypes the types that are being supplied at the point of invocation
 	 * @param typeConverter a registered type converter
-	 * @return a MatchInfo object indicating what kind of match it was or null if it was not a match
+	 * @return a MatchInfo object indicating what kind of match it was,
+	 * or {@code null} if it was not a match
 	 */
 	static ArgumentsMatchInfo compareArgumentsVarargs(
 			List<TypeDescriptor> expectedArgTypes, List<TypeDescriptor> suppliedArgTypes, TypeConverter typeConverter) {
@@ -246,12 +249,14 @@ public class ReflectionHelper {
 	 * @param converter the type converter to use for attempting conversions
 	 * @param arguments the actual arguments that need conversion
 	 * @param methodOrCtor the target Method or Constructor
-	 * @param varargsPosition the known position of the varargs argument, if any (null if not varargs)
+	 * @param varargsPosition the known position of the varargs argument, if any
-	 * @return true if some kind of conversion occurred on an argument
+	 * ({@code null} if not varargs)
+	 * @return {@code true} if some kind of conversion occurred on an argument
 	 * @throws EvaluationException if a problem occurs during conversion
 	 */
 	static boolean convertArguments(TypeConverter converter, Object[] arguments, Object methodOrCtor,
 			Integer varargsPosition) throws EvaluationException {
+
 		boolean conversionOccurred = false;
 		if (varargsPosition == null) {
 			for (int i = 0; i < arguments.length; i++) {
@@ -320,9 +325,9 @@ public class ReflectionHelper {
 
 	/**
 	 * Package up the arguments so that they correctly match what is expected in parameterTypes.
-	 * For example, if parameterTypes is (int, String[]) because the second parameter was declared String...
+	 * For example, if parameterTypes is {@code (int, String[])} because the second parameter
-	 * then if arguments is [1,"a","b"] then it must be repackaged as [1,new String[]{"a","b"}] in order to
+	 * was declared {@code String...}, then if arguments is {@code [1,"a","b"]} then it must be
-	 * match the expected parameterTypes.
+	 * repackaged as {@code [1,new String[]{"a","b"}]} in order to match the expected types.
 	 * @param requiredParameterTypes the types of the parameters for the invocation
 	 * @param args the arguments to be setup ready for the invocation
 	 * @return a repackaged array of arguments where any varargs setup has been done
@@ -374,10 +379,11 @@ public class ReflectionHelper {
 
 
 	/**
-	 * An instance of ArgumentsMatchInfo describes what kind of match was achieved between two sets of arguments -
+	 * An instance of ArgumentsMatchInfo describes what kind of match was achieved
-	 * the set that a method/constructor is expecting and the set that are being supplied at the point of invocation.
+	 * between two sets of arguments - the set that a method/constructor is expecting
-	 * If the kind indicates that conversion is required for some of the arguments then the arguments that require
+	 * and the set that are being supplied at the point of invocation. If the kind
-	 * conversion are listed in the argsRequiringConversion array.
+	 * indicates that conversion is required for some of the arguments then the arguments
+	 * that require conversion are listed in the argsRequiringConversion array.
 	 */
 	static class ArgumentsMatchInfo {
 

spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodResolver.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2014 the original author or authors.
+ * Copyright 2002-2015 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -62,17 +62,18 @@ public class ReflectiveMethodResolver implements MethodResolver {
 
 
 	public ReflectiveMethodResolver() {
-		this.useDistance = false;
+		this.useDistance = true;
 	}
 
 	/**
-	 * This constructors allows the ReflectiveMethodResolver to be configured such that it will
+	 * This constructor allows the ReflectiveMethodResolver to be configured such that it
-	 * use a distance computation to check which is the better of two close matches (when there
+	 * will use a distance computation to check which is the better of two close matches
-	 * are multiple matches). Using the distance computation is intended to ensure matches
+	 * (when there are multiple matches). Using the distance computation is intended to
-	 * are more closely representative of what a Java compiler would do when taking into
+	 * ensure matches are more closely representative of what a Java compiler would do
-	 * account boxing/unboxing and whether the method candidates are declared to handle a
+	 * when taking into account boxing/unboxing and whether the method candidates are
-	 * supertype of the type (of the argument) being passed in.
+	 * declared to handle a supertype of the type (of the argument) being passed in.
-	 * @param useDistance true if distance computation should be used when calculating matches
+	 * @param useDistance {@code true} if distance computation should be used when
+	 * calculating matches; {@code false} otherwise
 	 */
 	public ReflectiveMethodResolver(boolean useDistance) {
 		this.useDistance = useDistance;
@@ -175,17 +176,17 @@ public class ReflectiveMethodResolver implements MethodResolver {
 							return new ReflectiveMethodExecutor(method);
 						}
 						else if (matchInfo.isCloseMatch()) {
-							if (!this.useDistance) {
+							if (this.useDistance) {
-								// Take this as a close match if there isn't one already
+								int matchDistance = ReflectionHelper.getTypeDifferenceWeight(paramDescriptors, argumentTypes);
-								if (closeMatch == null) {
+								if (closeMatch == null || matchDistance < closeMatchDistance) {
+									// This is a better match...
 									closeMatch = method;
+									closeMatchDistance = matchDistance;
 								}
 							}
 							else {
-								int matchDistance = ReflectionHelper.getTypeDifferenceWeight(paramDescriptors, argumentTypes);
+								// Take this as a close match if there isn't one already
-								if (matchDistance < closeMatchDistance) {
+								if (closeMatch == null) {
-									// This is a better match...
-									closeMatchDistance = matchDistance;
 									closeMatch = method;
 								}
 							}

spring-expression/src/test/java/org/springframework/expression/spel/AbstractExpressionTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2014 the original author or authors.
+ * Copyright 2002-2015 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -35,24 +35,26 @@ import static org.junit.Assert.*;
  */
 public abstract class AbstractExpressionTests {
 
-	private final static boolean DEBUG = false;
+	private static final boolean DEBUG = false;
+
+	protected static final boolean SHOULD_BE_WRITABLE = true;
+
+	protected static final boolean SHOULD_NOT_BE_WRITABLE = false;
 
-	protected final static boolean SHOULD_BE_WRITABLE = true;
-	protected final static boolean SHOULD_NOT_BE_WRITABLE = false;
 
 	protected final ExpressionParser parser = new SpelExpressionParser();
+
 	protected final StandardEvaluationContext eContext = TestScenarioCreator.getTestEvaluationContext();
 
 
 	/**
-	 * Evaluate an expression and check that the actual result matches the expectedValue and the class of the result
+	 * Evaluate an expression and check that the actual result matches the
-	 * matches the expectedClassOfResult.
+	 * expectedValue and the class of the result matches the expectedClassOfResult.
-	 * @param expression The expression to evaluate
+	 * @param expression the expression to evaluate
 	 * @param expectedValue the expected result for evaluating the expression
 	 * @param expectedResultType the expected class of the evaluation result
 	 */
-	protected void evaluate(String expression, Object expectedValue, Class<?> expectedResultType) {
+	public void evaluate(String expression, Object expectedValue, Class<?> expectedResultType) {
-		try {
 		Expression expr = parser.parseExpression(expression);
 		if (expr == null) {
 			fail("Parser returned null for expression");
@@ -60,10 +62,6 @@ public abstract class AbstractExpressionTests {
 		if (DEBUG) {
 			SpelUtilities.printAbstractSyntaxTree(System.out, expr);
 		}
-			// Class<?> expressionType = expr.getValueType();
-			// assertEquals("Type of the expression is not as expected. Should be '" +
-			// expectedResultType + "' but is '"
-			// + expressionType + "'", expectedResultType, expressionType);
 
 		Object value = expr.getValue(eContext);
 
@@ -72,17 +70,12 @@ public abstract class AbstractExpressionTests {
 			if (expectedValue == null) {
 				return;  // no point doing other checks
 			}
-				assertEquals("Expression returned null value, but expected '" + expectedValue + "'", expectedValue,
+			assertEquals("Expression returned null value, but expected '" + expectedValue + "'", expectedValue, null);
-					null);
 		}
 
 		Class<?> resultType = value.getClass();
-			assertEquals("Type of the actual result was not as expected.  Expected '" + expectedResultType
+		assertEquals("Type of the actual result was not as expected.  Expected '" + expectedResultType +
-					+ "' but result was of type '" + resultType + "'", expectedResultType, resultType);
+				"' but result was of type '" + resultType + "'", expectedResultType, resultType);
-			// .equals/* isAssignableFrom */(resultType), truers);
-
-			// TODO isAssignableFrom would allow some room for compatibility
-			// in the above expression...
 
 		if (expectedValue instanceof String) {
 			assertEquals("Did not get expected value for expression '" + expression + "'.", expectedValue,
@@ -92,18 +85,8 @@ public abstract class AbstractExpressionTests {
 			assertEquals("Did not get expected value for expression '" + expression + "'.", expectedValue, value);
 		}
 	}
-		catch (EvaluationException ee) {
-			ee.printStackTrace();
-			fail("Unexpected Exception: " + ee.getMessage());
-		}
-		catch (ParseException pe) {
-			pe.printStackTrace();
-			fail("Unexpected Exception: " + pe.getMessage());
-		}
-	}
 
-	protected void evaluateAndAskForReturnType(String expression, Object expectedValue, Class<?> expectedResultType) {
+	public void evaluateAndAskForReturnType(String expression, Object expectedValue, Class<?> expectedResultType) {
-		try {
 		Expression expr = parser.parseExpression(expression);
 		if (expr == null) {
 			fail("Parser returned null for expression");
@@ -111,64 +94,45 @@ public abstract class AbstractExpressionTests {
 		if (DEBUG) {
 			SpelUtilities.printAbstractSyntaxTree(System.out, expr);
 		}
-			// Class<?> expressionType = expr.getValueType();
-			// assertEquals("Type of the expression is not as expected. Should be '" +
-			// expectedResultType + "' but is '"
-			// + expressionType + "'", expectedResultType, expressionType);
 
 		Object value = expr.getValue(eContext, expectedResultType);
 		if (value == null) {
-				if (expectedValue == null)
+			if (expectedValue == null) {
 				return;  // no point doing other checks
-				assertEquals("Expression returned null value, but expected '" + expectedValue + "'", expectedValue,
+			}
-					null);
+			assertEquals("Expression returned null value, but expected '" + expectedValue + "'", expectedValue, null);
 		}
 
 		Class<?> resultType = value.getClass();
-			assertEquals("Type of the actual result was not as expected.  Expected '" + expectedResultType
+		assertEquals("Type of the actual result was not as expected.  Expected '" + expectedResultType +
-					+ "' but result was of type '" + resultType + "'", expectedResultType, resultType);
+				"' but result was of type '" + resultType + "'", expectedResultType, resultType);
-			// .equals/* isAssignableFrom */(resultType), truers);
 		assertEquals("Did not get expected value for expression '" + expression + "'.", expectedValue, value);
-			// isAssignableFrom would allow some room for compatibility
-			// in the above expression...
-		}
-		catch (EvaluationException ee) {
-			SpelEvaluationException ex = (SpelEvaluationException) ee;
-			ex.printStackTrace();
-			fail("Unexpected EvaluationException: " + ex.getMessage());
-		}
-		catch (ParseException pe) {
-			fail("Unexpected ParseException: " + pe.getMessage());
-		}
 	}
 
 	/**
-	 * Evaluate an expression and check that the actual result matches the expectedValue and the class of the result
+	 * Evaluate an expression and check that the actual result matches the
-	 * matches the expectedClassOfResult. This method can also check if the expression is writable (for example, it is a
+	 * expectedValue and the class of the result matches the expectedClassOfResult.
-	 * variable or property reference).
+	 * This method can also check if the expression is writable (for example,
-	 *
+	 * it is a variable or property reference).
-	 * @param expression The expression to evaluate
+	 * @param expression the expression to evaluate
 	 * @param expectedValue the expected result for evaluating the expression
 	 * @param expectedClassOfResult the expected class of the evaluation result
 	 * @param shouldBeWritable should the parsed expression be writable?
 	 */
-	protected void evaluate(String expression, Object expectedValue, Class<?> expectedClassOfResult,
+	public void evaluate(String expression, Object expectedValue, Class<?> expectedClassOfResult, boolean shouldBeWritable) {
-			boolean shouldBeWritable) {
+		Expression expr = parser.parseExpression(expression);
-		try {
+		if (expr == null) {
-			Expression e = parser.parseExpression(expression);
-			if (e == null) {
 			fail("Parser returned null for expression");
 		}
 		if (DEBUG) {
-				SpelUtilities.printAbstractSyntaxTree(System.out, e);
+			SpelUtilities.printAbstractSyntaxTree(System.out, expr);
 		}
-			Object value = e.getValue(eContext);
+		Object value = expr.getValue(eContext);
 		if (value == null) {
-				if (expectedValue == null)
+			if (expectedValue == null) {
-					return; // no point doing other
+				return;  // no point doing other checks
-				// checks
+			}
-				assertEquals("Expression returned null value, but expected '" + expectedValue + "'", expectedValue,
+			assertEquals("Expression returned null value, but expected '" + expectedValue + "'", expectedValue, null);
-					null);
 		}
 		Class<? extends Object> resultType = value.getClass();
 		if (expectedValue instanceof String) {
@@ -178,15 +142,10 @@ public abstract class AbstractExpressionTests {
 		else {
 			assertEquals("Did not get expected value for expression '" + expression + "'.", expectedValue, value);
 		}
-			// assertEquals("Did not get expected value for expression '" + expression +
+		assertEquals("Type of the result was not as expected.  Expected '" + expectedClassOfResult +
-			// "'.", expectedValue, stringValueOf(value));
+				"' but result was of type '" + resultType + "'", expectedClassOfResult.equals(resultType), true);
-			assertEquals("Type of the result was not as expected.  Expected '" + expectedClassOfResult
-					+ "' but result was of type '" + resultType + "'",
-				expectedClassOfResult.equals/* isAssignableFrom */(resultType), true);
-			// TODO isAssignableFrom would allow some room for compatibility
-			// in the above expression...
 
-			boolean isWritable = e.isWritable(eContext);
+		boolean isWritable = expr.isWritable(eContext);
 		if (isWritable != shouldBeWritable) {
 			if (shouldBeWritable)
 				fail("Expected the expression to be writable but it is not");
@@ -194,37 +153,28 @@ public abstract class AbstractExpressionTests {
 				fail("Expected the expression to be readonly but it is not");
 		}
 	}
-		catch (EvaluationException ee) {
-			ee.printStackTrace();
-			fail("Unexpected Exception: " + ee.getMessage());
-		}
-		catch (ParseException pe) {
-			pe.printStackTrace();
-			fail("Unexpected Exception: " + pe.getMessage());
-		}
-	}
 
 	/**
-	 * Evaluate the specified expression and ensure the expected message comes out. The message may have inserts and
+	 * Evaluate the specified expression and ensure the expected message comes out.
-	 * they will be checked if otherProperties is specified. The first entry in otherProperties should always be the
+	 * The message may have inserts and they will be checked if otherProperties is specified.
-	 * position.
+	 * The first entry in otherProperties should always be the position.
-	 * @param expression The expression to evaluate
+	 * @param expression the expression to evaluate
-	 * @param expectedMessage The expected message
+	 * @param expectedMessage the expected message
-	 * @param otherProperties The expected inserts within the message
+	 * @param otherProperties the expected inserts within the message
 	 */
 	protected void evaluateAndCheckError(String expression, SpelMessage expectedMessage, Object... otherProperties) {
 		evaluateAndCheckError(expression, null, expectedMessage, otherProperties);
 	}
 
 	/**
-	 * Evaluate the specified expression and ensure the expected message comes out. The message may have inserts and
+	 * Evaluate the specified expression and ensure the expected message comes out.
-	 * they will be checked if otherProperties is specified. The first entry in otherProperties should always be the
+	 * The message may have inserts and they will be checked if otherProperties is specified.
-	 * position.
+	 * The first entry in otherProperties should always be the position.
-	 * @param expression The expression to evaluate
+	 * @param expression the expression to evaluate
-	 * @param expectedReturnType Ask the expression return value to be of this type if possible (null indicates don't
+	 * @param expectedReturnType ask the expression return value to be of this type if possible
-	 * ask for conversion)
+	 * ({@code null} indicates don't ask for conversion)
-	 * @param expectedMessage The expected message
+	 * @param expectedMessage the expected message
-	 * @param otherProperties The expected inserts within the message
+	 * @param otherProperties the expected inserts within the message
 	 */
 	protected void evaluateAndCheckError(String expression, Class<?> expectedReturnType, SpelMessage expectedMessage,
 			Object... otherProperties) {
@@ -234,19 +184,16 @@ public abstract class AbstractExpressionTests {
 				fail("Parser returned null for expression");
 			}
 			if (expectedReturnType != null) {
-				@SuppressWarnings("unused")
+				expr.getValue(eContext, expectedReturnType);
-				Object value = expr.getValue(eContext, expectedReturnType);
 			}
 			else {
-				@SuppressWarnings("unused")
+				expr.getValue(eContext);
-				Object value = expr.getValue(eContext);
 			}
 			fail("Should have failed with message " + expectedMessage);
 		}
 		catch (EvaluationException ee) {
 			SpelEvaluationException ex = (SpelEvaluationException) ee;
 			if (ex.getMessageCode() != expectedMessage) {
-				ex.printStackTrace();
 				assertEquals("Failed to get expected message", expectedMessage, ex.getMessageCode());
 			}
 			if (otherProperties != null && otherProperties.length != 0) {
@@ -260,47 +207,39 @@ public abstract class AbstractExpressionTests {
 						inserts = new Object[0];
 					}
 					if (inserts.length < otherProperties.length - 1) {
-						ex.printStackTrace();
+						fail("Cannot check " + (otherProperties.length - 1) +
-						fail("Cannot check " + (otherProperties.length - 1)
+								" properties of the exception, it only has " + inserts.length + " inserts");
-								+ " properties of the exception, it only has " + inserts.length + " inserts");
 					}
 					for (int i = 1; i < otherProperties.length; i++) {
 						if (otherProperties[i] == null) {
 							if (inserts[i - 1] != null) {
-								ex.printStackTrace();
+								fail("Insert does not match, expected 'null' but insert value was '" +
-								fail("Insert does not match, expected 'null' but insert value was '" + inserts[i - 1]
+										inserts[i - 1] + "'");
-										+ "'");
 							}
 						}
 						else if (inserts[i - 1] == null) {
 							if (otherProperties[i] != null) {
-								ex.printStackTrace();
+								fail("Insert does not match, expected '" + otherProperties[i] +
-								fail("Insert does not match, expected '" + otherProperties[i]
+										"' but insert value was 'null'");
-										+ "' but insert value was 'null'");
 							}
 						}
 						else if (!inserts[i - 1].equals(otherProperties[i])) {
-							ex.printStackTrace();
+							fail("Insert does not match, expected '" + otherProperties[i] +
-							fail("Insert does not match, expected '" + otherProperties[i] + "' but insert value was '"
+									"' but insert value was '" + inserts[i - 1] + "'");
-									+ inserts[i - 1] + "'");
 						}
 					}
 				}
 			}
 		}
-		catch (ParseException pe) {
-			pe.printStackTrace();
-			fail("Unexpected Exception: " + pe.getMessage());
-		}
 	}
 
 	/**
-	 * Parse the specified expression and ensure the expected message comes out. The message may have inserts and they
+	 * Parse the specified expression and ensure the expected message comes out.
-	 * will be checked if otherProperties is specified. The first entry in otherProperties should always be the
+	 * The message may have inserts and they will be checked if otherProperties is specified.
-	 * position.
+	 * The first entry in otherProperties should always be the position.
-	 * @param expression The expression to evaluate
+	 * @param expression the expression to evaluate
-	 * @param expectedMessage The expected message
+	 * @param expectedMessage the expected message
-	 * @param otherProperties The expected inserts within the message
+	 * @param otherProperties the expected inserts within the message
 	 */
 	protected void parseAndCheckError(String expression, SpelMessage expectedMessage, Object... otherProperties) {
 		try {
@@ -309,21 +248,8 @@ public abstract class AbstractExpressionTests {
 			fail("Parsing should have failed!");
 		}
 		catch (ParseException pe) {
-			// pe.printStackTrace();
+			SpelParseException ex = (SpelParseException)pe;
-			// Throwable t = pe.getCause();
-			// if (t == null) {
-			// fail("ParseException caught with no defined cause");
-			// }
-			// if (!(t instanceof SpelEvaluationException)) {
-			// t.printStackTrace();
-			// fail("Cause of parse exception is not a SpelException");
-			// }
-			// SpelEvaluationException ex = (SpelEvaluationException) t;
-			// pe.printStackTrace();
-			SpelParseException ex = (SpelParseException) pe;
 			if (ex.getMessageCode() != expectedMessage) {
-				// System.out.println(ex.getMessage());
-				ex.printStackTrace();
 				assertEquals("Failed to get expected message", expectedMessage, ex.getMessageCode());
 			}
 			if (otherProperties != null && otherProperties.length != 0) {
@@ -337,15 +263,13 @@ public abstract class AbstractExpressionTests {
 						inserts = new Object[0];
 					}
 					if (inserts.length < otherProperties.length - 1) {
-						ex.printStackTrace();
+						fail("Cannot check " + (otherProperties.length - 1) +
-						fail("Cannot check " + (otherProperties.length - 1)
+								" properties of the exception, it only has " + inserts.length + " inserts");
-								+ " properties of the exception, it only has " + inserts.length + " inserts");
 					}
 					for (int i = 1; i < otherProperties.length; i++) {
 						if (!inserts[i - 1].equals(otherProperties[i])) {
-							ex.printStackTrace();
+							fail("Insert does not match, expected '" + otherProperties[i] +
-							fail("Insert does not match, expected '" + otherProperties[i] + "' but insert value was '"
+									"' but insert value was '" + inserts[i - 1] + "'");
-									+ inserts[i - 1] + "'");
 						}
 					}
 				}
@@ -353,13 +277,13 @@ public abstract class AbstractExpressionTests {
 		}
 	}
 
+
 	protected static String stringValueOf(Object value) {
 		return stringValueOf(value, false);
 	}
 
 	/**
 	 * Produce a nice string representation of the input object.
-	 *
 	 * @param value object to be formatted
 	 * @return a nice string
 	 */
@@ -395,8 +319,8 @@ public abstract class AbstractExpressionTests {
 					sb.append("}");
 				}
 				else {
-					throw new RuntimeException("Please implement support for type " + primitiveType.getName()
+					throw new RuntimeException("Please implement support for type " + primitiveType.getName() +
-							+ " in ExpressionTestCase.stringValueOf()");
+							" in ExpressionTestCase.stringValueOf()");
 				}
 			}
 			else if (value.getClass().getComponentType().isArray()) {

spring-expression/src/test/java/org/springframework/expression/spel/MethodInvocationTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2014 the original author or authors.
+ * Copyright 2002-2015 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -54,17 +54,6 @@ public class MethodInvocationTests extends AbstractExpressionTests {
 		evaluate("getPlaceOfBirth().getCity()", "SmilJan", String.class);
 	}
 
-	// public void testBuiltInProcessors() {
-	// evaluate("new int[]{1,2,3,4}.count()", 4, Integer.class);
-	// evaluate("new int[]{4,3,2,1}.sort()[3]", 4, Integer.class);
-	// evaluate("new int[]{4,3,2,1}.average()", 2, Integer.class);
-	// evaluate("new int[]{4,3,2,1}.max()", 4, Integer.class);
-	// evaluate("new int[]{4,3,2,1}.min()", 1, Integer.class);
-	// evaluate("new int[]{4,3,2,1,2,3}.distinct().count()", 4, Integer.class);
-	// evaluate("{1,2,3,null}.nonnull().count()", 3, Integer.class);
-	// evaluate("new int[]{4,3,2,1,2,3}.distinct().count()", 4, Integer.class);
-	// }
-
 	@Test
 	public void testStringClass() {
 		evaluate("new java.lang.String('hello').charAt(2)", 'l', Character.class);
@@ -107,7 +96,7 @@ public class MethodInvocationTests extends AbstractExpressionTests {
 
 		// Normal exit
 		StandardEvaluationContext eContext = TestScenarioCreator.getTestEvaluationContext();
-		eContext.setVariable("bar",3);
+		eContext.setVariable("bar", 3);
 		Object o = expr.getValue(eContext);
 		assertEquals(o, 3);
 		assertEquals(1, parser.parseExpression("counter").getValue(eContext));
@@ -115,46 +104,44 @@ public class MethodInvocationTests extends AbstractExpressionTests {
 		// Now the expression has cached that throwException(int) is the right thing to call
 		// Let's change 'bar' to be a PlaceOfBirth which indicates the cached reference is
 		// out of date.
-		eContext.setVariable("bar",new PlaceOfBirth("London"));
+		eContext.setVariable("bar", new PlaceOfBirth("London"));
 		o = expr.getValue(eContext);
 		assertEquals("London", o);
 		// That confirms the logic to mark the cached reference stale and retry is working
 
-
+		// Now let's cause the method to exit via exception and ensure it doesn't cause a retry.
-		// Now let's cause the method to exit via exception and ensure it doesn't cause
-		// a retry.
 
 		// First, switch back to throwException(int)
-		eContext.setVariable("bar",3);
+		eContext.setVariable("bar", 3);
 		o = expr.getValue(eContext);
 		assertEquals(3, o);
 		assertEquals(2, parser.parseExpression("counter").getValue(eContext));
 
 
 		// Now cause it to throw an exception:
-		eContext.setVariable("bar",1);
+		eContext.setVariable("bar", 1);
 		try {
 			o = expr.getValue(eContext);
 			fail();
-		} catch (Exception e) {
+		}
-			if (e instanceof SpelEvaluationException) {
+		catch (Exception ex) {
-				e.printStackTrace();
+			if (ex instanceof SpelEvaluationException) {
-				fail("Should not be a SpelEvaluationException");
+				fail("Should not be a SpelEvaluationException: " + ex);
 			}
 			// normal
 		}
 		// If counter is 4 then the method got called twice!
 		assertEquals(3, parser.parseExpression("counter").getValue(eContext));
 
-		eContext.setVariable("bar",4);
+		eContext.setVariable("bar", 4);
 		try {
 			o = expr.getValue(eContext);
 			fail();
-		} catch (Exception e) {
+		}
+		catch (Exception ex) {
 			// 4 means it will throw a checked exception - this will be wrapped
-			if (!(e instanceof ExpressionInvocationTargetException)) {
+			if (!(ex instanceof ExpressionInvocationTargetException)) {
-				e.printStackTrace();
+				fail("Should have been wrapped: " + ex);
-				fail("Should have been wrapped");
 			}
 			// normal
 		}
@@ -176,14 +163,14 @@ public class MethodInvocationTests extends AbstractExpressionTests {
 		SpelExpressionParser parser = new SpelExpressionParser();
 		Expression expr = parser.parseExpression("throwException(#bar)");
 
-		eContext.setVariable("bar",2);
+		eContext.setVariable("bar", 2);
 		try {
 			expr.getValue(eContext);
 			fail();
-		} catch (Exception e) {
+		}
-			if (e instanceof SpelEvaluationException) {
+		catch (Exception ex) {
-				e.printStackTrace();
+			if (ex instanceof SpelEvaluationException) {
-				fail("Should not be a SpelEvaluationException");
+				fail("Should not be a SpelEvaluationException: " + ex);
 			}
 			// normal
 		}
@@ -200,18 +187,16 @@ public class MethodInvocationTests extends AbstractExpressionTests {
 		SpelExpressionParser parser = new SpelExpressionParser();
 		Expression expr = parser.parseExpression("throwException(#bar)");
 
-		eContext.setVariable("bar",4);
+		eContext.setVariable("bar", 4);
 		try {
 			expr.getValue(eContext);
 			fail();
-		} catch (ExpressionInvocationTargetException e) {
-			Throwable t = e.getCause();
-			assertEquals(
-					"org.springframework.expression.spel.testresources.Inventor$TestException",
-					t.getClass().getName());
-			return;
 		}
-		fail("Should not be a SpelEvaluationException");
+		catch (ExpressionInvocationTargetException ex) {
+			Throwable cause = ex.getCause();
+			assertEquals("org.springframework.expression.spel.testresources.Inventor$TestException",
+					cause.getClass().getName());
+		}
 	}
 
 	@Test
@@ -224,7 +209,7 @@ public class MethodInvocationTests extends AbstractExpressionTests {
 
 		// Filter will be called but not do anything, so first doit() will be invoked
 		SpelExpression expr = (SpelExpression) parser.parseExpression("doit(1)");
-		String result = expr.getValue(context,String.class);
+		String result = expr.getValue(context, String.class);
 		assertEquals("1", result);
 		assertTrue(filter.filterCalled);
 
@@ -232,15 +217,15 @@ public class MethodInvocationTests extends AbstractExpressionTests {
 		filter.removeIfNotAnnotated = true;
 		filter.filterCalled = false;
 		expr = (SpelExpression) parser.parseExpression("doit(1)");
-		result = expr.getValue(context,String.class);
+		result = expr.getValue(context, String.class);
 		assertEquals("double 1.0", result);
 		assertTrue(filter.filterCalled);
 
 		// check not called for other types
-		filter.filterCalled=false;
+		filter.filterCalled = false;
 		context.setRootObject(new String("abc"));
 		expr = (SpelExpression) parser.parseExpression("charAt(0)");
-		result = expr.getValue(context,String.class);
+		result = expr.getValue(context, String.class);
 		assertEquals("a", result);
 		assertFalse(filter.filterCalled);
 
@@ -249,64 +234,11 @@ public class MethodInvocationTests extends AbstractExpressionTests {
 		context.registerMethodFilter(TestObject.class,null);//clear filter
 		context.setRootObject(new TestObject());
 		expr = (SpelExpression) parser.parseExpression("doit(1)");
-		result = expr.getValue(context,String.class);
+		result = expr.getValue(context, String.class);
 		assertEquals("1", result);
 		assertFalse(filter.filterCalled);
 	}
 
-	// Simple filter
-	static class LocalFilter implements MethodFilter {
-
-		public boolean removeIfNotAnnotated = false;
-
-		public boolean filterCalled = false;
-
-		private boolean isAnnotated(Method m) {
-			Annotation[] annos = m.getAnnotations();
-			if (annos==null) {
-				return false;
-			}
-			for (Annotation anno: annos) {
-				String s = anno.annotationType().getName();
-				if (s.endsWith("Anno")) {
-					return true;
-				}
-			}
-			return false;
-		}
-
-		@Override
-		public List<Method> filter(List<Method> methods) {
-			filterCalled = true;
-			List<Method> forRemoval = new ArrayList<Method>();
-			for (Method m: methods) {
-				if (removeIfNotAnnotated && !isAnnotated(m)) {
-					forRemoval.add(m);
-				}
-			}
-			for (Method m: forRemoval) {
-				methods.remove(m);
-			}
-			return methods;
-		}
-
-	}
-
-	@Retention(RetentionPolicy.RUNTIME)
-	@interface Anno {}
-
-	class TestObject {
-		public int doit(int i) {
-			return i;
-		}
-
-		@Anno
-		public String doit(double d) {
-			return "double "+d;
-		}
-
-	}
-
 	@Test
 	public void testAddingMethodResolvers() {
 		StandardEvaluationContext ctx = new StandardEvaluationContext();
@@ -329,17 +261,6 @@ public class MethodInvocationTests extends AbstractExpressionTests {
 		assertEquals(2, ctx.getMethodResolvers().size());
 	}
 
-	static class DummyMethodResolver implements MethodResolver {
-
-		@Override
-		public MethodExecutor resolve(EvaluationContext context, Object targetObject, String name,
-				List<TypeDescriptor> argumentTypes) throws AccessException {
-			throw new UnsupportedOperationException("Auto-generated method stub");
-		}
-
-	}
-
-
 	@Test
 	public void testVarargsInvocation01() {
 		// Calling 'public int aVarargsMethod(String... strings)'
@@ -383,8 +304,7 @@ public class MethodInvocationTests extends AbstractExpressionTests {
 		StandardEvaluationContext context = new StandardEvaluationContext(bytes);
 		context.setBeanResolver(new BeanResolver() {
 			@Override
-			public Object resolve(EvaluationContext context, String beanName)
+			public Object resolve(EvaluationContext context, String beanName) throws AccessException {
-					throws AccessException {
 				if ("service".equals(beanName)) {
 					return service;
 				}
@@ -396,10 +316,78 @@ public class MethodInvocationTests extends AbstractExpressionTests {
 		assertSame(bytes, outBytes);
 	}
 
+
+	// Simple filter
+	static class LocalFilter implements MethodFilter {
+
+		public boolean removeIfNotAnnotated = false;
+
+		public boolean filterCalled = false;
+
+		private boolean isAnnotated(Method method) {
+			Annotation[] anns = method.getAnnotations();
+			if (anns == null) {
+				return false;
+			}
+			for (Annotation ann : anns) {
+				String name = ann.annotationType().getName();
+				if (name.endsWith("Anno")) {
+					return true;
+				}
+			}
+			return false;
+		}
+
+		@Override
+		public List<Method> filter(List<Method> methods) {
+			filterCalled = true;
+			List<Method> forRemoval = new ArrayList<Method>();
+			for (Method method: methods) {
+				if (removeIfNotAnnotated && !isAnnotated(method)) {
+					forRemoval.add(method);
+				}
+			}
+			for (Method method: forRemoval) {
+				methods.remove(method);
+			}
+			return methods;
+		}
+	}
+
+
+	@Retention(RetentionPolicy.RUNTIME)
+	@interface Anno {
+	}
+
+
+	class TestObject {
+
+		public int doit(int i) {
+			return i;
+		}
+
+		@Anno
+		public String doit(double d) {
+			return "double "+d;
+		}
+	}
+
+
+	static class DummyMethodResolver implements MethodResolver {
+
+		@Override
+		public MethodExecutor resolve(EvaluationContext context, Object targetObject, String name,
+				List<TypeDescriptor> argumentTypes) throws AccessException {
+			throw new UnsupportedOperationException();
+		}
+	}
+
+
 	public static class BytesService {
 
 		public byte[] handleBytes(byte[] bytes) {
 			return bytes;
 		}
 	}
+
 }

spring-expression/src/test/java/org/springframework/expression/spel/SpelReproTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2014 the original author or authors.
+ * Copyright 2002-2015 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -1895,6 +1895,15 @@ public class SpelReproTests extends AbstractExpressionTests {
 		assertTrue(((List) value).isEmpty());
 	}
 
+	@Test
+	public void SPR12803() throws Exception {
+		StandardEvaluationContext sec = new StandardEvaluationContext();
+		sec.setVariable("iterable", Collections.emptyList());
+		SpelExpressionParser parser = new SpelExpressionParser();
+		Expression expression = parser.parseExpression("T(org.springframework.expression.spel.SpelReproTests.GuavaLists).newArrayList(#iterable)");
+		assertTrue(expression.getValue(sec) instanceof ArrayList);
+	}
+
 
 	private static enum ABC { A, B, C }
 
@@ -2180,4 +2189,16 @@ public class SpelReproTests extends AbstractExpressionTests {
 		}
 	}
 
+
+	public static class GuavaLists {
+
+		public static <T> List<T> newArrayList(Iterable<T> iterable) {
+			return new ArrayList<T>();
+		}
+
+		public static <T> List<T> newArrayList(Object... elements) {
+			throw new UnsupportedOperationException();
+		}
+	}
+
 }

spring-expression/src/test/java/org/springframework/expression/spel/TestScenarioCreator.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2009 the original author or authors.
+ * Copyright 2002-2015 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -22,9 +22,9 @@ import org.springframework.expression.spel.support.StandardEvaluationContext;
 import org.springframework.expression.spel.testresources.Inventor;
 import org.springframework.expression.spel.testresources.PlaceOfBirth;
 
-///CLOVER:OFF
 /**
- * Builds an evaluation context for test expressions. Features of the test evaluation context are:
+ * Builds an evaluation context for test expressions.
+ * Features of the test evaluation context are:
  * <ul>
  * <li>The root context object is an Inventor instance {@link Inventor}
  * </ul>
@@ -45,21 +45,19 @@ public class TestScenarioCreator {
 	 */
 	private static void populateFunctions(StandardEvaluationContext testContext) {
 		try {
-			testContext.registerFunction("isEven", TestScenarioCreator.class.getDeclaredMethod("isEven",
+			testContext.registerFunction("isEven",
-					new Class[] { Integer.TYPE }));
+					TestScenarioCreator.class.getDeclaredMethod("isEven", Integer.TYPE));
-			testContext.registerFunction("reverseInt", TestScenarioCreator.class.getDeclaredMethod("reverseInt",
+			testContext.registerFunction("reverseInt",
-					new Class[] { Integer.TYPE, Integer.TYPE, Integer.TYPE }));
+					TestScenarioCreator.class.getDeclaredMethod("reverseInt", Integer.TYPE, Integer.TYPE, Integer.TYPE));
-			testContext.registerFunction("reverseString", TestScenarioCreator.class.getDeclaredMethod("reverseString",
+			testContext.registerFunction("reverseString",
-					new Class[] { String.class }));
+					TestScenarioCreator.class.getDeclaredMethod("reverseString", String.class));
-			testContext.registerFunction("varargsFunctionReverseStringsAndMerge", TestScenarioCreator.class
+			testContext.registerFunction("varargsFunctionReverseStringsAndMerge",
-					.getDeclaredMethod("varargsFunctionReverseStringsAndMerge", new Class[] { String[].class }));
+					TestScenarioCreator.class.getDeclaredMethod("varargsFunctionReverseStringsAndMerge", String[].class));
-			testContext.registerFunction("varargsFunctionReverseStringsAndMerge2", TestScenarioCreator.class
+			testContext.registerFunction("varargsFunctionReverseStringsAndMerge2",
-					.getDeclaredMethod("varargsFunctionReverseStringsAndMerge2", new Class[] { Integer.TYPE,
+					TestScenarioCreator.class.getDeclaredMethod("varargsFunctionReverseStringsAndMerge2", Integer.TYPE, String[].class));
-							String[].class }));
+		}
-		} catch (SecurityException e) {
+		catch (Exception ex) {
-			e.printStackTrace();
+			throw new IllegalStateException(ex);
-		} catch (NoSuchMethodException e) {
-			e.printStackTrace();
 		}
 	}
 
@@ -72,9 +70,8 @@ public class TestScenarioCreator {
 	}
 
 	/**
-	 * Create the root context object, an Inventor instance. Non-qualified property and method references will be
+	 * Create the root context object, an Inventor instance. Non-qualified property
-	 * resolved against this context object.
+	 * and method references will be resolved against this context object.
-	 *
 	 * @param testContext the evaluation context in which to set the root object
 	 */
 	private static void setupRootContextObject(StandardEvaluationContext testContext) {
@@ -88,12 +85,14 @@ public class TestScenarioCreator {
 		testContext.setRootObject(tesla);
 	}
 
+
 	// These methods are registered in the test context and therefore accessible through function calls
 	// in test expressions
 
 	public static String isEven(int i) {
-		if ((i % 2) == 0)
+		if ((i % 2) == 0) {
 			return "y";
+		}
 		return "n";
 	}
 
@@ -129,4 +128,5 @@ public class TestScenarioCreator {
 		}
 		return sb.toString();
 	}
+
 }