CC @adonovan @timothy-king @golang/tools-team

If there are false positives (the outer-a not being used in for, but an inner-a is,) moving the outer declaration to after the block probably improves readability.

I agree, this case seems like a pretty straightforward readability improvement. But since the analyzer is not reporting (with high confidence) an actual mistake, it's not the kind of thing we would enable by default in vet.

If a mix of outer and inner a are used, that could be detected by checking if outer-a is being used at all in the block, before flagging the shadowing.

If a were named err, if would not be at all surprising to find it referenced within the block, and for the block to also declare new variables named err that shadow it. So I doubt that would be very reliable.

I suspect the only way to improve the shadowing analysis is to do an empirical study of a large body of code.

If a were named err, if would not be at all surprising to find it referenced within the block

I don't see why. When would you declare an err right before the block, not use it, and then declare another err inside the block? I use this sometimes:

var err error
if party {
  // ... Preparations...
  err = sliceAPizza()
} else {
  // ... Preparations...
  err = sliceACucumber()
}
if err != nil {
  return err
}

but there, an inner-err would be as bad as in my previous example.

We could possibly even restrict it to the zero-valued case of var a []int. I.e.

a := []int{1}  // Different.
if party {
  a := sliceAPizza()
}

would not be considered a shadowing.

Edit: Additionally, initially, perhaps it could be limited to the case where the outer-a is not used in the block at all. I.e. having the variable declared before the block is completely pointless.

(The lack of the ternary operator is probably why I think this lint check is important in Go.)

I don't see why.

I said that I didn't think it would be that uncommon for an outer err var to be both referenced and shadowed by a block, for example:

   var err error
   if ... {
         err = doMainThingA()
         if err := someCleanupStep(); err != nil { ... } // shadowed
   } else {
         err = doMainThingB()
   }
   if err != nil { ... } 

But all of this is speculation. I think the right thing to do is build a prototype and test it on some large open-source repositories and demonstrate that it has a high signal-to-noise ratio.

Thanks for the clarification and example. (I don't think this case should be flagged, since the inner err is actually one extra scope away from the outer err.)

It sounds like this (building a narrower shadow pass) is something we agree would be useful. Agreed that the next step is a prototype and tweaking until we have rule(s) specific enough to have good SNR.

I'll see what I can do. Thanks.

If the check was to flag this example it would be a false positive:

var someSlice []int
if !party {
  someSlice = sliceACucumber()
}
eat(someSlice)

You will need some way of concluding that the above is different from the given example:

var someSlice []int
if party {
  someSlice := make([]int, 0, 42)
  someSlice = append(someSlice, sliceAPizza()...)
} else {
  someSlice = sliceACucumber()
}
// ...
eat(someSlice)

Here is an approach that could distinguish these. You can do dead write inference on the someSlice = append(..., ...) to get that someSlice is dead at the end of the if, then conclude append(someSlice, sliceAPizza()...) is a dead write to the underlying someSlice array (which is uniquely aliased to make([]int, 0, 42)), then conclude then someSlice read in append is effectively unread, and therefore as someSlice is shadowed variable all of whose usages are dead the analyzer can warn. (Maybe you can come up with a different justification/set of rules?)

To get something into cmd/vet, I would recommend collecting more real world examples to narrow down what the correctness issues you are trying to solve is. (Shadowing is not itself a correctness problem in Go.) This might even be more valuable than proposing a prototype at this stage. (For this particular example, it might be easier to warn on dead writes in vet? Unclear.)

Thanks for the input.

Did you mean to use := in the first example? As it stands, there is no shadowing.

(Assuming :=) I don't see why they should be different. They are both "scary" in the sense that if another developer started working on this code (including me three months later,) there would be a higher risk of confusion/bugs. Wouldn't it be better to suggest avoiding this shadowing? And now, I'm still talking specifically about the case where a variable is declared right before some control block that then assigns to it. Even if the variable was named err, the fact that it was introduced right before the block should be a strong indicator that I mean to assign to it in the block.

Perhaps looking at this as a dead write problem is indeed the better approach.

I did run some tests with a very hacky declaration-before-if detector. I ran it on some big Go projects (Go, Kubernetes, Juju, Gogs) and it looked generally fine. I think I saw 4-5 flagged issues. I caught one thing that could have been a bug, but the function returned early, so it wasn't a problem. The rest were false positives, but could also be a source of confusion. Sadly I don't remember where I put the results, and I broke the implementation since, so I can't tell you more.

I realize this issue is about detecting special cases of shadowing that could be fine, to make the check always correct so it can be included in vet by default. I think the suggestion below is relevant, but I can take it to a new issue.

I run shadow for additional checks in my project. But lots of "err" is flagged. I believe it is very common in Go code to shadow err. It would already help a lot if I can tell shadow to ignore shadowed "err" (and perhaps other variables, "ctx" comes to mind, though I've renamed those in my code). "err" is also used in this discussion as a reason to improve shadow. Perhaps it's enough to solve just that case? I currently filter out warnings about "err" in a separate Makefile target, because it fails. See https://github.com/mjl-/mox/blob/main/Makefile#L32. If there is already an option to skip warnings for certain shadowed variable names, I couldn't find it. I'm also curious if projects exist with a policy to never shadow "err". Perhaps not warning about about "err" could even be the default, with an option to enable checking for it. It wouldn't be enough to run "shadow" by default, but it could make it more useful for the typical case.

@mjl- This almost sounds like the inverse of what I'm suggesting here. You have a codebase that is already shadow-friendly and want to be able to configure exceptions. I'd suggest a new issue.