golang / go

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

proposal: leave "if err != nil" alone? #32825

Closed miekg closed 5 years ago

miekg commented 5 years ago

The Go2 proposal #32437 adds new syntax to the language to make the if err != nil { return ... } boilerplate less cumbersome.

There are various alternative proposals: #32804 and #32811 as the original one is not universally loved.

To throw another alternative in the mix: Why not keep it as is?

I've come to like the explicit nature of the if err != nil construct and as such I don't understand why we need new syntax for this. Is it really that bad?

dullgiulio commented 5 years ago

I second this. I really like how decorating every error before returning it adds human readable documentation to the source (usually we format our errors as "could not [what I am doing in these lines of code]: [previous error]") and also to the users reading errors.

Errors generated this way are extremely informative and much easier to read than stack traces. Printed errors that include stack traces usually assume you have ready access to sources (administrators might not have such access) and actually know your way in the code.

Errors without any form of context or tracing (the bare string "EOF") are absolutely useless. I think having shortcuts that make it easier to return naked errors will make Go programs print a lot of useless errors.

If anything, we should push and support decorating errors with context, maybe with new vet and lint rules.

andreynering commented 5 years ago

I also like the explicitly error check. try is confusing and the implicit return is strange.

I think that instead of rethinking errors, we could try an alternative approach to make these checks shorter.

Here's an example which I don't necessarily agree:

value, err := foo()
return err if err != nil

This would allow an shorter but still explicit approach. And it'd allow adding context!

That said, inline ifs are a Ruby thing and don't feel very Goish, but this is just brainstorming. Maybe we find something else.


EDIT: I added a proposal for this here: https://github.com/golang/go/issues/32860

firstrow commented 5 years ago

there should be only one way of doing a thing

DisposaBoy commented 5 years ago

[...]Why not keep it as is?

I think it's fair to say that we all know the answer to this. You need only go read one of the various proposals to find out the answer if you sincerely don't know.

IMO, there's too little detail here for us to have a focused discussion (i.e. I don't think it qualifies as a proposal) and it will soon turn into another bike-shed full of circle-jerking and ideas that make the code less readable.

henderjon commented 5 years ago

So much this.

jochasinga commented 5 years ago

Arguably I got into Go because of this explicit error handling. It sits somewhere between implicit try-catch that many languages go for and function types like Option or Maybe, which favors being returned to the user and be handled explicitly.

I'm not sure if a new construct would really solve this. If you wrapped if err := nil in a helper function like this, it might help a little (pardon my rusty Go):

func handleErr(err error, cb func(error)) {
        if err := nil {
                cb(err)
        }
}

But the issue that makes this helper function less generally useful is the type system, which is a different topic.

tux21b commented 5 years ago

I second this. if err != nil { return err } is not part of any code in our code base. Therefore the try "macro" does not make any sense at all. We only return wrapped errors, with a message describing the context.

Adding context via defer does not make sense either, since we want to return different error messages to distinguish the different kind of errors. A try(fn(), "my error message: %w") might be useful though. But even then, the if err != nil construct might be still preferable, because of shorter line lengths.

punmechanic commented 5 years ago

Frankly, I don't want an implicit return that try provides. If we had generics, I would much prefer a solution that used monad-ish behaviour instead.

type Result<T> interface {
  Expect(err error) T
  OrElse(defaultValue T) T
}

func From<T>(value T, err error) Result<T> { ... }

To me, this is a lot cleaner than the builtin currently being proposed, although further changes would be required to the above since you'd have a proliferation of methods that returned (value, error) and Result

lpar commented 5 years ago

The current try proposal, having no way to explicitly decorate the errors, doesn't meet my needs. I can't imagine ever using it. Frankly, it might as well be called code_smell.

jtarchie commented 5 years ago

It might not make sense to change it, because the wrong problem is trying to be solved.

The code that we are familiar with is not error handling.

if err != nil {
  return err
}

This is error nil handling. At no point in this pattern is the value of an error handled.

If I were to demonstrate this in a different language, Ruby.

begin
 some_method_that_raises_an_error
rescue => e # catch any exception
  retry e        # throw it up again
end

This relays the same behavior as the golang code. When we detect that an exception occurred and then reraise it. We just throw it up the stack.

In golang, we return it.

Where is the actual error handling occurring?

We've all had similar experiences of the failure of this pattern. For example, receiving a file not found error and then spending a large length of time tracing the original thrower of this error.

