From c939bb6af8a15447252e33300b5d1235db0a6449 Mon Sep 17 00:00:00 2001 From: SalvadorC Date: Mon, 16 Aug 2021 00:30:08 +0200 Subject: [PATCH] add new rule useless-break (#551) --- README.md | 1 + RULES_DESCRIPTIONS.md | 11 +++++ config/config.go | 1 + rule/useless-break.go | 77 +++++++++++++++++++++++++++++++++ test/useless-break_test.go | 12 ++++++ testdata/useless-break.go | 87 ++++++++++++++++++++++++++++++++++++++ 6 files changed, 189 insertions(+) create mode 100644 rule/useless-break.go create mode 100644 test/useless-break_test.go create mode 100644 testdata/useless-break.go 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, + }) + } +} 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) + } +}