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

Co-authored-by: SalvadorC <[email protected]>

Author: doniacld

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)
@@ -480,6 +481,19 @@ _Description_: Packages declaring structs that contain other inline struct defin
 
 _Configuration_: N/A
 
+## optimize-operands-order
+
+_Description_: conditional expressions can be written to take advantage of short circuit evaluation and speed up its average evaluation time by forcing the evaluation of less time-consuming terms before more costly ones. This rule spots logical expressions where the order of evaluation of terms seems non optimal. Please notice that confidence of this rule is low and is up to the user to decide if the suggested rewrite of the expression keeps the semantics of the original one.
+
+_Configuration_: N/A
+
+Example:
+
+    if isGenerated(content) && !config.IgnoreGeneratedHeader {
+Swap left and right side :
+
+    if !config.IgnoreGeneratedHeader && isGenerated(content) {
+
 ## package-comments
 
 _Description_: Packages should have comments. This rule warns on undocumented packages and when packages comments are detached to the `package` keyword.

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
+}