Don't os.Exit(2) inside flag.Usage #1

Do this:

flag.Usage = func() {
	fmt.Fprintf(os.Stderr, "Usage of %s:\n", os.Args[0])
	flag.PrintDefaults()
}

Don't do this:

flag.Usage = func() {
	fmt.Fprintf(os.Stderr, "Usage of %s:\n", os.Args[0])
	flag.PrintDefaults()
	os.Exit(2)
}

The os.Exit call is unnecessary because flag package is already responsible for doing it. See here and here for examples.

What in the flag package does this? I had a look at the source and see that it will do that for parsing errors, but I can't see how else it might happen?

@jabley Take a look at the default value of Usage, it does not call os.Exit(2):

// Usage prints to standard error a usage message documenting all defined command-line flags.
// It is called when an error occurs while parsing flags.
// The function is a variable that may be changed to point to a custom function.
// By default it prints a simple header and calls PrintDefaults; for details about the
// format of the output and how to control it, see the documentation for PrintDefaults.
var Usage = func() {
	fmt.Fprintf(os.Stderr, "Usage of %s:\n", os.Args[0])
	PrintDefaults()
}

When you're using the default flag set and you call flag.Parse(), it executes:

// Ignore errors; CommandLine is set for ExitOnError.
CommandLine.Parse(os.Args[1:])

CommandLine is defined as:

// CommandLine is the default set of command-line flags, parsed from os.Args.
// The top-level functions such as BoolVar, Arg, and so on are wrappers for the
// methods of CommandLine.
var CommandLine = NewFlagSet(os.Args[0], ExitOnError)

Notice ExitOnError used as the ErrorHandling value.

That causes os.Exit(2) to get executed on this line when there's an error parsing flags.

I've also edited the original post to include two examples of where I saw this happen in real world:

See here and here for examples.

I agree with you, but it seems some people don't :) https://go-review.googlesource.com/c/45870/

I've seen similar cases where the Go codebase itself has explicit breaks in empty case blocks on purpose. I'm also not a big fan, but I guess it comes down to personal/project preference.

@mvdan, that's a bummer. I've also had an unsuccessful experience with Rob Pike on this change:

https://github.com/robpike/ivy/pull/37

So far, he's only person who has responded in such a way. Everyone else has appreciated the improvement and incorporated it into their codebase. For example:

• https://github.com/mdempsky/unconvert/commit/3b9aa41c71a855a5f5fee96dc751979ce6e8cbbe
• https://github.com/campoy/embedmd/pull/12
• https://upspin-review.googlesource.com/c/9642/

Rob Pike has not provided any convincing arguments for why his style is better, he just says it's "nice and clear". I don't agree with that. Consider the following segment of Go code in func main():

if *flagFoo == "bar" {
	fmt.Fprintln(os.Stderr, "flag -foo must not be bar")
	flag.Usage()
}
run(*flagFoo)

Can you tell what will happen if an invalid value for -foo flag is provided? It will print a message and the usage string to stderr... and maybe proceed to run? Or maybe exit? You can't tell without looking at the usage func implementation. If it's the default one from flag package, it won't exit. If it's a custom one, I'd expect it not to exit either.

Contrast that with the following alternative code:

if *flagFoo == "bar" {
	fmt.Fprintln(os.Stderr, "flag -foo must not be bar")
	flag.Usage()
	os.Exit(2)
}
run(*flagFoo)

To me, that's nice and clear. It's also correct API usage, which is important to me.

Given your example, I too think this is a rule to follow.

I wonder if the flag package should be strict about usage funcs not calling exit on their own. There is no reason why they should, and as you say it can lead to confusion.

Then again, I doubt the godoc would say anything more than "should not", so not much might change.

In theory, people can put arbitrary code inside the usage func. I don't think that the docs need to police against that. The docs at https://godoc.org/flag#Usage currently say:

Usage prints to standard error a usage message documenting all defined command-line flags.

And I think that's completely fine. It says what the purpose/responsibility of the func is. That implies it shouldn't do extraneous stuff.

As far as I can tell, the only reason people sometimes put os.Exit(2) in usage funcs is because of some bad examples in GOROOT that they copy/paste code or learn from. I doubt anyone would think of doing that if they begin by reading flag package docs.