proposal: spec: improve error handing using `guard` and `must` keywords

[markusheukelom]

I'll start with an example. Here's a version of the CopyFile example function form the proposal for improved error handling in Go 2.0, rewritting using must and guard:

func CopyFile(src, dst string) (err error) {
	defer func() {
		if err != nil {
			err = fmt.Errorf("copy %s to %s: %v", src, dst, err)
		}
	}()

	r := guard os.Open(src) 
	defer must r.Close()		

	w := guard os.Create(dst)
	defer must w.Close()

	err = io.Copy(w, r)
	
	// here we need to do extra stuff when an Copy error happens: now we must use the 'normal' error handling method, and cannot use guard or must
	if err != nil { 
		_ := os.Remove(dst) // fail silently if errors happen during error handling
	}
	return
}

The must keyword is syntactic sugar to panic on any error returned by a function call:

w := must os.Open("foo.txt")

is conceptually equivalent to

w, err := os.Open("foot.text")
if err != nil {
	panic(err)
}

The must keyword can only be used on function calls that return an error as last return argument. In functions that do not return an error as last return value, or where return values are unnamed, must is exactly equivalent to the code above. In case a function does return a named error value, when a error is returned from the call, must assigns this error to function's named return error value, then executes all deferred functions just like a normal panic, and then uses the return error value when propagating the panic upwards. As a result, a defer statement can therefore also be used to augment errors raised in a panic caused by the must keyword.

The guard keyword is syntactic sugar to return from a function when a function call returns in an error (as last argument):

func Foo() (int, string, error) {
	w := guard os.Open("foo.txt")
	// ....
}

is equivalent to:

func Foo() (int, string, error) {
	w, err := os.Open("foot.text")
	if err != nil {
		return 0, "", err	// guard returns zero values for all but the last return arguments
	}
	// ....
}

The guard keyword can be used only on function calls that return an error as last return value, in functions that also return an error as last value.

The must and guard keywords cannot be used in conjunction with the go keyword.

Benefits

The if err := ...; err != nil {} pattern has several drawbacks:

• It is so omni-present, it quickly become noise in the code
• Code like doFileStuff(os.Open("foobar.txt")) does not work, because Open also returns an (optional) error

Both of these drawbacks would disappear (in second case either guard or must can be used). Of course there is the very valid argument that errors should be augmented with more relevant error information that only the caller can provide. In general there are three situations:

1. No extra error info is needed / available
2. The extra info is general for all errors that appear in the function body
3. Extra info is added for a specific error

In the first case we can just use guard (or must) and we're done. In the second case, the defer technique can be used to add function specific information (mostly the call argument values) for all errors that are returned (including panics via must).

Finally there is the case where we want to add information to a specific error only. In that case, as well in the case more has to be done than just augmenting the error, the current if err != nil {} pattern should be used: especially if more error handling code is needed there is no real reason to move this important code elsewhere as it is specific to the code directly above it.

--

As an extra benefit, with the must keyword all functions of the MustXXX() pattern are no longer needed.

Benefits over the 2.0 proposal

My main concern with the handle keyword is that it adds another code path: the error handling path. While it would be fine for error augmentation (added relative information to the error), you need to 'read down, then read up' checking all handle clauses to see what happens on an error. This not ideal. If any additional stuff must happen that should really be done under that function call, using the if err != nil {}: it is there where the error happens and any error handling code should be below it, not somewhere above in any of multiple of levels.

The advange of must and guard, in my opinion, over handle and check are that specific error handling code stay where it is done now, but all other cases, where we need to add general function wide info, or no info at all, can be handle much more easily and cleaner.

[bcmills]

CC @mpvl @neild @jba

[ianlancetaylor]

I think that any error handling proposal should make it just as easy to annotate errors with additional information as it is to not annotate errors. That is true today, for example, where it is very easy to write return fmt.Errorf(..., err). It seems less true with this proposal.

[networkimprov]

A "default handler" for panic() is a good idea, as plenty of calls should never yield an error. But I question the wisdom of a new keyword for that sole purpose.

The Draft Design already has a default handler for return err but that may just encourage people to return errors without context, which was a major focus of the proposal.

Re check, there are other ways to dispatch errors to local handlers; see Requirements to Consider for Go 2 Error Handling.

Please add a link to this proposal on https://github.com/golang/go/wiki/Go2ErrorHandlingFeedback

[dpinela]

I like the idea of using defer for annotating errors, as a substitute for handle - it's something you can even do in current Go. My least favourite part of the error handling draft design is how handle partially overlaps with defer.

[markusheukelom]

I ran some quick numbers on the kubernetes code base (per April 15 2019) to get some idea on actual error handling usage in production quality code.

# number of 'if err != nil' (and friends):
rgrep --include='*.go' -e 'if err.*!= nil' . | grep -v generate | wc -l
58074

# number of bare error returns (no additional error handling, no error annotations)
rgrep --include='*.go' -B1 -e 'return.*err$' . | grep 'if err' | grep -v generate | wc -l
30167

