Modify SpEL code gen to take account of null safe refs

With this change the code generation for method and property
references is modified to include branching logic in the
case of null safe dereferencing (?.). This is complicated
by the possible usage of primitives on the left hand side
of the dereference. To cope with this case primitives are
promoted to boxed types when this situation occurs enabling
null to be passed as a possible result.

Issue: SPR-16489

Author: aclement

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

@@ -1003,4 +1003,19 @@ public interface ClinitAdder {
 		void generateCode(MethodVisitor mv, CodeFlow codeflow);
 	}
 
+	public static String toBoxedDescriptor(String primitiveDescriptor) {
+		switch (primitiveDescriptor.charAt(0)) {
+			case 'I': return "Ljava/lang/Integer";
+			case 'J': return "Ljava/lang/Long";
+			case 'F': return "Ljava/lang/Float";
+			case 'D': return "Ljava/lang/Double";
+			case 'B': return "Ljava/lang/Byte";
+			case 'C': return "Ljava/lang/Character";
+			case 'S': return "Ljava/lang/Short";
+			case 'Z': return "Ljava/lang/Boolean";
+			default:
+				throw new IllegalArgumentException("Unexpected non primitive descriptor "+primitiveDescriptor);
+		}	
+	}
+
 }

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

@@ -24,6 +24,7 @@
 import java.util.Collections;
 import java.util.List;
 
+import org.springframework.asm.Label;
 import org.springframework.asm.MethodVisitor;
 import org.springframework.core.convert.TypeDescriptor;
 import org.springframework.expression.AccessException;
