Boolean fix (#1148)

* Fixing booleans

* Fixed "and", "or" evaluating RHS when not required

examples/data1.yaml

@@ -1 +1,2 @@
-cat: purrs
+a: mike
+b: [t, f]

pkg/yqlib/operator_booleans.go

@@ -27,41 +27,48 @@ func isTruthy(c *CandidateNode) (bool, error) {
 	return isTruthyNode(node)
 }
 
-type boolOp func(bool, bool) bool
+func getBoolean(candidate *CandidateNode) (bool, error) {
+	if candidate != nil {
+		candidate.Node = unwrapDoc(candidate.Node)
+		return isTruthy(candidate)
+	}
+	return false, nil
+}
 
-func performBoolOp(op boolOp) func(d *dataTreeNavigator, context Context, lhs *CandidateNode, rhs *CandidateNode) (*CandidateNode, error) {
+func getOwner(lhs *CandidateNode, rhs *CandidateNode) *CandidateNode {
-	return func(d *dataTreeNavigator, context Context, lhs *CandidateNode, rhs *CandidateNode) (*CandidateNode, error) {
+	owner := lhs
-		owner := lhs
 
-		if lhs == nil && rhs == nil {
+	if lhs == nil && rhs == nil {
-			owner = &CandidateNode{}
+		owner = &CandidateNode{}
-		} else if lhs == nil {
+	} else if lhs == nil {
-			owner = rhs
+		owner = rhs
+	}
+	return owner
+}
+
+func returnRhsTruthy(d *dataTreeNavigator, context Context, lhs *CandidateNode, rhs *CandidateNode) (*CandidateNode, error) {
+	owner := getOwner(lhs, rhs)
+	rhsBool, err := getBoolean(rhs)
+	if err != nil {
+		return nil, err
+	}
+
+	return createBooleanCandidate(owner, rhsBool), nil
+}
+
+func returnLHSWhen(targetBool bool) func(lhs *CandidateNode) (*CandidateNode, error) {
+	return func(lhs *CandidateNode) (*CandidateNode, error) {
+		var err error
+		var lhsBool bool
+
+		if lhsBool, err = getBoolean(lhs); err != nil || lhsBool != targetBool {
+			return nil, err
 		}
-
+		owner := &CandidateNode{}
-		var errDecoding error
-		lhsTrue := false
 		if lhs != nil {
-			lhs.Node = unwrapDoc(lhs.Node)
+			owner = lhs
-			lhsTrue, errDecoding = isTruthy(lhs)
-
-			if errDecoding != nil {
-				return nil, errDecoding
-			}
 		}
-		log.Debugf("-- lhsTrue", lhsTrue)
+		return createBooleanCandidate(owner, targetBool), nil
-
-		rhsTrue := false
-		if rhs != nil {
-			rhs.Node = unwrapDoc(rhs.Node)
-			rhsTrue, errDecoding = isTruthy(rhs)
-			if errDecoding != nil {
-				return nil, errDecoding
-			}
-		}
-		log.Debugf("-- rhsTrue", rhsTrue)
-
-		return createBooleanCandidate(owner, op(lhsTrue, rhsTrue)), nil
 	}
 }
 
@@ -133,20 +140,21 @@ func anyOperator(d *dataTreeNavigator, context Context, expressionNode *Expressi
 }
 
 func orOperator(d *dataTreeNavigator, context Context, expressionNode *ExpressionNode) (Context, error) {
-	log.Debugf("-- orOp")
+	prefs := crossFunctionPreferences{
-	return crossFunction(d, context.ReadOnlyClone(), expressionNode, performBoolOp(
+		CalcWhenEmpty:  true,
-		func(b1 bool, b2 bool) bool {
+		Calculation:    returnRhsTruthy,
-			log.Debugf("-- peformingOrOp with %v and %v", b1, b2)
+		LhsResultValue: returnLHSWhen(true),
-			return b1 || b2
+	}
-		}), true)
+	return crossFunctionWithPrefs(d, context.ReadOnlyClone(), expressionNode, prefs)
 }
 
 func andOperator(d *dataTreeNavigator, context Context, expressionNode *ExpressionNode) (Context, error) {
-	log.Debugf("-- AndOp")
+	prefs := crossFunctionPreferences{
-	return crossFunction(d, context.ReadOnlyClone(), expressionNode, performBoolOp(
+		CalcWhenEmpty:  true,
-		func(b1 bool, b2 bool) bool {
+		Calculation:    returnRhsTruthy,
-			return b1 && b2
+		LhsResultValue: returnLHSWhen(false),
-		}), true)
+	}
+	return crossFunctionWithPrefs(d, context.ReadOnlyClone(), expressionNode, prefs)
 }
 
 func notOperator(d *dataTreeNavigator, context Context, expressionNode *ExpressionNode) (Context, error) {

pkg/yqlib/operator_booleans_test.go

@@ -44,6 +44,22 @@ var booleanOperatorScenarios = []expressionScenario{
 			"D0, P[], (doc)::b: hi\n",
 		},
 	},
+	{
+		skipDoc:     true,
+		description: "And should not run 2nd arg if first is false",
+		expression:  `false and test(3)`,
+		expected: []string{
+			"D0, P[], (!!bool)::false\n",
+		},
+	},
+	{
+		skipDoc:     true,
+		description: "Or should not run 2nd arg if first is true",
+		expression:  `true or test(3)`,
+		expected: []string{
+			"D0, P[], (!!bool)::true\n",
+		},
+	},
 	{
 		description: "`and` example",
 		expression:  `true and false`,
@@ -151,12 +167,21 @@ var booleanOperatorScenarios = []expressionScenario{
 		document:   `{a: true, b: false}`,
 		expression: `.[] or (false, true)`,
 		expected: []string{
-			"D0, P[a], (!!bool)::true\n",
 			"D0, P[a], (!!bool)::true\n",
 			"D0, P[b], (!!bool)::false\n",
 			"D0, P[b], (!!bool)::true\n",
 		},
 	},
+	{
+		skipDoc:    true,
+		document:   `{a: true, b: false}`,
+		expression: `.[] and (false, true)`,
+		expected: []string{
+			"D0, P[a], (!!bool)::false\n",
+			"D0, P[a], (!!bool)::true\n",
+			"D0, P[b], (!!bool)::false\n",
+		},
+	},
 	{
 		skipDoc:    true,
 		document:   `{}`,

pkg/yqlib/operators.go

@@ -54,10 +54,25 @@ func emptyOperator(d *dataTreeNavigator, context Context, expressionNode *Expres
 
 type crossFunctionCalculation func(d *dataTreeNavigator, context Context, lhs *CandidateNode, rhs *CandidateNode) (*CandidateNode, error)
 
-func resultsForRHS(d *dataTreeNavigator, context Context, lhsCandidate *CandidateNode, rhs Context, calculation crossFunctionCalculation, results *list.List, calcWhenEmpty bool) error {
+func resultsForRHS(d *dataTreeNavigator, context Context, lhsCandidate *CandidateNode, prefs crossFunctionPreferences, rhsExp *ExpressionNode, results *list.List) error {
 
-	if calcWhenEmpty && rhs.MatchingNodes.Len() == 0 {
+	if prefs.LhsResultValue != nil {
-		resultCandidate, err := calculation(d, context, lhsCandidate, nil)
+		result, err := prefs.LhsResultValue(lhsCandidate)
+		if err != nil {
+			return err
+		} else if result != nil {
+			results.PushBack(result)
+			return nil
+		}
+	}
+
+	rhs, err := d.GetMatchingNodes(context, rhsExp)
+	if err != nil {
+		return err
+	}
+
+	if prefs.CalcWhenEmpty && rhs.MatchingNodes.Len() == 0 {
+		resultCandidate, err := prefs.Calculation(d, context, lhsCandidate, nil)
 		if err != nil {
 			return err
 		}
@@ -70,7 +85,7 @@ func resultsForRHS(d *dataTreeNavigator, context Context, lhsCandidate *Candidat
 	for rightEl := rhs.MatchingNodes.Front(); rightEl != nil; rightEl = rightEl.Next() {
 		log.Debugf("Applying calc")
 		rhsCandidate := rightEl.Value.(*CandidateNode)
-		resultCandidate, err := calculation(d, context, lhsCandidate, rhsCandidate)
+		resultCandidate, err := prefs.Calculation(d, context, lhsCandidate, rhsCandidate)
 		if err != nil {
 			return err
 		}
@@ -81,7 +96,15 @@ func resultsForRHS(d *dataTreeNavigator, context Context, lhsCandidate *Candidat
 	return nil
 }
 
-func doCrossFunc(d *dataTreeNavigator, context Context, expressionNode *ExpressionNode, calculation crossFunctionCalculation, calcWhenEmpty bool) (Context, error) {
+type crossFunctionPreferences struct {
+	CalcWhenEmpty bool
+	// if this returns a result node,
+	// we wont bother calculating the RHS
+	LhsResultValue func(*CandidateNode) (*CandidateNode, error)
+	Calculation    crossFunctionCalculation
+}
+
+func doCrossFunc(d *dataTreeNavigator, context Context, expressionNode *ExpressionNode, prefs crossFunctionPreferences) (Context, error) {
 	var results = list.New()
 	lhs, err := d.GetMatchingNodes(context, expressionNode.LHS)
 	if err != nil {
@@ -89,14 +112,8 @@ func doCrossFunc(d *dataTreeNavigator, context Context, expressionNode *Expressi
 	}
 	log.Debugf("crossFunction LHS len: %v", lhs.MatchingNodes.Len())
 
-	rhs, err := d.GetMatchingNodes(context, expressionNode.RHS)
+	if prefs.CalcWhenEmpty && lhs.MatchingNodes.Len() == 0 {
-
+		err := resultsForRHS(d, context, nil, prefs, expressionNode.RHS, results)
-	if err != nil {
-		return Context{}, err
-	}
-
-	if calcWhenEmpty && lhs.MatchingNodes.Len() == 0 {
-		err := resultsForRHS(d, context, nil, rhs, calculation, results, calcWhenEmpty)
 		if err != nil {
 			return Context{}, err
 		}
@@ -105,7 +122,7 @@ func doCrossFunc(d *dataTreeNavigator, context Context, expressionNode *Expressi
 	for el := lhs.MatchingNodes.Front(); el != nil; el = el.Next() {
 		lhsCandidate := el.Value.(*CandidateNode)
 
-		err := resultsForRHS(d, context, lhsCandidate, rhs, calculation, results, calcWhenEmpty)
+		err = resultsForRHS(d, context, lhsCandidate, prefs, expressionNode.RHS, results)
 		if err != nil {
 			return Context{}, err
 		}
@@ -115,6 +132,11 @@ func doCrossFunc(d *dataTreeNavigator, context Context, expressionNode *Expressi
 }
 
 func crossFunction(d *dataTreeNavigator, context Context, expressionNode *ExpressionNode, calculation crossFunctionCalculation, calcWhenEmpty bool) (Context, error) {
+	prefs := crossFunctionPreferences{CalcWhenEmpty: calcWhenEmpty, Calculation: calculation}
+	return crossFunctionWithPrefs(d, context, expressionNode, prefs)
+}
+
+func crossFunctionWithPrefs(d *dataTreeNavigator, context Context, expressionNode *ExpressionNode, prefs crossFunctionPreferences) (Context, error) {
 	var results = list.New()
 
 	var evaluateAllTogether = true
@@ -124,15 +146,16 @@ func crossFunction(d *dataTreeNavigator, context Context, expressionNode *Expres
 			break
 		}
 	}
+
 	if evaluateAllTogether {
 		log.Debug("crossFunction evaluateAllTogether!")
-		return doCrossFunc(d, context, expressionNode, calculation, calcWhenEmpty)
+		return doCrossFunc(d, context, expressionNode, prefs)
 	}
 
 	log.Debug("crossFunction evaluate apart!")
 
 	for matchEl := context.MatchingNodes.Front(); matchEl != nil; matchEl = matchEl.Next() {
-		innerResults, err := doCrossFunc(d, context.SingleChildContext(matchEl.Value.(*CandidateNode)), expressionNode, calculation, calcWhenEmpty)
+		innerResults, err := doCrossFunc(d, context.SingleChildContext(matchEl.Value.(*CandidateNode)), expressionNode, prefs)
 		if err != nil {
 			return Context{}, err
 		}