VerifyError when trying to compile constructor invocation with SpEL [SPR-12326]

[spring-projects-issues]

Thomas Darimont opened SPR-12326 and commented

I tried to speed up dynamic instance creation by compiling constructor invocations with SpEL. Unfortunately I get a VerifyError when I try to evaluate the expression via getValue();.
I added a test case that demonstrates the issue.
The test case passes when SpelCompilerMode.OFF is used.

Tested this with (java version "1.8.0_20") and compile target java6.

package test;

import org.junit.Assert;
import org.junit.Test;
import org.springframework.expression.Expression;
import org.springframework.expression.spel.SpelCompilerMode;
import org.springframework.expression.spel.SpelParserConfiguration;
import org.springframework.expression.spel.standard.SpelExpressionParser;

/**
 * @author Thomas Darimont
 */
public class CompiledSpelBug {

	@Test
	public void compiledConstructorInvocations(){

		SpelExpressionParser parser = new SpelExpressionParser(new SpelParserConfiguration(SpelCompilerMode.IMMEDIATE, Thread.currentThread().getContextClassLoader()));
		Expression expr = parser.parseExpression("new test.CompiledSpelBug.Obj([0])");

		Obj o = (Obj)expr.getValue(new Object[]{"test"});
		Assert.assertEquals("test",o.param1);

		//the next line fails with:
		//java.lang.VerifyError: (class: spel/Ex2, method: getValue signature: (Ljava/lang/Object;Lorg/springframework/expression/ EvaluationContext;)Ljava/lang/Object;) Incompatible argument to function
		o = (Obj)expr.getValue(new Object[]{"test"});

		Assert.assertEquals("test",o.param1);
	}

	public static class Obj{

		private final String param1;

		public Obj(String param1){
			this.param1 = param1;
		}
	}
}

StackTrace with Spring Framework 4.1:

java.lang.VerifyError: (class: spel/Ex2, method: getValue signature: (Ljava/lang/Object;Lorg/springframework/expression/EvaluationContext;)Ljava/lang/Object;) Incompatible argument to function
	at java.lang.Class.getDeclaredConstructors0(Native Method)
	at java.lang.Class.privateGetDeclaredConstructors(Class.java:2658)
	at java.lang.Class.getConstructor0(Class.java:2964)
	at java.lang.Class.newInstance(Class.java:403)
	at org.springframework.expression.spel.standard.SpelCompiler.compile(SpelCompiler.java:106)
	at org.springframework.expression.spel.standard.SpelExpression.compileExpression(SpelExpression.java:464)
	at org.springframework.expression.spel.standard.SpelExpression.checkCompile(SpelExpression.java:434)
	at org.springframework.expression.spel.standard.SpelExpression.getValue(SpelExpression.java:153)
	at bob.spel.CompiledSpelBug.foo(CompiledSpelBug.java:24)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:271)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:70)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:50)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:238)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:53)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:309)
	at org.junit.runner.JUnitCore.run(JUnitCore.java:160)
	at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:74)
	at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:211)
	at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:67)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at com.intellij.rt.execution.application.AppMain.main(AppMain.java:134)

StackTrace with Spring Framework 4.1.1:

java.lang.IllegalStateException: Failed to instantiate CompiledExpression
	at org.springframework.expression.spel.standard.SpelCompiler.compile(SpelCompiler.java:111)
	at org.springframework.expression.spel.standard.SpelExpression.compileExpression(SpelExpression.java:464)
	at org.springframework.expression.spel.standard.SpelExpression.checkCompile(SpelExpression.java:434)
	at org.springframework.expression.spel.standard.SpelExpression.getValue(SpelExpression.java:153)
	at bob.spel.CompiledSpelBug.compiledConstructorInvocations(CompiledSpelBug.java:26)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:271)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:70)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:50)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:238)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:53)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:309)
	at org.junit.runner.JUnitCore.run(JUnitCore.java:160)
	at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:74)
	at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:211)
	at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:67)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at com.intellij.rt.execution.application.AppMain.main(AppMain.java:134)
