Repository: mgechev/revive (go)

Issue:
Incorrectly identifies channel draining as "empty code block"
**Describe the bug**
`revive` incorrectly identities channel draining as an empty code block and suggests to remove it.

**To Reproduce**
Steps to reproduce the behavior:
1. I updated revive `go get -u github.com/mgechev/revive`
2. I run it with the following flags & configuration file:

```shell
cd $PROJECT_DIR
revive -config revive.toml .
```

```toml
ignoreGeneratedHeader = false
severity = "error"
confidence = 0.8
errorCode = 1
warningCode = 0

[rule.add-constant]
    arguments = [{maxLitCount = "3",allowStrs ="\"\"",allowInts="0,1,2"}]
[rule.argument-limit]
    arguments = [6]
[rule.atomic]
[rule.blank-imports]
[rule.bool-literal-in-expr]
[rule.call-to-gc]
[rule.constant-logical-expr]
[rule.context-as-argument]
[rule.context-keys-type]
[rule.cyclomatic]
    arguments = [10]
[rule.deep-exit]
[rule.dot-imports]
[rule.duplicated-imports]
[rule.empty-block]
[rule.error-naming]
[rule.error-return]
[rule.error-strings]
[rule.errorf]
[rule.exported]
[rule.flag-parameter]
[rule.function-result-limit]
    arguments = [4]
[rule.get-return]
[rule.if-return]
[rule.import-shadowing]
[rule.increment-decrement]
[rule.indent-error-flow]
[rule.line-length-limit]
    arguments = [120]
[rule.package-comments]
[rule.range]
[rule.range-val-in-closure]
[rule.receiver-naming]
[rule.redefines-builtin-id]
[rule.superfluous-else]
[rule.time-naming]
[rule.unexported-return]
[rule.unhandled-error]
[rule.unreachable-code]
[rule.unused-parameter]
[rule.unused-receiver]
[rule.var-declaration]
[rule.var-naming]
[rule.waitgroup-by-value]
```

A really bad example, but in our scenario, we have many routines reading from `processChan` where we send another message to have them stop processing. We reuse the channel, but we want to ensure the channel was empty from previous processing. 
```go
package main

import "sync"

const (
	chanSize = 10
	maxNums = 100
	modNum = 5
)

func main() {
	wg := sync.WaitGroup{}
	processChan := make(chan int, chanSize)

	wg.Add(1)
	go func() {
		defer wg.Done()
		for i := 0; i < maxNums; i++ {
			processChan <- i
		}
	}()

	wg.Add(1)
	go func() {
		defer wg.Done()

		for i := 0; i < maxNums; i++ {
			if !isValidNum(i) {
				break
			}
		}
	}()

	for range processChan {
	}
}

func isValidNum(i int) bool {
	return i%modNum == 0
}

```

3. Revive returns an error
>myflie.go:34:24: this block is empty, you can remove it

**Expected behavior**
No error to occur as this is channel draining.

**Logs**
If applicable, add screenshots to help explain your problem.

**Desktop (please complete the following information):**
 - OS: macOS 10.14.6
 - Version of Go: v1.14.1

**Additional context**
We have two workarounds:
1. We can ignore the few lines across our project where we are iterating over a channel, however, it would be nicer if `revive` knew that this is channel draining. 
2. We can delete and recreate the channel, but since the channel is passed around on startup / Dependency Injection, we cannot just "create a new one and reinitialize all services" easily. 


Pull request:
fix issue 386: Incorrectly identifies channel draining as "empty code…
Closes #386 


Patch:
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
-}


Test patch:
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/
+	}
+
 }