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

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

Closed
miekg opened this issue Jun 28, 2019 · 303 comments

Comments

Projects
None yet
@miekg
Copy link
Contributor

commented Jun 28, 2019

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?

@gopherbot gopherbot added this to the Proposal milestone Jun 28, 2019

@gopherbot gopherbot added the Proposal label Jun 28, 2019

@dullgiulio

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

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

This comment has been minimized.

Copy link

commented Jun 28, 2019

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: #32860

@firstrow

This comment has been minimized.

Copy link

commented Jun 28, 2019

there should be only one way of doing a thing

@DisposaBoy

This comment has been minimized.

Copy link

commented Jun 28, 2019

[...]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

This comment has been minimized.

Copy link

commented Jun 28, 2019

So much this.

@jochasinga

This comment has been minimized.

Copy link

commented Jun 28, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

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.

@danpantry

This comment has been minimized.

Copy link

commented Jun 28, 2019

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

This comment has been minimized.

Copy link

commented Jun 28, 2019

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

This comment has been minimized.

Copy link

commented Jun 28, 2019

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

This comment has been minimized.

Copy link

commented Jun 28, 2019

I support keeping err!=nil check as is.

@allingeek

This comment has been minimized.

Copy link

commented Jun 28, 2019

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:

  • Those codepaths exist one way or another.
  • Even if you're not required to hide the codepath, giving people the opportunity to hide it will make that the default behavior (because apparently we still measure how hard a language is to use by line count).
  • If the default behavior hides codepaths then I'd be on the lookout for new "missing cleanup" bugs.
  • The meaning and patterns for returned errors is diverse enough that this proposal would only capture a portion of the perceived issue
  • If only a portion is captured then we'd surely get a bunch of solutions
  • With a bunch of solutions would come the temptation to have some use-case adaptive magic to roll them up
  • That if this were actually an issue then people are free to create their own simple solution or use some mass-adopted pattern. I haven't seen anything like that. Maybe I just haven't looked hard enough.
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

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

This comment has been minimized.

Copy link

commented Jun 28, 2019

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.

@lane-c-wagner

This comment has been minimized.

Copy link

commented Jun 28, 2019

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

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

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

This comment has been minimized.

Copy link

commented Jun 28, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

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

This comment has been minimized.

Copy link

commented Jun 28, 2019

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

This comment has been minimized.

Copy link

commented Jun 28, 2019

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

This comment has been minimized.

Copy link

commented Jun 28, 2019

ˋ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

This comment has been minimized.

Copy link

commented Jun 28, 2019

Agree, leave if err != nil alone.

@kevineaton

This comment has been minimized.

Copy link

commented Jun 28, 2019

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

This comment has been minimized.

Copy link

commented Jun 28, 2019

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

This comment has been minimized.

Copy link

commented Jun 28, 2019

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

This comment has been minimized.

Copy link

commented Jun 28, 2019

Please don't change my holy grail.

@icholy

This comment has been minimized.

Copy link

commented Jun 28, 2019

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

@kevineaton

This comment has been minimized.

Copy link

commented Jun 28, 2019

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

@deanveloper

This comment has been minimized.

Copy link

commented Jul 9, 2019

@as I was not saying that to justify the feature, I was just saying it to clarify what the feature did. I'm quite in the middle with the try proposal.

Not that this makes much of a difference, but it's also a feature in Swift as well, although with a keyword instead of a macro.

@urandom

This comment has been minimized.

Copy link

commented Jul 9, 2019

It seems there is some confusion as to what exactly try is trying to achieve. IMHO, the problem isn't writing multiple if blocks that check for errors. You write those once and you are done. The problem is reading code that has multiple of those blocks. We do a lot more reading than writing. And these blocks obfuscate the actual code because they intertwine with it. Worse yet, a lot of the time they are almost exactly the same, with only a minor string difference somewhere within that if block.

I personally preferred the old check - handle draft, but this at least does a good job separating error and business paths. And we might finally be able to have a single function scope context as opposed to for each call, which currently has a good chance of repeating the same thing as the parent error.

@Freeaqingme

This comment has been minimized.

Copy link

commented Jul 9, 2019

@icholy wrote:

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

I just looked up the survey here: https://blog.golang.org/survey2018-results

Apparently the question was: "What is the biggest challenge you personally face using Go today?" with possible answer "Error handling".

I seriously wonder how based on that question+answer it was deduced that a more brief syntax was required. I might have also answered 'error handling', but by no means I'd have wanted to see another syntax. If I'd checked this option in the survey, I'd have thought of better allowing to wrap errors, provide them with stack traces, etc.

