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
diff --git a/go/types/assignments.go b/go/types/assignments.go
index b21d4e0..7ee1abc 100644
--- a/go/types/assignments.go
+++ b/go/types/assignments.go
@@ -161,7 +161,7 @@
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 7882604..a392d91 100644
--- a/go/types/call.go
+++ b/go/types/call.go
@@ -266,7 +266,9 @@
// 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 c527ca3..4c21984 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 @@
// 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 @@
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 (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 @@
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 @@
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 e16e52e..dddae30 100644
--- a/go/types/check_test.go
+++ b/go/types/check_test.go
@@ -53,6 +53,7 @@
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 6865d74..37b3fc4 100644
--- a/go/types/object.go
+++ b/go/types/object.go
@@ -37,9 +37,6 @@
// 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 @@
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) 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 @@
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 @@
// 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 @@
}
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 @@
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 @@
// 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 db4f7db..690c44f 100644
--- a/go/types/resolver.go
+++ b/go/types/resolver.go
@@ -113,6 +113,15 @@
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 @@
// 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 @@
}
// 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 @@
}
// 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 @@
// 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 @@
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 5b128e2..df5a482 100644
--- a/go/types/return.go
+++ b/go/types/return.go
@@ -31,7 +31,7 @@
// 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 f33cc68..8ab0f64 100644
--- a/go/types/scope.go
+++ b/go/types/scope.go
@@ -71,14 +71,19 @@
// 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 0539837..9b96436 100644
--- a/go/types/stmt.go
+++ b/go/types/stmt.go
@@ -329,7 +329,7 @@
// 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 0000000..8301820
--- /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 0000000..f24bb9a
--- /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 f47b8c8..d9a4f20 100644
--- a/go/types/typexpr.go
+++ b/go/types/typexpr.go
@@ -23,7 +23,7 @@
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 @@
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 @@
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 @@
}
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 @@
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:
