From b10dfb99c537d8ff190f5c9585e5f112691d1fc5 Mon Sep 17 00:00:00 2001
From: Robert Griesemer <gri@golang.org>
Date: Thu, 23 Oct 2014 09:39:20 -0700
Subject: [PATCH] go/types: don't mark external package objects as used

Also removes a potential race condition regarding the
used flag of Var objects when type-checking packages
concurrently.

Implementation: Rather than marking all used dot-imported
objects and then deduce which corresponding package was used,
now we consider all dot-imported packages as unused and remove
each package from the unused packages map as objects are used.

Now only objects that can be marked as used have a used field
(variables, labels, and packages).

As a result, the code became cleaner and simpler.

Fixes golang/go#8969.

LGTM=adonovan
R=adonovan
CC=golang-codereviews
https://golang.org/cl/163740043
---
 go/types/assignments.go            |  2 +-
 go/types/call.go                   |  4 ++-
 go/types/check.go                  | 39 +++++++++++----------
 go/types/check_test.go             |  1 +
 go/types/object.go                 | 33 +++++++-----------
 go/types/resolver.go               | 54 ++++++++++++++----------------
 go/types/return.go                 |  2 +-
 go/types/scope.go                  | 13 ++++---
 go/types/stmt.go                   |  2 +-
 go/types/testdata/importdecl1a.src | 11 ++++++
 go/types/testdata/importdecl1b.src |  7 ++++
 go/types/typexpr.go                | 24 ++++++-------
 12 files changed, 105 insertions(+), 87 deletions(-)
 create mode 100644 go/types/testdata/importdecl1a.src
 create mode 100644 go/types/testdata/importdecl1b.src

