Proposal: "collect": dealing the error handling boilerplate from another angle

ghost commented 5 years ago

Introduction

While there sure is boilerplate about current error handling in Go, there are many things that many people like about it - Go forces programmer to think about and handle errors manually; Go treat errors as value and there is no magic about them (so exit point and flow control are very visible). That makes me think that if err != nil is not boilerplate, instead, they are needed in a lot of cases. So it occurs to me that it is not if err != nil being too much becomes boilerplate, but rather, them being too frequent is the problem.

So, what if we handle error the same way before, but just less frequently? It sounds weird, but we already has that in some idioms. In the Official Go Blog post errors are values, Rob Pike promotes an idiom where errors are collected - but not handled until all other "steps" statements. The steps are simply skipped if there is an error collected before. It seems to me a good solution to many boilerplate caused by too frequently appearing if err != nil. However, it has limited uses since "steps" may not have the same signature and writing a helper wrapper type might as well be boilerplate.

But with help of built-in magic, I think the idea of the idiom can be applied and thus improving much more cases. And I propose so.

Proposal

I propose a new built-in function called collect. The collect built-in takes three arguments: a pointer to an error, a function call, and an error wrapper. If the error held by the pointer is nil, it returns what the function call returns except the last error, which is wrapped by error wrapper and then collected to the error pointer; otherwise, it returns zero values matches the signature of function call. collect cannot be nested - which means that collect appearing in another collect will trigger a compiler error. An error wrapper is a function with signature func(error) error - it takes in an error and returns the wrapped error. If the wrapper is nil, the error is collected without wrapping. If in rare case that the wrapper decides to return nil, nil is collected.

In psedu-code, collect can be written like:

func collect(err *error, f func(...) ..., w func(error) error) ... {
    if *err != nil {
        return _,... // zero values
    }

    v1, v2, ..., _err := f()
    if _err != nil {
        if w != nil {
            _err = w(_err)
        }
        *err = _err
    }
    return v1, v2, ...
}

To avoid bugs from recycling error pointers, consecutive uses of collect should be considered as declaration to the errors, so it can trigger declared and not used error from the compiler.

To reduce boilerplate of writing wrapper helper function, standard library (preferably errors package) should provide factories to wrappers, such as:

func Wrapperf(format string, args ...interface{}) func(error) error {
    return func(err error) error {
        a := append(args,err)
        return fmt.Errorf(format,a...)
    }
}

Example

The example from draft design overview:

func CopyFile(src, dst string) (err error) {
    wf := Wrapperf("copy %s %s: %v", src, dst)
    var err error

        r := collect(&err,os.Open(src),wf)
        defer r.Close()

        w := collect(&err,os.Create(dst),wf)
        if err == nil {
            defer os.Remove(dst)
        }

        collect(&err,io.Copy(w, r),wf)
        collect(&err,w.Close(),wf)
        return err
}

The example of printing sum:

func printSum(a, b string) error {
        var err error
        x := collect(&err,strconv.Atoi(a),Wrapper("a"))
        y := collect(&err,strconv.Atoi(b),Wrapper("b"))
        collect(&err, fmt.Println("result:", x + y), nil)
        return err
}

As for the example using nested try, collect shall not be nested.

The main function from rsc's unhex program can be simplified without splitting:

func main() {
    hex := collect(&err, ioutil.ReadAll(os.Stdin), nil)
    data := collect(&err, parseHexdump(string(hex)), nil)
    if err != nil {
        log.Fatal(err)
    }
    os.Stdout.Write(data)
}

Since collect does not handle errors, language tools to deal with errors can still work well:

n := collect(&err,src.Read(buf),nil)
if err == io.EOF {
        err = nil
        break
}
collect(&err,otherStuff(),nil)

Pros and Cons

Pros:
- collect does not interfere control flows (of the callers). Thus exit point remains very visible.
- collect does not handle errors. Code still needs to do that to avoid compile errors. So the language still encourages (or forces) people to think about errors and handle them gracefully.
- collect collects error so error handling may happen in a concentrated or unified way, which will make the happy path cleaner and more visible, and what happens to errors remain clear.
- collect does only minimal work, so errors remain values and language tools to program around values can still work well.
Cons:
- It does not help at all if there is only one if err != nil
- It might hinder performance since there might be another function call. I am not sure whether it can be optimized by the compiler or not.

Discussion

