Fix SpEL compilation of static method/property/field operations

Before this change the compilation of a method reference or property/field
access was not properly cleaning up the stack if compilation meant
calling a static method or accessing a static field. In these cases there
is no need for a target object on the stack and it should be removed if
present. For a simple expression it is harmless since the end result of
the expression is the thing on the top of the stack, but for nested
expressions if the inner expression suffered this issue, the outer
expression can find itself operating on the wrong element.

The particular issue covered the case of a static field access but this
fix (and associated tests) cover static method, property and field access.

Issue: SPR-13781
(cherry picked from commit a28fc76)

spring-expression/src/main/java/org/springframework/expression/spel/ast/CompoundExpression.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,7 +62,7 @@ public class CompoundExpression extends SpelNodeImpl {
 			}
 			try {
 				state.pushActiveContextObject(result);
-				nextNode = this.children[cc-1];
+				nextNode = this.children[cc - 1];
 				return nextNode.getValueRef(state);
 			}
 			finally {
@@ -124,14 +124,8 @@ public class CompoundExpression extends SpelNodeImpl {
 	
 	@Override
 	public void generateCode(MethodVisitor mv, CodeFlow cf) {
-		// TODO could optimize T(SomeType).staticMethod - no need to generate the T() part
 		for (int i = 0; i < this.children.length;i++) {
-			SpelNodeImpl child = this.children[i];
-			if (child instanceof TypeReference && (i + 1) < this.children.length &&
-					this.children[i + 1] instanceof MethodReference) {
-				continue;
-			}
-			child.generateCode(mv, cf);
+			this.children[i].generateCode(mv, cf);
 		}
 		cf.pushDescriptor(this.exitTypeDescriptor);
 	}

spring-expression/src/main/java/org/springframework/expression/spel/ast/MethodReference.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.
@@ -287,35 +287,40 @@ public class MethodReference extends SpelNodeImpl {
 			throw new IllegalStateException("No applicable cached executor found: " + executorToCheck);
 		}
 
-		ReflectiveMethodExecutor methodExecutor = (ReflectiveMethodExecutor)executorToCheck.get();
+		ReflectiveMethodExecutor methodExecutor = (ReflectiveMethodExecutor) executorToCheck.get();
 		Method method = methodExecutor.getMethod();
 		boolean isStaticMethod = Modifier.isStatic(method.getModifiers());
 		String descriptor = cf.lastDescriptor();
 
-		if (descriptor == null && !isStaticMethod) {
+		if (descriptor == null) {
+			if (!isStaticMethod) {
+				// Nothing on the stack but something is needed
 				cf.loadTarget(mv);
 			}
+		}
+		else {
+			if (isStaticMethod) {
+				// Something on the stack when nothing is needed
+				mv.visitInsn(POP);
+			}
+		}
 		
 		if (CodeFlow.isPrimitive(descriptor)) {
 			CodeFlow.insertBoxIfNecessary(mv, descriptor.charAt(0));
 		}
 
-		boolean itf = method.getDeclaringClass().isInterface();
-		String methodDeclaringClassSlashedDescriptor = null;
-		if (Modifier.isPublic(method.getDeclaringClass().getModifiers())) {
-			methodDeclaringClassSlashedDescriptor = method.getDeclaringClass().getName().replace('.', '/');
-		}
-		else {
-			methodDeclaringClassSlashedDescriptor = methodExecutor.getPublicDeclaringClass().getName().replace('.', '/');			
-		}
+		String classDesc = (Modifier.isPublic(method.getDeclaringClass().getModifiers()) ?
+				method.getDeclaringClass().getName().replace('.', '/') :
+				methodExecutor.getPublicDeclaringClass().getName().replace('.', '/'));
 		if (!isStaticMethod) {
-			if (descriptor == null || !descriptor.substring(1).equals(methodDeclaringClassSlashedDescriptor)) {
-				CodeFlow.insertCheckCast(mv, "L"+ methodDeclaringClassSlashedDescriptor);
+			if (descriptor == null || !descriptor.substring(1).equals(classDesc)) {
+				CodeFlow.insertCheckCast(mv, "L" + classDesc);
 			}
 		}
-		generateCodeForArguments(mv, cf, method, children);		
-		mv.visitMethodInsn(isStaticMethod ? INVOKESTATIC : INVOKEVIRTUAL,
-				methodDeclaringClassSlashedDescriptor, method.getName(), CodeFlow.createSignatureDescriptor(method), itf);
+
+		generateCodeForArguments(mv, cf, method, this.children);
+		mv.visitMethodInsn((isStaticMethod ? INVOKESTATIC : INVOKEVIRTUAL), classDesc, method.getName(),
+				CodeFlow.createSignatureDescriptor(method), method.getDeclaringClass().isInterface());
 		cf.pushDescriptor(this.exitTypeDescriptor);
 	}
 

spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectivePropertyAccessor.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.
@@ -58,7 +58,10 @@ import org.springframework.util.StringUtils;
  */
 public class ReflectivePropertyAccessor implements PropertyAccessor {
 
+	private static final Set<Class<?>> ANY_TYPES = Collections.emptySet();
+
 	private static final Set<Class<?>> BOOLEAN_TYPES;
+
 	static {
 		Set<Class<?>> booleanTypes = new HashSet<Class<?>>();
 		booleanTypes.add(Boolean.class);
@@ -66,8 +69,6 @@ public class ReflectivePropertyAccessor implements PropertyAccessor {
 		BOOLEAN_TYPES = Collections.unmodifiableSet(booleanTypes);
 	}
 
-	private static final Set<Class<?>> ANY_TYPES = Collections.emptySet();
-
 
 	private final Map<CacheKey, InvokerPair> readerCache = new ConcurrentHashMap<CacheKey, InvokerPair>(64);
 
@@ -122,7 +123,7 @@ public class ReflectivePropertyAccessor implements PropertyAccessor {
 	}
 
 	public Member getLastReadInvokerPair() {
-		return lastReadInvokerPair.member;
+		return this.lastReadInvokerPair.member;
 	}
 
 	@Override
@@ -165,7 +166,7 @@ public class ReflectivePropertyAccessor implements PropertyAccessor {
 					return new TypedValue(value, invoker.typeDescriptor.narrow(value));
 				}
 				catch (Exception ex) {
-					throw new AccessException("Unable to access property '" + name + "' through getter", ex);
+					throw new AccessException("Unable to access property '" + name + "' through getter method", ex);
 				}
 			}
 		}
@@ -187,12 +188,12 @@ public class ReflectivePropertyAccessor implements PropertyAccessor {
 					return new TypedValue(value, invoker.typeDescriptor.narrow(value));
 				}
 				catch (Exception ex) {
-					throw new AccessException("Unable to access field: " + name, ex);
+					throw new AccessException("Unable to access field '" + name + "'", ex);
 				}
 			}
 		}
 
-		throw new AccessException("Neither getter nor field found for property '" + name + "'");
+		throw new AccessException("Neither getter method nor field found for property '" + name + "'");
 	}
 
 	@Override
