From 67c83886d7fe3a70fd0bce82fdf828e5a0a55d87 Mon Sep 17 00:00:00 2001 From: SalvadorC Date: Fri, 8 May 2020 20:14:21 +0200 Subject: [PATCH] Late return rule (#406) * fisrt working version of late-return rule * late-update: adds doc * renames to early-return * fix rule failure condition * fix alphabetical sorting of early-return --- README.md | 2 +- RULES_DESCRIPTIONS.md | 24 ++++++++++++ config.go | 1 + rule/early-return.go | 78 +++++++++++++++++++++++++++++++++++++++ test/early-return_test.go | 12 ++++++ testdata/early-return.go | 62 +++++++++++++++++++++++++++++++ 6 files changed, 178 insertions(+), 1 deletion(-) create mode 100644 rule/early-return.go create mode 100644 test/early-return_test.go create mode 100644 testdata/early-return.go 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/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 +} 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() + } +}