diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/CodeFlow.java b/spring-expression/src/main/java/org/springframework/expression/spel/CodeFlow.java index 95e220f8e9c..82eb9bc1917 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/CodeFlow.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/CodeFlow.java @@ -515,9 +515,9 @@ public class CodeFlow implements Opcodes { } /** - * Determine whether the descriptor is for a primitive type. + * Determine whether the descriptor is for a primitive type or {@code void}. * @param descriptor type descriptor - * @return {@code true} if a primitive type + * @return {@code true} if a primitive type or {@code void} */ public static boolean isPrimitive(@Nullable String descriptor) { return (descriptor != null && descriptor.length() == 1); 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 e79e6a9affe..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 @@ -60,7 +60,7 @@ public class MethodReference extends SpelNodeImpl { private final String name; @Nullable - private String originalPrimitiveExitTypeDescriptor; + private Character originalPrimitiveExitTypeDescriptor; @Nullable private volatile CachedMethodExecutor cachedExecutor; @@ -259,8 +259,8 @@ 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)) { - this.originalPrimitiveExitTypeDescriptor = descriptor; + if (this.nullSafe && CodeFlow.isPrimitive(descriptor) && (descriptor.charAt(0) != 'V')) { + this.originalPrimitiveExitTypeDescriptor = descriptor.charAt(0); this.exitTypeDescriptor = CodeFlow.toBoxedDescriptor(descriptor); } else { @@ -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/main/java/org/springframework/expression/spel/standard/SpelCompiler.java b/spring-expression/src/main/java/org/springframework/expression/spel/standard/SpelCompiler.java index 0b30e76eab0..1ca414375b5 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/standard/SpelCompiler.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/standard/SpelCompiler.java @@ -184,8 +184,9 @@ public final class SpelCompiler implements Opcodes { cf.finish(); byte[] data = cw.toByteArray(); - // TODO need to make this conditionally occur based on a debug flag - // dump(expressionToCompile.toStringAST(), clazzName, data); + // TODO Save generated class files conditionally based on a debug flag. + // Source code for the following method resides in SpelCompilationCoverageTests. + // saveGeneratedClassFile(expressionToCompile.toStringAST(), className, data); return loadClass(StringUtils.replace(className, "/", "."), data); } 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 0257ef8d2d5..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 @@ -29,6 +29,8 @@ import java.util.Set; import java.util.StringTokenizer; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.springframework.asm.MethodVisitor; import org.springframework.expression.AccessException; @@ -55,6 +57,7 @@ import static org.assertj.core.api.InstanceOfAssertFactories.BOOLEAN; * Checks SpelCompiler behavior. This should cover compilation all compiled node types. * * @author Andy Clement + * @author Sam Brannen * @since 4.1 */ public class SpelCompilationCoverageTests extends AbstractExpressionTests { @@ -768,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(); @@ -997,6 +1028,60 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { assertThat(expression.getValue(ctx).toString()).isEqualTo("4.0"); } + @ParameterizedTest + @ValueSource(strings = {"voidMethod", "voidWrapperMethod"}) + public void voidFunctionReference(String method) throws Exception { + assertVoidFunctionReferenceBehavior(method); + } + + private void assertVoidFunctionReferenceBehavior(String methodName) throws Exception { + Method method = getClass().getDeclaredMethod(methodName, String.class); + + EvaluationContext ctx = new StandardEvaluationContext(); + ctx.setVariable("voidMethod", method); + + expression = parser.parseExpression("#voidMethod('a')"); + + voidMethodInvokedWith = null; + expression.getValue(ctx); + assertThat(voidMethodInvokedWith).isEqualTo("a"); + assertCanCompile(expression); + + voidMethodInvokedWith = null; + expression.getValue(ctx); + assertThat(voidMethodInvokedWith).isEqualTo("a"); + assertCanCompile(expression); + + voidMethodInvokedWith = null; + expression.getValue(ctx); + assertThat(voidMethodInvokedWith).isEqualTo("a"); + assertCanCompile(expression); + + expression = parser.parseExpression("#voidMethod(#a)"); + ctx.setVariable("a", "foo"); + + voidMethodInvokedWith = null; + expression.getValue(ctx); + assertThat(voidMethodInvokedWith).isEqualTo("foo"); + assertCanCompile(expression); + + voidMethodInvokedWith = null; + expression.getValue(ctx); + assertThat(voidMethodInvokedWith).isEqualTo("foo"); + assertCanCompile(expression); + } + + private static String voidMethodInvokedWith; + + public static Void voidWrapperMethod(String str) { + voidMethodInvokedWith = str; + return null; + } + + public static void voidMethod(String str) { + voidMethodInvokedWith = str; + } + @Test public void functionReferenceVisibility_SPR12359() throws Exception { // Confirms visibility of what is being called. @@ -3841,6 +3926,74 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { tc.reset(); } + @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; + 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 nullSafeInvocationOfStaticVoidWrapperMethod() { + // static method, no args, Void return + expression = parser.parseExpression("T(%s)?.twoVoidWrapper()".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 void methodReference() { TestClass5 tc = new TestClass5(); @@ -5405,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; } } @@ -5653,10 +5809,24 @@ 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; + this.i = 1; + return null; + } public static void two() { _i = 1; } + public static Void twoVoidWrapper() { + _i = 1; + return null; + } + public String three() { return "hello"; } public long four() { return 3277700L; } @@ -6255,4 +6425,19 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { } } + // NOTE: saveGeneratedClassFile() can be copied to SpelCompiler and uncommented + // at the end of createExpressionClass(SpelNodeImpl) in order to review generated + // byte code for debugging purposes. + // + // private static void saveGeneratedClassFile(String stringAST, String className, byte[] data) { + // Path path = Path.of("build", StringUtils.replace(className, "/", ".") + ".class"); + // System.out.println("Writing compiled SpEL expression [%s] to [%s]".formatted(stringAST, path.toAbsolutePath())); + // try { + // Files.copy(new ByteArrayInputStream(data), path); + // } + // catch (IOException ex) { + // throw new UncheckedIOException(ex); + // } + // } + }