Repository: mgechev/revive (go)

Issue:
new rule: late-return
This new rule is inspired by [this question on twitter](https://twitter.com/francesc/status/1255620804705808384): 

> Is there a static analyzer for #golang that will warn about possible early returns?
```go
if foo {
 .... 
 return a
} else {
  return b
}
```
Should be:
```go
if ! foo {
  return b
}
...
return a
```
Contrary to other `revive` rules this is more a style-related rule. I'm not sure if it has its place in `revive` 

Pull request description:
Late return rule
Closes #404 

Patch:
diff --git a/README.md b/README.md
index b205a1d..3e51462 100644
--- a/README.md
+++ b/README.md
@@ -349,7 +349,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a
 | [`unhandled-error`](./RULES_DESCRIPTIONS.md#unhandled-error)   | []string   | Warns on unhandled errors returned by funcion calls    |    no    |  yes   |
 | [`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   |
-
+| [`early-return`](./RULES_DESCRIPTIONS.md#early-return)          |  n/a   | Spots if-then-else statements that can be refactored to simplify code reading            |    no    |  no   |
 ## Configurable rules
 
 Here you can find how you can configure some of the existing rules:
diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md
index 932f9cb..901543c 100644
--- a/RULES_DESCRIPTIONS.md
+++ b/RULES_DESCRIPTIONS.md
@@ -21,6 +21,7 @@ List of all available rules.
   - [deep-exit](#deep-exit)
   - [dot-imports](#dot-imports)
   - [duplicated-imports](#duplicated-imports)
+  - [early-return](#early-return)
   - [empty-block](#empty-block)
   - [empty-lines](#empty-lines)
   - [error-naming](#error-naming)
@@ -203,6 +204,29 @@ _Description_: It is possible to unintentionally import the same package twice.
 
 _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
+```go
+if cond {
+  // do something 
+} else {
+  // do other thing
+  return ...
+}
+```
+that can be rewritten into more idiomatic:
+```go
+if ! cond {
+  // do other thing
+  return ... 
+}
+
+// do something
+```
+
+_Configuration_: N/A
+
 ## empty-block
 
 _Description_: Empty blocks make code less readable and could be a symptom of a bug or unfinished refactoring.
diff --git a/config.go b/config.go
index f8a117d..95543b8 100644
--- a/config.go
+++ b/config.go
@@ -83,6 +83,7 @@ var allRules = append([]lint.Rule{
 	&rule.UnhandledErrorRule{},
 	&rule.CognitiveComplexityRule{},
 	&rule.StringOfIntRule{},
+	&rule.EarlyReturnRule{},
 }, defaultRules...)
 
 var allFormatters = []lint.Formatter{
diff --git a/go.mod b/go.mod
index abddc3f..b3305af 100644
--- a/go.mod
+++ b/go.mod
@@ -10,5 +10,5 @@ require (
 	github.com/mitchellh/go-homedir v1.1.0
 	github.com/olekukonko/tablewriter v0.0.4
 	github.com/pkg/errors v0.9.1
-	golang.org/x/tools v0.0.0-20200504152539-33427f1b0364
+	golang.org/x/tools v0.0.0-20200505023115-26f46d2f7ef8
 )
diff --git a/go.sum b/go.sum
index 19b75d8..7aaecd1 100644
--- a/go.sum
+++ b/go.sum
@@ -48,6 +48,8 @@ golang.org/x/tools v0.0.0-20200502202811-ed308ab3e770 h1:M9Fif0OxNji8w+HvmhVQ8KJ
 golang.org/x/tools v0.0.0-20200502202811-ed308ab3e770/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE=
 golang.org/x/tools v0.0.0-20200504152539-33427f1b0364 h1:B3dlRmcq+I6Bd22nHKKa7E+r0/6mLEoJQa75WjfILUE=
 golang.org/x/tools v0.0.0-20200504152539-33427f1b0364/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE=
+golang.org/x/tools v0.0.0-20200505023115-26f46d2f7ef8 h1:BMFHd4OFnFtWX46Xj4DN6vvT1btiBxyq+s0orYBqcQY=
+golang.org/x/tools v0.0.0-20200505023115-26f46d2f7ef8/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE=
 golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
 golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
 golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 h1:E7g+9GITq07hpfrRu66IVDexMakfv52eLZ2CXBWiKr4=
diff --git a/rule/early-return.go b/rule/early-return.go
new file mode 100644
index 0000000..ffb568a
--- /dev/null
+++ b/rule/early-return.go
@@ -0,0 +1,78 @@
+package rule
+
+import (
+	"go/ast"
+
+	"github.com/mgechev/revive/lint"
+)
+
+// EarlyReturnRule lints given else constructs.
+type EarlyReturnRule struct{}
+
+// Apply applies the rule to given file.
+func (r *EarlyReturnRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
+	var failures []lint.Failure
+
+	onFailure := func(failure lint.Failure) {
+		failures = append(failures, failure)
+	}
+
+	w := lintEarlyReturnRule{onFailure: onFailure}
+	ast.Walk(w, file.AST)
+	return failures
+}
+
+// Name returns the rule name.
+func (r *EarlyReturnRule) Name() string {
+	return "early-return"
+}
+
+type lintEarlyReturnRule struct {
+	onFailure func(lint.Failure)
+}
+
+func (w lintEarlyReturnRule) Visit(node ast.Node) ast.Visitor {
+	switch n := node.(type) {
+	case *ast.IfStmt:
+		if n.Else == nil {
+			// no else branch
+			return w
+		}
+
+		elseBlock, ok := n.Else.(*ast.BlockStmt)
+		if !ok {
+			// is if-else-if
+			return w
+		}
+
+		lenElseBlock := len(elseBlock.List)
+		if lenElseBlock < 1 {
+			// empty else block, continue (there is another rule that warns on empty blocks)
+			return w
+		}
+
+		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 }",
+			})
+
+			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 } ...",
+			})
+		}
+	}
+
+	return w
+}


Test patch:
diff --git a/test/early-return_test.go b/test/early-return_test.go
new file mode 100644
index 0000000..0f1b63b
--- /dev/null
+++ b/test/early-return_test.go
@@ -0,0 +1,12 @@
+package test
+
+import (
+	"testing"
+
+	"github.com/mgechev/revive/rule"
+)
+
+// TestEarlyReturn tests early-return rule.
+func TestEarlyReturn(t *testing.T) {
+	testRule(t, "early-return", &rule.EarlyReturnRule{})
+}
diff --git a/testdata/early-return.go b/testdata/early-return.go
new file mode 100644
index 0000000..bb4b8f2
--- /dev/null
+++ b/testdata/early-return.go
@@ -0,0 +1,62 @@
+// Test of empty-blocks.
+
+package fixtures
+
+func earlyRet() bool {
+	if cond { //   MATCH /if c {...} else {... return } can be simplified to if !c { ... return } .../
+		println()
+		println()
+		println()
+	} else {
+		return false
+	}
+
+	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 }/
+	} else {
+		return false
+	}
+
+	if cond {
+		println()
+	} else 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 } .../
+		println()
+	} else {
+		return false
+	}
+
+	// Case already covered by golint
+	if cond {
+		return true
+	} else {
+		return false
+	}
+
+	if cond { //MATCH /if c {...} else {... return } can be simplified to if !c { ... return } .../
+		println()
+		println()
+		println()
+	} else {
+		return false
+	}
+
+	if cond {
+		println()
+		println()
+		println()
+	} else {
+		println()
+	}
+}