go.tools/go/types: don't change dot-imported object's parents

With this CL, an Object.Parent() Scope is always the scope in
which the object was originally declared. For dot-imported
objects, that is the package scope of the package from which
the objects are imported (not the file scope into which they
are imported).

Also:

- Changed Scope.Insert to be agnostic regarding blank
identifiers - blank identifiers must be handled outside.

- Fixed handling of blank labels: they are never declared.

Fixes golang/go#7537.

LGTM=adonovan
R=adonovan
CC=golang-codereviews
https://golang.org/cl/75570043

go/types/api.go

@@ -174,10 +174,15 @@ type Info struct {
 	// Selections maps selector expressions to their corresponding selections.
 	Selections map[*ast.SelectorExpr]*Selection
 
-	// Scopes maps ast.Nodes to the scopes they define. Note that package scopes
-	// are not associated with a specific node but with all files belonging to a
-	// package. Thus, the package scope can be found in the type-checked package
-	// object.
+	// Scopes maps ast.Nodes to the scopes they define. Package scopes are not
+	// associated with a specific node but with all files belonging to a package.
+	// Thus, the package scope can be found in the type-checked Package object.
+	// Scopes nest, with the Universe scope being the outermost scope, enclosing
+	// the package scope, which contains (one or more) files scopes, which enclose
+	// function scopes which in turn enclose statement and function literal scopes.
+	// Note that even though package-level functions are declared in the package
+	// scope, the function scopes are embedded in the file scope of the file
+	// containing the function declaration.
 	//
 	// The following node types may appear in Scopes:
 	//

go/types/assignments.go

@@ -277,7 +277,8 @@ func (check *checker) shortVarDecl(pos token.Pos, lhs, rhs []ast.Expr) {
 		if ident, _ := lhs.(*ast.Ident); ident != nil {
 			// Use the correct obj if the ident is redeclared. The
 			// variable's scope starts after the declaration; so we
-			// must use Scope.Lookup here and call Scope.Insert later.
+			// must use Scope.Lookup here and call Scope.Insert
+			// (via check.declare) later.
 			name := ident.Name
 			if alt := scope.Lookup(name); alt != nil {
 				// redeclared object must be a variable

go/types/decl.go

@@ -21,10 +21,16 @@ func (check *checker) reportAltDecl(obj Object) {
 }
 
 func (check *checker) declare(scope *Scope, id *ast.Ident, obj Object) {
-	if alt := scope.Insert(obj); alt != nil {
-		check.errorf(obj.Pos(), "%s redeclared in this block", obj.Name())
-		check.reportAltDecl(alt)
-		return
+	// spec: "The blank identifier, represented by the underscore
+	// character _, may be used in a declaration like any other
+	// identifier but the declaration does not introduce a new
+	// binding."
+	if obj.Name() != "_" {
+		if alt := scope.Insert(obj); alt != nil {
+			check.errorf(obj.Pos(), "%s redeclared in this block", obj.Name())
+			check.reportAltDecl(alt)
+			return
+		}
 	}
 	if id != nil {
 		check.recordDef(id, obj)

go/types/labels.go

@@ -129,41 +129,42 @@ func (check *checker) blockBranches(all *Scope, parent *block, lstmt *ast.Labele
 			}
 
 		case *ast.LabeledStmt:
-			// declare label
-			name := s.Label.Name
-			lbl := NewLabel(s.Label.Pos(), name)
-			if alt := all.Insert(lbl); alt != nil {
-				check.errorf(lbl.pos, "label %s already declared", name)
-				check.reportAltDecl(alt)
-				// ok to continue
-			} else {
-				b.insert(s)
-				check.recordDef(s.Label, lbl)
-			}
-			// resolve matching forward jumps and remove them from fwdJumps
-			i := 0
-			for _, jmp := range fwdJumps {
-				if jmp.Label.Name == name {
-					// match
-					lbl.used = true
-					check.recordUse(jmp.Label, lbl)
-					if jumpsOverVarDecl(jmp) {
-						check.errorf(
-							jmp.Label.Pos(),
-							"goto %s jumps over variable declaration at line %d",
-							name,
-							check.fset.Position(varDeclPos).Line,
-						)
-						// ok to continue
-					}
+			// declare non-blank label
+			if name := s.Label.Name; name != "_" {
+				lbl := NewLabel(s.Label.Pos(), name)
+				if alt := all.Insert(lbl); alt != nil {
+					check.softErrorf(lbl.pos, "label %s already declared", name)
+					check.reportAltDecl(alt)
+					// ok to continue
 				} else {
-					// no match - record new forward jump
-					fwdJumps[i] = jmp
-					i++
+					b.insert(s)
+					check.recordDef(s.Label, lbl)
 				}
+				// resolve matching forward jumps and remove them from fwdJumps
+				i := 0
+				for _, jmp := range fwdJumps {
+					if jmp.Label.Name == name {
+						// match
+						lbl.used = true
+						check.recordUse(jmp.Label, lbl)
+						if jumpsOverVarDecl(jmp) {
+							check.softErrorf(
+								jmp.Label.Pos(),
+								"goto %s jumps over variable declaration at line %d",
+								name,
+								check.fset.Position(varDeclPos).Line,
+							)
+							// ok to continue
+						}
+					} else {
+						// no match - record new forward jump
+						fwdJumps[i] = jmp
+						i++
+					}
+				}
+				fwdJumps = fwdJumps[:i]
+				lstmt = s
 			}
-			fwdJumps = fwdJumps[:i]
-			lstmt = s
 			stmtBranches(s.Stmt)
 
 		case *ast.BranchStmt:

go/types/resolver.go

@@ -196,8 +196,6 @@ func (check *checker) collectObjects() {
 								// A package scope may contain non-exported objects,
 								// do not import them!
 								if obj.Exported() {
-									// Note: This will change each imported object's scope!
-									//       May be an issue for type aliases.
 									check.declare(fileScope, nil, obj)
 									check.recordImplicit(s, obj)
 								}

go/types/scope.go

@@ -81,24 +81,13 @@ func (s *Scope) LookupParent(name string) Object {
 	return nil
 }
 
-// TODO(gri): Should Insert not be exported?
-
 // Insert attempts to insert an object obj into scope s.
 // If s already contains an alternative object alt with
 // the same name, Insert leaves s unchanged and returns alt.
-// Otherwise it inserts obj, sets the object's scope to
-// s, and returns nil. Objects with blank "_" names are
-// not inserted, but have their parent field set to s.
+// Otherwise it inserts obj, sets the object's parent scope
+// if not already set, and returns nil.
 func (s *Scope) Insert(obj Object) Object {
 	name := obj.Name()
-	// spec: "The blank identifier, represented by the underscore
-	// character _, may be used in a declaration like any other
-	// identifier but the declaration does not introduce a new
-	// binding."
-	if name == "_" {
-		obj.setParent(s)
-		return nil
-	}
 	if alt := s.elems[name]; alt != nil {
 		return alt
 	}
@@ -106,7 +95,9 @@ func (s *Scope) Insert(obj Object) Object {
 		s.elems = make(map[string]Object)
 	}
 	s.elems[name] = obj
-	obj.setParent(s)
+	if obj.Parent() == nil {
+		obj.setParent(s)
+	}
 	return nil
 }
 

go/types/testdata/labels.src

@@ -180,4 +180,28 @@ L3:
 		goto L2
 		goto L3
 	}
-}
+}
+
+// Blank labels are never declared.
+
+func f4() {
+_:
+_: // multiple blank labels are ok
+	goto _ /* ERROR "label _ not declared" */
+}
+
+func f5() {
+_:
+	for {
+		break _ /* ERROR "invalid break label _" */
+		continue _ /* ERROR "invalid continue label _" */
+	}
+}
+
+func f6() {
+_:
+	switch {
+	default:
+		break _ /* ERROR "invalid break label _" */
+	}
+}