From 9aab4a60f50510422edf4bada754acf855c4d75b Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Thu, 28 Sep 2023 13:22:15 +0200 Subject: [PATCH] Support safe navigation operator with void methods in SpEL Prior to this commit the Spring Expression Language (SpEL) was able to properly parse an expression that uses the safe navigation operator (?.) with a method that has a `void` return type (for example, "myObject?.doSomething()"); however, SpEL was not able to evaluate or compile such expressions. This commit addresses the evaluation issue by selectively not boxing the exit type descriptor (for inclusion in the generated bytecode) when the method's return type is `void`. This commit addresses the compilation issue by pushing a null object reference onto the stack in the generated byte code when the method's return type is `void`. Closes gh-27421 --- .../expression/spel/ast/MethodReference.java | 10 ++- .../spel/SpelCompilationCoverageTests.java | 86 +++++++++++++++++-- 2 files changed, 85 insertions(+), 11 deletions(-) diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/MethodReference.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/MethodReference.java index 1edf395b910..0e7af3e02eb 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/MethodReference.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/MethodReference.java @@ -259,7 +259,7 @@ public class MethodReference extends SpelNodeImpl { if (executorToCheck != null && executorToCheck.get() instanceof ReflectiveMethodExecutor reflectiveMethodExecutor) { Method method = reflectiveMethodExecutor.getMethod(); String descriptor = CodeFlow.toDescriptor(method.getReturnType()); - if (this.nullSafe && CodeFlow.isPrimitive(descriptor)) { + if (this.nullSafe && CodeFlow.isPrimitive(descriptor) && (descriptor.charAt(0) != 'V')) { this.originalPrimitiveExitTypeDescriptor = descriptor.charAt(0); this.exitTypeDescriptor = CodeFlow.toBoxedDescriptor(descriptor); } @@ -359,10 +359,16 @@ public class MethodReference extends SpelNodeImpl { if (this.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 + // so to have a 'common stack' element at the skipIfNull target we need to box the primitive. CodeFlow.insertBoxIfNecessary(mv, this.originalPrimitiveExitTypeDescriptor); } + if (skipIfNull != null) { + if ("V".equals(this.exitTypeDescriptor)) { + // If the method return type is 'void', we need to push a null object + // reference onto the stack to satisfy the needs of the skipIfNull target. + mv.visitInsn(ACONST_NULL); + } mv.visitLabel(skipIfNull); } } diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java index 698ee14e2ba..c7fa4f887ee 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java @@ -771,6 +771,34 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { assertThat(expression.getValue(context)).isNull(); } + @Test // gh-27421 + public void nullSafeMethodChainingWithNonStaticVoidMethod() throws Exception { + FooObjectHolder foh = new FooObjectHolder(); + StandardEvaluationContext context = new StandardEvaluationContext(foh); + SpelExpression expression = (SpelExpression) parser.parseExpression("getFoo()?.doFoo()"); + + FooObject.doFooInvoked = false; + assertThat(expression.getValue(context)).isNull(); + assertThat(FooObject.doFooInvoked).isTrue(); + + FooObject.doFooInvoked = false; + foh.foo = null; + assertThat(expression.getValue(context)).isNull(); + assertThat(FooObject.doFooInvoked).isFalse(); + + assertCanCompile(expression); + + FooObject.doFooInvoked = false; + foh.foo = new FooObject(); + assertThat(expression.getValue(context)).isNull(); + assertThat(FooObject.doFooInvoked).isTrue(); + + FooObject.doFooInvoked = false; + foh.foo = null; + assertThat(expression.getValue(context)).isNull(); + assertThat(FooObject.doFooInvoked).isFalse(); + } + @Test public void nullsafeMethodChaining_SPR16489() throws Exception { FooObjectHolder foh = new FooObjectHolder(); @@ -3898,37 +3926,71 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { tc.reset(); } - @Test - void nullSafeInvocationOfNonStaticVoidWrapperMethod() { + @Test // gh-27421 + public void nullSafeInvocationOfNonStaticVoidMethod() { + // non-static method, no args, void return + expression = parser.parseExpression("new %s()?.one()".formatted(TestClass5.class.getName())); + + assertCantCompile(expression); + + TestClass5._i = 0; + assertThat(expression.getValue()).isNull(); + assertThat(TestClass5._i).isEqualTo(1); + + TestClass5._i = 0; + assertCanCompile(expression); + assertThat(expression.getValue()).isNull(); + assertThat(TestClass5._i).isEqualTo(1); + } + + @Test // gh-27421 + public void nullSafeInvocationOfStaticVoidMethod() { + // static method, no args, void return + expression = parser.parseExpression("T(%s)?.two()".formatted(TestClass5.class.getName())); + + assertCantCompile(expression); + + TestClass5._i = 0; + assertThat(expression.getValue()).isNull(); + assertThat(TestClass5._i).isEqualTo(1); + + TestClass5._i = 0; + assertCanCompile(expression); + assertThat(expression.getValue()).isNull(); + assertThat(TestClass5._i).isEqualTo(1); + } + + @Test // gh-27421 + public void nullSafeInvocationOfNonStaticVoidWrapperMethod() { // non-static method, no args, Void return expression = parser.parseExpression("new %s()?.oneVoidWrapper()".formatted(TestClass5.class.getName())); assertCantCompile(expression); TestClass5._i = 0; - expression.getValue(); + assertThat(expression.getValue()).isNull(); assertThat(TestClass5._i).isEqualTo(1); TestClass5._i = 0; assertCanCompile(expression); - expression.getValue(); + assertThat(expression.getValue()).isNull(); assertThat(TestClass5._i).isEqualTo(1); } - @Test - void nullSafeInvocationOfStaticVoidWrapperMethod() { + @Test // gh-27421 + public void nullSafeInvocationOfStaticVoidWrapperMethod() { // static method, no args, Void return expression = parser.parseExpression("T(%s)?.twoVoidWrapper()".formatted(TestClass5.class.getName())); assertCantCompile(expression); TestClass5._i = 0; - expression.getValue(); + assertThat(expression.getValue()).isNull(); assertThat(TestClass5._i).isEqualTo(1); TestClass5._i = 0; assertCanCompile(expression); - expression.getValue(); + assertThat(expression.getValue()).isNull(); assertThat(TestClass5._i).isEqualTo(1); } @@ -5496,7 +5558,10 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { public static class FooObject { + static boolean doFooInvoked = false; + public Object getObject() { return "hello"; } + public void doFoo() { doFooInvoked = true; } } @@ -5744,7 +5809,10 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { field = null; } - public void one() { i = 1; } + public void one() { + _i = 1; + this.i = 1; + } public Void oneVoidWrapper() { _i = 1;