go: proposal: flag: add Fatalf

Problem

Setting up command-line flags with flag is easy. For most cases, the built-in restrictions are enough - for example, if one wants to accept only unsigned integers, one can use flag.Uint.

Adding one’s restrictions to a flag is also fairly straightforward by implementing the flag.Value interface. For example, we could use this to enforce that a flag only accept integers between 1 and 10:

invalid value "13" for flag -test.bar: number must be between 1 and 10
Usage of foo: [...]

However, there’s another kind of restriction that is common - the kind that concerns multiple flags or external factors. Let’s take one example right from the standard library, from cmd/vet/all/main.go:

flag.Parse()
switch {
case *flagAll && *flagPlatforms != "":
        log.Print("-all and -p flags are incompatible")
        flag.Usage()
        os.Exit(2)

At the time of writing, the best way to implement such restrictions is by printing the error, calling Usage() and calling os.Exit. This has several shortcomings:

It’s somewhat complex. From experience, some implementations forget parts of it, like calling Usage.
Programs hard-code the os.Exit(2) call, instead of letting the flag package handle exiting itself.
The way the error is printed varies greatly; the most common is fmt.Fprint{ln,f}(os.Stderr, ... yet this one calls log.Print.
No matter what output the error is printed to, it’s wrong as it doesn’t match flag.SetOutput.

The last point, while important, could be solved with https://github.com/golang/go/issues/17628. However, that would add a bit more complexity to the code that everyone must write correctly, and I’m sure plenty of implementations would keep on hard-coding others like os.Stderr.

Proposed solution

I propose adding two new functions to the flag package:

func (f *FlagSet) Fatalf(format string, a ...interface{}) {
        fmt.Fprintf(f.out(), format, a...)
        f.usage()
        os.Exit(2)
}

func Fatalf(format string, a ...interface{}) {
        CommandLine.Fatalf(format, a...)
}

Then our real example from above would become simply:

flag.Parse()
switch {
case *flagAll && *flagPlatforms != "":
        flag.Fatalf("-all and -p flags are incompatible")

And it would have zero margin for error, including its use of log and the missing flag.Output() piece.

I’ll submit the patch with this very change now. Happy to work on transitioning all these cases in the standard library to Fatalf if it gets accepted and merged.

CC @josharian who had a similar idea and gave his input CC @dominikh who might be interested given his other flag package API issue CC @robpike as per golang.org/s/owners

About this issue

Original URL
State: closed
Created 7 years ago
Reactions: 8
Comments: 18 (17 by maintainers)

Commits related to this issue

staticcheck: remove CheckFlagUsage (SA1022) The Go team clarified that calling os.Exit in usage functions is perfectly fine. See the following links for further information: - https://github.com/rsc... — committed to dominikh/go-tools by dominikh 7 years ago

Most upvoted comments

I’m really excited about this proposal; I am frequently frustrated by the amount of boilerplate involved when checking flags for correctness.

On reflection, I’m in favor of only adding the top level Fatalf; if you’re using a custom FlagSet, you probably want more sophisticated handling anyway. And it solves a few of the problems @shurcooL brought up.

As for Fatal and Fatalln: Just say no? (Or just say yes to Fatal?)

josharian on Aug 19, 2017

Flag already has enough semantics around error-handling. I don’t believe we should add more.

I assert the following:

It should be pretty clear that flag.PrintDefaults does not also exit. It only prints.
It should be clear from go doc flag.Usage (see below) that flag.Usage need not exit, since the default one does not.
It should be clear from the description of how it is used and also from many examples that it’s OK for you to write a flag.Usage function that does exit. If that’s unclear, we can add words to the doc comment making it explicit.
If I write a function called usage that exits, and I call usage(), then it’s perfectly fine for me to know and depend on the fact that usage exits. The choice of unexported function name is not something we attempt to mandate.
If I assign my usage function to flag.Usage and then I call flag.Usage, it remains perfectly fine for me to know and depend on the fact that flag.Usage exits.

Any attempt to introduce new API here doesn’t invalidate any of the above, so new API is not necessary.

If new API were added, it seems clear from the discussion and past pull requests that the next thing that would happen is there would be many changes going around trying to update code to use flag.Fatalf instead of flag.Usage (in programs where flag.Usage has been reassigned to a function that exits). That’s completely unnecessary churn, and cutting off that churn seems to me an equally good reason not to introduce new API, even beyond the fact that it’s unnecessary.

If not for testing, I think flag.Usage would have exited from the start, but changing that now is an example of something that’s high cost, low reward.

$ go doc flag.Usage
var Usage = func() {
	fmt.Fprintf(CommandLine.out(), "Usage of %s:\n", os.Args[0])
	PrintDefaults()
}
    Usage prints a usage message documenting all defined command-line flags to
    CommandLine's output, which by default is os.Stderr. 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.

$

rsc on Aug 21, 2017

It might be useful to take a couple of existing Go programs and rewrite them using this hypothetical API addition to see what it looks like, and how much it saves.

Some potential concerns:

Fatalf name implies exiting… In fact, if Fatalf does NOT call os.Exit unconditionally, I would argue that is very misleading and harmful, because it will make Go programmers spend more time whenever reviewing any code that calls a Fatalf method: “is it okay there’s no return after this Fatalf call? Yes, it is, because this is log.Fatalf and not flag.Fatalf.” That would be very bad.

A big however here is there could be flag.NewFlagSet("name", flag.ContinueOnError) and flag.NewFlagSet("name", flag.PanicOnError) flag sets out there. Would it make sense for Fatalf to do os.Exit(2) in those cases?
log.Fatalf exits with status code 1, but flag usage errors typically exit with status code 2. Is it okay and intuitive that Fatalf will use a different status code, depending on whether it’s log.Fatalf vs flag.Fatalf? Maybe it is, maybe not, just raising the question.
This could be a slippery slope, where after adding Fatalf, people might want to see Fatal and Fatalln variants being added too. That’d be 6 new identifiers in total.

Possible simplifications:

Instead of adding both func (f *FlagSet) Fatalf(format string, a ...interface{}) method and func Fatalf(format string, a ...interface{} function, consider adding only the package level function. It can benefit from the fact that top level flag set always uses flag.ExitOnError as its error handling, so hardcoding os.Exit(2) could make a lot of sense.
Does this have to be in standard library right away? Could we try it out as an external package implementation first? And consider moving into flag later? Aside from needing f.out(), this doesn’t require any internal APIs, that even that might get made exported if #17628 is resolved. Until then, using os.Stderr can be a sufficient temporary placeholder solution.

dmitshur on Aug 19, 2017