diff --git a/go/types/assignments.go b/go/types/assignments.go
index b21d4e06247..7ee1abcc989 100644
--- a/go/types/assignments.go
+++ b/go/types/assignments.go
@@ -161,7 +161,7 @@ func (check *Checker) assignVar(lhs ast.Expr, x *operand) Type {
 	var v *Var
 	var v_used bool
 	if ident != nil {
-		if obj := check.scope.LookupParent(ident.Name); obj != nil {
+		if _, obj := check.scope.LookupParent(ident.Name); obj != nil {
 			v, _ = obj.(*Var)
 			if v != nil {
 				v_used = v.used
diff --git a/go/types/call.go b/go/types/call.go
index 788260417bf..a392d9106d5 100644
--- a/go/types/call.go
+++ b/go/types/call.go
@@ -266,7 +266,9 @@ func (check *Checker) selector(x *operand, e *ast.SelectorExpr) {
 	// can only appear in qualified identifiers which are mapped to
 	// selector expressions.
 	if ident, ok := e.X.(*ast.Ident); ok {
-		if pkg, _ := check.scope.LookupParent(ident.Name).(*PkgName); pkg != nil {
+		_, obj := check.scope.LookupParent(ident.Name)
+		if pkg, _ := obj.(*PkgName); pkg != nil {
+			assert(pkg.pkg == check.pkg)
 			check.recordUse(ident, pkg)
 			pkg.used = true
 			exp := pkg.imported.scope.Lookup(sel)
diff --git a/go/types/check.go b/go/types/check.go
index c527ca3d824..4c21984aa0e 100644
--- a/go/types/check.go
+++ b/go/types/check.go
@@ -7,7 +7,6 @@
 package types
 
 import (
-	"fmt"
 	"go/ast"
 	"go/token"
 
@@ -73,9 +72,8 @@ type Checker struct {
 	// information collected during type-checking of a set of package files
 	// (initialized by Files, valid only for the duration of check.Files;
 	// maps and lists are allocated on demand)
-	files      []*ast.File              // package files
-	fileScopes []*Scope                 // file scope for each file
-	dotImports []map[*Package]token.Pos // positions of dot-imports for each file
+	files            []*ast.File                       // package files
+	unusedDotImports map[*Scope]map[*Package]token.Pos // positions of unused dot-imported packages for each file scope
 
 	firstErr error                 // first error encountered
 	methods  map[string][]*Func    // maps type names to associated methods
@@ -91,6 +89,22 @@ type Checker struct {
 	indent int // indentation for tracing
 }
 
+// addUnusedImport adds the position of a dot-imported package
+// pkg to the map of dot imports for the given file scope.
+func (check *Checker) addUnusedDotImport(scope *Scope, pkg *Package, pos token.Pos) {
+	mm := check.unusedDotImports
+	if mm == nil {
+		mm = make(map[*Scope]map[*Package]token.Pos)
+		check.unusedDotImports = mm
+	}
+	m := mm[scope]
+	if m == nil {
+		m = make(map[*Package]token.Pos)
+		mm[scope] = m
+	}
+	m[pkg] = pos
+}
+
 // addDeclDep adds the dependency edge (check.decl -> to) if check.decl exists
 func (check *Checker) addDeclDep(to Object) {
 	from := check.decl
@@ -161,8 +175,7 @@ func NewChecker(conf *Config, fset *token.FileSet, pkg *Package, info *Info) *Ch
 func (check *Checker) initFiles(files []*ast.File) {
 	// start with a clean slate (check.Files may be called multiple times)
 	check.files = nil
-	check.fileScopes = nil
-	check.dotImports = nil
+	check.unusedDotImports = nil
 
 	check.firstErr = nil
 	check.methods = nil
@@ -170,9 +183,9 @@ func (check *Checker) initFiles(files []*ast.File) {
 	check.funcs = nil
 	check.delayed = nil
 
-	// determine package name, files, and set up file scopes, dotImports maps
+	// determine package name and collect valid files
 	pkg := check.pkg
-	for i, file := range files {
+	for _, file := range files {
 		switch name := file.Name.Name; pkg.name {
 		case "":
 			if name != "_" {
@@ -184,16 +197,6 @@ func (check *Checker) initFiles(files []*ast.File) {
 
 		case name:
 			check.files = append(check.files, file)
-			var comment string
-			if pos := file.Pos(); pos.IsValid() {
-				comment = "file " + check.fset.File(pos).Name()
-			} else {
-				comment = fmt.Sprintf("file[%d]", i)
-			}
-			fileScope := NewScope(pkg.scope, comment)
-			check.recordScope(file, fileScope)
-			check.fileScopes = append(check.fileScopes, fileScope)
-			check.dotImports = append(check.dotImports, nil) // element (map) is lazily allocated
 
 		default:
 			check.errorf(file.Package, "package %s; expected %s", name, pkg.name)
diff --git a/go/types/check_test.go b/go/types/check_test.go
index e16e52e8e44..dddae3038d1 100644
--- a/go/types/check_test.go
+++ b/go/types/check_test.go
@@ -53,6 +53,7 @@ var (
 var tests = [][]string{
 	{"testdata/errors.src"},
 	{"testdata/importdecl0a.src", "testdata/importdecl0b.src"},
+	{"testdata/importdecl1a.src", "testdata/importdecl1b.src"},
 	{"testdata/cycles.src"},
 	{"testdata/cycles1.src"},
 	{"testdata/cycles2.src"},
diff --git a/go/types/object.go b/go/types/object.go
index 6865d74a9fe..37b3fc45c3d 100644
--- a/go/types/object.go
+++ b/go/types/object.go
@@ -37,9 +37,6 @@ type Object interface {
 	// 0 for all other objects (including objects in file scopes).
 	order() uint32
 
-	// isUsed reports whether the object was marked as 'used'.
-	isUsed() bool
-
 	// setOrder sets the order number of the object. It must be > 0.
 	setOrder(uint32)
 
@@ -82,9 +79,7 @@ type object struct {
 	pkg    *Package
 	name   string
 	typ    Type
-
 	order_ uint32
-	used   bool
 }
 
 func (obj *object) Parent() *Scope { return obj.parent }
@@ -95,9 +90,7 @@ func (obj *object) Type() Type     { return obj.typ }
 func (obj *object) Exported() bool { return ast.IsExported(obj.name) }
 func (obj *object) Id() string     { return Id(obj.pkg, obj.name) }
 func (obj *object) String() string { panic("abstract") }
-
-func (obj *object) order() uint32 { return obj.order_ }
-func (obj *object) isUsed() bool  { return obj.used }
+func (obj *object) order() uint32  { return obj.order_ }
 
 func (obj *object) setOrder(order uint32)   { assert(order > 0); obj.order_ = order }
 func (obj *object) setParent(parent *Scope) { obj.parent = parent }
@@ -128,10 +121,11 @@ func (obj *object) sameId(pkg *Package, name string) bool {
 type PkgName struct {
 	object
 	imported *Package
+	used     bool // set if the package was used
 }
 
 func NewPkgName(pos token.Pos, pkg *Package, name string, imported *Package) *PkgName {
-	return &PkgName{object{nil, pos, pkg, name, Typ[Invalid], 0, false}, imported}
+	return &PkgName{object{nil, pos, pkg, name, Typ[Invalid], 0}, imported, false}
 }
 
 // Imported returns the package that was imported.
@@ -141,13 +135,12 @@ func (obj *PkgName) Imported() *Package { return obj.imported }
 // A Const represents a declared constant.
 type Const struct {
 	object
-	val exact.Value
-
+	val     exact.Value
 	visited bool // for initialization cycle detection
 }
 
 func NewConst(pos token.Pos, pkg *Package, name string, typ Type, val exact.Value) *Const {
-	return &Const{object: object{nil, pos, pkg, name, typ, 0, false}, val: val}
+	return &Const{object{nil, pos, pkg, name, typ, 0}, val, false}
 }
 
 func (obj *Const) Val() exact.Value { return obj.val }
@@ -158,28 +151,28 @@ type TypeName struct {
 }
 
 func NewTypeName(pos token.Pos, pkg *Package, name string, typ Type) *TypeName {
-	return &TypeName{object{nil, pos, pkg, name, typ, 0, false}}
+	return &TypeName{object{nil, pos, pkg, name, typ, 0}}
 }
 
 // A Variable represents a declared variable (including function parameters and results, and struct fields).
 type Var struct {
 	object
-
 	anonymous bool // if set, the variable is an anonymous struct field, and name is the type name
 	visited   bool // for initialization cycle detection
 	isField   bool // var is struct field
+	used      bool // set if the variable was used
 }
 
 func NewVar(pos token.Pos, pkg *Package, name string, typ Type) *Var {
-	return &Var{object: object{nil, pos, pkg, name, typ, 0, false}}
+	return &Var{object: object{nil, pos, pkg, name, typ, 0}}
 }
 
 func NewParam(pos token.Pos, pkg *Package, name string, typ Type) *Var {
-	return &Var{object: object{nil, pos, pkg, name, typ, 0, true}} // parameters are always 'used'
+	return &Var{object: object{nil, pos, pkg, name, typ, 0}, used: true} // parameters are always 'used'
 }
 
 func NewField(pos token.Pos, pkg *Package, name string, typ Type, anonymous bool) *Var {
-	return &Var{object: object{nil, pos, pkg, name, typ, 0, false}, anonymous: anonymous, isField: true}
+	return &Var{object: object{nil, pos, pkg, name, typ, 0}, anonymous: anonymous, isField: true}
 }
 
 func (obj *Var) Anonymous() bool { return obj.anonymous }
@@ -199,7 +192,7 @@ func NewFunc(pos token.Pos, pkg *Package, name string, sig *Signature) *Func {
 	if sig != nil {
 		typ = sig
 	}
-	return &Func{object{nil, pos, pkg, name, typ, 0, false}}
+	return &Func{object{nil, pos, pkg, name, typ, 0}}
 }
 
 // FullName returns the package- or receiver-type-qualified name of
@@ -217,17 +210,17 @@ func (obj *Func) Scope() *Scope {
 // A Label represents a declared label.
 type Label struct {
 	object
+	used bool // set if the label was used
 }
 
 func NewLabel(pos token.Pos, pkg *Package, name string) *Label {
-	return &Label{object{pos: pos, pkg: pkg, name: name, typ: Typ[Invalid]}}
+	return &Label{object{pos: pos, pkg: pkg, name: name, typ: Typ[Invalid]}, false}
 }
 
 // A Builtin represents a built-in function.
 // Builtins don't have a valid type.
 type Builtin struct {
 	object
-
 	id builtinId
 }
 
diff --git a/go/types/resolver.go b/go/types/resolver.go
index db4f7dbb2c4..690c44fa5f5 100644
--- a/go/types/resolver.go
+++ b/go/types/resolver.go
@@ -113,6 +113,15 @@ func (check *Checker) declarePkgObj(ident *ast.Ident, obj Object, d *declInfo) {
 	obj.setOrder(uint32(len(check.objMap)))
 }
 
+// filename returns a filename suitable for debugging output.
+func (check *Checker) filename(fileNo int) string {
+	file := check.files[fileNo]
+	if pos := file.Pos(); pos.IsValid() {
+		return check.fset.File(pos).Name()
+	}
+	return fmt.Sprintf("file[%d]", fileNo)
+}
+
 // collectObjects collects all file and package objects and inserts them
 // into their respective scopes. It also performs imports and associates
 // methods with receiver base type names.
@@ -139,7 +148,9 @@ func (check *Checker) collectObjects() {
 		// The package identifier denotes the current package,
 		// but there is no corresponding package object.
 		check.recordDef(file.Name, nil)
-		fileScope := check.fileScopes[fileNo]
+
+		fileScope := NewScope(check.pkg.scope, check.filename(fileNo))
+		check.recordScope(file, fileScope)
 
 		for _, decl := range file.Decls {
 			switch d := decl.(type) {
@@ -223,12 +234,7 @@ func (check *Checker) collectObjects() {
 							}
 							// add position to set of dot-import positions for this file
 							// (this is only needed for "imported but not used" errors)
-							posSet := check.dotImports[fileNo]
-							if posSet == nil {
-								posSet = make(map[*Package]token.Pos)
-								check.dotImports[fileNo] = posSet
-							}
-							posSet[imp] = s.Pos()
+							check.addUnusedDotImport(fileScope, imp, s.Pos())
 						} else {
 							// declare imported package object in file scope
 							check.declare(fileScope, nil, obj)
@@ -351,7 +357,7 @@ func (check *Checker) collectObjects() {
 	}
 
 	// verify that objects in package and file scopes have different names
-	for _, scope := range check.fileScopes {
+	for _, scope := range check.pkg.scope.children /* file scopes */ {
 		for _, obj := range scope.elems {
 			if alt := pkg.scope.Lookup(obj.Name()); alt != nil {
 				if pkg, ok := obj.(*PkgName); ok {
@@ -407,11 +413,11 @@ func (check *Checker) unusedImports() {
 	// 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 i, scope := range check.fileScopes {
-		var usedDotImports map[*Package]bool // lazily allocated
+
+	// check use of regular imported packages
+	for _, scope := range check.pkg.scope.children /* file scopes */ {
 		for _, obj := range scope.elems {
-			switch obj := obj.(type) {
-			case *PkgName:
+			if obj, ok := obj.(*PkgName); ok {
 				// Unused "blank imports" are automatically ignored
 				// since _ identifiers are not entered into scopes.
 				if !obj.used {
@@ -423,24 +429,14 @@ func (check *Checker) unusedImports() {
 						check.softErrorf(obj.pos, "%q imported but not used as %s", path, obj.name)
 					}
 				}
-			default:
-				// All other objects in the file scope must be dot-
-				// imported. If an object was used, mark its package
-				// as used.
-				if obj.isUsed() {
-					if usedDotImports == nil {
-						usedDotImports = make(map[*Package]bool)
-					}
-					usedDotImports[obj.Pkg()] = true
-				}
-			}
-		}
-		// Iterate through all dot-imports for this file and
-		// check if the corresponding package was used.
-		for pkg, pos := range check.dotImports[i] {
-			if !usedDotImports[pkg] {
-				check.softErrorf(pos, "%q imported but not used", pkg.path)
 			}
 		}
 	}
+
+	// check use of dot-imported packages
+	for _, unusedDotImports := range check.unusedDotImports {
+		for pkg, pos := range unusedDotImports {
+			check.softErrorf(pos, "%q imported but not used", pkg.path)
+		}
+	}
 }
diff --git a/go/types/return.go b/go/types/return.go
index 5b128e2a081..df5a482ad44 100644
--- a/go/types/return.go
+++ b/go/types/return.go
@@ -31,7 +31,7 @@ func (check *Checker) isTerminating(s ast.Stmt, label string) bool {
 		// the predeclared (possibly parenthesized) panic() function is terminating
 		if call, _ := unparen(s.X).(*ast.CallExpr); call != nil {
 			if id, _ := call.Fun.(*ast.Ident); id != nil {
-				if obj := check.scope.LookupParent(id.Name); obj != nil {
+				if _, obj := check.scope.LookupParent(id.Name); obj != nil {
 					if b, _ := obj.(*Builtin); b != nil && b.id == _Panic {
 						return true
 					}
diff --git a/go/types/scope.go b/go/types/scope.go
index f33cc68c5e4..8ab0f64feb8 100644
--- a/go/types/scope.go
+++ b/go/types/scope.go
@@ -71,14 +71,19 @@ func (s *Scope) Lookup(name string) Object {
 
 // LookupParent follows the parent chain of scopes starting with s until
 // it finds a scope where Lookup(name) returns a non-nil object, and then
-// returns that object. If no such scope exists, the result is nil.
-func (s *Scope) LookupParent(name string) Object {
+// returns that scope and object. If no such scope exists, the result is (nil, nil).
+//
+// Note that obj.Parent() may be different from the returned scope if the
+// object was inserted into the scope and already had a parent at that
+// time (see Insert, below). This can only happen for dot-imported objects
+// whose scope is the scope of the package that exported them.
+func (s *Scope) LookupParent(name string) (*Scope, Object) {
 	for ; s != nil; s = s.parent {
 		if obj := s.elems[name]; obj != nil {
-			return obj
+			return s, obj
 		}
 	}
-	return nil
+	return nil, nil
 }
 
 // Insert attempts to insert an object obj into scope s.
diff --git a/go/types/stmt.go b/go/types/stmt.go
index 0539837e23b..9b96436b8f5 100644
--- a/go/types/stmt.go
+++ b/go/types/stmt.go
@@ -329,7 +329,7 @@ func (check *Checker) stmt(ctxt stmtContext, s ast.Stmt) {
 				// list in a "return" statement if a different entity (constant, type, or variable)
 				// with the same name as a result parameter is in scope at the place of the return."
 				for _, obj := range res.vars {
-					if alt := check.scope.LookupParent(obj.name); alt != nil && alt != obj {
+					if _, alt := check.scope.LookupParent(obj.name); alt != nil && alt != obj {
 						check.errorf(s.Pos(), "result parameter %s not in scope at return", obj.name)
 						check.errorf(alt.Pos(), "\tinner declaration of %s", obj)
 						// ok to continue
diff --git a/go/types/testdata/importdecl1a.src b/go/types/testdata/importdecl1a.src
new file mode 100644
index 00000000000..8301820dda9
--- /dev/null
+++ b/go/types/testdata/importdecl1a.src
@@ -0,0 +1,11 @@
+// Copyright 2014 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.
+
+// Test case for issue 8969.
+
+package importdecl1
+
+import . "unsafe"
+
+var _ Pointer // use dot-imported package unsafe
diff --git a/go/types/testdata/importdecl1b.src b/go/types/testdata/importdecl1b.src
new file mode 100644
index 00000000000..f24bb9ade97
--- /dev/null
+++ b/go/types/testdata/importdecl1b.src
@@ -0,0 +1,7 @@
+// Copyright 2014 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 importdecl1
+
+import . /* ERROR "imported but not used" */ "unsafe"
diff --git a/go/types/typexpr.go b/go/types/typexpr.go
index f47b8c85580..d9a4f204098 100644
--- a/go/types/typexpr.go
+++ b/go/types/typexpr.go
@@ -23,7 +23,7 @@ func (check *Checker) ident(x *operand, e *ast.Ident, def *Named, path []*TypeNa
 	x.mode = invalid
 	x.expr = e
 
-	obj := check.scope.LookupParent(e.Name)
+	scope, obj := check.scope.LookupParent(e.Name)
 	if obj == nil {
 		if e.Name == "_" {
 			check.errorf(e.Pos(), "cannot use _ as value or type")
@@ -38,18 +38,20 @@ func (check *Checker) ident(x *operand, e *ast.Ident, def *Named, path []*TypeNa
 	typ := obj.Type()
 	assert(typ != nil)
 
+	// The object may be dot-imported: If so, remove its package from
+	// the map of unused dot imports for the respective file scope.
+	// (This code is only needed for dot-imports. Without them,
+	// we only have to mark variables, see *Var case below).
+	if pkg := obj.Pkg(); pkg != check.pkg && pkg != nil {
+		delete(check.unusedDotImports[scope], pkg)
+	}
+
 	switch obj := obj.(type) {
 	case *PkgName:
 		check.errorf(e.Pos(), "use of package %s not in selector", obj.name)
 		return
 
 	case *Const:
-		// The constant may be dot-imported. Mark it as used so that
-		// later we can determine if the corresponding dot-imported
-		// package was used. Same applies for other objects, below.
-		// (This code is only used for dot-imports. Without them, we
-		// would only have to mark Vars.)
-		obj.used = true
 		check.addDeclDep(obj)
 		if typ == Typ[Invalid] {
 			return
@@ -67,7 +69,6 @@ func (check *Checker) ident(x *operand, e *ast.Ident, def *Named, path []*TypeNa
 		x.mode = constant
 
 	case *TypeName:
-		obj.used = true
 		x.mode = typexpr
 		// check for cycle
 		// (it's ok to iterate forward because each named type appears at most once in path)
@@ -86,7 +87,9 @@ func (check *Checker) ident(x *operand, e *ast.Ident, def *Named, path []*TypeNa
 		}
 
 	case *Var:
-		obj.used = true
+		if obj.pkg == check.pkg {
+			obj.used = true
+		}
 		check.addDeclDep(obj)
 		if typ == Typ[Invalid] {
 			return
@@ -94,17 +97,14 @@ func (check *Checker) ident(x *operand, e *ast.Ident, def *Named, path []*TypeNa
 		x.mode = variable
 
 	case *Func:
-		obj.used = true
 		check.addDeclDep(obj)
 		x.mode = value
 
 	case *Builtin:
-		obj.used = true // for built-ins defined by package unsafe
 		x.id = obj.id
 		x.mode = builtin
 
 	case *Nil:
-		// no need to "use" the nil object
 		x.mode = value
 
 	default: