proposal: cmd/vet: require explicit variable shadowing

[networkimprov]

Branched from #377.

Problem

Unintended shadows are easy to create with := and cause nearly invisible bugs.

Go takes one of eight possible paths for a, b := f(), as each var is defined, assigned, or shadowed. One cannot tell which without searching prior code, so it's easy to write something that behaves incorrectly.

Proposal

Benefits provided

a) eliminate bugs due to unintended := shadows
b) reduce the complexity of := statements (a, b := f() would have three paths)
c) no language change; tiny learning curve

Changes to go vet

1. Let go vet flag implicit shadows, s := t.

2. Let var s T or var s = t in the new scope silence go vet for intended shadows.

   x, err := Fa()
   if err != nil {
      var err error     // explicit shadow; not flagged by go vet
      y, z, err := Fb()
      ...
      x := 1            // implicit shadow: flagged by vet
   }
   

If accepted, consider...

From #30321: Let var name allow redeclaration of the name within its scope, without creating a shadow. This idea is a companion to explicit shadowing. (Python also permits this.)

x, err := Fa()
if err == nil {      // OR: if var err; err == nil 
   var err           // no shadow in if-scope
   y, z, err := Fb() // no need for separate declarations of y & z
   ...
}
if err != nil { ... }

[beoran]

Apparently such a check already exists, though it is experimental, see: #29260

[mvdan]

What @beoran said. I think it only makes sense to keep this proposal open if it proposes some way to enable the shadow checker by default. See Alan's reasoning as to why that hasn't been a good idea so far.

[networkimprov]

@alandonovan wrote:

If we can improve the heuristics used by 'shadow' to the point where it would no longer be considered experimental then there's no reason not to add it back to the core vet suite.

Change (2) above is that improved heuristic: shadows created with var are not flagged.

This isn't a request for a new -shadow switch.

[josharian]

shadows created with var are not flagged.

Given that vet runs as part of go test, this will provide serious pressure to change code to be compliant. Maybe that’s good, but it could cause serious churn (or worse, lead to people disabling vet for their tests). It would be good to get a sense for exactly how much disruption this would cause.

[ianlancetaylor]

It seems possible that the error handling proposal will let us consider changing := to not redeclare variables. If we were able to make that change, then perhaps this issue would be less important.

I suggest that we postpone taking action on this issue until after the error handling proposal has landed or been rejected.

[networkimprov]

@ianlancetaylor, how could future error handling abolish redeclaration by :=, when much existing code has no reason to revise its error handling?

Note the initial Draft Design requires if err != nil {} for recoverable (ie non-returned) errors. But I expect the next draft to offer named handlers for that case :-)

@josharian, undoubtedly a lot of existing code would be newly flagged. We'd have to roll out in stages;
a) default-off with a switch to enable it, like before,
b) default-on with a switch to disable it.

Also, wouldn't performance improve by fixing needless shadows?

a, err := Fa()
for ... {
   b, err := Fb()  // extra allocation every iteration
}

[randall77]

Also, wouldn't performance improve by fixing needless shadows?

a, err := Fa()
for ... {
b, err := Fb() // extra allocation every iteration
}

No, the generated code should be unaffected. There's no (heap) allocation here.
At most it would require an additional stack slot (if err was used after the for loop), which is almost free.

[ianlancetaylor]

@networkimprov The ability to make that kind of change is the point of #28221. People won't get the new behavior in old code.

It still may not be feasible. But my point is that I think we'll have a better idea later. There is no rush.

[networkimprov]

@randall77, I meant an extra stack object per iteration. EDIT: That seems to trigger a GC(?) every 500K iterations in https://play.golang.org/p/mKCYL9AyJJs

@ianlancetaylor, #28221 doesn't really help; when revising existing code, no one wants to either:
a) rewrite all error handling, or
b) forgo any new error handling.

We're stuck with redeclaration. That is good reason to distinguish it from shadowing.

In any event, we could take the first step and implement this proposal for a -shadow switch.

[beoran]

I feel the heuristic proposed in this issue is too simple and will give too many false positives. I think the thing to do is to improve upon the existing shadow checker heuristic so it can actually be enabled by default. This probably will involve quite a bit of research on existing go code to see where implicit shadowing is used erroneously.

[randall77]

@randall77, I meant an extra stack object per iteration. EDIT: That seems to trigger a GC(?) every 500K iterations in https://play.golang.org/p/mKCYL9AyJJs

Normally it would require a single extra stack object for all iterations.

In your example code you're taking the address of the inner err variable and passing it to fmt, which makes it escape. In that case, yes, you get an allocation on every iteration. But this has nothing to do with shadowing; it would happen if you renamed the inner err variable to something else.

[ianlancetaylor]

I think all discussions about shadowing should wait until we see what happens with error handling. Maybe that will help, maybe it won't. Let's find out first.

I'm putting this proposal on hold.

[networkimprov]

@randall77 is the memory-use impact of doing &v for variables declared in a loop documented anywhere? I don't recall seeing anything. It seems like an FAQ candidate...

[ianlancetaylor]

That effect on memory impact is very specific to the exact version of the compiler being used. And note that the impact is not due to &v in itself; it's due to passing &v to a function that as far as the current compiler version can tell lets the pointer escape to the heap. It would not be surprising if future compiler versions get smart enough to see that in this particular example the pointer does not escape.

[networkimprov]

Even if it's not a permanent feature, it can have a dramatic unforeseen impact on memory use. Shouldn't that be documented somewhere easy-to-find?

[alandonovan]

it can have a dramatic unforeseen impact on memory use.

That's true, but it's also true of many implementation pragmatics, in every language. The correct place to document them is not in the language spec, but in a guide to performance optimization, and every good guide to performance optimization in Go already says something to the effect of "understand the escape analysis".

[networkimprov]

@gopherbot add Go2

To be fair, I suggested an addition to the FAQ, not the spec :-)

[networkimprov]

Could we take the Hold off this proposal, since error handling changes have been deferred indefinitely?

[ianlancetaylor]

Done.

[rsc]

cmd/vet/README lists the requirements for a vet check.
This proposal seems to me to fail "Correctness" and "Precision".
There is plenty of code that shadows err in particular that is completely correct.
Rejecting it in vet will generate far too many false positives, which makes people not trust vet output (even output from other checks).

[networkimprov]

False positives would be eliminated by adding code to make intended shadows explicit. See (2) in Changes to go vet.

Adding that code is the proposed cost to alleviate the possibility of bugs due to shadowing, which bite new and experienced Go users.

[rsc]

Sorry, @networkimprov, but having to type var err error all over the place to silence vet, as in (2), is not going to fly. We just came off of trying to make error handling more lightweight not more heavyweight.

Putting this check in vet (and enabling in go test, which we want to be able to do for everything in vet) would break essentially all Go code in existence. This is a non-starter.

This seems like a likely decline, because it is a (very) backwards-incompatible change.

Leaving open for a week for final comments.

[networkimprov]

Ah, I'd overlooked how common error variable shadows are. I religiously avoid that, but most ppl don't.