My suggestion would be to step back from all of the error handling proposals (effectively what @miekg was suggesting). And first determine what it actually is that the community wants, document that. Then find out why that's what they want. And only afterwards start looking at ways to achieve that.

I've just been going through the try proposal, but unless I'm missing something it neglects to say why it's being proposed, other than "to eliminate the boilerplate if statements [...}". But there's no mention on why elimination of those boilerplate if statements is necessary.

@deanveloper

This comment has been minimized.

Copy link

commented Jul 9, 2019

I definitely agree with the above. Let's see if the new error values changes help aid the error handling complaints that people have with Go. Then we can see if a more brief syntax is required.

@icholy

This comment has been minimized.

Copy link

commented Jul 9, 2019

People here are arguing against try because they feel that all returned errors should be annotated. The reality is that, in the current corpus of code (including the standard library), a high percentage of error checks have bare un-annotated error returns and would benefit from try. Your belief of how code SHOULD be has nothing to do with the way code IS. Spare me your dogma.

@lane-c-wagner

This comment has been minimized.

Copy link

commented Jul 9, 2019

@icholy Ignoring errors indicates that the dev doesn't care about that error. The error is insignificant or is believed by the caller to be impossible. If that is the case then "try" is just as pointless, the caller simply wouldn't wrap the function in a "try".

@lpar

This comment has been minimized.

Copy link

commented Jul 9, 2019

My suggestion would be to step back from all of the error handling proposals (effectively what @miekg was suggesting). And first determine what it actually is that the community wants, document that. Then find out why that's what they want. And only afterwards start looking at ways to achieve that.

I agree strongly with this. I see a lot of basic disagreement on what functionality any improvement to Go error handling should even support. Every different chunk of functionality people mention is triggering bikeshedding over its naming and syntax, so the discussion is going nowhere.

I'd like to know in more detail what it is that the broader Go community actually wants from any proposed new error handling feature.

I've put together a survey listing a bunch of different features, pieces of error handling functionality I've seen people propose. I've carefully omitted any proposed naming or syntax, and of course tried to make the survey neutral rather than favoring my own opinions.

If people would like to participate, here's the link, shortened for sharing:

https://forms.gle/gaCBgxKRE4RMCz7c7

Everyone who participates should be able to see the summary results. Then perhaps once we have a better idea what people actually want, we'll be able to have an intelligent discussion about whether the try proposal provides those things. (And then maybe even go on to discussing syntax.)

@icholy

This comment has been minimized.

Copy link

commented Jul 9, 2019

@lane-c-wagner are you trying to saying that returning an un-annotated error it is the same as not returning it at all? edit: fixed previous comment

@lane-c-wagner

This comment has been minimized.

Copy link

commented Jul 9, 2019

@icholy Ah I misunderstood. When you said "bare" I thought you meant "_" ignored errors.

@as

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

This proposal argues that no action should be a valid action. This change affects all users of the language because they read the code. As such, a survey identifying the biggest hurdle still needs to ask the community whether this hurdle is worth fixing. This proposal is the closest evaluation of such a question.

@tv42

This comment has been minimized.

Copy link

commented Jul 12, 2019

Please stop saying "that everybody is free to ignore" try. We read code written by others.

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2019

@tv42 I don't know if you are addressing me here, but I've said that as well, and you have a point. Guilty as charged. I will try to be more careful with generalizations such as that. Thanks.

@sirkon

This comment has been minimized.

Copy link

commented Jul 14, 2019

@griesemer you survey was heavily lacking. I voted for error handling, but the issue I meant was full type safety, not verbosity. You better make another one about errors only.

And I still want sum types.

@urban-wombat

This comment has been minimized.

Copy link

commented Jul 15, 2019

This is a proposal about the way gofmt currently formats if err != nil

(This is not an opinion about the try() proposal.)

When an if statement returns a one-line not-nil error value, such as:

err := myFunc()
if err != nil {
    return err
}

gofmt could relax its own if-statement rule and format it on one line like this:

err := myFunc()
if err != nil { return err }

Three lines of error handling code becomes just one line. Less clutter. Easier to follow program flow.

There will need to be some judgement about where to draw the line (pun acknowledged) with this
gofmt rule change. It might include some decoration, such as:

