go.tools/go/types: assignment checking cleanup (round 2)

- consolited remainign assignment check routines
- removed more dead code
- fixed incorrect scope hierarchy in case of errors for some statements
- fixed scope of key iteration variable for range clauses

R=adonovan
CC=golang-dev
https://golang.org/cl/10694044

go/types/api.go

@@ -68,7 +68,9 @@ type Context struct {
 	// is nil if the identifier was not declared. Ident may be called
 	// multiple times for the same identifier (e.g., for typed variable
 	// declarations with multiple initialization statements); but Ident
-	// will report the same obj for a given id in this case.
+	// will report the same obj for a given id in this case. The object
+	// may not be fully set up at the time of the callback (e.g., its type
+	// may be nil and only filled in later).
 	Ident func(id *ast.Ident, obj Object)
 	// TODO(gri) Can we make this stronger, so that Ident is called
 	// always exactly once (w/o resorting to a map, internally)?

go/types/assignments.go

@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
+// This file implements initialization and assignment checks.
+
 package types
 
 import (
@@ -10,6 +12,29 @@ import (
 	"code.google.com/p/go.tools/go/exact"
 )
 
+// assignment reports whether x can be assigned to a variable of type 'to',
+// if necessary by attempting to convert untyped values to the appropriate
+// type. If x.mode == invalid upon return, then assignment has already
+// issued an error message and the caller doesn't have to report another.
+// TODO(gri) This latter behavior is for historic reasons and complicates
+// callers. Needs to be cleaned up.
+func (check *checker) assignment(x *operand, to Type) bool {
+	if x.mode == invalid {
+		return false
+	}
+
+	if t, ok := x.typ.(*Tuple); ok {
+		// TODO(gri) elsewhere we use "assignment count mismatch" (consolidate)
+		check.errorf(x.pos(), "%d-valued expression %s used as single value", t.Len(), x)
+		x.mode = invalid
+		return false
+	}
+
+	check.convertUntyped(x, to)
+
+	return x.mode != invalid && x.isAssignable(check.ctxt, to)
+}
+
 func (check *checker) initConst(lhs *Const, x *operand) {
 	lhs.val = exact.MakeUnknown()
 
@@ -48,9 +73,7 @@ func (check *checker) initConst(lhs *Const, x *operand) {
 }
 
 func (check *checker) initVar(lhs *Var, x *operand) {
-	typ := x.typ
+	if x.mode == invalid || x.typ == Typ[Invalid] || lhs.typ == Typ[Invalid] {
-
-	if x.mode == invalid || typ == Typ[Invalid] || lhs.typ == Typ[Invalid] {
 		if lhs.typ == nil {
 			lhs.typ = Typ[Invalid]
 		}
@@ -59,6 +82,7 @@ func (check *checker) initVar(lhs *Var, x *operand) {
 
 	// If the lhs doesn't have a type yet, use the type of x.
 	if lhs.typ == nil {
+		typ := x.typ
 		if isUntyped(typ) {
 			// convert untyped types to default types
 			if typ == Typ[UntypedNil] {
@@ -78,10 +102,35 @@ func (check *checker) initVar(lhs *Var, x *operand) {
 	}
 }
 
-func invalidateVars(list []*Var) {
+func (check *checker) assignVar(lhs ast.Expr, x *operand) {
-	for _, obj := range list {
+	if x.mode == invalid || x.typ == Typ[Invalid] {
-		if obj.typ == nil {
+		return
-			obj.typ = Typ[Invalid]
+	}
+
+	// Don't evaluate lhs if it is the blank identifier.
+	if ident, _ := lhs.(*ast.Ident); ident != nil && ident.Name == "_" {
+		check.callIdent(ident, nil)
+		check.updateExprType(x.expr, x.typ, true) // rhs has its final type
+		return
+	}
+
+	var z operand
+	check.expr(&z, lhs)
+	if z.mode == invalid || z.typ == Typ[Invalid] {
+		return
+	}
+
+	if z.mode == constant || z.mode == value {
+		check.errorf(z.pos(), "cannot assign to non-variable %s", &z)
+		return
+	}
+
+	// TODO(gri) z.mode can also be valueok which in some cases is ok (maps)
+	// but in others isn't (channels). Complete the checks here.
+
+	if !check.assignment(x, z.typ) {
+		if x.mode != invalid {
+			check.errorf(x.pos(), "cannot assign %s to %s", x, &z)
 		}
 	}
 }
@@ -143,6 +192,57 @@ func (check *checker) initVars(lhs []*Var, rhs []ast.Expr, allowCommaOk bool) {
 	invalidateVars(lhs)
 }
 
+func (check *checker) assignVars(lhs, rhs []ast.Expr) {
+	assert(len(lhs) > 0)
+
+	// If the lhs and rhs have corresponding expressions,
+	// treat each matching pair as an individual pair.
+	if len(lhs) == len(rhs) {
+		var x operand
+		for i, e := range rhs {
+			check.expr(&x, e)
+			check.assignVar(lhs[i], &x)
+		}
+		return
+	}
+
+	// Otherwise, the rhs must be a single expression (possibly
+	// a function call returning multiple values, or a comma-ok
+	// expression).
+	if len(rhs) == 1 {
+		// len(lhs) > 1
+		var x operand
+		check.expr(&x, rhs[0])
+		if x.mode == invalid {
+			return
+		}
+
+		if t, ok := x.typ.(*Tuple); ok && len(lhs) == t.Len() {
+			// function result
+			for i, lhs := range lhs {
+				x.mode = value
+				x.expr = rhs[0]
+				x.typ = t.At(i).typ
+				check.assignVar(lhs, &x)
+			}
+			return
+		}
+
+		if x.mode == valueok && len(lhs) == 2 {
+			// comma-ok expression
+			x.mode = value
+			check.assignVar(lhs[0], &x)
+
+			x.mode = value
+			x.typ = Typ[UntypedBool]
+			check.assignVar(lhs[1], &x)
+			return
+		}
+	}
+
+	check.errorf(rhs[0].Pos(), "assignment count mismatch: %d = %d", len(lhs), len(rhs))
+}
+
 func (check *checker) shortVarDecl(lhs, rhs []ast.Expr) {
 	scope := check.topScope
 
@@ -192,3 +292,11 @@ func (check *checker) shortVarDecl(lhs, rhs []ast.Expr) {
 		check.errorf(vars[0].Pos(), "no new variables on left side of :=")
 	}
 }
+
+func invalidateVars(list []*Var) {
+	for _, obj := range list {
+		if obj.typ == nil {
+			obj.typ = Typ[Invalid]
+		}
+	}
+}

go/types/stmt.go

@@ -11,186 +11,19 @@ import (
 	"go/token"
 )
 
-// assigment reports whether x can be assigned to a variable of type 'to',
-// if necessary by attempting to convert untyped values to the appropriate
-// type. If x.mode == invalid upon return, then assignment has already
-// issued an error message and the caller doesn't have to report another.
-// TODO(gri) This latter behavior is for historic reasons and complicates
-// callers. Needs to be cleaned up.
-func (check *checker) assignment(x *operand, to Type) bool {
-	if x.mode == invalid {
-		return false
-	}
-
-	if t, ok := x.typ.(*Tuple); ok {
-		// TODO(gri) elsewhere we use "assignment count mismatch" (consolidate)
-		check.errorf(x.pos(), "%d-valued expression %s used as single value", t.Len(), x)
-		x.mode = invalid
-		return false
-	}
-
-	check.convertUntyped(x, to)
-
-	return x.mode != invalid && x.isAssignable(check.ctxt, to)
-}
-
-// TODO(gri) assign1to1 is only used in one place now with decl = true.
-// Remove and consolidate with other assignment functions.
-
-// assign1to1 typechecks a single assignment of the form lhs = rhs (if rhs != nil), or
-// lhs = x (if rhs == nil). If decl is set, the lhs expression must be an identifier;
-// if its type is not set, it is deduced from the type of x or set to Typ[Invalid] in
-// case of an error.
-//
-func (check *checker) assign1to1(lhs, rhs ast.Expr, x *operand, decl bool) {
-	// Start with rhs so we have an expression type
-	// for declarations with implicit type.
-	if x == nil {
-		x = new(operand)
-		check.expr(x, rhs)
-		// don't exit for declarations - we need the lhs first
-		if x.mode == invalid && !decl {
-			return
-		}
-	}
-	// x.mode == valid || decl
-
-	// lhs may be an identifier
-	ident, _ := lhs.(*ast.Ident)
-
-	// regular assignment; we know x is valid
-	if !decl {
-		// anything can be assigned to the blank identifier
-		if ident != nil && ident.Name == "_" {
-			check.callIdent(ident, nil)
-			// the rhs has its final type
-			check.updateExprType(rhs, x.typ, true)
-			return
-		}
-
-		var z operand
-		check.expr(&z, lhs)
-		if z.mode == invalid || z.typ == Typ[Invalid] {
-			return
-		}
-
-		// TODO(gri) verify that all other z.mode values
-		//           that may appear here are legal
-		if z.mode == constant || !check.assignment(x, z.typ) {
-			if x.mode != invalid {
-				check.errorf(x.pos(), "cannot assign %s to %s", x, &z)
-			}
-		}
-		return
-	}
-
-	// declaration with initialization; lhs must be an identifier
-	if ident == nil {
-		check.errorf(lhs.Pos(), "cannot declare %s", lhs)
-		return
-	}
-
-	// Determine the type of lhs from the type of x;
-	// if x is invalid, set the object type to Typ[Invalid].
-	obj := NewVar(ident.Pos(), check.pkg, ident.Name, nil)
-	defer check.declare(check.topScope, ident, obj)
-
-	var typ Type = Typ[Invalid]
-	if x.mode != invalid {
-		typ = x.typ
-		if isUntyped(typ) {
-			// convert untyped types to default types
-			if typ == Typ[UntypedNil] {
-				check.errorf(x.pos(), "use of untyped nil")
-				typ = Typ[Invalid]
-			} else {
-				typ = defaultType(typ)
-			}
-		}
-	}
-	obj.typ = typ
-
-	// nothing else to check if we don't have a valid lhs or rhs
-	if typ == Typ[Invalid] || x.mode == invalid {
-		return
-	}
-
-	if !check.assignment(x, typ) {
-		if x.mode != invalid {
-			if x.typ != Typ[Invalid] && typ != Typ[Invalid] {
-				check.errorf(x.pos(), "cannot initialize %s (type %s) with %s", ident.Name, typ, x)
-			}
-		}
-		return
-	}
-}
-
-// TODO(gri) assignNtoM is only used in one place now.
-// Remove and consolidate with other assignment functions.
-
-// assignNtoM typechecks a regular assignment.
-// Precondition: len(lhs) > 0 .
-//
-func (check *checker) assignNtoM(lhs, rhs []ast.Expr) {
-	assert(len(lhs) > 0)
-
-	// If the lhs and rhs have corresponding expressions, treat each
-	// matching pair as an individual pair.
-	if len(lhs) == len(rhs) {
-		for i, e := range rhs {
-			check.assign1to1(lhs[i], e, nil, false)
-		}
-		return
-	}
-
-	// Otherwise, the rhs must be a single expression (possibly
-	// a function call returning multiple values, or a comma-ok
-	// expression).
-	if len(rhs) == 1 {
-		// len(lhs) > 1
-		// Start with rhs so we have expression types
-		// for declarations with implicit types.
-		var x operand
-		check.expr(&x, rhs[0])
-		if x.mode == invalid {
-			return
-		}
-
-		if t, ok := x.typ.(*Tuple); ok && len(lhs) == t.Len() {
-			// function result
-			x.mode = value
-			for i := 0; i < len(lhs); i++ {
-				obj := t.At(i)
-				x.expr = nil // TODO(gri) should do better here
-				x.typ = obj.typ
-				check.assign1to1(lhs[i], nil, &x, false)
-			}
-			return
-		}
-
-		if x.mode == valueok && len(lhs) == 2 {
-			// comma-ok expression
-			x.mode = value
-			check.assign1to1(lhs[0], nil, &x, false)
-
-			x.typ = Typ[UntypedBool]
-			check.assign1to1(lhs[1], nil, &x, false)
-			return
-		}
-	}
-
-	check.errorf(lhs[0].Pos(), "assignment count mismatch: %d = %d", len(lhs), len(rhs))
-}
-
 func (check *checker) optionalStmt(s ast.Stmt) {
 	if s != nil {
+		scope := check.topScope
 		check.stmt(s)
+		assert(check.topScope == scope)
 	}
 }
 
 func (check *checker) stmtList(list []ast.Stmt) {
 	for _, s := range list {
+		scope := check.topScope
 		check.stmt(s)
+		assert(check.topScope == scope)
 	}
 }
 
@@ -317,7 +150,7 @@ func (check *checker) stmt(s ast.Stmt) {
 		if x.mode == invalid {
 			return
 		}
-		check.assign1to1(s.X, nil, &x, false)
+		check.assignVar(s.X, &x)
 
 	case *ast.AssignStmt:
 		switch s.Tok {
@@ -330,7 +163,7 @@ func (check *checker) stmt(s ast.Stmt) {
 				check.shortVarDecl(s.Lhs, s.Rhs)
 			} else {
 				// regular assignment
-				check.assignNtoM(s.Lhs, s.Rhs)
+				check.assignVars(s.Lhs, s.Rhs)
 			}
 
 		default:
@@ -373,7 +206,7 @@ func (check *checker) stmt(s ast.Stmt) {
 			if x.mode == invalid {
 				return
 			}
-			check.assign1to1(s.Lhs[0], nil, &x, false)
+			check.assignVar(s.Lhs[0], &x)
 		}
 
 	case *ast.GoStmt:
@@ -497,6 +330,7 @@ func (check *checker) stmt(s ast.Stmt) {
 
 	case *ast.TypeSwitchStmt:
 		check.openScope(s)
+		defer check.closeScope()
 		check.optionalStmt(s.Init)
 
 		// A type switch guard must be of the form:
@@ -590,7 +424,6 @@ func (check *checker) stmt(s ast.Stmt) {
 			check.stmtList(clause.Body)
 			check.closeScope()
 		}
-		check.closeScope()
 
 	case *ast.SelectStmt:
 		check.multipleDefaults(s.Body.List)
@@ -621,6 +454,8 @@ func (check *checker) stmt(s ast.Stmt) {
 
 	case *ast.RangeStmt:
 		check.openScope(s)
+		defer check.closeScope()
+
 		// check expression to iterate over
 		decl := s.Tok == token.DEFINE
 		var x operand
@@ -678,27 +513,63 @@ func (check *checker) stmt(s ast.Stmt) {
 		}
 
 		// check assignment to/declaration of iteration variables
-		// TODO(gri) The error messages/positions are not great here,
+		// (irregular assignment, cannot easily map to existing assignment checks)
-		//           they refer to the expression in the range clause.
+		if s.Key == nil {
-		//           Should give better messages w/o too much code
-		//           duplication (assignment checking).
-		x.mode = value
-		if s.Key != nil {
-			x.typ = key
-			x.expr = s.Key
-			check.assign1to1(s.Key, nil, &x, decl)
-		} else {
 			check.invalidAST(s.Pos(), "range clause requires index iteration variable")
 			// ok to continue
 		}
-		if s.Value != nil {
+
-			x.typ = val
+		// lhs expressions and initialization value (rhs) types
-			x.expr = s.Value
+		lhs := [2]ast.Expr{s.Key, s.Value}
-			check.assign1to1(s.Value, nil, &x, decl)
+		rhs := [2]Type{key, val}
+
+		if decl {
+			// declaration; variable scope starts after the range clause
+			var idents []*ast.Ident
+			var vars []*Var
+			for i, lhs := range lhs {
+				if lhs == nil {
+					continue
+				}
+
+				// determine lhs variable
+				name := "_" // dummy, in case lhs is not an identifier
+				ident, _ := lhs.(*ast.Ident)
+				if ident != nil {
+					name = ident.Name
+				} else {
+					check.errorf(lhs.Pos(), "cannot declare %s", lhs)
+				}
+				idents = append(idents, ident)
+
+				obj := NewVar(lhs.Pos(), check.pkg, name, nil)
+				vars = append(vars, obj)
+
+				// initialize lhs variable
+				x.mode = value
+				x.expr = lhs // we don't have a better rhs expression to use here
+				x.typ = rhs[i]
+				check.initVar(obj, &x)
+			}
+
+			// declare variables
+			for i, ident := range idents {
+				check.declare(check.topScope, ident, vars[i])
+			}
+		} else {
+			// ordinary assignment
+			for i, lhs := range lhs {
+				if lhs == nil {
+					continue
+				}
+				x.mode = value
+				x.expr = lhs // we don't have a better rhs expression to use here
+				x.typ = rhs[i]
+				check.assignVar(lhs, &x)
+			}
 		}
 
 		check.stmt(s.Body)
-		check.closeScope()
 
 	default:
 		check.errorf(s.Pos(), "invalid statement")