Fix Go's error handling

knocte commented 8 years ago

(I'm really glad someone is trying to fix Go, even if it's from a transpiler perspective. E.g.: I really like the fact that you support Generics.)

Now, it would be great if you could fix error handling in Go. It has several disadvantages, such as the default not being safe (ignoring errors), and that it's easy to lose context (stacktrace) when propagating errors with new errors.

There's a good discussion about this here: https://news.ycombinator.com/item?id=12563887

This talk is also interesting because it highlights the common mistakes of Go devs when using error handling, and how difficult is to do it properly: https://www.youtube.com/watch?v=lsBF58Q-DnY (at the end of the talk, the speaker goes ahead and recommends the usage of an error-utility-library he implemented himself, maybe Have's best approach would just be code generation that makes use of this library? or maybe some kind of try-catch approach...).

Thanks

vrok commented 8 years ago

It's definitely on the roadmap, I'll soon write a longer blog post about future plans. Thanks!

christophmegusta commented 7 years ago

I also really like your efforts with Have. Can't await to see it raising 👍 ! The enforced verbosity of Go's natural error handling was also bugging me from my beginnings with this otherwise wonderful programming language. If try/catch is too much breaking the very reasons of the Go makers (which i understand), what about reducing the boilerplate at least?

What about something like:

_, err := ioutil.ReadAll(r)
if err != nil {
        return errors.Wrap(err, "read failed")
}

// getting transpiled from:
_ := ioutil.ReadAll(r) !"read failed"
// or
_ := ioutil.ReadAll(r) !ErrReadFailed

So we have either one keyword or a symbol to mark a method as error handler of any kind. In my example it is "!".

We can translate this to other use cases like:

// the fameous boilerplate 'return err block'
// i really can't befriend with that one! Go is hurting my arms by forcing me to type this 3 lines after almost every function :-/
_, err := ioutil.ReadAll(r)
if err != nil {
        return err
}

// getting transpiled from:
_ := ioutil.ReadAll(r)!
// so a single exclamation mark will just return the err if there is any

Or doing a recover by code block:

x, err := ioutil.ReadAll(r)
if err != nil {
        // x isnt required to be read, we have fallback content :)
        x = getFallbackContent()
}

// getting transpiled from:
x := ioutil.ReadAll(r)! {
        // recover from error
        x = getFallbackContent()
}

Just my brainstormings. Do whatever you like with it. Get inspiration, change it, burn it. Just wanted to take part in this discussion. :-)

Keep it up, cheers!

vrok commented 7 years ago

@christophmegusta thanks!

I've just uploaded https://github.com/vrok/have/wiki/Brain-dump-about-contributing, and there's another idea mentioned there.

robert-zaremba commented 7 years ago

@christophmegusta:

the recovery example doesn't cut the lines numbers and adds unicorns. I don't see it helpful. Also I don't see it will work win a general case.
for errors - what if somebody will like to use different function instead of errors.Wrap, or will don't want to return?

erizocosmico commented 7 years ago

what about keyword try?

x := try ioutil.ReadAll(r)

which would be transpiled to:

x, err := ioutil.ReadAll(r)
if err != nil {
    return err
}

Because we know the signature of the function we can return automatically a 0-value of every return value that is not the error itself

func foo() (int, *Foo, error) {
     n := try bar()
     f := try baz()
     return n, f, nil
}

which would be:

func foo() (int, *Foo, error) {
     n, err := bar()
     if err != nil {
         return 0, nil, err
     }

     f, err := baz()
     if err != nil {
         return 0, nil, err
     }

     return n, f, nil
}

The only problem I see with this is that the behaviour is kinda magic but all other error handling with multiple return values that I can think of has at least one corner case where it's flawed.

For example, the solution proposed https://github.com/vrok/have/wiki/Brain-dump-about-contributing has a very subtle flaw:

watch err {
   if err != nil {
      return err
   }
}

foo, err := foo()
funcThatMutatesErr(&err)

err would be mutated somewhere else and watch would not catch it

vrok commented 7 years ago

@mvader That's interesting, I like it.

As for the issue with watchers, I'll quote myself:

