Skip to content

Commit fc07946

Browse files
sbrannensbrannen
committed
Polishing
1 parent 061c13d commit fc07946

File tree

3 files changed

+91
-82
lines changed

3 files changed

+91
-82
lines changed

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

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -957,7 +957,7 @@ public static String[] toDescriptors(Class<?>[] types) {
957957
*/
958958
public static void insertOptimalLoad(MethodVisitor mv, int value) {
959959
if (value < 6) {
960-
mv.visitInsn(ICONST_0+value);
960+
mv.visitInsn(ICONST_0 + value);
961961
}
962962
else if (value < Byte.MAX_VALUE) {
963963
mv.visitIntInsn(BIPUSH, value);
@@ -971,23 +971,24 @@ else if (value < Short.MAX_VALUE) {
971971
}
972972

973973
/**
974-
* Produce appropriate bytecode to store a stack item in an array. The
975-
* instruction to use varies depending on whether the type
976-
* is a primitive or reference type.
974+
* Produce appropriate bytecode to store a stack item in an array.
975+
* <p>The instruction to use varies depending on whether the type is a
976+
* primitive or reference type.
977977
* @param mv where to insert the bytecode
978-
* @param arrayElementType the type of the array elements
978+
* @param arrayComponentType the component type of the array
979979
*/
980-
public static void insertArrayStore(MethodVisitor mv, String arrayElementType) {
981-
if (arrayElementType.length() == 1) {
982-
switch (arrayElementType.charAt(0)) {
980+
public static void insertArrayStore(MethodVisitor mv, String arrayComponentType) {
981+
if (arrayComponentType.length() == 1) {
982+
char componentType = arrayComponentType.charAt(0);
983+
switch (componentType) {
983984
case 'B', 'Z' -> mv.visitInsn(BASTORE);
984985
case 'I' -> mv.visitInsn(IASTORE);
985986
case 'J' -> mv.visitInsn(LASTORE);
986987
case 'F' -> mv.visitInsn(FASTORE);
987988
case 'D' -> mv.visitInsn(DASTORE);
988989
case 'C' -> mv.visitInsn(CASTORE);
989990
case 'S' -> mv.visitInsn(SASTORE);
990-
default -> throw new IllegalArgumentException("Unexpected array type " + arrayElementType.charAt(0));
991+
default -> throw new IllegalArgumentException("Unexpected array component type " + componentType);
991992
}
992993
}
993994
else {

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

Lines changed: 47 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -211,18 +211,22 @@ protected ValueRef getValueRef(ExpressionState state) throws EvaluationException
211211

212212

213213
/**
214-
* Generate code that handles building the argument values for the specified method.
215-
* <p>This method will take into account whether the invoked method is a varargs method,
216-
* and if it is then the argument values will be appropriately packaged into an array.
214+
* Generate code that handles building the argument values for the specified
215+
* {@link Member} (method or constructor).
216+
* <p>This method takes into account whether the method or constructor was
217+
* declared to accept varargs, and if it was then the argument values will be
218+
* appropriately packaged into an array.
217219
* @param mv the method visitor where code should be generated
218-
* @param cf the current codeflow
220+
* @param cf the current {@link CodeFlow}
219221
* @param member the method or constructor for which arguments are being set up
220222
* @param arguments the expression nodes for the expression supplied argument values
221223
* @deprecated As of Spring Framework 6.2, in favor of
222224
* {@link #generateCodeForArguments(MethodVisitor, CodeFlow, Executable, SpelNodeImpl[])}
223225
*/
224226
@Deprecated(since = "6.2")
225-
protected static void generateCodeForArguments(MethodVisitor mv, CodeFlow cf, Member member, SpelNodeImpl[] arguments) {
227+
protected static void generateCodeForArguments(
228+
MethodVisitor mv, CodeFlow cf, Member member, SpelNodeImpl[] arguments) {
229+
226230
if (member instanceof Executable executable) {
227231
generateCodeForArguments(mv, cf, executable, arguments);
228232
}
@@ -233,7 +237,7 @@ protected static void generateCodeForArguments(MethodVisitor mv, CodeFlow cf, Me
233237
/**
234238
* Generate code that handles building the argument values for the specified
235239
* {@link Executable} (method or constructor).
236-
* <p>This method takes into account whether the invoked executable was
240+
* <p>This method takes into account whether the method or constructor was
237241
* declared to accept varargs, and if it was then the argument values will be
238242
* appropriately packaged into an array.
239243
* @param mv the method visitor where code should be generated
@@ -244,52 +248,56 @@ protected static void generateCodeForArguments(MethodVisitor mv, CodeFlow cf, Me
244248
* values
245249
* @since 6.2
246250
*/
247-
protected static void generateCodeForArguments(MethodVisitor mv, CodeFlow cf, Executable executable, SpelNodeImpl[] arguments) {
251+
protected static void generateCodeForArguments(
252+
MethodVisitor mv, CodeFlow cf, Executable executable, SpelNodeImpl[] arguments) {
253+
248254
Class<?>[] parameterTypes = executable.getParameterTypes();
249-
String[] paramDescriptors = CodeFlow.toDescriptors(parameterTypes);
255+
String[] parameterDescriptors = CodeFlow.toDescriptors(parameterTypes);
256+
int parameterCount = parameterTypes.length;
250257

251258
if (executable.isVarArgs()) {
252259
// The final parameter may or may not need packaging into an array, or nothing may
253-
// have been passed to satisfy the varargs and so something needs to be built.
254-
int p = 0; // Current supplied argument being processed
255-
int childCount = arguments.length;
260+
// have been passed to satisfy the varargs which means something needs to be built.
261+
262+
int varargsIndex = parameterCount - 1;
263+
int argumentCount = arguments.length;
264+
int p = 0; // Current supplied argument being processed
256265

257-
// Fulfill all the parameter requirements except the last one
258-
for (p = 0; p < paramDescriptors.length - 1; p++) {
259-
cf.generateCodeForArgument(mv, arguments[p], paramDescriptors[p]);
266+
// Fulfill all the parameter requirements except the last one (the varargs array).
267+
for (p = 0; p < varargsIndex; p++) {
268+
cf.generateCodeForArgument(mv, arguments[p], parameterDescriptors[p]);
260269
}
261270

262-
SpelNodeImpl lastChild = (childCount == 0 ? null : arguments[childCount - 1]);
271+
SpelNodeImpl lastArgument = (argumentCount != 0 ? arguments[argumentCount - 1] : null);
263272
ClassLoader classLoader = executable.getDeclaringClass().getClassLoader();
264-
Class<?> lastChildType = (lastChild != null ?
265-
loadClassForExitDescriptor(lastChild.getExitDescriptor(), classLoader) : null);
266-
Class<?> lastParameterType = parameterTypes[parameterTypes.length - 1];
273+
Class<?> lastArgumentType = (lastArgument != null ?
274+
loadClassForExitDescriptor(lastArgument.getExitDescriptor(), classLoader) : null);
275+
Class<?> lastParameterType = parameterTypes[varargsIndex];
267276

268277
// Determine if the final passed argument is already suitably packaged in array
269-
// form to be passed to the method
270-
if (lastChild != null && lastChildType != null && lastParameterType.isAssignableFrom(lastChildType)) {
271-
cf.generateCodeForArgument(mv, lastChild, paramDescriptors[p]);
278+
// form to be passed to the method.
279+
if (lastArgument != null && lastArgumentType != null && lastParameterType.isAssignableFrom(lastArgumentType)) {
280+
cf.generateCodeForArgument(mv, lastArgument, parameterDescriptors[p]);
272281
}
273282
else {
274-
String arrayType = paramDescriptors[paramDescriptors.length - 1];
275-
arrayType = arrayType.substring(1); // trim the leading '[', may leave other '['
276-
// build array big enough to hold remaining arguments
277-
CodeFlow.insertNewArrayCode(mv, childCount - p, arrayType);
278-
// Package up the remaining arguments into the array
279-
int arrayindex = 0;
280-
while (p < childCount) {
281-
SpelNodeImpl child = arguments[p];
283+
String arrayComponentType = parameterDescriptors[varargsIndex];
284+
// Trim the leading '[', potentially leaving other '[' characters.
285+
arrayComponentType = arrayComponentType.substring(1);
286+
// Build array big enough to hold remaining arguments.
287+
CodeFlow.insertNewArrayCode(mv, argumentCount - p, arrayComponentType);
288+
// Package up the remaining arguments into the array.
289+
int arrayIndex = 0;
290+
while (p < argumentCount) {
282291
mv.visitInsn(DUP);
283-
CodeFlow.insertOptimalLoad(mv, arrayindex++);
284-
cf.generateCodeForArgument(mv, child, arrayType);
285-
CodeFlow.insertArrayStore(mv, arrayType);
286-
p++;
292+
CodeFlow.insertOptimalLoad(mv, arrayIndex++);
293+
cf.generateCodeForArgument(mv, arguments[p++], arrayComponentType);
294+
CodeFlow.insertArrayStore(mv, arrayComponentType);
287295
}
288296
}
289297
}
290298
else {
291-
for (int i = 0; i < paramDescriptors.length;i++) {
292-
cf.generateCodeForArgument(mv, arguments[i], paramDescriptors[i]);
299+
for (int i = 0; i < parameterCount; i++) {
300+
cf.generateCodeForArgument(mv, arguments[i], parameterDescriptors[i]);
293301
}
294302
}
295303
}
@@ -308,8 +316,10 @@ private static Class<?> loadClassForExitDescriptor(@Nullable String exitDescript
308316
}
309317

310318
/**
311-
* Ask an argument to generate its bytecode and then follow it up
312-
* with any boxing/unboxing/checkcasting to ensure it matches the expected parameter descriptor.
319+
* Generate bytecode that loads the supplied argument onto the stack.
320+
* <p>This method also performs any boxing, unboxing, or check-casting
321+
* necessary to ensure that the type of the argument on the stack matches the
322+
* supplied {@code paramDesc}.
313323
* @deprecated As of Spring Framework 6.2, in favor of
314324
* {@link CodeFlow#generateCodeForArgument(MethodVisitor, SpelNode, String)}
315325
*/

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

Lines changed: 34 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -2642,14 +2642,12 @@ void functionReferenceVarargs_SPR12359() throws Exception {
26422642
assertCanCompile(expression);
26432643
assertThat(expression.getValue(context)).isEqualTo(20);
26442644

2645-
26462645
expression = parser.parseExpression("#appendChar('abc'.charAt(0),'abc'.charAt(1))");
26472646
assertThat(expression.getValue(context)).isEqualTo("ab");
26482647
assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue();
26492648
assertCanCompile(expression);
26502649
assertThat(expression.getValue(context)).isEqualTo("ab");
26512650

2652-
26532651
expression = parser.parseExpression("#append4('a','b','c')");
26542652
assertThat(expression.getValue(context).toString()).isEqualTo("a::bc");
26552653
assertThat(((SpelExpression) expression).getAST().isCompilable()).isTrue();
@@ -3259,7 +3257,6 @@ void opEq_SPR14863() {
32593257
assertThat(b).isTrue();
32603258
assertThat(aa.gotComparedTo).isEqualTo(bb);
32613259

3262-
32633260
List<String> ls = new ArrayList<>();
32643261
ls.add("foo");
32653262
StandardEvaluationContext context = new StandardEvaluationContext(ls);
@@ -4682,7 +4679,6 @@ void failsWhenSettingContextForExpression_SPR12326() {
46824679
assertThat(expression.getValue(Boolean.class)).isNull();
46834680
}
46844681

4685-
46864682
/**
46874683
* Test variants of using T(...) and static/non-static method/property/field references.
46884684
*/
@@ -4775,7 +4771,6 @@ void constructorReference_SPR13781() {
47754771
assertThat(expression.getValue(StaticsHelper.sh)).isEqualTo("mb");
47764772
assertCanCompile(expression);
47774773
assertThat(expression.getValue(StaticsHelper.sh)).isEqualTo("mb");
4778-
47794774
}
47804775

47814776
@Test
@@ -4863,86 +4858,89 @@ void methodReferenceMissingCastAndRootObjectAccessing_SPR12326() {
48634858
context = new StandardEvaluationContext(new Object[] {person.getAge()});
48644859
context.setVariable("it", person);
48654860
assertThat(ex.getValue(context, Boolean.class)).isTrue();
4866-
assertThat(ex.getValue(context, Boolean.class)).isTrue();
48674861

48684862
PersonInOtherPackage person2 = new PersonInOtherPackage(1);
48694863
ex = parser.parseRaw("#it?.age.equals([0])");
48704864
context = new StandardEvaluationContext(new Object[] {person2.getAge()});
48714865
context.setVariable("it", person2);
48724866
assertThat(ex.getValue(context, Boolean.class)).isTrue();
4873-
assertThat(ex.getValue(context, Boolean.class)).isTrue();
48744867

48754868
ex = parser.parseRaw("#it?.age.equals([0])");
48764869
context = new StandardEvaluationContext(new Object[] {person2.getAge()});
48774870
context.setVariable("it", person2);
48784871
assertThat((Boolean) ex.getValue(context)).isTrue();
4879-
assertThat((Boolean) ex.getValue(context)).isTrue();
48804872
}
48814873

48824874
@Test
48834875
void constructorReference() {
4884-
// simple ctor
4876+
// simple constructor
48854877
expression = parser.parseExpression("new String('123')");
48864878
assertThat(expression.getValue()).isEqualTo("123");
48874879
assertCanCompile(expression);
48884880
assertThat(expression.getValue()).isEqualTo("123");
48894881

4890-
String testclass8 = "org.springframework.expression.spel.SpelCompilationCoverageTests$TestClass8";
4891-
// multi arg ctor that includes primitives
4882+
String testclass8 = TestClass8.class.getName();
4883+
Object result;
4884+
4885+
// multi arg constructor that includes primitives
48924886
expression = parser.parseExpression("new " + testclass8 + "(42,'123',4.0d,true)");
4893-
assertThat(expression.getValue().getClass().getName()).isEqualTo(testclass8);
4887+
result = expression.getValue();
4888+
assertThat(result).isExactlyInstanceOf(TestClass8.class);
48944889
assertCanCompile(expression);
4895-
Object o = expression.getValue();
4896-
assertThat(o.getClass().getName()).isEqualTo(testclass8);
4897-
TestClass8 tc8 = (TestClass8) o;
4890+
result = expression.getValue();
4891+
assertThat(result).isExactlyInstanceOf(TestClass8.class);
4892+
TestClass8 tc8 = (TestClass8) result;
48984893
assertThat(tc8.i).isEqualTo(42);
48994894
assertThat(tc8.s).isEqualTo("123");
49004895
assertThat(tc8.d).isCloseTo(4.0d, within(0.5d));
49014896

49024897
assertThat(tc8.z).isTrue();
49034898

4904-
// no-arg ctor
4899+
// no-arg constructor
49054900
expression = parser.parseExpression("new " + testclass8 + "()");
4906-
assertThat(expression.getValue().getClass().getName()).isEqualTo(testclass8);
4901+
result = expression.getValue();
4902+
assertThat(result).isExactlyInstanceOf(TestClass8.class);
49074903
assertCanCompile(expression);
4908-
o = expression.getValue();
4909-
assertThat(o.getClass().getName()).isEqualTo(testclass8);
4904+
result = expression.getValue();
4905+
assertThat(result).isExactlyInstanceOf(TestClass8.class);
49104906

4911-
// pass primitive to reference type ctor
4907+
// pass primitive to reference type constructor
49124908
expression = parser.parseExpression("new " + testclass8 + "(42)");
4913-
assertThat(expression.getValue().getClass().getName()).isEqualTo(testclass8);
4909+
result = expression.getValue();
4910+
assertThat(result).isExactlyInstanceOf(TestClass8.class);
49144911
assertCanCompile(expression);
4915-
o = expression.getValue();
4916-
assertThat(o.getClass().getName()).isEqualTo(testclass8);
4917-
tc8 = (TestClass8) o;
4912+
result = expression.getValue();
4913+
assertThat(result).isExactlyInstanceOf(TestClass8.class);
4914+
tc8 = (TestClass8) result;
49184915
assertThat(tc8.i).isEqualTo(42);
49194916

49204917
// varargs
49214918
expression = parser.parseExpression("new " + testclass8 + "(#root)");
49224919
Object[] objectArray = { "a", "b", "c" };
4923-
o = expression.getValue(objectArray);
4924-
assertThat(o).isExactlyInstanceOf(TestClass8.class);
4920+
result = expression.getValue(objectArray);
4921+
assertThat(result).isExactlyInstanceOf(TestClass8.class);
49254922
assertCanCompile(expression);
4926-
o = expression.getValue(objectArray);
4927-
assertThat(o).isExactlyInstanceOf(TestClass8.class);
4928-
tc8 = (TestClass8) o;
4923+
result = expression.getValue(objectArray);
4924+
assertThat(result).isExactlyInstanceOf(TestClass8.class);
4925+
tc8 = (TestClass8) result;
49294926
assertThat(tc8.args).containsExactly("a", "b", "c");
49304927

49314928
// varargs with argument component type that is a subtype of the varargs component type.
49324929
expression = parser.parseExpression("new " + testclass8 + "(#root)");
49334930
String[] stringArray = { "a", "b", "c" };
4934-
o = expression.getValue(stringArray);
4935-
assertThat(o).isExactlyInstanceOf(TestClass8.class);
4931+
result = expression.getValue(stringArray);
4932+
assertThat(result).isExactlyInstanceOf(TestClass8.class);
49364933
assertCanCompile(expression);
4937-
o = expression.getValue(stringArray);
4938-
assertThat(o).isExactlyInstanceOf(TestClass8.class);
4939-
tc8 = (TestClass8) o;
4934+
result = expression.getValue(stringArray);
4935+
assertThat(result).isExactlyInstanceOf(TestClass8.class);
4936+
tc8 = (TestClass8) result;
49404937
assertThat(tc8.args).containsExactly("a", "b", "c");
49414938

49424939
// private class, can't compile it
4943-
String testclass9 = "org.springframework.expression.spel.SpelCompilationCoverageTests$TestClass9";
4940+
String testclass9 = TestClass9.class.getName();
49444941
expression = parser.parseExpression("new " + testclass9 + "(42)");
4945-
assertThat(expression.getValue().getClass().getName()).isEqualTo(testclass9);
4942+
result = expression.getValue();
4943+
assertThat(result).isExactlyInstanceOf(TestClass9.class);
49464944
assertCannotCompile(expression);
49474945
}
49484946

0 commit comments

Comments
 (0)