Fix compilation of expressions using instanceof and primitives

Prior to this commit the SpEL compiler would generate bad bytecode
if the left hand operand of an instanceof was a primitive or
if the right hand operand was a primitive type reference.
With the fixes primitives on the left hand side are now
correctly boxed and special handling is in place for when the
right hand side is a primitive type reference. Using a primitive
type reference on the right always causes the instanceof
check to return false.

Additionally a guard has been added such that compilation is
not allowed when the right hand side of an expression
is not a type reference. If it is, for example, a variable
reference that evaluates to a type reference then that
cannot be expressed in bytecode so compilation is not performed.

Issue: SPR-14250

Author: aclement

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2014 the original author or authors.
+ * Copyright 2002-2016 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.
@@ -53,8 +53,9 @@ public OperatorInstanceof(int pos, SpelNodeImpl... operands) {
 	 */
 	@Override
 	public BooleanTypedValue getValueInternal(ExpressionState state) throws EvaluationException {
+		SpelNodeImpl rightOperand = getRightOperand();
 		TypedValue left = getLeftOperand().getValueInternal(state);
-		TypedValue right = getRightOperand().getValueInternal(state);
+		TypedValue right = rightOperand.getValueInternal(state);
 		Object leftValue = left.getValue();
 		Object rightValue = right.getValue();
 		BooleanTypedValue result = null;
@@ -71,7 +72,11 @@ public BooleanTypedValue getValueInternal(ExpressionState state) throws Evaluati
 			result = BooleanTypedValue.forValue(rightClass.isAssignableFrom(leftValue.getClass()));
 		}
 		this.type = rightClass;
-		this.exitTypeDescriptor = "Z";
+		if (rightOperand instanceof TypeReference) {
+			// Can only generate bytecode where the right operand is a direct type reference, 
+			// not if it is indirect (for example when right operand is a variable reference)
+			this.exitTypeDescriptor = "Z";
+		}
 		return result;
 	}
 
@@ -83,7 +88,16 @@ public boolean isCompilable() {
 	@Override
 	public void generateCode(MethodVisitor mv, CodeFlow cf) {
 		getLeftOperand().generateCode(mv, cf);
-		mv.visitTypeInsn(INSTANCEOF,Type.getInternalName(this.type));
+		CodeFlow.insertBoxIfNecessary(mv, cf.lastDescriptor());
+		if (this.type.isPrimitive()) {
+			// always false - but left operand code always driven
+			// in case it had side effects
+			mv.visitInsn(POP);
+			mv.visitInsn(ICONST_0); // value of false
+		} 
+		else {
+			mv.visitTypeInsn(INSTANCEOF,Type.getInternalName(this.type));
+		}
 		cf.pushDescriptor(this.exitTypeDescriptor);
 	}
 

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2015 the original author or authors.
+ * Copyright 2002-2016 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.
@@ -235,6 +235,53 @@ public void operatorInstanceOf() throws Exception {
 		assertEquals(true,expression.getValue(root));
 	}
 
+	@Test
+	public void operatorInstanceOf_SPR14250() throws Exception {
+		// primitive left operand - should get boxed, return true
+		expression = parse("3 instanceof T(Integer)");
+		assertEquals(true,expression.getValue());
+		assertCanCompile(expression);
+		assertEquals(true,expression.getValue());
+
+		// primitive left operand - should get boxed, return false
+		expression = parse("3 instanceof T(String)");
+		assertEquals(false,expression.getValue());
+		assertCanCompile(expression);
+		assertEquals(false,expression.getValue());
+		
+		// double slot left operand - should get boxed, return false
+		expression = parse("3.0d instanceof T(Integer)");
+		assertEquals(false,expression.getValue());
+		assertCanCompile(expression);
+		assertEquals(false,expression.getValue());
+
+		// double slot left operand - should get boxed, return true
+		expression = parse("3.0d instanceof T(Double)");
+		assertEquals(true,expression.getValue());
+		assertCanCompile(expression);
+		assertEquals(true,expression.getValue());
+		
+		// Only when the right hand operand is a direct type reference
+		// will it be compilable.
+		StandardEvaluationContext ctx = new StandardEvaluationContext();
+		ctx.setVariable("foo", String.class);
+		expression = parse("3 instanceof #foo");
+		assertEquals(false,expression.getValue(ctx));
+		assertCantCompile(expression);
+
+		// use of primitive as type for instanceof check - compilable
+		// but always false
+		expression = parse("3 instanceof T(int)");
+		assertEquals(false,expression.getValue());
+		assertCanCompile(expression);
+		assertEquals(false,expression.getValue());
+		
+		expression = parse("3 instanceof T(long)");
+		assertEquals(false,expression.getValue());
+		assertCanCompile(expression);
+		assertEquals(false,expression.getValue());
+	}
+	
 	@Test
 	public void stringLiteral() throws Exception {
 		expression = parser.parseExpression("'abcde'");