Enhance SpEL compilation to cover additional expression types

This change introduces support for compilation of expressions
involving inline lists, string concatenation and method
invocations where the method being invoked is declared
with a varargs parameter. It also fixes a problem with
compiling existing method invocations where the target
method is on a non public type.

Issue: SPR-12328

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

@@ -19,15 +19,19 @@ package org.springframework.expression.spel;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Method;
 import java.util.ArrayList;
+import java.util.List;
 import java.util.Stack;
 
+import org.springframework.asm.ClassWriter;
 import org.springframework.asm.MethodVisitor;
 import org.springframework.asm.Opcodes;
+import org.springframework.expression.spel.ast.SpelNodeImpl;
 import org.springframework.util.Assert;
 
 /**
- * Records intermediate compilation state as the bytecode is generated for a parsed
- * expression. Also contains bytecode generation helper functions.
+ * Manages the class being generated by the compilation process. It records
+ * intermediate compilation state as the bytecode is generated. It also includes
+ * various bytecode generation helper functions.
  *
  * @author Andy Clement
  * @since 4.1
@@ -42,13 +46,51 @@ public class CodeFlow implements Opcodes {
 	 */
 	private final Stack<ArrayList<String>> compilationScopes;
 
+	/**
+	 * The current class being generated
+	 */
+	private ClassWriter cw;
+
+	/**
+	 * As SpEL ast nodes are called to generate code for the main evaluation method
+	 * they can register to add a field to this class. Any registered FieldAdders
+	 * will be called after the main evaluation function has finished being generated.
+	 */
+	private List<FieldAdder> fieldAdders = null;
+	
+	/**
+	 * As SpEL ast nodes are called to generate code for the main evaluation method
+	 * they can register to add code to a static initializer in the class. Any
+	 * registered ClinitAdders will be called after the main evaluation function
+	 * has finished being generated.
+	 */
+	private List<ClinitAdder> clinitAdders = null;
+	
+	/**
+	 * Name of the class being generated. Typically used when generating code
+	 * that accesses freshly generated fields on the generated type.
+	 */
+	private String clazzName;
+	
+	/**
+	 * When code generation requires holding a value in a class level field, this
+	 * is used to track the next available field id (used as a name suffix).
+	 */
+	private int nextFieldId = 1;
+	
+	/**
+	 * When code generation requires an intermediate variable within a method,
+	 * this method records the next available variable (variable 0 is 'this').
+	 */
+	private int nextFreeVariableId = 1;
 
