From 1da965b65f8a0e3aa83cd9d6d38153a32f308960 Mon Sep 17 00:00:00 2001 From: SalvadorC Date: Mon, 11 May 2020 02:43:56 +0200 Subject: [PATCH] =?UTF-8?q?fix=20issue=20386:=20Incorrectly=20identifies?= =?UTF-8?q?=20channel=20draining=20as=20"empty=20code=E2=80=A6=20(#415)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix issue 386: Incorrectly identifies channel draining as "empty code block" * updates doc of empty-block rule --- README.md | 2 +- rule/empty-block.go | 64 ++++++++++++++++++----------------------- testdata/empty-block.go | 21 ++++++++++---- 3 files changed, 44 insertions(+), 43 deletions(-) diff --git a/README.md b/README.md index 7c6809c..e0c471f 100644 --- a/README.md +++ b/README.md @@ -316,7 +316,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a | [`cyclomatic`](./RULES_DESCRIPTIONS.md#cyclomatic) | int | Sets restriction for maximum Cyclomatic complexity. | no | no | | [`max-public-structs`](./RULES_DESCRIPTIONS.md#max-public-structs) | int | The maximum number of public structs in a file. | no | no | | [`file-header`](./RULES_DESCRIPTIONS.md#file-header) | string | Header which each file should have. | no | no | -| [`empty-block`](./RULES_DESCRIPTIONS.md#empty-block) | n/a | Warns on empty code blocks | no | no | +| [`empty-block`](./RULES_DESCRIPTIONS.md#empty-block) | n/a | Warns on empty code blocks | no | yes | | [`superfluous-else`](./RULES_DESCRIPTIONS.md#superfluous-else) | n/a | Prevents redundant else statements (extends [`indent-error-flow`](./RULES_DESCRIPTIONS.md#indent-error-flow)) | no | no | | [`confusing-naming`](./RULES_DESCRIPTIONS.md#confusing-naming) | n/a | Warns on methods with names that differ only by capitalization | no | no | | [`get-return`](./RULES_DESCRIPTIONS.md#get-return) | n/a | Warns on getters that do not yield any result | no | no | diff --git a/rule/empty-block.go b/rule/empty-block.go index 7861394..4eb0ce8 100644 --- a/rule/empty-block.go +++ b/rule/empty-block.go @@ -2,6 +2,7 @@ package rule import ( "go/ast" + "go/types" "github.com/mgechev/revive/lint" ) @@ -17,7 +18,12 @@ func (r *EmptyBlockRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure failures = append(failures, failure) } - w := lintEmptyBlock{make([]*ast.BlockStmt, 0), onFailure} + err := file.Pkg.TypeCheck() + if err != nil { + panic("unable to type check " + file.Name + ":" + err.Error()) + } + + w := lintEmptyBlock{make(map[*ast.BlockStmt]bool, 0), file.Pkg, onFailure} ast.Walk(w, file.AST) return failures } @@ -28,49 +34,35 @@ func (r *EmptyBlockRule) Name() string { } type lintEmptyBlock struct { - ignore []*ast.BlockStmt + ignore map[*ast.BlockStmt]bool + pkg *lint.Package onFailure func(lint.Failure) } func (w lintEmptyBlock) Visit(node ast.Node) ast.Visitor { - fd, ok := node.(*ast.FuncDecl) - if ok { - w.ignore = append(w.ignore, fd.Body) + switch n := node.(type) { + case *ast.FuncDecl: + w.ignore[n.Body] = true return w - } - - fl, ok := node.(*ast.FuncLit) - if ok { - w.ignore = append(w.ignore, fl.Body) + case *ast.FuncLit: + w.ignore[n.Body] = true return w - } + case *ast.RangeStmt: + t := w.pkg.TypeOf(n.X) - block, ok := node.(*ast.BlockStmt) - if !ok { - return w - } - - if mustIgnore(block, w.ignore) { - return w - } - - if len(block.List) == 0 { - w.onFailure(lint.Failure{ - Confidence: 1, - Node: block, - Category: "logic", - Failure: "this block is empty, you can remove it", - }) + if _, ok := t.(*types.Chan); ok { + w.ignore[n.Body] = true + } + case *ast.BlockStmt: + if !w.ignore[n] && len(n.List) == 0 { + w.onFailure(lint.Failure{ + Confidence: 1, + Node: n, + Category: "logic", + Failure: "this block is empty, you can remove it", + }) + } } return w } - -func mustIgnore(block *ast.BlockStmt, blackList []*ast.BlockStmt) bool { - for _, b := range blackList { - if b == block { - return true - } - } - return false -} diff --git a/testdata/empty-block.go b/testdata/empty-block.go index 9f04d5b..81b7262 100644 --- a/testdata/empty-block.go +++ b/testdata/empty-block.go @@ -2,18 +2,18 @@ package fixtures -func f(x int) bool {} // Must not match +func f(x int) {} // Must not match -type foo struct {} +type foo struct{} -func (f foo) f(x *int) string {} // Must not match -func (f *foo) g(y *int) string {} // Must not match +func (f foo) f(x *int) {} // Must not match +func (f *foo) g(y *int) {} // Must not match -func g(f func() bool) string { +func g(f func() bool) { { // MATCH /this block is empty, you can remove it/ } - a := func(e error){} // Must not match + _ = func(e error) {} // Must not match if ok := f(); ok { // MATCH /this block is empty, you can remove it/ // only a comment @@ -35,4 +35,13 @@ func g(f func() bool) string { } + // issue 386 + var c = make(chan int) + for range c { + } + + var s = "a string" + for range s { // MATCH /this block is empty, you can remove it/ + } + }