FAQ
Hi all --

I blew an hour or so yesterday tracking down a bug that was a simple
case of forgetting to handle an error. I wrote

doSomething(...)

when I should have written

err = doSomething(...)
if err != nil {
return err
}

Oops, my bad. Easy mistake to make, though. It just occurred to me
that "go vet" ought to be able to catch this: if calling a function
that returns just "error", and the result isn't assigned to anything,
complain. The logic would presumably be a little more complex for
multi-return functions, but it should be doable. (Or should it? I have
not looked at the source for "go vet" at all.)

FWIW, I've already rigged things up so my build fails if "go vet"
complains about anything, and that has caught a few errors.

(Incidentally, I discovered yesterday that the compiler complains if
you ignore the return value from 'append()'. So there's something
sorta like precedent for this.)

Greg
--
Greg Ward http://www.gerg.ca
<greg@gerg.ca> @gergdotca

--

Search Discussions

  • Martin Angers at Jan 25, 2013 at 7:57 pm
    The various cases are:

    1. f := os.Open("file")
    2. f, _ := os.Open("file")
    3. os.Open("file")

    Case 1) is a compiler error (only one lval, 2 expected). Case 2) is
    explicitly ignored, so neither the compiler nor go vet should complain.
    Case 3) causes no compiler error, and is the one case that would get caught
    by your proposal in go vet, right? So the warning would always be in the
    3rd case, regardless of how many values are returned by the function, as
    long as one of them is an error.

    I can see this being annoying in some cases (File.Close() comes to mind, as
    it is often in a defer call, without the lval). But then, isn't it go vet's
    role to be the verbose, annoying little brother? If I make a parallel with
    javascript, jshint (not Douglas Crockford's stricter jslint) is very
    customizable and can be configured to complain about the things you want.
    I'd like go vet to be that flexible, though honestly I haven't used it
    enough to comment on it.

    Le vendredi 25 janvier 2013 14:05:18 UTC-5, Greg Ward a écrit :
    Hi all --

    I blew an hour or so yesterday tracking down a bug that was a simple
    case of forgetting to handle an error. I wrote

    doSomething(...)

    when I should have written

    err = doSomething(...)
    if err != nil {
    return err
    }

    Oops, my bad. Easy mistake to make, though. It just occurred to me
    that "go vet" ought to be able to catch this: if calling a function
    that returns just "error", and the result isn't assigned to anything,
    complain. The logic would presumably be a little more complex for
    multi-return functions, but it should be doable. (Or should it? I have
    not looked at the source for "go vet" at all.)

    FWIW, I've already rigged things up so my build fails if "go vet"
    complains about anything, and that has caught a few errors.

    (Incidentally, I discovered yesterday that the compiler complains if
    you ignore the return value from 'append()'. So there's something
    sorta like precedent for this.)

    Greg
    --
    Greg Ward http://www.gerg.ca
    <gr...@gerg.ca <javascript:>> @gergdotca
    --
  • Nate Finch at Jan 25, 2013 at 8:11 pm
    This is actually something I was thinking about... what *is* the correct
    way to call file.close in a defer? Seems like defer doesn't give you any
    way to handle the error (not that there's a lot you can do about the file
    not closing... but you could still return the error to the caller, if you
    weren't using defer).
    On Friday, January 25, 2013 2:57:52 PM UTC-5, Martin Angers wrote:


    I can see this being annoying in some cases (File.Close() comes to mind,
    as it is often in a defer call, without the lval).
    --
  • Dan Kortschak at Jan 25, 2013 at 8:33 pm
    It's just a little more verbose. Assuming named returns:

    defer func() { err = x.Close() }()
    On 26/01/2013, at 6:41 AM, "Nate Finch" wrote:

    Seems like defer doesn't give you any way to handle the error (not that there's a lot you can do about the file not closing... but you could still return the error to the caller, if you weren't using defer).
    --
  • Nate Finch at Jan 25, 2013 at 9:14 pm
    Ahh, named returns, that's how you can make it work. Cool.

    Sorry to derail the thread.

    For the record, I think having go vet complain about errors that are not
    assigned to a variable is a good idea. Yes, this would catch defer
    x.Close(), which is probably a good thing.
    On Friday, January 25, 2013 3:33:34 PM UTC-5, kortschak wrote:

    It's just a little more verbose. Assuming named returns:

    defer func() { err = x.Close() }()

    On 26/01/2013, at 6:41 AM, "Nate Finch" <nate....@gmail.com <javascript:>>
    wrote:
    Seems like defer doesn't give you any way to handle the error (not that
    there's a lot you can do about the file not closing... but you could still
    return the error to the caller, if you weren't using defer).
  • Alexey Palazhchenko at Jan 25, 2013 at 9:40 pm

    It's just a little more verbose. Assuming named returns:

    defer func() { err = x.Close() }()
    This code will ignore error if it was present. It probably should be:

    defer func() {
    if err != nil {
    err = x.Close()
    }
    }()


    But… How often do you check error returned by fmt.Print() for example?

    --
  • Bryanturley at Jan 25, 2013 at 9:59 pm

    On Friday, January 25, 2013 3:40:39 PM UTC-6, Alexey Palazhchenko wrote:
    It's just a little more verbose. Assuming named returns:
    defer func() { err = x.Close() }()
    This code will ignore error if it was present. It probably should be:

    defer func() {
    if err != nil {
    err = x.Close()
    }
    }()
    But that version doesn't Close() x if there is already a possibly unrelated
    error.
    If you need to check an error on a Close() or similar you probably
    shouldn't defer it.
  • Alexey Palazhchenko at Jan 25, 2013 at 10:08 pm

    But that version doesn't Close() x if there is already a possibly
    unrelated error.
    My bad, thanks. It should be even more verbose:

    defer func() {
    e := x.Close()
    if err == nil {
    err = e
    }
    }()

    Close() may return error. Typically, previous error is more interesting.


    If you need to check an error on a Close() or similar you probably
    shouldn't defer it.
    It's useful when function has several returns/panics.


    My question is: should we care about every possible error? Should we check
    result of fmt.Print()? Should we check result of os.File.Close() Should we
    write code like above, or just type "defer x.Close()"?

    --
  • Bryanturley at Jan 25, 2013 at 10:16 pm

    On Friday, January 25, 2013 4:08:16 PM UTC-6, Alexey Palazhchenko wrote:
    But that version doesn't Close() x if there is already a possibly
    unrelated error.
    My bad, thanks. It should be even more verbose:

    defer func() {
    e := x.Close()
    if err == nil {
    err = e
    }
    }()

    Close() may return error. Typically, previous error is more interesting.


    If you need to check an error on a Close() or similar you probably
    shouldn't defer it.
    It's useful when function has several returns/panics.
    If you are not abusing panic it is probably ok to not close anything when
    you have a panic.
  • Dan Kortschak at Jan 25, 2013 at 11:51 pm
    I should get more sleep. Yes - this is what I usually do in this situation (where I care about defered returned errors/behaviours).

    On 26/01/2013, at 8:38 AM, "Alexey Palazhchenko" wrote:

    But that version doesn't Close() x if there is already a possibly unrelated error.

    My bad, thanks. It should be even more verbose:

    defer func() {
    e := x.Close()
    if err == nil {
    err = e
    }
    }()

    Close() may return error. Typically, previous error is more interesting.
  • Itmitica at Jan 25, 2013 at 10:18 pm

    On Friday, January 25, 2013 11:40:39 PM UTC+2, Alexey Palazhchenko wrote:
    This code will ignore error if it was present.
    This works: http://play.golang.org/p/TdTvw5X2cO

    --
  • Itmitica at Jan 25, 2013 at 10:22 pm
    This is why:

    http://golang.org/ref/spec#Defer_statements
    For instance, if the deferred function is a function literal and the
    surrounding function has named result parameters that are in scope within
    the literal, the deferred function may access and modify the result
    parameters before they are returned.
    --
  • Dan Kortschak at Jan 25, 2013 at 11:48 pm
    Good point. What you've written is how my actual code looks where I do this. The laziness of non-executed code.

    On 26/01/2013, at 8:10 AM, "Alexey Palazhchenko" wrote:

    It's just a little more verbose. Assuming named returns:

    defer func() { err = x.Close() }()

    This code will ignore error if it was present. It probably should be:

    defer func() {
    if err != nil {
    err = x.Close()
    }
    }()


    But… How often do you check error returned by fmt.Print() for example?

    --
  • Kevin Gillette at Jan 25, 2013 at 8:34 pm
    The "correct way" varies based entirely on whether you care. For *os.File,
    usually all you could do for a close error is log the error (it's not like
    you'd continually try closing the file until it returned without an error).
    There are cases where Close() has a side-effect, such as archive/zip and
    encoding/base64 (a base64 Close going to fail, though), where it is
    important to check for the presence of the error.
    On Friday, January 25, 2013 1:11:27 PM UTC-7, Nate Finch wrote:

    This is actually something I was thinking about... what *is* the correct
    way to call file.close in a defer? Seems like defer doesn't give you any
    way to handle the error (not that there's a lot you can do about the file
    not closing... but you could still return the error to the caller, if you
    weren't using defer).
    On Friday, January 25, 2013 2:57:52 PM UTC-5, Martin Angers wrote:


    I can see this being annoying in some cases (File.Close() comes to mind,
    as it is often in a defer call, without the lval).
    --
  • Kamil Kisiel at Jan 26, 2013 at 1:10 am
    This has been discussed numerous times on the mailing list, do a search in
    the Google group.

    In short: It should be possible to implement in the vet tool once go/types
    is available in Go 1.1. It would probably require a user-configurable
    whitelist for certain functions you usually don't care about (eg:
    fmt.Printf and friends).
    On Friday, January 25, 2013 11:05:18 AM UTC-8, Greg Ward wrote:

    Hi all --

    I blew an hour or so yesterday tracking down a bug that was a simple
    case of forgetting to handle an error. I wrote

    doSomething(...)

    when I should have written

    err = doSomething(...)
    if err != nil {
    return err
    }

    Oops, my bad. Easy mistake to make, though. It just occurred to me
    that "go vet" ought to be able to catch this: if calling a function
    that returns just "error", and the result isn't assigned to anything,
    complain. The logic would presumably be a little more complex for
    multi-return functions, but it should be doable. (Or should it? I have
    not looked at the source for "go vet" at all.)

    FWIW, I've already rigged things up so my build fails if "go vet"
    complains about anything, and that has caught a few errors.

    (Incidentally, I discovered yesterday that the compiler complains if
    you ignore the return value from 'append()'. So there's something
    sorta like precedent for this.)

    Greg
    --
    Greg Ward http://www.gerg.ca
    <gr...@gerg.ca <javascript:>> @gergdotca
    --

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-nuts @
categoriesgo
postedJan 25, '13 at 7:05p
activeJan 26, '13 at 1:10a
posts15
users9
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase