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

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

@@ -1003,4 +1003,19 @@ public class CodeFlow implements Opcodes {
 		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.ArrayList;
 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 @@ public class MethodReference extends SpelNodeImpl {
 		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 class MethodReference extends SpelNodeImpl {
 		boolean isStaticMethod = Modifier.isStatic(method.getModifiers());
 		String descriptor = cf.lastDescriptor();
 
-		if (descriptor == null) {
+		Label skipIfNull = null;
-			if (!isStaticMethod) {
+		if (descriptor == null && !isStaticMethod) {
-				// Nothing on the stack but something is needed
+			// Nothing on the stack but something is needed
-				cf.loadTarget(mv);
+			cf.loadTarget(mv);
-			}
 		}
-		else {
+		if ((descriptor != null || !isStaticMethod) && nullSafe) {
-			if (isStaticMethod) {
+			mv.visitInsn(DUP);
-				// Something on the stack when nothing is needed
+			skipIfNull = new Label();
-				mv.visitInsn(POP);
+			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 class MethodReference extends SpelNodeImpl {
 		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.HashMap;
 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 class PropertyOrFieldReference extends SpelNodeImpl {
 		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 class PropertyOrFieldReference extends SpelNodeImpl {
 		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 class PropertyOrFieldReference extends SpelNodeImpl {
 					this.ref.getValueInternal(this.contextObject, this.evalContext, this.autoGrowNullReferences);
 			PropertyAccessor accessorToUse = this.ref.cachedReadAccessor;
 			if (accessorToUse instanceof CompilablePropertyAccessor) {
-				this.ref.exitTypeDescriptor =
+				this.ref.setExitTypeDescriptor(CodeFlow.toDescriptor(((CompilablePropertyAccessor) accessorToUse).getPropertyType()));
-						CodeFlow.toDescriptor(((CompilablePropertyAccessor) accessorToUse).getPropertyType());
 			}
 			return value;
 		}

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

@@ -702,6 +702,166 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 		assertEquals("def", expression.getValue());
 	}
 	
+	@Test
+	public void nullsafeFieldPropertyDereferencing_SPR16489() throws Exception {
+		FooObjectHolder foh = new FooObjectHolder();
+		StandardEvaluationContext context = new StandardEvaluationContext();
+		context.setRootObject(foh);
+
+		// First non compiled:
+		SpelExpression expression = (SpelExpression) parser.parseExpression("foo?.object");
+		assertEquals("hello",expression.getValue(context));
+		foh.foo = null;
+		assertNull(expression.getValue(context));
+		
+		// Now revert state of foh and try compiling it:
+		foh.foo = new FooObject();
+		assertEquals("hello",expression.getValue(context));
+		assertCanCompile(expression);
+		assertEquals("hello",expression.getValue(context));
+		foh.foo = null;
+		assertNull(expression.getValue(context));
+		
+		// Static references
+		expression = (SpelExpression)parser.parseExpression("#var?.propertya");
+		context.setVariable("var", StaticsHelper.class);
+		assertEquals("sh",expression.getValue(context).toString());
+		context.setVariable("var", null);
+		assertNull(expression.getValue(context));
+		assertCanCompile(expression);
+		context.setVariable("var", StaticsHelper.class);
+		assertEquals("sh",expression.getValue(context).toString());
+		context.setVariable("var", null);
+		assertNull(expression.getValue(context));
+
+		// Single size primitive (boolean)
+		expression = (SpelExpression)parser.parseExpression("#var?.a");
+		context.setVariable("var", new TestClass4());
+		assertFalse((Boolean)expression.getValue(context));
+		context.setVariable("var", null);
+		assertNull(expression.getValue(context));
+		assertCanCompile(expression);
+		context.setVariable("var", new TestClass4());
+		assertFalse((Boolean)expression.getValue(context));
+		context.setVariable("var", null);
+		assertNull(expression.getValue(context));
+
+		// Double slot primitives
+		expression = (SpelExpression)parser.parseExpression("#var?.four");
+		context.setVariable("var", new Three());
+		assertEquals("0.04",expression.getValue(context).toString());
+		context.setVariable("var", null);
+		assertNull(expression.getValue(context));
+		assertCanCompile(expression);
+		context.setVariable("var", new Three());
+		assertEquals("0.04",expression.getValue(context).toString());
+		context.setVariable("var", null);
+		assertNull(expression.getValue(context));
+	}
+	
+	@Test
+	public void nullsafeMethodChaining_SPR16489() throws Exception {
+		FooObjectHolder foh = new FooObjectHolder();
+		StandardEvaluationContext context = new StandardEvaluationContext();
+		context.setRootObject(foh);
+
+		// First non compiled:
+		SpelExpression expression = (SpelExpression) parser.parseExpression("getFoo()?.getObject()");
+		assertEquals("hello",expression.getValue(context));
+		foh.foo = null;
+		assertNull(expression.getValue(context));
+		assertCanCompile(expression);
+		foh.foo = new FooObject();
+		assertEquals("hello",expression.getValue(context));
+		foh.foo = null;
+		assertNull(expression.getValue(context));
+		
+		// Static method references
+		expression = (SpelExpression)parser.parseExpression("#var?.methoda()");
+		context.setVariable("var", StaticsHelper.class);
+		assertEquals("sh",expression.getValue(context).toString());
+		context.setVariable("var", null);
+		assertNull(expression.getValue(context));
+		assertCanCompile(expression);
+		context.setVariable("var", StaticsHelper.class);
+		assertEquals("sh",expression.getValue(context).toString());
+		context.setVariable("var", null);
+		assertNull(expression.getValue(context));
+		
+		// Nullsafe guard on expression element evaluating to primitive/null
+		expression = (SpelExpression)parser.parseExpression("#var?.intValue()");
+		context.setVariable("var", 4);
+		assertEquals("4",expression.getValue(context).toString());
+		context.setVariable("var", null);
+		assertNull(expression.getValue(context));
+		assertCanCompile(expression);
+		context.setVariable("var", 4);
+		assertEquals("4",expression.getValue(context).toString());
+		context.setVariable("var", null);
+		assertNull(expression.getValue(context));
+
+		
+		// Nullsafe guard on expression element evaluating to primitive/null
+		expression = (SpelExpression)parser.parseExpression("#var?.booleanValue()");
+		context.setVariable("var", false);
+		assertEquals("false",expression.getValue(context).toString());
+		context.setVariable("var", null);
+		assertNull(expression.getValue(context));
+		assertCanCompile(expression);
+		context.setVariable("var", false);
+		assertEquals("false",expression.getValue(context).toString());
+		context.setVariable("var", null);
+		assertNull(expression.getValue(context));
+
+		// Nullsafe guard on expression element evaluating to primitive/null
+		expression = (SpelExpression)parser.parseExpression("#var?.booleanValue()");
+		context.setVariable("var", true);
+		assertEquals("true",expression.getValue(context).toString());
+		context.setVariable("var", null);
+		assertNull(expression.getValue(context));
+		assertCanCompile(expression);
+		context.setVariable("var", true);
+		assertEquals("true",expression.getValue(context).toString());
+		context.setVariable("var", null);
+		assertNull(expression.getValue(context));
+
+		// Nullsafe guard on expression element evaluating to primitive/null
+		expression = (SpelExpression)parser.parseExpression("#var?.longValue()");
+		context.setVariable("var", 5L);
+		assertEquals("5",expression.getValue(context).toString());
+		context.setVariable("var", null);
+		assertNull(expression.getValue(context));
+		assertCanCompile(expression);
+		context.setVariable("var", 5L);
+		assertEquals("5",expression.getValue(context).toString());
+		context.setVariable("var", null);
+		assertNull(expression.getValue(context));
+
+		// Nullsafe guard on expression element evaluating to primitive/null
+		expression = (SpelExpression)parser.parseExpression("#var?.floatValue()");
+		context.setVariable("var", 3f);
+		assertEquals("3.0",expression.getValue(context).toString());
+		context.setVariable("var", null);
+		assertNull(expression.getValue(context));
+		assertCanCompile(expression);
+		context.setVariable("var", 3f);
+		assertEquals("3.0",expression.getValue(context).toString());
+		context.setVariable("var", null);
+		assertNull(expression.getValue(context));
+
+		// Nullsafe guard on expression element evaluating to primitive/null
+		expression = (SpelExpression)parser.parseExpression("#var?.shortValue()");
+		context.setVariable("var", (short)8);
+		assertEquals("8",expression.getValue(context).toString());
+		context.setVariable("var", null);
+		assertNull(expression.getValue(context));
+		assertCanCompile(expression);
+		context.setVariable("var", (short)8);
+		assertEquals("8",expression.getValue(context).toString());
+		context.setVariable("var", null);
+		assertNull(expression.getValue(context));
+	}
+	
 	@Test
 	public void elvis() throws Exception {
 		Expression expression = parser.parseExpression("'a'?:'b'");
@@ -3063,19 +3223,47 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 		assertEquals(1.0f, expression.getValue());
 	}
 
+	@Test
+	public void compilationOfBasicNullSafeMethodReference() {
+		SpelExpressionParser parser = new SpelExpressionParser(
+				new SpelParserConfiguration(SpelCompilerMode.OFF, getClass().getClassLoader()));
+		SpelExpression expression = parser.parseRaw("#it?.equals(3)");
+		StandardEvaluationContext context = new StandardEvaluationContext(new Object[] {1});
+		context.setVariable("it", 3);
+		expression.setEvaluationContext(context);
+		assertTrue(expression.getValue(Boolean.class));
+		context.setVariable("it", null);
+		assertNull(expression.getValue(Boolean.class));
+		
+		assertCanCompile(expression);
+		
+		context.setVariable("it", 3);
+		assertTrue(expression.getValue(Boolean.class));
+		context.setVariable("it", null);
+		assertNull(expression.getValue(Boolean.class));
+	}
+	
 	@Test
 	public void failsWhenSettingContextForExpression_SPR12326() {
 		SpelExpressionParser parser = new SpelExpressionParser(
-				new SpelParserConfiguration(SpelCompilerMode.IMMEDIATE, getClass().getClassLoader()));
+				new SpelParserConfiguration(SpelCompilerMode.OFF, getClass().getClassLoader()));
 		Person3 person = new Person3("foo", 1);
 		SpelExpression expression = parser.parseRaw("#it?.age?.equals([0])");
 		StandardEvaluationContext context = new StandardEvaluationContext(new Object[] {1});
 		context.setVariable("it", person);
 		expression.setEvaluationContext(context);
 		assertTrue(expression.getValue(Boolean.class));
+		// This will trigger compilation (second usage)
 		assertTrue(expression.getValue(Boolean.class));
+		context.setVariable("it", null);
+		assertNull(expression.getValue(Boolean.class));
+		
 		assertCanCompile(expression);
+		
+		context.setVariable("it", person);
 		assertTrue(expression.getValue(Boolean.class));
+		context.setVariable("it", null);
+		assertNull(expression.getValue(Boolean.class));
 	}
 
 
@@ -5078,6 +5266,14 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 		}
 	}
 
+	public static class FooObjectHolder {
+		
+		private FooObject foo = new FooObject();
+		
+		public FooObject getFoo() {
+			return foo;
+		}
+	}
 
 	public static class FooObject {