From 331c428e7671b353ec7112e73605f5ef32c8d168 Mon Sep 17 00:00:00 2001 From: Rob Pike Date: Fri, 21 Jun 2013 11:27:53 -0700 Subject: [PATCH] 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 ++++- cmd/vet/shadow.go | 227 +++++++++++++++++++++++++++++++++++++ cmd/vet/testdata/shadow.go | 54 +++++++++ cmd/vet/types.go | 7 ++ cmd/vet/vet_test.go | 3 +- 5 files changed, 313 insertions(+), 4 deletions(-) create mode 100644 cmd/vet/shadow.go create mode 100644 cmd/vet/testdata/shadow.go diff --git a/cmd/vet/main.go b/cmd/vet/main.go index 597f772e..9db7ab02 100644 --- a/cmd/vet/main.go +++ b/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) diff --git a/cmd/vet/shadow.go b/cmd/vet/shadow.go new file mode 100644 index 00000000..91dfe5fa --- /dev/null +++ b/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())) + } +} diff --git a/cmd/vet/testdata/shadow.go b/cmd/vet/testdata/shadow.go new file mode 100644 index 00000000..e0af5ec4 --- /dev/null +++ b/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 +} diff --git a/cmd/vet/types.go b/cmd/vet/types.go index 970a9af1..ee1b57a6 100644 --- a/cmd/vet/types.go +++ b/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) {}, } diff --git a/cmd/vet/vet_test.go b/cmd/vet/vet_test.go index fa8d9f22..235c1b9b 100644 --- a/cmd/vet/vet_test.go +++ b/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) {