Repository: mgechev/revive (go)

Issue:
Rule for detecting useless os.Exit in TestMain
**Is your feature request related to a problem? Please describe.**

Implement a rule that suggests removing `os.Exit` in `TestMain` for test files.

Before:

```go
func TestMain(m *testing.M) {
	os.Exit(m.Run())
}
```


After:
```go
func TestMain(m *testing.M) {
	m.Run()
}
```

**Describe the solution you'd like**

A new rule `redundant-test-main-os-exit` that suggests removing `os.Exit`.

**Describe alternatives you've considered**

Search for `os.Exit` manually and refactor `TestMain` functions.

**Additional context**

As of Go 1.15, we do not need to call `os.Exit(m.Run())` explicitly, as value returned by `m.Run()` is stored into unexported field of `m` and go test executable is smart enough to automatically call `os.Exit(retValue)` when `TestMain` returns. See golang/go#34129 and https://pkg.go.dev/testing#hdr-Main

These PRs performs removing `os.Exit` from `TestMain` in a wild:

- https://github.com/go-delve/delve/pull/3856
- https://github.com/goreleaser/goreleaser/pull/5257
- https://github.com/lima-vm/lima/pull/2869


Pull request description:
Add redundant-test-main-exit rule
The `redundant-test-main-exit` rule reports unnecessary `Exit` calls in `TestMain` functions, as the Go test runner has handled termination automatically since Go 1.15; the details are explained in the issue.

fixes #1189

The `deep-exit` rule currently excludes reporting `Exit` calls in `TestMain` functions, but we could consider updating its behavior to include these calls, which might eliminate the need to define this new rule.


