add new rule useless-break (#551)

Author: chavacava

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
 

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.

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{

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/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{})
+}

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)
+	}
+}