Skip to content

Commit b87d391

Browse files
mdelahmdelah
authored
Fix early-return false positive and other tweaks (#776)
1 parent d5d9da1 commit b87d391

File tree

4 files changed

+201
-38
lines changed

4 files changed

+201
-38
lines changed

‎README.md

+1-1
Original file line numberDiff line numberDiff line change
@@ -487,7 +487,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a
487487
| [`cognitive-complexity`](./RULES_DESCRIPTIONS.md#cognitive-complexity) | int | Sets restriction for maximum Cognitive complexity. | no | no |
488488
| [`string-of-int`](./RULES_DESCRIPTIONS.md#string-of-int) | n/a | Warns on suspicious casts from int to string | no | yes |
489489
| [`string-format`](./RULES_DESCRIPTIONS.md#string-format) | map | Warns on specific string literals that fail one or more user-configured regular expressions | no | no |
490-
| [`early-return`](./RULES_DESCRIPTIONS.md#early-return) | n/a | Spots if-then-else statements that can be refactored to simplify code reading | no | no |
490+
| [`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 |
491491
| [`unconditional-recursion`](./RULES_DESCRIPTIONS.md#unconditional-recursion) | n/a | Warns on function calls that will lead to (direct) infinite recursion | no | no |
492492
| [`identical-branches`](./RULES_DESCRIPTIONS.md#identical-branches) | n/a | Spots if-then-else statements with identical `then` and `else` branches | no | no |
493493
| [`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 |

‎RULES_DESCRIPTIONS.md

+2-2
Original file line numberDiff line numberDiff line change
@@ -288,7 +288,7 @@ _Configuration_: N/A
288288

289289
## early-return
290290

291-
_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
291+
_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
292292
```go
293293
if cond {
294294
// do something
@@ -297,7 +297,7 @@ if cond {
297297
return ...
298298
}
299299
```
300-
that can be rewritten into more idiomatic:
300+
where the `if` condition may be inverted in order to reduce nesting:
301301
```go
302302
if ! cond {
303303
// do other thing

‎rule/early-return.go

+127-29
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,15 @@
11
package rule
22

33
import (
4+
"fmt"
45
"go/ast"
6+
"go/token"
57

68
"github.com/mgechev/revive/lint"
79
)
810

9-
// EarlyReturnRule lints given else constructs.
11+
// EarlyReturnRule finds opportunities to reduce nesting by inverting
12+
// the condition of an "if" block.
1013
type EarlyReturnRule struct{}
1114

1215
// Apply applies the rule to given file.
@@ -32,47 +35,142 @@ type lintEarlyReturnRule struct {
3235
}
3336

3437
func (w lintEarlyReturnRule) Visit(node ast.Node) ast.Visitor {
35-
switch n := node.(type) {
38+
ifStmt, ok := node.(*ast.IfStmt)
39+
if !ok {
40+
return w
41+
}
42+
43+
w.visitIf(ifStmt, false, false)
44+
return nil
45+
}
46+
47+
func (w lintEarlyReturnRule) visitIf(ifStmt *ast.IfStmt, hasNonReturnBranch, hasIfInitializer bool) {
48+
// look for other if-else chains nested inside this if { } block
49+
ast.Walk(w, ifStmt.Body)
50+
51+
if ifStmt.Else == nil {
52+
// no else branch
53+
return
54+
}
55+
56+
if as, ok := ifStmt.Init.(*ast.AssignStmt); ok && as.Tok == token.DEFINE {
57+
hasIfInitializer = true
58+
}
59+
bodyFlow := w.branchFlow(ifStmt.Body)
60+
61+
switch elseBlock := ifStmt.Else.(type) {
3662
case *ast.IfStmt:
37-
if n.Else == nil {
38-
// no else branch
39-
return w
63+
if bodyFlow.canFlowIntoNext() {
64+
hasNonReturnBranch = true
4065
}
66+
w.visitIf(elseBlock, hasNonReturnBranch, hasIfInitializer)
67+
68+
case *ast.BlockStmt:
69+
// look for other if-else chains nested inside this else { } block
70+
ast.Walk(w, elseBlock)
4171

42-
elseBlock, ok := n.Else.(*ast.BlockStmt)
43-
if !ok {
44-
// is if-else-if
45-
return w
72+
if hasNonReturnBranch && bodyFlow != branchFlowEmpty {
73+
// if we de-indent this block then a previous branch
74+
// might flow into it, affecting program behaviour
75+
return
4676
}
4777

48-
lenElseBlock := len(elseBlock.List)
49-
if lenElseBlock < 1 {
50-
// empty else block, continue (there is another rule that warns on empty blocks)
51-
return w
78+
if !bodyFlow.canFlowIntoNext() {
79+
// avoid overlapping with superfluous-else
80+
return
5281
}
5382

54-
lenThenBlock := len(n.Body.List)
55-
if lenThenBlock < 1 {
56-
// then block is empty thus the stmt can be simplified
57-
w.onFailure(lint.Failure{
58-
Confidence: 1,
59-
Node: n,
60-
Failure: "if c { } else {... return} can be simplified to if !c { ... return }",
61-
})
83+
elseFlow := w.branchFlow(elseBlock)
84+
if !elseFlow.canFlowIntoNext() {
85+
failMsg := fmt.Sprintf("if c {%[1]s } else {%[2]s } can be simplified to if !c {%[2]s }%[1]s",
86+
bodyFlow, elseFlow)
6287

63-
return w
64-
}
88+
if hasIfInitializer {
89+
// if statement has a := initializer, so we might need to move the assignment
90+
// onto its own line in case the body references it
91+
failMsg += " (move short variable declaration to its own line if necessary)"
92+
}
6593

66-
_, lastThenStmtIsReturn := n.Body.List[lenThenBlock-1].(*ast.ReturnStmt)
67-
_, lastElseStmtIsReturn := elseBlock.List[lenElseBlock-1].(*ast.ReturnStmt)
68-
if lastElseStmtIsReturn && !lastThenStmtIsReturn {
6994
w.onFailure(lint.Failure{
7095
Confidence: 1,
71-
Node: n,
72-
Failure: "if c {...} else {... return } can be simplified to if !c { ... return } ...",
96+
Node: ifStmt,
97+
Failure: failMsg,
7398
})
7499
}
100+
101+
default:
102+
panic("invalid node type for else")
75103
}
104+
}
76105

77-
return w
106+
type branchFlowKind int
107+
108+
const (
109+
branchFlowEmpty branchFlowKind = iota
110+
branchFlowReturn
111+
branchFlowPanic
112+
branchFlowContinue
113+
branchFlowBreak
114+
branchFlowGoto
115+
branchFlowRegular
116+
)
117+
118+
func (w lintEarlyReturnRule) branchFlow(block *ast.BlockStmt) branchFlowKind {
119+
blockLen := len(block.List)
120+
if blockLen == 0 {
121+
return branchFlowEmpty
122+
}
123+
124+
switch stmt := block.List[blockLen-1].(type) {
125+
case *ast.ReturnStmt:
126+
return branchFlowReturn
127+
case *ast.BlockStmt:
128+
return w.branchFlow(stmt)
129+
case *ast.BranchStmt:
130+
switch stmt.Tok {
131+
case token.BREAK:
132+
return branchFlowBreak
133+
case token.CONTINUE:
134+
return branchFlowContinue
135+
case token.GOTO:
136+
return branchFlowGoto
137+
}
138+
case *ast.ExprStmt:
139+
if call, ok := stmt.X.(*ast.CallExpr); ok && isIdent(call.Fun, "panic") {
140+
return branchFlowPanic
141+
}
142+
}
143+
144+
return branchFlowRegular
145+
}
146+
147+
// Whether this branch's control can flow into the next statement following the if-else chain
148+
func (k branchFlowKind) canFlowIntoNext() bool {
149+
switch k {
150+
case branchFlowReturn, branchFlowPanic, branchFlowContinue, branchFlowBreak, branchFlowGoto:
151+
return false
152+
default:
153+
return true
154+
}
155+
}
156+
157+
func (k branchFlowKind) String() string {
158+
switch k {
159+
case branchFlowEmpty:
160+
return ""
161+
case branchFlowReturn:
162+
return " ... return"
163+
case branchFlowPanic:
164+
return " ... panic()"
165+
case branchFlowContinue:
166+
return " ... continue"
167+
case branchFlowBreak:
168+
return " ... break"
169+
case branchFlowGoto:
170+
return " ... goto"
171+
case branchFlowRegular:
172+
return " ..."
173+
default:
174+
panic("invalid kind")
175+
}
78176
}

‎testdata/early-return.go

+71-6
Original file line numberDiff line numberDiff line change
@@ -3,35 +3,36 @@
33
package fixtures
44

55
func earlyRet() bool {
6-
if cond { // MATCH /if c {...} else {... return } can be simplified to if !c { ... return } .../
6+
if cond { // MATCH /if c { ... } else { ... return } can be simplified to if !c { ... return } .../
77
println()
88
println()
99
println()
1010
} else {
1111
return false
1212
}
1313

14-
if cond { //MATCH /if c {...} else {... return } can be simplified to if !c { ... return } .../
14+
if cond { //MATCH /if c { ... } else { ... return } can be simplified to if !c { ... return } .../
1515
println()
1616
} else {
1717
return false
1818
}
1919

20-
if cond { //MATCH /if c { } else {... return} can be simplified to if !c { ... return }/
20+
if cond { //MATCH /if c { } else { ... return } can be simplified to if !c { ... return }/
2121
} else {
2222
return false
2323
}
2424

2525
if cond {
2626
println()
27-
} else if cond { //MATCH /if c { } else {... return} can be simplified to if !c { ... return }/
27+
} else if cond { //MATCH /if c { } else { ... return } can be simplified to if !c { ... return }/
2828
} else {
2929
return false
3030
}
3131

32+
// the first branch does not return, so we can't reduce nesting here
3233
if cond {
3334
println()
34-
} else if cond { //MATCH /if c {...} else {... return } can be simplified to if !c { ... return } .../
35+
} else if cond {
3536
println()
3637
} else {
3738
return false
@@ -44,7 +45,7 @@ func earlyRet() bool {
4445
return false
4546
}
4647

47-
if cond { //MATCH /if c {...} else {... return } can be simplified to if !c { ... return } .../
48+
if cond { //MATCH /if c { ... } else { ... return } can be simplified to if !c { ... return } .../
4849
println()
4950
println()
5051
println()
@@ -59,4 +60,68 @@ func earlyRet() bool {
5960
} else {
6061
println()
6162
}
63+
64+
if cond {
65+
if cond { //MATCH /if c { ... } else { ... return } can be simplified to if !c { ... return } .../
66+
println()
67+
} else {
68+
return false
69+
}
70+
}
71+
72+
if cond {
73+
println()
74+
} else {
75+
if cond { //MATCH /if c { ... } else { ... return } can be simplified to if !c { ... return } .../
76+
println()
77+
} else {
78+
return false
79+
}
80+
}
81+
82+
if cond {
83+
println()
84+
} else if cond {
85+
println()
86+
} else {
87+
if cond { //MATCH /if c { ... } else { ... return } can be simplified to if !c { ... return } .../
88+
println()
89+
} else {
90+
return false
91+
}
92+
}
93+
94+
for {
95+
if cond { //MATCH /if c { ... } else { ... continue } can be simplified to if !c { ... continue } .../
96+
println()
97+
} else {
98+
continue
99+
}
100+
}
101+
102+
for {
103+
if cond { //MATCH /if c { ... } else { ... break } can be simplified to if !c { ... break } .../
104+
println()
105+
} else {
106+
break
107+
}
108+
}
109+
110+
if cond { //MATCH /if c { ... } else { ... panic() } can be simplified to if !c { ... panic() } .../
111+
println()
112+
} else {
113+
panic("!")
114+
}
115+
116+
if cond { //MATCH /if c { ... } else { ... goto } can be simplified to if !c { ... goto } .../
117+
println()
118+
} else {
119+
goto X
120+
}
121+
122+
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)/
123+
println(x)
124+
} else {
125+
return false
126+
}
62127
}

0 commit comments

Comments
 (0)