feature(deep-exit): detect exit-triggering flag usage (#1544)

cli/main.go

@@ -173,7 +173,7 @@ func initConfig() {
 	flag.BoolVar(&versionFlag, "version", false, versionUsage)
 	flag.BoolVar(&setExitStatus, "set_exit_status", false, exitStatusUsage)
 	flag.IntVar(&maxOpenFiles, "max_open_files", 0, maxOpenFilesUsage)
-	flag.Parse()
+	flag.Parse() //revive:disable-line:deep-exit
 }
 
 // getVersion returns build info (version, commit, date, and builtBy).

rule/deep_exit.go

@@ -7,6 +7,7 @@ import (
 	"unicode"
 	"unicode/utf8"
 
+	"github.com/mgechev/revive/internal/astutils"
 	"github.com/mgechev/revive/lint"
 )
 
@@ -64,12 +65,18 @@ func (w *lintDeepExit) Visit(node ast.Node) ast.Visitor {
 
 	pkg := id.Name
 	fn := fc.Sel.Name
-	if isCallToExitFunction(pkg, fn) {
+	if isCallToExitFunction(pkg, fn, ce.Args) {
+		msg := fmt.Sprintf("calls to %s.%s only in main() or init() functions", pkg, fn)
+
+		if pkg == "flag" && fn == "NewFlagSet" &&
+			len(ce.Args) == 2 && astutils.IsPkgDotName(ce.Args[1], "flag", "ExitOnError") {
+			msg = "calls to flag.NewFlagSet with flag.ExitOnError only in main() or init() functions"
+		}
 		w.onFailure(lint.Failure{
 			Confidence: 1,
 			Node:       ce,
 			Category:   lint.FailureCategoryBadPractice,
-			Failure:    fmt.Sprintf("calls to %s.%s only in main() or init() functions", pkg, fn),
+			Failure:    msg,
 		})
 	}
 

rule/redundant_test_main_exit.go

@@ -66,7 +66,7 @@ func (w *lintRedundantTestMainExit) Visit(node ast.Node) ast.Visitor {
 
 	pkg := id.Name
 	fn := fc.Sel.Name
-	if isCallToExitFunction(pkg, fn) {
+	if isCallToExitFunction(pkg, fn, ce.Args) {
 		w.onFailure(lint.Failure{
 			Confidence: 1,
 			Node:       ce,

rule/unconditional_recursion.go

@@ -190,9 +190,7 @@ func (*lintUnconditionalRecursionRule) hasControlExit(node ast.Node) bool {
 
 			functionName := se.Sel.Name
 			pkgName := id.Name
-			if isCallToExitFunction(pkgName, functionName) {
-				return true
-			}
+			return isCallToExitFunction(pkgName, functionName, n.Args)
 		}
 
 		return false

rule/utils.go

@@ -2,24 +2,40 @@ package rule
 
 import (
 	"fmt"
+	"go/ast"
 	"go/token"
 	"regexp"
 	"strings"
 
+	"github.com/mgechev/revive/internal/astutils"
 	"github.com/mgechev/revive/lint"
 )
 
+// exitChecker is a function type that checks whether a function call is an exit function.
+type exitFuncChecker func(args []ast.Expr) bool
+
+var alwaysTrue exitFuncChecker = func([]ast.Expr) bool { return true }
+
 // exitFunctions is a map of std packages and functions that are considered as exit functions.
-var exitFunctions = map[string]map[string]bool{
-	"os":      {"Exit": true},
-	"syscall": {"Exit": true},
+var exitFunctions = map[string]map[string]exitFuncChecker{
+	"os":      {"Exit": alwaysTrue},
+	"syscall": {"Exit": alwaysTrue},
 	"log": {
-		"Fatal":   true,
-		"Fatalf":  true,
-		"Fatalln": true,
-		"Panic":   true,
-		"Panicf":  true,
-		"Panicln": true,
+		"Fatal":   alwaysTrue,
+		"Fatalf":  alwaysTrue,
+		"Fatalln": alwaysTrue,
+		"Panic":   alwaysTrue,
+		"Panicf":  alwaysTrue,
+		"Panicln": alwaysTrue,
+	},
+	"flag": {
+		"Parse": func([]ast.Expr) bool { return true },
+		"NewFlagSet": func(args []ast.Expr) bool {
+			if len(args) != 2 {
+				return false
+			}
+			return astutils.IsPkgDotName(args[1], "flag", "ExitOnError")
+		},
 	},
 }
 
@@ -86,8 +102,18 @@ func isDirectiveComment(line string) bool {
 }
 
 // isCallToExitFunction checks if the function call is a call to an exit function.
-func isCallToExitFunction(pkgName, functionName string) bool {
-	return exitFunctions[pkgName] != nil && exitFunctions[pkgName][functionName]
+func isCallToExitFunction(pkgName, functionName string, callArgs []ast.Expr) bool {
+	m, ok := exitFunctions[pkgName]
+	if !ok {
+		return false
+	}
+
+	check, ok := m[functionName]
+	if !ok {
+		return false
+	}
+
+	return check(callArgs)
 }
 
 // newInternalFailureError returns a slice of Failure with a single internal failure in it.

rule/utils_test.go

@@ -2,6 +2,7 @@ package rule
 
 import (
 	"fmt"
+	"go/ast"
 	"testing"
 )
 
@@ -9,23 +10,39 @@ func TestIsCallToExitFunction(t *testing.T) {
 	tests := []struct {
 		pkgName      string
 		functionName string
+		functionArgs []ast.Expr
 		expected     bool
 	}{
-		{"os", "Exit", true},
-		{"syscall", "Exit", true},
-		{"log", "Fatal", true},
-		{"log", "Fatalf", true},
-		{"log", "Fatalln", true},
-		{"log", "Panic", true},
-		{"log", "Panicf", true},
-		{"log", "Print", false},
-		{"fmt", "Errorf", false},
+		{"os", "Exit", nil, true},
+		{"syscall", "Exit", nil, true},
+		{"log", "Fatal", nil, true},
+		{"log", "Fatalf", nil, true},
+		{"log", "Fatalln", nil, true},
+		{"log", "Panic", nil, true},
+		{"log", "Panicf", nil, true},
+		{"flag", "Parse", nil, true},
+		{"flag", "NewFlagSet", []ast.Expr{
+			nil,
+			&ast.SelectorExpr{
+				X:   &ast.Ident{Name: "flag"},
+				Sel: &ast.Ident{Name: "ExitOnError"},
+			},
+		}, true},
+		{"log", "Print", nil, false},
+		{"fmt", "Errorf", nil, false},
+		{"flag", "NewFlagSet", []ast.Expr{
+			nil,
+			&ast.SelectorExpr{
+				X:   &ast.Ident{Name: "flag"},
+				Sel: &ast.Ident{Name: "ContinueOnError"},
+			},
+		}, false},
 	}
 
 	for _, tt := range tests {
 		t.Run(fmt.Sprintf("%s.%s", tt.pkgName, tt.functionName), func(t *testing.T) {
-			if got := isCallToExitFunction(tt.pkgName, tt.functionName); got != tt.expected {
-				t.Errorf("isCallToExitFunction(%s, %s) = %v; want %v", tt.pkgName, tt.functionName, got, tt.expected)
+			if got := isCallToExitFunction(tt.pkgName, tt.functionName, tt.functionArgs); got != tt.expected {
+				t.Errorf("isCallToExitFunction(%s, %s, %v) = %v; want %v", tt.pkgName, tt.functionName, tt.functionArgs, got, tt.expected)
 			}
 		})
 	}

testdata/deep_exit.go

@@ -1,6 +1,7 @@
 package fixtures
 
 import (
+	"flag"
 	"log"
 	"os"
 	"syscall"
@@ -36,3 +37,15 @@ func TestMain(m *testing.M) {
 	// must match because this is not a test file
 	os.Exit(m.Run()) // MATCH /calls to os.Exit only in main() or init() functions/
 }
+
+func flagParseOutsideMain() {
+	flag.Parse() // MATCH /calls to flag.Parse only in main() or init() functions/
+}
+
+func flagNewFlagSetExitOnErrorOutsideMain() {
+	flag.NewFlagSet("cmd", flag.ExitOnError) // MATCH /calls to flag.NewFlagSet with flag.ExitOnError only in main() or init() functions/
+}
+
+func flagNewFlagSetContinueOnErrorOK() {
+	flag.NewFlagSet("cmd", flag.ContinueOnError)
+}