Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cli/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

@chavacava chavacava Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact we disable the rule in our own code makes me think on the pertinence of checking calls to flag.Parse outside the main function.

Copy link
Contributor

@ccoVeille ccoVeille Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's because the right way to do is to use either this

func main() {

	// (...)
	flag.BoolVar(&setExitStatus, "set_exit_status", false, exitStatusUsage)
	flag.IntVar(&maxOpenFiles, "max_open_files", 0, maxOpenFilesUsage)
	flag.Parse()

	// (...)
}

or this

func main() {
	err := run()
	// (...)
}

func run() error {
	fl := flag.NewFlagSet(os.Args[0], flag.ContinueOnError)
	fl.BoolVar(&setExitStatus, "set_exit_status", false, exitStatusUsage)
	fl.IntVar(&maxOpenFiles, "max_open_files", 0, maxOpenFilesUsage)
	err := fl.Parse(os.Args[1:])
	if err != nil {
		return err
	}
	// (...)

	return nil
}

You can take a look at the code I wrote for gh-reaction

But maybe, the error message for flag.Parse should be way clearer, about the fact there is a need to use flag.ContinueOnError

Please note also, that the same code, using flag.NewFlagSet(os.Args[0], flag.ExitOnError) or flag.NewFlagSet(os.Args[0], flag.PanicOnError) should be reported as deep-exit, but it would be a next move, and somehow a niche maybe

EDIT: it's not a niche :P https://github.com/search?q=language%3Ago%20%2Fflag%5C.NewFlagSet.*flag%5C.(Exit%7CPanic)OnError%2F%20-is%3Afork%20-is%3Aarchived&type=code

}

// getVersion returns build info (version, commit, date, and builtBy).
Expand Down
10 changes: 9 additions & 1 deletion rule/deep_exit.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,21 @@

pkg := id.Name
fn := fc.Sel.Name
if isCallToExitFunction(pkg, fn) {
switch {
case isCallToExitFunction(pkg, fn):
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),
})
case isConditionalExitFunction(pkg, fn, ce):

Check failure on line 75 in rule/deep_exit.go

View workflow job for this annotation

GitHub Actions / Lint Go

File is not properly formatted (goimports)

Check failure on line 75 in rule/deep_exit.go

View workflow job for this annotation

GitHub Actions / Lint Go

File is not properly formatted (gofmt)

Check failure on line 75 in rule/deep_exit.go

View workflow job for this annotation

GitHub Actions / Lint Go

File is not properly formatted (gci)
w.onFailure(lint.Failure{
Confidence: 1,
Node: ce,
Category: lint.FailureCategoryBadPractice,
Failure: fmt.Sprintf("calls to %s.%s with exit-triggering argument only in main() or init() functions", pkg, fn),
})
}

return w
Expand Down
27 changes: 27 additions & 0 deletions rule/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import (
"fmt"
"go/ast"
"go/token"
"regexp"
"strings"
Expand All @@ -21,6 +22,7 @@
"Panicf": true,
"Panicln": true,
},
"flag": {"Parse": true},
}

func srcLine(src []byte, p token.Position) string {
Expand Down Expand Up @@ -90,6 +92,31 @@
return exitFunctions[pkgName] != nil && exitFunctions[pkgName][functionName]
}

var conditionalExitFunctions = map[string]map[string]func(*ast.CallExpr) bool{
"flag": {

Check failure on line 96 in rule/utils.go

View workflow job for this annotation

GitHub Actions / Lint Go

File is not properly formatted (goimports)

Check failure on line 96 in rule/utils.go

View workflow job for this annotation

GitHub Actions / Lint Go

File is not properly formatted (gofmt)

Check failure on line 96 in rule/utils.go

View workflow job for this annotation

GitHub Actions / Lint Go

File is not properly formatted (gci)
"NewFlagSet": func(ce *ast.CallExpr) bool {
if len(ce.Args) == 2 {
if arg, ok := ce.Args[1].(*ast.SelectorExpr); ok {
if id, ok := arg.X.(*ast.Ident); ok {
return id.Name == "flag" && arg.Sel.Name == "ExitOnError"
}
}
}
return false
},
},
}

// isConditionalExitFunction checks if the function call is a call to a conditional exit function.
func isConditionalExitFunction(pkgName, functionName string, ce *ast.CallExpr) bool {
if m, ok := conditionalExitFunctions[pkgName]; ok {
if check, ok := m[functionName]; ok {
return check(ce)
}
}
return false
}

// newInternalFailureError returns a slice of Failure with a single internal failure in it.
func newInternalFailureError(e error) []lint.Failure {
return []lint.Failure{lint.NewInternalFailure(e.Error())}
Expand Down
13 changes: 13 additions & 0 deletions testdata/deep_exit.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package fixtures

import (
"flag"
"log"
"os"
"syscall"
Expand Down Expand Up @@ -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 exit-triggering argument only in main() or init() functions/
}

func flagNewFlagSetContinueOnErrorOK() {
flag.NewFlagSet("cmd", flag.ContinueOnError)
}
Loading