From cc52b8b7f834aeb2ec013b631d93696c75ee5ca1 Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Mon, 8 Jul 2013 09:40:30 -0700 Subject: [PATCH] go.tools/go/types: clean up assignment checks (round 1) Various bug fixes: - don't allow := to redeclare non-variables - don't permit a comma-ok expression as a two-value function return Lots of dead code removed. Fixes golang/go#5500. R=adonovan CC=golang-dev https://golang.org/cl/10792044 --- go/exact/exact.go | 4 +- go/types/assignments.go | 212 +++++++++++++++++++------------- go/types/resolver.go | 26 +--- go/types/scope.go | 6 +- go/types/stmt.go | 191 ++++++---------------------- go/types/testdata/stmt0.src | 20 ++- ssa/interp/testdata/mrvchain.go | 5 +- 7 files changed, 198 insertions(+), 266 deletions(-) diff --git a/go/exact/exact.go b/go/exact/exact.go index 6d63a639..3eb39411 100644 --- a/go/exact/exact.go +++ b/go/exact/exact.go @@ -280,7 +280,7 @@ func BitLen(x Value) int { // Sign returns -1, 0, or 1 depending on whether // x < 0, x == 0, or x > 0. For complex values z, // the sign is 0 if z == 0, otherwise it is != 0. -// The result is 0 for unknown values. +// The result is 1 for unknown values. func Sign(x Value) int { switch x := x.(type) { case int64Val: @@ -298,7 +298,7 @@ func Sign(x Value) int { case complexVal: return x.re.Sign() | x.im.Sign() case unknownVal: - return 0 + return 1 // avoid spurious division by zero errors } panic(fmt.Sprintf("invalid Sign(%v)", x)) } diff --git a/go/types/assignments.go b/go/types/assignments.go index 398a9b67..1733ca2f 100644 --- a/go/types/assignments.go +++ b/go/types/assignments.go @@ -10,90 +10,92 @@ import ( "code.google.com/p/go.tools/go/exact" ) -// TODO(gri) initialize is very close to the 2nd half of assign1to1. -func (check *checker) assign(obj Object, x *operand) { - // Determine typ of lhs: If the object doesn't have a type - // yet, determine it from the type of x; if x is invalid, - // set the object type to Typ[Invalid]. - var typ Type - switch obj := obj.(type) { - default: - unreachable() +func (check *checker) initConst(lhs *Const, x *operand) { + lhs.val = exact.MakeUnknown() - case *Const: - typ = obj.typ // may already be Typ[Invalid] - if typ == nil { - typ = Typ[Invalid] - if x.mode != invalid { - typ = x.typ - } - obj.typ = typ - } - - case *Var: - typ = obj.typ // may already be Typ[Invalid] - if typ == nil { - typ = 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 + if x.mode == invalid || x.typ == Typ[Invalid] || lhs.typ == Typ[Invalid] { + if lhs.typ == nil { + lhs.typ = Typ[Invalid] } + return // nothing else to check } - // nothing else to check if we don't have a valid lhs or rhs - if typ == Typ[Invalid] || x.mode == invalid { - return + // If the lhs doesn't have a type yet, use the type of x. + if lhs.typ == nil { + lhs.typ = x.typ } - if !check.assignment(x, typ) { + if !check.assignment(x, lhs.typ) { if x.mode != invalid { - if x.typ != Typ[Invalid] && typ != Typ[Invalid] { - check.errorf(x.pos(), "cannot initialize %s (type %s) with %s", obj.Name(), typ, x) - } + check.errorf(x.pos(), "cannot define constant %s (type %s) as %s", lhs.Name(), lhs.typ, x) } return } - // for constants, set their value - if obj, _ := obj.(*Const); obj != nil { - obj.val = exact.MakeUnknown() // failure case: we don't know the constant value - if x.mode == constant { - if isConstType(x.typ) { - obj.val = x.val - } else if x.typ != Typ[Invalid] { - check.errorf(x.pos(), "%s has invalid constant type", x) + // rhs must be a constant + if x.mode != constant { + check.errorf(x.pos(), "%s is not constant", x) + return + } + + // rhs type must be a valid constant type + if !isConstType(x.typ) { + check.errorf(x.pos(), "%s has invalid constant type", x) + return + } + + lhs.val = x.val +} + +func (check *checker) initVar(lhs *Var, x *operand) { + typ := x.typ + + if x.mode == invalid || typ == Typ[Invalid] || lhs.typ == Typ[Invalid] { + if lhs.typ == nil { + lhs.typ = Typ[Invalid] + } + return // nothing else to check + } + + // If the lhs doesn't have a type yet, use the type of x. + if lhs.typ == nil { + if isUntyped(typ) { + // convert untyped types to default types + if typ == Typ[UntypedNil] { + check.errorf(x.pos(), "use of untyped nil") + lhs.typ = Typ[Invalid] + return // nothing else to check } - } else if x.mode != invalid { - check.errorf(x.pos(), "%s is not constant", x) + typ = defaultType(typ) + } + lhs.typ = typ + } + + if !check.assignment(x, lhs.typ) { + if x.mode != invalid { + check.errorf(x.pos(), "cannot initialize variable %s (type %s) with %s", lhs.Name(), lhs.typ, x) } } } -func (check *checker) assignMulti(lhs []Object, rhs []ast.Expr) { +func invalidateVars(list []*Var) { + for _, obj := range list { + if obj.typ == nil { + obj.typ = Typ[Invalid] + } + } +} + +func (check *checker) initVars(lhs []*Var, rhs []ast.Expr, allowCommaOk bool) { assert(len(lhs) > 0) - const decl = false - - // If the lhs and rhs have corresponding expressions, treat each - // matching pair as an individual pair. + // 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) - if x.mode == invalid { - goto Error - } - check.assign(lhs[i], &x) + check.initVar(lhs[i], &x) } return } @@ -108,49 +110,85 @@ func (check *checker) assignMulti(lhs []Object, rhs []ast.Expr) { var x operand check.expr(&x, rhs[0]) if x.mode == invalid { - goto Error + invalidateVars(lhs) + 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.assign(lhs[i], &x) + for i, lhs := range lhs { + x.mode = value + x.expr = rhs[0] + x.typ = t.At(i).typ + check.initVar(lhs, &x) } return } - if x.mode == valueok && len(lhs) == 2 { + if allowCommaOk && x.mode == valueok && len(lhs) == 2 { // comma-ok expression x.mode = value - check.assign(lhs[0], &x) + check.initVar(lhs[0], &x) + x.mode = value x.typ = Typ[UntypedBool] - check.assign(lhs[1], &x) + check.initVar(lhs[1], &x) return } } - check.errorf(lhs[0].Pos(), "assignment count mismatch: %d = %d", len(lhs), len(rhs)) + // lhs variables may be function parameters (return assignment); + // use rhs position information for properly located error messages + check.errorf(rhs[0].Pos(), "assignment count mismatch: %d = %d", len(lhs), len(rhs)) + invalidateVars(lhs) +} -Error: - // In case of a declaration, set all lhs types to Typ[Invalid]. - for _, obj := range lhs { - switch obj := obj.(type) { - case *Const: - if obj.typ == nil { - obj.typ = Typ[Invalid] +func (check *checker) shortVarDecl(lhs, rhs []ast.Expr) { + scope := check.topScope + + // collect lhs variables + vars := make([]*Var, len(lhs)) + for i, lhs := range lhs { + var obj *Var + 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. + if alt := scope.Lookup(nil, ident.Name); alt != nil { + // redeclared object must be a variable + if alt, _ := alt.(*Var); alt != nil { + obj = alt + } else { + check.errorf(lhs.Pos(), "cannot assign to %s", lhs) + } + } else { + // declare new variable + obj = NewVar(ident.Pos(), check.pkg, ident.Name, nil) } - obj.val = exact.MakeUnknown() - case *Var: - if obj.typ == nil { - obj.typ = Typ[Invalid] - } - default: - unreachable() + check.callIdent(ident, obj) // obj may be nil + } else { + check.errorf(lhs.Pos(), "cannot declare %s", lhs) + } + if obj == nil { + obj = NewVar(lhs.Pos(), check.pkg, "_", nil) // dummy variable + } + vars[i] = obj + } + + check.initVars(vars, rhs, true) + + // declare variables + n := 0 // number of new variables + for _, obj := range vars { + if obj.name == "_" { + obj.setParent(scope) + continue // blank identifiers are not visible + } + if scope.Insert(obj) == nil { + n++ // new declaration } } + if n == 0 { + check.errorf(vars[0].Pos(), "no new variables on left side of :=") + } } diff --git a/go/types/resolver.go b/go/types/resolver.go index 13bf95dc..bf7da01f 100644 --- a/go/types/resolver.go +++ b/go/types/resolver.go @@ -29,22 +29,6 @@ func (check *checker) declare(scope *Scope, id *ast.Ident, obj Object) { } } -func (check *checker) declareShort(scope *Scope, list []Object) { - n := 0 // number of new objects - for _, obj := range list { - if obj.Name() == "_" { - obj.setParent(scope) - continue // blank identifiers are not visible - } - if scope.Insert(obj) == nil { - n++ // new declaration - } - } - if n == 0 { - check.errorf(list[0].Pos(), "no new variables on left side of :=") - } -} - // A decl describes a package-level const, type, var, or func declaration. type decl struct { file *Scope // scope of file containing this declaration @@ -393,7 +377,7 @@ func (check *checker) declareConst(obj *Const, typ, init ast.Expr) { goto Error } - check.assign(obj, &x) + check.initConst(obj, &x) check.iota = nil return @@ -448,7 +432,7 @@ func (check *checker) declareVar(obj *Var, typ, init ast.Expr) { for i, lhs := range proj.lhs { x.expr = nil // TODO(gri) should do better here x.typ = t.At(i).typ - check.assign(lhs, &x) + check.initVar(lhs, &x) } return } @@ -456,10 +440,10 @@ func (check *checker) declareVar(obj *Var, typ, init ast.Expr) { if x.mode == valueok && len(proj.lhs) == 2 { // comma-ok expression x.mode = value - check.assign(proj.lhs[0], &x) + check.initVar(proj.lhs[0], &x) x.typ = Typ[UntypedBool] - check.assign(proj.lhs[1], &x) + check.initVar(proj.lhs[1], &x) return } @@ -468,7 +452,7 @@ func (check *checker) declareVar(obj *Var, typ, init ast.Expr) { goto Error } - check.assign(obj, &x) + check.initVar(obj, &x) return Error: diff --git a/go/types/scope.go b/go/types/scope.go index caf93d01..7922bc85 100644 --- a/go/types/scope.go +++ b/go/types/scope.go @@ -119,10 +119,12 @@ func (s *Scope) LookupParent(name string) Object { // If s already contains an object with the same package path // and name, Insert leaves s unchanged and returns that object. // Otherwise it inserts obj, sets the object's scope to s, and -// returns nil. +// returns nil. The object must not have the blank _ name. // func (s *Scope) Insert(obj Object) Object { - if alt := s.Lookup(obj.Pkg(), obj.Name()); alt != nil { + name := obj.Name() + assert(name != "_") + if alt := s.Lookup(obj.Pkg(), name); alt != nil { return alt } s.entries = append(s.entries, obj) diff --git a/go/types/stmt.go b/go/types/stmt.go index c362aea5..fe3d6752 100644 --- a/go/types/stmt.go +++ b/go/types/stmt.go @@ -9,8 +9,6 @@ package types import ( "go/ast" "go/token" - - "code.google.com/p/go.tools/go/exact" ) // assigment reports whether x can be assigned to a variable of type 'to', @@ -36,14 +34,15 @@ func (check *checker) assignment(x *operand, to Type) bool { 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, isConst bool) { - assert(!isConst || decl) - +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 { @@ -91,52 +90,25 @@ func (check *checker) assign1to1(lhs, rhs ast.Expr, x *operand, decl bool, isCon return } - // Determine typ of lhs: If the object doesn't have a type - // yet, determine it from the type of x; if x is invalid, - // set the object type to Typ[Invalid]. - var obj Object - var typ Type - if isConst { - obj = NewConst(ident.Pos(), check.pkg, ident.Name, nil, nil) - } else { - obj = NewVar(ident.Pos(), check.pkg, ident.Name, nil) - } + // 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) - // TODO(gri) remove this switch, combine with code above - switch obj := obj.(type) { - default: - unreachable() - - case *Const: - typ = obj.typ // may already be Typ[Invalid] - if typ == nil { - typ = Typ[Invalid] - if x.mode != invalid { - typ = x.typ + 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 - } - - case *Var: - typ = obj.typ // may already be Typ[Invalid] - if typ == nil { - typ = 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 } } + obj.typ = typ // nothing else to check if we don't have a valid lhs or rhs if typ == Typ[Invalid] || x.mode == invalid { @@ -151,38 +123,22 @@ func (check *checker) assign1to1(lhs, rhs ast.Expr, x *operand, decl bool, isCon } return } - - // for constants, set their value - if obj, _ := obj.(*Const); obj != nil { - obj.val = exact.MakeUnknown() // failure case: we don't know the constant value - if x.mode == constant { - if isConstType(x.typ) { - obj.val = x.val - } else if x.typ != Typ[Invalid] { - check.errorf(x.pos(), "%s has invalid constant type", x) - } - } else if x.mode != invalid { - check.errorf(x.pos(), "%s is not constant", x) - } - } } -// TODO(gri) assignNtoM is only used in one place now. remove and consolidate with other assignment functions. +// TODO(gri) assignNtoM is only used in one place now. +// Remove and consolidate with other assignment functions. -// assignNtoM typechecks a general assignment. If decl is set, the lhs expressions -// must be identifiers; if their types are not set, they are deduced from the types -// of the corresponding rhs expressions, or set to Typ[Invalid] in case of an error. +// assignNtoM typechecks a regular assignment. // Precondition: len(lhs) > 0 . // -func (check *checker) assignNtoM(lhs, rhs []ast.Expr, decl bool, isConst bool) { +func (check *checker) assignNtoM(lhs, rhs []ast.Expr) { assert(len(lhs) > 0) - assert(!isConst || decl) // 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, decl, isConst) + check.assign1to1(lhs[i], e, nil, false) } return } @@ -197,7 +153,7 @@ func (check *checker) assignNtoM(lhs, rhs []ast.Expr, decl bool, isConst bool) { var x operand check.expr(&x, rhs[0]) if x.mode == invalid { - goto Error + return } if t, ok := x.typ.(*Tuple); ok && len(lhs) == t.Len() { @@ -207,7 +163,7 @@ func (check *checker) assignNtoM(lhs, rhs []ast.Expr, decl bool, isConst bool) { obj := t.At(i) x.expr = nil // TODO(gri) should do better here x.typ = obj.typ - check.assign1to1(lhs[i], nil, &x, decl, isConst) + check.assign1to1(lhs[i], nil, &x, false) } return } @@ -215,44 +171,15 @@ func (check *checker) assignNtoM(lhs, rhs []ast.Expr, decl bool, isConst bool) { if x.mode == valueok && len(lhs) == 2 { // comma-ok expression x.mode = value - check.assign1to1(lhs[0], nil, &x, decl, isConst) + check.assign1to1(lhs[0], nil, &x, false) x.typ = Typ[UntypedBool] - check.assign1to1(lhs[1], nil, &x, decl, isConst) + check.assign1to1(lhs[1], nil, &x, false) return } } check.errorf(lhs[0].Pos(), "assignment count mismatch: %d = %d", len(lhs), len(rhs)) - -Error: - // In case of a declaration, set all lhs types to Typ[Invalid]. - if decl { - for _, e := range lhs { - ident, _ := e.(*ast.Ident) - if ident == nil { - check.errorf(e.Pos(), "cannot declare %s", e) - continue - } - - var obj Object - if isConst { - obj = NewConst(ident.Pos(), check.pkg, ident.Name, nil, nil) - } else { - obj = NewVar(ident.Pos(), check.pkg, ident.Name, nil) - } - defer check.declare(check.topScope, ident, obj) - - switch obj := obj.(type) { - case *Const: - obj.typ = Typ[Invalid] - case *Var: - obj.typ = Typ[Invalid] - default: - unreachable() - } - } - } } func (check *checker) optionalStmt(s ast.Stmt) { @@ -390,7 +317,7 @@ func (check *checker) stmt(s ast.Stmt) { if x.mode == invalid { return } - check.assign1to1(s.X, nil, &x, false, false) + check.assign1to1(s.X, nil, &x, false) case *ast.AssignStmt: switch s.Tok { @@ -400,29 +327,10 @@ func (check *checker) stmt(s ast.Stmt) { return } if s.Tok == token.DEFINE { - // short variable declaration - lhs := make([]Object, len(s.Lhs)) - for i, x := range s.Lhs { - var obj Object - if ident, ok := x.(*ast.Ident); ok { - // use the correct obj if the ident is redeclared - obj = NewVar(ident.Pos(), check.pkg, ident.Name, nil) - if alt := check.topScope.Lookup(nil, ident.Name); alt != nil { - obj = alt - } - check.callIdent(ident, obj) - } else { - check.errorf(x.Pos(), "cannot declare %s", x) - // create a dummy variable - obj = NewVar(x.Pos(), check.pkg, "_", nil) - } - lhs[i] = obj - } - check.assignMulti(lhs, s.Rhs) - check.declareShort(check.topScope, lhs) // scope starts after the assignment - + check.shortVarDecl(s.Lhs, s.Rhs) } else { - check.assignNtoM(s.Lhs, s.Rhs, s.Tok == token.DEFINE, false) + // regular assignment + check.assignNtoM(s.Lhs, s.Rhs) } default: @@ -465,7 +373,7 @@ func (check *checker) stmt(s ast.Stmt) { if x.mode == invalid { return } - check.assign1to1(s.Lhs[0], nil, &x, false, false) + check.assign1to1(s.Lhs[0], nil, &x, false) } case *ast.GoStmt: @@ -482,37 +390,18 @@ func (check *checker) stmt(s ast.Stmt) { sig := check.funcsig if n := sig.results.Len(); n > 0 { // determine if the function has named results - { - named := false - lhs := make([]Object, len(sig.results.vars)) - for i, res := range sig.results.vars { - if res.name != "" { - // a blank (_) result parameter is a named result - named = true - } - lhs[i] = res - } - if len(s.Results) > 0 || !named { - check.assignMulti(lhs, s.Results) - return - } - } - - // TODO(gri) should not have to compute lhs, named every single time - clean this up - lhs := make([]ast.Expr, n) - named := false // if set, function has named results + named := false + lhs := make([]*Var, n) for i, res := range sig.results.vars { - if len(res.name) > 0 { + if res.name != "" { // a blank (_) result parameter is a named result named = true } - name := ast.NewIdent(res.name) - name.NamePos = s.Pos() - lhs[i] = name + lhs[i] = res } if len(s.Results) > 0 || !named { - // TODO(gri) assignNtoM should perhaps not require len(lhs) > 0 - check.assignNtoM(lhs, s.Results, false, false) + check.initVars(lhs, s.Results, false) + return } } else if len(s.Results) > 0 { check.errorf(s.Pos(), "no result values expected") @@ -797,7 +686,7 @@ func (check *checker) stmt(s ast.Stmt) { if s.Key != nil { x.typ = key x.expr = s.Key - check.assign1to1(s.Key, nil, &x, decl, false) + check.assign1to1(s.Key, nil, &x, decl) } else { check.invalidAST(s.Pos(), "range clause requires index iteration variable") // ok to continue @@ -805,7 +694,7 @@ func (check *checker) stmt(s ast.Stmt) { if s.Value != nil { x.typ = val x.expr = s.Value - check.assign1to1(s.Value, nil, &x, decl, false) + check.assign1to1(s.Value, nil, &x, decl) } check.stmt(s.Body) diff --git a/go/types/testdata/stmt0.src b/go/types/testdata/stmt0.src index a87bd9e8..7e098177 100644 --- a/go/types/testdata/stmt0.src +++ b/go/types/testdata/stmt0.src @@ -14,7 +14,7 @@ func assignments() { c = s /* ERROR "cannot assign" */ s = b /* ERROR "cannot assign" */ - v0 /* ERROR "mismatch" */, v1, v2 := 1, 2, 3, 4 + v0, v1, v2 := 1 /* ERROR "mismatch" */ , 2, 3, 4 b = true @@ -45,6 +45,24 @@ func assignments() { _ map[int]string = nil _ chan int = nil ) + + // test cases for issue 5500 + _ = func() (int, bool) { + var m map[int]int + return m /* ERROR "assignment count mismatch" */ [0] + } + + g := func(int, bool){} + var m map[int]int + g/* ERROR "too few arguments" */ (m[0]) +} + +func shortVarDecls() { + const c = 0 + type d int + a, b, c /* ERROR "cannot assign" */ , d /* ERROR "cannot assign" */ := 1, "zwei", 3.0, 4 + var _ int = a // a is of type int + var _ string = b // b is of type string } func incdecs() { diff --git a/ssa/interp/testdata/mrvchain.go b/ssa/interp/testdata/mrvchain.go index 1c588bcf..0799459a 100644 --- a/ssa/interp/testdata/mrvchain.go +++ b/ssa/interp/testdata/mrvchain.go @@ -41,9 +41,10 @@ func appendArgs() ([]string, string) { return []string{"foo"}, "bar" } -func h() (interface{}, bool) { +func h() (i interface{}, ok bool) { m := map[int]string{1: "hi"} - return m[1] // string->interface{} conversion within multi-valued expression + i, ok = m[1] // string->interface{} conversion within multi-valued expression + return } func main() {