From b8f13c4c9b0de6077c2c1556343d3e1e2804db75 Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Tue, 9 Jul 2013 09:45:09 -0700 Subject: [PATCH] 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 | 4 +- go/types/assignments.go | 122 +++++++++++++++++-- go/types/stmt.go | 251 ++++++++++------------------------------ 3 files changed, 179 insertions(+), 198 deletions(-) diff --git a/go/types/api.go b/go/types/api.go index be7f8abc..d203126e 100644 --- a/go/types/api.go +++ b/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)? diff --git a/go/types/assignments.go b/go/types/assignments.go index 1733ca2f..8af3366f 100644 --- a/go/types/assignments.go +++ b/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 || typ == Typ[Invalid] || lhs.typ == Typ[Invalid] { + if x.mode == invalid || x.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) { - for _, obj := range list { - if obj.typ == nil { - obj.typ = Typ[Invalid] +func (check *checker) assignVar(lhs ast.Expr, x *operand) { + if x.mode == invalid || x.typ == Typ[Invalid] { + return + } + + // 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] + } + } +} diff --git a/go/types/stmt.go b/go/types/stmt.go index fe3d6752..dcd54fe3 100644 --- a/go/types/stmt.go +++ b/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, - // they refer to the expression in the range clause. - // 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 { + // (irregular assignment, cannot easily map to existing assignment checks) + if s.Key == nil { check.invalidAST(s.Pos(), "range clause requires index iteration variable") // ok to continue } - if s.Value != nil { - x.typ = val - x.expr = s.Value - check.assign1to1(s.Value, nil, &x, decl) + + // lhs expressions and initialization value (rhs) types + lhs := [2]ast.Expr{s.Key, s.Value} + 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")