diff --git a/README.md b/README.md index 3e4b9bd..9f18dfb 100644 --- a/README.md +++ b/README.md @@ -591,6 +591,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a | [`unexported-naming`](./RULES_DESCRIPTIONS.md#unexported-naming) | n/a | Warns on wrongly named un-exported symbols | no | no | | [`unexported-return`](./RULES_DESCRIPTIONS.md#unexported-return) | n/a | Warns when a public return is from unexported type. | yes | yes | | [`unhandled-error`](./RULES_DESCRIPTIONS.md#unhandled-error) | []string | Warns on unhandled errors returned by function calls | no | yes | +| [`unnecessary-if`](./RULES_DESCRIPTIONS.md#unnecessary-if) | n/a | Identifies `if-else` statements that can be replaced by simpler statements | no | no | | [`unnecessary-format`](./RULES_DESCRIPTIONS.md#unnecessary-format) | n/a | Identifies calls to formatting functions where the format string does not contain any formatting verbs | no | no | | [`unnecessary-stmt`](./RULES_DESCRIPTIONS.md#unnecessary-stmt) | n/a | Suggests removing or simplifying unnecessary statements | no | no | | [`unreachable-code`](./RULES_DESCRIPTIONS.md#unreachable-code) | n/a | Warns on unreachable code | no | no | diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md index 29282bd..817ae71 100644 --- a/RULES_DESCRIPTIONS.md +++ b/RULES_DESCRIPTIONS.md @@ -87,6 +87,7 @@ List of all available rules. - [unexported-naming](#unexported-naming) - [unexported-return](#unexported-return) - [unhandled-error](#unhandled-error) +- [unnecessary-if](#unnecessary-if) - [unnecessary-format](#unnecessary-format) - [unnecessary-stmt](#unnecessary-stmt) - [unreachable-code](#unreachable-code) @@ -1500,6 +1501,39 @@ arguments = [ ] ``` +## unnecessary-if + +_Description_: Detects unnecessary `if-else` statements that return or assign a boolean value +based on a condition and suggests a simplified, direct return or assignment. +The `if-else` block is redundant because the condition itself is already a boolean expression. +The simplified version is immediately clearer, more idiomatic, and reduces cognitive load for the reader. + +### Examples (unnecessary-if) + +```go +if y <= 0 { + z = true +} else { + z = false +} + +if x > 10 { + return false +} else { + return true +} +``` + +Fixed code: + +```go +z = y <= 0 + +return x <= 10 +``` + +_Configuration_: N/A + ## unnecessary-format _Description_: This rule identifies calls to formatting functions where the format string does not contain any formatting verbs diff --git a/config/config.go b/config/config.go index d163308..ba97832 100644 --- a/config/config.go +++ b/config/config.go @@ -115,6 +115,7 @@ var allRules = append([]lint.Rule{ &rule.UnsecureURLSchemeRule{}, &rule.InefficientMapLookupRule{}, &rule.ForbiddenCallInWgGoRule{}, + &rule.UnnecessaryIfRule{}, }, defaultRules...) // allFormatters is a list of all available formatters to output the linting results. diff --git a/rule/unnecessary_if.go b/rule/unnecessary_if.go new file mode 100644 index 0000000..f2f8174 --- /dev/null +++ b/rule/unnecessary_if.go @@ -0,0 +1,202 @@ +package rule + +import ( + "fmt" + "go/ast" + "go/token" + + "github.com/mgechev/revive/internal/astutils" + "github.com/mgechev/revive/lint" +) + +// UnnecessaryIfRule warns on if...else statements that can be replaced by simpler expressions. +type UnnecessaryIfRule struct{} + +// Apply applies the rule to given file. +func (*UnnecessaryIfRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { + var failures []lint.Failure + + onFailure := func(failure lint.Failure) { + failures = append(failures, failure) + } + + w := &lintUnnecessaryIf{onFailure: onFailure} + for _, decl := range file.AST.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || fn.Body == nil { + continue + } + + ast.Walk(w, fn.Body) + } + + return failures +} + +// Name returns the rule name. +func (*UnnecessaryIfRule) Name() string { + return "unnecessary-if" +} + +type lintUnnecessaryIf struct { + onFailure func(lint.Failure) +} + +// Visit walks the AST looking for if statements of the form: +// +// if cond { return } else { return } +// +// or +// +// if cond { = } else { = }. +func (w *lintUnnecessaryIf) Visit(node ast.Node) ast.Visitor { + ifStmt, ok := node.(*ast.IfStmt) + if !ok { + return w // not an if statement + } + + // if without else or if with initialization + mustSkip := ifStmt.Else == nil || ifStmt.Init != nil + if mustSkip { + return w + } + + elseBranch, ok := ifStmt.Else.(*ast.BlockStmt) + if !ok { // if-else-if construction + return w // the rule only copes with single if...else statements + } + + thenStmts := ifStmt.Body.List + elseStmts := elseBranch.List + if len(thenStmts) != 1 || len(elseStmts) != 1 { + return w // then and else branches do not have just one statement + } + + replacement := "" + thenBool := false + switch thenStmt := thenStmts[0].(type) { + case *ast.ReturnStmt: + replacement, thenBool = w.replacementForReturnStmt(thenStmt, elseStmts) + case *ast.AssignStmt: + replacement, thenBool = w.replacementForAssignmentStmt(thenStmt, elseStmts) + default: + return w // the then branch is neither a return nor an assignment + } + + if replacement == "" { + return w // no replacement found + } + + cond := w.condAsString(ifStmt.Cond, !thenBool) + msg := "replace this conditional by: " + replacement + " " + cond + + w.onFailure(lint.Failure{ + Confidence: 1.0, + Node: ifStmt, + Category: lint.FailureCategoryLogic, + Failure: msg, + }) + + return nil +} + +var relationalOppositeOf = map[token.Token]token.Token{ + token.EQL: token.NEQ, // == != + token.GEQ: token.LSS, // >= < + token.GTR: token.LEQ, // > <= + token.LEQ: token.GTR, // <= > + token.LSS: token.GEQ, // < >= + token.NEQ: token.EQL, // != == +} + +// condAsString yields the string representation of the given condition expression. +// The method will try to minimize the negations in the resulting expression. +func (*lintUnnecessaryIf) condAsString(cond ast.Expr, mustNegate bool) string { + result := astutils.GoFmt(cond) + + if mustNegate { + result = "!(" + result + ")" // naive negation + + // check if we can build a simpler expression + if binExp, ok := cond.(*ast.BinaryExpr); ok { + originalOp := binExp.Op + opposite, ok := relationalOppositeOf[originalOp] + if ok { + binExp.Op = opposite + result = astutils.GoFmt(binExp) // replace initial result by a simpler one + binExp.Op = originalOp + } + } + } + + return result +} + +// replacementForAssignmentStmt returns a replacement statement != "" +// iff both then and else statements are of the form = +// If the replacement != "" then the second return value is the Boolean value +// of in the then-side assignment, false otherwise. +func (w *lintUnnecessaryIf) replacementForAssignmentStmt(thenStmt *ast.AssignStmt, elseStmts []ast.Stmt) (replacement string, thenBool bool) { + thenBoolStr, ok := w.isSingleBooleanLiteral(thenStmt.Rhs) + if !ok { + return "", false + } + + thenLHS := astutils.GoFmt(thenStmt.Lhs[0]) + + elseStmt, ok := elseStmts[0].(*ast.AssignStmt) + if !ok { + return "", false + } + + elseLHS := astutils.GoFmt(elseStmt.Lhs[0]) + if thenLHS != elseLHS { + return "", false + } + + _, ok = w.isSingleBooleanLiteral(elseStmt.Rhs) + if !ok { + return "", false + } + + return fmt.Sprintf("%s %s", thenLHS, thenStmt.Tok.String()), thenBoolStr == "true" +} + +// replacementForReturnStmt returns a replacement statement != "" +// iff both then and else statements are of the form return +// If the replacement != "" then the second return value is the string representation +// of in the then-side assignment, "" otherwise. +func (w *lintUnnecessaryIf) replacementForReturnStmt(thenStmt *ast.ReturnStmt, elseStmts []ast.Stmt) (replacement string, thenBool bool) { + thenBoolStr, ok := w.isSingleBooleanLiteral(thenStmt.Results) + if !ok { + return "", false + } + + elseStmt, ok := elseStmts[0].(*ast.ReturnStmt) + if !ok { + return "", false + } + + _, ok = w.isSingleBooleanLiteral(elseStmt.Results) + if !ok { + return "", false + } + + return "return", thenBoolStr == "true" +} + +// isSingleBooleanLiteral returns the string representation of and true +// if the given list of expressions has exactly one element and that element is a bool literal (true or false), +// otherwise it returns "" and false. +func (*lintUnnecessaryIf) isSingleBooleanLiteral(exprs []ast.Expr) (string, bool) { + if len(exprs) != 1 { + return "", false + } + + ident, ok := exprs[0].(*ast.Ident) + if !ok { + return "", false + } + + return ident.Name, (ident.Name == "true" || ident.Name == "false") +} diff --git a/test/unnecessary_if_test.go b/test/unnecessary_if_test.go new file mode 100644 index 0000000..f6712b0 --- /dev/null +++ b/test/unnecessary_if_test.go @@ -0,0 +1,12 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/lint" + "github.com/mgechev/revive/rule" +) + +func TestUnnecessaryIf(t *testing.T) { + testRule(t, "unnecessary_if", &rule.UnnecessaryIfRule{}, &lint.RuleConfig{}) +} diff --git a/testdata/unnecessary_if.go b/testdata/unnecessary_if.go new file mode 100644 index 0000000..4862449 --- /dev/null +++ b/testdata/unnecessary_if.go @@ -0,0 +1,153 @@ +package fixtures + +func unnecessaryIf() bool { + var cond bool + var id bool + + // test return replacements + if cond { // MATCH /replace this conditional by: return cond/ + return true + } else { + return false + } + + if cond { // MATCH /replace this conditional by: return !(cond)/ + return false + } else { + return true + } + + // test assignment replacements + if cond { // MATCH /replace this conditional by: id = cond/ + id = true + } else { + id = false + } + + if cond { // MATCH /replace this conditional by: id = !(cond)/ + id = false + } else { + id = true + } + + // test suggestions for (in)equalities + //// assignments + if cond == id { // MATCH /replace this conditional by: id = cond == id/ + id = true + } else { + id = false + } + + if cond == id { // MATCH /replace this conditional by: id = cond != id/ + id = false + } else { + id = true + } + + if cond != id { // MATCH /replace this conditional by: id = cond != id/ + id = true + } else { + id = false + } + + if cond != id { // MATCH /replace this conditional by: id = cond == id/ + id = false + } else { + id = true + } + + //// return + if cond == id { // MATCH /replace this conditional by: return cond == id/ + return true + } else { + return false + } + + if cond == id { // MATCH /replace this conditional by: return cond != id/ + return false + } else { + return true + } + + if cond != id { // MATCH /replace this conditional by: return cond != id/ + return true + } else { + return false + } + + if cond != id { // MATCH /replace this conditional by: return cond == id/ + return false + } else { + return true + } + + //// assignments + if cond <= id { // MATCH /replace this conditional by: id = cond <= id/ + id = true + } else { + id = false + } + + if cond <= id { // MATCH /replace this conditional by: id = cond > id/ + id = false + } else { + id = true + } + + if cond >= id { // MATCH /replace this conditional by: id = cond >= id/ + id = true + } else { + id = false + } + + if cond >= id { // MATCH /replace this conditional by: id = cond < id/ + id = false + } else { + id = true + } + + if cond > id { // MATCH /replace this conditional by: id = cond > id/ + id = true + } else { + id = false + } + + if cond > id { // MATCH /replace this conditional by: id = cond <= id/ + id = false + } else { + id = true + } + + if cond < id { // MATCH /replace this conditional by: id = cond < id/ + id = true + } else { + id = false + } + + if cond < id { // MATCH /replace this conditional by: id = cond >= id/ + id = false + } else { + id = true + } + + if (something > 0) && (!id) || (something+10 <= 0) { // MATCH /replace this conditional by: id = !((something > 0) && (!id) || (something+10 <= 0))/ + id = false + } else { + id = true + } + + // conditionals with initialization + if cond := false; cond { + return true + } else { + return false + } + + if cond := false; cond { + id = true + } else { + id = false + } + + return id == id +}