@@ -240,7 +241,7 @@ public class ReflectivePropertyAccessor implements PropertyAccessor {
 						newValue, TypeDescriptor.forObject(newValue), typeDescriptor);
 			}
 			catch (EvaluationException evaluationException) {
-				throw new AccessException("Type conversion failure",evaluationException);
+				throw new AccessException("Type conversion failure", evaluationException);
 			}
 		}
 		CacheKey cacheKey = new CacheKey(type, name, target instanceof Class);
@@ -262,7 +263,7 @@ public class ReflectivePropertyAccessor implements PropertyAccessor {
 					return;
 				}
 				catch (Exception ex) {
-					throw new AccessException("Unable to access property '" + name + "' through setter", ex);
+					throw new AccessException("Unable to access property '" + name + "' through setter method", ex);
 				}
 			}
 		}
@@ -283,12 +284,12 @@ public class ReflectivePropertyAccessor implements PropertyAccessor {
 					return;
 				}
 				catch (Exception ex) {
-					throw new AccessException("Unable to access field: " + name, ex);
+					throw new AccessException("Unable to access field '" + name + "'", ex);
 				}
 			}
 		}
 
-		throw new AccessException("Neither setter nor field found for property '" + name + "'");
+		throw new AccessException("Neither setter method nor field found for property '" + name + "'");
 	}
 
 	private TypeDescriptor getTypeDescriptor(EvaluationContext context, Object target, String name) {
@@ -469,11 +470,11 @@ public class ReflectivePropertyAccessor implements PropertyAccessor {
 		InvokerPair invocationTarget = this.readerCache.get(cacheKey);
 
 		if (invocationTarget == null || invocationTarget.member instanceof Method) {
-			Method method = (Method) (invocationTarget==null?null:invocationTarget.member);
+			Method method = (Method) (invocationTarget != null ? invocationTarget.member : null);
 			if (method == null) {
 				method = findGetterForProperty(name, type, target);
 				if (method != null) {
-					invocationTarget = new InvokerPair(method,new TypeDescriptor(new MethodParameter(method,-1)));
+					invocationTarget = new InvokerPair(method, new TypeDescriptor(new MethodParameter(method, -1)));
 					ReflectionUtils.makeAccessible(method);
 					this.readerCache.put(cacheKey, invocationTarget);
 				}
@@ -497,6 +498,7 @@ public class ReflectivePropertyAccessor implements PropertyAccessor {
 				return new OptimalPropertyAccessor(invocationTarget);
 			}
 		}
+
 		return this;
 	}
 
@@ -577,16 +579,8 @@ public class ReflectivePropertyAccessor implements PropertyAccessor {
 		OptimalPropertyAccessor(InvokerPair target) {
 			this.member = target.member;
 			this.typeDescriptor = target.typeDescriptor;
-			if (this.member instanceof Field) {
-				Field field = (Field) this.member;
-				this.needsToBeMadeAccessible = (!Modifier.isPublic(field.getModifiers()) ||
-						!Modifier.isPublic(field.getDeclaringClass().getModifiers())) && !field.isAccessible();
-			}
-			else {
-				Method method = (Method) this.member;
-				this.needsToBeMadeAccessible = ((!Modifier.isPublic(method.getModifiers()) ||
-						!Modifier.isPublic(method.getDeclaringClass().getModifiers())) && !method.isAccessible());
-			}
+			this.needsToBeMadeAccessible = (!Modifier.isPublic(this.member.getModifiers()) ||
+					!Modifier.isPublic(this.member.getDeclaringClass().getModifiers()));
 		}
 
 		@Override
@@ -599,10 +593,12 @@ public class ReflectivePropertyAccessor implements PropertyAccessor {
 			if (target == null) {
 				return false;
 			}
+
 			Class<?> type = (target instanceof Class ? (Class<?>) target : target.getClass());
 			if (type.isArray()) {
 				return false;
 			}
+
 			if (this.member instanceof Method) {
 				Method method = (Method) this.member;
 				String getterName = "get" + StringUtils.capitalize(name);
@@ -621,30 +617,31 @@ public class ReflectivePropertyAccessor implements PropertyAccessor {
 		@Override
 		public TypedValue read(EvaluationContext context, Object target, String name) throws AccessException {
 			if (this.member instanceof Method) {
+				Method method = (Method) this.member;
 				try {
-					if (this.needsToBeMadeAccessible) {
-						ReflectionUtils.makeAccessible((Method) this.member);
+					if (this.needsToBeMadeAccessible && !method.isAccessible()) {
+						method.setAccessible(true);
 					}
-					Object value = ((Method) this.member).invoke(target);
+					Object value = method.invoke(target);
 					return new TypedValue(value, this.typeDescriptor.narrow(value));
 				}
 				catch (Exception ex) {
-					throw new AccessException("Unable to access property '" + name + "' through getter", ex);
+					throw new AccessException("Unable to access property '" + name + "' through getter method", ex);
 				}
 			}
-			if (this.member instanceof Field) {
+			else {
+				Field field = (Field) this.member;
 				try {
-					if (this.needsToBeMadeAccessible) {
-						ReflectionUtils.makeAccessible((Field) this.member);
+					if (this.needsToBeMadeAccessible && !field.isAccessible()) {
+						field.setAccessible(true);
 					}
-					Object value = ((Field) this.member).get(target);
+					Object value = field.get(target);
 					return new TypedValue(value, this.typeDescriptor.narrow(value));
 				}
 				catch (Exception ex) {
-					throw new AccessException("Unable to access field: " + name, ex);
+					throw new AccessException("Unable to access field '" + name + "'", ex);
 				}
 			}
-			throw new AccessException("Neither getter nor field found for property '" + name + "'");
 		}
 
 		@Override
@@ -665,11 +662,11 @@ public class ReflectivePropertyAccessor implements PropertyAccessor {
 
 		@Override
 		public Class<?> getPropertyType() {
-			if (this.member instanceof Field) {
-				return ((Field) this.member).getType();
+			if (this.member instanceof Method) {
+				return ((Method) this.member).getReturnType();
 			}
 			else {
-				return ((Method) this.member).getReturnType();
+				return ((Field) this.member).getType();
 			}
 		}
 
@@ -677,22 +674,31 @@ public class ReflectivePropertyAccessor implements PropertyAccessor {
 		public void generateCode(String propertyName, MethodVisitor mv, CodeFlow cf) {
 			boolean isStatic = Modifier.isStatic(this.member.getModifiers());
 			String descriptor = cf.lastDescriptor();
-			String memberDeclaringClassSlashedDescriptor = this.member.getDeclaringClass().getName().replace('.', '/');
+			String classDesc = this.member.getDeclaringClass().getName().replace('.', '/');
+
 			if (!isStatic) {
 				if (descriptor == null) {
 					cf.loadTarget(mv);
 				}
-				if (descriptor == null || !memberDeclaringClassSlashedDescriptor.equals(descriptor.substring(1))) {
-					mv.visitTypeInsn(CHECKCAST, memberDeclaringClassSlashedDescriptor);
+				if (descriptor == null || !classDesc.equals(descriptor.substring(1))) {
+					mv.visitTypeInsn(CHECKCAST, classDesc);
 				}
 			}
-			if (this.member instanceof Field) {
-				mv.visitFieldInsn(isStatic ? GETSTATIC : GETFIELD, memberDeclaringClassSlashedDescriptor,
-						this.member.getName(), CodeFlow.toJvmDescriptor(((Field) this.member).getType()));
+			else {
+				if (descriptor != null) {
+					// A static field/method call will not consume what is on the stack,
+					// it needs to be popped off.
+					mv.visitInsn(POP);
+				}
+			}
+
+			if (this.member instanceof Method) {
+				mv.visitMethodInsn((isStatic ? INVOKESTATIC : INVOKEVIRTUAL), classDesc, this.member.getName(),
+						CodeFlow.createSignatureDescriptor((Method) this.member), false);
 			}
 			else {
-				mv.visitMethodInsn(isStatic ? INVOKESTATIC : INVOKEVIRTUAL, memberDeclaringClassSlashedDescriptor,
-						this.member.getName(), CodeFlow.createSignatureDescriptor((Method) this.member),false);
+				mv.visitFieldInsn((isStatic ? GETSTATIC : GETFIELD), classDesc, this.member.getName(),
+						CodeFlow.toJvmDescriptor(((Field) this.member).getType()));
 			}
 		}
 	}

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

@@ -2008,6 +2008,103 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 		assertTrue(expression.getValue(Boolean.class));
 	}
 
+
+	/**
+	 * Test variants of using T(...) and static/non-static method/property/field references.
+	 */
+	@Test
+	public void constructorReference_SPR13781() {
+		// Static field access on a T() referenced type
+		expression = parser.parseExpression("T(java.util.Locale).ENGLISH");
+		assertEquals("en",expression.getValue().toString());
+		assertCanCompile(expression);
+		assertEquals("en",expression.getValue().toString());
+
+		// The actual expression from the bug report. It fails if the ENGLISH reference fails
+		// to pop the type reference for Locale off the stack (if it isn't popped then
+		// toLowerCase() will be called with a Locale parameter). In this situation the
+		// code generation for ENGLISH should notice there is something on the stack that
+		// is not required and pop it off.
+		expression = parser.parseExpression("#userId.toString().toLowerCase(T(java.util.Locale).ENGLISH)");
+		StandardEvaluationContext context =
+				new StandardEvaluationContext();
+		context.setVariable("userId", "RoDnEy");
+		assertEquals("rodney",expression.getValue(context));
+		assertCanCompile(expression);
+		assertEquals("rodney",expression.getValue(context));
+
+		// Property access on a class object
+		expression = parser.parseExpression("T(String).name");
+		assertEquals("java.lang.String",expression.getValue());
+		assertCanCompile(expression);
+		assertEquals("java.lang.String",expression.getValue());
+
+		// Now the type reference isn't on the stack, and needs loading
+		context = new StandardEvaluationContext(String.class);
+		expression = parser.parseExpression("name");
+		assertEquals("java.lang.String",expression.getValue(context));
+		assertCanCompile(expression);
+		assertEquals("java.lang.String",expression.getValue(context));
+
+		expression = parser.parseExpression("T(String).getName()");
+		assertEquals("java.lang.String",expression.getValue());
+		assertCanCompile(expression);
+		assertEquals("java.lang.String",expression.getValue());
+		
+		// These tests below verify that the chain of static accesses (either method/property or field)
+		// leave the right thing on top of the stack for processing by any outer consuming code.
+		// Here the consuming code is the String.valueOf() function.  If the wrong thing were on
+		// the stack (for example if the compiled code for static methods wasn't popping the 
+		// previous thing off the stack) the valueOf() would operate on the wrong value.
+
+		String shclass = StaticsHelper.class.getName();
+		// Basic chain: property access then method access
+		expression = parser.parseExpression("T(String).valueOf(T(String).name.valueOf(1))");
+		assertEquals("1",expression.getValue());
+		assertCanCompile(expression);
+		assertEquals("1",expression.getValue());
+
+		// chain of statics ending with static method
+		expression = parser.parseExpression("T(String).valueOf(T("+shclass+").methoda().methoda().methodb())");
+		assertEquals("mb",expression.getValue());
+		assertCanCompile(expression);
+		assertEquals("mb",expression.getValue());
+
+		// chain of statics ending with static field
+		expression = parser.parseExpression("T(String).valueOf(T("+shclass+").fielda.fielda.fieldb)");
+		assertEquals("fb",expression.getValue());
+		assertCanCompile(expression);
+		assertEquals("fb",expression.getValue());
+
+		// chain of statics ending with static property access
+		expression = parser.parseExpression("T(String).valueOf(T("+shclass+").propertya.propertya.propertyb)");
+		assertEquals("pb",expression.getValue());
+		assertCanCompile(expression);
+		assertEquals("pb",expression.getValue());
+
+		// variety chain
+		expression = parser.parseExpression("T(String).valueOf(T("+shclass+").fielda.methoda().propertya.fieldb)");
+		assertEquals("fb",expression.getValue());
+		assertCanCompile(expression);
+		assertEquals("fb",expression.getValue());
+
+		expression = parser.parseExpression("T(String).valueOf(fielda.fieldb)");
+		assertEquals("fb",expression.getValue(StaticsHelper.sh));
+		assertCanCompile(expression);
+		assertEquals("fb",expression.getValue(StaticsHelper.sh));
+		
+		expression = parser.parseExpression("T(String).valueOf(propertya.propertyb)");
+		assertEquals("pb",expression.getValue(StaticsHelper.sh));
+		assertCanCompile(expression);
+		assertEquals("pb",expression.getValue(StaticsHelper.sh));
+
+		expression = parser.parseExpression("T(String).valueOf(methoda().methodb())");
+		assertEquals("mb",expression.getValue(StaticsHelper.sh));
+		assertCanCompile(expression);
+		assertEquals("mb",expression.getValue(StaticsHelper.sh));
+		
+	}
+	
 	@Test
 	public void constructorReference_SPR12326() {
 		String type = this.getClass().getName();
@@ -4353,4 +4450,29 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 		}
 	}
 
+	public static class StaticsHelper {
+		static StaticsHelper sh = new StaticsHelper();
+		public static StaticsHelper methoda() {
+			return sh;
+		}
+		public static String methodb() {
+			return "mb";
+		}
+		
+		public static StaticsHelper getPropertya() {
+			return sh;
+		}
+
+		public static String getPropertyb() {
+			return "pb";
+		}
+		
+
+		public static StaticsHelper fielda = sh;
+		public static String fieldb = "fb";
+		
+		public String toString() {
+			return "sh";
+		}
+	}
 }