-	public CodeFlow() {
+	public CodeFlow(String clazzName, ClassWriter cw) {
 		this.compilationScopes = new Stack<ArrayList<String>>();
 		this.compilationScopes.add(new ArrayList<String>());
+		this.cw = cw;
+		this.clazzName = clazzName;
 	}
 
-
 	/**
 	 * Push the byte code to load the target (i.e. what was passed as the first argument
 	 * to CompiledExpression.getValue(target, context))
@@ -106,7 +148,6 @@ public class CodeFlow implements Opcodes {
 		}
 	}
 
-
 	/**
 	 * Insert any necessary cast and value call to convert from a boxed type to a
 	 * primitive value
@@ -612,5 +653,256 @@ public class CodeFlow implements Opcodes {
 		}
 		return descriptors;
 	}
+	
+	/**
+	 * Called after the main expression evaluation method has been generated, this
+	 * method will callback any registered FieldAdders or ClinitAdders to add any
+	 * extra information to the class representing the compiled expression.
+	 */
+	public void finish() {
+		if (fieldAdders != null) {
+			for (FieldAdder fieldAdder: fieldAdders) {
+				fieldAdder.generateField(cw,this);
+			}
+		}
+		if (clinitAdders != null) {
+			MethodVisitor mv = cw.visitMethod(ACC_PUBLIC | ACC_STATIC, "<clinit>", "()V", null, null);
+			mv.visitCode();
+			nextFreeVariableId = 0; // To 0 because there is no 'this' in a clinit
+			for (ClinitAdder clinitAdder: clinitAdders) {
+				clinitAdder.generateCode(mv, this);
+			}
+			mv.visitInsn(RETURN);
+			mv.visitMaxs(0,0); // not supplied due to COMPUTE_MAXS
+			mv.visitEnd();
+		}
+	}
 
+	/**
+	 * Register a FieldAdder which will add a new field to the generated
+	 * class to support the code produced by an ast nodes primary
+	 * generateCode() method.
+	 */
+	public void registerNewField(FieldAdder fieldAdder) {
+		if (fieldAdders == null) {
+			fieldAdders = new ArrayList<FieldAdder>();
+		}
+		fieldAdders.add(fieldAdder);
+	}
+
+	/**
+	 * Register a ClinitAdder which will add code to the static 
+	 * initializer in the generated class to support the code
+	 * produced by an ast nodes primary generateCode() method.
+	 */
+	public void registerNewClinit(ClinitAdder clinitAdder) {
+		if (clinitAdders == null) {
+			clinitAdders = new ArrayList<ClinitAdder>();
+		}
+		clinitAdders.add(clinitAdder);
+	}
+
+	public int nextFieldId() {
+		return nextFieldId++;
+	}
+
+	public int nextFreeVariableId() {
+		return nextFreeVariableId++;
+	}
+
+	public String getClassname() {
+		return clazzName;
+	}
+	
+	public interface FieldAdder {
+		public void generateField(ClassWriter cw, CodeFlow codeflow);
+	}
+	
+	public interface ClinitAdder {
+		public void generateCode(MethodVisitor mv, CodeFlow codeflow);
+	}
+
+	/**
+	 * Create the optimal instruction for loading a number on the stack.
+	 * @param mv where to insert the bytecode
+	 * @param value the value to be loaded
+	 */
+	public static void insertOptimalLoad(MethodVisitor mv, int value) {
+		if (value < 6) {
+			mv.visitInsn(ICONST_0+value);
+		}
+		else if (value < Byte.MAX_VALUE) {
+			mv.visitIntInsn(BIPUSH, value);
+		}
+		else if (value < Short.MAX_VALUE) {
+			mv.visitIntInsn(SIPUSH, value);
+		}
+		else {
+			mv.visitLdcInsn(value);
+		}		
+	}
+
+	/**
+	 * Produce appropriate bytecode to store a stack item in an array. The 
+	 * instruction to use varies depending on whether the type
+	 * is a primitive or reference type.
+	 * @param mv where to insert the bytecode
+	 * @param arrayElementType the type of the array elements
+	 */
+	public static void insertArrayStore(MethodVisitor mv, String arrayElementType) {
+		if (arrayElementType.length()==1) {
+			switch (arrayElementType.charAt(0)) {
+				case 'I': mv.visitInsn(IASTORE); break;
+				case 'J': mv.visitInsn(LASTORE); break;
+				case 'F': mv.visitInsn(FASTORE); break;
+				case 'D': mv.visitInsn(DASTORE); break;
+				case 'B': mv.visitInsn(BASTORE); break;
+				case 'C': mv.visitInsn(CASTORE); break;
+				case 'S': mv.visitInsn(SASTORE); break;
+				case 'Z': mv.visitInsn(BASTORE); break;
+				default:
+					throw new IllegalArgumentException("Unexpected arraytype "+arrayElementType.charAt(0));
+			}
+		}
+		else {
+			mv.visitInsn(AASTORE);
+		}
+	}
+
+	/**
+	 * Determine the appropriate T tag to use for the NEWARRAY bytecode.
+	 * @param arraytype the array primitive component type
+	 * @return the T tag to use for NEWARRAY
+	 */
+	public static int arrayCodeFor(String arraytype) {
+		switch (arraytype.charAt(0)) {
+			case 'I': return T_INT;
+			case 'J': return T_LONG; 
+			case 'F': return T_FLOAT;
+			case 'D': return T_DOUBLE;
+			case 'B': return T_BYTE;
+			case 'C': return T_CHAR;
+			case 'S': return T_SHORT;
+			case 'Z': return T_BOOLEAN;
+			default:
+				throw new IllegalArgumentException("Unexpected arraytype "+arraytype.charAt(0));
+		}
+	}
+
+	/**
+	 * @return true if the supplied array type has a core component reference type
+	 */
+	public static boolean isReferenceTypeArray(String arraytype) {
+		int length = arraytype.length();
+		for (int i=0;i<length;i++) {
+			char ch = arraytype.charAt(i);
+			if (ch == '[') continue;
+			return ch=='L';
+		}
+		return false;
+	}
+	
+	/**
+	 * Produce the correct bytecode to build an array. The opcode to use and the 
+	 * signature to pass along with the opcode can vary depending on the signature
+	 * of the array type.
+	 * @param mv the methodvisitor into which code should be inserted
+	 * @param size the size of the array
+	 * @param arraytype the type of the array
+	 */
+	public static void insertNewArrayCode(MethodVisitor mv, int size, String arraytype) {
+		insertOptimalLoad(mv, size);
+		if (arraytype.length() == 1) {
+			mv.visitIntInsn(NEWARRAY, CodeFlow.arrayCodeFor(arraytype));
+		}
+		else {
+			if (arraytype.charAt(0) == '[') {
+				// Handling the nested array case here. If vararg
+				// is [[I then we want [I and not [I;
+				if (CodeFlow.isReferenceTypeArray(arraytype)) {
+					mv.visitTypeInsn(ANEWARRAY, arraytype+";");
+				} else {
+					mv.visitTypeInsn(ANEWARRAY, arraytype);
+				}
+			}
+			else {
+				mv.visitTypeInsn(ANEWARRAY, arraytype.substring(1));
+			}
+		}
+	}
+	
+	/**
+	 * Generate code that handles building the argument values for the specified method. This method will take account
+	 * of whether the invoked method is a varargs method and if it is then the argument values will be appropriately
+	 * packaged into an array.
+	 * @param mv the method visitor where code should be generated
+	 * @param cf the current codeflow
+	 * @param method the method for which arguments are being setup
+	 * @param arguments the expression nodes for the expression supplied argument values
+	 */
+	public static void generateCodeForArguments(MethodVisitor mv, CodeFlow cf, Method method, SpelNodeImpl[] arguments) {
+		String[] paramDescriptors = CodeFlow.toParamDescriptors(method);
+		if (method.isVarArgs()) {
+			// The final parameter may or may not need packaging into an array, or nothing may
+			// have been passed to satisfy the varargs and so something needs to be built.
+			int p = 0; // Current supplied argument being processed
+			int childcount = arguments.length;
+						
+			// Fulfill all the parameter requirements except the last one
+			for (p = 0; p < paramDescriptors.length-1;p++) {
+				generateCodeForArgument(mv, cf, arguments[p], paramDescriptors[p]);
+			}
+			
+			SpelNodeImpl lastchild = (childcount == 0 ? null : arguments[childcount-1]);			
+			String arraytype = paramDescriptors[paramDescriptors.length-1];
+			// Determine if the final passed argument is already suitably packaged in array
+			// form to be passed to the method
+			if (lastchild != null && lastchild.getExitDescriptor().equals(arraytype)) {
+				generateCodeForArgument(mv, cf, lastchild, paramDescriptors[p]);
+			}
+			else {
+				arraytype = arraytype.substring(1); // trim the leading '[', may leave other '['		
+				// build array big enough to hold remaining arguments
+				CodeFlow.insertNewArrayCode(mv, childcount-p, arraytype);
+				// Package up the remaining arguments into the array
+				int arrayindex = 0;
+				while (p < childcount) {
+					SpelNodeImpl child = arguments[p];
+					mv.visitInsn(DUP);
+					CodeFlow.insertOptimalLoad(mv, arrayindex++);
+					generateCodeForArgument(mv, cf, child, arraytype);
+					CodeFlow.insertArrayStore(mv, arraytype);
+					p++;
+				}
+			}
+		}
+		else {
+			for (int i = 0; i < paramDescriptors.length;i++) {
+				generateCodeForArgument(mv, cf, arguments[i], paramDescriptors[i]);
+			}
+		}
+	}
+
+	/**
+	 * Ask an argument to generate its bytecode and then follow it up
+	 * with any boxing/unboxing/checkcasting to ensure it matches the expected parameter descriptor.
+	 */
+	public static void generateCodeForArgument(MethodVisitor mv, CodeFlow cf, SpelNodeImpl argument, String paramDescriptor) {
+		cf.enterCompilationScope();
+		argument.generateCode(mv, cf);
+		boolean primitiveOnStack = CodeFlow.isPrimitive(cf.lastDescriptor());
+		// Check if need to box it for the method reference?
+		if (primitiveOnStack && paramDescriptor.charAt(0) == 'L') {
+			CodeFlow.insertBoxIfNecessary(mv, cf.lastDescriptor().charAt(0));
+		}
+		else if (paramDescriptor.length() == 1 && !primitiveOnStack) {
+			CodeFlow.insertUnboxInsns(mv, paramDescriptor.charAt(0), cf.lastDescriptor());
+		}
+		else if (!cf.lastDescriptor().equals(paramDescriptor)) {
+			// This would be unnecessary in the case of subtyping (e.g. method takes Number but Integer passed in)
+			CodeFlow.insertCheckCast(mv, paramDescriptor);
+		}
+		cf.exitCompilationScope();
+	}
+	
 }

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

