Repository: mgechev/revive (go)

Issue:
unconditional-recursive warn where no any reasone
**Describe the bug**
Show warning where is no any reasone

**To Reproduce**

```go

// NewFactory - создает фабрику калькуляций
func NewFactory(options ICalculationFactoryOptions) *Factory {
	return &Factory{factoryOptions: options}
}

var defaultFactory = NewFactory(DefaultFactoryOptions())
```

Will warn about `unconditional-recursive` - where is no any reasone to do it

Pull request description:
fix: unconditional-recursion false positive when the function is called right after its declaration (#1212)
The root cause of the bug was that the analyzer  (AST walker) was not reinitialized at the end of function bodies. Thus, if the function was called outside the body (like in the example below) the analyzer raised a failure.

```go
func foo() int {
	return 0
}

var f = foo()
```

The fix is: iterate over global declarations of the file and create an analyzer for each function declaration (instead of create a single analyzer for the whole file) Like that, each analyzer is scoped within a single function body.

Closes #1212


Patch:
diff --git a/README.md b/README.md
index 0442ffe..cb23e3e 100644
--- a/README.md
+++ b/README.md
@@ -560,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/unconditional_recursion.go b/rule/unconditional_recursion.go
index b611d24..b59275d 100644
--- a/rule/unconditional_recursion.go
+++ b/rule/unconditional_recursion.go
@@ -17,8 +17,35 @@ func (*UnconditionalRecursionRule) Apply(file *lint.File, _ lint.Arguments) []li
 		failures = append(failures, failure)
 	}
 
-	w := &lintUnconditionalRecursionRule{onFailure: onFailure}
-	ast.Walk(w, file.AST)
+	// Range over global declarations of the file to detect func/method declarations and analyze them
+	for _, decl := range file.AST.Decls {
+		n, ok := decl.(*ast.FuncDecl)
+		if !ok {
+			continue // not a func/method declaration
+		}
+
+		if n.Body == nil {
+			continue // func/method with empty body => it can not be recursive
+		}
+
+		var rec *ast.Ident
+		switch {
+		case n.Recv == nil:
+			rec = nil
+		case n.Recv.NumFields() < 1 || len(n.Recv.List[0].Names) < 1:
+			rec = &ast.Ident{Name: "_"}
+		default:
+			rec = n.Recv.List[0].Names[0]
+		}
+
+		w := &lintUnconditionalRecursionRule{
+			onFailure:   onFailure,
+			currentFunc: &funcStatus{&funcDesc{rec, n.Name}, false},
+		}
+
+		ast.Walk(w, n.Body)
+	}
+
 	return failures
 }
 
@@ -50,8 +77,7 @@ type lintUnconditionalRecursionRule struct {
 	inGoStatement bool
 }
 
-// Visit will traverse the file AST.
-// The rule is based in the following algorithm: inside each function body we search for calls to the function itself.
+// Visit will traverse function's body we search for calls to the function itself.
 // We do not search inside conditional control structures (if, for, switch, ...) because any recursive call inside them is conditioned
 // We do search inside conditional control structures are statements that will take the control out of the function (return, exit, panic)
 // If we find conditional control exits, it means the function is NOT unconditionally-recursive
@@ -59,17 +85,6 @@ type lintUnconditionalRecursionRule struct {
 // In resume: if we found a recursive call control-dependent from the entry point of the function then we raise a failure.
 func (w *lintUnconditionalRecursionRule) Visit(node ast.Node) ast.Visitor {
 	switch n := node.(type) {
-	case *ast.FuncDecl:
-		var rec *ast.Ident
-		switch {
-		case n.Recv == nil:
-			rec = nil
-		case n.Recv.NumFields() < 1 || len(n.Recv.List[0].Names) < 1:
-			rec = &ast.Ident{Name: "_"}
-		default:
-			rec = n.Recv.List[0].Names[0]
-		}
-		w.currentFunc = &funcStatus{&funcDesc{rec, n.Name}, false}
 	case *ast.CallExpr:
 		// check if call arguments has a recursive call
 		for _, arg := range n.Args {


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/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/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")
+}
diff --git a/testdata/unconditional_recursion.go b/testdata/unconditional_recursion.go
index a35e9cc..be91a9f 100644
--- a/testdata/unconditional_recursion.go
+++ b/testdata/unconditional_recursion.go
@@ -199,3 +199,10 @@ func nr902() {
 		nr902() // MATCH /unconditional recursive call/
 	}()
 }
+
+// Test for issue #1212
+func NewFactory() int {
+	return 0
+}
+
+var defaultFactory = NewFactory()