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

Author: aclement

spring-expression/src/main/java/org/springframework/expression/spel/ast/CompoundExpression.java

@@ -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

@@ -292,8 +292,16 @@ public void generateCode(MethodVisitor mv, CodeFlow cf) {
 		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)) {

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

@@ -685,6 +685,12 @@ public void generateCode(String propertyName, MethodVisitor mv, CodeFlow cf) {
 				if (descriptor == null || !memberDeclaringClassSlashedDescriptor.equals(descriptor.substring(1))) {
 					mv.visitTypeInsn(CHECKCAST, memberDeclaringClassSlashedDescriptor);
 				}
+			} 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 Field) {
 				mv.visitFieldInsn(isStatic ? GETSTATIC : GETFIELD, memberDeclaringClassSlashedDescriptor,

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

@@ -2958,6 +2958,103 @@ public void failsWhenSettingContextForExpression_SPR12326() {
 		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();
@@ -5341,4 +5438,29 @@ public static String format(String s, Object... args) {
 		}
 	}
 
+	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";
+		}
+	}
 }