name Naming things is hard and I don't seem to be good at it. While collect reflects the fact that it collects error, it does not reflect the more important part: skipping function calls when error has already occured. However, how to summarize that and form a better name is beyond me.
defer and go It is quite obvious that defer collect(...) makes no senses at all. Therefore, I think it is best to ban this kind of usage. On the other hand, go collect(...) might be valid, but it is prone to (if not always leads to) data races (on &err). Hence, I would suggest to ban this kind of use too.
One interesting case to note is needs for cases like collect(&err, defer f(), w). While it is invalid go syntax, I can think of many cases that would need it. I haven't decided whether or not to allow such uses. If allowed, the previous example can be written as: func CopyFile(src, dst string) (err error) { wf := Wrapperf("copy %s %s: %v", src, dst) var err error
```
    r := collect(&err,os.Open(src),wf)
    defer r.Close()

    w := collect(&err,os.Create(dst),wf)
    collect(&err,defer os.Remove(dst),wf)

    collect(&err,io.Copy(w, r),wf)
    collect(&err,w.Close(),wf)
    return err
```
}
for While it is easy to use collect in a loop, there is two problems. The first problem is that collect can only help to skip a loop if you wrap the loop into a function literal. It sure looks ugly, but maybe it is better to check errors manually before entering a loop. The second problem is that careless use of collect might create a busy loop. Vet should handle this case.
chaining / nesting There are two problems about chaining / nesting. First problem is that since collect enables chaining in places like os.Open(file), it is prone to less visible need to write defer statement. But unlike try, there is at least a trace that an error is collected, which might remind the programmer to deal with it more carefully. The second problems is that nesting/chaining obscures control flow in a hidden way - the reader (or even writer) of the code has to decide which error gets collected first since order of collecting can change behavior. Therefore, to avoid confusing behavior, nested collect should be disabled. However, chaining, on the other hand, might be ok.
performance It is unsure to me whether using collect would hinder performance of function calls or the compiler can optimize it.

networkimprov commented 5 years ago

See also #25626

ghost commented 5 years ago

Though sharing name with #25626, I would argue they are very different. This proposal does not include anything like goto, and does not require collected errors being in the same block.

beoran commented 5 years ago

This is a good use case for hygienic macros: #32620

ianlancetaylor commented 5 years ago

This is an interesting proposal, but I note that it doesn't support what is by far the most common case when an error occurs: returning from the function. It's true that if your function is all top-level calls to collect then this doesn't matter, since collect will skip execution of everything after an error occurs. But most functions have conditionals and loops, and using those with collect will require lots of function literals, which is simply too awkward and is in fact almost like a continuation passing language. To me those seem like fatal problems with this idea.

ghost commented 5 years ago

@ianlancetaylor Yes, it does not support returing from the function, and I deliberately propose so, since the biggest issue all error handling proposal I've seen so far has a common problem of obscuring control flow and exit points.

I am aware of collect, being a builtin and a non-returner, would not cope well with things other than basic expressions; it is not designed to do so. I justify this by saying: collect is not meant to handle error, rather, it only collects them. If there is a loop or an conditions, it would be a case that the code requires better and more dedicated error handling - by that, if err != nil and other language stuff comes to play. I would say this is a good side effects - it forces programmer to think about errors when entering more complex code blocks. And on the other hand, I would argue that error handling before loops or conditions are unlikely boilerplate code to begin with.

However, like opionions I presented on the proposal, all these are based on experience of my own and those who are close to me, which by no means can match up the Go society; but I do not have ability to get more datas and opionions on them, so I am unclear about whether they are widely accepted or not.

mvndaai commented 5 years ago

Since collect returns the zeros of the function calls but does return from the function wouldn't this mean that if there is an error the flow would continue but with zeros?

Meaning if os.Open returned an error, then r would be a zero. Meaning the defer r.Close() would attempt to close a zero, maybe nil. Then io.Copy would try to copy from a zero?

r := collect(&err,os.Open(src),wf)
defer r.Close()
...
collect(&err,io.Copy(w, r),wf)

ghost commented 5 years ago

Since collect returns the zeros of the function calls but does return from the function wouldn't this mean that if there is an error the flow would continue but with zeros?

No, it skips if err is not nil. But if you stop using collect, but with the error-prone valuable, yes, it continue with zero value.

Meaning if os.Open returned an error, then r would be a zero. Meaning the defer r.Close() would attempt to close a zero, maybe nil. Then io.Copy would try to copy from a zero?
r := collect(&err,os.Open(src),wf)
defer r.Close()
...
collect(&err,io.Copy(w, r),wf)

Yes, in current form, defer r.Close() run as defer nil.Close() while the nil is a typed nil. While this is ok in os.File case, it is not very desirable. So I write in the proposal to discuss maybe it is better to allow collect(&err, defer r.Close(), wf). And No, if r is nil, it means there is an error collected, so err is not nil - thus, there will be NO attempt to run io.Copy(w, r), so io.Copy will NOT try to copy from zero.

mvndaai commented 5 years ago

@leafbebop thanks for that clarification. What worries me is that after the first collect all other functions would also need a collect because if the if err != nil { skip }. It would even be necessary on functions that cannot return an error because of the zero values.

ghost commented 5 years ago

@mvndaai I understand that. But as I say in the proposal, collect only collects error. If the logic becomes too complex to be written in collect, I think it is time to actually handle the error.

Unlike other proposals (try or others), collect does not aim to replace if err != nil, rather, it aims to accompany it.

ianlancetaylor commented 5 years ago

In order to use this, essentially everything after the first call to collect has to be a call to collect. Or has to check err != nil in which case collect didn't help us at all.

Although collect does not itself return or cause any control flow, in effect it completely obscures the control flow, because when err becomes non-nil, all future function calls do not occur, even though it seems that they do. This will also likely cause execution to be somewhat slower.

Even in the base case, in practice every line will start will collect, making it harder to see what the function does.

If you accidentally forget a second or subsequent collect, and just call a function directly, any number of errors could occur.

-- for @golang/proposal-review

golang / go