golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
120.06k stars 17.23k forks source link

Proposal: A built-in Go error check function, "try" #32437

Closed griesemer closed 4 years ago

griesemer commented 4 years ago

Proposal: A built-in Go error check function, try

This proposal has been closed. Thanks, everybody, for your input.

Before commenting, please read the detailed design doc and see the discussion summary as of June 6, the summary as of June 10, and most importantly the advice on staying focussed. Your question or suggestion may have already been answered or made. Thanks.

We propose a new built-in function called try, designed specifically to eliminate the boilerplate if statements typically associated with error handling in Go. No other language changes are suggested. We advocate using the existing defer statement and standard library functions to help with augmenting or wrapping of errors. This minimal approach addresses most common scenarios while adding very little complexity to the language. The try built-in is easy to explain, straightforward to implement, orthogonal to other language constructs, and fully backward-compatible. It also leaves open a path to extending the mechanism, should we wish to do so in the future.

[The text below has been edited to reflect the design doc more accurately.]

The try built-in function takes a single expression as argument. The expression must evaluate to n+1 values (where n may be zero) where the last value must be of type error. It returns the first n values (if any) if the (final) error argument is nil, otherwise it returns from the enclosing function with that error. For instance, code such as

f, err := os.Open(filename)
if err != nil {
    return …, err  // zero values for other results, if any
}

can be simplified to

f := try(os.Open(filename))

try can only be used in a function which itself returns an error result, and that result must be the last result parameter of the enclosing function.

This proposal reduces the original draft design presented at last year's GopherCon to its essence. If error augmentation or wrapping is desired there are two approaches: Stick with the tried-and-true if statement, or, alternatively, “declare” an error handler with a defer statement:

defer func() {
    if err != nil { // no error may have occurred - check for it
        err = … // wrap/augment error
    }
}()

Here, err is the name of the error result of the enclosing function. In practice, suitable helper functions will reduce the declaration of an error handler to a one-liner. For instance

defer fmt.HandleErrorf(&err, "copy %s %s", src, dst)

(where fmt.HandleErrorf decorates *err) reads well and can be implemented without the need for new language features.

The main drawback of this approach is that the error result parameter needs to be named, possibly leading to less pretty APIs. Ultimately this is a matter of style, and we believe we will adapt to expecting the new style, much as we adapted to not having semicolons.

In summary, try may seem unusual at first, but it is simply syntactic sugar tailor-made for one specific task, error handling with less boilerplate, and to handle that task well enough. As such it fits nicely into the philosophy of Go. try is not designed to address all error handling situations; it is designed to handle the most common case well, to keep the design simple and clear.

Credits

This proposal is strongly influenced by the feedback we have received so far. Specifically, it borrows ideas from:

Detailed design doc

https://github.com/golang/proposal/blob/master/design/32437-try-builtin.md

tryhard tool for exploring impact of try

https://github.com/griesemer/tryhard

rasky commented 4 years ago

I agree this is the best way forward: fixing the most common issue with a simple design.

I don't want to bikeshed (feel free to postpone this conversation), but Rust went there and eventually settled with the ? postfix operator rather than a builtin function, for increased readability.

The gophercon proposal cites ? in the considered ideas and gives three reason why it was discarded: the first ("control flow transfers are as a general rule accompanied by keywords") and the third ("handlers are more naturally defined with a keyword, so checks should too") do not apply anymore. The second is stylistic: it says that, even if the postfix operator works better for chaining, it can still read worse in some cases like:

check io.Copy(w, check newReader(foo))

rather than:

io.Copy(w, newReader(foo)?)?

but now we would have:

try(io.Copy(w, try(newReader(foo))))

which I think it's clearly the worse of the three, as it's not even obvious anymore which is the main function being called.

So the gist of my comment is that all three reasons cited in the gophercon proposal for not using ? do not apply to this try proposal; ? is concise, very readable, it does not obscure the statement structure (with its internal function call hierarchy), and it is chainable. It removes even more clutter from the view, while not obscuring the control flow more than the proposed try() already does.

