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] 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 | 131 +++++++++++++++--- 2 files changed, 154 insertions(+), 63 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 06499fa14c..2568c68f2a 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]); + 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, 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 { - 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++; - } + // 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 f579d66080..d5843e9654 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(); + 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(); + // 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;