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()).

[earthboundkid]

Here is an idea about guard that is slightly more general and also solves the ternary initialization issue.

The basic syntax is similar to if with a minor differences (no optional else block). The semantics are what really separate guard from if. (Most obviously, guard acts as "unless", not "if".)

1. If you introduce a variable in an if, it is only available in the conditional block, but for guard, it would be available for the rest of the function:

guard val, ok := myMap[key]; ok {
   // handle !ok here
}
// val is in scope here

As a ternary initializer:

guard name := obj.Name(); name != "" {
   name = "Unknown"
}
// name is in scope here

2. For expressions returning a final error value guard defaults to ; err == nil { return err } and the final error value can be omitted.

guard f := os.Open(file)
defer f.Close()
// etc.

[griesemer]

@carlmjohnson It seems to me that your proposed guard is a complicated if combined with try. Considering your examples, if I understood correctly:

guard val, ok := myMap[key]; ok {
   // handle !ok here
}
// val is in scope here

can be written today as

val, ok := myMap[key]
if !ok {
   // handle !ok here
}
// val is in scope here

And if you don't want the ok in scope at the end, that can be finessed, too.

Your ternary initializer

guard name := obj.Name(); name != "" {
   name = "Unknown"
}
// name is in scope here

becomes

name := obj.Name()
if name == "" {
   name = "Unknown"
}
// name is in scope here

And finally

guard f := os.Open(file)
defer f.Close()

becomes

f := try(os.Open(file))
defer f.Close()

It is hard to see why this should be any better than writing an if statement. Using an if seems clearer and - if you just count characters incl. newlines - shorter, too (because if is shorter than guard). It's clearer because the condition doesn't need to be inverted when looking at the block. It's also simpler because there's no "combination" of condition or err != nil check to keep in mind. Better to make the latter a separate operation using try. The last example reads just as well if not better with try than with guard and is the same size.

In short, I see no benefit in making guard a new statement. This original proposal suggests guard as a unary operator, which I think is a much cleaner, simpler, and thus better idea. Its semantics are basically the same as the proposed try built-in.

[earthboundkid]

That makes sense. I think an unless-like guard makes more sense in Swift, where the problem is that you need to convert Maybe-types to value types. In Go, there are no Maybe-types, so there is less of a problem for it to solve.

In general, all of the handler block proposals seem to suffer from not being better than an if statement.

[griesemer]

In general, all of the handler block proposals seem to suffer from not being better than an if statement.

That is exactly one of our observations as well. Only if the error handling is the same in many places (say where the error is decorated the same way), does it make sense to share and factor out that code, in some sort of handler. It appears that those cases are not the common.

[networkimprov]

The Error Values feature enabling returned errors to be wrapped should make those cases a lot more common. However it may take a couple years after the arrival of 1.13 to see that impact.

And there is one proposal which is better than a vanilla if for short handlers, #32611 :-)

[mvndaai]

@markusheukelom thank you for realizing that errors in defer functions need to be addressed. My concern is must having to panic. I have a proposal #33162 to add a Handle(err) function to errors that could be used instead of panicking.

[sina-devel]

error handling could be done without proposing new keywords, specially keywords like guard which are popular words for naming variables. for example, using a question mark could be useful:

func f() (int, error) {
    i := getIntOrError()? // returns (0, err) on error
    return i, nil
}

This is good, but it's not interesting if we want to wrap the error

[switchupcb]

The must keyword can only be used on function calls that return an error as last return argument.
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.

guard functions in a similar manner to #37141

2 new keywords are introduced, with "complex" descriptions. However, it can be argued that these improve readability and the verbosity of error handling, as per the original error-handling draft design.

Each keywords treats errors as special values.

It doesn't allow wrapping.

Based on historical decisions with regards to error-handling, this proposal is bound to fail. However, it's backwards compatible.

[NoobforAl]

Hi,
Excuses me i have idea.
Why we not make one function like init for error handling.

• errorInit run for each exceptions.
• this function have all exceptions in program/module.
• if we use keyword guard this function run.
• this function get two arg ( d for data in program, error one error happened in program )
• first arg can remove and only get error

for example ( in file exception.go ):

package main

// sample error
type sizeError struct {
	Err  error
}

func errorInit[T any](d T, err error) error {
    switch v := err.(type) {

    // for close some file with error
    case *fs.PathError:
		if f, ok := any(d).(*os.File); ok {
			f.Close()
		}
		panic(v)

    // sometime one error not necessary
    case sizeError:
        // do something
        return nil

    // for sometimes we get one error but needed send custom error
    case Error404:
        // do something
        return d.JSON(404, gin.H{
			"message": "not found!",
		})


    // for sometime we just now no have exception
    default:
		panic(v)
	}
}


// if we use errorInit not needed f.Close()
func Sample(name string) error{

    // send tow data on os.File and error
    // if program get error 
    // guard get error from errorInit
    // and return this func error
    // else f value get os.File
    f := guard os.Open(name)
    guard f.Write([]byte("FOO"))

    //throw sizeError 
    s := guard getSize(name)
    fmt.Println(s)
    return nil
}

If not found in errorInit program run default (guard) and return only error without custom proses.

[chad-bekmezian-snap]

What's the status of this proposal? Seems like there hasn't been much activity for a while. I personally like it and am interested in where it's at!

[griesemer]

At the moment we are not focussing on error handling through language changes; no progress has been made here at this point.