Clean up TODOs in SpEL

Author: sbrannen

Diff for: spring-expression/src/main/java/org/springframework/expression/spel/ast/ConstructorReference.java

@@ -76,7 +76,6 @@ public class ConstructorReference extends SpelNodeImpl {
 	@Nullable
 	private final SpelNodeImpl[] dimensions;
 
-	// TODO is this caching safe - passing the expression around will mean this executor is also being passed around
 	/** The cached executor that may be reused on subsequent evaluations. */
 	@Nullable
 	private volatile ConstructorExecutor cachedExecutor;

Diff for: spring-expression/src/main/java/org/springframework/expression/spel/ast/Projection.java

@@ -82,7 +82,7 @@ protected ValueRef getValueRef(ExpressionState state) throws EvaluationException
 					state.exitScope();
 				}
 			}
-			return new ValueRef.TypedValueHolderValueRef(new TypedValue(result), this);  // TODO unable to build correct type descriptor
+			return new ValueRef.TypedValueHolderValueRef(new TypedValue(result), this);
 		}
 
 		boolean operandIsArray = ObjectUtils.isArray(operand);

Diff for: spring-expression/src/main/java/org/springframework/expression/spel/ast/Selection.java

@@ -90,7 +90,6 @@ protected ValueRef getValueRef(ExpressionState state) throws EvaluationException
 		SpelNodeImpl selectionCriteria = this.children[0];
 
 		if (operand instanceof Map<?, ?> mapdata) {
-			// TODO don't lose generic info for the new map
 			Map<Object, Object> result = new HashMap<>();
 			Object lastKey = null;
 

Diff for: spring-expression/src/main/java/org/springframework/expression/spel/ast/TypeReference.java

@@ -54,7 +54,7 @@ public TypeReference(int startPos, int endPos, SpelNodeImpl qualifiedId, int dim
 
 	@Override
 	public TypedValue getValueInternal(ExpressionState state) throws EvaluationException {
-		// TODO possible optimization here if we cache the discovered type reference, but can we do that?
+		// TODO Possible optimization: if we cache the discovered type reference, but can we do that?
 		String typeName = (String) this.children[0].getValueInternal(state).getValue();
 		Assert.state(typeName != null, "No type name");
 		if (!typeName.contains(".") && Character.isLowerCase(typeName.charAt(0))) {
@@ -99,7 +99,7 @@ public boolean isCompilable() {
 
 	@Override
 	public void generateCode(MethodVisitor mv, CodeFlow cf) {
-		// TODO Future optimization - if followed by a static method call, skip generating code here
+		// TODO Future optimization: if followed by a static method call, skip generating code here.
 		Assert.state(this.type != null, "No type available");
 		if (this.type.isPrimitive()) {
 			if (this.type == boolean.class) {

Diff for: spring-expression/src/main/java/org/springframework/expression/spel/standard/InternalSpelExpressionParser.java

@@ -733,7 +733,6 @@ else if (t.kind == TokenKind.SELECT_LAST) {
 
 	/**
 	 * Eat an identifier, possibly qualified (meaning that it is dotted).
-	 * TODO AndyC Could create complete identifiers (a.b.c) here rather than a sequence of them? (a, b, c)
 	 */
 	private SpelNodeImpl eatPossiblyQualifiedId() {
 		Deque<SpelNodeImpl> qualifiedIdPieces = new ArrayDeque<>();
@@ -783,7 +782,6 @@ private boolean maybeEatMethodOrProperty(boolean nullSafeNavigation) {
 			// method reference
 			push(new MethodReference(nullSafeNavigation, methodOrPropertyName.stringValue(),
 					methodOrPropertyName.startPos, methodOrPropertyName.endPos, args));
-			// TODO what is the end position for a method reference? the name or the last arg?
 			return true;
 		}
 		return false;
@@ -826,7 +824,6 @@ private boolean maybeEatConstructorReference() {
 			else {
 				// regular constructor invocation
 				eatConstructorArgs(nodes);
-				// TODO correct end position?
 				push(new ConstructorReference(newToken.startPos, newToken.endPos, nodes.toArray(new SpelNodeImpl[0])));
 			}
 			return true;

Diff for: spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectionHelper.java

@@ -233,7 +233,6 @@ else if (typeConverter.canConvert(suppliedArg, TypeDescriptor.valueOf(varargsPar
 		return (match != null ? new ArgumentsMatchInfo(match) : null);
 	}
 
-	// TODO could do with more refactoring around argument handling and varargs
 	/**
 	 * Convert the supplied set of arguments into the parameter types specified
 	 * by the supplied {@link Method}.

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

@@ -64,7 +64,7 @@
 public class SpelCompilationCoverageTests extends AbstractExpressionTests {
 
 	/*
-	 * Further TODOs for compilation:
+	 * TODO Potential optimizations for SpEL compilation:
 	 *
 	 * - OpMinus with a single literal operand could be treated as a negative literal. Will save a
 	 *   pointless loading of 0 and then a subtract instruction in code gen.
@@ -1205,12 +1205,12 @@ void functionReferenceVarargs_SPR12359() throws Exception {
 		assertCanCompile(expression);
 		assertThat(expression.getValue(context, new SomeCompareMethod2()).toString()).isEqualTo("xyz");
 
-		// TODO fails due to conversionservice handling of String[] to Object...
-		//	expression = parser.parseExpression("#append2(#stringArray)");
-		//	assertEquals("xyz", expression.getValue(context).toString());
-		//	assertTrue(((SpelNodeImpl)((SpelExpression) expression).getAST()).isCompilable());
-		//	assertCanCompile(expression);
-		//	assertEquals("xyz", expression.getValue(context).toString());
+		// TODO Determine why the String[] is passed as the first element of the Object... varargs array instead of the entire varargs array.
+		// expression = parser.parseExpression("#append2(#stringArray)");
+		// assertThat(expression.getValue(context)).hasToString("xyz");
+		// assertThat(((SpelNodeImpl) ((SpelExpression) expression).getAST()).isCompilable()).isTrue();
+		// assertCanCompile(expression);
+		// assertThat(expression.getValue(context)).hasToString("xyz");
 
 		expression = parser.parseExpression("#sum(1,2,3)");
 		assertThat(expression.getValue(context)).isEqualTo(6);
@@ -3703,16 +3703,16 @@ void methodReferenceVarargs() {
 		assertThat(tc.s).isEqualTo("aaabbbccc");
 		tc.reset();
 
-		// TODO Fails related to conversion service converting a String[] to satisfy Object...
-//		expression = parser.parseExpression("sixteen(stringArray)");
-//		assertCantCompile(expression);
-//		expression.getValue(tc);
-//		assertEquals("aaabbbccc", tc.s);
-//		assertCanCompile(expression);
-//		tc.reset();
-//		expression.getValue(tc);
-//		assertEquals("aaabbbccc", tc.s);
-//		tc.reset();
+		// TODO Determine why the String[] is passed as the first element of the Object... varargs array instead of the entire varargs array.
+		// expression = parser.parseExpression("sixteen(stringArray)");
+		// assertCantCompile(expression);
+		// expression.getValue(tc);
+		// assertThat(tc.s).isEqualTo("aaabbbccc");
+		// assertCanCompile(expression);
+		// tc.reset();
+		// expression.getValue(tc);
+		// assertThat(tc.s).isEqualTo("aaabbbccc");
+		// tc.reset();
 
 		// varargs int
 		expression = parser.parseExpression("twelve(1,2,3)");