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: cmd/vet: require explicit variable shadowing #31064

Open
networkimprov opened this issue Mar 26, 2019 · 19 comments

Comments

@networkimprov
Copy link

commented Mar 26, 2019

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

This comment has been minimized.

Copy link

commented Mar 27, 2019

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

@mvdan

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

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

This comment has been minimized.

Copy link
Author

commented Mar 27, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

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

This comment has been minimized.

Copy link
Author

commented Mar 28, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

@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

This comment has been minimized.

Copy link
Author

commented Mar 28, 2019

@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

This comment has been minimized.

Copy link

commented Mar 28, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

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

This comment has been minimized.

Copy link
Author

commented Mar 28, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

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

This comment has been minimized.

Copy link
Author

commented Mar 28, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

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

This comment has been minimized.

Copy link
Author

commented Mar 28, 2019

@gopherbot add Go2

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

@networkimprov

This comment has been minimized.

Copy link
Author

commented Aug 9, 2019

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

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

Done.

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