This is why I believe the try proposal (and others) are faulting. We don't have a good pattern for actually handling errors.

I've see err.Error() string checking, type assertions, etc. to actually inspect the error. We need a pattern for this inconsistency. It feels like xerrs might be solving this, but it also doesn't feel complete yet.

chrispassas commented 5 years ago

I support keeping err!=nil check as is.

allingeek commented 5 years ago

Every time I dig into a sizable Go code base I ask myself how I might reduce some of the boilerplate. I always come back to:

ianlancetaylor commented 5 years ago

The issue tracker is useful for many things, but one thing it is not useful for is a detailed discussion of a complex topic. The issue tracker provides no threading, and replying to a specific message is awkward. Since there is no actual proposal here, just a response to other proposals, I really strongly encourage you to take this discussion to the golang-nuts mailing list.

EthanZeigler commented 5 years ago

If I may, I believe this is the answer. This new error proposal is in direct conflict with the goals of the language.

The reason I love golang is because of its simplicity and clear use of control flow. One of the things I despise most about Java is the try throw construct. It's so disgusting. It encourages terrible error handling. Sending exceptions up the call stack is a horrible and disgusting method of handling control flow. On top, it encourages wrapping everything in a giant check and calling it a day instead of a self documenting and explicit handling of each error situation.

If err != nil encourages good error handling, is self documenting and encourages good documentation as to the specific case, and it's honestly one of the things I love most about go. Making this new control flow interrupt, using messy, somewhat ambiguous returns and parameters, and confusing semantics is not in the spirit of the language I've come to adore.

Verbosity is not a bad thing. Unnecessary verbosity is, but I'd argue that go's error handling is not unnecessary. It's part of the language's charm.

wagslane commented 5 years ago

Couldn't agree more. The explicit error handling is one of the best features of the language IMO. I always feel like many who are bothered by it just aren't used to it yet.

mattn commented 5 years ago

It is not good for the issues are separated, but I'm thinking that two opinions are merged as one opinion in this case.

  1. We don't like new syntax (try or new if-err syntax)
  2. Anyways, we don't want to add new syntax

GitHub vote icons can not interpret the second.

Gats commented 5 years ago

The explicit error handling in go is one of the reasons why I love golang. I don't understand why any go developer would want it any other way. I think the proposal to add new syntax is mostly from people comfortable using syntax used in other languages. it may take some getting used to but it works perfectly once you get use to it.

natefinch commented 5 years ago

I wrote #32811 and I support this proposal more... I'd rather just leave error handling alone. I think the emoji reactions to this proposal say a lot.

marcospedreiro commented 5 years ago

I personally agree with leaving err handling as it is. One of things I like about Go is that the language is minimal, and generally speaking has one way of doing things. By adding new syntax for error handling, we’ll create a world where x% of code uses the current method, and y% uses the new method. This will, among other issues already discussed, create inconsistent code bases. I personally don’t think the value of new error handling syntax is worth the trade offs, since I consider the existing syntax enough/sufficient.

crueber commented 5 years ago

As someone that is newer to Golang, one of the things that I find refreshing about the language is the explicit error handling. I've worked in Java, Ruby, Python, and Node pretty heavily, and dealing with errors is so much more onerous than in Go. I would rather see the clear 'path' of errors, than have it implied to me by some language construct that makes it more vague.

aseure commented 5 years ago

ˋreturn ... if ...ˋ suggestion from @andreynering is actually fairly smart imho. Keeps the code explicit (no hidden control flow break) while cutting down the boilerplate (one-liner). ‬

troy0820 commented 5 years ago

Agree, leave if err != nil alone.

kevineaton commented 5 years ago

I prefer the current format. It is clear and an easy pattern to teach. Bringing new engineers up to speed is simple as they can learn one simple pattern and repeat it. It also asks the users to at least consider the error in the current context, ensuring that at least the engineer is acknowledging an error can occur here and I need to think about what to do.

integrii commented 5 years ago

I wrote #32804 and I would much rather see things NOT change. If your code is long, its because it does a lot of stuff. If you have a lot of error handling code, it's because you're doing a good job of handling all your cases.

Please, lets not add things just for the sake of adding things.

rothrock commented 5 years ago

I enjoy the simplicity of the error handling as is.

Expect is just an anagram for except, and I'd rather not use it. Thanks for starting this.

