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

@@ -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{

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:

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

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"},

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)

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()) }

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
+		fileScope *Scope                      // current file scope
-		scope   *Scope                      // current file scope
+		objList   []Object                    // list of package-level objects to type-check
-		objList []Object                    // list of package-level objects to type-check
+		objMap    = make(map[Object]declInfo) // declaration info for each package-level object
-		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)
+		fileScope = NewScope(pkg.scope)
-		check.recordScope(file, scope)
+		check.recordScope(file, fileScope)
-		scopes = append(scopes, scope)
+		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)
 	}
 }

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

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
+			op := assignOp(s.Tok)
-			var op token.Token
+			if op == token.ILLEGAL {
-			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:
 				check.invalidAST(s.TokPos, "unknown assignment operation %s", s.Tok)
 				return
 			}

go/types/testdata/decls0.src

@@ -6,23 +6,7 @@
 
 package decls0
 
-import (
+import "unsafe"
-	"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{}
 
 const pi = 3.1415
 

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"
+}

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

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)
+		}
+	}
+}

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