Caused by: java.lang.VerifyError: (class: spel/Ex2, method: getValue signature: (Ljava/lang/Object;Lorg/springframework/expression/EvaluationContext;)Ljava/lang/Object;) Incompatible argument to function
	at java.lang.Class.getDeclaredConstructors0(Native Method)
	at java.lang.Class.privateGetDeclaredConstructors(Class.java:2658)
	at java.lang.Class.getConstructor0(Class.java:2964)
	at java.lang.Class.newInstance(Class.java:403)
	at org.springframework.expression.spel.standard.SpelCompiler.compile(SpelCompiler.java:108)
	... 30 more

---

Affects: 4.1 GA, 4.1.1

Issue Links:

• VerifyErrors when using SpEL compilation with Thymeleaf [SPR-12271] #16876 VerifyErrors when using SpEL compilation with Thymeleaf
• SpEL chooses wrong method to invoke when varargs method is present [SPR-12803] #17400 SpEL chooses wrong method to invoke when varargs method is present

0 votes, 6 watchers

[spring-projects-issues]

Juergen Hoeller commented

Andy, may I assign this to you... The provided test case does fail so the issue is definitely valid - and hopefully easy enough to resolve.

Juergen

[spring-projects-issues]

Christoph Strobl commented

The test below passes if Person resides in the same package as the test does, but fails once Person is moved somewhere else.

@Test
public void youShouldNotFailSouldYou() {

  SpelExpressionParser parser = new SpelExpressionParser(new SpelParserConfiguration(SpelCompilerMode.IMMEDIATE, this
      .getClass().getClassLoader()));

  Person person = new Person(1);

  SpelExpression ex = parser.parseRaw("#it?.age?.equals([0])");
  StandardEvaluationContext context = new StandardEvaluationContext(new Object[] { person.getAge() });
  context.setVariable("it", person);

  assertThat(ex.getValue(context, Boolean.class), is(true));
  assertThat(ex.getValue(context, Boolean.class), is(true));
}

public class Person {

	private int age;

	public Person(int age) {
		this.age = age;
	}

	public int getAge() {
		return age;
	}

	public void setAge(int age) {
		this.age = age;
	}
}

[spring-projects-issues]

Thomas Darimont commented

Hello,

I had a quick look at the issue:

It seems that in org.springframework.expression.spel.standard.SpelExpression#getValue(org.springframework.expression.EvaluationContext)
we always pass null as the target parameter, but the constructor parameters are pulled from that target.

So if I change:

Object result = this.compiledAst.getValue(target,context);

to be:

Object target = context.getRootObject().getValue();
Object result = this.compiledAst.getValue(target,context);

We can make sure that the approrpriate target value is passed on.

Another thing I found was, that there seems to be a CHECKCAST instruction missing in org.springframework.expression.spel.ast.ConstructorReference.generateCode(MethodVisitor, CodeFlow) before

cf.exitCompilationScope();
``` within the loop for each parameter value (to please the bytecode verifier).

So if I change:
```java 
...
  cf.exitCompilationScope();
}
...

to be:

...
  mv.visitTypeInsn(CHECKCAST,paramDescriptors[c-1].substring(1)); // it's probably not always needed
  cf.exitCompilationScope();
}
...

I can run the above SpEL expression from the example:

"new test.CompiledSpelBug.Obj([0])"

without any problems in IMMEDIATE mode :)

Hope this helps!

Cheers,
Thomas

[spring-projects-issues]

Thomas Darimont commented

I think Christophs problem is similar to the one mentioned above.

In org.springframework.expression.spel.standard.SpelExpression#getValue(org.springframework.expression.EvaluationContext, java.lang.Class<T>) null is passed as the target value. If I change:

Object result = this.compiledAst.getValue(null,context);

to

Object target = context.getRootObject().getValue();
Object result = this.compiledAst.getValue(target,context);

and load the target variable in org.springframework.expression.spel.ast.MethodReference#generateCode on to the stack, by adjusting the if condition from:

