diff --git a/README.md b/README.md index 56f4b36..9bbc0b0 100644 --- a/README.md +++ b/README.md @@ -487,7 +487,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a | [`cognitive-complexity`](./RULES_DESCRIPTIONS.md#cognitive-complexity) | int | Sets restriction for maximum Cognitive complexity. | no | no | | [`string-of-int`](./RULES_DESCRIPTIONS.md#string-of-int) | n/a | Warns on suspicious casts from int to string | no | yes | | [`string-format`](./RULES_DESCRIPTIONS.md#string-format) | map | Warns on specific string literals that fail one or more user-configured regular expressions | no | no | -| [`early-return`](./RULES_DESCRIPTIONS.md#early-return) | n/a | Spots if-then-else statements that can be refactored to simplify code reading | no | no | +| [`early-return`](./RULES_DESCRIPTIONS.md#early-return) | n/a | Spots if-then-else statements where the predicate may be inverted to reduce nesting | no | no | | [`unconditional-recursion`](./RULES_DESCRIPTIONS.md#unconditional-recursion) | n/a | Warns on function calls that will lead to (direct) infinite recursion | no | no | | [`identical-branches`](./RULES_DESCRIPTIONS.md#identical-branches) | n/a | Spots if-then-else statements with identical `then` and `else` branches | no | no | | [`defer`](./RULES_DESCRIPTIONS.md#defer) | map | Warns on some [defer gotchas](https://blog.learngoprogramming.com/5-gotchas-of-defer-in-go-golang-part-iii-36a1ab3d6ef1) | no | no | diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md index 986ba7a..3865f86 100644 --- a/RULES_DESCRIPTIONS.md +++ b/RULES_DESCRIPTIONS.md @@ -288,7 +288,7 @@ _Configuration_: N/A ## early-return -_Description_: In GO it is idiomatic to minimize nesting statements, a typical example is to avoid if-then-else constructions. This rule spots constructions like +_Description_: In Go it is idiomatic to minimize nesting statements, a typical example is to avoid if-then-else constructions. This rule spots constructions like ```go if cond { // do something @@ -297,7 +297,7 @@ if cond { return ... } ``` -that can be rewritten into more idiomatic: +where the `if` condition may be inverted in order to reduce nesting: ```go if ! cond { // do other thing diff --git a/rule/early-return.go b/rule/early-return.go index bfbf671..ed0fcfa 100644 --- a/rule/early-return.go +++ b/rule/early-return.go @@ -1,12 +1,15 @@ package rule import ( + "fmt" "go/ast" + "go/token" "github.com/mgechev/revive/lint" ) -// EarlyReturnRule lints given else constructs. +// EarlyReturnRule finds opportunities to reduce nesting by inverting +// the condition of an "if" block. type EarlyReturnRule struct{} // Apply applies the rule to given file. @@ -32,47 +35,142 @@ type lintEarlyReturnRule struct { } func (w lintEarlyReturnRule) Visit(node ast.Node) ast.Visitor { - switch n := node.(type) { + ifStmt, ok := node.(*ast.IfStmt) + if !ok { + return w + } + + w.visitIf(ifStmt, false, false) + return nil +} + +func (w lintEarlyReturnRule) visitIf(ifStmt *ast.IfStmt, hasNonReturnBranch, hasIfInitializer bool) { + // look for other if-else chains nested inside this if { } block + ast.Walk(w, ifStmt.Body) + + if ifStmt.Else == nil { + // no else branch + return + } + + if as, ok := ifStmt.Init.(*ast.AssignStmt); ok && as.Tok == token.DEFINE { + hasIfInitializer = true + } + bodyFlow := w.branchFlow(ifStmt.Body) + + switch elseBlock := ifStmt.Else.(type) { case *ast.IfStmt: - if n.Else == nil { - // no else branch - return w + if bodyFlow.canFlowIntoNext() { + hasNonReturnBranch = true + } + w.visitIf(elseBlock, hasNonReturnBranch, hasIfInitializer) + + case *ast.BlockStmt: + // look for other if-else chains nested inside this else { } block + ast.Walk(w, elseBlock) + + if hasNonReturnBranch && bodyFlow != branchFlowEmpty { + // if we de-indent this block then a previous branch + // might flow into it, affecting program behaviour + return } - elseBlock, ok := n.Else.(*ast.BlockStmt) - if !ok { - // is if-else-if - return w + if !bodyFlow.canFlowIntoNext() { + // avoid overlapping with superfluous-else + return } - lenElseBlock := len(elseBlock.List) - if lenElseBlock < 1 { - // empty else block, continue (there is another rule that warns on empty blocks) - return w - } + elseFlow := w.branchFlow(elseBlock) + if !elseFlow.canFlowIntoNext() { + failMsg := fmt.Sprintf("if c {%[1]s } else {%[2]s } can be simplified to if !c {%[2]s }%[1]s", + bodyFlow, elseFlow) + + if hasIfInitializer { + // if statement has a := initializer, so we might need to move the assignment + // onto its own line in case the body references it + failMsg += " (move short variable declaration to its own line if necessary)" + } - lenThenBlock := len(n.Body.List) - if lenThenBlock < 1 { - // then block is empty thus the stmt can be simplified w.onFailure(lint.Failure{ Confidence: 1, - Node: n, - Failure: "if c { } else {... return} can be simplified to if !c { ... return }", + Node: ifStmt, + Failure: failMsg, }) - - return w } - _, lastThenStmtIsReturn := n.Body.List[lenThenBlock-1].(*ast.ReturnStmt) - _, lastElseStmtIsReturn := elseBlock.List[lenElseBlock-1].(*ast.ReturnStmt) - if lastElseStmtIsReturn && !lastThenStmtIsReturn { - w.onFailure(lint.Failure{ - Confidence: 1, - Node: n, - Failure: "if c {...} else {... return } can be simplified to if !c { ... return } ...", - }) + default: + panic("invalid node type for else") + } +} + +type branchFlowKind int + +const ( + branchFlowEmpty branchFlowKind = iota + branchFlowReturn + branchFlowPanic + branchFlowContinue + branchFlowBreak + branchFlowGoto + branchFlowRegular +) + +func (w lintEarlyReturnRule) branchFlow(block *ast.BlockStmt) branchFlowKind { + blockLen := len(block.List) + if blockLen == 0 { + return branchFlowEmpty + } + + switch stmt := block.List[blockLen-1].(type) { + case *ast.ReturnStmt: + return branchFlowReturn + case *ast.BlockStmt: + return w.branchFlow(stmt) + case *ast.BranchStmt: + switch stmt.Tok { + case token.BREAK: + return branchFlowBreak + case token.CONTINUE: + return branchFlowContinue + case token.GOTO: + return branchFlowGoto + } + case *ast.ExprStmt: + if call, ok := stmt.X.(*ast.CallExpr); ok && isIdent(call.Fun, "panic") { + return branchFlowPanic } } - return w + return branchFlowRegular +} + +// Whether this branch's control can flow into the next statement following the if-else chain +func (k branchFlowKind) canFlowIntoNext() bool { + switch k { + case branchFlowReturn, branchFlowPanic, branchFlowContinue, branchFlowBreak, branchFlowGoto: + return false + default: + return true + } +} + +func (k branchFlowKind) String() string { + switch k { + case branchFlowEmpty: + return "" + case branchFlowReturn: + return " ... return" + case branchFlowPanic: + return " ... panic()" + case branchFlowContinue: + return " ... continue" + case branchFlowBreak: + return " ... break" + case branchFlowGoto: + return " ... goto" + case branchFlowRegular: + return " ..." + default: + panic("invalid kind") + } } diff --git a/testdata/early-return.go b/testdata/early-return.go index bb4b8f2..212b6c9 100644 --- a/testdata/early-return.go +++ b/testdata/early-return.go @@ -3,7 +3,7 @@ package fixtures func earlyRet() bool { - if cond { // MATCH /if c {...} else {... return } can be simplified to if !c { ... return } .../ + if cond { // MATCH /if c { ... } else { ... return } can be simplified to if !c { ... return } .../ println() println() println() @@ -11,27 +11,28 @@ func earlyRet() bool { return false } - if cond { //MATCH /if c {...} else {... return } can be simplified to if !c { ... return } .../ + if cond { //MATCH /if c { ... } else { ... return } can be simplified to if !c { ... return } .../ println() } else { return false } - if cond { //MATCH /if c { } else {... return} can be simplified to if !c { ... return }/ + if cond { //MATCH /if c { } else { ... return } can be simplified to if !c { ... return }/ } else { return false } if cond { println() - } else if cond { //MATCH /if c { } else {... return} can be simplified to if !c { ... return }/ + } else if cond { //MATCH /if c { } else { ... return } can be simplified to if !c { ... return }/ } else { return false } + // the first branch does not return, so we can't reduce nesting here if cond { println() - } else if cond { //MATCH /if c {...} else {... return } can be simplified to if !c { ... return } .../ + } else if cond { println() } else { return false @@ -44,7 +45,7 @@ func earlyRet() bool { return false } - if cond { //MATCH /if c {...} else {... return } can be simplified to if !c { ... return } .../ + if cond { //MATCH /if c { ... } else { ... return } can be simplified to if !c { ... return } .../ println() println() println() @@ -59,4 +60,68 @@ func earlyRet() bool { } else { println() } + + if cond { + if cond { //MATCH /if c { ... } else { ... return } can be simplified to if !c { ... return } .../ + println() + } else { + return false + } + } + + if cond { + println() + } else { + if cond { //MATCH /if c { ... } else { ... return } can be simplified to if !c { ... return } .../ + println() + } else { + return false + } + } + + if cond { + println() + } else if cond { + println() + } else { + if cond { //MATCH /if c { ... } else { ... return } can be simplified to if !c { ... return } .../ + println() + } else { + return false + } + } + + for { + if cond { //MATCH /if c { ... } else { ... continue } can be simplified to if !c { ... continue } .../ + println() + } else { + continue + } + } + + for { + if cond { //MATCH /if c { ... } else { ... break } can be simplified to if !c { ... break } .../ + println() + } else { + break + } + } + + if cond { //MATCH /if c { ... } else { ... panic() } can be simplified to if !c { ... panic() } .../ + println() + } else { + panic("!") + } + + if cond { //MATCH /if c { ... } else { ... goto } can be simplified to if !c { ... goto } .../ + println() + } else { + goto X + } + + if x, ok := foo(); ok { //MATCH /if c { ... } else { ... return } can be simplified to if !c { ... return } ... (move short variable declaration to its own line if necessary)/ + println(x) + } else { + return false + } }