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 golang/go#5634.

R=golang-dev, gri
CC=golang-dev
https://golang.org/cl/10409047

cmd/vet/main.go

@@ -26,6 +26,8 @@ import (
 )
 
 var verbose = flag.Bool("v", false, "verbose")
+var strictShadowing = flag.Bool("shadowstrict", false, "whether to be strict about shadowing; can be noisy")
+var testFlag = flag.Bool("test", false, "for testing only: sets -all and -shadow")
 var exitCode = 0
 
 // Flags to control which checks to perform. "all" is set to true here, and disabled later if
@@ -40,6 +42,7 @@ var report = map[string]*bool{
 	"methods":     flag.Bool("methods", false, "check that canonically named methods are canonically defined"),
 	"printf":      flag.Bool("printf", false, "check printf-like invocations"),
 	"rangeloops":  flag.Bool("rangeloops", false, "check that range loop variables are used correctly"),
+	"shadow":      flag.Bool("shadow", false, "check for shadowed variables (experimental; must be set explicitly)"),
 	"structtags":  flag.Bool("structtags", false, "check that struct field tags have canonical format"),
 	"unreachable": flag.Bool("unreachable", false, "check for unreachable code"),
 }
@@ -48,6 +51,12 @@ var report = map[string]*bool{
 
 // vet tells whether to report errors for the named check, a flag name.
 func vet(name string) bool {
+	if *testFlag {
+		return true
+	}
+	if name == "shadow" {
+		return *report["shadow"]
+	}
 	return *report["all"] || *report[name]
 }
 
@@ -184,8 +193,10 @@ func doPackageDir(directory string) {
 
 type Package struct {
 	path   string
+	idents map[*ast.Ident]types.Object
 	types  map[ast.Expr]types.Type
 	values map[ast.Expr]exact.Value
+	spans  map[types.Object]Span
 	files  []*File
 }
 
@@ -305,6 +316,7 @@ func (f *File) Badf(pos token.Pos, format string, args ...interface{}) {
 	setExit(1)
 }
 
+// loc returns a formatted representation of the position.
 func (f *File) loc(pos token.Pos) string {
 	if pos == token.NoPos {
 		return ""
@@ -313,17 +325,17 @@ func (f *File) loc(pos token.Pos) string {
 	// expression instead of the inner part with the actual error, the
 	// precision can mislead.
 	posn := f.fset.Position(pos)
-	return fmt.Sprintf("%s:%d: ", posn.Filename, posn.Line)
+	return fmt.Sprintf("%s:%d", posn.Filename, posn.Line)
 }
 
 // Warn reports an error but does not set the exit code.
 func (f *File) Warn(pos token.Pos, args ...interface{}) {
-	fmt.Fprint(os.Stderr, f.loc(pos)+fmt.Sprintln(args...))
+	fmt.Fprint(os.Stderr, f.loc(pos)+": "+fmt.Sprintln(args...))
 }
 
 // Warnf reports a formatted error but does not set the exit code.
 func (f *File) Warnf(pos token.Pos, format string, args ...interface{}) {
-	fmt.Fprintf(os.Stderr, f.loc(pos)+format+"\n", args...)
+	fmt.Fprintf(os.Stderr, f.loc(pos)+": "+format+"\n", args...)
 }
 
 // walkFile walks the file's tree.
@@ -347,6 +359,8 @@ func (f *File) Visit(node ast.Node) ast.Visitor {
 		f.walkFuncDecl(n)
 	case *ast.FuncLit:
 		f.walkFuncLit(n)
+	case *ast.GenDecl:
+		f.walkGenDecl(n)
 	case *ast.InterfaceType:
 		f.walkInterfaceType(n)
 	case *ast.RangeStmt:
@@ -359,6 +373,7 @@ func (f *File) Visit(node ast.Node) ast.Visitor {
 func (f *File) walkAssignStmt(stmt *ast.AssignStmt) {
 	f.checkAssignStmt(stmt)
 	f.checkAtomicAssignment(stmt)
+	f.checkShadowAssignment(stmt)
 }
 
 // walkCall walks a call expression.
@@ -402,6 +417,11 @@ func (f *File) walkFuncDecl(d *ast.FuncDecl) {
 	}
 }
 
+// walkGenDecl walks a general declaration.
+func (f *File) walkGenDecl(d *ast.GenDecl) {
+	f.checkShadowDecl(d)
+}
+
 // walkFuncLit walks a function literal.
 func (f *File) walkFuncLit(x *ast.FuncLit) {
 	f.checkUnreachable(x.Body)

cmd/vet/shadow.go

@@ -0,0 +1,227 @@
+// Copyright 2013 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+/*
+This file contains the code to check for shadowed variables.
+A shadowed variable is a variable declared in an inner scope
+with the same name and type as a variable in an outer scope,
+and where the outer variable is mentioned after the inner one
+is declared.
+
+(This definition can be refined; the module generates too many
+false positives and is not yet enabled by default.)
+
+For example:
+
+	func BadRead(f *os.File, buf []byte) error {
+		var err error
+		for {
+			n, err := f.Read(buf) // shadows the function variable 'err'
+			if err != nil {
+				break // causes return of wrong value
+			}
+			foo(buf)
+		}
+		return err
+	}
+
+*/
+
+package main
+
+import (
+	"go/ast"
+	"go/token"
+
+	"code.google.com/p/go.tools/go/types"
+)
+
+// Span stores the minimum range of byte positions in the file in which a
+// given variable (types.Object) is mentioned. It is lexically defined: it spans
+// from the beginning of its first mention to the end of its last mention.
+// A variable is considered shadowed (if *strictShadowing is off) only if the
+// shadowing variable is declared within the span of the shadowed variable.
+// In other words, if a variable is shadowed but not used after the shadowed
+// variable is declared, it is inconsequential and not worth complaining about.
+// This simple check dramatically reduces the nuisance rate for the shadowing
+// check, at least until something cleverer comes along.
+//
+// One wrinkle: A "naked return" is a silent use of a variable that the Span
+// will not capture, but the compilers catch naked returns of shadowed
+// variables so we don't need to.
+//
+// Cases this gets wrong (TODO):
+// - If a for loop's continuation statement mentions a variable redeclared in
+// the block, we should complain about it but don't.
+// - A variable declared inside a function literal can falsely be identified
+// as shadowing a variable in the outer function.
+//
+type Span struct {
+	min token.Pos
+	max token.Pos
+}
+
+// contains reports whether the position is inside the span.
+func (s Span) contains(pos token.Pos) bool {
+	return s.min <= pos && pos < s.max
+}
+
+// growSpan expands the span for the object to contain the instance represented
+// by the identifier.
+func (pkg *Package) growSpan(ident *ast.Ident, obj types.Object) {
+	if *strictShadowing {
+		return // No need
+	}
+	pos := ident.Pos()
+	end := ident.End()
+	span, ok := pkg.spans[obj]
+	if ok {
+		if span.min > pos {
+			span.min = pos
+		}
+		if span.max < end {
+			span.max = end
+		}
+	} else {
+		span = Span{pos, end}
+	}
+	pkg.spans[obj] = span
+}
+
+// checkShadowAssignment checks for shadowing in a short variable declaration.
+func (f *File) checkShadowAssignment(a *ast.AssignStmt) {
+	if !vet("shadow") {
+		return
+	}
+	if a.Tok != token.DEFINE {
+		return
+	}
+	if f.idiomaticShortRedecl(a) {
+		return
+	}
+	for _, expr := range a.Lhs {
+		ident, ok := expr.(*ast.Ident)
+		if !ok {
+			f.Badf(expr.Pos(), "invalid AST: short variable declaration of non-identifier")
+			return
+		}
+		f.checkShadowing(ident)
+	}
+}
+
+// idiomaticShortRedecl reports whether this short declaration can be ignored for
+// the purposes of shadowing, that is, that any redeclarations it contains are deliberate.
+func (f *File) idiomaticShortRedecl(a *ast.AssignStmt) bool {
+	// Don't complain about deliberate redeclarations of the form
+	//	i := i
+	// Such constructs are idiomatic in range loops to create a new variable
+	// for each iteration. Another example is
+	//	switch n := n.(type)
+	if len(a.Rhs) != len(a.Lhs) {
+		return false
+	}
+	// We know it's an assignment, so the LHS must be all identifiers. (We check anyway.)
+	for i, expr := range a.Lhs {
+		lhs, ok := expr.(*ast.Ident)
+		if !ok {
+			f.Badf(expr.Pos(), "invalid AST: short variable declaration of non-identifier")
+			return true // Don't do any more processing.
+		}
+		switch rhs := a.Rhs[i].(type) {
+		case *ast.Ident:
+			if lhs.Name != rhs.Name {
+				return false
+			}
+		case *ast.TypeAssertExpr:
+			if id, ok := rhs.X.(*ast.Ident); ok {
+				if lhs.Name != id.Name {
+					return false
+				}
+			}
+		}
+	}
+	return true
+}
+
+// idiomaticRedecl reports whether this declaration spec can be ignored for
+// the purposes of shadowing, that is, that any redeclarations it contains are deliberate.
+func (f *File) idiomaticRedecl(d *ast.ValueSpec) bool {
+	// Don't complain about deliberate redeclarations of the form
+	//	var i, j = i, j
+	if len(d.Names) != len(d.Values) {
+		return false
+	}
+	for i, lhs := range d.Names {
+		if rhs, ok := d.Values[i].(*ast.Ident); ok {
+			if lhs.Name != rhs.Name {
+				return false
+			}
+		}
+	}
+	return true
+}
+
+// checkShadowDecl checks for shadowing in a general variable declaration.
+func (f *File) checkShadowDecl(d *ast.GenDecl) {
+	if !vet("shadow") {
+		return
+	}
+	if d.Tok != token.VAR {
+		return
+	}
+	for _, spec := range d.Specs {
+		valueSpec, ok := spec.(*ast.ValueSpec)
+		if !ok {
+			f.Badf(spec.Pos(), "invalid AST: var GenDecl not ValueSpec")
+			return
+		}
+		// Don't complain about deliberate redeclarations of the form
+		//	var i = i
+		if f.idiomaticRedecl(valueSpec) {
+			return
+		}
+		for _, ident := range valueSpec.Names {
+			f.checkShadowing(ident)
+		}
+	}
+}
+
+// checkShadowing checks whether the identifier shadows an identifier in an outer scope.
+func (f *File) checkShadowing(ident *ast.Ident) {
+	obj := f.pkg.idents[ident]
+	if obj == nil {
+		return
+	}
+	// obj.Parent.Parent is the surrounding scope. If we can find another declaration
+	// starting from there, we have a shadowed variable.
+	shadowed := obj.Parent().Parent().LookupParent(obj.Name())
+	if shadowed == nil {
+		return
+	}
+	// Don't complain if it's shadowing a universe-declared variable; that's fine.
+	if shadowed.Parent() == types.Universe {
+		return
+	}
+	if *strictShadowing {
+		// The shadowed variable must appear before this one to be an instance of shadowing.
+		if shadowed.Pos() > ident.Pos() {
+			return
+		}
+	} else {
+		// Don't complain if the span of validity of the shadowed variable doesn't include
+		// the shadowing variable.
+		span, ok := f.pkg.spans[shadowed]
+		if !ok {
+			f.Badf(ident.Pos(), "internal error: no range for %s", ident.Name)
+			return
+		}
+		if !span.contains(ident.Pos()) {
+			return
+		}
+	}
+	// Don't complain if the types differ: that implies the programmer really wants two variables.
+	if types.IsIdentical(obj.Type(), shadowed.Type()) {
+		f.Badf(ident.Pos(), "declaration of %s shadows declaration at %s", obj.Name(), f.loc(shadowed.Pos()))
+	}
+}

cmd/vet/testdata/shadow.go

@@ -0,0 +1,54 @@
+// Copyright 2013 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// This file contains tests for the shadowed variable checker.
+// Some of these errors are caught by the compiler (shadowed return parameters for example)
+// but are nonetheless useful tests.
+
+package testdata
+
+import "os"
+
+func ShadowRead(f *os.File, buf []byte) (err error) {
+	var x int
+	if f != nil {
+		err := 3 // OK - different type.
+	}
+	if f != nil {
+		_, err := f.Read(buf) // ERROR "declaration of err shadows declaration at testdata/shadow.go:13"
+		if err != nil {
+			return
+		}
+		i := 3 // OK
+		_ = i
+	}
+	if f != nil {
+		var _, err = f.Read(buf) // ERROR "declaration of err shadows declaration at testdata/shadow.go:13"
+		if err != nil {
+			return
+		}
+	}
+	for i := 0; i < 10; i++ {
+		i := i // OK: obviously intentional idiomatic redeclaration
+		go func() {
+			println(i)
+		}()
+	}
+	var shadowTemp interface{}
+	switch shadowTemp := shadowTemp.(type) { // OK: obviously intentional idiomatic redeclaration
+	case int:
+		println("OK")
+	}
+	var shadowTemp = shadowTemp // OK: obviously intentional idiomatic redeclaration
+	if true {
+		var f *os.File // OK because f is not mentioned later in the function.
+		// The declaration of x is a shadow because x is mentioned below.
+		var x int // ERROR "declaration of x shadows declaration at testdata/shadow.go:14"
+		_ = x
+		f = nil
+	}
+	// Use a couple of variables to trigger shadowing errors.
+	_, _ = err, x
+	return
+}

cmd/vet/types.go

@@ -15,6 +15,8 @@ import (
 )
 
 func (pkg *Package) check(fs *token.FileSet, astFiles []*ast.File) error {
+	pkg.idents = make(map[*ast.Ident]types.Object)
+	pkg.spans = make(map[types.Object]Span)
 	pkg.types = make(map[ast.Expr]types.Type)
 	pkg.values = make(map[ast.Expr]exact.Value)
 	exprFn := func(x ast.Expr, typ types.Type, val exact.Value) {
@@ -23,9 +25,14 @@ func (pkg *Package) check(fs *token.FileSet, astFiles []*ast.File) error {
 			pkg.values[x] = val
 		}
 	}
+	identFn := func(id *ast.Ident, obj types.Object) {
+		pkg.idents[id] = obj
+		pkg.growSpan(id, obj)
+	}
 	// By providing the Context with our own error function, it will continue
 	// past the first error. There is no need for that function to do anything.
 	context := types.Context{
+		Ident: identFn,
 		Expr:  exprFn,
 		Error: func(error) {},
 	}

cmd/vet/vet_test.go

@@ -20,7 +20,7 @@ const (
 
 // Run this shell script, but do it in Go so it can be run by "go test".
 // 	go build -o testvet
-// 	$(GOROOT)/test/errchk ./testvet -printfuncs='Warn:1,Warnf:1' testdata/*.go testdata/*.s
+// 	$(GOROOT)/test/errchk ./testvet -shadow -printfuncs='Warn:1,Warnf:1' testdata/*.go testdata/*.s
 // 	rm testvet
 //
 func TestVet(t *testing.T) {
@@ -50,6 +50,7 @@ func TestVet(t *testing.T) {
 	flags := []string{
 		"./" + binary,
 		"-printfuncs=Warn:1,Warnf:1",
+		"-test", // TODO: Delete once -shadow is part of -all.
 	}
 	cmd = exec.Command(errchk, append(flags, files...)...)
 	if !run(cmd, t) {