proposal: flag: add Fatalf

[mvdan]

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 #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

[mvdan]

Another point in favor of not hard-coding os.Exit(2) is that ErrorHandling could be obeyed in Fatalf. I'm not sure how useful this would be, however.

[dmitshur]

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 flag: no way to access a FlagSet's output #17628 is resolved. Until then, using os.Stderr can be a sufficient temporary placeholder solution.

[josharian]

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?)

[mvdan]

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.

Sure; I'll send a CL tomorrow (after the one with this API change) with a bunch of changes using the new API.

In fact, if Fatalf does NOT call os.Exit unconditionally, I would argue that is very misleading and harmful

Agreed. When I said ErrorHandling, I didn't think of this; it was a last-minute thought.

Is it okay and intuitive that Fatalf will use a different status code, depending on whether it's log.Fatalf vs flag.Fatalf?

The point of flag.Fatalf is to be consistent with how flag itself reports errors, so I'd say that using an exit code 2 is important. I'd say Fatal implies exiting, but not necessarily an exit code of exactly 1.

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

I share Dmitri's concern. However, there is little precedent for having all three besides convenience - and this is such a niche API, that I'd say Fatalf is more than enough. I can be convinced otherwise, though.

adding only the package level function

Sounds like a good idea, and I agree with Josh that those defining their own flagsets are probably fine with more boilerplate and should get all the details right.

Could we try it out as an external package implementation first? And consider moving into flag later?

While I would normally agree, I don't think it's a good idea in this case. Not because of the reflection needed, but rather because noone would depend on a third-party library to avoid two extra lines of code. I think a CL showing its impact on the standard library itself should be enough on its own.

If anyone wants to write a package and recommend its use, by all means go ahead; I just don't think it's worth it and won't do it myself.

[dmitshur]

Sure; I'll send a CL tomorrow (after the one with this API change) with a bunch of changes using the new API.

Great, thanks.

I'm eager to try this sooner, so this will be a thought experiment/case study on the robpike.io/ivy package.

Its current relevant code is this:

func main() {
	flag.Usage = usage
	flag.Parse()

	if *origin != 0 && *origin != 1 {
		fmt.Fprintf(os.Stderr, "ivy: illegal origin value %d\n", *origin)
		os.Exit(2)
	}

	if *gformat {
		*format = "%.12g"
	}

	conf.SetFormat(*format)
	conf.SetMaxBits(*maxbits)
	conf.SetMaxDigits(*maxdigits)
	conf.SetOrigin(*origin)
	conf.SetPrompt(*prompt)
	if len(*debugFlag) > 0 {
		for _, debug := range strings.Split(*debugFlag, ",") {
			if !conf.SetDebug(debug, true) {
				fmt.Fprintf(os.Stderr, "ivy: unknown debug flag %q\n", debug)
				os.Exit(2)
			}
		}
	}

	...
}

func usage() {
	fmt.Fprintf(os.Stderr, "usage: ivy [options] [file ...]\n")
	fmt.Fprintf(os.Stderr, "Flags:\n")
	flag.PrintDefaults()
	os.Exit(2)
}

Given your proposal so far, I believe that code can be changed as follows:

func main() {
	flag.Usage = usage
	flag.Parse()

	if *origin != 0 && *origin != 1 {
-		fmt.Fprintf(os.Stderr, "ivy: illegal origin value %d\n", *origin)
-		os.Exit(2)
+		flag.Fatalf("ivy: illegal origin value %d\n", *origin)
	}

	if *gformat {
		*format = "%.12g"
	}

	conf.SetFormat(*format)
	conf.SetMaxBits(*maxbits)
	conf.SetMaxDigits(*maxdigits)
	conf.SetOrigin(*origin)
	conf.SetPrompt(*prompt)
	if len(*debugFlag) > 0 {
		for _, debug := range strings.Split(*debugFlag, ",") {
			if !conf.SetDebug(debug, true) {
-				fmt.Fprintf(os.Stderr, "ivy: unknown debug flag %q\n", debug)
-				os.Exit(2)
+				flag.Fatalf("ivy: unknown debug flag %q\n", debug)
			}
		}
	}

	...
}

func usage() {
	fmt.Fprintf(os.Stderr, "usage: ivy [options] [file ...]\n")
	fmt.Fprintf(os.Stderr, "Flags:\n")
	flag.PrintDefaults()
-	os.Exit(2)
}

That looks like a pretty nice simplification to me.

There were rejections of removing os.Exit(2) from usage function in the past, with the argument that "it's magical that os.Exit(2) gets called correctly." Would this proposal address that concern? Or would people want to keep the os.Exit(2) after flag.Fatalf, weakening its usefulness?

[mvdan]

That looks like a pretty nice simplification to me.

Glad to hear that. To me, the most important part is reducing the amount of things that developers can get wrong for no good reason.

Would this proposal address that concern? Or would people want to keep the os.Exit(2) after flag.Fatalf, weakening its usefulness?

