Revise SpEL inline collection caching improvements

This commit revises the contribution for gh-25921 in the following ways.

- Use instanceof pattern matching

- Use List.of() and Map.of()

- Add missing @⁠since tags

- Polish Javadoc

- Rename isNegativeNumber() to isNegativeNumberLiteral()

- Restructure InlineCollectionTests using @⁠Nested, etc.

- Fix testListWithVariableNotCached() test: it previously set a SpEL
  "variable" but tested a "property" in the root context object, which
  effectively did not test anything.

- Introduce additional tests: listWithPropertyAccessIsNotCached(),
  mapWithVariableIsNotCached(), and mapWithPropertyAccessIsNotCached().

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

@@ -68,7 +68,7 @@ public class InlineList extends SpelNodeImpl {
 						return null;
 					}
 				}
-				else if (!(child instanceof OpMinus) || !((OpMinus) child).isNegativeNumber()) {
+				else if (!(child instanceof OpMinus opMinus) || !opMinus.isNegativeNumberLiteral()) {
 					return null;
 				}
 			}

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

@@ -71,7 +71,7 @@ public class InlineMap extends SpelNodeImpl {
 					}
 				}
 				else if (!(c % 2 == 0 && child instanceof PropertyOrFieldReference)) {
-					if (!(child instanceof OpMinus) || !((OpMinus) child).isNegativeNumber()) {
+					if (!(child instanceof OpMinus opMinus) || !opMinus.isNegativeNumberLiteral()) {
 						return null;
 					}
 				}

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

@@ -53,6 +53,18 @@ public abstract class Literal extends SpelNodeImpl {
 		return getLiteralValue();
 	}
 
+	/**
+	 * Determine if this literal represents a number.
+	 * @return {@code true} if this literal represents a number
+	 * @since 6.1
+	 */
+	public boolean isNumberLiteral() {
+		return (this instanceof IntLiteral ||
+				this instanceof LongLiteral ||
+				this instanceof FloatLiteral ||
+				this instanceof RealLiteral);
+	}
+
 	@Override
 	public String toString() {
 		return String.valueOf(getLiteralValue().getValue());
@@ -111,15 +123,4 @@ public abstract class Literal extends SpelNodeImpl {
 		}
 	}
 
-	/**
-	 * Check whether this literal is a number.
-	 * @return true if this is a number
-	 */
-	public boolean isNumberLiteral() {
-		return this instanceof IntLiteral ||
-				this instanceof LongLiteral ||
-				this instanceof FloatLiteral ||
-				this instanceof RealLiteral;
-	}
-
 }

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

@@ -54,6 +54,17 @@ public class OpMinus extends Operator {
 	}
 
 
+	/**
+	 * Determine if this operator is a unary minus and its child is a
+	 * {@linkplain Literal#isNumberLiteral() number literal}.
+	 * @return {@code true} if it is a negative number literal
+	 * @since 6.1
+	 */
+	public boolean isNegativeNumberLiteral() {
+		return (this.children.length == 1 && this.children[0] instanceof Literal literal &&
+				literal.isNumberLiteral());
+	}
+
 	@Override
 	public TypedValue getValueInternal(ExpressionState state) throws EvaluationException {
 		SpelNodeImpl leftOp = getLeftOperand();
@@ -206,15 +217,4 @@ public class OpMinus extends Operator {
 		cf.pushDescriptor(this.exitTypeDescriptor);
 	}
 
-	/**
-	 * Check whether this operator is an unary minus and it's child is a number.
-	 * @return true if it is a negative number
-	 */
-	public boolean isNegativeNumber() {
-		if (children.length == 1 && children[0] instanceof Literal) {
-			return ((Literal) children[0]).isNumberLiteral();
-		}
-		return false;
-	}
-
 }

spring-expression/src/test/java/org/springframework/expression/spel/ast/InlineCollectionTests.java

@@ -16,10 +16,10 @@
 
 package org.springframework.expression.spel.ast;
 
-import java.util.Arrays;
+import java.util.List;
-import java.util.HashMap;
 import java.util.Map;
 
+import org.junit.jupiter.api.Nested;
 import org.junit.jupiter.api.Test;
 
 import org.springframework.expression.ExpressionParser;
@@ -32,144 +32,188 @@ import org.springframework.expression.spel.support.StandardEvaluationContext;
 import static org.assertj.core.api.Assertions.assertThat;
 
 /**
+ * Tests for {@link InlineList} and {@link InlineMap}.
+ *
  * @author Semyon Danilov
+ * @author Sam Brannen
+ * @since 6.1
  */