@@ -53,6 +54,8 @@ public class MethodReference extends SpelNodeImpl {
 
 	private final boolean nullSafe;
 
+	private String originalPrimitiveExitTypeDescriptor = null;
+
 	private volatile CachedMethodExecutor cachedExecutor;
 
 
@@ -233,7 +236,14 @@ private void updateExitTypeDescriptor() {
 		CachedMethodExecutor executorToCheck = this.cachedExecutor;
 		if (executorToCheck != null && executorToCheck.get() instanceof ReflectiveMethodExecutor) {
 			Method method = ((ReflectiveMethodExecutor) executorToCheck.get()).getMethod();
-			this.exitTypeDescriptor = CodeFlow.toDescriptor(method.getReturnType());
+			String descriptor = CodeFlow.toDescriptor(method.getReturnType());
+			if (this.nullSafe && CodeFlow.isPrimitive(descriptor)) {
+				originalPrimitiveExitTypeDescriptor = descriptor;
+				this.exitTypeDescriptor = CodeFlow.toBoxedDescriptor(descriptor);
+			}
+			else {
+				this.exitTypeDescriptor = descriptor;
+			}
 		}
 	}
 
@@ -293,17 +303,23 @@ public void generateCode(MethodVisitor mv, CodeFlow cf) {
 		boolean isStaticMethod = Modifier.isStatic(method.getModifiers());
 		String descriptor = cf.lastDescriptor();
 
-		if (descriptor == null) {
-			if (!isStaticMethod) {
-				// Nothing on the stack but something is needed
-				cf.loadTarget(mv);
-			}
+		Label skipIfNull = null;
+		if (descriptor == null && !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 ((descriptor != null || !isStaticMethod) && nullSafe) {
+			mv.visitInsn(DUP);
+			skipIfNull = new Label();
+			Label continueLabel = new Label();
+			mv.visitJumpInsn(IFNONNULL,continueLabel);
+			CodeFlow.insertCheckCast(mv, this.exitTypeDescriptor);
+			mv.visitJumpInsn(GOTO, skipIfNull);
+			mv.visitLabel(continueLabel);
+		}
+		if (descriptor != null && isStaticMethod) {
+			// Something on the stack when nothing is needed
+			mv.visitInsn(POP);
 		}
 
 		if (CodeFlow.isPrimitive(descriptor)) {
@@ -323,6 +339,14 @@ public void generateCode(MethodVisitor mv, CodeFlow cf) {
 		mv.visitMethodInsn((isStaticMethod ? INVOKESTATIC : INVOKEVIRTUAL), classDesc, method.getName(),
 				CodeFlow.createSignatureDescriptor(method), method.getDeclaringClass().isInterface());
 		cf.pushDescriptor(this.exitTypeDescriptor);
+		if (originalPrimitiveExitTypeDescriptor != null) {
+			// The output of the accessor will be a primitive but from the block above it might be null,
+			// so to have a 'common stack' element at skipIfNull target we need to box the primitive
+			CodeFlow.insertBoxIfNecessary(mv, originalPrimitiveExitTypeDescriptor);
+		}
+		if (skipIfNull != null) {
+			mv.visitLabel(skipIfNull);
+		}
 	}
 
 

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

@@ -21,6 +21,7 @@
 import java.util.List;
 import java.util.Map;
 
+import org.springframework.asm.Label;
 import org.springframework.asm.MethodVisitor;
 import org.springframework.core.convert.TypeDescriptor;
 import org.springframework.expression.AccessException;
@@ -47,6 +48,8 @@ public class PropertyOrFieldReference extends SpelNodeImpl {
 
 	private final boolean nullSafe;
 
+	private String originalPrimitiveExitTypeDescriptor = null;
+
 	private final String name;
 
 	private volatile PropertyAccessor cachedReadAccessor;
@@ -83,7 +86,7 @@ public TypedValue getValueInternal(ExpressionState state) throws EvaluationExcep
 		PropertyAccessor accessorToUse = this.cachedReadAccessor;
 		if (accessorToUse instanceof CompilablePropertyAccessor) {
 			CompilablePropertyAccessor accessor = (CompilablePropertyAccessor) accessorToUse;
-			this.exitTypeDescriptor = CodeFlow.toDescriptor(accessor.getPropertyType());
+			setExitTypeDescriptor(CodeFlow.toDescriptor(accessor.getPropertyType()));
 		}
 		return tv;
 	}
@@ -350,8 +353,40 @@ public void generateCode(MethodVisitor mv, CodeFlow cf) {
 		if (!(accessorToUse instanceof CompilablePropertyAccessor)) {
 			throw new IllegalStateException("Property accessor is not compilable: " + accessorToUse);
 		}
+		Label skipIfNull = null;
+		if (nullSafe) {
+			mv.visitInsn(DUP);
+			skipIfNull = new Label();
+			Label continueLabel = new Label();
+			mv.visitJumpInsn(IFNONNULL,continueLabel);
+			CodeFlow.insertCheckCast(mv, this.exitTypeDescriptor);
+			mv.visitJumpInsn(GOTO, skipIfNull);
+			mv.visitLabel(continueLabel);
+		}
 		((CompilablePropertyAccessor) accessorToUse).generateCode(this.name, mv, cf);
 		cf.pushDescriptor(this.exitTypeDescriptor);
+		if (originalPrimitiveExitTypeDescriptor != null) {
+			// The output of the accessor is a primitive but from the block above it might be null,
+			// so to have a common stack element type at skipIfNull target it is necessary
+			// to box the primitive
+			CodeFlow.insertBoxIfNecessary(mv, originalPrimitiveExitTypeDescriptor);
+		}
+		if (skipIfNull != null) {
+			mv.visitLabel(skipIfNull);
+		}
+	}
+
+	void setExitTypeDescriptor(String descriptor) {
+		// If this property or field access would return a primitive - and yet
+		// it is also marked null safe - then the exit type descriptor must be
+		// promoted to the box type to allow a null value to be passed on
+		if (this.nullSafe && CodeFlow.isPrimitive(descriptor)) {
+			this.originalPrimitiveExitTypeDescriptor = descriptor;
+			this.exitTypeDescriptor = CodeFlow.toBoxedDescriptor(descriptor);
+		}
+		else {
+			this.exitTypeDescriptor = descriptor;
+		}
 	}
 
 
@@ -379,8 +414,7 @@ public TypedValue getValue() {
 					this.ref.getValueInternal(this.contextObject, this.evalContext, this.autoGrowNullReferences);
 			PropertyAccessor accessorToUse = this.ref.cachedReadAccessor;
 			if (accessorToUse instanceof CompilablePropertyAccessor) {
-				this.ref.exitTypeDescriptor =
-						CodeFlow.toDescriptor(((CompilablePropertyAccessor) accessorToUse).getPropertyType());
+				this.ref.setExitTypeDescriptor(CodeFlow.toDescriptor(((CompilablePropertyAccessor) accessorToUse).getPropertyType()));
 			}
 			return value;
 		}