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

pkg/yqlib/operator_booleans.go

@@ -27,8 +27,6 @@ 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)
@@ -48,42 +46,30 @@ func getOwner(lhs *CandidateNode, rhs *CandidateNode) *CandidateNode {
 	return owner
 }
 
-func andOp(d *dataTreeNavigator, context Context, lhs *CandidateNode, rhs *CandidateNode) (*CandidateNode, error) {
+func returnRhsTruthy(d *dataTreeNavigator, context Context, lhs *CandidateNode, rhs *CandidateNode) (*CandidateNode, error) {
 	owner := getOwner(lhs, rhs)
-	var err error
+	rhsBool, err := getBoolean(rhs)
-	lhsBool := true
+	if err != nil {
-	if lhsBool, err = getBoolean(lhs); err != nil {
-		return nil, err
-	}
-	if lhsBool == false {
-		return createBooleanCandidate(owner, false), nil
-	}
-
-	rhsBool := true
-	if rhsBool, err = getBoolean(rhs); err != nil {
 		return nil, err
 	}
 
 	return createBooleanCandidate(owner, rhsBool), nil
 }
 
-func orOp(d *dataTreeNavigator, context Context, lhs *CandidateNode, rhs *CandidateNode) (*CandidateNode, error) {
+func returnLHSWhen(targetBool bool) func(lhs *CandidateNode) (*CandidateNode, error) {
-	owner := getOwner(lhs, rhs)
+	return func(lhs *CandidateNode) (*CandidateNode, error) {
 		var err error
-	lhsBool := true
+		var lhsBool bool
-	if lhsBool, err = getBoolean(lhs); err != nil {
+
+		if lhsBool, err = getBoolean(lhs); err != nil || lhsBool != targetBool {
 			return nil, err
 		}
-	if lhsBool == true {
+		owner := &CandidateNode{}
-		return createBooleanCandidate(owner, true), nil
+		if lhs != nil {
+			owner = lhs
 		}
-
+		return createBooleanCandidate(owner, targetBool), nil
-	rhsBool := true
-	if rhsBool, err = getBoolean(rhs); err != nil {
-		return nil, err
 	}
-
-	return createBooleanCandidate(owner, rhsBool), nil
 }
 
 func findBoolean(wantBool bool, d *dataTreeNavigator, context Context, expressionNode *ExpressionNode, sequenceNode *yaml.Node) (bool, error) {
@@ -154,12 +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, orOp, true)
+		CalcWhenEmpty:  true,
+		Calculation:    returnRhsTruthy,
+		LhsResultValue: returnLHSWhen(true),
+	}
+	return crossFunctionWithPrefs(d, context.ReadOnlyClone(), expressionNode, prefs)
 }
 
 func andOperator(d *dataTreeNavigator, context Context, expressionNode *ExpressionNode) (Context, error) {
-
+	prefs := crossFunctionPreferences{
+		CalcWhenEmpty:  true,
+		Calculation:    returnRhsTruthy,
+		LhsResultValue: returnLHSWhen(false),
+	}
+	return crossFunctionWithPrefs(d, context.ReadOnlyClone(), expressionNode, prefs)
 }
 
 func notOperator(d *dataTreeNavigator, context Context, expressionNode *ExpressionNode) (Context, error) {

pkg/yqlib/operator_booleans_test.go

@@ -47,17 +47,17 @@ var booleanOperatorScenarios = []expressionScenario{
 	{
 		skipDoc:     true,
 		description: "And should not run 2nd arg if first is false",
-		expression:  `select(false and test(3))`,
+		expression:  `false and test(3)`,
 		expected: []string{
-			"D0, P[], (doc)::b: hi\n",
+			"D0, P[], (!!bool)::false\n",
 		},
 	},
 	{
 		skipDoc:     true,
 		description: "Or should not run 2nd arg if first is true",
-		expression:  `select(true or test(3))`,
+		expression:  `true or test(3)`,
 		expected: []string{
-			"D0, P[], (doc)::b: hi\n",
+			"D0, P[], (!!bool)::true\n",
 		},
 	},
 	{
@@ -167,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
 		}