There is one important thing, however. It wouldn't be possible to let a variable with a watcher "escape", so it wouldn't be possible to get its address or use it in a closure (but it shouldn't be a problem for errors).

(Doing these things would result in a compile error.)

erizocosmico commented 7 years ago

Oh, then is completely fine. Didn't read that part. With that issue solved I think I actually like more the concept of watchers because there is no magic hidden behind it. On Fri, 30 Sep 2016 at 10:55, Marcin Wrochniak notifications@github.com wrote:

@mvader https://github.com/mvader That's interesting, I like it.

As for the issue with watchers, I'll quote myself:

There is one important thing, however. It wouldn't be possible to let a variable with a watcher "escape", so it wouldn't be possible to get its address or use it in a closure (but it shouldn't be a problem for errors).

(Doing these things would result in a compile error.)

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/vrok/have/issues/2#issuecomment-250694048, or mute the thread https://github.com/notifications/unsubscribe-auth/ABQFF6P9CfpofsRgHZ4h4lPoqveq8L4Oks5qvM5RgaJpZM4KFk5g .

vrok commented 7 years ago

BTW. We'll be probably writing the code that checks for variable escaping anyways, to prevent the loop variable scoping issue (the first one here: https://gist.github.com/lavalamp/4bd23295a9f32706a48f).

knocte commented 7 years ago

I like the try solution better, looks easier to wrap your head around to, as it's a keyword used in other languages.

erizocosmico commented 7 years ago

The problem I see with the try solution is, even though is easier, masks the error values and does a lot of magic behind the scenes that is not obvious for the developer. The try thing would world better with a Result type a la Rust but it's a bit harder to do with multiple return values On Fri, 30 Sep 2016 at 14:05, Andres G. Aragoneses notifications@github.com wrote:

I like the try solution better, looks easier to wrap your head around to, as it's a keyword used in other languages.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/vrok/have/issues/2#issuecomment-250727909, or mute the thread https://github.com/notifications/unsubscribe-auth/ABQFF0Vm65uNHqj9rmXcOv7oRj1Gx2Poks5qvPrPgaJpZM4KFk5g .

christophmegusta commented 7 years ago

the problem with "try" is it has in our context the wrong semantics. Usually we are used from other languages to ignore and/or catch a error from the "try" keyword. So its the exact opposite we want to achieve. Instead of ignoring a possible error case we want to explicitly leave the current function and return the error to the caller. Maybe something like "returnOnError" fits better here. But its a bit long and "panic" is already used in Go.

// maybe just "throw" instead of "try"
_, err = throw io.ReadFull(...)

// or do something weird with return... (or better not)
_, return(err) = io.ReadFull(...)

erizocosmico commented 7 years ago

@christophmegusta rust uses try with the same semantics and they do just fine

boazy commented 7 years ago

I think all proposed error handling mechanisms will be great improvement.

Watcher Proposal

The watch proposal is original. I don't think I've ever seen this type of error-handling, but it's very easy to understand and reason about. It's also probably the most flexible approach here, but it has three major drawbacks, in my opinion:

The exceptional behavior is not quite clear on the line where the error variable is assigned. In other words when you see:
```
result, err := DoSomething()
anotherResult, err := DoSomethingWithResult(a)
return err
```
It looks like err is just being ignored, until you go back to the beginning of the function and see the watch statement. This problem can be somehow mitigated by choosing a different variable name such as throwOnErr.
You're usually going to have 1 or 2 major error-handling strategies in your app/library: e.g. return err and return errors.Wrap(err, "Custom message defined at catch site"). Ideally you want to give users the freedom of defining their own error-handling strategies and also allow them to use more than one strategy in the same function. Watch gives you that, but in the price of some boilerplate attached to your function. A possible solution is to be able to predefine reusable watch blocks, but allowing them to cross packages seem to be hard. Another option is to let the user supply watch with a function with a signature func handleErr(originalErr error) error. Then the watcher will call the function with the error value and check the result. If the result is not nil, it will return the error and use zero values for all the rest. Then you can have implementations like:
```
func returnOnError(err) error {
return err
}
```

func returnAndWrapOnError(err) error { if err == nil { return nil } return errors.Wrap(err, "Wrapped error") }

func returnOnErrorExceptEOF(err) error { if err == io.EOF { return nil } return err }


The downside of my proposed solution is that it's only useful for error values. You could make the handler function generic, but even then it won't be able to do everything an inline watcher can do. On the other hand, it seems like watchers will be used for handling error values in more than 99% of the cases, so perhaps it's not an issue.

3. The third major drawback, is that you can't have parameters to the watcher defined at the catch-site, so you don't get the benefits of @christophmegusta's first solution (using the `!` bang operator with a parameter):
```go
 _ := ioutil.ReadAll(r) !"read failed"

I can't think of a good solution for that using watchers.

Bang Operator Proposal

@christophmegusta's proposal has some nice perks:

Ability to add parameters to the exception on the catch-site (DoSomething()! "Error message").
Custom error-handling on the catch-site
Less boilerplate for 2 very common error handling strategies (if err != nil { return err } and if err != nil { return errors.Wrap(err, "Custom message") }). You don't need to write anything special in the beginning of the function.

The drawback is that you can't customize it with other strategies and that's it's tied to a third-party library. Even though pkg/errors is written by an unofficial guru of Go error handling and widely accepted by the community, it still seems unacceptable.

There needs to be a way to customize error handling. It's probably feasible to modify this proposal to fix it.

Try Keyword Proposal

@erizocosmico suggested a try keyword, mimicing Rust's try! macro. The main difference is that in Rust try! is implemented entirely in terms of the macro system, but havelang would require a keyword for that.

If we're going for a language-level feature, I would actually prefer to have a dedicated operator which does the same thing, since error-handling is one of the most common idioms of the language and therefore should be as typist friendly as possible. ;)

In any case, the proposed try solution seems much less flexible than either watchers or the bang operators: it only supports returning on error and no other behavior. It does have one nice advantage that the other two proposals don't have: it can convert two-value returns into 1-value returns and therefore simplify expressions such as:

  price, err := dbRow.GetIntFieldValue("price")
  if err != nil {
    return err
  }
  amount, err := dbRow.GetIntFieldValue("amount")
  if err != nil {
    return err
  }
  err := cart.AddTotal(price * amount)
  if err != nil {
    return err
  }

to

  try cart.AddTotal(try dbRow.GetIntFieldValue("price") * try dbRow.GetIntFieldValue("amount"))

Or even clearer, following rust and using a dedicated operator:

  cart.AddTotal(dbRow.GetIntFieldValue("price")? * dbRow.GetIntFieldValue("amount")?)?

Rust-style Try-Catch Proposal

We can take the try proposal further by replacing taking the full try-catch proposal from rust RFC 243 which will allows us to customize error handling behavior with catch handlers:

    try {
      dbConn := db.Open("localhost", "3333")?
      dbRow := dbConn.GetRowById("OrderItems", orderItemId)
      cart.AddTotal(dbRow.GetIntFieldValue("price")? * dbRow.GetIntFieldValue("amount")?)?
    } catch(err) {
      // Ignore error and don't add items without price or amount to cart
      if err != db.FieldNotFoundError {
        return errors.Wrap(err, "DB error")
      }
    }

This gives you all the functionality of watchers and allows for more natural expressions (eliding error values). The downside is that we're still going to get some boilerplate, and it still doesn't have a way to customize error messages for errors.Wrap on the throw site.

It is possible to deal with the boilerplate by the same type of functions I proposed for watchers, allowing you to chain them for added effect:

  func ignoreError(sentinelErr error) func(error) error {
    return func(err error) error {
      if err == sentinelErr {
        return nil
      }
      return err
    }
  }

  func wrapAndReturnOnError(err error) error {
    if err != nil {
      return errors.Wrap(err)
    }
  }

  func addOrderItemToCart(orderItemId string, cart *Cart) {
    try {
      dbConn := db.Open("localhost", "3333")?
      dbRow := dbConn.GetRowById("OrderItems", orderItemId)
      cart.AddTotal(dbRow.GetIntFieldValue("price")? * dbRow.GetIntFieldValue("amount")?)?
    } catch ignoreError(db.FieldNotFound)
    catch wrapAndReturnOnError

vrok / have