EAGLESIX-2700 Fixed errors in AndExpression. Tests are complete and pass. However, I don't like the result of the optimization. It is missing obvious trivial optimizations.

lib/pipeline/expressions/AndExpression.js

@@ -14,7 +14,7 @@
 var AndExpression = module.exports = function AndExpression() {
 	if (arguments.length !== 0) throw new Error("zero args expected");
 	base.call(this);
-}, klass = AndExpression, base = require("./VariadicExpressionT")(AndExpression), proto = klass.prototype = Object.create(base.prototype, {constructor: {value: klass}});
+}, klass = AndExpression, base = require("./VariadicExpressionT")(klass), proto = klass.prototype = Object.create(base.prototype, {constructor: {value: klass}});
 
 // DEPENDENCIES
 var Value = require("../Value"),
@@ -35,7 +35,7 @@ proto.getOpName = function getOpName() {
 proto.evaluateInternal = function evaluateInternal(vars) {
 	for (var i = 0, n = this.operands.length; i < n; ++i) {
 		var value = this.operands[i].evaluateInternal(vars);
-		if (!Value.coerceToBool()) return false;
+		if (!Value.coerceToBool(value)) return false;
 	}
 	return true;
 };
@@ -55,7 +55,7 @@ proto.optimize = function optimize() {
 	if (!(lastExpr instanceof ConstantExpression)) return expr;
 
 	// Evaluate and coerce the last argument to a boolean.  If it's false, then we can replace this entire expression.
-	var last = Value.coerceToBool(lastExpr.evaluate());
+	var last = Value.coerceToBool(lastExpr.evaluateInternal());
 	if (!last) return new ConstantExpression(false);
 
 	// If we got here, the final operand was true, so we don't need it anymore.

test/lib/pipeline/expressions/AndExpression_test.js

@@ -1,6 +1,11 @@
 "use strict";
 var assert = require("assert"),
 	AndExpression = require("../../../../lib/pipeline/expressions/AndExpression"),
+	VariablesParseState = require("../../../../lib/pipeline/expressions/VariablesParseState"),
+	VariablesIdGenerator = require("../../../../lib/pipeline/expressions/VariablesIdGenerator"),
+	CoerceToBoolExpression = require("../../../../lib/pipeline/expressions/CoerceToBoolExpression"),
+	ConstantExpression = require("../../../../lib/pipeline/expressions/ConstantExpression"),
+	FieldPathExpression = require("../../../../lib/pipeline/expressions/FieldPathExpression"),
 	Expression = require("../../../../lib/pipeline/expressions/Expression");
 
 
@@ -8,6 +13,10 @@ module.exports = {
 
 	"AndExpression": {
 
+		beforeEach: function() {
+			this.vps = new VariablesParseState(new VariablesIdGenerator());
+		},
+
 		"constructor()": {
 
 			"should not throw Error when constructing without args": function testConstructor(){
@@ -36,51 +45,51 @@ module.exports = {
 		"#evaluate()": {
 
 			"should return true if no operands were given; {$and:[]}": function testEmpty(){
-				assert.equal(Expression.parseOperand({$and:[]}).evaluate(), true);
+				assert.equal(Expression.parseOperand({$and:[]},this.vps).evaluate(), true);
 			},
 
 			"should return true if operands is one true; {$and:[true]}": function testTrue(){
-				assert.equal(Expression.parseOperand({$and:[true]}).evaluate(), true);
+				assert.equal(Expression.parseOperand({$and:[true]},this.vps).evaluate(), true);
 			},
 
 			"should return false if operands is one false; {$and:[false]}": function testFalse(){
-				assert.equal(Expression.parseOperand({$and:[false]}).evaluate(), false);
+				assert.equal(Expression.parseOperand({$and:[false]},this.vps).evaluate(), false);
 			},
 
 			"should return true if operands are true and true; {$and:[true,true]}": function testTrueTrue(){
-				assert.equal(Expression.parseOperand({$and:[true,true]}).evaluate(), true);
+				assert.equal(Expression.parseOperand({$and:[true,true]},this.vps).evaluate(), true);
 			},
 
 			"should return false if operands are true and false; {$and:[true,false]}": function testTrueFalse(){
-				assert.equal(Expression.parseOperand({$and:[true,false]}).evaluate(), false);
+				assert.equal(Expression.parseOperand({$and:[true,false]},this.vps).evaluate(), false);
 			},
 
 			"should return false if operands are false and true; {$and:[false,true]}": function testFalseTrue(){
-				assert.equal(Expression.parseOperand({$and:[false,true]}).evaluate(), false);
+				assert.equal(Expression.parseOperand({$and:[false,true]},this.vps).evaluate(), false);
 			},
 
 			"should return false if operands are false and false; {$and:[false,false]}": function testFalseFalse(){
-				assert.equal(Expression.parseOperand({$and:[false,false]}).evaluate(), false);
+				assert.equal(Expression.parseOperand({$and:[false,false]},this.vps).evaluate(), false);
 			},
 
 			"should return true if operands are true, true, and true; {$and:[true,true,true]}": function testTrueTrueTrue(){
-				assert.equal(Expression.parseOperand({$and:[true,true,true]}).evaluate(), true);
+				assert.equal(Expression.parseOperand({$and:[true,true,true]},this.vps).evaluate(), true);
 			},
 
 			"should return false if operands are true, true, and false; {$and:[true,true,false]}": function testTrueTrueFalse(){
-				assert.equal(Expression.parseOperand({$and:[true,true,false]}).evaluate(), false);
+				assert.equal(Expression.parseOperand({$and:[true,true,false]},this.vps).evaluate(), false);
 			},
 
 			"should return false if operands are 0 and 1; {$and:[0,1]}": function testZeroOne(){
-				assert.equal(Expression.parseOperand({$and:[0,1]}).evaluate(), false);
+				assert.equal(Expression.parseOperand({$and:[0,1]},this.vps).evaluate(), false);
 			},
 
 			"should return false if operands are 1 and 2; {$and:[1,2]}": function testOneTwo(){
-				assert.equal(Expression.parseOperand({$and:[1,2]}).evaluate(), true);
+				assert.equal(Expression.parseOperand({$and:[1,2]},this.vps).evaluate(), true);
 			},
 
 			"should return true if operand is a path String to a truthy value; {$and:['$a']}": function testFieldPath(){
-				assert.equal(Expression.parseOperand({$and:['$a']}).evaluate({a:1}), true);
+				assert.equal(Expression.parseOperand({$and:['$a']},this.vps).evaluate({a:1}), true);
 			}
 
 		},
@@ -88,50 +97,129 @@ module.exports = {
 		"#optimize()": {
 
 			"should optimize a constant expression to a constant; {$and:[1]} == true": function testOptimizeConstantExpression(){
-				assert.deepEqual(Expression.parseOperand({$and:[1]}).optimize().toJSON(true), {$const:true});
+				var a = Expression.parseOperand({$and:[1]}, this.vps).optimize();
+				assert.equal(a.operands.length, 0, "The operands should have been optimized away");
+				assert.equal(a.evaluateInternal(), true);
 			},
 
 			"should not optimize a non-constant expression; {$and:['$a']}": function testNonConstant(){
-				assert.deepEqual(Expression.parseOperand({$and:['$a']}).optimize().toJSON(), {$and:['$a']});
+				var a = Expression.parseOperand({$and:['$a']}, this.vps).optimize();
+				assert.equal(a.operands[0]._fieldPath.fieldNames.length, 2);
+				assert.deepEqual(a.operands[0]._fieldPath.fieldNames[0], "CURRENT");
+				assert.deepEqual(a.operands[0]._fieldPath.fieldNames[1], "a");
 			},
 
-			"optimize an expression beginning with a constant; {$and:[1,'$a']};": function testConstantNonConstant(){
-				assert.deepEqual(Expression.parseOperand({$and:[1,'$a']}).optimize().toJSON(), {$and:[1,'$a']});
-				assert.notEqual(Expression.parseOperand({$and:[1,'$a']}).optimize().toJSON(), {$and:[0,'$a']});
+			"should not optimize an expression ending with a non-constant. This makes me sad; {$and:[1,'$a']};": function testConstantNonConstant(){
+				var a = Expression.parseOperand({$and:[1,'$a']}, this.vps).optimize();
+				assert.equal(a.operands.length, 2, "Both operands should remain.");
+
+				// The constant is in the front and as such, remains.
+				assert.equal(a.operands[0].evaluateInternal(), 1, "The constant operand should remain, inexplicably");
+
+				// This is the '$a' which cannot be optimized.
+				assert.deepEqual(a.operands[1]._fieldPath.fieldNames.length, 2);
+				assert.deepEqual(a.operands[1]._fieldPath.fieldNames[0], "CURRENT");
+				assert.deepEqual(a.operands[1]._fieldPath.fieldNames[1], "a");
 			},
 
-			"should optimize an expression with a path and a '1' (is entirely constant); {$and:['$a',1]}": function testNonConstantOne(){
-				assert.deepEqual(Expression.parseOperand({$and:['$a',1]}).optimize().toJSON(), {$and:['$a']});
+			"should optimize an expression with a path and a '1'; {$and:['$a',1]}": function testNonConstantOne(){
+				var a = Expression.parseOperand({$and:['$a', 1]}, this.vps).optimize();
+				// The 1 should be removed as it is redundant.
+				assert(a instanceof CoerceToBoolExpression, "The result should be forced to a boolean");
+
+				// This is the '$a' which cannot be optimized.
+				assert.equal(a.expression._fieldPath.fieldNames.length, 2);
+				assert.equal(a.expression._fieldPath.fieldNames[0], "CURRENT");
+				assert.equal(a.expression._fieldPath.fieldNames[1], "a");
 			},
 
 			"should optimize an expression with a field path and a '0'; {$and:['$a',0]}": function testNonConstantZero(){
-				assert.deepEqual(Expression.parseOperand({$and:['$a',0]}).optimize().toJSON(true), {$const:false});
+				var a = Expression.parseOperand({$and:['$a',0]}, this.vps).optimize();
+				assert.equal(a.operands.length, 0, "The operands should have been optimized away");
+				assert.equal(a.evaluateInternal(), false, "The 0 operand should have been converted to false");
 			},
 
 			"should optimize an expression with two field paths and '1'; {$and:['$a','$b',1]}": function testNonConstantNonConstantOne(){
-				assert.deepEqual(Expression.parseOperand({$and:['$a','$b',1]}).optimize().toJSON(), {$and:['$a','$b']});
+				var a = Expression.parseOperand({$and:['$a', '$b', 1]}, this.vps).optimize();
+				assert.equal(a.operands.length, 2, "Two operands should remain.");
+
+				// This is the '$a' which cannot be optimized.
+				assert.deepEqual(a.operands[0]._fieldPath.fieldNames.length, 2);
+				assert.deepEqual(a.operands[0]._fieldPath.fieldNames[0], "CURRENT");
+				assert.deepEqual(a.operands[0]._fieldPath.fieldNames[1], "a");
+
+				// This is the '$b' which cannot be optimized.
+				assert.deepEqual(a.operands[1]._fieldPath.fieldNames.length, 2);
+				assert.deepEqual(a.operands[1]._fieldPath.fieldNames[0], "CURRENT");
+				assert.deepEqual(a.operands[1]._fieldPath.fieldNames[1], "b");
 			},
 
 			"should optimize an expression with two field paths and '0'; {$and:['$a','$b',0]}": function testNonConstantNonConstantZero(){
-				assert.deepEqual(Expression.parseOperand({$and:['$a','$b',0]}).optimize().toJSON(true), {$const:false});
+				var a = Expression.parseOperand({$and:['$a', '$b', 0]}, this.vps).optimize();
+				assert(a instanceof ConstantExpression, "With that trailing false, we know the result...");
+				assert.equal(a.operands.length, 0, "The operands should have been optimized away");
+				assert.equal(a.evaluateInternal(), false);
 			},
 
 			"should optimize an expression with '0', '1', and a field path; {$and:[0,1,'$a']}": function testZeroOneNonConstant(){
-				assert.deepEqual(Expression.parseOperand({$and:[0,1,'$a']}).optimize().toJSON(true), {$const:false});
+				var a = Expression.parseOperand({$and:[0,1,'$a']}, this.vps).optimize();
+				assert.equal(a.operands.length, 3, "Because a non-constant is on the right, no optimization occurs. /fume");
+				assert.equal(a.operands[0].evaluateInternal(), 0);
+				assert.equal(a.operands[1].evaluateInternal(), 1);
+
+				assert.equal(a.operands[2]._fieldPath.fieldNames.length, 2);
+				assert.equal(a.operands[2]._fieldPath.fieldNames[0], "CURRENT");
+				assert.equal(a.operands[2]._fieldPath.fieldNames[1], "a");
 			},
 
 			"should optimize an expression with '1', '1', and a field path; {$and:[1,1,'$a']}": function testOneOneNonConstant(){
-				assert.deepEqual(Expression.parseOperand({$and:[1,1,'$a']}).optimize().toJSON(), {$and:['$a']});
+				var a = Expression.parseOperand({$and:[1,1,'$a']}, this.vps).optimize();
+				assert.equal(a.operands.length, 3, "Because a non-constant is on the right, no optimization occurs. /fume");
+				assert.equal(a.operands[0].evaluateInternal(), 1);
+				assert.equal(a.operands[1].evaluateInternal(), 1);
+
+				assert.equal(a.operands[2]._fieldPath.fieldNames.length, 2);
+				assert.equal(a.operands[2]._fieldPath.fieldNames[0], "CURRENT");
+				assert.equal(a.operands[2]._fieldPath.fieldNames[1], "a");
 			},
 
 			"should optimize nested $and expressions properly and optimize out values evaluating to true; {$and:[1,{$and:[1]},'$a','$b']}": function testNested(){
-				assert.deepEqual(Expression.parseOperand({$and:[1,{$and:[1]},'$a','$b']}).optimize().toJSON(), {$and:['$a','$b']});
+				var a = Expression.parseOperand({$and:[1,{$and:[1]},'$a','$b']}, this.vps).optimize();
+				assert.equal(a.operands.length, 4, "There should be 4 operands because optimization largely stops when the right-hand operand is a non-constant");
+				assert(a.operands[0] instanceof ConstantExpression, "But why is this even here?");
+				assert.equal(a.operands[0].evaluateInternal(), true);
+				assert(a.operands[1] instanceof ConstantExpression, "But why is this even here?");
+				assert.equal(a.operands[1].evaluateInternal(), true);
+				assert(a.operands[2] instanceof FieldPathExpression);
+				assert(a.operands[3] instanceof FieldPathExpression);
 			},
 
 			"should optimize nested $and expressions containing a nested value evaluating to false; {$and:[1,{$and:[1]},'$a','$b']}": function testNested(){
-				assert.deepEqual(Expression.parseOperand({$and:[1,{$and:[{$and:[0]}]},'$a','$b']}).optimize().toJSON(true), {$const:false});
+				//assert.deepEqual(Expression.parseOperand({$and:[1,{$and:[{$and:[0]}]},'$a','$b']}, this.vps).optimize().toJSON(true), {$const:false});
+				var a = Expression.parseOperand({$and:[1,{$and:[{$and:[0]}]},'$a','$b']}, this.vps).optimize();
+				assert.equal(a.operands.length, 4, "There should be 4 operands because optimization largely stops when the right-hand operand is a non-constant");
+				assert(a.operands[0] instanceof ConstantExpression, "But why is this even here?");
+				assert.equal(a.operands[0].evaluateInternal(), true);
+				assert(a.operands[1] instanceof ConstantExpression, "But why is this even here?");
+				assert.equal(a.operands[1].evaluateInternal(), false);
+				assert(a.operands[2] instanceof FieldPathExpression);
+				assert(a.operands[3] instanceof FieldPathExpression);
+			},
+
+			"should optimize when the constants are on the right of the operand list. The rightmost is true": function(){
+				// 1, "x", and 1 are all true.  They should be optimized away.
+				var a = Expression.parseOperand({$and:['$a', 1, "x", 1]}, this.vps).optimize();
+				assert.equal(a.operands.length, 3, "The constants should have been optimized away");
+				assert(a.operands[0] instanceof FieldPathExpression, "Only the path should remain");
+				assert(a.operands[1] instanceof ConstantExpression, "Why is this here?");
+				assert(a.operands[2] instanceof ConstantExpression, "Why is this here?");
+			},
+			"should optimize when the constants are on the right of the operand list. The rightmost is false": function(){
+				// 1, "x", and 1 are all true.  They should be optimized away.
+				var a = Expression.parseOperand({$and:['$a', 1, "x", 0]}, this.vps).optimize();
+				assert(a instanceof ConstantExpression, "The rightmost false kills it all");
+				assert.equal(a.evaluateInternal(), false);
 			}
-
 		}
 
 	}