Repository: mgechev/revive (go)

Issue:
Warn on useless break statements in case clauses
When executing switch or select statements, GO, unlike other programming languages like C, only executes statements of the selected case clause while ignoring the subsequent clauses. Therefore, inserting a `break` at the end of a case clause has no effect. I propose to add a rule to spot those cases.

More over, because `break` statements are rarely used in case clauses, when switch or select statements are inside a for-loop, the programmer might wrongly assume that a `break` in a case clause will take the control out of the loop. The rule could emit a specific warning in such cases.

Testing a prototype implementation over some code bases shows that the problem is a common one and also spotted [some potential bugs](https://github.com/onsi/ginkgo/issues/827)

```
tools/cmd/auth/netrcauth/netrcauth.go:99:5: useless break in case clause (WARN: this break statement affects the switch or select statement and not the loop enclosing it)
kubernetes/vendor/github.com/exponent-io/jsonpath/decoder.go:69:3: useless break in case clause
kubernetes/vendor/github.com/exponent-io/jsonpath/decoder.go:72:3: useless break in case clause
kubernetes/vendor/github.com/Microsoft/hcsshim/errors.go:117:3: useless break in case clause
kubernetes/vendor/github.com/exponent-io/jsonpath/decoder.go:98:4: useless break in case clause
kubernetes/vendor/github.com/cpuguy83/go-md2man/v2/md2man/roff.go:122:3: useless break in case clause
kubernetes/vendor/github.com/onsi/ginkgo/ginkgo/convert/ginkgo_ast_nodes.go:57:4: useless break in case clause
kubernetes/vendor/github.com/onsi/ginkgo/internal/leafnodes/runner.go:79:6: useless break in case clause
kubernetes/vendor/github.com/pquerna/cachecontrol/cacheobject/object.go:66:3: useless break in case clause
kubernetes/vendor/github.com/clusterhq/flocker-go/client.go:325:4: useless break in case clause (WARN: this break statement affects the switch or select statement and not the loop enclosing it)
kubernetes/vendor/github.com/aws/aws-sdk-go/aws/context_sleep.go:18:3: useless break in case clause
kubernetes/vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta/method.go:239:3: useless break in case clause
kubernetes/vendor/github.com/mitchellh/mapstructure/mapstructure.go:814:5: useless break in case clause
kubernetes/vendor/github.com/moby/spdystream/connection.go:301:3: useless break in case clause
kubernetes/vendor/k8s.io/utils/mount/mount.go:293:4: useless break in case clause
kubernetes/vendor/github.com/moby/spdystream/stream.go:162:3: useless break in case clause
hugo/tpl/internal/go_templates/texttemplate/exec.go:394:3: useless break in case clause
kubernetes/vendor/github.com/Microsoft/hcsshim/errors.go:160:3: useless break in case clause
kubernetes/vendor/go.uber.org/zap/zapcore/field.go:172:3: useless break in case clause
kubernetes/vendor/github.com/exponent-io/jsonpath/decoder.go:101:4: useless break in case clause
kubernetes/vendor/github.com/russross/blackfriday/v2/html.go:622:3: useless break in case clause
kubernetes/vendor/github.com/russross/blackfriday/v2/inline.go:1023:4: useless break in case clause (WARN: this break statement affects the switch or select statement and not the loop enclosing it)
kubernetes/vendor/github.com/cilium/ebpf/elf_reader.go:722:4: useless break in case clause
kubernetes/vendor/gopkg.in/square/go-jose.v2/json/decode.go:484:3: useless break in case clause
kubernetes/vendor/github.com/pquerna/cachecontrol/cacheobject/object.go:68:3: useless break in case clause
terraform/configs/provider_requirements.go:216:5: useless break in case clause
kubernetes/vendor/k8s.io/utils/mount/mount.go:295:4: useless break in case clause
kubernetes/vendor/github.com/moby/spdystream/stream.go:177:3: useless break in case clause
kubernetes/vendor/github.com/exponent-io/jsonpath/decoder.go:141:3: useless break in case clause
kubernetes/vendor/gopkg.in/yaml.v2/emitterc.go:145:3: useless break in case clause
kubernetes/vendor/github.com/cilium/ebpf/elf_reader.go:724:4: useless break in case clause
kubernetes/vendor/github.com/moby/spdystream/stream.go:274:3: useless break in case clause
kubernetes/vendor/k8s.io/utils/mount/mount.go:305:4: useless break in case clause
kubernetes/vendor/gopkg.in/yaml.v2/emitterc.go:148:3: useless break in case clause
kubernetes/vendor/github.com/exponent-io/jsonpath/decoder.go:153:3: useless break in case clause
kubernetes/vendor/github.com/cilium/ebpf/elf_reader.go:726:4: useless break in case clause
kubernetes/vendor/gopkg.in/yaml.v2/emitterc.go:151:3: useless break in case clause
kubernetes/vendor/k8s.io/utils/mount/mount.go:307:4: useless break in case clause
kubernetes/vendor/golang.org/x/crypto/ssh/handshake.go:274:5: useless break in case clause (WARN: this break statement affects the switch or select statement and not the loop enclosing it)
kubernetes/vendor/gopkg.in/yaml.v3/emitterc.go:177:3: useless break in case clause
kubernetes/vendor/gopkg.in/yaml.v3/emitterc.go:180:3: useless break in case clause
kubernetes/vendor/gopkg.in/yaml.v3/emitterc.go:183:3: useless break in case clause
kubernetes/vendor/golang.org/x/text/encoding/korean/euckr.go:80:4: useless break in case clause (WARN: this break statement affects the switch or select statement and not the loop enclosing it)
kubernetes/vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/ratelimit.go:72:3: useless break in case clause
```

Pull request description:
add new rule useless-break
Closes #550 


Patch:
diff --git a/README.md b/README.md
index 52efcb4..ef67f51 100644
--- a/README.md
+++ b/README.md
@@ -453,6 +453,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a
 | [`unexported-naming`](./RULES_DESCRIPTIONS.md#unexported-naming)          |  n/a   |  Warns on wrongly named un-exported symbols       |    no    |  no   |
 | [`function-length`](./RULES_DESCRIPTIONS.md#function-length)          |  n/a   |  Warns on functions exceeding the statements or lines max |    no    |  no   |
 | [`nested-structs`](./RULES_DESCRIPTIONS.md#nested-structs)          |  n/a   |  Warns on structs within structs |    no    |  no   |
+| [`useless-break`](./RULES_DESCRIPTIONS.md#useless-break)          |  n/a   |  Warns on useless `break` statements in case clauses |    no    |  no   |
 
 ## Configurable rules
 
diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md
index 7c3ba95..48fa412 100644
--- a/RULES_DESCRIPTIONS.md
+++ b/RULES_DESCRIPTIONS.md
@@ -67,6 +67,7 @@ List of all available rules.
   - [unreachable-code](#unreachable-code)
   - [unused-parameter](#unused-parameter)
   - [unused-receiver](#unused-receiver)
+  - [useless-break](#useless-break)
   - [waitgroup-by-value](#waitgroup-by-value)
 
 ## add-constant
@@ -612,6 +613,16 @@ _Description_: This rule warns on unused method receivers. Methods with unused r
 
 _Configuration_: N/A
 
+## useless-break
+
+_Description_: This rule warns on useless `break` statements in case clauses of switch and select statements. GO, unlike other programming languages like C, only executes statements of the selected case while ignoring the subsequent case clauses. 
+Therefore, inserting a `break` at the end of a case clause has no effect. 
+
+Because `break` statements are rarely used in case clauses, when switch or select statements are inside a for-loop, the programmer might wrongly assume that a `break` in a case clause will take the control out of the loop.
+The rule emits a specific warning for such cases.
+
+_Configuration_: N/A
+
 ## waitgroup-by-value
 
 _Description_: Function parameters that are passed by value, are in fact a copy of the original argument. Passing a copy of a `sync.WaitGroup` is usually not what the developer wants to do.
diff --git a/config/config.go b/config/config.go
index 1ca54d6..298c0bd 100644
--- a/config/config.go
+++ b/config/config.go
@@ -80,6 +80,7 @@ var allRules = append([]lint.Rule{
 	&rule.FunctionLength{},
 	&rule.NestedStructs{},
 	&rule.IfReturnRule{},
+	&rule.UselessBreak{},
 }, defaultRules...)
 
 var allFormatters = []lint.Formatter{
diff --git a/rule/useless-break.go b/rule/useless-break.go
new file mode 100644
index 0000000..9e9c829
--- /dev/null
+++ b/rule/useless-break.go
@@ -0,0 +1,77 @@
+package rule
+
+import (
+	"go/ast"
+	"go/token"
+
+	"github.com/mgechev/revive/lint"
+)
+
+// UselessBreak lint rule.
+type UselessBreak struct{}
+
+// Apply applies the rule to given file.
+func (r *UselessBreak) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
+	var failures []lint.Failure
+
+	onFailure := func(failure lint.Failure) {
+		failures = append(failures, failure)
+	}
+
+	astFile := file.AST
+	w := &lintUselessBreak{onFailure, false}
+	ast.Walk(w, astFile)
+	return failures
+}
+
+// Name returns the rule name.
+func (r *UselessBreak) Name() string {
+	return "useless-break"
+}
+
+type lintUselessBreak struct {
+	onFailure  func(lint.Failure)
+	inLoopBody bool
+}
+
+func (w *lintUselessBreak) Visit(node ast.Node) ast.Visitor {
+	switch v := node.(type) {
+	case *ast.ForStmt:
+		w.inLoopBody = true
+		ast.Walk(w, v.Body)
+		w.inLoopBody = false
+		return nil
+	case *ast.CommClause:
+		for _, n := range v.Body {
+			w.inspectCaseStatement(n)
+		}
+		return nil
+	case *ast.CaseClause:
+		for _, n := range v.Body {
+			w.inspectCaseStatement(n)
+		}
+		return nil
+	}
+	return w
+}
+
+func (w *lintUselessBreak) inspectCaseStatement(n ast.Stmt) {
+	switch s := n.(type) {
+	case *ast.BranchStmt:
+		if s.Tok != token.BREAK {
+			return // not a break statement
+		}
+		if s.Label != nil {
+			return // labeled break statement, usually affects a nesting loop
+		}
+		msg := "useless break in case clause"
+		if w.inLoopBody {
+			msg += " (WARN: this break statement affects this switch or select statement and not the loop enclosing it)"
+		}
+		w.onFailure(lint.Failure{
+			Confidence: 1,
+			Node:       s,
+			Failure:    msg,
+		})
+	}
+}


Test patch:
diff --git a/test/useless-break_test.go b/test/useless-break_test.go
new file mode 100644
index 0000000..d77dfc3
--- /dev/null
+++ b/test/useless-break_test.go
@@ -0,0 +1,12 @@
+package test
+
+import (
+	"testing"
+
+	"github.com/mgechev/revive/rule"
+)
+
+// UselessBreak rule.
+func TestUselessBreak(t *testing.T) {
+	testRule(t, "useless-break", &rule.UselessBreak{})
+}
diff --git a/testdata/useless-break.go b/testdata/useless-break.go
new file mode 100644
index 0000000..1e6457d
--- /dev/null
+++ b/testdata/useless-break.go
@@ -0,0 +1,87 @@
+package fixtures
+
+import "reflect"
+
+func UselessBreaks() {
+
+	switch {
+	case true:
+		break // MATCH /useless break in case clause/
+	case false:
+		if false {
+			break
+		}
+	}
+
+	select {
+	case c:
+		break // MATCH /useless break in case clause/
+	case n:
+		if true {
+			if false {
+				break
+			}
+			break
+		}
+	}
+
+	for {
+		switch {
+		case c1:
+			break // MATCH /useless break in case clause (WARN: this break statement affects the switch or select statement and not the loop enclosing it)/
+		}
+	}
+
+	for _, node := range desc.Args {
+		switch node := node.(type) {
+		case *ast.FuncLit:
+			found = true
+			funcLit = node
+			break // MATCH /useless break in case clause (WARN: this break statement affects the switch or select statement and not the loop enclosing it)/
+		}
+	}
+
+	switch val.Kind() {
+	case reflect.Array, reflect.Slice:
+		if val.Len() == 0 {
+			break
+		}
+		for i := 0; i < val.Len(); i++ {
+			oneIteration(reflect.ValueOf(i), val.Index(i))
+		}
+		return
+	case reflect.Map:
+		if val.Len() == 0 {
+			break
+		}
+		om := fmtsort.Sort(val)
+		for i, key := range om.Key {
+			oneIteration(key, om.Value[i])
+		}
+		return
+	case reflect.Chan:
+		if val.IsNil() {
+			break
+		}
+		if val.Type().ChanDir() == reflect.SendDir {
+			s.errorf("range over send-only channel %v", val)
+			break
+		}
+		i := 0
+		for ; ; i++ {
+			elem, ok := val.Recv()
+			if !ok {
+				break
+			}
+			oneIteration(reflect.ValueOf(i), elem)
+		}
+		if i == 0 {
+			break
+		}
+		return
+	case reflect.Invalid:
+		break // MATCH /useless break in case clause/
+	default:
+		s.errorf("range can't iterate over %v", val)
+	}
+}