Repository: mgechev/revive

Issue:
useless fallthrought: report them
Here it would be interesting to consider reporting empty fallthrought

```go
switch a {
  case 1:
    bar()
  case 2:
    fallthrought
  case 3:
    foo()
}
```

Maybe with an other PR anyway, and maybe another rule

But this would be expected

```go
switch a {
  case 1:
    bar()
  case 2, 3:
    foo()
}
```


_Originally posted by @ccoVeille in https://github.com/mgechev/revive/pull/1448#discussion_r2241802977_

Hints from issue comments:
@chavacava: I ran a draft version of the rule on some code bases and maybe the rule should **not** warn when falling-through the `default` clause.
These clauses falling through to `default` seem to be used as placeholders/documentation.

For example:

```go
	switch kind {
	case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
		value := valueHelp.asInt64(val)
		return MaximumInt(path, in, value, int64(max), exclusive)
	case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64:
		value := valueHelp.asUint64(val)
		if max < 0 {
			return errors.ExceedsMaximum(path, in, max, exclusive, val)
		}
		return MaximumUint(path, in, value, uint64(max), exclusive)
	case reflect.Float32, reflect.Float64:
		fallthrough
	default:
		value := valueHelp.asFloat64(val)
		return Maximum(path, in, value, max, exclusive)
	}
```

What do you say?@ccoVeille: I would report them with a lower confidence.@ccoVeille: Did you find real life strange thing that the rule could help to fix ?@chavacava: > Did you find real life strange thing that the rule could help to fix ?

I think so.

From things like

```go
	switch subjectKind {
	case rbacv1.GroupKind:
		sar = &authv1.SubjectAccessReview{
			Spec: authv1.SubjectAccessReviewSpec{
				Groups:             []string{subject},
				ResourceAttributes: resource,
			},
		}
	case rbacv1.UserKind:
		fallthrough
	case rbacv1.ServiceAccountKind:
		sar = &authv1.SubjectAccessReview{
			Spec: authv1.SubjectAccessReviewSpec{
				User:               subject,
				ResourceAttributes: resource,
			},
		}
	default:
		framework.Failf("invalid subjectKind %s", subjectKind)
	}
``` 

to 

```go
	switch n.Type {
	case Document:
		fallthrough
	case BlockQuote:
		fallthrough
	case List:
		fallthrough
	case Item:
		fallthrough
	case Paragraph:
		fallthrough
	case Heading:
		fallthrough
	case Emph:
		fallthrough
	case Strong:
		fallthrough
	case Del:
		fallthrough
	case Link:
		fallthrough
	case Image:
		fallthrough
	case Table:
		fallthrough
	case TableHead:
		fallthrough
	case TableBody:
		fallthrough
	case TableRow:
		fallthrough
	case TableCell:
		return true
	default:
		return false
	}
```@chavacava: Side note on performance. As I suspected, using useless `fallthrough` is suboptimal.

For
```go
func toto(a int) bool {
	switch a {
	case 0:
		fallthrough
	case 1:
		fallthrough
	case 2:
		return true
	default:
		return false
	}
}
```
the resulting SSA is
```
# /app/public/buildbox/e2419dfd-7102-11f0-9e73-0242ac16000d/main.go
00000 (3) TEXT command-line-arguments.toto(SB), ABIInternal
00001 (3) FUNCDATA $0, gclocals·g2BeySu+wFnoycgXfElmcg==(SB)
00002 (3) FUNCDATA $1, gclocals·g2BeySu+wFnoycgXfElmcg==(SB)
00003 (3) FUNCDATA $5, command-line-arguments.toto.arginfo1(SB)
00004 (3) FUNCDATA $6, command-line-arguments.toto.argliveinfo(SB)
b1
00005 (3) PCDATA $3, $1
v21
00006 (+5) TESTQ AX, AX
b1
00007 (5) JEQ 12
v5
00008 (+7) CMPQ AX, $1
b5
00009 (7) JEQ 12
v4
00010 (+9) CMPQ AX, $2
b9
00011 (9) JNE 14
v22
00012 (+10) MOVL $1, AX
b13
00013 (10) RET
v23
00014 (+12) XORL AX, AX
b14
00015 (12) RET
00016 (?) END
```
while the SSA for the switch with consolidated case clauses 
```go
func toto(a int) bool {
	switch a {
	case 0, 1, 2:
		return true
	default:
		return false
	}
}
```
is

```
# /app/public/buildbox/06de37da-7103-11f0-9e73-0242ac16000d/main.go
00000 (3) TEXT command-line-arguments.toto(SB), ABIInternal
00001 (3) FUNCDATA $0, gclocals·g2BeySu+wFnoycgXfElmcg==(SB)
00002 (3) FUNCDATA $1, gclocals·g2BeySu+wFnoycgXfElmcg==(SB)
00003 (3) FUNCDATA $5, command-line-arguments.toto.arginfo1(SB)
00004 (3) FUNCDATA $6, command-line-arguments.toto.argliveinfo(SB)
b5
00005 (3) PCDATA $3, $1
v3
00006 (+5) CMPQ AX, $2
b5
00007 (5) JHI 10
v20
00008 (+6) MOVL $1, AX
b6
00009 (6) RET
v19
00010 (+8) XORL AX, AX
b7
00011 (8) RET
00012 (?) END
``` @ccoVeille: Thanks@veeam-denis: @chavacava I'm curious: how did you get these assembly listings?@chavacava: > @chavacava I'm curious: how did you get these assembly listings?

I used to use the Go toolchain to generate a web page with info on all compilation phases but I forgot the command :(

For the above examples I've used https://golang.design/gossa 

Solution patch:
diff --git a/README.md b/README.md
index a6e6fb1..c12da86 100644
--- a/README.md
+++ b/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   |
diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md
index 14c1be7..2463f86 100644
--- a/RULES_DESCRIPTIONS.md
+++ b/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.
diff --git a/config/config.go b/config/config.go
index 200753e..eb1585a 100644
--- a/config/config.go
+++ b/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.
diff --git a/rule/useless_fallthrough.go b/rule/useless_fallthrough.go
new file mode 100644
index 0000000..2238767
--- /dev/null
+++ b/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 patch:
diff --git a/test/useless_fallthrough_test.go b/test/useless_fallthrough_test.go
new file mode 100644
index 0000000..0496c81
--- /dev/null
+++ b/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{})
+}
diff --git a/testdata/useless_fallthrough.go b/testdata/useless_fallthrough.go
new file mode 100644
index 0000000..7f2b9cd
--- /dev/null
+++ b/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()
+	}
+
+}