- 
                Notifications
    You must be signed in to change notification settings 
- Fork 312
deep-exit: detect exit-triggering flag usage #1544
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
deep-exit: detect exit-triggering flag usage #1544
Conversation
Signed-off-by: Godsgift Uloamaka <[email protected]>
| @alexandear Can you please help review this PR. | 
| 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 | 
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| Thank you @uloamaka for the PR and @alexandear @ccoVeille for the feedback | 
This PR resolves #1523,
Packages using
flag.ExitOnErrororflag.Parse()can exit on invalid flags or--help, which violates the principle of reusability the deep-exit rule enforces.This PR enhances the deep-exit rule to detect indirect program termination caused by the Go flag package.
Specifically, it adds checks for:
flag.NewFlagSetcreated withflag.ExitOnErrorflag.Parse()outside ofmain()orinit()These usages can terminate programs unexpectedly, reducing reusability of packages that expose such functions.
Updates: