From 0cb4043aac8a3193b2147c0bebd6c06f8eac9037 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Fri, 29 Sep 2023 12:33:59 +0200 Subject: [PATCH 1/4] Polishing --- .../springframework/expression/spel/CodeFlow.java | 4 ++-- .../expression/spel/ast/MethodReference.java | 4 ++-- .../expression/spel/standard/SpelCompiler.java | 7 ++++--- .../spel/SpelCompilationCoverageTests.java | 15 +++++++++++++++ 4 files changed, 23 insertions(+), 7 deletions(-) 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 20f3d4cdd62..f9e2fb11b48 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..1edf395b910 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; @@ -260,7 +260,7 @@ public class MethodReference extends SpelNodeImpl { Method method = reflectiveMethodExecutor.getMethod(); String descriptor = CodeFlow.toDescriptor(method.getReturnType()); if (this.nullSafe && CodeFlow.isPrimitive(descriptor)) { - this.originalPrimitiveExitTypeDescriptor = descriptor; + this.originalPrimitiveExitTypeDescriptor = descriptor.charAt(0); this.exitTypeDescriptor = CodeFlow.toBoxedDescriptor(descriptor); } else { 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 695d82c1b36..2f73dbf321c 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2023 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -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..01cd0e446b7 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 @@ -6255,4 +6255,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); + // } + // } + } From 5ff9e6955ce372ccca13a93dd615348197f4249f Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Wed, 27 Sep 2023 17:16:54 +0200 Subject: [PATCH 2/4] Test status quo for void function references in SpEL --- .../spel/SpelCompilationCoverageTests.java | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) 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 01cd0e446b7..f1c247400a4 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 { @@ -997,6 +1000,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. From 1fe2216c59695bc6279b2a98e731bd1880496441 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Thu, 28 Sep 2023 16:04:27 +0200 Subject: [PATCH 3/4] Test status quo for null-safe Void method references in SpEL --- .../spel/SpelCompilationCoverageTests.java | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) 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 f1c247400a4..698ee14e2ba 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 @@ -3898,6 +3898,40 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { tc.reset(); } + @Test + 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(TestClass5._i).isEqualTo(1); + + TestClass5._i = 0; + assertCanCompile(expression); + expression.getValue(); + assertThat(TestClass5._i).isEqualTo(1); + } + + @Test + 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(TestClass5._i).isEqualTo(1); + + TestClass5._i = 0; + assertCanCompile(expression); + expression.getValue(); + assertThat(TestClass5._i).isEqualTo(1); + } + @Test void methodReference() { TestClass5 tc = new TestClass5(); @@ -5712,8 +5746,19 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { public void one() { 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; } From 9aab4a60f50510422edf4bada754acf855c4d75b Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Thu, 28 Sep 2023 13:22:15 +0200 Subject: [PATCH 4/4] 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;