Rework compilation of OpNE/OpEQ SpEL operators

For SPR-14863 we need to adjust the code generation for OpNE
to use !x.equals(y) rather than x!=y. There are also further
cases in the equalityCheck() code in Operator that were not
being handled in the compilation case (when comparators are
used for example). This latter issue also affects OpEQ.

Rather than add yet more bytecode generation, both OpNE and
OpEQ generateCode() methods have been simplified. The
generated code now delegates to equalityCheck() in Operator
which is exactly what the interpreted case does.

This ensures that the compiled code continues to behave just
like the interpreted case. It ensures changes to the interpreted
case are automatically picked up for the compiled case. It
makes the bytecode generation simpler.

The benefit of compilation of SpEL expressions is to avoid
slow reflective calls - that doesn't apply for a basic
(in)equality test so there is no need to go crazy in bytecode
gen.

Issue: SPR-14863

spring-expression/src/main/java/org/springframework/expression/spel/CodeFlow.java

@@ -99,6 +99,15 @@ public class CodeFlow implements Opcodes {
 		mv.visitVarInsn(ALOAD, 1);
 	}
 
+	/**
+	 * Push the bytecode to load the EvaluationContext (the second parameter passed to
+	 * the compiled expression method).
+	 * @param mv the visitor into which the load instruction should be inserted
+	 */
+	public void loadEvaluationContext(MethodVisitor mv) {
+		mv.visitVarInsn(ALOAD, 2);
+	}
+
 	/**
 	 * Record the descriptor for the most recently evaluated expression element.
 	 * @param descriptor type descriptor for most recently evaluated element

spring-expression/src/main/java/org/springframework/expression/spel/ast/OpEQ.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.
@@ -16,8 +16,8 @@
 
 package org.springframework.expression.spel.ast;
 
-import org.springframework.asm.Label;
 import org.springframework.asm.MethodVisitor;
+import org.springframework.expression.EvaluationContext;
 import org.springframework.expression.EvaluationException;
 import org.springframework.expression.spel.CodeFlow;
 import org.springframework.expression.spel.ExpressionState;
@@ -36,112 +36,61 @@ public class OpEQ extends Operator {
 		this.exitTypeDescriptor = "Z";
 	}
 
-
 	@Override
-	public BooleanTypedValue getValueInternal(ExpressionState state) throws EvaluationException {
+	public BooleanTypedValue getValueInternal(ExpressionState state)
+			throws EvaluationException {
 		Object left = getLeftOperand().getValueInternal(state).getValue();
 		Object right = getRightOperand().getValueInternal(state).getValue();
 		this.leftActualDescriptor = CodeFlow.toDescriptorFromObject(left);
 		this.rightActualDescriptor = CodeFlow.toDescriptorFromObject(right);
-		return BooleanTypedValue.forValue(equalityCheck(state, left, right));
+		return BooleanTypedValue.forValue(
+				equalityCheck(state.getEvaluationContext(), left, right));
 	}
-	
+
 	// This check is different to the one in the other numeric operators (OpLt/etc)
 	// because it allows for simple object comparison
 	@Override
 	public boolean isCompilable() {
 		SpelNodeImpl left = getLeftOperand();
-		SpelNodeImpl right= getRightOperand();
+		SpelNodeImpl right = getRightOperand();
 		if (!left.isCompilable() || !right.isCompilable()) {
 			return false;
 		}
 
 		String leftDesc = left.exitTypeDescriptor;
 		String rightDesc = right.exitTypeDescriptor;
-		DescriptorComparison dc = DescriptorComparison.checkNumericCompatibility(
+		DescriptorComparison dc = DescriptorComparison.checkNumericCompatibility(leftDesc,
-				leftDesc, rightDesc, this.leftActualDescriptor, this.rightActualDescriptor);
+				rightDesc, this.leftActualDescriptor, this.rightActualDescriptor);
 		return (!dc.areNumbers || dc.areCompatible);
 	}
-	
+
-	
 	@Override
 	public void generateCode(MethodVisitor mv, CodeFlow cf) {
+		cf.loadEvaluationContext(mv);
 		String leftDesc = getLeftOperand().exitTypeDescriptor;
 		String rightDesc = getRightOperand().exitTypeDescriptor;
-		Label elseTarget = new Label();
-		Label endOfIf = new Label();
 		boolean leftPrim = CodeFlow.isPrimitive(leftDesc);
 		boolean rightPrim = CodeFlow.isPrimitive(rightDesc);
 
-		DescriptorComparison dc = DescriptorComparison.checkNumericCompatibility(
+		cf.enterCompilationScope();
-				leftDesc, rightDesc, this.leftActualDescriptor, this.rightActualDescriptor);
+		getLeftOperand().generateCode(mv, cf);
-		
+		cf.exitCompilationScope();
-		if (dc.areNumbers && dc.areCompatible) {
+		if (leftPrim) {
-			char targetType = dc.compatibleType;
+			CodeFlow.insertBoxIfNecessary(mv, leftDesc.charAt(0));
-			getLeftOperand().generateCode(mv, cf);
-			if (!leftPrim) {
-				CodeFlow.insertUnboxInsns(mv, targetType, leftDesc);
-			}
-			cf.enterCompilationScope();
-			getRightOperand().generateCode(mv, cf);
-			cf.exitCompilationScope();
-			if (!rightPrim) {
-				CodeFlow.insertUnboxInsns(mv, targetType, rightDesc);
-			}
-			// assert: SpelCompiler.boxingCompatible(leftDesc, rightDesc)
-			if (targetType == 'D') {
-				mv.visitInsn(DCMPL);
-				mv.visitJumpInsn(IFNE, elseTarget);
-			}
-			else if (targetType == 'F') {
-				mv.visitInsn(FCMPL);		
-				mv.visitJumpInsn(IFNE, elseTarget);
-			}
-			else if (targetType == 'J') {
-				mv.visitInsn(LCMP);		
-				mv.visitJumpInsn(IFNE, elseTarget);
-			}
-			else if (targetType == 'I' || targetType == 'Z') {
-				mv.visitJumpInsn(IF_ICMPNE, elseTarget);		
-			}
-			else {
-				throw new IllegalStateException("Unexpected descriptor " + leftDesc);
-			}
 		}
-		else {
+		cf.enterCompilationScope();
-			getLeftOperand().generateCode(mv, cf);
+		getRightOperand().generateCode(mv, cf);
-			if (leftPrim) {
+		cf.exitCompilationScope();
-				CodeFlow.insertBoxIfNecessary(mv, leftDesc.charAt(0));
+		if (rightPrim) {
-			}
+			CodeFlow.insertBoxIfNecessary(mv, rightDesc.charAt(0));
-			getRightOperand().generateCode(mv, cf);
-			if (rightPrim) {
-				CodeFlow.insertBoxIfNecessary(mv, rightDesc.charAt(0));
-			}
-			Label leftNotNull = new Label();
-			mv.visitInsn(DUP_X1);  // dup right on the top of the stack
-			mv.visitJumpInsn(IFNONNULL, leftNotNull);
-			// Right is null!
-			mv.visitInsn(SWAP);
-			mv.visitInsn(POP);  // remove it
-			Label rightNotNull = new Label();
-			mv.visitJumpInsn(IFNONNULL, rightNotNull);
-			// Left is null too
-			mv.visitInsn(ICONST_1);
-			mv.visitJumpInsn(GOTO, endOfIf);
-			mv.visitLabel(rightNotNull);
-			mv.visitInsn(ICONST_0);
-			mv.visitJumpInsn(GOTO, endOfIf);
-			mv.visitLabel(leftNotNull);
-			mv.visitMethodInsn(INVOKEVIRTUAL, "java/lang/Object", "equals", "(Ljava/lang/Object;)Z", false);
-			mv.visitLabel(endOfIf);
-			cf.pushDescriptor("Z");
-			return;
 		}
-		mv.visitInsn(ICONST_1);
+
-		mv.visitJumpInsn(GOTO, endOfIf);
+		String operatorClassName = Operator.class.getName().replace('.', '/');
-		mv.visitLabel(elseTarget);
+		String evaluationContextClassName = EvaluationContext.class.getName().replace('.',
-		mv.visitInsn(ICONST_0);
+				'/');
-		mv.visitLabel(endOfIf);
+		mv.visitMethodInsn(INVOKESTATIC, operatorClassName, "equalityCheck", "(L"
+				+ evaluationContextClassName + ";Ljava/lang/Object;Ljava/lang/Object;)Z",
+				false);
 		cf.pushDescriptor("Z");
 	}
 

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

@@ -18,6 +18,7 @@ package org.springframework.expression.spel.ast;
 
 import org.springframework.asm.Label;
 import org.springframework.asm.MethodVisitor;
+import org.springframework.expression.EvaluationContext;
 import org.springframework.expression.EvaluationException;
 import org.springframework.expression.spel.CodeFlow;
 import org.springframework.expression.spel.ExpressionState;
@@ -36,14 +37,15 @@ public class OpNE extends Operator {
 		this.exitTypeDescriptor = "Z";
 	}
 
-
 	@Override
-	public BooleanTypedValue getValueInternal(ExpressionState state) throws EvaluationException {
+	public BooleanTypedValue getValueInternal(ExpressionState state)
+			throws EvaluationException {
 		Object left = getLeftOperand().getValueInternal(state).getValue();
 		Object right = getRightOperand().getValueInternal(state).getValue();
 		this.leftActualDescriptor = CodeFlow.toDescriptorFromObject(left);
 		this.rightActualDescriptor = CodeFlow.toDescriptorFromObject(right);
-		return BooleanTypedValue.forValue(!equalityCheck(state, left, right));
+		return BooleanTypedValue.forValue(
+				!equalityCheck(state.getEvaluationContext(), left, right));
 	}
 
 	// This check is different to the one in the other numeric operators (OpLt/etc)
@@ -51,72 +53,56 @@ public class OpNE extends Operator {
 	@Override
 	public boolean isCompilable() {
 		SpelNodeImpl left = getLeftOperand();
-		SpelNodeImpl right= getRightOperand();
+		SpelNodeImpl right = getRightOperand();
 		if (!left.isCompilable() || !right.isCompilable()) {
 			return false;
 		}
 
 		String leftDesc = left.exitTypeDescriptor;
 		String rightDesc = right.exitTypeDescriptor;
-		DescriptorComparison dc =  DescriptorComparison.checkNumericCompatibility(
+		DescriptorComparison dc = DescriptorComparison.checkNumericCompatibility(leftDesc,
-				leftDesc, rightDesc, this.leftActualDescriptor, this.rightActualDescriptor);
+				rightDesc, this.leftActualDescriptor, this.rightActualDescriptor);
 		return (!dc.areNumbers || dc.areCompatible);
 	}
-	
+
 	@Override
 	public void generateCode(MethodVisitor mv, CodeFlow cf) {
+		cf.loadEvaluationContext(mv);
 		String leftDesc = getLeftOperand().exitTypeDescriptor;
 		String rightDesc = getRightOperand().exitTypeDescriptor;
-		Label elseTarget = new Label();
-		Label endOfIf = new Label();
 		boolean leftPrim = CodeFlow.isPrimitive(leftDesc);
 		boolean rightPrim = CodeFlow.isPrimitive(rightDesc);
 
-		DescriptorComparison dc = DescriptorComparison.checkNumericCompatibility(
+		cf.enterCompilationScope();
-				leftDesc, rightDesc, this.leftActualDescriptor, this.rightActualDescriptor);
+		getLeftOperand().generateCode(mv, cf);
-		
+		cf.exitCompilationScope();
-		if (dc.areNumbers && dc.areCompatible) {
+		if (leftPrim) {
-			char targetType = dc.compatibleType;
+			CodeFlow.insertBoxIfNecessary(mv, leftDesc.charAt(0));
-			getLeftOperand().generateCode(mv, cf);
-			if (!leftPrim) {
-				CodeFlow.insertUnboxInsns(mv, targetType, leftDesc);
-			}
-			cf.enterCompilationScope();
-			getRightOperand().generateCode(mv, cf);
-			cf.exitCompilationScope();
-			if (!rightPrim) {
-				CodeFlow.insertUnboxInsns(mv, targetType, rightDesc);
-			}
-			// assert: SpelCompiler.boxingCompatible(leftDesc, rightDesc)
-			if (targetType == 'D') {
-				mv.visitInsn(DCMPL);
-				mv.visitJumpInsn(IFEQ, elseTarget);
-			}
-			else if (targetType == 'F') {
-				mv.visitInsn(FCMPL);		
-				mv.visitJumpInsn(IFEQ, elseTarget);
-			}
-			else if (targetType == 'J') {
-				mv.visitInsn(LCMP);		
-				mv.visitJumpInsn(IFEQ, elseTarget);
-			}
-			else if (targetType == 'I' || targetType == 'Z') {
-				mv.visitJumpInsn(IF_ICMPEQ, elseTarget);		
-			}
-			else {
-				throw new IllegalStateException("Unexpected descriptor " + leftDesc);
-			}
 		}
-		else {
+		cf.enterCompilationScope();
-			getLeftOperand().generateCode(mv, cf);
+		getRightOperand().generateCode(mv, cf);
-			getRightOperand().generateCode(mv, cf);
+		cf.exitCompilationScope();
-			mv.visitJumpInsn(IF_ACMPEQ, elseTarget);
+		if (rightPrim) {
+			CodeFlow.insertBoxIfNecessary(mv, rightDesc.charAt(0));
 		}
+
+		String operatorClassName = Operator.class.getName().replace('.', '/');
+		String evaluationContextClassName = EvaluationContext.class.getName().replace('.',
+				'/');
+		mv.visitMethodInsn(INVOKESTATIC, operatorClassName, "equalityCheck", "(L"
+				+ evaluationContextClassName + ";Ljava/lang/Object;Ljava/lang/Object;)Z",
+				false);
+
+		// Invert the boolean
+		Label notZero = new Label();
+		Label end = new Label();
+		mv.visitJumpInsn(IFNE, notZero);
 		mv.visitInsn(ICONST_1);
-		mv.visitJumpInsn(GOTO,endOfIf);
+		mv.visitJumpInsn(GOTO, end);
-		mv.visitLabel(elseTarget);
+		mv.visitLabel(notZero);
 		mv.visitInsn(ICONST_0);
-		mv.visitLabel(endOfIf);
+		mv.visitLabel(end);
+
 		cf.pushDescriptor("Z");
 	}
 

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

@@ -21,8 +21,8 @@ import java.math.BigInteger;
 
 import org.springframework.asm.Label;
 import org.springframework.asm.MethodVisitor;
+import org.springframework.expression.EvaluationContext;
 import org.springframework.expression.spel.CodeFlow;
-import org.springframework.expression.spel.ExpressionState;
 import org.springframework.util.ClassUtils;
 import org.springframework.util.NumberUtils;
 import org.springframework.util.ObjectUtils;
@@ -114,7 +114,9 @@ public abstract class Operator extends SpelNodeImpl {
 				leftDesc, rightDesc, this.leftActualDescriptor, this.rightActualDescriptor);
 		char targetType = dc.compatibleType;  // CodeFlow.toPrimitiveTargetDesc(leftDesc);
 		
+		cf.enterCompilationScope();
 		getLeftOperand().generateCode(mv, cf);
+		cf.exitCompilationScope();
 		if (unboxLeft) {
 			CodeFlow.insertUnboxInsns(mv, targetType, leftDesc);
 		}
@@ -157,7 +159,7 @@ public abstract class Operator extends SpelNodeImpl {
 		cf.pushDescriptor("Z");
 	}
 
-	protected boolean equalityCheck(ExpressionState state, Object left, Object right) {
+	public static boolean equalityCheck(EvaluationContext context, Object left, Object right) {
 		if (left instanceof Number && right instanceof Number) {
 			Number leftNumber = (Number) left;
 			Number rightNumber = (Number) right;
@@ -207,7 +209,7 @@ public abstract class Operator extends SpelNodeImpl {
 		if (left instanceof Comparable && right instanceof Comparable) {
 			Class<?> ancestor = ClassUtils.determineCommonAncestor(left.getClass(), right.getClass());
 			if (ancestor != null && Comparable.class.isAssignableFrom(ancestor)) {
-				return (state.getTypeComparator().compare(left, right) == 0);
+				return (context.getTypeComparator().compare(left, right) == 0);
 			}
 		}
 

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

@@ -1335,8 +1335,6 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 	public void opEq() throws Exception {
 		String tvar = "35";
 		expression = parse("#root == 35");
-		Boolean bb = (Boolean)expression.getValue(tvar);
-		System.out.println(bb);
 		assertFalse((Boolean)expression.getValue(tvar));
 		assertCanCompile(expression);
 		assertFalse((Boolean)expression.getValue(tvar));
@@ -1623,6 +1621,122 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 		assertTrue((Boolean)expression.getValue());
 	}
 
+	@Test
+	public void opNe_SPR14863() throws Exception {
+		// First part is from the test case specified in the bug report
+		Map<String, String> data = new HashMap<>();
+        data.put("my-key", new String("my-value"));
+        StandardEvaluationContext context = new StandardEvaluationContext(new MyContext(data));
+        SpelParserConfiguration configuration = new SpelParserConfiguration(SpelCompilerMode.MIXED,
+                                                                            ClassLoader.getSystemClassLoader());
+        SpelExpressionParser parser = new SpelExpressionParser(configuration);
+        Expression expression = parser.parseExpression("data['my-key'] != 'my-value'");
+        assertFalse(expression.getValue(context, Boolean.class));
+        assertCanCompile(expression);
+        ((SpelExpression) expression).compileExpression();
+        assertFalse(expression.getValue(context, Boolean.class));
+
+        List<String> ls = new ArrayList<String>();
+        ls.add(new String("foo"));
+        context = new StandardEvaluationContext(ls);
+        expression = parse("get(0) != 'foo'");
+        assertFalse(expression.getValue(context, Boolean.class));
+        assertCanCompile(expression);
+        assertFalse(expression.getValue(context, Boolean.class));
+        
+        ls.remove(0);
+        ls.add("goo");
+        assertTrue(expression.getValue(context, Boolean.class));
+	}
+	
+    @Test
+    public void opEq_SPR14863() throws Exception {
+        // Exercise the comparator invocation code that runs in
+        // equalityCheck() (called from interpreted and compiled code)
+        expression = parser.parseExpression("#aa==#bb");
+        StandardEvaluationContext sec = new StandardEvaluationContext();
+        Apple aa = new Apple(1);
+        Apple bb = new Apple(2);
+        sec.setVariable("aa",aa);
+        sec.setVariable("bb",bb);
+        boolean b = expression.getValue(sec,Boolean.class);
+        // Verify what the expression caused aa to be compared to
+        assertEquals(bb,aa.gotComparedTo);
+        assertFalse(b);
+        bb.setValue(1);
+        b = expression.getValue(sec,Boolean.class);
+        assertEquals(bb,aa.gotComparedTo);
+        assertTrue(b);
+        
+        assertCanCompile(expression);
+        
+        // Similar test with compiled expression
+        aa = new Apple(99);
+        bb = new Apple(100);
+        sec.setVariable("aa",aa);
+        sec.setVariable("bb",bb);
+        b = expression.getValue(sec,Boolean.class);
+        assertFalse(b);
+        assertEquals(bb,aa.gotComparedTo);
+        bb.setValue(99);
+        b = expression.getValue(sec,Boolean.class);
+        assertTrue(b);
+        assertEquals(bb,aa.gotComparedTo);
+        
+
+        List<String> ls = new ArrayList<String>();
+        ls.add(new String("foo"));
+        StandardEvaluationContext context = new StandardEvaluationContext(ls);
+        expression = parse("get(0) == 'foo'");
+        assertTrue(expression.getValue(context, Boolean.class));
+        assertCanCompile(expression);
+        assertTrue(expression.getValue(context, Boolean.class));
+        
+        ls.remove(0);
+        ls.add("goo");
+        assertFalse(expression.getValue(context, Boolean.class));
+    }
+	
+	public static class Apple implements Comparable<Apple> {
+		public Object gotComparedTo = null;
+		public int i;
+
+		public Apple(int i) {
+			this.i = i;
+		}
+		
+		public void setValue(int i) {
+			this.i = i;
+		}
+
+		@Override
+		public int compareTo(Apple that) {
+			this.gotComparedTo = that;
+			if (this.i<that.i) {
+				return -1;
+			} else if (this.i>that.i) {
+				return +1;
+			} else {
+				return 0;
+			}
+		}
+		
+	}
+	
+	// For opNe_SPR14863
+	public static class MyContext {
+
+        private final Map<String, String> data;
+
+        public MyContext(Map<String, String> data) {
+            this.data = data;
+        }
+
+        public Map<String, String> getData() {
+            return data;
+        }
+	}
+
 	@Test
 	public void opPlus() throws Exception {
 		expression = parse("2+2");
@@ -3901,7 +4015,7 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 		m.put("andy","778");
 
 		expression = parse("['andy']==null?1:2");
-		System.out.println(expression.getValue(m));
+		assertEquals(2,expression.getValue(m));
 		assertCanCompile(expression);
 		assertEquals(2,expression.getValue(m));
 		m.remove("andy");