feature: new rule useless-fallthrough (#1462)

README.md

@@ -599,6 +599,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a
 | [`use-errors-new`](./RULES_DESCRIPTIONS.md#use-errors-new) | n/a   | Spots calls to `fmt.Errorf` that can be replaced by `errors.New` |   no    |  no   |
 | [`use-fmt-print`](./RULES_DESCRIPTIONS.md#use-fmt-print) | n/a   | Proposes to replace calls to built-in `print` and `println` with their equivalents from `fmt`. |   no    |  no   |
 | [`useless-break`](./RULES_DESCRIPTIONS.md#useless-break)          |  n/a   |  Warns on useless `break` statements in case clauses |    no    |  no   |
+| [`useless-fallthrough`](./RULES_DESCRIPTIONS.md#useless-fallthrough)  |  n/a   |  Warns on useless `fallthrough` statements in case clauses |    no    |  no   |
 | [`var-declaration`](./RULES_DESCRIPTIONS.md#var-declaration)     |  n/a   | Reduces redundancies around variable declaration.                |   yes    |  yes  |
 | [`var-naming`](./RULES_DESCRIPTIONS.md#var-naming)          |  allowlist & blocklist of initialisms   | Naming rules.                                                    |   yes    |  no   |
 | [`waitgroup-by-value`](./RULES_DESCRIPTIONS.md#waitgroup-by-value)  |  n/a   | Warns on functions taking sync.WaitGroup as a by-value parameter |    no    |  no   |

RULES_DESCRIPTIONS.md

@@ -93,6 +93,7 @@ List of all available rules.
   - [use-errors-new](#use-errors-new)
   - [use-fmt-print](#use-fmt-print)
   - [useless-break](#useless-break)
+  - [useless-fallthrough](#useless-fallthrough)
   - [var-declaration](#var-declaration)
   - [var-naming](#var-naming)
   - [waitgroup-by-value](#waitgroup-by-value)
@@ -1389,6 +1390,44 @@ The rule emits a specific warning for such cases.
 
 _Configuration_: N/A
 
+## useless-fallthrough
+
+_Description_: This rule warns on useless `fallthrough` statements in case clauses of switch statements.
+A `fallthrough` is considered _useless_ if it's the single statement of a case clause block.
+
+Go allows `switch` statements with clauses that group multiple cases.
+Thus, for example:
+
+```go
+switch category {
+  case "Lu":
+    fallthrough
+  case "Ll":
+    fallthrough    
+  case "Lt":
+    fallthrough
+  case "Lm": 
+    fallthrough
+  case "Lo":
+    return true
+  default:
+    return false
+}
+```
+
+can be written as
+
+```go
+switch category {
+  case "Lu", "Ll", "Lt", "Lm", "Lo":
+      return true
+  default:
+    return false
+}
+```
+
+_Configuration_: N/A
+
 ## var-declaration
 
 _Description_: This rule proposes simplifications of variable declarations.

config/config.go

@@ -110,6 +110,7 @@ var allRules = append([]lint.Rule{
 	&rule.IdenticalIfElseIfConditionsRule{},
 	&rule.IdenticalIfElseIfBranchesRule{},
 	&rule.IdenticalSwitchBranchesRule{},
+	&rule.UselessFallthroughRule{},
 }, defaultRules...)
 
 // allFormatters is a list of all available formatters to output the linting results.

rule/useless_fallthrough.go

@@ -0,0 +1,91 @@
+package rule
+
+import (
+	"go/ast"
+	"go/token"
+
+	"github.com/mgechev/revive/lint"
+)
+
+// UselessFallthroughRule warns on useless fallthroughs in switch case clauses.
+type UselessFallthroughRule struct{}
+
+// Apply applies the rule to given file.
+func (*UselessFallthroughRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
+	var failures []lint.Failure
+
+	commentsMap := file.CommentMap()
+
+	onFailure := func(failure lint.Failure) {
+		failures = append(failures, failure)
+	}
+
+	w := &lintUselessFallthrough{onFailure: onFailure, commentsMap: commentsMap}
+	for _, decl := range file.AST.Decls {
+		fn, ok := decl.(*ast.FuncDecl)
+		if !ok || fn.Body == nil {
+			continue
+		}
+
+		ast.Walk(w, fn.Body)
+	}
+
+	return failures
+}
+
+// Name returns the rule name.
+func (*UselessFallthroughRule) Name() string {
+	return "useless-fallthrough"
+}
+
+type lintUselessFallthrough struct {
+	onFailure   func(lint.Failure)
+	commentsMap ast.CommentMap
+}
+
+func (w *lintUselessFallthrough) Visit(node ast.Node) ast.Visitor {
+	switchStmt, ok := node.(*ast.SwitchStmt)
+	if !ok { // not a switch statement, keep walking the AST
+		return w
+	}
+
+	if switchStmt.Tag == nil {
+		return w // Not interested in un-tagged switches
+	}
+
+	casesCount := len(switchStmt.Body.List)
+	for i := range casesCount - 1 {
+		caseClause := switchStmt.Body.List[i].(*ast.CaseClause)
+		caseBody := caseClause.Body
+
+		if len(caseBody) != 1 {
+			continue // skip if body is not exactly one statement
+		}
+
+		branchStmt, ok := caseBody[0].(*ast.BranchStmt)
+		if !ok || branchStmt.Tok != token.FALLTHROUGH {
+			continue // not a fallthrough
+		}
+
+		confidence := 1.0
+		if nextCaseClause := switchStmt.Body.List[i+1].(*ast.CaseClause); nextCaseClause.List == nil {
+			// the next case clause is the default clause, report with lower confidence.
+			confidence = 0.8
+		}
+		if _, ok := w.commentsMap[branchStmt]; ok {
+			// The fallthrough has a comment, report with lower confidence.
+			confidence = 0.5
+		}
+
+		w.onFailure(lint.Failure{
+			Confidence: confidence,
+			Node:       branchStmt,
+			Category:   lint.FailureCategoryCodeStyle,
+			Failure:    `this "fallthrough" can be removed by consolidating this case clause with the next one`,
+		})
+
+		ast.Walk(w, caseClause)
+	}
+
+	return nil
+}

test/useless_fallthrough_test.go

@@ -0,0 +1,11 @@
+package test
+
+import (
+	"testing"
+
+	"github.com/mgechev/revive/rule"
+)
+
+func TestUselessFallTrhough(t *testing.T) {
+	testRule(t, "useless_fallthrough", &rule.UselessFallthroughRule{})
+}

testdata/useless_fallthrough.go

@@ -0,0 +1,79 @@
+package fixtures
+
+func uselessFallthrough() {
+
+	switch a {
+	case 0:
+		println()
+		fallthrough
+	default:
+	}
+
+	switch a {
+	case 0:
+		fallthrough // MATCH /this "fallthrough" can be removed by consolidating this case clause with the next one/
+	case 1:
+		println()
+	default:
+	}
+
+	switch a {
+	case 0:
+		fallthrough // MATCH /this "fallthrough" can be removed by consolidating this case clause with the next one/
+	case 1:
+		fallthrough // MATCH /this "fallthrough" can be removed by consolidating this case clause with the next one/
+	case 2:
+		println()
+	default:
+	}
+
+	switch a {
+	case 0:
+		fallthrough // json:{"MATCH": "this \"fallthrough\" can be removed by consolidating this case clause with the next one","Confidence": 0.8}
+	default:
+		println()
+	}
+
+	switch a {
+	case 0:
+		fallthrough // json:{"MATCH": "this \"fallthrough\" can be removed by consolidating this case clause with the next one","Confidence": 0.8}
+	default:
+		println()
+	case 1:
+		fallthrough // MATCH /this "fallthrough" can be removed by consolidating this case clause with the next one/
+	case 2:
+		println()
+	}
+
+	switch a {
+	case 0:
+		// This a comment on the case, the confidence must be 0.5
+		fallthrough // json:{"MATCH": "this \"fallthrough\" can be removed by consolidating this case clause with the next one","Confidence": 0.5}
+	default:
+		println()
+	}
+
+	switch goos {
+	case "linux":
+		// TODO(bradfitz): be fancy and use linkat with AT_EMPTY_PATH to avoid
+		// copying? I couldn't get it to work, though.
+		// For now, just do the same thing as every other Unix and copy
+		// the binary.
+		fallthrough // json:{"MATCH": "this \"fallthrough\" can be removed by consolidating this case clause with the next one","Confidence": 0.5}
+	case "darwin", "freebsd", "openbsd", "netbsd":
+		return
+	case "windows":
+		return
+	default:
+		return
+	}
+
+	switch a {
+	case 0:
+		//foo:bar
+		fallthrough // json:{"MATCH": "this \"fallthrough\" can be removed by consolidating this case clause with the next one","Confidence": 0.5}
+	default:
+		println()
+	}
+
+}