From 29bb7b907c354ebc494afdfe38a0ed6f72f6272c Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Mon, 13 May 2024 15:15:16 +0200 Subject: [PATCH 1/3] Polish SpelCompilationCoverageTests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit See gh-32804 Co-authored-by: Mikaël Francoeur --- .../spel/SpelCompilationCoverageTests.java | 64 +++++++++---------- 1 file changed, 32 insertions(+), 32 deletions(-) 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 32dd5eac5f9..f579d66080e 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 @@ -2277,7 +2277,7 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { @Test void compiledExpressionShouldWorkWhenUsingCustomFunctionWithVarargs() throws Exception { - StandardEvaluationContext context = null; + StandardEvaluationContext context; // Here the target method takes Object... and we are passing a string expression = parser.parseExpression("#doFormat('hey %s', 'there')"); @@ -2287,7 +2287,7 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { ((SpelExpression) expression).setEvaluationContext(context); assertThat(expression.getValue(String.class)).isEqualTo("hey there"); - assertThat(((SpelNodeImpl) ((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(String.class)).isEqualTo("hey there"); @@ -2298,7 +2298,7 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { ((SpelExpression) expression).setEvaluationContext(context); assertThat(expression.getValue(String.class)).isEqualTo("hey there"); - assertThat(((SpelNodeImpl) ((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(String.class)).isEqualTo("hey there"); @@ -2310,7 +2310,7 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { ((SpelExpression) expression).setEvaluationContext(context); assertThat(expression.getValue(String.class)).isEqualTo("hey there"); - assertThat(((SpelNodeImpl) ((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(String.class)).isEqualTo("hey there"); } @@ -2465,173 +2465,173 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { expression = parser.parseExpression("#append('a','b','c')"); assertThat(expression.getValue(context).toString()).isEqualTo("abc"); - assertThat(((SpelNodeImpl)((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(context).toString()).isEqualTo("abc"); expression = parser.parseExpression("#append('a')"); assertThat(expression.getValue(context).toString()).isEqualTo("a"); - assertThat(((SpelNodeImpl)((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(context).toString()).isEqualTo("a"); expression = parser.parseExpression("#append()"); assertThat(expression.getValue(context).toString()).isEmpty(); - assertThat(((SpelNodeImpl)((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(context).toString()).isEmpty(); expression = parser.parseExpression("#append(#stringArray)"); assertThat(expression.getValue(context).toString()).isEqualTo("xyz"); - assertThat(((SpelNodeImpl)((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(context).toString()).isEqualTo("xyz"); // This is a methodreference invocation, to compare with functionreference expression = parser.parseExpression("append(#stringArray)"); assertThat(expression.getValue(context, new SomeCompareMethod2()).toString()).isEqualTo("xyz"); - assertThat(((SpelNodeImpl)((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(context, new SomeCompareMethod2()).toString()).isEqualTo("xyz"); expression = parser.parseExpression("#append2('a','b','c')"); assertThat(expression.getValue(context).toString()).isEqualTo("abc"); - assertThat(((SpelNodeImpl)((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(context).toString()).isEqualTo("abc"); expression = parser.parseExpression("append2('a','b')"); assertThat(expression.getValue(context, new SomeCompareMethod2()).toString()).isEqualTo("ab"); - assertThat(((SpelNodeImpl)((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(context, new SomeCompareMethod2()).toString()).isEqualTo("ab"); expression = parser.parseExpression("#append2('a','b')"); assertThat(expression.getValue(context).toString()).isEqualTo("ab"); - assertThat(((SpelNodeImpl)((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(context).toString()).isEqualTo("ab"); expression = parser.parseExpression("#append2()"); assertThat(expression.getValue(context).toString()).isEmpty(); - assertThat(((SpelNodeImpl)((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(context).toString()).isEmpty(); expression = parser.parseExpression("#append3(#stringArray)"); assertThat(expression.getValue(context, new SomeCompareMethod2()).toString()).isEqualTo("xyz"); - assertThat(((SpelNodeImpl)((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(context, new SomeCompareMethod2()).toString()).isEqualTo("xyz"); // TODO Determine why the String[] is passed as the first element of the Object... varargs array instead of the entire varargs array. // expression = parser.parseExpression("#append2(#stringArray)"); // assertThat(expression.getValue(context)).hasToString("xyz"); - // assertThat(((SpelNodeImpl) ((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + // assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); // assertCanCompile(expression); // assertThat(expression.getValue(context)).hasToString("xyz"); expression = parser.parseExpression("#sum(1,2,3)"); assertThat(expression.getValue(context)).isEqualTo(6); - assertThat(((SpelNodeImpl)((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(context)).isEqualTo(6); expression = parser.parseExpression("#sum(2)"); assertThat(expression.getValue(context)).isEqualTo(2); - assertThat(((SpelNodeImpl)((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(context)).isEqualTo(2); expression = parser.parseExpression("#sum()"); assertThat(expression.getValue(context)).isEqualTo(0); - assertThat(((SpelNodeImpl)((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(context)).isEqualTo(0); expression = parser.parseExpression("#sum(#intArray)"); assertThat(expression.getValue(context)).isEqualTo(20); - assertThat(((SpelNodeImpl)((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(context)).isEqualTo(20); expression = parser.parseExpression("#sumDouble(1.0d,2.0d,3.0d)"); assertThat(expression.getValue(context)).isEqualTo(6); - assertThat(((SpelNodeImpl)((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(context)).isEqualTo(6); expression = parser.parseExpression("#sumDouble(2.0d)"); assertThat(expression.getValue(context)).isEqualTo(2); - assertThat(((SpelNodeImpl)((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(context)).isEqualTo(2); expression = parser.parseExpression("#sumDouble()"); assertThat(expression.getValue(context)).isEqualTo(0); - assertThat(((SpelNodeImpl)((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(context)).isEqualTo(0); expression = parser.parseExpression("#sumDouble(#doubleArray)"); assertThat(expression.getValue(context)).isEqualTo(20); - assertThat(((SpelNodeImpl)((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(context)).isEqualTo(20); expression = parser.parseExpression("#sumFloat(1.0f,2.0f,3.0f)"); assertThat(expression.getValue(context)).isEqualTo(6); - assertThat(((SpelNodeImpl)((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(context)).isEqualTo(6); expression = parser.parseExpression("#sumFloat(2.0f)"); assertThat(expression.getValue(context)).isEqualTo(2); - assertThat(((SpelNodeImpl)((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(context)).isEqualTo(2); expression = parser.parseExpression("#sumFloat()"); assertThat(expression.getValue(context)).isEqualTo(0); - assertThat(((SpelNodeImpl)((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(context)).isEqualTo(0); expression = parser.parseExpression("#sumFloat(#floatArray)"); assertThat(expression.getValue(context)).isEqualTo(20); - assertThat(((SpelNodeImpl)((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(context)).isEqualTo(20); expression = parser.parseExpression("#appendChar('abc'.charAt(0),'abc'.charAt(1))"); assertThat(expression.getValue(context)).isEqualTo("ab"); - assertThat(((SpelNodeImpl)((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(context)).isEqualTo("ab"); expression = parser.parseExpression("#append4('a','b','c')"); assertThat(expression.getValue(context).toString()).isEqualTo("a::bc"); - assertThat(((SpelNodeImpl)((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(context).toString()).isEqualTo("a::bc"); expression = parser.parseExpression("#append4('a','b')"); assertThat(expression.getValue(context).toString()).isEqualTo("a::b"); - assertThat(((SpelNodeImpl)((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(context).toString()).isEqualTo("a::b"); expression = parser.parseExpression("#append4('a')"); assertThat(expression.getValue(context).toString()).isEqualTo("a::"); - assertThat(((SpelNodeImpl)((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(context).toString()).isEqualTo("a::"); expression = parser.parseExpression("#append4('a',#stringArray)"); assertThat(expression.getValue(context).toString()).isEqualTo("a::xyz"); - assertThat(((SpelNodeImpl)((SpelExpression) expression).getAST()).isCompilable()).isTrue(); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(context).toString()).isEqualTo("a::xyz"); } From 12727a2c4f9ba6b7db3a5afa09a43419a5f2c6e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mika=C3=ABl=20Francoeur?= Date: Fri, 3 May 2024 19:21:08 -0400 Subject: [PATCH 2/3] Support compilation of varargs invocations in SpEL for array subtypes This commit introduces support for compiling SpEL expressions that contain varargs invocations where the supplied array is a subtype of the declared varargs array type. See gh-32804 --- .../expression/spel/ast/SpelNodeImpl.java | 86 +++++------ .../spel/SpelCompilationCoverageTests.java | 133 +++++++++++++++--- 2 files changed, 155 insertions(+), 64 deletions(-) diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/SpelNodeImpl.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/SpelNodeImpl.java index 06499fa14c4..2568c68f2a9 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/SpelNodeImpl.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/SpelNodeImpl.java @@ -16,9 +16,8 @@ package org.springframework.expression.spel.ast; -import java.lang.reflect.Constructor; -import java.lang.reflect.Member; -import java.lang.reflect.Method; +import java.lang.reflect.Executable; +import java.util.Objects; import java.util.function.Supplier; import org.springframework.asm.MethodVisitor; @@ -214,62 +213,67 @@ public abstract class SpelNodeImpl implements SpelNode, Opcodes { * and if it is then the argument values will be appropriately packaged into an array. * @param mv the method visitor where code should be generated * @param cf the current codeflow - * @param member the method or constructor for which arguments are being set up + * @param executable the method or constructor for which arguments are being set up * @param arguments the expression nodes for the expression supplied argument values */ - protected static void generateCodeForArguments(MethodVisitor mv, CodeFlow cf, Member member, SpelNodeImpl[] arguments) { - String[] paramDescriptors = null; - boolean isVarargs = false; - if (member instanceof Constructor ctor) { - paramDescriptors = CodeFlow.toDescriptors(ctor.getParameterTypes()); - isVarargs = ctor.isVarArgs(); - } - else { // Method - Method method = (Method)member; - paramDescriptors = CodeFlow.toDescriptors(method.getParameterTypes()); - isVarargs = method.isVarArgs(); - } - if (isVarargs) { + protected static void generateCodeForArguments(MethodVisitor mv, CodeFlow cf, Executable executable, SpelNodeImpl[] arguments) { + String[] paramDescriptors = CodeFlow.toDescriptors(executable.getParameterTypes()); + int paramCount = paramDescriptors.length; + + if (executable.isVarArgs()) { // The final parameter may or may not need packaging into an array, or nothing may // have been passed to satisfy the varargs and so something needs to be built. - int p = 0; // Current supplied argument being processed - int childCount = arguments.length; // Fulfill all the parameter requirements except the last one - for (p = 0; p < paramDescriptors.length - 1; p++) { - cf.generateCodeForArgument(mv, arguments[p], paramDescriptors[p]); + for (int i = 0; i < paramCount - 1; i++) { + cf.generateCodeForArgument(mv, arguments[i], paramDescriptors[i]); } - SpelNodeImpl lastChild = (childCount == 0 ? null : arguments[childCount - 1]); - String arrayType = paramDescriptors[paramDescriptors.length - 1]; - // Determine if the final passed argument is already suitably packaged in array - // form to be passed to the method - if (lastChild != null && arrayType.equals(lastChild.getExitDescriptor())) { - cf.generateCodeForArgument(mv, lastChild, paramDescriptors[p]); - } - else { - arrayType = arrayType.substring(1); // trim the leading '[', may leave other '[' - // build array big enough to hold remaining arguments - CodeFlow.insertNewArrayCode(mv, childCount - p, arrayType); - // Package up the remaining arguments into the array - int arrayindex = 0; - while (p < childCount) { - SpelNodeImpl child = arguments[p]; + int argCount = arguments.length; + String varargsType = paramDescriptors[paramCount - 1]; + String varargsComponentType = varargsType.substring(1); // trim the leading '[', may leave other '[' + + if (needsVarargsArrayWrapping(arguments, paramDescriptors)) { + // Package up the remaining arguments into an array + CodeFlow.insertNewArrayCode(mv, argCount - paramCount + 1, varargsComponentType); + for (int argIndex = paramCount - 1, arrayIndex = 0; argIndex < argCount; argIndex++, arrayIndex++) { + SpelNodeImpl child = arguments[argIndex]; mv.visitInsn(DUP); - CodeFlow.insertOptimalLoad(mv, arrayindex++); - cf.generateCodeForArgument(mv, child, arrayType); - CodeFlow.insertArrayStore(mv, arrayType); - p++; + CodeFlow.insertOptimalLoad(mv, arrayIndex); + cf.generateCodeForArgument(mv, child, varargsComponentType); + CodeFlow.insertArrayStore(mv, varargsComponentType); } } + else if (varargsType.equals(arguments[argCount - 1].getExitDescriptor())) { + // varargs type matches type of last argument + cf.generateCodeForArgument(mv, arguments[argCount - 1], paramDescriptors[paramCount - 1]); + } + else { + // last argument is an array and varargs component type is a supertype of argument component type + cf.generateCodeForArgument(mv, arguments[argCount - 1], varargsComponentType); + } } else { - for (int i = 0; i < paramDescriptors.length;i++) { + for (int i = 0; i < paramCount; i++) { cf.generateCodeForArgument(mv, arguments[i], paramDescriptors[i]); } } } + private static boolean needsVarargsArrayWrapping(SpelNodeImpl[] arguments, String[] paramDescriptors) { + if (arguments.length == 0) { + return true; + } + + String lastExitTypeDescriptor = arguments[arguments.length - 1].exitTypeDescriptor; + String lastParamDescriptor = paramDescriptors[paramDescriptors.length - 1]; + return countLeadingOpeningBrackets(Objects.requireNonNull(lastExitTypeDescriptor)) != countLeadingOpeningBrackets(lastParamDescriptor); + } + + private static long countLeadingOpeningBrackets(String lastExitTypeDescriptor) { + return lastExitTypeDescriptor.chars().takeWhile(c -> c == '[').count(); + } + /** * Ask an argument to generate its bytecode and then follow it up * with any boxing/unboxing/checkcasting to ensure it matches the expected parameter descriptor. 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 f579d66080e..d5843e9654c 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 @@ -28,8 +28,10 @@ import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.StringTokenizer; +import java.util.stream.Collectors; import java.util.stream.Stream; import example.Color; @@ -2267,6 +2269,12 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { return a+b; } + public static String concat2(Object... args) { + return Arrays.stream(args) + .map(Objects::toString) + .collect(Collectors.joining()); + } + public static String join(String...strings) { StringBuilder buf = new StringBuilder(); for (String string: strings) { @@ -2279,7 +2287,7 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { void compiledExpressionShouldWorkWhenUsingCustomFunctionWithVarargs() throws Exception { StandardEvaluationContext context; - // Here the target method takes Object... and we are passing a string + // single string argument expression = parser.parseExpression("#doFormat('hey %s', 'there')"); context = new StandardEvaluationContext(); context.registerFunction("doFormat", @@ -2291,6 +2299,7 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { assertCanCompile(expression); assertThat(expression.getValue(String.class)).isEqualTo("hey there"); + // single string argument from root array access expression = parser.parseExpression("#doFormat([0], 'there')"); context = new StandardEvaluationContext(new Object[] {"hey %s"}); context.registerFunction("doFormat", @@ -2302,6 +2311,7 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { assertCanCompile(expression); assertThat(expression.getValue(String.class)).isEqualTo("hey there"); + // single string from variable expression = parser.parseExpression("#doFormat([0], #arg)"); context = new StandardEvaluationContext(new Object[] {"hey %s"}); context.registerFunction("doFormat", @@ -2313,13 +2323,29 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); assertCanCompile(expression); assertThat(expression.getValue(String.class)).isEqualTo("hey there"); + + // string array argument + expression = parser.parseExpression("#doFormat('hey %s', #arg)"); + context = new StandardEvaluationContext(); + context.registerFunction("doFormat", + DelegatingStringFormat.class.getDeclaredMethod("format", String.class, Object[].class)); + context.setVariable("arg", new String[] { "there" }); + ((SpelExpression) expression).setEvaluationContext(context); + assertThat(expression.getValue(String.class)).isEqualTo("hey there"); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); + assertCanCompile(expression); + assertThat(expression.getValue(String.class)).isEqualTo("hey there"); } @Test void functionReference() throws Exception { EvaluationContext ctx = new StandardEvaluationContext(); Method m = getClass().getDeclaredMethod("concat", String.class, String.class); - ctx.setVariable("concat",m); + ctx.setVariable("concat", m); + Method m2 = getClass().getDeclaredMethod("concat2", Object[].class); + ctx.setVariable("concat2", m2); + Method m3 = getClass().getDeclaredMethod("join", String[].class); + ctx.setVariable("join", m3); expression = parser.parseExpression("#concat('a','b')"); assertThat(expression.getValue(ctx)).isEqualTo("ab"); @@ -2331,6 +2357,20 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { assertCanCompile(expression); assertThat(expression.getValue(ctx)).isEqualTo('b'); + // varargs + expression = parser.parseExpression("#join(#stringArray)"); + ctx.setVariable("stringArray", new String[] { "a", "b", "c" }); + assertThat(expression.getValue(ctx)).isEqualTo("abc"); + assertCanCompile(expression); + assertThat(expression.getValue(ctx)).isEqualTo("abc"); + + // varargs with argument component type that is a subtype of the varargs component type. + expression = parser.parseExpression("#concat2(#stringArray)"); + ctx.setVariable("stringArray", new String[] { "a", "b", "c" }); + assertThat(expression.getValue(ctx)).isEqualTo("abc"); + assertCanCompile(expression); + assertThat(expression.getValue(ctx)).isEqualTo("abc"); + expression = parser.parseExpression("#concat(#a,#b)"); ctx.setVariable("a", "foo"); ctx.setVariable("b", "bar"); @@ -2524,12 +2564,11 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { assertCanCompile(expression); assertThat(expression.getValue(context, new SomeCompareMethod2()).toString()).isEqualTo("xyz"); - // TODO Determine why the String[] is passed as the first element of the Object... varargs array instead of the entire varargs array. - // expression = parser.parseExpression("#append2(#stringArray)"); - // assertThat(expression.getValue(context)).hasToString("xyz"); - // assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); - // assertCanCompile(expression); - // assertThat(expression.getValue(context)).hasToString("xyz"); + expression = parser.parseExpression("#append2(#stringArray)"); + assertThat(expression.getValue(context)).hasToString("xyz"); + assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue(); + assertCanCompile(expression); + assertThat(expression.getValue(context)).hasToString("xyz"); expression = parser.parseExpression("#sum(1,2,3)"); assertThat(expression.getValue(context)).isEqualTo(6); @@ -4878,6 +4917,25 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { tc8 = (TestClass8) o; assertThat(tc8.i).isEqualTo(42); + // varargs + expression = parser.parseExpression("new " + testclass8 + "(#root)"); + Object[] objectArray = { "a", "b", "c" }; + assertThat(expression.getValue(objectArray).getClass().getName()).isEqualTo(testclass8); + assertCanCompile(expression); + o = expression.getValue(objectArray); + assertThat(o.getClass().getName()).isEqualTo(testclass8); + tc8 = (TestClass8) o; + assertThat(tc8.args).containsExactly("a", "b", "c"); + + // varargs with argument component type that is a subtype of the varargs component type. + expression = parser.parseExpression("new " + testclass8 + "(#root)"); + assertThat(expression.getValue(objectArray).getClass().getName()).isEqualTo(testclass8); + assertCanCompile(expression); + o = expression.getValue(new String[] { "a", "b", "c" }); + assertThat(o.getClass().getName()).isEqualTo(testclass8); + tc8 = (TestClass8) o; + assertThat(tc8.args).containsExactly("a", "b", "c"); + // private class, can't compile it String testclass9 = "org.springframework.expression.spel.SpelCompilationCoverageTests$TestClass9"; expression = parser.parseExpression("new " + testclass9 + "(42)"); @@ -4984,6 +5042,7 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { assertThat(tc.s).isEqualTo("aaabbbccc"); tc.reset(); + // varargs object expression = parser.parseExpression("sixteen('aaa','bbb','ccc')"); assertCannotCompile(expression); expression.getValue(tc); @@ -4994,27 +5053,38 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { assertThat(tc.s).isEqualTo("aaabbbccc"); tc.reset(); + // string array from property in varargs object expression = parser.parseExpression("sixteen(seventeen)"); assertCannotCompile(expression); expression.getValue(tc); assertThat(tc.s).isEqualTo("aaabbbccc"); assertCanCompile(expression); tc.reset(); - // see TODO below - // expression.getValue(tc); - // assertThat(tc.s).isEqualTo("aaabbbccc"); - // tc.reset(); - - // TODO Determine why the String[] is passed as the first element of the Object... varargs array instead of the entire varargs array. - // expression = parser.parseExpression("sixteen(stringArray)"); - // assertCannotCompile(expression); - // expression.getValue(tc); - // assertThat(tc.s).isEqualTo("aaabbbccc"); - // assertCanCompile(expression); - // tc.reset(); - // expression.getValue(tc); - // assertThat(tc.s).isEqualTo("aaabbbccc"); - // tc.reset(); + expression.getValue(tc); + assertThat(tc.s).isEqualTo("aaabbbccc"); + tc.reset(); + + // string array from variable in varargs object + expression = parser.parseExpression("sixteen(stringArray)"); + assertCannotCompile(expression); + expression.getValue(tc); + assertThat(tc.s).isEqualTo("aaabbbccc"); + assertCanCompile(expression); + tc.reset(); + expression.getValue(tc); + assertThat(tc.s).isEqualTo("aaabbbccc"); + tc.reset(); + + // string array in varargs object with other parameter + expression = parser.parseExpression("eighteen('AAA', stringArray)"); + assertCannotCompile(expression); + expression.getValue(tc); + assertThat(tc.s).isEqualTo("AAA::aaabbbccc"); + assertCanCompile(expression); + tc.reset(); + expression.getValue(tc); + assertThat(tc.s).isEqualTo("AAA::aaabbbccc"); + tc.reset(); // varargs int expression = parser.parseExpression("twelve(1,2,3)"); @@ -6863,6 +6933,18 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { public String[] seventeen() { return new String[] { "aaa", "bbb", "ccc" }; } + + public void eighteen(String a, Object... vargs) { + if (vargs == null) { + s = a + "::"; + } + else { + s = a+"::"; + for (Object varg: vargs) { + s += varg; + } + } + } } @@ -6911,6 +6993,7 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { public String s; public double d; public boolean z; + public Object[] args; public TestClass8(int i, String s, double d, boolean z) { this.i = i; @@ -6926,6 +7009,10 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { this.i = i; } + public TestClass8(Object... args) { + this.args = args; + } + @SuppressWarnings("unused") private TestClass8(String a, String b) { this.s = a+b; From 8fe4493a7d5af7df63165ceb8f6d984d7f285a3d Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Mon, 13 May 2024 17:13:47 +0200 Subject: [PATCH 3/3] Revise compilation support in SpEL for varargs array subtypes This commit first reverts changes to SpelNodeImpl from the previous commit in order to reduce the scope of the overall change set. This commit then implements a different approach to support type-safe checks for array subtype compatibility. In order to support backward compatibility, this commit also reintroduces generateCodeForArguments(MethodVisitor, CodeFlow, Member, SpelNodeImpl[]) in deprecated form. See gh-32804 --- .../expression/spel/ast/SpelNodeImpl.java | 107 ++++++++++++------ .../spel/SpelCompilationCoverageTests.java | 15 ++- 2 files changed, 79 insertions(+), 43 deletions(-) diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/SpelNodeImpl.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/SpelNodeImpl.java index 2568c68f2a9..bc71a1c243c 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/SpelNodeImpl.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/SpelNodeImpl.java @@ -17,11 +17,12 @@ package org.springframework.expression.spel.ast; import java.lang.reflect.Executable; -import java.util.Objects; +import java.lang.reflect.Member; import java.util.function.Supplier; import org.springframework.asm.MethodVisitor; import org.springframework.asm.Opcodes; +import org.springframework.asm.Type; import org.springframework.expression.EvaluationException; import org.springframework.expression.TypedValue; import org.springframework.expression.common.ExpressionUtils; @@ -32,7 +33,9 @@ import org.springframework.expression.spel.SpelMessage; import org.springframework.expression.spel.SpelNode; import org.springframework.lang.Nullable; import org.springframework.util.Assert; +import org.springframework.util.ClassUtils; import org.springframework.util.ObjectUtils; +import org.springframework.util.StringUtils; /** * The common supertype of all AST nodes in a parsed Spring Expression Language @@ -213,65 +216,95 @@ public abstract class SpelNodeImpl implements SpelNode, Opcodes { * and if it is then the argument values will be appropriately packaged into an array. * @param mv the method visitor where code should be generated * @param cf the current codeflow - * @param executable the method or constructor for which arguments are being set up + * @param member the method or constructor for which arguments are being set up * @param arguments the expression nodes for the expression supplied argument values + * @deprecated As of Spring Framework 6.2, in favor of + * {@link #generateCodeForArguments(MethodVisitor, CodeFlow, Executable, SpelNodeImpl[])} + */ + @Deprecated(since = "6.2") + protected static void generateCodeForArguments(MethodVisitor mv, CodeFlow cf, Member member, SpelNodeImpl[] arguments) { + if (member instanceof Executable executable) { + generateCodeForArguments(mv, cf, executable, arguments); + } + throw new IllegalArgumentException( + "The supplied member must be an instance of java.lang.reflect.Executable: " + member); + } + + /** + * Generate code that handles building the argument values for the specified + * {@link Executable} (method or constructor). + *

This method takes into account whether the invoked executable was + * declared to accept varargs, and if it was then the argument values will be + * appropriately packaged into an array. + * @param mv the method visitor where code should be generated + * @param cf the current {@link CodeFlow} + * @param executable the {@link Executable} (method or constructor) for which + * arguments are being set up + * @param arguments the expression nodes for the expression supplied argument + * values + * @since 6.2 */ protected static void generateCodeForArguments(MethodVisitor mv, CodeFlow cf, Executable executable, SpelNodeImpl[] arguments) { - String[] paramDescriptors = CodeFlow.toDescriptors(executable.getParameterTypes()); - int paramCount = paramDescriptors.length; + Class[] parameterTypes = executable.getParameterTypes(); + String[] paramDescriptors = CodeFlow.toDescriptors(parameterTypes); if (executable.isVarArgs()) { // The final parameter may or may not need packaging into an array, or nothing may // have been passed to satisfy the varargs and so something needs to be built. + int p = 0; // Current supplied argument being processed + int childCount = arguments.length; // Fulfill all the parameter requirements except the last one - for (int i = 0; i < paramCount - 1; i++) { - cf.generateCodeForArgument(mv, arguments[i], paramDescriptors[i]); + for (p = 0; p < paramDescriptors.length - 1; p++) { + cf.generateCodeForArgument(mv, arguments[p], paramDescriptors[p]); } - int argCount = arguments.length; - String varargsType = paramDescriptors[paramCount - 1]; - String varargsComponentType = varargsType.substring(1); // trim the leading '[', may leave other '[' + SpelNodeImpl lastChild = (childCount == 0 ? null : arguments[childCount - 1]); + ClassLoader classLoader = executable.getDeclaringClass().getClassLoader(); + Class lastChildType = (lastChild != null ? + loadClassForExitDescriptor(lastChild.getExitDescriptor(), classLoader) : null); + Class lastParameterType = parameterTypes[parameterTypes.length - 1]; - if (needsVarargsArrayWrapping(arguments, paramDescriptors)) { - // Package up the remaining arguments into an array - CodeFlow.insertNewArrayCode(mv, argCount - paramCount + 1, varargsComponentType); - for (int argIndex = paramCount - 1, arrayIndex = 0; argIndex < argCount; argIndex++, arrayIndex++) { - SpelNodeImpl child = arguments[argIndex]; - mv.visitInsn(DUP); - CodeFlow.insertOptimalLoad(mv, arrayIndex); - cf.generateCodeForArgument(mv, child, varargsComponentType); - CodeFlow.insertArrayStore(mv, varargsComponentType); - } - } - else if (varargsType.equals(arguments[argCount - 1].getExitDescriptor())) { - // varargs type matches type of last argument - cf.generateCodeForArgument(mv, arguments[argCount - 1], paramDescriptors[paramCount - 1]); + // Determine if the final passed argument is already suitably packaged in array + // form to be passed to the method + if (lastChild != null && lastChildType != null && lastParameterType.isAssignableFrom(lastChildType)) { + cf.generateCodeForArgument(mv, lastChild, paramDescriptors[p]); } else { - // last argument is an array and varargs component type is a supertype of argument component type - cf.generateCodeForArgument(mv, arguments[argCount - 1], varargsComponentType); + String arrayType = paramDescriptors[paramDescriptors.length - 1]; + arrayType = arrayType.substring(1); // trim the leading '[', may leave other '[' + // build array big enough to hold remaining arguments + CodeFlow.insertNewArrayCode(mv, childCount - p, arrayType); + // Package up the remaining arguments into the array + int arrayindex = 0; + while (p < childCount) { + SpelNodeImpl child = arguments[p]; + mv.visitInsn(DUP); + CodeFlow.insertOptimalLoad(mv, arrayindex++); + cf.generateCodeForArgument(mv, child, arrayType); + CodeFlow.insertArrayStore(mv, arrayType); + p++; + } } } else { - for (int i = 0; i < paramCount; i++) { + for (int i = 0; i < paramDescriptors.length;i++) { cf.generateCodeForArgument(mv, arguments[i], paramDescriptors[i]); } } } - private static boolean needsVarargsArrayWrapping(SpelNodeImpl[] arguments, String[] paramDescriptors) { - if (arguments.length == 0) { - return true; + @Nullable + private static Class loadClassForExitDescriptor(@Nullable String exitDescriptor, ClassLoader classLoader) { + if (!StringUtils.hasText(exitDescriptor)) { + return null; } - - String lastExitTypeDescriptor = arguments[arguments.length - 1].exitTypeDescriptor; - String lastParamDescriptor = paramDescriptors[paramDescriptors.length - 1]; - return countLeadingOpeningBrackets(Objects.requireNonNull(lastExitTypeDescriptor)) != countLeadingOpeningBrackets(lastParamDescriptor); - } - - private static long countLeadingOpeningBrackets(String lastExitTypeDescriptor) { - return lastExitTypeDescriptor.chars().takeWhile(c -> c == '[').count(); + String typeDescriptor = exitDescriptor; + if (typeDescriptor.startsWith("[") || typeDescriptor.startsWith("L")) { + typeDescriptor += ";"; + } + String className = Type.getType(typeDescriptor).getClassName(); + return ClassUtils.resolveClassName(className, classLoader); } /** 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 d5843e9654c..688d517334a 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 @@ -4920,19 +4920,22 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { // varargs expression = parser.parseExpression("new " + testclass8 + "(#root)"); Object[] objectArray = { "a", "b", "c" }; - assertThat(expression.getValue(objectArray).getClass().getName()).isEqualTo(testclass8); + o = expression.getValue(objectArray); + assertThat(o).isExactlyInstanceOf(TestClass8.class); assertCanCompile(expression); o = expression.getValue(objectArray); - assertThat(o.getClass().getName()).isEqualTo(testclass8); + assertThat(o).isExactlyInstanceOf(TestClass8.class); tc8 = (TestClass8) o; assertThat(tc8.args).containsExactly("a", "b", "c"); // varargs with argument component type that is a subtype of the varargs component type. expression = parser.parseExpression("new " + testclass8 + "(#root)"); - assertThat(expression.getValue(objectArray).getClass().getName()).isEqualTo(testclass8); + String[] stringArray = { "a", "b", "c" }; + o = expression.getValue(stringArray); + assertThat(o).isExactlyInstanceOf(TestClass8.class); assertCanCompile(expression); - o = expression.getValue(new String[] { "a", "b", "c" }); - assertThat(o.getClass().getName()).isEqualTo(testclass8); + o = expression.getValue(stringArray); + assertThat(o).isExactlyInstanceOf(TestClass8.class); tc8 = (TestClass8) o; assertThat(tc8.args).containsExactly("a", "b", "c"); @@ -6939,7 +6942,7 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { s = a + "::"; } else { - s = a+"::"; + s = a + "::"; for (Object varg: vargs) { s += varg; }