@@ -165,23 +165,9 @@ public class FunctionReference extends SpelNodeImpl {
 	@Override 
 	public void generateCode(MethodVisitor mv,CodeFlow cf) {
 		String methodDeclaringClassSlashedDescriptor = this.method.getDeclaringClass().getName().replace('.', '/');
-		String[] paramDescriptors = CodeFlow.toParamDescriptors(this.method);
-		for (int c = 0; c < this.children.length; c++) {
-			SpelNodeImpl child = this.children[c];
-			cf.enterCompilationScope();
-			child.generateCode(mv, cf);
-			// Check if need to box it for the method reference?
-			if (CodeFlow.isPrimitive(cf.lastDescriptor()) && paramDescriptors[c].charAt(0) == 'L') {
-				CodeFlow.insertBoxIfNecessary(mv, cf.lastDescriptor().charAt(0));
-			}
-			else if (!cf.lastDescriptor().equals(paramDescriptors[c])) {
-				// This would be unnecessary in the case of subtyping (e.g. method takes a Number but passed in is an Integer)
-				CodeFlow.insertCheckCast(mv, paramDescriptors[c]);
-			}
-			cf.exitCompilationScope();
-		}
+		CodeFlow.generateCodeForArguments(mv, cf, method, this.children);
 		mv.visitMethodInsn(INVOKESTATIC, methodDeclaringClassSlashedDescriptor, this.method.getName(),
-				CodeFlow.createSignatureDescriptor(this.method),false);
+				CodeFlow.createSignatureDescriptor(this.method), false);
 		cf.pushDescriptor(this.exitTypeDescriptor);
 	}
 

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