jimmyfrasche commented 4 years ago

To clarify:

Does

func f() (n int, err error) {
  n = 7
  try(errors.New("x"))
  // ...
}

return (0, "x") or (7, "x")? I'd assume the latter.

Does the error return have to be named in the case where there's no decoration or handling (like in an internal helper function)? I'd assume not.

ianlancetaylor commented 4 years ago

Your example returns 7, errors.New("x"). This should be clear in the full doc that will soon be submitted (https://golang.org/cl/180557).

The error result parameter does not need to be named in order to use try. It only needs to be named if the function needs to refer to it in a deferred function or elsewhere.

dominikh commented 4 years ago

I am really unhappy with a built-in function affecting control flow of the caller. This is very unintuitive and a first for Go. I appreciate the impossibility of adding new keywords in Go 1, but working around that issue with magic built-in functions just seems wrong to me. It's worsened by the fact that built-ins can be shadowed, which drastically changes the way try(foo) behaves. Shadowing of other built-ins doesn't have results as unpredictable as control flow changing. It makes reading snippets of code without all of the context much harder.

I don't like the way postfix ? looks, but I think it still beats try(). As such, I agree with @rasky .

Edit: Well, I managed to completely forget that panic exists and isn't a keyword.

griesemer commented 4 years ago

The detailed proposal is now here (pending formatting improvements, to come shortly) and will hopefully answer a lot of questions.

griesemer commented 4 years ago

@dominikh The detailed proposal discusses this at length, but please note that panic and recover are two built-ins that affect control flow as well.

nictuku commented 4 years ago

One clarification / suggestion for improvement:

if the last argument supplied to try, of type error, is not nil, the enclosing function’s error result variable (...) is set to that non-nil error value before the enclosing function returns

Could this instead say is set to that non-nil error value and the enclosing function returns? (s/before/and)

On first reading, before the enclosing function returns seemed like it would eventually set the error value at some point in the future right before the function returned - possibly in a later line. The correct interpretation is that try may cause the current function to return. That's a surprising behavior for the current language, so a clearer text would be welcomed.

purpleidea commented 4 years ago

I think this is just sugar, and a small number of vocal opponents teased golang about the repeated use of typing if err != nil ... and someone took it seriously. I don't think it's a problem. The only missing things are these two built-ins:

https://github.com/purpleidea/mgmt/blob/a235b760dc3047a0d66bb0b9d63c25bc746ed274/util/errwrap/errwrap.go#L26

webermaster commented 4 years ago

Not sure why anyone ever would write a function like this but what would be the envisioned output for

try(foobar())

If foobar returned (error, error)

dominikh commented 4 years ago

I retract my previous concerns about control flow and I no longer suggest using ?. I apologize for the knee-jerk response (though I'd like to point out this wouldn't have happened had the issue been filed after the full proposal was available).

I disagree with the necessity for simplified error handling, but I'm sure that is a losing battle. try as laid out in the proposal seems to be the least bad way of doing it.

ianlancetaylor commented 4 years ago

@webermaster Only the last error result is special for the expression passed to try, as described in the proposal doc.

akyoto commented 4 years ago

Like @dominikh, I also disagree with the necessity of simplified error handling.

It moves vertical complexity into horizontal complexity which is rarely a good idea.

If I absolutely had to choose between simplifying error handling proposals, though, this would be my preferred proposal.

cespare commented 4 years ago

It would be helpful if this could be accompanied (at some stage of accepted-ness) by a tool to transform Go code to use try in some subset of error-returning functions where such a transformation can be easily performed without changing semantics. Three benefits occur to me:

lestrrat commented 4 years ago

I just would like to express that I think a bare try(foo()) actually bailing out of the calling function takes away from us the visual cue that function flow may change depending on the result.

I feel I can work with try given enough getting used, but I also do feel we will need extra IDE support (or some such) to highlight try to efficiently recognize the implicit flow in code reviews/debugging sessions

Goodwine commented 4 years ago

The thing I'm most concerned about is the need to have named return values just so that the defer statement is happy.

I think the overall error handling issue that the community complains about is a combination of the boilerplate of if err != nil AND adding context to errors. The FAQ clearly states that the latter is left out intentionally as a separate problem, but I feel like then this becomes an incomplete solution, but I'll be willing to give it a chance after thinking on these 2 things:

  1. Declare err at the beginning of the function. Does this work? I recall issues with defer & unnamed results. If it doesn't the proposal needs to consider this.
    func sample() (string, error) {
    var err error
    defer fmt.HandleErrorf(&err, "whatever")
    s := try(f())
    return s, nil
    }
  2. Assign values like we did in the past, but use a helper wrapf function that has the if err != nil boilerplate.
    func sample() (string, error) {
    s, err := f()
    try(wrapf(err, "whatever"))
    return s, nil
    }
    func wrapf(err error, format string, ...v interface{}) error {
    if err != nil {
    // err = wrapped error
    }
    return err
    }

If either work, I can deal with it.

randall77 commented 4 years ago
func sample() (string, error) {
  var err error
  defer fmt.HandleErrorf(&err, "whatever")
  s := try(f())
  return s, nil
}

This will not work. The defer will update the local err variable, which is unrelated to the return value.

func sample() (string, error) {
  s, err := f()
  try(wrapf(err, "whatever"))
  return s, nil
}
func wrapf(err error, format string, ...v interface{}) error {
  if err != nil {
    // err = wrapped error
  }
  return err
}

That should work. It will call wrapf even on a nil error, though. This will also (continue to) work, and is IMO a lot clearer:

func sample() (string, error) {
  s, err := f()
  if err != nil {
      return "", wrap(err)
  }
  return s, nil
}

No one is going to make you use try.

singhpradeep commented 4 years ago

Not sure why anyone ever would write a function like this but what would be the envisioned output for

try(foobar())

If foobar returned (error, error)

Why would you return more than one error from a function? If you are returning more than one error from function, perhaps function should be split into two separate ones in the first place, each returning just one error.

Could you elaborate with an example?

griesemer commented 4 years ago

@cespare: It should be possible for somebody to write a go fix that rewrites existing code suitable for try such that it uses try. It may be useful to get a feel for how existing code could be simplified. We don't expect any significant changes in code size or performance, since try is just syntactic sugar, replacing a common pattern by a shorter piece of source code that produces essentially the same output code. Note also that code that uses try will be bound to use a Go version that's at least the version at which try was introduced.

@lestrrat: Agreed that one will have to learn that try can change control flow. We suspect that IDE's could highlight that easily enough.

@Goodwine: As @randall77 already pointed out, your first suggestion won't work. One option we have thought about (but not discussed in the doc) is the possibility of having some predeclared variable that denotes the error result (if one is present in the first place). That would eliminate the need for naming that result just so it can be used in a defer. But that would be even more magic; it doesn't seem justified. The problem with naming the return result is essentially cosmetic, and where that matters most is in the auto-generated APIs served by go doc and friends. It would be easy to address this in those tools (see also the detailed design doc's FAQ on this subject).

griesemer commented 4 years ago

@nictuku: Regarding your suggestion for clarification (s/before/and/): I think the code immediately before the paragraph you're referring to makes it clear what happens exactly, but I see your point, s/before/and/ may make the prose clearer. I'll make the change.

See CL 180637.

deanveloper commented 4 years ago

I actually really like this proposal. However, I do have one criticism. The exit point of functions in Go have always been marked by a return. Panics are also exit points, however those are catastrophic errors that are typically not meant to ever be encountered.

Making an exit point of a function that isn't a return, and is meant to be commonplace, may lead to much less readable code. I had heard about this in a talk and it is hard to unsee the beauty of how this code is structured:

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

    w, err := os.Create(dst)
    if err != nil {
        return fmt.Errorf("copy %s %s: %v", src, dst, err)
    }

    if _, err := io.Copy(w, r); err != nil {
        w.Close()
        os.Remove(dst)
        return fmt.Errorf("copy %s %s: %v", src, dst, err)
    }

    if err := w.Close(); err != nil {
        os.Remove(dst)
        return fmt.Errorf("copy %s %s: %v", src, dst, err)
    }
}

This code may look like a big mess, and was meant to by the error handling draft, but let's compare it to the same thing with try.

func CopyFile(src, dst string) error {
    defer func() {
        err = fmt.Errorf("copy %s %s: %v", src, dst, err)
    }()
    r, err := try(os.Open(src))
    defer r.Close()

    w, err := try(os.Create(dst))

    defer w.Close()
    defer os.Remove(dst)
    try(io.Copy(w, r))
    try(w.Close())

    return nil
}

You may look at this at first glance and think it looks better, because there is a lot less repeated code. However, it was very easy to spot all of the spots that the function returned in the first example. They were all indented and started with return, followed by a space. This is because of the fact that all conditional returns must be inside of conditional blocks, thereby being indented by gofmt standards. return is also, as previously stated, the only way to leave a function without saying that a catastrophic error occurred. In the second example, there is only a single return, so it looks like the only thing that the function ever should return is nil. The last two try calls are easy to see, but the first two are a bit harder, and would be even harder if they were nested somewhere, ie something like proc := try(os.FindProcess(try(strconv.Atoi(os.Args[1])))).

Returning from a function has seemed to have been a "sacred" thing to do, which is why I personally think that all exit points of a function should be marked by return.

s4n-gt commented 4 years ago

Someone has already implemented this 5 years ago. If you are interested, you can try this feature

https://news.ycombinator.com/item?id=20101417

I implemented try() in Go five years ago with an AST preprocessor and used it in real projects, it was pretty nice: https://github.com/lunixbochs/og

Here are some examples of me using it in error-check-heavy functions: https://github.com/lunixbochs/poxd/blob/master/tls.go#L13

jasonmoo commented 4 years ago

I appreciate the effort that went into this. I think it's the most go-ey solution I've seen so far. But I think it introduces a bunch of work when debugging. Unwrapping try and adding an if block every time I debug and rewrapping it when I'm done is tedious. And I also have some cringe about the magical err variable that I need to consider. I've never been bothered by the explicit error checking so perhaps I'm the wrong person to ask. It always struck me as "ready to debug".

Goodwine commented 4 years ago

@griesemer My problem with your proposed use of defer as a way to handle the error wrapping is that the behavior from the snippet I showed (repeated below) is not very common AFAICT, and because it's very rare then I can imagine people writing this thinking it works when it doesn't.

Like.. a beginner wouldn't know this, if they have a bug because of this they won't go "of course, I need a named return", they would get stressed out because it should work and it doesn't.

var err error
defer fmt.HandleErrorf(err);

try is already too magic so you may as well go all the way and add that implicit error value. Think on the beginners, not on those who know all the nuances of Go. If it's not clear enough, I don't think it's the right solution.

Or... Don't suggest using defer like this, try another way that's safer but still readable.

griesemer commented 4 years ago

@deanveloper It is true that this proposal (and for that matter, any proposal trying to attempt the same thing) will remove explicitly visible return statements from the source code - that is the whole point of the proposal after all, isn't it? To remove the boilerplate of if statements and returns that are all the same. If you want to keep the return's, don't use try.

We are used to immediately recognize return statements (and panic's) because that's how this kind of control flow is expressed in Go (and many other languages). It seems not far fetched that we will also recognize try as changing control flow after some getting used to it, just like we do for return. I have no doubt that good IDE support will help with this as well.

buchanae commented 4 years ago

I have two concerns:

In my experience, adding context to errors immediately after each call site is critical to having code that can be easily debugged. And named returns have caused confusion for nearly every Go developer I know at some point.

A more minor, stylistic concern is that it's unfortunate how many lines of code will now be wrapped in try(actualThing()). I can imagine seeing most lines in a codebase wrapped in try(). That feels unfortunate.

I think these concerns would be addressed with a tweak:

a, b, err := myFunc()
check(err, "calling myFunc on %v and %v", a, b)

check() would behave much like try(), but would drop the behavior of passing through function return values generically, and instead would provide the ability to add context. It would still trigger a return.

This would retain many of the advantages of try():

griesemer commented 4 years ago

@s4n-gt Thanks for this link. I was not aware of it.

griesemer commented 4 years ago

@Goodwine Point taken. The reason for not providing more direct error handling support is discussed in the design doc in detail. It is also a fact that over the course of a year or so (since the draft designs published at last year's Gophercon) no satisfying solution for explicit error handling has come up. Which is why this proposal leaves this out on purpose (and instead suggests to use a defer). This proposal still leaves the door open for future improvements in that regard.

josharian commented 4 years ago

The proposal mentions changing package testing to allow tests and benchmarks to return an error. Though it wouldn’t be “a modest library change”, we could consider accepting func main() error as well. It’d make writing little scripts much nicer. The semantics would be equivalent to:

func main() {
  if err := newmain(); err != nil {
    println(err.Error())
    os.Exit(1)
  }
}
deanveloper commented 4 years ago

One last criticism. Not really a criticism to the proposal itself, but instead a criticism to a common response to the "function controlling flow" counterargument.

The response to "I don't like that a function is controlling flow" is that "panic also controls the flow of the program!". However, there are a few reasons that it's more okay for panic to do this that don't apply to try.

  1. panic is friendly to beginner programmers because what it does is intuitive, it continues unwrapping the stack. One shouldn't even have to look up how panic works in order to understand what it does. Beginner programmers don't even need to worry about recover, since beginners aren't typically building panic recovery mechanisms, especially since they are nearly always less favorable than simply avoiding the panic in the first place.

  2. panic is a name that is easy to see. It brings worry, and it needs to. If one sees panic in a codebase, they should be immediately thinking of how to avoid the panic, even if it's trivial.

  3. Piggybacking off of the last point, panic cannot be nested in a call, making it even easier to see.

It is okay for panic to control the flow of the program because it is extremely easy to spot, and it is intuitive as to what it does.

The try function satisfies none of these points.

  1. One cannot guess what try does without looking up the documentation for it. Many languages use the keyword in different ways, making it hard to understand what it would mean in Go.

  2. try does not catch my eye, especially when it is a function. Especially when syntax highlighting will highlight it as a function. ESPECIALLY after developing in a language like Java, where try is seen as unnecessary boilerplate (because of checked exceptions).

  3. try can be used in an argument to a function call, as per my example in my previous comment proc := try(os.FindProcess(try(strconv.Atoi(os.Args[1])))). This makes it even harder to spot.

My eyes ignore the try functions, even when I am specifically looking for them. My eyes will see them, but immediately skip to the os.FindProcess or strconv.Atoi calls. try is a conditional return. Control flow AND returns are both held up on pedestals in Go. All control flow within a function is indented, and all returns begin with return. Mixing both of these concepts together into an easy-to-miss function call just feels a bit off.


This comment and my last are my only real criticisms to the idea though. I think I may be coming off as not liking this proposal, but I still think that it is an overall win for Go. This solution still feels more Go-like than the other solutions. If this were added I would be happy, however I think that it can still be improved, I'm just not sure how.

josharian commented 4 years ago

@buchanae interesting. As written, though, it moves fmt-style formatting from a package into the language itself, which opens up a can of worms.

buchanae commented 4 years ago

As written, though, it moves fmt-style formatting from a package into the language itself, which opens up a can of worms.

Good point. A simpler example:

a, b, err := myFunc()
check(err, "calling myFunc")
griesemer commented 4 years ago

@buchanae We have considered making explicit error handling more directly connected with try - please see the detailed design doc, specifically the section on Design iterations. Your specific suggestion of check would only allow to augment errors through something like a fmt.Errorf like API (as part of the check), if I understand correctly. In general, people may want to do all kinds of things with errors, not just create a new one that refers to the original one via its error string.

Again, this proposal does not attempt to solve all error handling situations. I suspect in most cases try makes sense for code that now looks basically like this:

a, b, c, ... err := try(someFunctionCall())
if err != nil {
   return ..., err
}

There is an awful lot of code that looks like this. And not every piece of code looking like this needs more error handling. And where defer is not right, one can still use an if statement.

josharian commented 4 years ago

I don’t follow this line:

defer fmt.HandleErrorf(&err, “foobar”)

It drops the inbound error on the floor, which is unusual. Is it meant to be used something more like this?

defer fmt.HandleErrorf(&err, “foobar: %v”, err)

The duplication of err is a bit stutter-y. This is not really directly apropos to the proposal, just a side comment about the doc.

adg commented 4 years ago

I share the two concerns raised by @buchanae, re: named returns and contextual errors.

I find named returns a bit troublesome as it is; I think they are only really beneficial as documentation. Leaning on them more heavily is a worry. Sorry to be so vague, though. I'll think about this more and provide some more concrete thoughts.

I do think there is a real concern that people will strive to structure their code so that try can be used, and therefore avoid adding context to errors. This is a particularly weird time to introduce this, given we're just now providing better ways to add context to errors through official error wrapping features.

I do think that try as-proposed makes some code significantly nicer. Here's a function I chose more or less at random from my current project's code base, with some of the names changed. I am particularly impressed by how try works when assigning to struct fields. (That is assuming my reading of the proposal is correct, and that this works?)

The existing code:

func NewThing(thingy *foo.Thingy, db *sql.DB, client pb.Client) (*Thing, error) {
        err := dbfile.RunMigrations(db, dbMigrations)
        if err != nil {
                return nil, err
        }
        t := &Thing{
                thingy: thingy,
        }
        t.scanner, err = newScanner(thingy, db, client)
        if err != nil {
                return nil, err
        }
        t.initOtherThing()
        return t, nil
}

With try:

func NewThing(thingy *foo.Thingy, db *sql.DB, client pb.Client) (*Thing, error) {
        try(dbfile.RunMigrations(db, dbMigrations))
        t := &Thing{
                thingy:  thingy,
                scanner: try(newScanner(thingy, db, client)),
        }
        t.initOtherThing()
        return t, nil
}

No loss of readability, except perhaps that it's less obvious that newScanner might fail. But then in a world with try Go programmers would be more sensitive to its presence.

griesemer commented 4 years ago

@josharian Regarding main returning an error: It seems to me that your little helper function is all that's needed to get the same effect. I'm not sure changing the signature of main is justified.

Regarding the "foobar" example: It's just a bad example. I should probably change it. Thanks for bringing it up.

josharian commented 4 years ago

defer fmt.HandleErrorf(&err, “foobar: %v”, err)

Actually, that can’t be right, because err will be evaluated too early. There are a couple of ways around this, but none of them as clean as the original (I think flawed) HandleErrorf. I think it’d be good to have a more realistic worked example or two of a helper function.

EDIT: this early evaluation bug is present in an example near the end of the doc:

defer fmt.HandleErrorf(&err, "copy %s %s: %v", src, dst, err)

griesemer commented 4 years ago

@adg Yes, try can be used as you're using it in your example. I let your comments re: named returns stand as is.

buchanae commented 4 years ago

people may want to do all kinds of things with errors, not just create a new one that refers to the original one via its error string.

try doesn't attempt to handle all the kinds of things people want to do with errors, only the ones that we can find a practical way to make significantly simpler. I believe my check example walks the same line.

In my experience, the most common form of error handling code is code that essentially adds a stack trace, sometimes with added context. I've found that stack trace to be very important for debugging, where I follow an error message through the code.

But, maybe other proposals will add stack traces to all errors? I've lost track.

In the example @adg gave, there are two potential failures but no context. If newScanner and RunMigrations don't themselves provide messages that clue you into which one went wrong, then you're left guessing.

adg commented 4 years ago

In the example @adg gave, there are two potential failures but no context. If newScanner and RunMigrations don't themselves provide messages that clue you into which one went wrong, then you're left guessing.

That's right, and that's the design choice we made in this particular piece of code. We do wrap errors a lot in other parts of the code.

typeless commented 4 years ago

I share the concern as @deanveloper and others that it might make debugging tougher. It's true that we can choose not to use it, but the styles of third-party dependencies are not under our control. If less repetitive if err := ... { return err } is the primary point, I wonder if a "conditional return" would suffice, like https://github.com/golang/go/issues/27794 proposed.

        return nil, err if f, err := os.Open(...)
        return nil, err if _, err := os.Write(...)
twisted1919 commented 4 years ago

I think the ? would be a better fit than try, and always having to chase the defer for error would also be tricky.

This also closes the gates for having exceptions using try/catch forever.

deanveloper commented 4 years ago

This also closes the gates for having exceptions using try/catch forever.

I am more than okay with this.

agnivade commented 4 years ago

I agree with some of the concerns raised above regarding adding context to an error. I am slowly trying to shift from just returning an error to always decorate it with a context and then returning it. With this proposal, I will have to completely change my function to use named return params (which I feel is odd because I barely use naked returns).

As @griesemer says:

Again, this proposal does not attempt to solve all error handling situations. I suspect in most cases try makes sense for code that now looks basically like this: a, b, c, ... err := try(someFunctionCall()) if err != nil { return ..., err } There is an awful lot of code that looks like this. And not every piece of code looking like this needs more error handling. And where defer is not right, one can still use an if statement.

Yes, but shouldn't good, idiomatic code always wrap/decorate their errors ? I believe that's why we are introducing refined error handling mechanisms to add context/wrap errors in stdlib. As I see, this proposal only seems to consider the most basic use case.

Moreover, this proposal addresses only the case of wrapping/decorating multiple possible error return sites at a single place, using named parameters with a defer call.

But it doesn't do anything for the case when one needs to add different contexts to different errors in a single function. For eg, it is very essential to decorate the DB errors to get more information on where they are coming from (assuming no stack traces)

This is an example of a real code I have -

func (p *pgStore) DoWork() error {
    tx, err := p.handle.Begin()
    if err != nil {
        return err
    }
    var res int64
    err = tx.QueryRow(`INSERT INTO table (...) RETURNING c1`, ...).Scan(&res)
    if err != nil {
        tx.Rollback()
        return fmt.Errorf("insert table: %w", err)
    }

    _, err = tx.Exec(`INSERT INTO table2 (...) VALUES ($1)`, res)
    if err != nil {
        tx.Rollback()
        return fmt.Errorf("insert table2: %w", err)
    }
    return tx.Commit()
}

According to the proposal:

If error augmentation or wrapping is desired there are two approaches: Stick with the tried-and-true if statement, or, alternatively, “declare” an error handler with a defer statement:

I think this will fall into the category of "stick with the tried-and-true if statement". I hope the proposal can be improved to address this too.

networkimprov commented 4 years ago

I strongly suggest the Go team prioritize generics, as that's where Go hears the most criticism, and wait on error-handling. Today's technique is not that painful (tho go fmt should let it sit on one line).

The try() concept has all the problems of check from check/handle:

  1. It doesn't read like Go. People want assignment syntax, without the subsequent nil test, as that looks like Go. Thirteen separate responses to check/handle suggested this; see Recurring Themes here: https://github.com/golang/go/wiki/Go2ErrorHandlingFeedback#recurring-themes

    f, #      := os.Open(...) // return on error
    f, #panic := os.Open(...) // panic on error
    f, #hname := os.Open(...) // invoke named handler on error
    // # is any available symbol or unambiguous pair
  2. Nesting of function calls that return errors obscures the order of operations, and hinders debugging. The state of affairs when an error occurs, and therefore the call sequence, should be clear, but here it’s not: try(step4(try(step1()), try(step3(try(step2()))))) Now recall that the language forbids: f(t ? a : b) and f(a++)

  3. It would be trivial to return errors without context. A key rationale of check/handle was to encourage contextualization.

  4. It's tied to type error and the last return value. If we need to inspect other return values/types for exceptional state, we're back to: if errno := f(); errno != 0 { ... }

  5. It doesn't offer multiple pathways. Code that calls storage or networking APIs handles such errors differently than those due to incorrect input or unexpected internal state. My code does one of these far more often than return err:

    • log.Fatal()
    • panic() for errors that should never arise
    • log a message and retry

@gopherbot add Go2, LanguageChange

leaxoy commented 4 years ago

How about use only ? to unwrap result just like rust

mattn commented 4 years ago

The reason we are skeptical about calling try() may be two implicit binding. We can not see the binding for the return value error and arguments for try(). For about try(), we can make a rule that we must use try() with argument function which have error in return values. But binding to return values are not. So I'm thinking more expression is required for users to understand what this code doing.

func doSomething() (int, %error) {
  f := try(foo())
  ...
}

It is hard to add new requirements/feature to the existing syntax.

To be honest, I think that foo() should also have %error.

Add 1 more rule

beoran commented 4 years ago

In the detailed design document I noticed that in an earlier iteration it was suggested to pass an error handler to the try builtin function. Like this:

handler := func(err error) error {
        return fmt.Errorf("foo failed: %v", err)  // wrap error
}

f := try(os.Open(filename), handler)  

or even better, like this:

f := try(os.Open(filename), func(err error) error {
        return fmt.Errorf("foo failed: %v", err)  // wrap error
})  

Although, as the document states, that this raises several questions, I think this proposal would be far more more desirable and useful if it had kept this possibility to optionally specify such an error handler function or closure.

Secondly, I don't mind that a built in that can cause the function to return, but, to bikeshed a bit, the name 'try' is too short to suggest that it can cause a return. So a longer name, like attempt seems better to me.

EDIT: Thirdly, ideally, go language should gain generics first, where an important use case would be the ability to implement this try function as a generic, so the bikeshedding can end, and everyone can get the error handling that they prefer themselves.

sheerun commented 4 years ago

Hacker news has some point: try doesn't behave like a normal function (it can return) so it's not good to give it function-like syntax. A return or defer syntax would be more appropriate:

func CopyFile(src, dst string) (err error) {
        r := try os.Open(src)
        defer r.Close()

        w := try os.Create(dst)
        defer func() {
                w.Close()
                if err != nil {
                        os.Remove(dst) // only if a “try” fails
                }
        }()

        try io.Copy(w, r)
        try w.Close()
        return nil
}
deanveloper commented 4 years ago

@sheerun the common counterargument to this is that panic is also a control-flow altering built-in function. I personally disagree with it, however it is correct.

peterbourgon commented 4 years ago
  1. Echoing @deanveloper above, as well as others' similar comments, I'm very afraid we're underestimating the costs of adding a new, somewhat subtle, and—especially when inlined in other function calls—easily overlooked keyword that manages call stack control flow. panic(...) is a relatively clear exception (pun not intended) to the rule that return is the only way out of a function. I don't think we should use its existence as justification to add a third.
  2. This proposal would canonize returning an un-wrapped error as the default behavior, and relegate wrapping errors as something you have to opt-in to, with additional ceremony. But, in my experience, that's precisely backwards to good practice. I'd hope that a proposal in this space would make it easier, or at least not more difficult, to add contextual information to errors at the error site.