From 9eea76820576b268b0a2cd3b9105b2e86295017a Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Wed, 6 Mar 2024 11:31:10 +0100 Subject: [PATCH] Polish SpEL internals --- .../expression/spel/ast/MethodReference.java | 20 +++++++++---------- .../support/ReflectiveMethodExecutor.java | 4 ++++ .../support/ReflectivePropertyAccessor.java | 2 +- .../expression/spel/SpelReproTests.java | 2 +- 4 files changed, 16 insertions(+), 12 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 3b60cff3ae..2bd823376c 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2023 the original author or authors. + * Copyright 2002-2024 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. @@ -309,18 +309,17 @@ public class MethodReference extends SpelNodeImpl { if (executorToCheck == null || !(executorToCheck.get() instanceof ReflectiveMethodExecutor methodExecutor)) { throw new IllegalStateException("No applicable cached executor found: " + executorToCheck); } - Method method = methodExecutor.getMethod(); - boolean isStaticMethod = Modifier.isStatic(method.getModifiers()); + boolean isStatic = Modifier.isStatic(method.getModifiers()); String descriptor = cf.lastDescriptor(); - if (descriptor == null && !isStaticMethod) { + if (descriptor == null && !isStatic) { // Nothing on the stack but something is needed cf.loadTarget(mv); } Label skipIfNull = null; - if (this.nullSafe && (descriptor != null || !isStaticMethod)) { + if (this.nullSafe && (descriptor != null || !isStatic)) { skipIfNull = new Label(); Label continueLabel = new Label(); mv.visitInsn(DUP); @@ -330,8 +329,9 @@ public class MethodReference extends SpelNodeImpl { mv.visitLabel(continueLabel); } - if (descriptor != null && isStaticMethod) { - // Something on the stack when nothing is needed + if (descriptor != null && isStatic) { + // A static method call will not consume what is on the stack, so + // it needs to be popped off. mv.visitInsn(POP); } @@ -349,13 +349,13 @@ public class MethodReference extends SpelNodeImpl { classDesc = publicDeclaringClass.getName().replace('.', '/'); } - if (!isStaticMethod && (descriptor == null || !descriptor.substring(1).equals(classDesc))) { + if (!isStatic && (descriptor == null || !descriptor.substring(1).equals(classDesc))) { CodeFlow.insertCheckCast(mv, "L" + classDesc); } generateCodeForArguments(mv, cf, method, this.children); - mv.visitMethodInsn((isStaticMethod ? INVOKESTATIC : (method.isDefault() ? INVOKEINTERFACE : INVOKEVIRTUAL)), - classDesc, method.getName(), CodeFlow.createSignatureDescriptor(method), + int opcode = (isStatic ? INVOKESTATIC : method.isDefault() ? INVOKEINTERFACE : INVOKEVIRTUAL); + mv.visitMethodInsn(opcode, classDesc, method.getName(), CodeFlow.createSignatureDescriptor(method), method.getDeclaringClass().isInterface()); cf.pushDescriptor(this.exitTypeDescriptor); diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodExecutor.java b/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodExecutor.java index 9de4917c0d..d155b6dd57 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodExecutor.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodExecutor.java @@ -40,6 +40,10 @@ public class ReflectiveMethodExecutor implements MethodExecutor { private final Method originalMethod; + /** + * The method to invoke via reflection, which is not necessarily the method + * to invoke in a compiled expression. + */ private final Method methodToInvoke; @Nullable diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectivePropertyAccessor.java b/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectivePropertyAccessor.java index 705c5be69e..c761b45671 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectivePropertyAccessor.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectivePropertyAccessor.java @@ -707,7 +707,7 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { } else { if (descriptor != null) { - // A static field/method call will not consume what is on the stack, + // A static field/method call will not consume what is on the stack, so // it needs to be popped off. mv.visitInsn(POP); } diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/SpelReproTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/SpelReproTests.java index 60d0a14a8f..cb2dbf60d7 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/SpelReproTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/SpelReproTests.java @@ -1325,7 +1325,7 @@ class SpelReproTests extends AbstractExpressionTests { assertThat(Array.get(result, 2)).isEqualTo(XYZ.Z); } - @Test + @Test // https://github.com/spring-projects/spring-framework/issues/15119 void SPR10486() { SpelExpressionParser parser = new SpelExpressionParser(); StandardEvaluationContext context = new StandardEvaluationContext();