From 5cba32df3253955eac7135d144ed9c0c3518d174 Mon Sep 17 00:00:00 2001
From: Sam Brannen <104798+sbrannen@users.noreply.github.com>
Date: Sat, 2 Mar 2024 17:38:31 +0100
Subject: [PATCH] Polish SpEL's VariableReference

---
 .../spel/ast/VariableReference.java           | 58 ++++++++++++-------
 1 file changed, 38 insertions(+), 20 deletions(-)

diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/VariableReference.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/VariableReference.java
index 3b8af0d32b..9124038496 100644
--- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/VariableReference.java
+++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/VariableReference.java
@@ -29,7 +29,8 @@ import org.springframework.expression.spel.SpelEvaluationException;
 import org.springframework.lang.Nullable;
 
 /**
- * Represents a variable reference &mdash; for example, {@code #someVar}.
+ * Represents a variable reference &mdash; for example, {@code #root}, {@code #this},
+ * {@code #someVar}, etc.
  *
  * @author Andy Clement
  * @author Sam Brannen
@@ -37,10 +38,11 @@ import org.springframework.lang.Nullable;
  */
 public class VariableReference extends SpelNodeImpl {
 
-	// Well known variables:
-	private static final String THIS = "this";  // currently active context object
+	/** Currently active context object. */
+	private static final String THIS = "this";
 
-	private static final String ROOT = "root";  // root context object
+	/** Root context object. */
+	private static final String ROOT = "root";
 
 
 	private final String name;
@@ -54,41 +56,56 @@ public class VariableReference extends SpelNodeImpl {
 
 	@Override
 	public ValueRef getValueRef(ExpressionState state) throws SpelEvaluationException {
-		if (this.name.equals(THIS)) {
+		if (THIS.equals(this.name)) {
 			return new ValueRef.TypedValueHolderValueRef(state.getActiveContextObject(), this);
 		}
-		if (this.name.equals(ROOT)) {
+		if (ROOT.equals(this.name)) {
 			return new ValueRef.TypedValueHolderValueRef(state.getRootContextObject(), this);
 		}
 		TypedValue result = state.lookupVariable(this.name);
-		// a null value will mean either the value was null or the variable was not found
+		// A null value in the returned VariableRef will mean either the value was
+		// null or the variable was not found.
 		return new VariableRef(this.name, result, state.getEvaluationContext());
 	}
 
 	@Override
 	public TypedValue getValueInternal(ExpressionState state) throws SpelEvaluationException {
-		if (this.name.equals(THIS)) {
+		if (THIS.equals(this.name)) {
 			return state.getActiveContextObject();
 		}
-		if (this.name.equals(ROOT)) {
+		if (ROOT.equals(this.name)) {
 			TypedValue result = state.getRootContextObject();
 			this.exitTypeDescriptor = CodeFlow.toDescriptorFromObject(result.getValue());
 			return result;
 		}
+
 		TypedValue result = state.lookupVariable(this.name);
-		Object value = result.getValue();
+		setExitTypeDescriptor(result.getValue());
+
+		// A null value in the returned TypedValue will mean either the value was
+		// null or the variable was not found.
+		return result;
+	}
+
+	/**
+	 * Set the exit type descriptor for the supplied value.
+	 * <p>If the value is {@code null}, we set the exit type descriptor to
+	 * {@link Object}.
+	 * <p>If the value's type is not public, {@link #generateCode} would insert
+	 * a checkcast to the non-public type in the generated byte code which would
+	 * result in an {@link IllegalAccessError} when the compiled byte code is
+	 * invoked. Thus, as a preventative measure, we set the exit type descriptor
+	 * to {@code Object} in such cases. If resorting to {@code Object} is not
+	 * sufficient, we could consider traversing the hierarchy to find the first
+	 * public type.
+	 */
+	private void setExitTypeDescriptor(@Nullable Object value) {
 		if (value == null || !Modifier.isPublic(value.getClass().getModifiers())) {
-			// If the type is not public then when generateCode produces a checkcast to it
-			// then an IllegalAccessError will occur.
-			// If resorting to Object isn't sufficient, the hierarchy could be traversed for
-			// the first public type.
 			this.exitTypeDescriptor = "Ljava/lang/Object";
 		}
 		else {
 			this.exitTypeDescriptor = CodeFlow.toDescriptorFromObject(value);
 		}
-		// a null value will mean either the value was null or the variable was not found
-		return result;
 	}
 
 	@Override
@@ -105,7 +122,7 @@ public class VariableReference extends SpelNodeImpl {
 
 	@Override
 	public boolean isWritable(ExpressionState expressionState) throws SpelEvaluationException {
-		return !(this.name.equals(THIS) || this.name.equals(ROOT));
+		return !(THIS.equals(this.name) || ROOT.equals(this.name));
 	}
 
 	@Override
@@ -115,13 +132,14 @@ public class VariableReference extends SpelNodeImpl {
 
 	@Override
 	public void generateCode(MethodVisitor mv, CodeFlow cf) {
-		if (this.name.equals(ROOT)) {
-			mv.visitVarInsn(ALOAD,1);
+		if (ROOT.equals(this.name)) {
+			mv.visitVarInsn(ALOAD, 1);
 		}
 		else {
 			mv.visitVarInsn(ALOAD, 2);
 			mv.visitLdcInsn(this.name);
-			mv.visitMethodInsn(INVOKEINTERFACE, "org/springframework/expression/EvaluationContext", "lookupVariable", "(Ljava/lang/String;)Ljava/lang/Object;",true);
+			mv.visitMethodInsn(INVOKEINTERFACE, "org/springframework/expression/EvaluationContext",
+					"lookupVariable", "(Ljava/lang/String;)Ljava/lang/Object;", true);
 		}
 		CodeFlow.insertCheckCast(mv, this.exitTypeDescriptor);
 		cf.pushDescriptor(this.exitTypeDescriptor);