diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/CodeFlow.java b/spring-expression/src/main/java/org/springframework/expression/spel/CodeFlow.java index 8a966072660..d974f6ca75d 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/CodeFlow.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/CodeFlow.java @@ -99,6 +99,15 @@ public class CodeFlow implements Opcodes { mv.visitVarInsn(ALOAD, 1); } + /** + * Push the bytecode to load the EvaluationContext (the second parameter passed to + * the compiled expression method). + * @param mv the visitor into which the load instruction should be inserted + */ + public void loadEvaluationContext(MethodVisitor mv) { + mv.visitVarInsn(ALOAD, 2); + } + /** * Record the descriptor for the most recently evaluated expression element. * @param descriptor type descriptor for most recently evaluated element diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpEQ.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpEQ.java index f40bbf30148..80c195fff17 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpEQ.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpEQ.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2014 the original author or authors. + * Copyright 2002-2016 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -16,8 +16,8 @@ package org.springframework.expression.spel.ast; -import org.springframework.asm.Label; import org.springframework.asm.MethodVisitor; +import org.springframework.expression.EvaluationContext; import org.springframework.expression.EvaluationException; import org.springframework.expression.spel.CodeFlow; import org.springframework.expression.spel.ExpressionState; @@ -36,112 +36,61 @@ public class OpEQ extends Operator { this.exitTypeDescriptor = "Z"; } - @Override - public BooleanTypedValue getValueInternal(ExpressionState state) throws EvaluationException { + public BooleanTypedValue getValueInternal(ExpressionState state) + throws EvaluationException { Object left = getLeftOperand().getValueInternal(state).getValue(); Object right = getRightOperand().getValueInternal(state).getValue(); this.leftActualDescriptor = CodeFlow.toDescriptorFromObject(left); this.rightActualDescriptor = CodeFlow.toDescriptorFromObject(right); - return BooleanTypedValue.forValue(equalityCheck(state, left, right)); + return BooleanTypedValue.forValue( + equalityCheck(state.getEvaluationContext(), left, right)); } - + // This check is different to the one in the other numeric operators (OpLt/etc) // because it allows for simple object comparison @Override public boolean isCompilable() { SpelNodeImpl left = getLeftOperand(); - SpelNodeImpl right= getRightOperand(); + SpelNodeImpl right = getRightOperand(); if (!left.isCompilable() || !right.isCompilable()) { return false; } String leftDesc = left.exitTypeDescriptor; String rightDesc = right.exitTypeDescriptor; - DescriptorComparison dc = DescriptorComparison.checkNumericCompatibility( - leftDesc, rightDesc, this.leftActualDescriptor, this.rightActualDescriptor); + DescriptorComparison dc = DescriptorComparison.checkNumericCompatibility(leftDesc, + rightDesc, this.leftActualDescriptor, this.rightActualDescriptor); return (!dc.areNumbers || dc.areCompatible); } - - + @Override public void generateCode(MethodVisitor mv, CodeFlow cf) { + cf.loadEvaluationContext(mv); String leftDesc = getLeftOperand().exitTypeDescriptor; String rightDesc = getRightOperand().exitTypeDescriptor; - Label elseTarget = new Label(); - Label endOfIf = new Label(); boolean leftPrim = CodeFlow.isPrimitive(leftDesc); boolean rightPrim = CodeFlow.isPrimitive(rightDesc); - DescriptorComparison dc = DescriptorComparison.checkNumericCompatibility( - leftDesc, rightDesc, this.leftActualDescriptor, this.rightActualDescriptor); - - if (dc.areNumbers && dc.areCompatible) { - char targetType = dc.compatibleType; - getLeftOperand().generateCode(mv, cf); - if (!leftPrim) { - CodeFlow.insertUnboxInsns(mv, targetType, leftDesc); - } - cf.enterCompilationScope(); - getRightOperand().generateCode(mv, cf); - cf.exitCompilationScope(); - if (!rightPrim) { - CodeFlow.insertUnboxInsns(mv, targetType, rightDesc); - } - // assert: SpelCompiler.boxingCompatible(leftDesc, rightDesc) - if (targetType == 'D') { - mv.visitInsn(DCMPL); - mv.visitJumpInsn(IFNE, elseTarget); - } - else if (targetType == 'F') { - mv.visitInsn(FCMPL); - mv.visitJumpInsn(IFNE, elseTarget); - } - else if (targetType == 'J') { - mv.visitInsn(LCMP); - mv.visitJumpInsn(IFNE, elseTarget); - } - else if (targetType == 'I' || targetType == 'Z') { - mv.visitJumpInsn(IF_ICMPNE, elseTarget); - } - else { - throw new IllegalStateException("Unexpected descriptor " + leftDesc); - } + cf.enterCompilationScope(); + getLeftOperand().generateCode(mv, cf); + cf.exitCompilationScope(); + if (leftPrim) { + CodeFlow.insertBoxIfNecessary(mv, leftDesc.charAt(0)); } - else { - getLeftOperand().generateCode(mv, cf); - if (leftPrim) { - CodeFlow.insertBoxIfNecessary(mv, leftDesc.charAt(0)); - } - getRightOperand().generateCode(mv, cf); - if (rightPrim) { - CodeFlow.insertBoxIfNecessary(mv, rightDesc.charAt(0)); - } - Label leftNotNull = new Label(); - mv.visitInsn(DUP_X1); // dup right on the top of the stack - mv.visitJumpInsn(IFNONNULL, leftNotNull); - // Right is null! - mv.visitInsn(SWAP); - mv.visitInsn(POP); // remove it - Label rightNotNull = new Label(); - mv.visitJumpInsn(IFNONNULL, rightNotNull); - // Left is null too - mv.visitInsn(ICONST_1); - mv.visitJumpInsn(GOTO, endOfIf); - mv.visitLabel(rightNotNull); - mv.visitInsn(ICONST_0); - mv.visitJumpInsn(GOTO, endOfIf); - mv.visitLabel(leftNotNull); - mv.visitMethodInsn(INVOKEVIRTUAL, "java/lang/Object", "equals", "(Ljava/lang/Object;)Z", false); - mv.visitLabel(endOfIf); - cf.pushDescriptor("Z"); - return; + cf.enterCompilationScope(); + getRightOperand().generateCode(mv, cf); + cf.exitCompilationScope(); + if (rightPrim) { + CodeFlow.insertBoxIfNecessary(mv, rightDesc.charAt(0)); } - mv.visitInsn(ICONST_1); - mv.visitJumpInsn(GOTO, endOfIf); - mv.visitLabel(elseTarget); - mv.visitInsn(ICONST_0); - mv.visitLabel(endOfIf); + + String operatorClassName = Operator.class.getName().replace('.', '/'); + String evaluationContextClassName = EvaluationContext.class.getName().replace('.', + '/'); + mv.visitMethodInsn(INVOKESTATIC, operatorClassName, "equalityCheck", "(L" + + evaluationContextClassName + ";Ljava/lang/Object;Ljava/lang/Object;)Z", + false); cf.pushDescriptor("Z"); } diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpNE.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpNE.java index 4f96b3bbddf..356f4f87d9b 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpNE.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/OpNE.java @@ -18,6 +18,7 @@ package org.springframework.expression.spel.ast; import org.springframework.asm.Label; import org.springframework.asm.MethodVisitor; +import org.springframework.expression.EvaluationContext; import org.springframework.expression.EvaluationException; import org.springframework.expression.spel.CodeFlow; import org.springframework.expression.spel.ExpressionState; @@ -36,14 +37,15 @@ public class OpNE extends Operator { this.exitTypeDescriptor = "Z"; } - @Override - public BooleanTypedValue getValueInternal(ExpressionState state) throws EvaluationException { + public BooleanTypedValue getValueInternal(ExpressionState state) + throws EvaluationException { Object left = getLeftOperand().getValueInternal(state).getValue(); Object right = getRightOperand().getValueInternal(state).getValue(); this.leftActualDescriptor = CodeFlow.toDescriptorFromObject(left); this.rightActualDescriptor = CodeFlow.toDescriptorFromObject(right); - return BooleanTypedValue.forValue(!equalityCheck(state, left, right)); + return BooleanTypedValue.forValue( + !equalityCheck(state.getEvaluationContext(), left, right)); } // This check is different to the one in the other numeric operators (OpLt/etc) @@ -51,72 +53,56 @@ public class OpNE extends Operator { @Override public boolean isCompilable() { SpelNodeImpl left = getLeftOperand(); - SpelNodeImpl right= getRightOperand(); + SpelNodeImpl right = getRightOperand(); if (!left.isCompilable() || !right.isCompilable()) { return false; } String leftDesc = left.exitTypeDescriptor; String rightDesc = right.exitTypeDescriptor; - DescriptorComparison dc = DescriptorComparison.checkNumericCompatibility( - leftDesc, rightDesc, this.leftActualDescriptor, this.rightActualDescriptor); + DescriptorComparison dc = DescriptorComparison.checkNumericCompatibility(leftDesc, + rightDesc, this.leftActualDescriptor, this.rightActualDescriptor); return (!dc.areNumbers || dc.areCompatible); } - + @Override public void generateCode(MethodVisitor mv, CodeFlow cf) { + cf.loadEvaluationContext(mv); String leftDesc = getLeftOperand().exitTypeDescriptor; String rightDesc = getRightOperand().exitTypeDescriptor; - Label elseTarget = new Label(); - Label endOfIf = new Label(); boolean leftPrim = CodeFlow.isPrimitive(leftDesc); boolean rightPrim = CodeFlow.isPrimitive(rightDesc); - DescriptorComparison dc = DescriptorComparison.checkNumericCompatibility( - leftDesc, rightDesc, this.leftActualDescriptor, this.rightActualDescriptor); - - if (dc.areNumbers && dc.areCompatible) { - char targetType = dc.compatibleType; - getLeftOperand().generateCode(mv, cf); - if (!leftPrim) { - CodeFlow.insertUnboxInsns(mv, targetType, leftDesc); - } - cf.enterCompilationScope(); - getRightOperand().generateCode(mv, cf); - cf.exitCompilationScope(); - if (!rightPrim) { - CodeFlow.insertUnboxInsns(mv, targetType, rightDesc); - } - // assert: SpelCompiler.boxingCompatible(leftDesc, rightDesc) - if (targetType == 'D') { - mv.visitInsn(DCMPL); - mv.visitJumpInsn(IFEQ, elseTarget); - } - else if (targetType == 'F') { - mv.visitInsn(FCMPL); - mv.visitJumpInsn(IFEQ, elseTarget); - } - else if (targetType == 'J') { - mv.visitInsn(LCMP); - mv.visitJumpInsn(IFEQ, elseTarget); - } - else if (targetType == 'I' || targetType == 'Z') { - mv.visitJumpInsn(IF_ICMPEQ, elseTarget); - } - else { - throw new IllegalStateException("Unexpected descriptor " + leftDesc); - } + cf.enterCompilationScope(); + getLeftOperand().generateCode(mv, cf); + cf.exitCompilationScope(); + if (leftPrim) { + CodeFlow.insertBoxIfNecessary(mv, leftDesc.charAt(0)); } - else { - getLeftOperand().generateCode(mv, cf); - getRightOperand().generateCode(mv, cf); - mv.visitJumpInsn(IF_ACMPEQ, elseTarget); + cf.enterCompilationScope(); + getRightOperand().generateCode(mv, cf); + cf.exitCompilationScope(); + if (rightPrim) { + CodeFlow.insertBoxIfNecessary(mv, rightDesc.charAt(0)); } + + String operatorClassName = Operator.class.getName().replace('.', '/'); + String evaluationContextClassName = EvaluationContext.class.getName().replace('.', + '/'); + mv.visitMethodInsn(INVOKESTATIC, operatorClassName, "equalityCheck", "(L" + + evaluationContextClassName + ";Ljava/lang/Object;Ljava/lang/Object;)Z", + false); + + // Invert the boolean + Label notZero = new Label(); + Label end = new Label(); + mv.visitJumpInsn(IFNE, notZero); mv.visitInsn(ICONST_1); - mv.visitJumpInsn(GOTO,endOfIf); - mv.visitLabel(elseTarget); + mv.visitJumpInsn(GOTO, end); + mv.visitLabel(notZero); mv.visitInsn(ICONST_0); - mv.visitLabel(endOfIf); + mv.visitLabel(end); + cf.pushDescriptor("Z"); } diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/Operator.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/Operator.java index 73ed4a75c7b..cc91a1691bf 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/Operator.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/Operator.java @@ -21,8 +21,8 @@ import java.math.BigInteger; import org.springframework.asm.Label; import org.springframework.asm.MethodVisitor; +import org.springframework.expression.EvaluationContext; import org.springframework.expression.spel.CodeFlow; -import org.springframework.expression.spel.ExpressionState; import org.springframework.util.ClassUtils; import org.springframework.util.NumberUtils; import org.springframework.util.ObjectUtils; @@ -114,7 +114,9 @@ public abstract class Operator extends SpelNodeImpl { leftDesc, rightDesc, this.leftActualDescriptor, this.rightActualDescriptor); char targetType = dc.compatibleType; // CodeFlow.toPrimitiveTargetDesc(leftDesc); + cf.enterCompilationScope(); getLeftOperand().generateCode(mv, cf); + cf.exitCompilationScope(); if (unboxLeft) { CodeFlow.insertUnboxInsns(mv, targetType, leftDesc); } @@ -157,7 +159,7 @@ public abstract class Operator extends SpelNodeImpl { cf.pushDescriptor("Z"); } - protected boolean equalityCheck(ExpressionState state, Object left, Object right) { + public static boolean equalityCheck(EvaluationContext context, Object left, Object right) { if (left instanceof Number && right instanceof Number) { Number leftNumber = (Number) left; Number rightNumber = (Number) right; @@ -207,7 +209,7 @@ public abstract class Operator extends SpelNodeImpl { if (left instanceof Comparable && right instanceof Comparable) { Class ancestor = ClassUtils.determineCommonAncestor(left.getClass(), right.getClass()); if (ancestor != null && Comparable.class.isAssignableFrom(ancestor)) { - return (state.getTypeComparator().compare(left, right) == 0); + return (context.getTypeComparator().compare(left, right) == 0); } } diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java index 0ef848a276b..0e712878838 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java @@ -1335,8 +1335,6 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { public void opEq() throws Exception { String tvar = "35"; expression = parse("#root == 35"); - Boolean bb = (Boolean)expression.getValue(tvar); - System.out.println(bb); assertFalse((Boolean)expression.getValue(tvar)); assertCanCompile(expression); assertFalse((Boolean)expression.getValue(tvar)); @@ -1623,6 +1621,122 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { assertTrue((Boolean)expression.getValue()); } + @Test + public void opNe_SPR14863() throws Exception { + // First part is from the test case specified in the bug report + Map data = new HashMap<>(); + data.put("my-key", new String("my-value")); + StandardEvaluationContext context = new StandardEvaluationContext(new MyContext(data)); + SpelParserConfiguration configuration = new SpelParserConfiguration(SpelCompilerMode.MIXED, + ClassLoader.getSystemClassLoader()); + SpelExpressionParser parser = new SpelExpressionParser(configuration); + Expression expression = parser.parseExpression("data['my-key'] != 'my-value'"); + assertFalse(expression.getValue(context, Boolean.class)); + assertCanCompile(expression); + ((SpelExpression) expression).compileExpression(); + assertFalse(expression.getValue(context, Boolean.class)); + + List ls = new ArrayList(); + ls.add(new String("foo")); + context = new StandardEvaluationContext(ls); + expression = parse("get(0) != 'foo'"); + assertFalse(expression.getValue(context, Boolean.class)); + assertCanCompile(expression); + assertFalse(expression.getValue(context, Boolean.class)); + + ls.remove(0); + ls.add("goo"); + assertTrue(expression.getValue(context, Boolean.class)); + } + + @Test + public void opEq_SPR14863() throws Exception { + // Exercise the comparator invocation code that runs in + // equalityCheck() (called from interpreted and compiled code) + expression = parser.parseExpression("#aa==#bb"); + StandardEvaluationContext sec = new StandardEvaluationContext(); + Apple aa = new Apple(1); + Apple bb = new Apple(2); + sec.setVariable("aa",aa); + sec.setVariable("bb",bb); + boolean b = expression.getValue(sec,Boolean.class); + // Verify what the expression caused aa to be compared to + assertEquals(bb,aa.gotComparedTo); + assertFalse(b); + bb.setValue(1); + b = expression.getValue(sec,Boolean.class); + assertEquals(bb,aa.gotComparedTo); + assertTrue(b); + + assertCanCompile(expression); + + // Similar test with compiled expression + aa = new Apple(99); + bb = new Apple(100); + sec.setVariable("aa",aa); + sec.setVariable("bb",bb); + b = expression.getValue(sec,Boolean.class); + assertFalse(b); + assertEquals(bb,aa.gotComparedTo); + bb.setValue(99); + b = expression.getValue(sec,Boolean.class); + assertTrue(b); + assertEquals(bb,aa.gotComparedTo); + + + List ls = new ArrayList(); + ls.add(new String("foo")); + StandardEvaluationContext context = new StandardEvaluationContext(ls); + expression = parse("get(0) == 'foo'"); + assertTrue(expression.getValue(context, Boolean.class)); + assertCanCompile(expression); + assertTrue(expression.getValue(context, Boolean.class)); + + ls.remove(0); + ls.add("goo"); + assertFalse(expression.getValue(context, Boolean.class)); + } + + public static class Apple implements Comparable { + public Object gotComparedTo = null; + public int i; + + public Apple(int i) { + this.i = i; + } + + public void setValue(int i) { + this.i = i; + } + + @Override + public int compareTo(Apple that) { + this.gotComparedTo = that; + if (this.ithat.i) { + return +1; + } else { + return 0; + } + } + + } + + // For opNe_SPR14863 + public static class MyContext { + + private final Map data; + + public MyContext(Map data) { + this.data = data; + } + + public Map getData() { + return data; + } + } + @Test public void opPlus() throws Exception { expression = parse("2+2"); @@ -3901,7 +4015,7 @@ public class SpelCompilationCoverageTests extends AbstractExpressionTests { m.put("andy","778"); expression = parse("['andy']==null?1:2"); - System.out.println(expression.getValue(m)); + assertEquals(2,expression.getValue(m)); assertCanCompile(expression); assertEquals(2,expression.getValue(m)); m.remove("andy");