-public class InlineCollectionTests {
+class InlineCollectionTests {
-
+
-	@Test
+	private final ExpressionParser parser = new SpelExpressionParser();
-	void testListCached() {
+
-		InlineList list = parseList("{1, -2, 3, 4}");
+
-		assertThat(list.isConstant()).isTrue();
+	@Nested
-		assertThat(list.getConstantValue()).isEqualTo(Arrays.asList(1, -2, 3, 4));
+	class InlineListTests {
-	}
+
-
+		@Test
-	@Test
+		void listIsCached() {
-	void testDynamicListNotCached() {
+			InlineList list = parseList("{1, -2, 3, 4}");
-		InlineList list = parseList("{1, 5-2, 3, 4}");
+			assertThat(list.isConstant()).isTrue();
-		assertThat(list.isConstant()).isFalse();
+			assertThat(list.getConstantValue()).isEqualTo(List.of(1, -2, 3, 4));
-		assertThat(list.getValue(null)).isEqualTo(Arrays.asList(1, 3, 3, 4));
+		}
-	}
+
-
+		@Test
-	@Test
+		void dynamicListIsNotCached() {
-	void testListWithVariableNotCached() {
+			InlineList list = parseList("{1, (5 - 3), 3, 4}");
-		InlineList list = parseList("{1, -a, 3, 4}");
+			assertThat(list.isConstant()).isFalse();
-		assertThat(list.isConstant()).isFalse();
+			assertThat(list.getValue(null)).isEqualTo(List.of(1, 2, 3, 4));
-		final StandardEvaluationContext standardEvaluationContext = new StandardEvaluationContext(new AHolder());
+		}
-		standardEvaluationContext.setVariable("a", 2);
+
-		assertThat(list.getValue(new ExpressionState(standardEvaluationContext))).isEqualTo(Arrays.asList(1, -2, 3, 4));
+		@Test
-	}
+		void listWithVariableIsNotCached() {
-
+			StandardEvaluationContext evaluationContext = new StandardEvaluationContext();
-	@Test
+			ExpressionState expressionState = new ExpressionState(evaluationContext);
-	void testListCanBeCompiled() {
+
-		SpelExpression listExpression = parseExpression("{1, -2, 3, 4}");
+			InlineList list = parseList("{1, -#num, 3, 4}");
-		assertThat(((SpelNodeImpl) listExpression.getAST()).isCompilable()).isTrue();
+			assertThat(list.isConstant()).isFalse();
-		assertThat(SpelCompiler.compile(listExpression)).isTrue();
+
-	}
+			evaluationContext.setVariable("num", 2);
-
+			assertThat(list.getValue(expressionState)).isEqualTo(List.of(1, -2, 3, 4));
-	@Test
+		}
-	void testDynamicListCantBeCompiled() {
+
-		SpelExpression listExpression = parseExpression("{1, 5-2, 3, 4}");
+		@Test
-		assertThat(((SpelNodeImpl) listExpression.getAST()).isCompilable()).isFalse();
+		void listWithPropertyAccessIsNotCached() {
-		assertThat(SpelCompiler.compile(listExpression)).isFalse();
+			StandardEvaluationContext evaluationContext = new StandardEvaluationContext(new NumberHolder());
-	}
+			ExpressionState expressionState = new ExpressionState(evaluationContext);
-
+
-	@Test
+			InlineList list = parseList("{1, -num, 3, 4}");
-	void testMapCached() {
+			assertThat(list.isConstant()).isFalse();
-		InlineMap map = parseMap("{1 : 2, 3 : 4}");
+			assertThat(list.getValue(expressionState)).isEqualTo(List.of(1, -99, 3, 4));
-		assertThat(map.isConstant()).isTrue();
+
-		final Map<Integer, Integer> expected = new HashMap<>();
+			parser.parseExpression("num = 2").getValue(evaluationContext);
-		expected.put(1, 2);
+			assertThat(list.getValue(expressionState)).isEqualTo(List.of(1, -2, 3, 4));
-		expected.put(3, 4);
+		}
-		assertThat(map.getValue(null)).isEqualTo(expected);
+
-	}
+		@Test
-
+		void listCanBeCompiled() {
-	@Test
+			SpelExpression listExpression = parseExpression("{1, -2, 3, 4}");
-	void testMapWithNegativeKeyCached() {
+			assertThat(((SpelNodeImpl) listExpression.getAST()).isCompilable()).isTrue();
-		InlineMap map = parseMap("{-1 : 2, -3 : 4}");
+			assertThat(SpelCompiler.compile(listExpression)).isTrue();
-		assertThat(map.isConstant()).isTrue();
+		}
-		final Map<Integer, Integer> expected = new HashMap<>();
+
-		expected.put(-1, 2);
+		@Test
-		expected.put(-3, 4);
+		void dynamicListCannotBeCompiled() {
-		assertThat(map.getValue(null)).isEqualTo(expected);
+			SpelExpression listExpression = parseExpression("{1, (5 - 3), 3, 4}");
-	}
+			assertThat(((SpelNodeImpl) listExpression.getAST()).isCompilable()).isFalse();
-
+			assertThat(SpelCompiler.compile(listExpression)).isFalse();
-	@Test
+		}
-	void testMapWithNegativeValueCached() {
+
-		InlineMap map = parseMap("{1 : -2, 3 : -4}");
+		private InlineList parseList(String s) {
-		assertThat(map.isConstant()).isTrue();
+			SpelExpression expression = parseExpression(s);
-		final Map<Integer, Integer> expected = new HashMap<>();
+			return (InlineList) expression.getAST();
-		expected.put(1, -2);
+		}
-		expected.put(3, -4);
+
-		assertThat(map.getValue(null)).isEqualTo(expected);
+	}
-	}
+
-
+	@Nested
-	@Test
+	class InlineMapTests {
-	void testMapWithNegativeLongTypesCached() {
+
-		InlineMap map = parseMap("{1L : -2L, 3L : -4L}");
+		@Test
-		assertThat(map.isConstant()).isTrue();
+		void mapIsCached() {
-		final Map<Long, Long> expected = new HashMap<>();
+			InlineMap map = parseMap("{1 : 2, 3 : 4}");
-		expected.put(1L, -2L);
+			assertThat(map.isConstant()).isTrue();
-		expected.put(3L, -4L);
+			Map<Integer, Integer> expected = Map.of(1, 2, 3, 4);
-		assertThat(map.getValue(null)).isEqualTo(expected);
+			assertThat(map.getValue(null)).isEqualTo(expected);
-	}
+		}
-
+
-	@Test
+		@Test
-	void testMapWithNegativeFloatTypesCached() {
+		void dynamicMapIsNotCached() {
-		InlineMap map = parseMap("{-1.0f : -2.0f, -3.0f : -4.0f}");
+			InlineMap map = parseMap("{-1 : 2, (-2 - 1) : -4}");
-		assertThat(map.isConstant()).isTrue();
+			assertThat(map.isConstant()).isFalse();
-		final Map<Float, Float> expected = new HashMap<>();
+			Map<Integer, Integer> expected = Map.of(-1, 2, -3, -4);
-		expected.put(-1.0f, -2.0f);
+			assertThat(map.getValue(null)).isEqualTo(expected);
-		expected.put(-3.0f, -4.0f);
+		}
-		assertThat(map.getValue(null)).isEqualTo(expected);
+
-	}
+		@Test
-
+		void mapWithVariableIsNotCached() {
-	@Test
+			StandardEvaluationContext evaluationContext = new StandardEvaluationContext();
-	void testMapWithNegativeRealTypesCached() {
+			ExpressionState expressionState = new ExpressionState(evaluationContext);
-		InlineMap map = parseMap("{-1.0 : -2.0, -3.0 : -4.0}");
+
-		assertThat(map.isConstant()).isTrue();
+			InlineMap map = parseMap("{-1 : 2, -3 : -#num}");
-		final Map<Double, Double> expected = new HashMap<>();
+			assertThat(map.isConstant()).isFalse();
-		expected.put(-1.0, -2.0);
+
-		expected.put(-3.0, -4.0);
+			evaluationContext.setVariable("num", 4);
-		assertThat(map.getValue(null)).isEqualTo(expected);
+			assertThat(map.getValue(expressionState)).isEqualTo(Map.of(-1, 2, -3, -4));
-	}
+		}
-
+
-	@Test
+		@Test
-	void testMapWithNegativeKeyAndValueCached() {
+		void mapWithPropertyAccessIsNotCached() {
-		InlineMap map = parseMap("{-1 : -2, -3 : -4}");
+			StandardEvaluationContext evaluationContext = new StandardEvaluationContext(new NumberHolder());
-		assertThat(map.isConstant()).isTrue();
+			ExpressionState expressionState = new ExpressionState(evaluationContext);
-		final Map<Integer, Integer> expected = new HashMap<>();
+
-		expected.put(-1, -2);
+			InlineMap map = parseMap("{-1 : 2, -3 : -num}");
-		expected.put(-3, -4);
+			assertThat(map.isConstant()).isFalse();
-		assertThat(map.getValue(null)).isEqualTo(expected);
+			assertThat(map.getValue(expressionState)).isEqualTo(Map.of(-1, 2, -3, -99));
-	}
+
-
+			parser.parseExpression("num = 4").getValue(evaluationContext);
-	@Test
+			assertThat(map.getValue(expressionState)).isEqualTo(Map.of(-1, 2, -3, -4));
-	void testMapWithDynamicNotCached() {
+		}
-		InlineMap map = parseMap("{-1 : 2, -3+1 : -4}");
+
-		assertThat(map.isConstant()).isFalse();
+		@Test
-		final Map<Integer, Integer> expected = new HashMap<>();
+		void mapWithNegativeKeysIsCached() {
-		expected.put(-1, 2);
+			InlineMap map = parseMap("{-1 : 2, -3 : 4}");
-		expected.put(-2, -4);
+			assertThat(map.isConstant()).isTrue();
-		assertThat(map.getValue(null)).isEqualTo(expected);
+			Map<Integer, Integer> expected = Map.of(-1, 2, -3, 4);
-	}
+			assertThat(map.getValue(null)).isEqualTo(expected);
-
+		}
-	private InlineMap parseMap(String s) {
+
-		SpelExpression expression = parseExpression(s);
+		@Test
-		return (InlineMap) expression.getAST();
+		void mapWithNegativeValuesIsCached() {
-	}
+			InlineMap map = parseMap("{1 : -2, 3 : -4}");
-
+			assertThat(map.isConstant()).isTrue();
-	private InlineList parseList(String s) {
+			Map<Integer, Integer> expected = Map.of(1, -2, 3, -4);
-		SpelExpression expression = parseExpression(s);
+			assertThat(map.getValue(null)).isEqualTo(expected);
-		return (InlineList) expression.getAST();
+		}
-	}
+
-
+		@Test
-	private SpelExpression parseExpression(final String s) {
+		void mapWithNegativeLongValuesIsCached() {
-		ExpressionParser parser = new SpelExpressionParser();
+			InlineMap map = parseMap("{1L : -2L, 3L : -4L}");
+			assertThat(map.isConstant()).isTrue();
+			Map<Long, Long> expected = Map.of(1L, -2L, 3L, -4L);
+			assertThat(map.getValue(null)).isEqualTo(expected);
+		}
+
+		@Test
+		void mapWithNegativeFloatValuesIsCached() {
+			InlineMap map = parseMap("{-1.0f : -2.0f, -3.0f : -4.0f}");
+			assertThat(map.isConstant()).isTrue();
+			Map<Float, Float> expected = Map.of(-1.0f, -2.0f, -3.0f, -4.0f);
+			assertThat(map.getValue(null)).isEqualTo(expected);
+		}
+
+		@Test
+		void mapWithNegativeRealValuesIsCached() {
+			InlineMap map = parseMap("{-1.0 : -2.0, -3.0 : -4.0}");
+			assertThat(map.isConstant()).isTrue();
+			Map<Double, Double> expected = Map.of(-1.0, -2.0, -3.0, -4.0);
+			assertThat(map.getValue(null)).isEqualTo(expected);
+		}
+
+		@Test
+		void mapWithNegativeKeysAndNegativeValuesIsCached() {
+			InlineMap map = parseMap("{-1 : -2, -3 : -4}");
+			assertThat(map.isConstant()).isTrue();
+			Map<Integer, Integer> expected = Map.of(-1, -2, -3, -4);
+			assertThat(map.getValue(null)).isEqualTo(expected);
+		}
+
+		private InlineMap parseMap(String s) {
+			SpelExpression expression = parseExpression(s);
+			return (InlineMap) expression.getAST();
+		}
+
+	}
+
+
+	private SpelExpression parseExpression(String s) {
 		return (SpelExpression) parser.parseExpression(s);
 	}
 
-	private static class AHolder {
+
-		public int a = 2;
+	private static class NumberHolder {
+		@SuppressWarnings("unused")
+		public int num = 99;
 	}
 
 }