Polishing

spring-expression/src/main/java/org/springframework/expression/spel/CodeFlow.java

@@ -971,15 +971,16 @@ public class CodeFlow implements Opcodes {
 	}
 
 	/**
-	 * Produce appropriate bytecode to store a stack item in an array. The
-	 * instruction to use varies depending on whether the type
-	 * is a primitive or reference type.
+	 * Produce appropriate bytecode to store a stack item in an array.
+	 * <p>The instruction to use varies depending on whether the type is a
+	 * primitive or reference type.
 	 * @param mv where to insert the bytecode
-	 * @param arrayElementType the type of the array elements
+	 * @param arrayComponentType the component type of the array
 	 */
-	public static void insertArrayStore(MethodVisitor mv, String arrayElementType) {
-		if (arrayElementType.length() == 1) {
-			switch (arrayElementType.charAt(0)) {
+	public static void insertArrayStore(MethodVisitor mv, String arrayComponentType) {
+		if (arrayComponentType.length() == 1) {
+			char componentType = arrayComponentType.charAt(0);
+			switch (componentType) {
 				case 'B', 'Z' -> mv.visitInsn(BASTORE);
 				case 'I' -> mv.visitInsn(IASTORE);
 				case 'J' -> mv.visitInsn(LASTORE);
@@ -987,7 +988,7 @@ public class CodeFlow implements Opcodes {
 				case 'D' -> mv.visitInsn(DASTORE);
 				case 'C' -> mv.visitInsn(CASTORE);
 				case 'S' -> mv.visitInsn(SASTORE);
-				default -> throw new IllegalArgumentException("Unexpected array type " + arrayElementType.charAt(0));
+				default -> throw new IllegalArgumentException("Unexpected array component type " + componentType);
 			}
 		}
 		else {

spring-expression/src/main/java/org/springframework/expression/spel/ast/SpelNodeImpl.java

@@ -211,18 +211,22 @@ public abstract class SpelNodeImpl implements SpelNode, Opcodes {
 
 
 	/**
-	 * Generate code that handles building the argument values for the specified method.
-	 * <p>This method will take into account whether the invoked method is a varargs method,
-	 * and if it is then the argument values will be appropriately packaged into an array.
+	 * Generate code that handles building the argument values for the specified
+	 * {@link Member} (method or constructor).
+	 * <p>This method takes into account whether the method or constructor 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 codeflow
+	 * @param cf the current {@link CodeFlow}
 	 * @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) {
+	protected static void generateCodeForArguments(
+			MethodVisitor mv, CodeFlow cf, Member member, SpelNodeImpl[] arguments) {
+
 		if (member instanceof Executable executable) {
 			generateCodeForArguments(mv, cf, executable, arguments);
 		}
@@ -233,7 +237,7 @@ public abstract class SpelNodeImpl implements SpelNode, Opcodes {
 	/**
 	 * Generate code that handles building the argument values for the specified
 	 * {@link Executable} (method or constructor).
-	 * <p>This method takes into account whether the invoked executable was
+	 * <p>This method takes into account whether the method or constructor 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
@@ -244,52 +248,56 @@ public abstract class SpelNodeImpl implements SpelNode, Opcodes {
 	 * values
 	 * @since 6.2
 	 */
-	protected static void generateCodeForArguments(MethodVisitor mv, CodeFlow cf, Executable executable, SpelNodeImpl[] arguments) {
+	protected static void generateCodeForArguments(
+			MethodVisitor mv, CodeFlow cf, Executable executable, SpelNodeImpl[] arguments) {
+
 		Class<?>[] parameterTypes = executable.getParameterTypes();
-		String[] paramDescriptors = CodeFlow.toDescriptors(parameterTypes);
+		String[] parameterDescriptors = CodeFlow.toDescriptors(parameterTypes);
+		int parameterCount = parameterTypes.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.
+			// have been passed to satisfy the varargs which means something needs to be built.
+
+			int varargsIndex = parameterCount - 1;
+			int argumentCount = arguments.length;
 			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]);
+			// Fulfill all the parameter requirements except the last one (the varargs array).
+			for (p = 0; p < varargsIndex; p++) {
+				cf.generateCodeForArgument(mv, arguments[p], parameterDescriptors[p]);
 			}
 
-			SpelNodeImpl lastChild = (childCount == 0 ? null : arguments[childCount - 1]);
+			SpelNodeImpl lastArgument = (argumentCount != 0 ? arguments[argumentCount - 1] : null);
 			ClassLoader classLoader = executable.getDeclaringClass().getClassLoader();
-			Class<?> lastChildType = (lastChild != null ?
-					loadClassForExitDescriptor(lastChild.getExitDescriptor(), classLoader) : null);
-			Class<?> lastParameterType = parameterTypes[parameterTypes.length - 1];
+			Class<?> lastArgumentType = (lastArgument != null ?
+					loadClassForExitDescriptor(lastArgument.getExitDescriptor(), classLoader) : null);
+			Class<?> lastParameterType = parameterTypes[varargsIndex];
 
 			// 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]);
+			// form to be passed to the method.
+			if (lastArgument != null && lastArgumentType != null && lastParameterType.isAssignableFrom(lastArgumentType)) {
+				cf.generateCodeForArgument(mv, lastArgument, parameterDescriptors[p]);
 			}
 			else {
-				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];
+				String arrayComponentType = parameterDescriptors[varargsIndex];
+				// Trim the leading '[', potentially leaving other '[' characters.
+				arrayComponentType = arrayComponentType.substring(1);
+				// Build array big enough to hold remaining arguments.
+				CodeFlow.insertNewArrayCode(mv, argumentCount - p, arrayComponentType);
+				// Package up the remaining arguments into the array.
+				int arrayIndex = 0;
+				while (p < argumentCount) {
 					mv.visitInsn(DUP);
-					CodeFlow.insertOptimalLoad(mv, arrayindex++);
-					cf.generateCodeForArgument(mv, child, arrayType);
-					CodeFlow.insertArrayStore(mv, arrayType);
-					p++;
+					CodeFlow.insertOptimalLoad(mv, arrayIndex++);
+					cf.generateCodeForArgument(mv, arguments[p++], arrayComponentType);
+					CodeFlow.insertArrayStore(mv, arrayComponentType);
 				}
 			}
 		}
 		else {
-			for (int i = 0; i < paramDescriptors.length;i++) {
-				cf.generateCodeForArgument(mv, arguments[i], paramDescriptors[i]);
+			for (int i = 0; i < parameterCount; i++) {
+				cf.generateCodeForArgument(mv, arguments[i], parameterDescriptors[i]);
 			}
 		}
 	}
@@ -308,8 +316,10 @@ public abstract class SpelNodeImpl implements SpelNode, Opcodes {
 	}
 
 	/**
-	 * 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.
+	 * Generate bytecode that loads the supplied argument onto the stack.
+	 * <p>This method also performs any boxing, unboxing, or check-casting
+	 * necessary to ensure that the type of the argument on the stack matches the
+	 * supplied {@code paramDesc}.
 	 * @deprecated As of Spring Framework 6.2, in favor of
 	 * {@link CodeFlow#generateCodeForArgument(MethodVisitor, SpelNode, String)}
 	 */

spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java

@@ -2642,14 +2642,12 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 		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(((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(((SpelExpression) expression).getAST().isCompilable()).isTrue();
@@ -3259,7 +3257,6 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 		assertThat(b).isTrue();
 		assertThat(aa.gotComparedTo).isEqualTo(bb);
 
-
 		List<String> ls = new ArrayList<>();
 		ls.add("foo");
 		StandardEvaluationContext context = new StandardEvaluationContext(ls);
@@ -4682,7 +4679,6 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 		assertThat(expression.getValue(Boolean.class)).isNull();
 	}
 
-
 	/**
 	 * Test variants of using T(...) and static/non-static method/property/field references.
 	 */
@@ -4775,7 +4771,6 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 		assertThat(expression.getValue(StaticsHelper.sh)).isEqualTo("mb");
 		assertCanCompile(expression);
 		assertThat(expression.getValue(StaticsHelper.sh)).isEqualTo("mb");
-
 	}
 
 	@Test
@@ -4863,86 +4858,89 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 		context = new StandardEvaluationContext(new Object[] {person.getAge()});
 		context.setVariable("it", person);
 		assertThat(ex.getValue(context, Boolean.class)).isTrue();
-		assertThat(ex.getValue(context, Boolean.class)).isTrue();
 
 		PersonInOtherPackage person2 = new PersonInOtherPackage(1);
 		ex = parser.parseRaw("#it?.age.equals([0])");
 		context = new StandardEvaluationContext(new Object[] {person2.getAge()});
 		context.setVariable("it", person2);
 		assertThat(ex.getValue(context, Boolean.class)).isTrue();
-		assertThat(ex.getValue(context, Boolean.class)).isTrue();
 
 		ex = parser.parseRaw("#it?.age.equals([0])");
 		context = new StandardEvaluationContext(new Object[] {person2.getAge()});
 		context.setVariable("it", person2);
 		assertThat((Boolean) ex.getValue(context)).isTrue();
-		assertThat((Boolean) ex.getValue(context)).isTrue();
 	}
 
 	@Test
 	void constructorReference() {
-		// simple ctor
+		// simple constructor
 		expression = parser.parseExpression("new String('123')");
 		assertThat(expression.getValue()).isEqualTo("123");
 		assertCanCompile(expression);
 		assertThat(expression.getValue()).isEqualTo("123");
 
-		String testclass8 = "org.springframework.expression.spel.SpelCompilationCoverageTests$TestClass8";
-		// multi arg ctor that includes primitives
+		String testclass8 = TestClass8.class.getName();
+		Object result;
+
+		// multi arg constructor that includes primitives
 		expression = parser.parseExpression("new " + testclass8 + "(42,'123',4.0d,true)");
-		assertThat(expression.getValue().getClass().getName()).isEqualTo(testclass8);
+		result = expression.getValue();
+		assertThat(result).isExactlyInstanceOf(TestClass8.class);
 		assertCanCompile(expression);
-		Object o = expression.getValue();
-		assertThat(o.getClass().getName()).isEqualTo(testclass8);
-		TestClass8 tc8 = (TestClass8) o;
+		result = expression.getValue();
+		assertThat(result).isExactlyInstanceOf(TestClass8.class);
+		TestClass8 tc8 = (TestClass8) result;
 		assertThat(tc8.i).isEqualTo(42);
 		assertThat(tc8.s).isEqualTo("123");
 		assertThat(tc8.d).isCloseTo(4.0d, within(0.5d));
 
 		assertThat(tc8.z).isTrue();
 
-		// no-arg ctor
+		// no-arg constructor
 		expression = parser.parseExpression("new " + testclass8 + "()");
-		assertThat(expression.getValue().getClass().getName()).isEqualTo(testclass8);
+		result = expression.getValue();
+		assertThat(result).isExactlyInstanceOf(TestClass8.class);
 		assertCanCompile(expression);
-		o = expression.getValue();
-		assertThat(o.getClass().getName()).isEqualTo(testclass8);
+		result = expression.getValue();
+		assertThat(result).isExactlyInstanceOf(TestClass8.class);
 
-		// pass primitive to reference type ctor
+		// pass primitive to reference type constructor
 		expression = parser.parseExpression("new " + testclass8 + "(42)");
-		assertThat(expression.getValue().getClass().getName()).isEqualTo(testclass8);
+		result = expression.getValue();
+		assertThat(result).isExactlyInstanceOf(TestClass8.class);
 		assertCanCompile(expression);
-		o = expression.getValue();
-		assertThat(o.getClass().getName()).isEqualTo(testclass8);
-		tc8 = (TestClass8) o;
+		result = expression.getValue();
+		assertThat(result).isExactlyInstanceOf(TestClass8.class);
+		tc8 = (TestClass8) result;
 		assertThat(tc8.i).isEqualTo(42);
 
 		// varargs
 		expression = parser.parseExpression("new " + testclass8 + "(#root)");
 		Object[] objectArray = { "a", "b", "c" };
-		o = expression.getValue(objectArray);
-		assertThat(o).isExactlyInstanceOf(TestClass8.class);
+		result = expression.getValue(objectArray);
+		assertThat(result).isExactlyInstanceOf(TestClass8.class);
 		assertCanCompile(expression);
-		o = expression.getValue(objectArray);
-		assertThat(o).isExactlyInstanceOf(TestClass8.class);
-		tc8 = (TestClass8) o;
+		result = expression.getValue(objectArray);
+		assertThat(result).isExactlyInstanceOf(TestClass8.class);
+		tc8 = (TestClass8) result;
 		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)");
 		String[] stringArray = { "a", "b", "c" };
-		o = expression.getValue(stringArray);
-		assertThat(o).isExactlyInstanceOf(TestClass8.class);
+		result = expression.getValue(stringArray);
+		assertThat(result).isExactlyInstanceOf(TestClass8.class);
 		assertCanCompile(expression);
-		o = expression.getValue(stringArray);
-		assertThat(o).isExactlyInstanceOf(TestClass8.class);
-		tc8 = (TestClass8) o;
+		result = expression.getValue(stringArray);
+		assertThat(result).isExactlyInstanceOf(TestClass8.class);
+		tc8 = (TestClass8) result;
 		assertThat(tc8.args).containsExactly("a", "b", "c");
 
 		// private class, can't compile it
-		String testclass9 = "org.springframework.expression.spel.SpelCompilationCoverageTests$TestClass9";
+		String testclass9 = TestClass9.class.getName();
 		expression = parser.parseExpression("new " + testclass9 + "(42)");
-		assertThat(expression.getValue().getClass().getName()).isEqualTo(testclass9);
+		result = expression.getValue();
+		assertThat(result).isExactlyInstanceOf(TestClass9.class);
 		assertCannotCompile(expression);
 	}