I think you're mixing two slightly different issues here.

One is calling os.Exit or return after flag.Fatalf. I think we can all agree that will be useless, as the func will always os.Exit.

The other is having an os.Exit inside an Usage implementation. I do think this proposal would help to dispel the arguments in its favor - this is what I meant by my comment in mdempsky/unconvert@3b9aa41#commitcomment-23728301.

However, I think this proposal holds enough value on its own, and that the "Usage calling Exit" debate need not happen at the same time. I fear this proposal might get into an endless debate over implementing Usage, while that is not crucial to whether or not Fatalf is introduced.

[gopherbot]

Change https://golang.org/cl/57311 mentions this issue: flag: add Fatalf package level func

[robpike]

We would prefer you wait for the proposal to be accepted before sending a code review implementing it. The discussion should be here, rather than in the code review, plus it might not be accepted.

[mvdan]

We would prefer you wait for the proposal to be accepted before sending a code review implementing it. The discussion should be here, rather than in the code review, plus it might not be accepted.

Of course - I didn't spend too much time on the CL since I know it might never get merged. I'll change it to "DO NOT SUBMIT" to make it clearer.

As for the discussion, note that all that's being talked about in the CL is the godoc and the test implementation. I would redirect any proposal discussion here, but there hasn't been any over there.

[griesemer]

I'm slightly in favor of this proposal. I rarely write tools that need to handle flags, but when I do, I always have to look up the pattern for correct error/usage/reporting.

[rsc]

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]

Also, note that in the proposed rewriting of ivy above, the diff changes two error exits that did not print the flag default dump into error exits that do. That seems undesirable to me in general (and, I suspect, to @robpike, or he'd have called flag.PrintDefaults explicitly). In many of these cases, dumping all the flags doesn't actually add to the error message - they're just noise - and we don't want to make the low-SNR outputs the ones that are easy to generate.

[griesemer]

I'm convinced. We should not do this.

[dmitshur]

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.

It is unclear. Adding words to the doc comment making it explicit would make it very clear.

In general, I don't believe it's possible to determine that something is correct from seeing many examples of it, especially when what's being done is counter-intuitive. It could be that many people are making a mistake without realizing it (and many of them just copied the code, without understanding it, from one another). That's why the only reliable source is the documentation.

I agree with the arguments presented and happy with the outcome, as long as the flag.Usage documentation will be clarified.

[mvdan]

Thanks for the detailed counter-argument, @rsc.

I'm a bit confused by how it centers around flag.Usage calling os.Exit. I understand that, if its godoc says it's OK for implementations to exit, and if one's implementations always exit, then one can avoid the repeated os.Exit call and remove part of the complexity needed without Fatalf.

However, that doesn't answer the other three points made in my original post. The boilerplate was the main argument of this proposal, and even though two lines are fewer, I also think it's still a point to consider.

I too am OK with this proposal being declined if there is no consensus. That should be the outcome for a proposal that intends to add new API. I would prefer to have this API out of convenience and to not have to os.Exit in my usage implementations, but I can live without those.

[rsc]

@mvdan, yes, sorry the discussion kind of diverged into flag.Usage and os.Exit. I do see that your original post was not directly about that.

My second reply above was the important one for the original proposal: if you make it easier to "print a message, print the usage, and exit", people will do that too often. The vast majority of the time, printing the full usage is mostly not helpful. Using your example in the original post above, if I pass an invalid value to -bar, it's good to point out my error using -bar. It doesn't help to then print information about 10 unrelated flags, but if you introduce the flag.Fatalf above, that's what will always happen. If anything I think people call flag.Usage too often (the vet example being one instance of this).

In my view the real problem here is that log.Fatalf is unusable in this context without proper initialization. If I had it to do over again (and I don't believe I do), I would make

log.SetFlags(0)
log.SetPrefix(filepath.Base(os.Args[0])+": ")

the default settings for package log, and then the right thing to do would be to call log.Fatalf, not a new flag.Fatalf that also prints mostly unrelated information.

[mvdan]

I agree that more often than not, printing the usage information is not very helpful. However, would that not apply to the way the flag package errors on invalid flag values too? In the -bar example above, if it expects an integer but you pass a string that isn't one, it will error and print the whole usage information.

It's an interesting point you raise about the log package. There is a slight problem with that one, though, and it's how log.SetOutput may differ from flag.CommandLine.SetOutput (as in the writers in each). In most cases they will both be os.Stderr as per the defaults, but things can still go wrong.

And it would also exit with exit code 1 instead of 2, which I think is fairly important.

There's always the option of introducing flag.Fatalf as originally proposed, but without flag.Usage(). That is, like the log.Fatalf default that you showed above. It is perhaps a middle ground between what I proposed and what you described here. Perhaps it's not complex enough to warrant its addition to the flag package, though.

[rsc]

Declining this proposal but I filed #21671 for documenting that flag.Usage implementations can exit.