From 86e41f819a814bcc3f7d015c16fc5e98a77775bc Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Thu, 19 Sep 2013 10:05:34 -0700 Subject: [PATCH] go.tools/go/types: "imported but not used" checks for packages also: - initial code for unused label errors - some cleanups, better names - additional tests TODO: Dot-imported packages are not handled yet; i.e., they are always considered used for now. R=adonovan CC=golang-dev https://golang.org/cl/13768043 --- go/types/api_test.go | 2 +- go/types/call.go | 3 +- go/types/check.go | 1 + go/types/check_test.go | 1 + go/types/eval_test.go | 2 + go/types/objects.go | 8 +++- go/types/resolver.go | 60 ++++++++++++++++++++++-------- go/types/stdlib_test.go | 1 - go/types/stmt.go | 46 +++++++++-------------- go/types/testdata/decls0.src | 18 +-------- go/types/testdata/importdecl0a.src | 43 +++++++++++++++++++++ go/types/testdata/importdecl0b.src | 12 ++++++ go/types/token_test.go | 47 +++++++++++++++++++++++ go/types/types.go | 3 +- 14 files changed, 178 insertions(+), 69 deletions(-) create mode 100644 go/types/testdata/importdecl0a.src create mode 100644 go/types/testdata/importdecl0b.src create mode 100644 go/types/token_test.go diff --git a/go/types/api_test.go b/go/types/api_test.go index 9293b24a..8cb955c2 100644 --- a/go/types/api_test.go +++ b/go/types/api_test.go @@ -96,7 +96,7 @@ func TestScopesInfo(t *testing.T) { {`package p`, []string{ "file:", }}, - {`package p; import ( "fmt"; m "math"; _ "os" )`, []string{ + {`package p; import ( "fmt"; m "math"; _ "os" ); var ( _ = fmt.Println; _ = m.Pi )`, []string{ "file:fmt m", }}, {`package p; func _() {}`, []string{ diff --git a/go/types/call.go b/go/types/call.go index 9fa53fd4..d81e2cf5 100644 --- a/go/types/call.go +++ b/go/types/call.go @@ -187,6 +187,7 @@ func (check *checker) selector(x *operand, e *ast.SelectorExpr) { if ident, ok := e.X.(*ast.Ident); ok { if pkg, _ := check.topScope.LookupParent(ident.Name).(*PkgName); pkg != nil { check.recordObject(ident, pkg) + pkg.used = true exp := pkg.pkg.scope.Lookup(sel) if exp == nil { if !pkg.pkg.fake { @@ -196,7 +197,7 @@ func (check *checker) selector(x *operand, e *ast.SelectorExpr) { } if !exp.IsExported() { check.errorf(e.Pos(), "%s not exported by package %s", sel, ident) - goto Error + // ok to continue } check.recordSelection(e, PackageObj, nil, exp, nil, false) // Simplified version of the code for *ast.Idents: diff --git a/go/types/check.go b/go/types/check.go index a1bbbcc3..d32363dc 100644 --- a/go/types/check.go +++ b/go/types/check.go @@ -49,6 +49,7 @@ type checker struct { // functions funcList []funcInfo // list of functions/methods with correct signatures and non-empty bodies funcSig *Signature // signature of currently type-checked function + labels *Scope // label scope of currently type-checked function; lazily allocated // debugging indent int // indentation for tracing diff --git a/go/types/check_test.go b/go/types/check_test.go index 596aecf3..0a58589e 100644 --- a/go/types/check_test.go +++ b/go/types/check_test.go @@ -45,6 +45,7 @@ var ( // Each tests entry is list of files belonging to the same package. var tests = [][]string{ + {"testdata/importdecl0a.src", "testdata/importdecl0b.src"}, {"testdata/cycles.src"}, {"testdata/decls0.src"}, {"testdata/decls1.src"}, diff --git a/go/types/eval_test.go b/go/types/eval_test.go index c75c78c3..90a6deee 100644 --- a/go/types/eval_test.go +++ b/go/types/eval_test.go @@ -87,6 +87,8 @@ import m "math" const c = 3.0 type T []int func f(a int, s string) float64 { + fmt.Println("calling f") + _ = m.Pi // use package math const d int = c + 1 var x int x = a + len(s) diff --git a/go/types/objects.go b/go/types/objects.go index a8bb549d..7d55f866 100644 --- a/go/types/objects.go +++ b/go/types/objects.go @@ -123,10 +123,12 @@ func (obj *object) sameId(pkg *Package, name string) bool { // A PkgName represents an imported Go package. type PkgName struct { object + + used bool } func NewPkgName(pos token.Pos, pkg *Package, name string) *PkgName { - return &PkgName{object{nil, pos, pkg, name, Typ[Invalid]}} + return &PkgName{object: object{nil, pos, pkg, name, Typ[Invalid]}} } func (obj *PkgName) String() string { return obj.toString("package", nil) } @@ -236,10 +238,12 @@ func (obj *Func) String() string { // A Label represents a declared label. type Label struct { object + + used bool } func NewLabel(pos token.Pos, name string) *Label { - return &Label{object{nil, pos, nil, name, nil}} + return &Label{object: object{nil, pos, nil, name, nil}} } func (obj *Label) String() string { return fmt.Sprintf("label %s", obj.Name()) } diff --git a/go/types/resolver.go b/go/types/resolver.go index b2c54044..c1169751 100644 --- a/go/types/resolver.go +++ b/go/types/resolver.go @@ -114,13 +114,13 @@ func (check *checker) resolveFiles(files []*ast.File) { pkg := check.pkg // Phase 1: Pre-declare all package-level objects so that they can be found - // independent of source order. Collect methods for a later phase. + // independent of source order. Associate methods with receiver + // base type names. var ( - scopes []*Scope // corresponding file scope per file - scope *Scope // current file scope - objList []Object // list of package-level objects to type-check - objMap = make(map[Object]declInfo) // declaration info for each package-level object + fileScope *Scope // current file scope + objList []Object // list of package-level objects to type-check + objMap = make(map[Object]declInfo) // declaration info for each package-level object ) // declare declares obj in the package scope, records its ident -> obj mapping, @@ -137,7 +137,7 @@ func (check *checker) resolveFiles(files []*ast.File) { check.declareObj(pkg.scope, ident, obj) objList = append(objList, obj) - objMap[obj] = declInfo{scope, typ, init, nil} + objMap[obj] = declInfo{fileScope, typ, init, nil} } importer := check.conf.Import @@ -148,14 +148,15 @@ func (check *checker) resolveFiles(files []*ast.File) { // only add packages to pkg.imports that have not been seen already seen := make(map[*Package]bool) + var fileScopes []*Scope // list of file scopes for _, file := range files { // The package identifier denotes the current package, // but there is no corresponding package object. check.recordObject(file.Name, nil) - scope = NewScope(pkg.scope) - check.recordScope(file, scope) - scopes = append(scopes, scope) + fileScope = NewScope(pkg.scope) + check.recordScope(file, fileScope) + fileScopes = append(fileScopes, fileScope) for _, decl := range file.Decls { switch d := decl.(type) { @@ -223,13 +224,13 @@ func (check *checker) resolveFiles(files []*ast.File) { if obj.IsExported() { // Note: This will change each imported object's scope! // May be an issue for type aliases. - check.declareObj(scope, nil, obj) + check.declareObj(fileScope, nil, obj) check.recordImplicit(s, obj) } } } else { // declare imported package object in file scope - check.declareObj(scope, nil, obj) + check.declareObj(fileScope, nil, obj) } case *ast.ValueSpec: @@ -332,7 +333,7 @@ func (check *checker) resolveFiles(files []*ast.File) { } } objList = append(objList, obj) - objMap[obj] = declInfo{scope, nil, nil, d} + objMap[obj] = declInfo{fileScope, nil, nil, d} default: check.invalidAST(d.Pos(), "unknown ast.Decl node %T", d) @@ -342,7 +343,7 @@ func (check *checker) resolveFiles(files []*ast.File) { // Phase 2: Verify that objects in package and file scopes have different names. - for _, scope := range scopes { + for _, scope := range fileScopes { for _, obj := range scope.elems { if alt := pkg.scope.Lookup(obj.Name()); alt != nil { check.errorf(alt.Pos(), "%s already declared in this file through import of package %s", obj.Name(), obj.Pkg().Name()) @@ -384,16 +385,43 @@ func (check *checker) resolveFiles(files []*ast.File) { check.topScope = f.sig.scope // open function scope check.funcSig = f.sig + check.labels = nil // lazily allocated check.stmtList(f.body.List, false) + if f.sig.results.Len() > 0 && !check.isTerminating(f.body, "") { check.errorf(f.body.Rbrace, "missing return") } + + // spec: "It is illegal to define a label that is never used." + if check.labels != nil { + for _, obj := range check.labels.elems { + if l := obj.(*Label); !l.used { + check.errorf(l.pos, "%s defined but not used", l.name) + } + } + } } // Phase 5: Check for declared but not used packages and variables. // Note: must happen after checking all functions because closures may affect outer scopes - // Each set of implicitly declared lhs variables of a type switch acts collectively + // spec: "It is illegal (...) to directly import a package without referring to + // any of its exported identifiers. To import a package solely for its side-effects + // (initialization), use the blank identifier as explicit package name." + for _, scope := range fileScopes { + // Unused "blank imports" are automatically ignored + // since _ identifiers are not entered into scopes. + for _, obj := range scope.elems { + if p, _ := obj.(*PkgName); p != nil && !p.used { + check.errorf(p.pos, "%q imported but not used", p.pkg.path) + } + // TODO(gri) dot-imports are not handled for now since we don't + // have mapping from Object to corresponding PkgName through + // which the object was imported. + } + } + + // Each set of implicitly declared lhs variables in a type switch acts collectively // as a single lhs variable. If any one was 'used', all of them are 'used'. Handle // them before the general analysis. for _, vars := range check.lhsVarsList { @@ -411,9 +439,9 @@ func (check *checker) resolveFiles(files []*ast.File) { } } + // spec: "Implementation restriction: A compiler may make it illegal to + // declare a variable inside a function body if the variable is never used." for _, f := range check.funcList { - // spec: "Implementation restriction: A compiler may make it illegal to - // declare a variable inside a function body if the variable is never used." check.usage(f.sig.scope) } } diff --git a/go/types/stdlib_test.go b/go/types/stdlib_test.go index e971d08d..f718debf 100644 --- a/go/types/stdlib_test.go +++ b/go/types/stdlib_test.go @@ -116,7 +116,6 @@ func TestStdfixed(t *testing.T) { "bug223.go", "bug413.go", "bug459.go", // TODO(gri) complete initialization checks "bug248.go", "bug302.go", "bug369.go", // complex test instructions - ignore "bug250.go", // TODO(gri) fix recursive interfaces - "bug373.go", // TODO(gri) implement use checks "bug376.go", // TODO(gri) built-ins must be called (no built-in function expressions) "issue3924.go", // TODO(gri) && and || produce bool result (not untyped bool) "issue4847.go", // TODO(gri) initialization cycle error not found diff --git a/go/types/stmt.go b/go/types/stmt.go index 0315f866..efeede87 100644 --- a/go/types/stmt.go +++ b/go/types/stmt.go @@ -63,6 +63,14 @@ func (check *checker) closeScope() { check.topScope = check.topScope.Parent() } +func assignOp(op token.Token) token.Token { + // token_test.go verifies the token ordering this function relies on + if token.ADD_ASSIGN <= op && op <= token.AND_NOT_ASSIGN { + return op + (token.ADD - token.ADD_ASSIGN) + } + return token.ILLEGAL +} + // stmt typechecks statement s. func (check *checker) stmt(s ast.Stmt, fallthroughOk bool) { // statements cannot use iota in general @@ -77,13 +85,17 @@ func (check *checker) stmt(s ast.Stmt, fallthroughOk bool) { check.declStmt(s.Decl) case *ast.LabeledStmt: - scope := check.funcSig.labels + scope := check.labels if scope == nil { scope = NewScope(nil) // no label scope chain - check.funcSig.labels = scope + check.labels = scope } label := s.Label - check.declareObj(scope, label, NewLabel(label.Pos(), label.Name)) + l := NewLabel(label.Pos(), label.Name) + // Labels are not resolved yet - mark them as used to avoid errors. + // TODO(gri) fix this + l.used = true + check.declareObj(scope, label, l) check.stmt(s.Stmt, fallthroughOk) case *ast.ExprStmt: @@ -169,32 +181,8 @@ func (check *checker) stmt(s ast.Stmt, fallthroughOk bool) { check.errorf(s.TokPos, "assignment operation %s requires single-valued expressions", s.Tok) return } - // TODO(gri) make this conversion more efficient - var op token.Token - switch s.Tok { - case token.ADD_ASSIGN: - op = token.ADD - case token.SUB_ASSIGN: - op = token.SUB - case token.MUL_ASSIGN: - op = token.MUL - case token.QUO_ASSIGN: - op = token.QUO - case token.REM_ASSIGN: - op = token.REM - case token.AND_ASSIGN: - op = token.AND - case token.OR_ASSIGN: - op = token.OR - case token.XOR_ASSIGN: - op = token.XOR - case token.SHL_ASSIGN: - op = token.SHL - case token.SHR_ASSIGN: - op = token.SHR - case token.AND_NOT_ASSIGN: - op = token.AND_NOT - default: + op := assignOp(s.Tok) + if op == token.ILLEGAL { check.invalidAST(s.TokPos, "unknown assignment operation %s", s.Tok) return } diff --git a/go/types/testdata/decls0.src b/go/types/testdata/decls0.src index b2e3b928..55a4b0e1 100644 --- a/go/types/testdata/decls0.src +++ b/go/types/testdata/decls0.src @@ -6,23 +6,7 @@ package decls0 -import ( - "unsafe" - // we can have multiple blank imports (was bug) - _ "math" - _ "net/rpc" - // reflect defines a type "flag" which shows up in the gc export data - "reflect" - . "reflect" - init /* ERROR "cannot declare init" */ "fmt" -) - -// reflect.flag must not be visible in this package -type flag int -type _ reflect /* ERROR "not exported" */ .flag - -// dot-imported exported objects may conflict with local objects -type Value /* ERROR "already declared in this file" */ struct{} +import "unsafe" const pi = 3.1415 diff --git a/go/types/testdata/importdecl0a.src b/go/types/testdata/importdecl0a.src new file mode 100644 index 00000000..db87ee5c --- /dev/null +++ b/go/types/testdata/importdecl0a.src @@ -0,0 +1,43 @@ +// 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. + +package importdecl0 + +import () + +import ( + // we can have multiple blank imports (was bug) + _ "math" + _ "net/rpc" + init /* ERROR "cannot declare init" */ "fmt" + // reflect defines a type "flag" which shows up in the gc export data + "reflect" + . "reflect" +) + +import "math" /* ERROR "imported but not used" */ +import m /* ERROR "imported but not used" */ "math" +import _ "math" + +import ( + "math/big" /* ERROR "imported but not used" */ + b /* ERROR "imported but not used" */ "math/big" + _ "math/big" +) + +import "fmt" +import f "fmt" + +// reflect.flag must not be visible in this package +type flag int +type _ reflect /* ERROR "not exported" */ .flag + +// dot-imported exported objects may conflict with local objects +type Value /* ERROR "already declared in this file" */ struct{} + +var _ = fmt.Println // use "fmt" + +func _() { + f.Println() // use "fmt" +} diff --git a/go/types/testdata/importdecl0b.src b/go/types/testdata/importdecl0b.src new file mode 100644 index 00000000..f5fcd368 --- /dev/null +++ b/go/types/testdata/importdecl0b.src @@ -0,0 +1,12 @@ +// 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. + +package importdecl0 + +import "math" +import m "math" + +// using "math" in this file doesn't affect its use in other files +const Pi0 = math.Pi +const Pi1 = m.Pi diff --git a/go/types/token_test.go b/go/types/token_test.go new file mode 100644 index 00000000..705bb295 --- /dev/null +++ b/go/types/token_test.go @@ -0,0 +1,47 @@ +// 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 checks invariants of token.Token ordering that we rely on +// since package go/token doesn't provide any guarantees at the moment. + +package types + +import ( + "go/token" + "testing" +) + +var assignOps = map[token.Token]token.Token{ + token.ADD_ASSIGN: token.ADD, + token.SUB_ASSIGN: token.SUB, + token.MUL_ASSIGN: token.MUL, + token.QUO_ASSIGN: token.QUO, + token.REM_ASSIGN: token.REM, + token.AND_ASSIGN: token.AND, + token.OR_ASSIGN: token.OR, + token.XOR_ASSIGN: token.XOR, + token.SHL_ASSIGN: token.SHL, + token.SHR_ASSIGN: token.SHR, + token.AND_NOT_ASSIGN: token.AND_NOT, +} + +func TestZeroTok(t *testing.T) { + // zero value for token.Token must be token.ILLEGAL + var zero token.Token + if token.ILLEGAL != zero { + t.Errorf("%s == %d; want 0", token.ILLEGAL, zero) + } +} + +func TestAssignOp(t *testing.T) { + // there are fewer than 256 tokens + for i := 0; i < 256; i++ { + tok := token.Token(i) + got := assignOp(tok) + want := assignOps[tok] + if got != want { + t.Errorf("for assignOp(%s): got %s; want %s", tok, got, want) + } + } +} diff --git a/go/types/types.go b/go/types/types.go index 10aaf96c..9144295d 100644 --- a/go/types/types.go +++ b/go/types/types.go @@ -194,7 +194,6 @@ func (t *Tuple) At(i int) *Var { return t.vars[i] } // A Signature represents a (non-builtin) function or method type. type Signature struct { scope *Scope // function scope, always present - labels *Scope // label scope, or nil (lazily allocated) recv *Var // nil if not a method params *Tuple // (incoming) parameters from left to right; or nil results *Tuple // (outgoing) results from left to right; or nil @@ -217,7 +216,7 @@ func NewSignature(scope *Scope, recv *Var, params, results *Tuple, isVariadic bo panic("types.NewSignature: variadic parameter must be of unnamed slice type") } } - return &Signature{scope, nil, recv, params, results, isVariadic} + return &Signature{scope, recv, params, results, isVariadic} } // Recv returns the receiver of signature s (if a method), or nil if a