tmathews commented 5 years ago

Please don't change my holy grail.

icholy commented 5 years ago

There was overwhelming community feedback requesting more streamlined error handling (from the annual survey). The Go Team is now addressing that issue.

kevineaton commented 5 years ago

@icholy Sure, but the current proposals leave a lot to be desired. They all seem to either obfuscate the error handling, revert to more try/catch/finally style implementations, bubble the error handling up out of context, or otherwise make it more complicated. Since Go is supposed to be a simple language, I think a lot of us were hoping for a simple option. I haven't seen any I personally like, so I think that the better option is to keep the current pattern.

One complaint was having to type it, but virtually every editor has shortcuts to insert code snippets, so it really isn't a big deal. Perhaps it is my own experience having used Go since pre 1.0, but I happen to like the simplicity and don't mind the redundancy.

icholy commented 5 years ago

@kevineaton you think try is complicated?

malexdev commented 5 years ago

I agree with this completely. I’m not even personally convinced we need to do anything - I agree the if err != nil checks look awkward at first blush but I haven’t seen anything proposed that actually solves the issue without broadly violating the very things that go is popular for.

kevineaton commented 5 years ago

@icholy after spending ten years writing Java and Python prior to Go, I think it can be. I think you run into Pokemon exception catching, or catch chaining of multiple exceptions, and otherwise introducing even more overhead and boilerplate. I wouldn't go back to that style of error handling if I could ever avoid it, as it almost always led to headaches and confusion ESPECIALLY when teaching. I also teach computer science on top of my day to day work as a software architect, so I am biased towards teaching new developers and mentoring. I would choose Go and it's simple error handling over more potentially more complicated or nuanced error handling any day.

bitfield commented 5 years ago

The issue tracker is useful for many things, but one thing it is not useful for is a detailed discussion of a complex topic.

Ain't that the truth. But here we are.

thomasf commented 5 years ago

if err != nil won't go away if try is added. I believe that try will add some clarity to code paths which are either error forwarding heavy or where a lot of different errors can be summe up easy in one defer error handler. . I don't really see how try encourages not handling errors that much more than a bunch of emptyif-err-return-err. It's easy to ignore actually handling the errors regardless of try is there or not. I think that try is one of the best suggestions for error handling yet because it looks like it will be easy to read code which is using it..

caspervonb commented 5 years ago

My unsolicited two cents, it just doesn't feel very "Go" like. It's too magical and we're going for implicit constructs over explicit ones.

From the Go FAQ

Why does Go not have the ?: operator? There is no ternary testing operation in Go. You may use the following to achieve the same result:

if expr {
   n = trueVal
} else {
    n = falseVal
}

The reason ?: is absent from Go is that the language's designers had seen the operation used too often to create impenetrably complex expressions. The if-else form, although longer, is unquestionably clearer. A language needs only one conditional control flow construct.

sanbornm commented 5 years ago

@ianlancetaylor

The issue tracker is useful for many things, but one thing it is not useful for is a detailed discussion of a complex topic. The issue tracker provides no threading, and replying to a specific message is awkward. Since there is no actual proposal here, just a response to other proposals, I really strongly encourage you to take this discussion to the golang-nuts mailing list.

You can reply to a specific message. I just replied to yours. :)

Since there is no actual proposal here, just a response to other proposals,

A proposal to me means a call for change. This particular issue is anti-change. Do you propose that we create a proposal to not change error handling? I think the proposal system is great but it leaves the status quo underrepresented.

icholy commented 5 years ago

after spending ten years writing Java and Python ... I also teach computer science on top of my day to day work as a software architect

@kevineaton are you done sucking your own dick?

natefinch commented 5 years ago

This issue functions as a long ongoing poll in a semi-official place where basically anyone can easily vote for or against proposals.

Not changing the language to remove if err != nil is a perfectly cromulent proposal that needs basically no added detail. I'm not sure what the problem is. No, it's not god awful long and hard to grok. That doesn't make it wrong or bad or insufficient.

nebiros commented 5 years ago

+1, if nothing better, good thing will be a really good stacktrace info (without frames dance stuff), I guess x/errors already achieve this, but, I would love something ala swift in the near future, like marking funcs using the throws keyword that would return an error + try keyword, preventing error var shadowing (which I personally hate), something like this:

func a() (int) throws {
  throw &someError{}
}

anInt, err := try a()
kevineaton commented 5 years ago

