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 1edf395b9108..0e7af3e02eb8 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 @@ private void updateExitTypeDescriptor() { 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 void generateCode(MethodVisitor mv, CodeFlow cf) { 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 698ee14e2baa..c7fa4f887ee3 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 void nullsafeFieldPropertyDereferencing_SPR16489() throws Exception { 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 @@ void methodReferenceVarargs() { 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 FooObject getFoo() { public static class FooObject { + static boolean doFooInvoked = false; + public Object getObject() { return "hello"; } + public void doFoo() { doFooInvoked = true; } } @@ -5744,7 +5809,10 @@ public void reset() { field = null; } - public void one() { i = 1; } + public void one() { + _i = 1; + this.i = 1; + } public Void oneVoidWrapper() { _i = 1;