if (descriptor == null && !isStaticMethod) {
   cf.loadTarget(mv);
}

to

if (descriptor == null || !isStaticMethod) {
   cf.loadTarget(mv);
}

I can run Christophs test without any problems.

[spring-projects-issues]

Andy Clement commented

I already have the fix ready for your problem Thomas - it was simply to change the constructor argument processing to use the new common argument processing code I built for method argument processing (under #16933). This also means constructor references can use varargs constructors as a bonus. I'll commit it in a bit after digesting Christoph's issue.

[spring-projects-issues]

Thomas Darimont commented

Great! thanks for having a look at tit :)

[spring-projects-issues]

Andy Clement commented

Fixes all pushed. Multiple fixes, some as Thomas discovered:

• the argument processing for constructors was quite naive and didn't have much test coverage. Method calls had much better coverage and the argument processing code was much more sophisticated (putting in the right casts when required, etc). It was also just rewritten last week to handle varargs. With a tweak I could just change it so that logic is used by both constructors and method calls now - which fixes up the first testcase here and as a side effect enables compilation of varargs constructor references.

• The second testcase here is very different (although there is a verify error). Thomas' point about handling the case where the root object is passed indirectly via the evaluation context was exactly right, that is now fixed. It also showed we aren't boxing primitives when method calls are made on them (e.g. 1.toString()). That is also now fixed.

I did not make the final change Thomas mentions though. The condition "if (descriptor == null && !isStaticMethod) {" is saying "if there is nothing on the stack but we need something", if I change that to "if (descriptor == null || !isStaticMethod) {" then I think I'll get unnecessary stuff loaded onto the stack and possibly the wrong thing. The problem the testcase was hitting is that there was a primitive on the stack and it wasn't being boxed before equals was being invoked, by adjusting it to '||' we were slapping the root object onto the stack again which just hid the problem with the primitive. A slightly variant of the failing testcase shows the primitive boxing is the right thing to do.

[spring-projects-issues]

Christoph Strobl commented

Thanks for the fix - though I ran into problems when trying the following.

	@Test
	public void failsWhenSettingContextForExpression() {

		SpelExpressionParser parser = new SpelExpressionParser(new SpelParserConfiguration(SpelCompilerMode.IMMEDIATE, this
				.getClass().getClassLoader()));

		Person person = new Person("foo", 1);

		SpelExpression expression = parser.parseRaw("#it?.age?.equals([0])");
		StandardEvaluationContext context = new StandardEvaluationContext(new Object[] { 1 });
		context.setVariable("it", person);

		expression.setEvaluationContext(context);

		System.out.println(expression.getValue(Boolean.class));
		System.out.println(expression.getValue(Boolean.class));
		
		//have to call it 3 time to fail
		System.out.println(expression.getValue(Boolean.class));
	}

the above works fine when I use expression.getValue(context, Boolean.class) or even expression.getValue(expression.getEvaluationContext(), Boolean.class)

[spring-projects-issues]

Andy Clement commented

I'll take a look, thanks for the testcase.

[spring-projects-issues]

Andy Clement commented

@Christoph fixes are in for your problem.

I've also made the following changes to improve varargs handling:

• more test cases and tweaks around passing a single value to a varargs accepting method.
• adjusted the method 'sorting' in ReflectiveMethodResolver. The ordering was unstable and in the case of foo(String) and foo(String...) it would sometimes put the varargs first, sometimes put it second. I've stabilised it with varargs always sorted after non-varargs. In order to ensure we always try to prefer non-varargs over varargs I've also changed what happens for close matches (where a method can accept what is being passed without a conversion but the method is actually specified to accept a super type). We now take the first close match rather than the last. For example the call foo('hello') and the candidates being foo(Object) and foo(Object...) - due to the sorting and taking the first close match we would choose the first foo option there.

The change to ordering and close match selection may affect existing programs out there but these programs are operating with unstable orderings right now that might change between invocations. I think we want stability.

[spring-projects-issues]

Christoph Strobl commented

Thanks Andy Clement that solved it!