@icholy That was incredibly uncalled for. This is a place for discussion and the Go community is supposed to be a welcoming community. There is no place for that kind of remark. I believe Socrates had something to say about insults in a debate.

fillest commented 5 years ago

The current error handling is human-error-prone. It is easy enough to forget to check err at the moment. If there are any checks already in the scope (and most of the time there are), compiler won't terminate with unused variable. Error handling should be strict - you either _ an error or check it - no leg-shooting should be possible.

maxk42 commented 5 years ago

@kevineaton you think try is complicated?

try is a code smell. It forces indentation amongst your entire block of code instead of just in one spot. Furthermore the "bubble-up" nature of exception handling creates de facto nondeterministic behavior in code and multiple exit points.

The beauty of using multiple return values instead of try is there is one value to check when your function is done and one point of exit from your function (unless, of course, using guard statements or other explicit returns).

try blocks defeat the whole fucking purpose of multiple returns.

theckman commented 5 years ago

@fillest While it would make the code a bit less readable, I do think this would be a value add in terms of safety / explicit error handling. If you look back at the original goals for how we handle errors in Go, I think that would be a nice iteration to avoid the class of bug you cite while still pursuing the spirit of explicit being good.

sanbornm commented 5 years ago

The current error handling is human-error-prone. It is easy enough to forget to check err at the moment. If there are any checks already in the scope (and most of the time there are), compiler won't terminate with unused variable. Error handling should be strict - you either _ an error or check it - no leg-shooting should be possible.

@fillest The proposed change to error handling makes "leg-shooting" easier and errors are more pronounced because they can lazily be handled.

skull-squadron commented 5 years ago

I stopped using Go because of the lack of generics, boilerplate propensity, GC, lack of resource limits/accounting and workload generated from PHP noobs who didn't understand what a compiler does. Haskell, C# and others solved error handling pretty well... the Go 2 proposal looks alright if it has explicit case handling as before (unsure).

lugu commented 5 years ago

Error handling is at the heart of programming. Modeling the business logic (however complex it is) is always simpler than responding to the invalid conditions this logic generates. Simply forwarding an error is a code smell. I wish Go doesn’t encourage this behavior but promotes error management patterns. Beginners often get confused with all this error handling code because they haven’t realized how central error management is.

sorenvonsarvort commented 5 years ago

Fully agree, since the try builtin will not help to wrap errors and add information to them even for a bit.

Before rewriting with try:

_, err := doSomething()
if err != nil {
    return nil, errors.Wrap(err, "failed to do something")
}

_, err = doOtherThing()
if err != nil {
  return nil, errors.Wrap("failed to do the other thing")
}

Imagine what will be after rewriting with try.

kazzmir commented 5 years ago

Since try already acts like a 1-argument function by enclosing its argument in parenthesis, it could accept a 2nd argument that is the error wrapping code.

try(extract_value(try(get_data(1), errors.Wrap(err, "failed to get data")), errors.Wrap(err, "failed to get data")))

Where the err value would have to be implicitly introduced (in a hygenic way). Then if try is used as a 1-argument function then it would just return its error unchanged.

jnericks commented 5 years ago

I agree, the only "syntactic sugar" thing that might make error handling a little simpler is letting us do something like the following when we have multiple returns from our functions... underscores would just be default values of whatever the return types are

if err != nil {
    return _, _, err
}
mccolljr commented 5 years ago

@sorenvonsarvort it doesn't seem that bad to me:

var errContext string 

defer func() {
  // err is a named return
  if err != nil {
    err = fmt.Errorf("%v: %w", errContext, err)
  }
}()

errContext = "failed to do something"
_ := try(doSomething())

errContext = "failed to do other thing"
_ := try(doOtherThing())

By my understanding, you can also still use if err != nil { ... } if it is clearer for that particular section of code.

try shines in other cases. Imagine something more like:

func trySomeComplexOp() (r result, err error) {
  a := try(step1())
  b := try(step2(a))
  c, d := try(step3(b))
  return try(lastStep(c, d)), nil
}

Code like the above code can be a lot cleaner than if you had to sprinkle in if err != nil blocks. Go is all about "linear readability" so I think try does well towards that end.

sirkon commented 5 years ago

There was overwhelming community feedback requesting more streamlined error handling (from the annual survey). The Go Team is now addressing that issue.

This is a vocal minority and I bet a good chunk of them don’t even use Go