code review 10409047: go.tools/cmd/vet: add check for shadowed variables

go.tools/cmd/vet: add check for shadowed variables
Experimental feature. It's too noisy yet to be enabled by default,
so it must be enabled explicitly by
        go tool vet -shadow *.go
or
        go tool vet -shadow directory
(The go command does not know about the -shadow flag.)

Fixes issue 5634.

[r, 2013-06-20 03:54:20]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go.tools

[gri, 2013-06-21 17:20:34]

LGTM

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/main.go
File cmd/vet/main.go (right):

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/main.go#newcode29
cmd/vet/main.go:29: var strictShadowing = flag.Bool("shadowstrict", false
/*TODO*/, "whether to be strict about shadowing; can be noisy")
fyi: TODOs have to be of the form: TODO(uid) to be found by godoc.

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/shadow.go
File cmd/vet/shadow.go (right):

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/shadow.go#newcode8
cmd/vet/shadow.go:8: with the same name and type as a variable in an outer
scope.
expand to incorporate spans?

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/shadow.go#newcode49
cmd/vet/shadow.go:49: type Span struct {
needs to be exported? or don't care?

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/shadow.go#newcode56
cmd/vet/shadow.go:56: return s.min <= pos && pos <= s.max
pos < s.max

if s.max is the End() of the last mention of the identifier, see below. Seems a
bit cleaner.

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/shadow.go#newcode65
cmd/vet/shadow.go:65: pos := ident.Pos()
It seems a bit cleaner to take the extent of the identifier into account.

end := ident.End()

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/shadow.go#newcode68
cmd/vet/shadow.go:68: if span.min > pos {
if pos < span.min {

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/shadow.go#newcode71
cmd/vet/shadow.go:71: if span.max < pos {
if span.max < end {
   span.max = end
}

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/shadow.go#newcode75
cmd/vet/shadow.go:75: span = Span{pos, pos}
Span{pos, end}

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/shadow.go#newcode94
cmd/vet/shadow.go:94: f.Badf(expr.Pos(), "internal error: short variable
declaration of non-identifier")
s/internal error/invalid AST/ (?)

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/shadow.go#newcode116
cmd/vet/shadow.go:116: f.Badf(expr.Pos(), "internal error: short variable
declaration of non-identifier")
s/internal error/invalid AST/

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/shadow.go#newcode119
cmd/vet/shadow.go:119: if rhs, ok := a.Rhs[i].(*ast.Ident); ok {
use a type switch here?

switch rhs := a.Rhs[i] {
case *ast.Ident:
 ...
case *ast.TypeAssertExpr:
 ...
}

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/shadow.go#newcode165
cmd/vet/shadow.go:165: f.Badf(spec.Pos(), "internal error: var GenDecl not
ValueSpec")
s/internal error/invalid AST/

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/testdata/shadow.go
File cmd/vet/testdata/shadow.go (right):

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/testdata/shadow.go#...
cmd/vet/testdata/shadow.go:45: var x int // ERROR "declaration of x shadows
declaration at testdata/shadow.go:12"
the compiler would complain about this one, no? it's never used

[r, 2013-06-21 18:27:56]

*** Submitted as
https://code.google.com/p/go/source/detail?r=6acfcebcb6c6&repo=tools ***

go.tools/cmd/vet: add check for shadowed variables
Experimental feature. It's too noisy yet to be enabled by default,
so it must be enabled explicitly by
        go tool vet -shadow *.go
or
        go tool vet -shadow directory
(The go command does not know about the -shadow flag.)

Fixes issue 5634.

R=golang-dev, gri
CC=golang-dev
https://codereview.appspot.com/10409047

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/main.go
File cmd/vet/main.go (right):

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/main.go#newcode29
cmd/vet/main.go:29: var strictShadowing = flag.Bool("shadowstrict", false
/*TODO*/, "whether to be strict about shadowing; can be noisy")
On 2013/06/21 17:20:35, gri wrote:
> fyi: TODOs have to be of the form: TODO(uid) to be found by godoc.

i just deleted it. it's obviously work in progress

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/shadow.go
File cmd/vet/shadow.go (right):

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/shadow.go#newcode49
cmd/vet/shadow.go:49: type Span struct {
On 2013/06/21 17:20:35, gri wrote:
> needs to be exported? or don't care?

don't care. this is main. there are many Capital types.

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/shadow.go#newcode65
cmd/vet/shadow.go:65: pos := ident.Pos()
On 2013/06/21 17:20:35, gri wrote:
> It seems a bit cleaner to take the extent of the identifier into account.
> 
> end := ident.End()

Done.

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/shadow.go#newcode68
cmd/vet/shadow.go:68: if span.min > pos {
On 2013/06/21 17:20:35, gri wrote:
> if pos < span.min {

i like to keep them the same order: if a OP b { a = b }

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/shadow.go#newcode71
cmd/vet/shadow.go:71: if span.max < pos {
On 2013/06/21 17:20:35, gri wrote:
> if span.max < end {
>    span.max = end
> }

Done.

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/shadow.go#newcode75
cmd/vet/shadow.go:75: span = Span{pos, pos}
On 2013/06/21 17:20:35, gri wrote:
> Span{pos, end}

Done.

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/shadow.go#newcode116
cmd/vet/shadow.go:116: f.Badf(expr.Pos(), "internal error: short variable
declaration of non-identifier")
On 2013/06/21 17:20:35, gri wrote:
> s/internal error/invalid AST/

Done.

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/shadow.go#newcode119
cmd/vet/shadow.go:119: if rhs, ok := a.Rhs[i].(*ast.Ident); ok {
On 2013/06/21 17:20:35, gri wrote:
> use a type switch here?
> 
> switch rhs := a.Rhs[i] {
> case *ast.Ident:
>  ...
> case *ast.TypeAssertExpr:
>  ...
> }

Done.

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/shadow.go#newcode165
cmd/vet/shadow.go:165: f.Badf(spec.Pos(), "internal error: var GenDecl not
ValueSpec")
On 2013/06/21 17:20:35, gri wrote:
> s/internal error/invalid AST/

Done.

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/testdata/shadow.go
File cmd/vet/testdata/shadow.go (right):

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/testdata/shadow.go#...
cmd/vet/testdata/shadow.go:45: var x int // ERROR "declaration of x shadows
declaration at testdata/shadow.go:12"
On 2013/06/21 17:20:35, gri wrote:
> the compiler would complain about this one, no? it's never used

Done.

[r, 2013-06-21 18:28:07]

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/shadow.go
File cmd/vet/shadow.go (right):

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/shadow.go#newcode8
cmd/vet/shadow.go:8: with the same name and type as a variable in an outer
scope.
On 2013/06/21 17:20:35, gri wrote:
> expand to incorporate spans?

Done.

https://codereview.appspot.com/10409047/diff/5001/cmd/vet/shadow.go#newcode49
cmd/vet/shadow.go:49: type Span struct {
On 2013/06/21 17:20:35, gri wrote:
> needs to be exported? or don't care?

Done.

[r, 2013-06-21 18:28:54]

I had lots more comments that don't appear here. They weren't important, but
yuck.