diff --git a/README.md b/README.md index 86702bb8e..1adb89c87 100644 --- a/README.md +++ b/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 diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md index 65cbd61b1..88fb97b82 100644 --- a/RULES_DESCRIPTIONS.md +++ b/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) diff --git a/config/config.go b/config/config.go index dc44f107e..6c21ea322 100644 --- a/config/config.go +++ b/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{ diff --git a/rule/optimize-operands-order.go b/rule/optimize-operands-order.go new file mode 100644 index 000000000..ccb8a3279 --- /dev/null +++ b/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 +} diff --git a/rule/utils.go b/rule/utils.go index 0d5744846..06139f57e 100644 --- a/rule/utils.go +++ b/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 diff --git a/test/optimize-operands-order_test.go b/test/optimize-operands-order_test.go new file mode 100644 index 000000000..e8d50b3f1 --- /dev/null +++ b/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{}) +} diff --git a/testdata/optimize-operands-order.go b/testdata/optimize-operands-order.go new file mode 100644 index 000000000..4ea2e9ea2 --- /dev/null +++ b/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 +}