Anti-patterns in Go: The shadowed err error

[Larry Clapp]

I've seen this in a lot of code (... well, enough to make me ask about it):

func foo(...) (status bool, err error) {

// ...

}

func bar(...) (status bool, err error) {

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

        return // returns err == nil!

    }

}

Worked example: http://play.golang.org/p/SsdRsjYj1G

Basically, you accidentally declare a new err variable in an if statement, which shadows the outer err, and thus your function return value isn't what you think it is.

Any advice on how to avoid this?

Maybe to always "return foo, err" when dealing with error returns?  Does "go vet" (or some other tool) complain about shadowed variables?

-- Larry

[Rémy Oudompheng]

On 2012/9/28 Larry Clapp <la...@theclapp.org> wrote:
> I've seen this in a lot of code (... well, enough to make me ask about it):
>
> func foo(...) (status bool, err error) {
> // ...
> }
>
> func bar(...) (status bool, err error) {
> if b, err := foo(); err != nil {
> return // returns err == nil!
> }
> }
>
> Worked example: http://play.golang.org/p/SsdRsjYj1G
>
> Basically, you accidentally declare a new err variable in an if statement,
> which shadows the outer err, and thus your function return value isn't what
> you think it is.

It is a compiler error. This code used to compile on old Go versions,
but not Go 1.0

Your worked example does now return a shadowed variable. It looks like
a common pattern to log an error but not return it.

Rémy.

[Dumitru Ungureanu]

It's all about scope. So one should be careful with var names and their scope.

[Francesc Campoy Flores]

Hi Larry,

In the code you propose the solution is just to remove a character:

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

to

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

In general, the problem of accidentally shadowing a variable can be avoided by being extra careful when using := at the beginning of a context.

Another typical shadowing case happens with global vars. For instance:

http://play.golang.org/p/8bcrDZpd-R

In this case the local var name is shadowing the global one, and logging it hides the compiler error saying that we declared a variable that is never used.

The solution for this case I would propose is using a temporary variable and assigning to the global one once the errors are checked.

http://play.golang.org/p/kmYUNET7nG

My rule of thumb is to avoid assigning to a variable I know is already declared using :=. And when you have to do it, as in the "name" case, you can always use a temporary variable with a different name.

I hope it helps,

Cheers,

-- Larry

--
 
 

--
--
Francesc

[Lars Pensjö]

I think you misunderstood the question?

I think Larry knows what the problem is, but shows a case where it is easy to do a mistake. It is also one I have done, and I have no good suggestion.

It is extra tricky as variables are shadowed sometimes, and sometimes not. In the following case, "err" is not shadowed?

err,a := x()

err,b := y()

There are exact specifications for this, but it is nevertheless going to bit newcomers as well as some experienced programmers now and then (at least me).

[Dave Cheney]

I think this example illustrates my view that named return values
should be used sparingly, and as a last resort.

> --
>
>

[Patrick Mylund Nielsen]

Totally agree. And even when you do use named return values, it doesn't hurt to be explicit: return foo, err

--

[Dumitru Ungureanu]

It is extra tricky as variables are shadowed sometimes, and sometimes not. In the following case, "err" is not shadowed?

err,a := x()

err,b := y()

The reassignment (:=) for the "same" variable (same id, really), only works for outer-inner contexts.
On the same context level, the above-like reassignment would generate an error: no new variables on left side of :=
http://play.golang.org/p/qB7FNyyGs9

[Dumitru Ungureanu]

Basically, you accidentally declare a new err variable in an if statement, which shadows the outer err, and thus your function return value isn't what you think it is.

 

Your function return value is what you *should* think it is.

I mean, you don't expect to use the same name (err), at the same time (inner context) for two completely different values (outer err and inner err), do you?

As far as I understand it, they are completely different tokens, except for sharing the name during the inner scope.

http://play.golang.org/p/GEvE80_MDm

An inner scope variable name used also in an outer scope is only means to keep code clean. If you choose to do so, you should also pay attention to return values.

http://play.golang.org/p/BIq4h6lOe4

[Jan Mercl]

On Sat, Sep 29, 2012 at 11:25 AM, Dumitru Ungureanu <itmi...@gmail.com> wrote:
>>> err,a := x()
>>> err,b := y()
>
>
> The reassignment (:=) for the "same" variable (same id, really), only works
> for outer-inner contexts.
>
> On the same context level, the above-like reassignment would generate an
> error: no new variables on left side of :=
> http://play.golang.org/p/qB7FNyyGs9

http://play.golang.org/p/GNsMZc8SKq

-j

[Dumitru Ungureanu]

Thanks Jan.

My mistake. I was doing:

err,a := x()
err,a := y()

not

err,a := x()
err,b := y()

To conclude:

- using the short declare syntax (:=) to redeclare a variable in two or more places on the same context level will change the reference completely to the variable in the last := for that context

- using the short declare syntax (:=) to redeclare an outer context variable in an inner context will change the reference temporarily to the inner context variable, until the inner context is closed

[Rémy Oudompheng]

2012/9/29 Dumitru Ungureanu <itmi...@gmail.com>:

> Thanks Jan.
>
> My mistake. I was doing:
>
> err,a := x()
> err,a := y()
>
> not
>
> err,a := x()
> err,b := y()
>
>
> To conclude:
>
> - using the short declare syntax (:=) to redeclare a variable in two or more
> places on the same context level will change the reference completely to the
> variable in the last := for that context

No. If you type

a, err := blah()
b, err := bloh()

The err is the same in both clauses.

In the spec: "Unlike regular variable declarations, a short variable
declaration may redeclare variables provided they were originally
declared in the same block with the same type, and at least one of the
non-blank variables is new. As a consequence, redeclaration can only
appear in a multi-variable short declaration. Redeclaration does not
introduce a new variable; it just assigns a new value to the
original."

Rémy.

[Dumitru Ungureanu]

Maybe I'm not testing it properly?
http://play.golang.org/p/PEMFFN1oYD

On each use of := on a previously declared variable, the memory address changes.
This suggests the err it's not the same as a variable, only the named reference "err" remains.

[Jan Mercl]

On Sat, Sep 29, 2012 at 3:25 PM, Dumitru Ungureanu <itmi...@gmail.com> wrote:
> Maybe I'm not testing it properly?
> http://play.golang.org/p/PEMFFN1oYD

http://play.golang.org/p/d6NM_DPz4s

-j

[minux]

On Sat, Sep 29, 2012 at 9:25 PM, Dumitru Ungureanu <itmi...@gmail.com> wrote:

Maybe I'm not testing it properly?
http://play.golang.org/p/PEMFFN1oYD

On each use of := on a previously declared variable, the memory address changes.
This suggests the err it's not the same as a variable, only the named reference "err" remains.

err is passed to function printError() by value (on stack), so &err in printError() is different from &err

in main().

[Rémy Oudompheng]

Your printError function prints the address of its local copy of err.

Rémy.


2012/9/29, Dumitru Ungureanu <itmi...@gmail.com>:

> Maybe I'm not testing it properly?
> http://play.golang.org/p/PEMFFN1oYD
>
> On each use of := on a previously declared variable, the memory address
> changes.
> This suggests the err it's not the same as a variable, only the named
> reference "err" remains.
>
> On Saturday, September 29, 2012 3:14:54 PM UTC+3, Rémy Oudompheng wrote:
>>

>> 2012/9/29 Dumitru Ungureanu <itmi...@gmail.com <javascript:>>:

> --
>
>
>

[Dumitru Ungureanu]

Thanks Jan, minux, Rémy

What about this:
http://play.golang.org/p/9CVK4dkK82

EDIT:
Never mind:
http://play.golang.org/p/1rcp2oEyAQ

I think this cleared a few things for me.

[Dumitru Ungureanu]

To correctly conclude, hopefully:

1. Using ":=" on the same level context, only does value reassignment to the same value, much like "=", and only in multi-variable short declaration.

2. Using ":=" to redeclare in an inner context an existing outer context variable creates a new variable, and the outer variable call is not possible unless it was first saved to another variable.
http://play.golang.org/p/q7lfga8AJU

EDIT:
Actually, it doesn't even matter how the inner context variable with the same name is declared.

As long as an inner variable exists and it has the same name with an outer variable, you could never use the outer variable without saving it in another variable before the inner variable is declared.

[Dumitru Ungureanu]

I came to the conclusion that Larry's code is lacking.
Double "println" in foo and "Println" in main is what's causing the problem. It's a bad use.

It should be done this way:

http://play.golang.org/p/gMQYmZmzwt

or this way:
http://play.golang.org/p/x5WPyhFIJI