Patch:
diff --git a/README.md b/README.md
index 70e56d0..cb23e3e 100644
--- a/README.md
+++ b/README.md
@@ -100,7 +100,6 @@ Here's how `revive` is different from `golint`:
     - [SARIF](#sarif)
   - [Extensibility](#extensibility)
     - [Writing a Custom Rule](#writing-a-custom-rule)
-      - [Example](#example)
       - [Using `revive` as a library](#using-revive-as-a-library)
     - [Custom Formatter](#custom-formatter)
   - [Speed Comparison](#speed-comparison)
@@ -561,6 +560,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a
 | [`filename-format`](./RULES_DESCRIPTIONS.md#filename-format) | regular expression (optional) | Enforces the formatting of filenames |   no    |  no   |
 | [`redundant-build-tag`](./RULES_DESCRIPTIONS.md#redundant-build-tag) | n/a   | Warns about redundant `// +build` comment lines |   no    |  no   |
 | [`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   |
+| [`redundant-test-main-exit`](./RULES_DESCRIPTIONS.md#redundant-test-main-exit) |  n/a   | Suggests removing `Exit` call in `TestMain` function for test files |    no    |  no   |
 
 ## Configurable rules
 
diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md
index 7627b83..0cb3856 100644
--- a/RULES_DESCRIPTIONS.md
+++ b/RULES_DESCRIPTIONS.md
@@ -66,6 +66,7 @@ List of all available rules.
   - [redefines-builtin-id](#redefines-builtin-id)
   - [redundant-import-alias](#redundant-import-alias)
   - [redundant-build-tag](#redundant-build-tag)
+  - [redundant-test-main-exit](#redundant-test-main-exit)
   - [string-format](#string-format)
   - [string-of-int](#string-of-int)
   - [struct-tag](#struct-tag)
@@ -284,7 +285,7 @@ _Configuration_: N/A
 _Description_: This rule warns on some common mistakes when using `defer` statement. It currently alerts on the following situations:
 
 | name              | description                                                                                                                                                                                     |
-|-------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+| ----------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
 | call-chain        | even if deferring call-chains of the form `foo()()` is valid, it does not helps code understanding (only the last call is deferred)                                                             |
 | loop              | deferring inside loops can be misleading (deferred functions are not executed at the end of the loop iteration but of the current function) and it could lead to exhausting the execution stack |
 | method-call       | deferring a call to a method can lead to subtle bugs if the method does not have a pointer receiver                                                                                             |
@@ -809,6 +810,14 @@ _Description_: This rule warns about redundant build tag comments `// +build` wh
 
 _Configuration_: N/A
 
+## redundant-test-main-exit
+
+_Description_: This rule warns about redundant `Exit` calls in the `TestMain` function, as the Go test runner automatically handles program termination starting from Go 1.15.
+
+_Configuration_: N/A
+
+_Note_: This rule is irrelevant for Go versions below 1.15.
+
 ## string-format
 
 _Description_: This rule allows you to configure a list of regular expressions that string literals in certain function calls are checked against.
diff --git a/config/config.go b/config/config.go
index 3aedd28..34340b7 100644
--- a/config/config.go
+++ b/config/config.go
@@ -100,6 +100,7 @@ var allRules = append([]lint.Rule{
 	&rule.FilenameFormatRule{},
 	&rule.RedundantBuildTagRule{},
 	&rule.UseErrorsNewRule{},
+	&rule.RedundantTestMainExitRule{},
 }, defaultRules...)
 
 // allFormatters is a list of all available formatters to output the linting results.
diff --git a/lint/package.go b/lint/package.go
index 22438c4..dc99e80 100644
--- a/lint/package.go
+++ b/lint/package.go
@@ -35,6 +35,7 @@ var (
 	trueValue  = 1
 	falseValue = 2
 
+	go115 = goversion.Must(goversion.NewVersion("1.15"))
 	go121 = goversion.Must(goversion.NewVersion("1.21"))
 	go122 = goversion.Must(goversion.NewVersion("1.22"))
 )
@@ -194,6 +195,11 @@ func (p *Package) lint(rules []Rule, config Config, failures chan Failure) error
 	return eg.Wait()
 }
 
+// IsAtLeastGo115 returns true if the Go version for this package is 1.15 or higher, false otherwise
+func (p *Package) IsAtLeastGo115() bool {
+	return p.goVersion.GreaterThanOrEqual(go115)
+}
+
 // IsAtLeastGo121 returns true if the Go version for this package is 1.21 or higher, false otherwise
 func (p *Package) IsAtLeastGo121() bool {
 	return p.goVersion.GreaterThanOrEqual(go121)
diff --git a/rule/exported.go b/rule/exported.go
index 964c06e..7d59c4d 100644
--- a/rule/exported.go
+++ b/rule/exported.go
@@ -182,7 +182,7 @@ func (w *lintExported) lintFuncDoc(fn *ast.FuncDecl) {
 		return
 	}
 
-	if fn.Doc == nil {
+	if !hasTextComment(fn.Doc) {
 		w.onFailure(lint.Failure{
 			Node:       fn,
 			Confidence: 1,
@@ -247,7 +247,7 @@ func (w *lintExported) lintTypeDoc(t *ast.TypeSpec, doc *ast.CommentGroup) {
 		return
 	}
 
-	if doc == nil {
+	if !hasTextComment(doc) {
 		w.onFailure(lint.Failure{
 			Node:       t,
 			Confidence: 1,
@@ -314,7 +314,7 @@ func (w *lintExported) lintValueSpecDoc(vs *ast.ValueSpec, gd *ast.GenDecl, genD
 		return
 	}
 
-	if vs.Doc == nil && gd.Doc == nil {
+	if !hasTextComment(vs.Doc) && !hasTextComment(gd.Doc) {
 		if genDeclMissingComments[gd] {
 			return
 		}
@@ -332,16 +332,16 @@ func (w *lintExported) lintValueSpecDoc(vs *ast.ValueSpec, gd *ast.GenDecl, genD
 		return
 	}
 	// If this GenDecl has parens and a comment, we don't check its comment form.
-	if gd.Doc != nil && gd.Lparen.IsValid() {
+	if hasTextComment(gd.Doc) && gd.Lparen.IsValid() {
 		return
 	}
 	// The relevant text to check will be on either vs.Doc or gd.Doc.
 	// Use vs.Doc preferentially.
 	var doc *ast.CommentGroup
 	switch {
-	case vs.Doc != nil:
+	case hasTextComment(vs.Doc):
 		doc = vs.Doc
-	case vs.Comment != nil && gd.Doc == nil:
+	case hasTextComment(vs.Comment) && !hasTextComment(gd.Doc):
 		doc = vs.Comment
 	default:
 		doc = gd.Doc
@@ -359,6 +359,19 @@ func (w *lintExported) lintValueSpecDoc(vs *ast.ValueSpec, gd *ast.GenDecl, genD
 	}
 }
 
+// hasTextComment returns true if the comment contains a text comment
+// e.g. //go:embed foo.txt a directive comment, not a text comment
+// e.g. //nolint:whatever is a directive comment, not a text comment
+func hasTextComment(comment *ast.CommentGroup) bool {
+	if comment == nil {
+		return false
+	}
+
+	// a comment could be directive and not a text comment
+	text := comment.Text()
+	return text != ""
+}
+
 // normalizeText is a helper function that normalizes comment strings by:
 // * removing one leading space
 //
@@ -388,7 +401,7 @@ func (w *lintExported) Visit(n ast.Node) ast.Visitor {
 	case *ast.TypeSpec:
 		// inside a GenDecl, which usually has the doc
 		doc := v.Doc
-		if doc == nil {
+		if !hasTextComment(doc) {
 			doc = w.lastGen.Doc
 		}
 		w.lintTypeDoc(v, doc)
@@ -424,7 +437,7 @@ func (w *lintExported) lintInterfaceMethod(typeName string, m *ast.Field) {
 		return
 	}
 	name := m.Names[0].Name
-	if m.Doc == nil {
+	if !hasTextComment(m.Doc) {
 		w.onFailure(lint.Failure{
 			Node:       m,
 			Confidence: 1,
diff --git a/rule/flag_param.go b/rule/flag_param.go
index e20aeb7..2f69503 100644
--- a/rule/flag_param.go
+++ b/rule/flag_param.go
@@ -69,6 +69,11 @@ func (w conditionVisitor) Visit(node ast.Node) ast.Visitor {
 			return false
 		}
 
+		_, ok = w.idents[ident.Name]
+		if !ok {
+			return false
+		}
+
 		return w.idents[ident.Name] == struct{}{}
 	}
 


Test patch:
diff --git a/cli/main_test.go b/cli/main_test.go
index 1730169..76441b4 100644
--- a/cli/main_test.go
+++ b/cli/main_test.go
@@ -14,7 +14,7 @@ func TestMain(m *testing.M) {
 	os.Unsetenv("USERPROFILE")
 	os.Unsetenv("XDG_CONFIG_HOME")
 	AppFs = afero.NewMemMapFs()
-	os.Exit(m.Run())
+	m.Run()
 }
 
 func TestXDGConfigDirIsPreferredFirst(t *testing.T) {
diff --git a/rule/redundant_test_main_exit.go b/rule/redundant_test_main_exit.go
new file mode 100644
index 0000000..d456aa2
--- /dev/null
+++ b/rule/redundant_test_main_exit.go
@@ -0,0 +1,79 @@
+package rule
+
+import (
+	"fmt"
+	"go/ast"
+
+	"github.com/mgechev/revive/lint"
+)
+
+// RedundantTestMainExitRule suggests removing Exit call in TestMain function for test files.
+type RedundantTestMainExitRule struct{}
+
+// Apply applies the rule to given file.
+func (*RedundantTestMainExitRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
+	var failures []lint.Failure
+
+	if !file.IsTest() || !file.Pkg.IsAtLeastGo115() {
+		// skip analysis for non-test files or for Go versions before 1.15
+		return failures
+	}
+
+	onFailure := func(failure lint.Failure) {
+		failures = append(failures, failure)
+	}
+
+	w := &lintRedundantTestMainExit{onFailure: onFailure}
+	ast.Walk(w, file.AST)
+	return failures
+}
+
+// Name returns the rule name.
+func (*RedundantTestMainExitRule) Name() string {
+	return "redundant-test-main-exit"
+}
+
+type lintRedundantTestMainExit struct {
+	onFailure func(lint.Failure)
+}
+
+func (w *lintRedundantTestMainExit) Visit(node ast.Node) ast.Visitor {
+	if fd, ok := node.(*ast.FuncDecl); ok {
+		if fd.Name.Name != "TestMain" {
+			return nil // skip analysis for other functions than TestMain
+		}
+
+		return w
+	}
+
+	se, ok := node.(*ast.ExprStmt)
+	if !ok {
+		return w
+	}
+	ce, ok := se.X.(*ast.CallExpr)
+	if !ok {
+		return w
+	}
+
+	fc, ok := ce.Fun.(*ast.SelectorExpr)
+	if !ok {
+		return w
+	}
+	id, ok := fc.X.(*ast.Ident)
+	if !ok {
+		return w
+	}
+
+	pkg := id.Name
+	fn := fc.Sel.Name
+	if isCallToExitFunction(pkg, fn) {
+		w.onFailure(lint.Failure{
+			Confidence: 1,
+			Node:       ce,
+			Category:   lint.FailureCategoryStyle,
+			Failure:    fmt.Sprintf("redundant call to %s.%s in TestMain function, the test runner will handle it automatically as of Go 1.15", pkg, fn),
+		})
+	}
+
+	return w
+}
diff --git a/test/exported_test.go b/test/exported_test.go
index 6e32345..a69b90e 100644
--- a/test/exported_test.go
+++ b/test/exported_test.go
@@ -36,3 +36,7 @@ func TestCheckDisablingOnDeclarationTypes(t *testing.T) {
 
 	testRule(t, "exported_issue_1045", &rule.ExportedRule{}, &lint.RuleConfig{Arguments: args})
 }
+
+func TestCheckDirectiveComment(t *testing.T) {
+	testRule(t, "exported_issue_1202", &rule.ExportedRule{}, &lint.RuleConfig{})
+}
diff --git a/test/redundant_test_main_exit_test.go b/test/redundant_test_main_exit_test.go
new file mode 100644
index 0000000..f64d014
--- /dev/null
+++ b/test/redundant_test_main_exit_test.go
@@ -0,0 +1,13 @@
+package test
+
+import (
+	"testing"
+
+	"github.com/mgechev/revive/rule"
+)
+
+func TestRedundantTestMainExit(t *testing.T) {
+	testRule(t, "go1.15/redundant_test_main_exit_test", &rule.RedundantTestMainExitRule{})
+	testRule(t, "go1.15/redundant_test_main_exit", &rule.RedundantTestMainExitRule{})
+	testRule(t, "redundant_test_main_exit_test", &rule.RedundantTestMainExitRule{})
+}
diff --git a/testdata/exported_issue_1202.go b/testdata/exported_issue_1202.go
new file mode 100644
index 0000000..aad298d
--- /dev/null
+++ b/testdata/exported_issue_1202.go
@@ -0,0 +1,27 @@
+package golint
+
+import (
+	_ "embed"
+)
+
+var A []byte // MATCH /exported var A should have comment or be unexported/
+
+var B string // MATCH /exported var B should have comment or be unexported/
+
+//go:embed foo.txt
+var C []byte // MATCH /exported var C should have comment or be unexported/
+
+//go:generate pwd
+var D string // MATCH /exported var D should have comment or be unexported/
+
+func E() string { // MATCH /exported function E should have comment or be unexported/
+	return "E"
+}
+
+//nolint:gochecknoglobals
+func F() string { // MATCH /exported function F should have comment or be unexported/
+	return "F"
+}
+
+//nolint:gochecknoglobals
+const G = "G" // MATCH /exported const G should have comment or be unexported/
diff --git a/testdata/flag_param.go b/testdata/flag_param.go
index 3b235a6..e04d7c7 100644
--- a/testdata/flag_param.go
+++ b/testdata/flag_param.go
@@ -1,11 +1,19 @@
 package fixtures
 
-func foo(a bool, b int) { // MATCH /parameter 'a' seems to be a control flag, avoid control coupling/
+func fooFlagP(a bool, b int) { // MATCH /parameter 'a' seems to be a control flag, avoid control coupling/
 	if a {
 
 	}
 }
 
-func foo(a bool, b int) {
+func barFlagP(a bool, b int) {
 	str := mystruct{a, b}
 }
+
+// issue #1211
+func bazFlagP(a int, b bool) {
+	lBool := true
+	if lBool {
+		// do something
+	}
+}
diff --git a/testdata/go1.15/go.mod b/testdata/go1.15/go.mod
new file mode 100644
index 0000000..f27de7b
--- /dev/null
+++ b/testdata/go1.15/go.mod
@@ -0,0 +1,3 @@
+module github.com/mgechev/revive/testdata
+
+go 1.15
diff --git a/testdata/go1.15/redundant_test_main_exit.go b/testdata/go1.15/redundant_test_main_exit.go
new file mode 100644
index 0000000..69fb6df
--- /dev/null
+++ b/testdata/go1.15/redundant_test_main_exit.go
@@ -0,0 +1,23 @@
+package fixtures
+
+import (
+	"fmt"
+	"os"
+	"testing"
+)
+
+func TestMain(m *testing.M) {
+	setup()
+	i := m.Run()
+	teardown()
+	// must not match because this is not a test file
+	os.Exit(i)
+}
+
+func setup() {
+	fmt.Println("Setup")
+}
+
+func teardown() {
+	fmt.Println("Teardown")
+}
diff --git a/testdata/go1.15/redundant_test_main_exit_test.go b/testdata/go1.15/redundant_test_main_exit_test.go
new file mode 100644
index 0000000..95dd88b
--- /dev/null
+++ b/testdata/go1.15/redundant_test_main_exit_test.go
@@ -0,0 +1,38 @@
+package fixtures
+
+import (
+	"fmt"
+	"os"
+	"syscall"
+	"testing"
+)
+
+func TestMain(m *testing.M) {
+	setup()
+	i := m.Run()
+	teardown()
+	os.Exit(i)      // MATCH /redundant call to os.Exit in TestMain function, the test runner will handle it automatically as of Go 1.15/
+	syscall.Exit(i) // MATCH /redundant call to syscall.Exit in TestMain function, the test runner will handle it automatically as of Go 1.15/
+}
+
+func setup() {
+	fmt.Println("Setup")
+}
+
+func teardown() {
+	fmt.Println("Teardown")
+}
+
+func Test_function(t *testing.T) {
+	t.Error("Fail")
+}
+
+func Test_os_exit(t *testing.T) {
+	// must not match because this is not TestMain function
+	os.Exit(1)
+}
+
+func Test_syscall_exit(t *testing.T) {
+	// must not match because this is not TestMain function
+	syscall.Exit(1)
+}
diff --git a/testdata/redundant_test_main_exit_test.go b/testdata/redundant_test_main_exit_test.go
new file mode 100644
index 0000000..13ece89
--- /dev/null
+++ b/testdata/redundant_test_main_exit_test.go
@@ -0,0 +1,27 @@
+package fixtures
+
+import (
+	"fmt"
+	"os"
+	"testing"
+)
+
+func TestMain(m *testing.M) {
+	setup()
+	i := m.Run()
+	teardown()
+	// must not match because the go version of this module is less than 1.15
+	os.Exit(i)
+}
+
+func setup() {
+	fmt.Println("Setup")
+}
+
+func teardown() {
+	fmt.Println("Teardown")
+}
+
+func Test_function(t *testing.T) {
+	t.Error("Fail")
+}