Add optimize-operands-order rule (mgechev#599)

README.md

@@ -477,6 +477,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a
 | [`nested-structs`](./RULES_DESCRIPTIONS.md#nested-structs)          |  n/a   |  Warns on structs within structs |    no    |  no   |
 | [`useless-break`](./RULES_DESCRIPTIONS.md#useless-break)          |  n/a   |  Warns on useless `break` statements in case clauses |    no    |  no   |
 | [`banned-characters`](./RULES_DESCRIPTIONS.md#banned-characters)          |  n/a   |  Checks banned characters in identifiers |    no    |  no   |
+| [`optimize-operands-order`](./RULES_DESCRIPTIONS.md#optimize-operands-order)          |  n/a   |  Checks inefficient conditional expressions |    no    |  no   |
 
 ## Configurable rules
 

RULES_DESCRIPTIONS.md

@@ -47,6 +47,7 @@ List of all available rules.
   - [modifies-parameter](#modifies-parameter)
   - [modifies-value-receiver](#modifies-value-receiver)
   - [nested-structs](#nested-structs)
+  - [optimize-operands-order](#optimize-operands-order)
   - [package-comments](#package-comments)
   - [range](#range)
   - [range-val-in-closure](#range-val-in-closure)
@@ -562,6 +563,21 @@ This rule highlights redundant _else-blocks_ that can be eliminated from the cod
 
 _Configuration_: N/A
 
+## optimize-operands-order
+
+_Description_: To improve slightly efficiency, order of terms in a conditional expression can speed up its average evaluation time.
+This rule spots caller on the left side of a conditional expression and a non-caller on the right side. 
+To avoid making a function call in those cases where the caller is evaluated to false.
+
+_Configuration_: N/A
+
+Example:
+
+    if isGenerated(content) && !config.IgnoreGeneratedHeader {
+Swap left and right side :
+
+    if !config.IgnoreGeneratedHeader && isGenerated(content) {
+
 ## time-equal
 
 _Description_: This rule warns when using `==` and `!=` for equality check `time.Time` and suggest to `time.time.Equal` method, for about information follow this [link](https://pkg.go.dev/time#Time)

config/config.go

@@ -83,6 +83,7 @@ var allRules = append([]lint.Rule{
 	&rule.UselessBreak{},
 	&rule.TimeEqualRule{},
 	&rule.BannedCharsRule{},
+	&rule.OptimizeOperandsOrderRule{},
 }, defaultRules...)
 
 var allFormatters = []lint.Formatter{

rule/optimize-operands-order.go

@@ -0,0 +1,77 @@
+package rule
+
+import (
+	"fmt"
+	"go/ast"
+	"go/token"
+
+	"github.com/mgechev/revive/lint"
+)
+
+// OptimizeOperandsOrderRule lints given else constructs.
+type OptimizeOperandsOrderRule struct{}
+
+// Apply applies the rule to given file.
+func (r *OptimizeOperandsOrderRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
+	var failures []lint.Failure
+
+	onFailure := func(failure lint.Failure) {
+		failures = append(failures, failure)
+	}
+	w := lintOptimizeOperandsOrderlExpr{
+		onFailure: onFailure,
+	}
+	ast.Walk(w, file.AST)
+	return failures
+}
+
+// Name returns the rule name.
+func (r *OptimizeOperandsOrderRule) Name() string {
+	return "optimize-operands-order"
+}
+
+type lintOptimizeOperandsOrderlExpr struct {
+	onFailure func(failure lint.Failure)
+}
+
+// Visit checks boolean AND and OR expressions to determine
+// if swapping their operands may result in an execution speedup.
+func (w lintOptimizeOperandsOrderlExpr) Visit(node ast.Node) ast.Visitor {
+	binExpr, ok := node.(*ast.BinaryExpr)
+	if !ok {
+		return w
+	}
+
+	switch binExpr.Op {
+	case token.LAND, token.LOR:
+	default:
+		return w
+	}
+
+	isCaller := func(n ast.Node) bool {
+		_, ok := n.(*ast.CallExpr)
+		return ok
+	}
+
+	// check if the left sub-expression contains a function call
+	nodes := pick(binExpr.X, isCaller, nil)
+	if len(nodes) < 1 {
+		return w
+	}
+
+	// check if the right sub-expression does not contain a function call
+	nodes = pick(binExpr.Y, isCaller, nil)
+	if len(nodes) > 0 {
+		return w
+	}
+
+	newExpr := ast.BinaryExpr{X: binExpr.Y, Y: binExpr.X, Op: binExpr.Op}
+	w.onFailure(lint.Failure{
+		Failure:    fmt.Sprintf("for better performance '%v' might be rewritten as '%v'", gofmt(binExpr), gofmt(&newExpr)),
+		Node:       node,
+		Category:   "optimization",
+		Confidence: 0.3,
+	})
+
+	return w
+}

rule/utils.go

@@ -111,7 +111,7 @@ func srcLine(src []byte, p token.Position) string {
 
 // pick yields a list of nodes by picking them from a sub-ast with root node n.
 // Nodes are selected by applying the fselect function
-// f function is applied to each selected node before inseting it in the final result.
+// f function is applied to each selected node before inserting it in the final result.
 // If f==nil then it defaults to the identity function (ie it returns the node itself)
 func pick(n ast.Node, fselect func(n ast.Node) bool, f func(n ast.Node) []ast.Node) []ast.Node {
 	var result []ast.Node

test/optimize-operands-order_test.go

@@ -0,0 +1,13 @@
+package test
+
+import (
+	"testing"
+
+	"github.com/mgechev/revive/lint"
+	"github.com/mgechev/revive/rule"
+)
+
+// Test that left and right side of Binary operators (only AND, OR) are swapable
+func TestOptimizeOperandsOrder(t *testing.T) {
+	testRule(t, "optimize-operands-order", &rule.OptimizeOperandsOrderRule{}, &lint.RuleConfig{})
+}

testdata/optimize-operands-order.go

@@ -0,0 +1,32 @@
+package fixtures
+
+func conditionalExpr(x, y bool) bool {
+	equal := x == y // should not match, not AND or OR operators
+	if x || y {     // should not match, no caller
+		return true
+	}
+	or := caller(x, y) || y // MATCH /for better performance 'caller(x, y) || y' might be rewritten as 'y || caller(x, y)'/
+	if caller(x, y) || y {  // MATCH /for better performance 'caller(x, y) || y' might be rewritten as 'y || caller(x, y)'/
+		return true
+	}
+
+	switch {
+	case x == y:
+		return y
+	case caller(x, y) && y: // MATCH /for better performance 'caller(x, y) && y' might be rewritten as 'y && caller(x, y)'/
+		return x
+	}
+
+	complexExpr := caller(caller(x, y) && y, y) || y
+	// MATCH:20 /for better performance 'caller(caller(x, y) && y, y) || y' might be rewritten as 'y || caller(caller(x, y) && y, y)'/
+	// MATCH:20 /for better performance 'caller(x, y) && y' might be rewritten as 'y && caller(x, y)'/
+
+	noSwap := caller(x, y) || (x && caller(y, x)) // should not match, caller in the right operand
+
+	callRight := caller(x, y) && (x && caller(y, x)) // should not match, caller in the right operand
+	return caller(x, y) && y                         // MATCH /for better performance 'caller(x, y) && y' might be rewritten as 'y && caller(x, y)'/
+}
+
+func caller(x, y bool) bool {
+	return true
+}