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
(cherry picked from commit 9000acd)

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

@@ -28,15 +28,28 @@ import org.springframework.asm.Opcodes;
 import org.springframework.util.Assert;
 
 /**
- * Manages the class being generated by the compilation process. It records
- * intermediate compilation state as the bytecode is generated. It also includes
- * various bytecode generation helper functions.
+ * Manages the class being generated by the compilation process.
+ *
+ * <p>Records intermediate compilation state as the bytecode is generated.
+ * Also includes various bytecode generation helper functions.
  *
  * @author Andy Clement
+ * @author Juergen Hoeller
  * @since 4.1
  */
 public class CodeFlow implements Opcodes {
 
+	/**
+	 * Name of the class being generated. Typically used when generating code
+	 * that accesses freshly generated fields on the generated type.
+	 */
+	private final String className;
+
+	/**
+	 * The current class being generated.
+	 */
+	private final ClassWriter classWriter;
+
 	/**
 	 * Record the type of what is on top of the bytecode stack (i.e. the type of the
 	 * output from the previous expression component). New scopes are used to evaluate
@@ -45,17 +58,12 @@ public class CodeFlow implements Opcodes {
 	 */
 	private final Stack<ArrayList<String>> compilationScopes;
 
-	/**
-	 * The current class being generated
-	 */
-	private ClassWriter cw;
-
 	/**
 	 * As SpEL ast nodes are called to generate code for the main evaluation method
 	 * they can register to add a field to this class. Any registered FieldAdders
 	 * will be called after the main evaluation function has finished being generated.
 	 */
-	private List<FieldAdder> fieldAdders = null;
+	private List<FieldAdder> fieldAdders;
 
 	/**
 	 * As SpEL ast nodes are called to generate code for the main evaluation method
@@ -63,13 +71,7 @@ public class CodeFlow implements Opcodes {
 	 * registered ClinitAdders will be called after the main evaluation function
 	 * has finished being generated.
 	 */
-	private List<ClinitAdder> clinitAdders = null;
-
-	/**
-	 * Name of the class being generated. Typically used when generating code
-	 * that accesses freshly generated fields on the generated type.
-	 */
-	private String clazzName;
+	private List<ClinitAdder> clinitAdders;
 
 	/**
 	 * When code generation requires holding a value in a class level field, this
@@ -83,13 +85,20 @@ public class CodeFlow implements Opcodes {
 	 */
 	private int nextFreeVariableId = 1;
 
-	public CodeFlow(String clazzName, ClassWriter cw) {
+
+	/**
+	 * Construct a new {@code CodeFlow} for the given class.
+	 * @param className the name of the class
+	 * @param classWriter the corresponding ASM {@code ClassWriter}
+	 */
+	public CodeFlow(String className, ClassWriter classWriter) {
+		this.className = className;
+		this.classWriter = classWriter;
 		this.compilationScopes = new Stack<ArrayList<String>>();
 		this.compilationScopes.add(new ArrayList<String>());
-		this.cw = cw;
-		this.clazzName = clazzName;
 	}
 
+
 	/**
 	 * Push the byte code to load the target (i.e. what was passed as the first argument
 	 * to CompiledExpression.getValue(target, context))
@@ -99,6 +108,16 @@ 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
+	 * @since 4.3.4
+	 */
+	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
@@ -155,18 +174,18 @@ public class CodeFlow implements Opcodes {
 	public void finish() {
 		if (this.fieldAdders != null) {
 			for (FieldAdder fieldAdder : this.fieldAdders) {
-				fieldAdder.generateField(cw,this);
+				fieldAdder.generateField(this.classWriter, this);
 			}
 		}
 		if (this.clinitAdders != null) {
-			MethodVisitor mv = cw.visitMethod(ACC_PUBLIC | ACC_STATIC, "<clinit>", "()V", null, null);
+			MethodVisitor mv = this.classWriter.visitMethod(ACC_PUBLIC | ACC_STATIC, "<clinit>", "()V", null, null);
 			mv.visitCode();
-			this.nextFreeVariableId = 0; // To 0 because there is no 'this' in a clinit
+			this.nextFreeVariableId = 0;  // to 0 because there is no 'this' in a clinit
 			for (ClinitAdder clinitAdder : this.clinitAdders) {
 				clinitAdder.generateCode(mv, this);
 			}
 			mv.visitInsn(RETURN);
-			mv.visitMaxs(0,0); // not supplied due to COMPUTE_MAXS
+			mv.visitMaxs(0,0);  // not supplied due to COMPUTE_MAXS
 			mv.visitEnd();
 		}
 	}
@@ -204,18 +223,18 @@ public class CodeFlow implements Opcodes {
 	}
 
 	public String getClassName() {
-		return this.clazzName;
+		return this.className;
 	}
 
 	@Deprecated
 	public String getClassname() {
-		return this.clazzName;
+		return this.className;
 	}
 
 
 	/**
 	 * Insert any necessary cast and value call to convert from a boxed type to a
-	 * primitive value
+	 * primitive value.
 	 * @param mv the method visitor into which instructions should be inserted
 	 * @param ch the primitive type desired as output
 	 * @param stackDescriptor the descriptor of the type on top of the stack

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;
@@ -43,105 +43,52 @@ public class OpEQ extends Operator {
 		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(
-				leftDesc, rightDesc, this.leftActualDescriptor, this.rightActualDescriptor);
+		DescriptorComparison dc = DescriptorComparison.checkNumericCompatibility(leftDesc,
+				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(
-				leftDesc, rightDesc, this.leftActualDescriptor, this.rightActualDescriptor);
-		
-		if (dc.areNumbers && dc.areCompatible) {
-			char targetType = dc.compatibleType;
-			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);
-			}
+		cf.enterCompilationScope();
+		getLeftOperand().generateCode(mv, cf);
+		cf.exitCompilationScope();
+		if (leftPrim) {
+			CodeFlow.insertBoxIfNecessary(mv, leftDesc.charAt(0));
 		}
-		else {
-			getLeftOperand().generateCode(mv, cf);
-			if (leftPrim) {
-				CodeFlow.insertBoxIfNecessary(mv, leftDesc.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;
+		cf.enterCompilationScope();
+		getRightOperand().generateCode(mv, cf);
+		cf.exitCompilationScope();
+		if (rightPrim) {
+			CodeFlow.insertBoxIfNecessary(mv, rightDesc.charAt(0));
 		}
-		mv.visitInsn(ICONST_1);
-		mv.visitJumpInsn(GOTO, endOfIf);
-		mv.visitLabel(elseTarget);
-		mv.visitInsn(ICONST_0);
-		mv.visitLabel(endOfIf);
+
+		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);
 		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;
@@ -43,7 +44,8 @@ public class OpNE extends Operator {
 		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,54 @@ 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(
-				leftDesc, rightDesc, this.leftActualDescriptor, this.rightActualDescriptor);
+		DescriptorComparison dc = DescriptorComparison.checkNumericCompatibility(leftDesc,
+				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(
-				leftDesc, rightDesc, this.leftActualDescriptor, this.rightActualDescriptor);
-		
-		if (dc.areNumbers && dc.areCompatible) {
-			char targetType = dc.compatibleType;
-			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);
-			}
+		cf.enterCompilationScope();
+		getLeftOperand().generateCode(mv, cf);
+		cf.exitCompilationScope();
+		if (leftPrim) {
+			CodeFlow.insertBoxIfNecessary(mv, leftDesc.charAt(0));
 		}
-		else {
-			getLeftOperand().generateCode(mv, cf);
-			getRightOperand().generateCode(mv, cf);
-			mv.visitJumpInsn(IF_ACMPEQ, elseTarget);
+		cf.enterCompilationScope();
+		getRightOperand().generateCode(mv, cf);
+		cf.exitCompilationScope();
+		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.visitLabel(elseTarget);
+		mv.visitJumpInsn(GOTO, end);
+		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,17 @@ public abstract class Operator extends SpelNodeImpl {
 		cf.pushDescriptor("Z");
 	}
 
-	protected boolean equalityCheck(ExpressionState state, Object left, Object right) {
+
+	/**
+	 * Perform an equality check for the given operand values.
+	 * <p>This method is not just used for reflective comparisons in subclasses
+	 * but also from compiled expression code, which is why it needs to be
+	 * declared as {@code public static} here.
+	 * @param context the current evaluation context
+	 * @param left the left-hand operand value
+	 * @param right the right-hand operand value
+	 */
+	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 +219,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);
 			}
 		}