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)

Author: aclement

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 @@ protected ValueRef getValueRef(ExpressionState state) throws EvaluationException
 			}
 			try {
 				state.pushActiveContextObject(result);
-				nextNode = this.children[cc-1];
+				nextNode = this.children[cc - 1];
 				return nextNode.getValueRef(state);
 			}
 			finally {
@@ -124,14 +124,8 @@ public boolean isCompilable() {
 
 	@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 void generateCode(MethodVisitor mv, CodeFlow cf) {
 			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) {
-			cf.loadTarget(mv);
+		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,16 +58,17 @@
  */
 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);
 		booleanTypes.add(Boolean.TYPE);
 		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);
 
@@ -120,9 +121,9 @@ public boolean canRead(EvaluationContext context, Object target, String name) th
 		}
 		return false;
 	}
-	
+
 	public Member getLastReadInvokerPair() {
-		return lastReadInvokerPair.member;
+		return this.lastReadInvokerPair.member;
 	}
 
 	@Override
@@ -165,7 +166,7 @@ public TypedValue read(EvaluationContext context, Object target, String name) th
 					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 TypedValue read(EvaluationContext context, Object target, String name) th
 					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 void write(EvaluationContext context, Object target, String name, Object
 						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 void write(EvaluationContext context, Object target, String name, Object
 					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 void write(EvaluationContext context, Object target, String name, Object
 					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 PropertyAccessor createOptimalAccessor(EvaluationContext evalContext, Obj
 		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 PropertyAccessor createOptimalAccessor(EvaluationContext evalContext, Obj
 				return new OptimalPropertyAccessor(invocationTarget);
 			}
 		}
+
 		return this;
 	}
 
@@ -577,16 +579,8 @@ public static class OptimalPropertyAccessor implements CompilablePropertyAccesso
 		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 boolean canRead(EvaluationContext context, Object target, String name) th
 			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 boolean canRead(EvaluationContext context, Object target, String name) th
 		@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
@@ -656,7 +653,7 @@ public boolean canWrite(EvaluationContext context, Object target, String name) {
 		public void write(EvaluationContext context, Object target, String name, Object newValue) {
 			throw new UnsupportedOperationException("Should not be called on an OptimalPropertyAccessor");
 		}
-		
+
 		@Override
 		public boolean isCompilable() {
 			return (Modifier.isPublic(this.member.getModifiers()) &&
@@ -665,34 +662,43 @@ public boolean isCompilable() {
 
 		@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();
 			}
 		}
 
 		@Override
 		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()));
 			}
 		}
 	}