err := myFunc()
if err != nil { return fmt.Errorf("myFunc() blew up! %v", err }

But elaborate multi-line error handling should remain as it is: multi-line and clear and explicit.

@networkimprov

This comment has been minimized.

Copy link

commented Jul 17, 2019

The try proposal has been withdrawn: #32437 (comment)

Generics anyone?

@thomasf

This comment has been minimized.

Copy link

commented Jul 17, 2019

This is a proposal about the way gofmt currently formats if err != nil

I have tried that, imho the code is even more unreadable that way than with multi line formatting. try is much better than that solution.

@plyhun

This comment has been minimized.

Copy link

commented Jul 17, 2019

IMO the problem here is rather not how the error handling is performed, but whether it is ignored. Wouldn't it be possible to leave the if err != nil syntax as is, but restrict the ignorance of the Error returns? Like make it a compiler warning/error with deseverity option for the legacy code.

@sorenvonsarvort

This comment has been minimized.

Copy link

commented Jul 17, 2019

IMO the problem here is rather not how the error handling is performed, but whether it is ignored. Wouldn't it be possible to leave the if err != nil syntax as is, but restrict the ignorance of the Error returns? Like make it a compiler warning/error with deseverity option for the legacy code.

Many people want a linter showing ignored errors.

@plyhun

This comment has been minimized.

Copy link

commented Jul 17, 2019

I'd prefer making this a hard error, but looking at the tons of already written legacy, linter is fair as well.

@therealplato

This comment has been minimized.

Copy link

commented Jul 17, 2019

i find https://github.com/kisielk/errcheck valuable for telling me about unhandled errors @plyhun @sorenvonsarvort

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2019

As seen in the discussion on #32437, this proposal has in effect been accepted for now. Closing. If the issue arises again, a new proposal can be opened.

@DeedleFake

This comment has been minimized.

Copy link

commented Jul 17, 2019

I'm starting to think that one of the reasons that a lot of the proposals feel like they don't quite fit right is because they're actually trying to address two different problems at the same time. On the one hand, it's true that having err != nil blocks after nearly every function call can break up the flow of the code in a weird way, although it certainly has its upsides, but I think that that's only half of the problem. The other issue is that handling multiple returns, regardless of whether there were errors involved or not, can be quite clunky.

Multiple return functions feel very, very different from single return functions, despite the seemingly small difference between the two. It's kind of like if there were extra restrictions on calling functions that take more than one argument. It feels very odd to deal with sometimes. When you call a function with multiple return values, you almost always need to do so on its own line, and it, combined with :=, is often the main source of the various variable shadowing problems that have been discussed elsewhere. You can't chain method calls onto them, you can't assign from them directly to a struct field and a new variable on the same line, and so on.

I don't know. Maybe it's just me. But I've used Go for nearly 10 years now and calling functions with multiple returns still feels kind of awkward to me sometimes.

@miekg

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

Thank you!

@mvndaai

This comment has been minimized.

Copy link

commented Jul 18, 2019

There is one actually issue with if err != nil, the scope of err can live longer than it should. When you inline the if it solves the issue, but not all case can be inlined.

if err := foo(); err != nil {
if _, err := bar(); err != nil {

When you have other variables from needed after the handling creates a problem.

a, err := bar()
if err != nil {

Or the other way variable already exists.

var err error
baz.A, err = bar()
if err != nil {

The err variable should not exists in the function scope after the if err != nil {} block completes. Here is my proposal that builds off of the try() proposal to fix the issue #33161. I would love some constructive feedback.

@Freeaqingme

This comment has been minimized.

Copy link

commented Jul 18, 2019

The err variable should not exists in the function scope after the if err != nil {} block completes.

why "should" it not exist after the if block completes? The compiler can optimize for it (if it'd deem that necessary), and there's no mental load when the err := stmt()\nif err != nil {} block completes because these almost always go together.

I haven't yet looked at your proposal in depth (though kudo's for going through the effort of writing one!). However, as I also outlined in my comment above, I think more research is required into any perceived problems, before we dig into any proposals to resolve them.

@mvndaai

This comment has been minimized.

Copy link

commented Jul 18, 2019

@Freeaqingme errors should not exist after the if err != nil block completes, mostly because we already act like it doesn't.

In the CopyFile example, there is r, err := os.Open(src) followed by w, err := os.Create(dst). The second err is shadowing the first one. Shadowing variables is usually frowned upon.

There are also other oddities. If I have err := foo() and later something like bar.V, err = baz(), if the code is refactored and I no longer need foo() I would need to add var err error before the baz line. . I don't think that refactoring a different location in a function should affect other places like that.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2019

Technically in

    r, err := os.Open(src)
    if err != nil {
        return ...
    }
    w, err := os.Create(dst)

the second instance of err does not shadow the first instance. They are actually the same variable. See the discussion of redeclaring variables at https://golang.org/ref/spec#Short_variable_declarations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.