@@ -254,7 +254,6 @@ public class Indexer extends SpelNodeImpl {
 			this.children[0].generateCode(mv, cf);
 			cf.exitCompilationScope();
 			mv.visitMethodInsn(INVOKEINTERFACE, "java/util/List", "get", "(I)Ljava/lang/Object;", true);
-			CodeFlow.insertCheckCast(mv, this.exitTypeDescriptor);
 		}
 		else if (this.indexedType == IndexedType.MAP) {
 			mv.visitTypeInsn(CHECKCAST, "java/util/Map");
@@ -271,7 +270,6 @@ public class Indexer extends SpelNodeImpl {
 				cf.exitCompilationScope();
 			}
 			mv.visitMethodInsn(INVOKEINTERFACE, "java/util/Map", "get", "(Ljava/lang/Object;)Ljava/lang/Object;", true);
-			CodeFlow.insertCheckCast(mv, this.exitTypeDescriptor);
 		} 
 		else if (this.indexedType == IndexedType.OBJECT) {
 			ReflectivePropertyAccessor.OptimalPropertyAccessor accessor =
@@ -493,7 +491,7 @@ public class Indexer extends SpelNodeImpl {
 		@Override
 		public TypedValue getValue() {
 			Object value = this.map.get(this.key);
-			exitTypeDescriptor = CodeFlow.toDescriptorFromObject(value);
+			exitTypeDescriptor = CodeFlow.toDescriptor(Object.class);
 			return new TypedValue(value, this.mapEntryDescriptor.getMapValueTypeDescriptor(value));
 		}
 
@@ -645,7 +643,7 @@ public class Indexer extends SpelNodeImpl {
 			growCollectionIfNecessary();
 			if (this.collection instanceof List) {
 				Object o = ((List) this.collection).get(this.index);
-				exitTypeDescriptor = CodeFlow.toDescriptorFromObject(o);
+				exitTypeDescriptor = CodeFlow.toDescriptor(Object.class);
 				return new TypedValue(o, this.collectionEntryDescriptor.elementTypeDescriptor(o));
 			}
 			int pos = 0;

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

@@ -17,11 +17,14 @@
 package org.springframework.expression.spel.ast;
 
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.List;
+import java.util.Collections;
 
+import org.springframework.asm.ClassWriter;
+import org.springframework.asm.MethodVisitor;
 import org.springframework.expression.EvaluationException;
 import org.springframework.expression.TypedValue;
+import org.springframework.expression.spel.CodeFlow;
 import org.springframework.expression.spel.ExpressionState;
 import org.springframework.expression.spel.SpelNode;
 
@@ -121,5 +124,63 @@ public class InlineList extends SpelNodeImpl {
 	public List<Object> getConstantValue() {
 		return (List<Object>) this.constant.getValue();
 	}
+	
+	@Override
+	public boolean isCompilable() {
+		return isConstant();
+	}
+	
+	@Override
+	public void generateCode(MethodVisitor mv, CodeFlow codeflow) {
+		final String constantFieldName = "inlineList$"+codeflow.nextFieldId();
+		final String clazzname = codeflow.getClassname();
+
+		codeflow.registerNewField(new CodeFlow.FieldAdder() {
+			public void generateField(ClassWriter cw, CodeFlow codeflow) {
+				cw.visitField(ACC_PRIVATE|ACC_STATIC|ACC_FINAL, constantFieldName, "Ljava/util/List;", null, null);
+			}
+		});
+		
+		codeflow.registerNewClinit(new CodeFlow.ClinitAdder() {
+			public void generateCode(MethodVisitor mv, CodeFlow codeflow) {
+				generateClinitCode(clazzname,constantFieldName, mv,codeflow,false);
+			}
+		});
+		
+		mv.visitFieldInsn(GETSTATIC, clazzname, constantFieldName, "Ljava/util/List;");
+		codeflow.pushDescriptor("Ljava/util/List");
+	}
+	
+	void generateClinitCode(String clazzname, String constantFieldName, MethodVisitor mv, CodeFlow codeflow, boolean nested) {
+		mv.visitTypeInsn(NEW, "java/util/ArrayList");
+		mv.visitInsn(DUP);
+		mv.visitMethodInsn(INVOKESPECIAL, "java/util/ArrayList", "<init>", "()V", false);
+		if (!nested) {
+			mv.visitFieldInsn(PUTSTATIC, clazzname, constantFieldName, "Ljava/util/List;");
+		}
+		int childcount = getChildCount();		
+		for (int c=0; c < childcount; c++) {
+			if (!nested) {
+				mv.visitFieldInsn(GETSTATIC, clazzname, constantFieldName, "Ljava/util/List;");
+			}
+			else {
+				mv.visitInsn(DUP);
+			}
+			// The children might be further lists if they are not constants. In this
+			// situation do not call back into generateCode() because it will register another clinit adder.
+			// Instead, directly build the list here:
+			if (children[c] instanceof InlineList) {
+				((InlineList)children[c]).generateClinitCode(clazzname, constantFieldName, mv, codeflow, true);
+			}
+			else {
+				children[c].generateCode(mv, codeflow);
+				if (CodeFlow.isPrimitive(codeflow.lastDescriptor())) {
+					CodeFlow.insertBoxIfNecessary(mv, codeflow.lastDescriptor().charAt(0));
+				}
+			}
+			mv.visitMethodInsn(INVOKEINTERFACE, "java/util/List", "add", "(Ljava/lang/Object;)Z", true);
+			mv.visitInsn(POP);
+		}
+	}
 
 }

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

@@ -268,20 +268,18 @@ public class MethodReference extends SpelNodeImpl {
 		}
 
 		ReflectiveMethodExecutor executor = (ReflectiveMethodExecutor) executorToCheck.get();
-		Method method = executor.getMethod();
-		if (!Modifier.isPublic(method.getModifiers()) || !Modifier.isPublic(method.getDeclaringClass().getModifiers())) {
-			return false;
-		}
-		if (method.isVarArgs()) {
+		if (executor.didArgumentConversionOccur()) {
 			return false;
 		}
-		if (executor.didArgumentConversionOccur()) {
+		Method method = executor.getMethod();
+		Class<?> clazz = method.getDeclaringClass();
+		if (!Modifier.isPublic(clazz.getModifiers()) && executor.getPublicDeclaringClass() == null) {
 			return false;
 		}
 
 		return true;
 	}
-
+	
 	@Override
 	public void generateCode(MethodVisitor mv, CodeFlow cf) {
 		CachedMethodExecutor executorToCheck = this.cachedExecutor;
@@ -289,7 +287,8 @@ public class MethodReference extends SpelNodeImpl {
 			throw new IllegalStateException("No applicable cached executor found: " + executorToCheck);
 		}
 
-		Method method = ((ReflectiveMethodExecutor) executorToCheck.get()).getMethod();
+		ReflectiveMethodExecutor methodExecutor = (ReflectiveMethodExecutor)executorToCheck.get();
+		Method method = methodExecutor.getMethod();
 		boolean isStaticMethod = Modifier.isStatic(method.getModifiers());
 		String descriptor = cf.lastDescriptor();
 
@@ -298,27 +297,19 @@ public class MethodReference extends SpelNodeImpl {
 		}
 
 		boolean itf = method.getDeclaringClass().isInterface();
-		String methodDeclaringClassSlashedDescriptor = method.getDeclaringClass().getName().replace('.', '/');
-		if (!isStaticMethod) {
-			if (descriptor == null || !descriptor.equals(methodDeclaringClassSlashedDescriptor)) {
-				mv.visitTypeInsn(CHECKCAST, methodDeclaringClassSlashedDescriptor);
-			}
+		String methodDeclaringClassSlashedDescriptor = null;
+		if (Modifier.isPublic(method.getDeclaringClass().getModifiers())) {
+			methodDeclaringClassSlashedDescriptor = method.getDeclaringClass().getName().replace('.', '/');
 		}
-		String[] paramDescriptors = CodeFlow.toParamDescriptors(method);
-		for (int i = 0; i < this.children.length;i++) {
-			SpelNodeImpl child = this.children[i];
-			cf.enterCompilationScope();
-			child.generateCode(mv, cf);
-			// Check if need to box it for the method reference?
-			if (CodeFlow.isPrimitive(cf.lastDescriptor()) && paramDescriptors[i].charAt(0) == 'L') {
-				CodeFlow.insertBoxIfNecessary(mv, cf.lastDescriptor().charAt(0));
-			}
-			else if (!cf.lastDescriptor().equals(paramDescriptors[i])) {
-				// This would be unnecessary in the case of subtyping (e.g. method takes Number but Integer passed in)
-				CodeFlow.insertCheckCast(mv, paramDescriptors[i]);
+		else {
+			methodDeclaringClassSlashedDescriptor = methodExecutor.getPublicDeclaringClass().getName().replace('.', '/');			
+		}
+		if (!isStaticMethod) {
+			if (descriptor == null || !descriptor.substring(1).equals(methodDeclaringClassSlashedDescriptor)) {
+				CodeFlow.insertCheckCast(mv, "L"+ methodDeclaringClassSlashedDescriptor);
 			}
-			cf.exitCompilationScope();
 		}
+		CodeFlow.generateCodeForArguments(mv, cf, method, children);		
 		mv.visitMethodInsn(isStaticMethod ? INVOKESTATIC : INVOKEVIRTUAL,
 				methodDeclaringClassSlashedDescriptor, method.getName(), CodeFlow.createSignatureDescriptor(method), itf);
 		cf.pushDescriptor(this.exitTypeDescriptor);

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

@@ -130,6 +130,7 @@ public class OpPlus extends Operator {
 		}
 
 		if (leftOperand instanceof String && rightOperand instanceof String) {
+			this.exitTypeDescriptor = "Ljava/lang/String";
 			return new TypedValue((String) leftOperand + rightOperand);
 		}
 
@@ -192,37 +193,65 @@ public class OpPlus extends Operator {
 		return (this.exitTypeDescriptor != null);
 	}
 
-	@Override
-	public void generateCode(MethodVisitor mv, CodeFlow cf) {
-		getLeftOperand().generateCode(mv, cf);
-		String leftDesc = getLeftOperand().exitTypeDescriptor;
-		if (!CodeFlow.isPrimitive(leftDesc)) {
-			CodeFlow.insertUnboxInsns(mv, this.exitTypeDescriptor.charAt(0), leftDesc);
+	/**
+	 * Walk through a possible tree of nodes that combine strings and append
+	 * them all to the same (on stack) StringBuilder.
+	 */
+	private void walk(MethodVisitor mv, CodeFlow cf, SpelNodeImpl operand) {
+		if (operand instanceof OpPlus) {
+			OpPlus plus = (OpPlus)operand;
+			walk(mv,cf,plus.getLeftOperand());
+			walk(mv,cf,plus.getRightOperand());
 		}
-		if (this.children.length > 1) {
+		else {
 			cf.enterCompilationScope();
-			getRightOperand().generateCode(mv, cf);
-			String rightDesc = getRightOperand().exitTypeDescriptor;
+			operand.generateCode(mv,cf);
 			cf.exitCompilationScope();
-			if (!CodeFlow.isPrimitive(rightDesc)) {
-				CodeFlow.insertUnboxInsns(mv, this.exitTypeDescriptor.charAt(0), rightDesc);
+			mv.visitMethodInsn(INVOKEVIRTUAL, "java/lang/StringBuilder", "append", "(Ljava/lang/String;)Ljava/lang/StringBuilder;", false);
+		}
+	}
+	
+	@Override
+	public void generateCode(MethodVisitor mv, CodeFlow cf) {
+		if (this.exitTypeDescriptor == "Ljava/lang/String") {
+			mv.visitTypeInsn(NEW, "java/lang/StringBuilder");
+			mv.visitInsn(DUP);
+			mv.visitMethodInsn(INVOKESPECIAL, "java/lang/StringBuilder", "<init>", "()V", false);
+			walk(mv,cf,getLeftOperand());
+			walk(mv,cf,getRightOperand());
+			mv.visitMethodInsn(INVOKEVIRTUAL, "java/lang/StringBuilder", "toString", "()Ljava/lang/String;", false);
+		}
+		else {
+			getLeftOperand().generateCode(mv, cf);
+			String leftDesc = getLeftOperand().exitTypeDescriptor;
+			if (!CodeFlow.isPrimitive(leftDesc)) {
+				CodeFlow.insertUnboxInsns(mv, this.exitTypeDescriptor.charAt(0), leftDesc);
 			}
-			switch (this.exitTypeDescriptor.charAt(0)) {
-				case 'I':
-					mv.visitInsn(IADD);
-					break;
-				case 'J':
-					mv.visitInsn(LADD);
-					break;
-				case 'F': 
-					mv.visitInsn(FADD);
-					break;
-				case 'D':
-					mv.visitInsn(DADD);
-					break;				
-				default:
-					throw new IllegalStateException(
-							"Unrecognized exit type descriptor: '" + this.exitTypeDescriptor + "'");
+			if (this.children.length > 1) {
+				cf.enterCompilationScope();
+				getRightOperand().generateCode(mv, cf);
+				String rightDesc = getRightOperand().exitTypeDescriptor;
+				cf.exitCompilationScope();
+				if (!CodeFlow.isPrimitive(rightDesc)) {
+					CodeFlow.insertUnboxInsns(mv, this.exitTypeDescriptor.charAt(0), rightDesc);
+				}
+				switch (this.exitTypeDescriptor.charAt(0)) {
+					case 'I':
+						mv.visitInsn(IADD);
+						break;
+					case 'J':
+						mv.visitInsn(LADD);
+						break;
+					case 'F': 
+						mv.visitInsn(FADD);
+						break;
+					case 'D':
+						mv.visitInsn(DADD);
+						break;				
+					default:
+						throw new IllegalStateException(
+								"Unrecognized exit type descriptor: '" + this.exitTypeDescriptor + "'");
+				}
 			}
 		}
 		cf.pushDescriptor(this.exitTypeDescriptor);

spring-expression/src/main/java/org/springframework/expression/spel/standard/SpelCompiler.java

@@ -153,7 +153,7 @@ public class SpelCompiler implements Opcodes {
 				new String[ ]{"org/springframework/expression/EvaluationException"});
 		mv.visitCode();
 
-		CodeFlow cf = new CodeFlow();
+		CodeFlow cf = new CodeFlow(clazzName, cw);
 
 		// Ask the expression AST to generate the body of the method
 		try {
@@ -176,6 +176,9 @@ public class SpelCompiler implements Opcodes {
 		mv.visitMaxs(0, 0);  // not supplied due to COMPUTE_MAXS
 		mv.visitEnd();
 		cw.visitEnd();
+		
+		cf.finish();
+		
 		byte[] data = cw.toByteArray();
 		// TODO need to make this conditionally occur based on a debug flag
 		// dump(expressionToCompile.toStringAST(), clazzName, data);
@@ -241,6 +244,7 @@ public class SpelCompiler implements Opcodes {
 			tempFile.delete();
 			File f = new File(tempFile, dir);
 			f.mkdirs();
+			// System.out.println("Expression '" + expressionText + "' compiled code dumped to " + dumpLocation);
 			if (logger.isDebugEnabled()) {
 				logger.debug("Expression '" + expressionText + "' compiled code dumped to " + dumpLocation);
 			}

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

@@ -287,7 +287,6 @@ public class ReflectionHelper {
 		if (method.isVarArgs()) {
 			Class<?>[] paramTypes = method.getParameterTypes();
 			varargsPosition = paramTypes.length - 1;
-			conversionOccurred = true;
 		}
 		for (int argPos = 0; argPos < arguments.length; argPos++) {
 			TypeDescriptor targetType;

spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectiveMethodExecutor.java

@@ -17,6 +17,7 @@
 package org.springframework.expression.spel.support;
 
 import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
 
 import org.springframework.core.MethodParameter;
 import org.springframework.core.convert.TypeDescriptor;
@@ -34,9 +35,13 @@ import org.springframework.util.ReflectionUtils;
 public class ReflectiveMethodExecutor implements MethodExecutor {
 
 	private final Method method;
-
+	
 	private final Integer varargsPosition;
 
+	private boolean computedPublicDeclaringClass = false;
+	
+	private Class<?> publicDeclaringClass;
+
 	private boolean argumentConversionOccurred = false;
 
 	public ReflectiveMethodExecutor(Method method) {
@@ -54,6 +59,41 @@ public class ReflectiveMethodExecutor implements MethodExecutor {
 		return this.method;
 	}
 	
+	/**
+	 * Find the first public class in the methods declaring class hierarchy that declares this method.
+	 * Sometimes the reflective method discovery logic finds a suitable method that can easily be 
+	 * called via reflection but cannot be called from generated code when compiling the expression
+	 * because of visibility restrictions. For example if a non public class overrides toString(), this
+	 * helper method will walk up the type hierarchy to find the first public type that declares the
+	 * method (if there is one!). For toString() it may walk as far as Object.
+	 */
+	public Class<?> getPublicDeclaringClass() {
+		if (!computedPublicDeclaringClass) {
+			this.publicDeclaringClass = discoverPublicClass(method, method.getDeclaringClass());
+			this.computedPublicDeclaringClass = true;
+		}
+		return this.publicDeclaringClass;
+	}
+
+	private Class<?> discoverPublicClass(Method method, Class<?> clazz) {
+		if (Modifier.isPublic(clazz.getModifiers())) {
+			try {
+				clazz.getDeclaredMethod(method.getName(), method.getParameterTypes());
+				return clazz;
+			} catch (NoSuchMethodException nsme) {
+				
+			}
+		}
+		Class<?>[] intfaces = clazz.getInterfaces();
+		for (Class<?> intface: intfaces) {
+			discoverPublicClass(method, intface);
+		}
+		if (clazz.getSuperclass() != null) {
+			return discoverPublicClass(method, clazz.getSuperclass());
+		}
+		return null;
+	}
+	
 	public boolean didArgumentConversionOccur() {
 		return this.argumentConversionOccurred;
 	}

spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationPerformanceTests.java

@@ -61,6 +61,144 @@ public class SpelCompilationPerformanceTests extends AbstractExpressionTests {
 			return duration;
 		}
 	}
+	
+	@Test
+	public void inlineLists() throws Exception {
+		expression = parser.parseExpression("{'abcde','ijklm'}[0].substring({1,3,4}[0],{1,3,4}[1])");
+		Object o = expression.getValue();
+		assertEquals("bc",o);
+		System.out.println("Performance check for SpEL expression: '{'abcde','ijklm'}[0].substring({1,3,4}[0],{1,3,4}[1])'");
+		long stime = System.currentTimeMillis();
+		for (int i=0;i<1000000;i++) {
+			o = expression.getValue();
+		}
+		System.out.println("One million iterations: "+(System.currentTimeMillis()-stime)+"ms");
+		stime = System.currentTimeMillis();
+		for (int i=0;i<1000000;i++) {
+			o = expression.getValue();
+		}
+		System.out.println("One million iterations: "+(System.currentTimeMillis()-stime)+"ms");
+		stime = System.currentTimeMillis();
+		for (int i=0;i<1000000;i++) {
+			o = expression.getValue();
+		}
+		System.out.println("One million iterations: "+(System.currentTimeMillis()-stime)+"ms");
+		compile(expression);
+		System.out.println("Now compiled:");
+		o = expression.getValue();
+		assertEquals("bc", o);
+		
+		stime = System.currentTimeMillis();
+		for (int i=0;i<1000000;i++) {
+			o = expression.getValue();
+		}
+		System.out.println("One million iterations: "+(System.currentTimeMillis()-stime)+"ms");
+		stime = System.currentTimeMillis();
+		for (int i=0;i<1000000;i++) {
+			o = expression.getValue();
+		}
+		System.out.println("One million iterations: "+(System.currentTimeMillis()-stime)+"ms");
+		stime = System.currentTimeMillis();
+		for (int i=0;i<1000000;i++) {
+			o = expression.getValue();
+		}
+		System.out.println("One million iterations: "+(System.currentTimeMillis()-stime)+"ms");
+	}
+	
+	@Test
+	public void inlineNestedLists() throws Exception {
+		expression = parser.parseExpression("{'abcde',{'ijklm','nopqr'}}[1][0].substring({1,3,4}[0],{1,3,4}[1])");
+		Object o = expression.getValue();
+		assertEquals("jk",o);
+		System.out.println("Performance check for SpEL expression: '{'abcde','ijklm'}[0].substring({1,3,4}[0],{1,3,4}[1])'");
+		long stime = System.currentTimeMillis();
+		for (int i=0;i<1000000;i++) {
+			o = expression.getValue();
+		}
+		System.out.println("One million iterations: "+(System.currentTimeMillis()-stime)+"ms");
+		stime = System.currentTimeMillis();
+		for (int i=0;i<1000000;i++) {
+			o = expression.getValue();
+		}
+		System.out.println("One million iterations: "+(System.currentTimeMillis()-stime)+"ms");
+		stime = System.currentTimeMillis();
+		for (int i=0;i<1000000;i++) {
+			o = expression.getValue();
+		}
+		System.out.println("One million iterations: "+(System.currentTimeMillis()-stime)+"ms");
+		compile(expression);
+		System.out.println("Now compiled:");
+		o = expression.getValue();
+		assertEquals("jk", o);
+		
+		stime = System.currentTimeMillis();
+		for (int i=0;i<1000000;i++) {
+			o = expression.getValue();
+		}
+		System.out.println("One million iterations: "+(System.currentTimeMillis()-stime)+"ms");
+		stime = System.currentTimeMillis();
+		for (int i=0;i<1000000;i++) {
+			o = expression.getValue();
+		}
+		System.out.println("One million iterations: "+(System.currentTimeMillis()-stime)+"ms");
+		stime = System.currentTimeMillis();
+		for (int i=0;i<1000000;i++) {
+			o = expression.getValue();
+		}
+		System.out.println("One million iterations: "+(System.currentTimeMillis()-stime)+"ms");
+	}
+
+
+	@Test
+	public void stringConcatenation() throws Exception {
+		expression = parser.parseExpression("'hello' + getWorld() + ' spring'");
+		Greeter g = new Greeter();
+		Object o = expression.getValue(g);
+		assertEquals("helloworld spring", o);
+
+		System.out.println("Performance check for SpEL expression: '{'abcde','ijklm'}[0].substring({1,3,4}[0],{1,3,4}[1])'");
+		long stime = System.currentTimeMillis();
+		for (int i=0;i<1000000;i++) {
+			o = expression.getValue(g);
+		}
+		System.out.println("One million iterations: "+(System.currentTimeMillis()-stime)+"ms");
+		stime = System.currentTimeMillis();
+		for (int i=0;i<1000000;i++) {
+			o = expression.getValue(g);
+		}
+		System.out.println("One million iterations: "+(System.currentTimeMillis()-stime)+"ms");
+		stime = System.currentTimeMillis();
+		for (int i=0;i<1000000;i++) {
+			o = expression.getValue(g);
+		}
+		System.out.println("One million iterations: "+(System.currentTimeMillis()-stime)+"ms");
+		compile(expression);
+		System.out.println("Now compiled:");
+		o = expression.getValue(g);
+		assertEquals("helloworld spring", o);
+		
+		stime = System.currentTimeMillis();
+		for (int i=0;i<1000000;i++) {
+			o = expression.getValue(g);
+		}
+		System.out.println("One million iterations: "+(System.currentTimeMillis()-stime)+"ms");
+		stime = System.currentTimeMillis();
+		for (int i=0;i<1000000;i++) {
+			o = expression.getValue(g);
+		}
+		System.out.println("One million iterations: "+(System.currentTimeMillis()-stime)+"ms");
+		stime = System.currentTimeMillis();
+		for (int i=0;i<1000000;i++) {
+			o = expression.getValue(g);
+		}
+		System.out.println("One million iterations: "+(System.currentTimeMillis()-stime)+"ms");
+	}
+	
+	public static class Greeter {
+		public String getWorld() {
+			return "world";
+		}
+	}
 
 	@Test
 	public void complexExpressionPerformance() throws Exception {