# number of annotated error returns
rgrep --include='*.go' -B1 -e 'return.*fmt.Errorf(.*err.*' . | grep 'if err' | grep -v generate | wc -l
2843

# number of unique error annotation messages
rgrep --include='*.go' -e 'return.*fmt.Errorf(.*err.*' . | grep -v generate | grep -o '".*"' | sort | uniq | wc -l
2517

# number of panic(err)s
rgrep --include='*.go' -e 'panic(err)' . | grep -v generate | wc -l
702

# number of 'defer *.Close()' 
rgrep --include='*.go' -e 'defer' . | grep -v generate | grep Close | wc -l
3286

This last number is interesting: there are almost 3300 errors ignored in Kubernetes (including all dependencies). These are potential bugs. Note that it only counts unchecked Close() functions and assumes all the Close methods return an error. These lines would really benefit from a 'must' or 'guard' (or check) keyword to make actually handling and not ignoring theses errors more easy. If think this is something that deserves more attention.

Notes:

• some files have 'generated' in their names. Assuming these are generated files, I excluded those. I include everything else, including the vendor and 3rd code.
• I check the grep results a bit by hand, but I am not 100% I am not counting false positives/negatives.
• This is the first time I touch kubernetes code, I am not a k8 dev and am not using it myself (personally or professionally)

[markusheukelom]

I think that any error handling proposal should make it just as easy to annotate errors with additional information as it is to not annotate errors. That is true today, for example, where it is very easy to write return fmt.Errorf(..., err). It seems less true with this proposal.

@ianlancetaylor I agree, and my argument is to keep using the if err != nil { return fmt.Errorf(..., err) } for this purpose. Why would we need another method for that? It's the perfect place to do it! And using a defer you can write a general error annotation for a function body if needed... The biggest annoyance I run into is when returning bare errors... this feels so repetitive and 'noisy' in Go 1.x. Please see my comment above for some error pattern counts in the kubernets code: returning bare errors seems to be very common, and repeated error annotations very uncommon (almost all error annotations are unique so a 'handle' would not help). Also note that not checking errors returned from Close() is very common (probably because it is tedious to check them in a defer), not sure if the check/handle provides some improvement there...

[griesemer]

@markusheukelom I've got a few questions regarding "defer must w.Close()" in your initial example; I'm just trying to understand your proposal better. A defer statement requires a function invocation as operand, so that's not technically the case here. But let's assume the "must w.Close()" is accepted with the "defer" and that the entire stanza "must w.Close()" is run as the deferred function. So if the w.Close() reports an error, this would panic when the defer is run at the end. But if I wanted to return the w.Close() error, presumably I would want to write "defer guard w.Close()", wouldn't I? That way, if the function executes properly but for the w.Close(), I'd get the error from w.Close(). Is that your intention? And finally, if we already have an error that's being returned, and w.Close() returns another error, wouldn't that error clobber (overwrite) the prior error being returned? Or should "defer guard" be disallowed?

[markusheukelom]

@griesemer

Yes, you've understood it as I intented it to work. And yes defer guard w.Close() would overwrite any existing error value if w.Close() returns an error. I would vote against disallowing it:

err = fmt.Errorf(....)
guard w.Close()		// works
defer guard w.Close()	// compile error?

It would be a little weird if the last line would not compile, as it does not do something fundamentally different than the line before it.

I do agree with you that defer guard ... can hide (overwrite) errors that happened earlier. If that is something you do not want, you should just write a normal defer function. Just as with error handling: anything more than just returning or panicing an error requires explicit code.

Thinking about this however did make me realise that an argument can be made to have guard only overwrite an error value if it is not nil. This would take the side of prefering to report earlier errors over later errors:

func example() (err error)

// ...

defer guard w.Close()

// guard:
defer func() {
	err2 := w.Close()
	if err2 != nil && err != nil {	// prefer earlier errors
		err = err2
	}
}

[networkimprov]

If a deferred function fails, you almost certainly need local code to handle that failure, e.g. retry or log something and abort the current context. Unless the function can't return an error any other way, defer guard f() is probably a bug; I imagine it would be disallowed.

However, a Go 2 error handling scheme is likely to provide error handlers, so could potentially offer deferred handlers:

// assuming named handlers, # attaches a handler to a function call

defer handle h1 { ... }
defer skipOnError()
defer thisFails#h1()

defer runLast()
defer handle h2 { ... }
defer thisFails#h2()

[griesemer]

@markusheukelom Thanks for your answers.

I don't see convincing reasons for allowing something like defer guard f() (in contrast to say: defer (guard f)()). Note that defer requires a function invocation. So defer guard f() when interpreted as defer (guard f()) is not acting on a function invocation. To make this work somehow, changes to the defer semantics are needed. I think we should try to avoid that, especially because they are not needed. You can already write:

defer func(f *os.File) {
   must f.Close()
}(r)

so there's no need for defer must r.Close()).