Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Go's error handling #2

Open
knocte opened this issue Sep 24, 2016 · 13 comments
Open

Fix Go's error handling #2

knocte opened this issue Sep 24, 2016 · 13 comments

Comments

@knocte
Copy link

knocte commented Sep 24, 2016

(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 vrok changed the title Unfuck Go's error handling Fix Go's error handling Sep 24, 2016
@vrok
Copy link
Owner

vrok commented Sep 24, 2016

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

@christophmegusta
Copy link

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
Copy link
Owner

vrok commented Sep 29, 2016

@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
Copy link

@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
Copy link

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
Copy link
Owner

vrok commented Sep 30, 2016

@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
Copy link

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
#2 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABQFF6P9CfpofsRgHZ4h4lPoqveq8L4Oks5qvM5RgaJpZM4KFk5g
.

@vrok
Copy link
Owner

vrok commented Sep 30, 2016

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
Copy link
Author

knocte commented Sep 30, 2016

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

@erizocosmico
Copy link

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
#2 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABQFF0Vm65uNHqj9rmXcOv7oRj1Gx2Poks5qvPrPgaJpZM4KFk5g
.

@christophmegusta
Copy link

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
Copy link

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

@boazy
Copy link

boazy commented Jan 16, 2017

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:

  1. 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.

  1. 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.

  1. 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):
 _ := 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:

  1. Ability to add parameters to the exception on the catch-site (DoSomething()! "Error message").
  2. Custom error-handling on the catch-site
  3. 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants