proposal: Go 2: prevent variable aliasing/shadowing

[kodawah]

This is a follow up to the original proposal: #9818

The following code will alias the logw variable and introduce a bug in all the code that falls in the default label.

func OpenLogFile(lo LogOptions) (io.WriteCloser, error) {
    var logw io.WriteCloser
    switch lo.LogPath {
    case "stderr":
        logw = os.Stderr
    default:
        logw, err := openLogFileInternal(lo)
        if err != nil {
            return logw, err
        }
    }
    log.SetOutput(logw)
    return logw, nil
}

logw is declared outside the switch case (and correctly initialized to nil), but then another logw is created together with err in the default case. This will result in logw being always set to nil if the code goes through the default case.

This is a mistake that both newbie and seasonal go developers do. In my opinion, aliasing in general should be avoided in all cases, since compatibility is relevant in this first phase, maybe the := syntax could be made a bit smarter and reuse an already existing variable (at least for multiple declarations, like the one in the sample code). Alternatively, although less ideal, it would be ok if auxiliary tools like go vet could warn the user about this kind of aliasing.

[BattleRattle]

But you could also do the mistake and write logw := os.Stderr, which would also be valid as the switch-case block opens a new scope. The only thing, were the compiler would yell at you, would be when the new variable is unused :)

[cznic]

If declarations in inner scopes cannot shadow declarations in outer scopes then what are scopes even good for?

[earthboundkid]

What if shadowing was allowed with var but not :=?

[pciet]

@cznic do you have an example showing shadowing as useful or necessary?

:= with something, err is a common pattern and isn’t intuitive to me for var scoping.

[dlsniper]

Yes, here: https://gist.github.com/posener/92a55c4cd441fc5e5e85f27bca008721#how-to-solve-this Maybe it's not the nicest way to do it, but it's the least code changing one to do. It's not necessary, but it's definitely useful.

[cznic]

do you have an example showing shadowing as useful or necessary?

Shadowing is useful wherever a separate scope is useful. Just one of the endless posibilities:

var i, j int
var l list
 // ... do something with i

for i := 0; i < n; i++ {
        var l list
        // ... shadowing i and l does not destroy their values in the outer scope
       j += foo(i)
}

println(i, j, list)

[zigo101]

I think whether or not the proposal is reasonable.
The case shown by OP is really a common trap ever encountered by most gophers.

btw, now "go vet" support a "-shadow" option to warn on such traps.

[dlsniper]

btw, now "go vet" support a "-shadow" option to warn on such traps.

go vet supported that for a long time now, but it's not accurate in terms of detection as not all shadows are wrong ,as seen by the examples above. Editors that integrate with go vet usually have that option turned on as well. GoLand also shows the error if you enable the Probable bugs | Shadowing variable inspection.

Given the tooling support that exists around this problem, and the fact that users can make a conscious decision of when they should or should not shadow variables, I argue that this functionality is more useful than detrimental to the language, alternative solutions for "fixing" this by not having shadowing allowed resulting in more code needed to handle those cases.

[kodawah]

Given the tooling support that exists around this problem, and the fact that users can make a conscious decision of when they should or should not shadow variables, I argue that this functionality is more useful than detrimental to the language, alternative solutions for "fixing" this by not having shadowing allowed resulting in more code needed to handle those cases.

Having opened the bug I might be biased, but I disagree with this.

The "more" code mentioned is a very little sacrifice, in comparison to the likelihood of introducing a tripping bug that would take hours to find and fix, due to being so intrinsic with the language. In my opinion this functionality adds little to no benefit, and adds semantic complications that are just not worth it. I don't have any data, but the number of people consciously shadowing variables are probably much fewer than those being tripped by it.

In other words, I would 100% trade the "more code" for a reduced risk of introducing a bug in it.

[kodawah]

A small interesting point about this topic, from an unrelated project

https://chromium-review.googlesource.com/c/chromium/src/+/927181

Chromium is disabling shadowing variables at build time.

[ianlancetaylor]

It's important to clarify what kinds of shadowing are disallowed. Is all shadowing disallowed? For example, since len is declared in the universe block, are we forbidden from using len as a local variable? For another example, what about function literals? If I use i in the outer function, am I forbidden from using i inside the function literal? And so forth.

We don't want a bunch of special cases for shadowing rules. But banning all shadowing makes it that much harder to pick clear, succinct identifiers in all cases.

[earthboundkid]

I think the rule "shadowing is allowed with var but not with :=" is simple and easy to understand, FWIW. :-)

[ianlancetaylor]

This is covered by the discussion in #377.