From 5d3ec727598bc694c912a0473321538d7221dfa6 Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Wed, 5 Mar 2014 15:05:34 -0800 Subject: [PATCH] go.tools/go/types: implement shadowed return parameter restriction Also: - slightly better error messages for return statements - more AST validity checking of parameter lists - use secondary error message for clarifying message in type switch errors Fixes golang/go#7469. LGTM=adonovan R=adonovan CC=golang-codereviews https://golang.org/cl/71780044 --- go/types/assignments.go | 14 ++++++++++---- go/types/decl.go | 2 +- go/types/stmt.go | 37 +++++++++++++++++++----------------- go/types/testdata/decls1.src | 1 + go/types/testdata/stmt0.src | 37 +++++++++++++++++++++++++++++++++--- go/types/typexpr.go | 12 ++++++++++++ 6 files changed, 78 insertions(+), 25 deletions(-) diff --git a/go/types/assignments.go b/go/types/assignments.go index ac8aa2c2..caef7fb6 100644 --- a/go/types/assignments.go +++ b/go/types/assignments.go @@ -101,7 +101,8 @@ func (check *checker) initConst(lhs *Const, x *operand) { lhs.val = x.val } -func (check *checker) initVar(lhs *Var, x *operand) Type { +// If result is set, lhs is a function result parameter and x is a return result. +func (check *checker) initVar(lhs *Var, x *operand, result bool) Type { if x.mode == invalid || x.typ == Typ[Invalid] || lhs.typ == Typ[Invalid] { if lhs.typ == nil { lhs.typ = Typ[Invalid] @@ -126,7 +127,12 @@ func (check *checker) initVar(lhs *Var, x *operand) Type { 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) + if result { + // don't refer to lhs.name because it may be an anonymous result parameter + check.errorf(x.pos(), "cannot return %s as value of type %s", x, lhs.typ) + } else { + check.errorf(x.pos(), "cannot initialize %s with %s", lhs, x) + } } return nil } @@ -223,7 +229,7 @@ func (check *checker) initVars(lhs []*Var, rhs []ast.Expr, returnPos token.Pos) var a [2]Type for i := range a { get(&x, i) - a[i] = check.initVar(lhs[i], &x) + a[i] = check.initVar(lhs[i], &x, returnPos.IsValid()) } check.recordCommaOkTypes(rhs[0], a) return @@ -231,7 +237,7 @@ func (check *checker) initVars(lhs []*Var, rhs []ast.Expr, returnPos token.Pos) for i, lhs := range lhs { get(&x, i) - check.initVar(lhs, &x) + check.initVar(lhs, &x, returnPos.IsValid()) } } diff --git a/go/types/decl.go b/go/types/decl.go index 60a689c3..51a306c1 100644 --- a/go/types/decl.go +++ b/go/types/decl.go @@ -146,7 +146,7 @@ func (check *checker) varDecl(obj *Var, lhs []*Var, typ, init ast.Expr) { assert(lhs == nil || lhs[0] == obj) var x operand check.expr(&x, init) - check.initVar(obj, &x) + check.initVar(obj, &x, false) return } diff --git a/go/types/stmt.go b/go/types/stmt.go index d1f94caa..762279ff 100644 --- a/go/types/stmt.go +++ b/go/types/stmt.go @@ -187,7 +187,7 @@ L: if T == nil && t == nil || T != nil && t != nil && Identical(T, t) { // talk about "case" rather than "type" because of nil case check.errorf(e.Pos(), "duplicate case in type switch") - check.errorf(pos, "previous case %s", T) + check.errorf(pos, "\tprevious case %s", T) // secondary error, \t indented continue L } } @@ -320,24 +320,27 @@ func (check *checker) stmt(ctxt stmtContext, s ast.Stmt) { check.suspendedCall("defer", s.Call) case *ast.ReturnStmt: - sig := check.sig - if n := sig.results.Len(); n > 0 { - // determine if the function has named results - named := false - lhs := make([]*Var, n) - for i, res := range sig.results.vars { - if res.name != "" { - // a blank (_) result parameter is a named result - named = true + res := check.sig.results + if res.Len() > 0 { + // function returns results + // (if one, say the first, result parameter is named, all of them are named) + if len(s.Results) == 0 && res.vars[0].name != "" { + // spec: "Implementation restriction: A compiler may disallow an empty expression + // 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 { + 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 + } } - lhs[i] = res - } - if len(s.Results) > 0 || !named { - check.initVars(lhs, s.Results, s.Return) - return + } else { + // return has results or result parameters are unnamed + check.initVars(res.vars, s.Results, s.Return) } } else if len(s.Results) > 0 { - check.errorf(s.Pos(), "no result values expected") + check.errorf(s.Results[0].Pos(), "no result values expected") } case *ast.BranchStmt: @@ -691,7 +694,7 @@ func (check *checker) stmt(ctxt stmtContext, s ast.Stmt) { 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) + check.initVar(obj, &x, false) } // declare variables diff --git a/go/types/testdata/decls1.src b/go/types/testdata/decls1.src index 56c88333..7855e461 100644 --- a/go/types/testdata/decls1.src +++ b/go/types/testdata/decls1.src @@ -69,6 +69,7 @@ var ( t18 float64 = math.Pi * 10.0 t19 int = t1 /* ERROR "cannot call" */ () t20 int = f0 /* ERROR "no value" */ () + t21 int = a /* ERROR "cannot initialize" */ ) // Various more complex expressions diff --git a/go/types/testdata/stmt0.src b/go/types/testdata/stmt0.src index c42bd36a..6cf22141 100644 --- a/go/types/testdata/stmt0.src +++ b/go/types/testdata/stmt0.src @@ -357,6 +357,37 @@ func continues() { } } +func returns0() { + return + return 0 /* ERROR no result values expected */ +} + +func returns1(x float64) (int, *float64) { + return 0, &x + return /* ERROR wrong number of return values */ + return "foo" /* ERROR "cannot convert" */, x /* ERROR "cannot return" */ + return /* ERROR wrong number of return values */ 0, &x, 1 +} + +func returns2() (a, b int) { + return + return 1, "foo" /* ERROR cannot convert */ + return /* ERROR wrong number of return values */ 1, 2, 3 + { + type a int + return 1, 2 + return /* ERROR a not in scope at return */ + } +} + +func returns3() (_ int) { + return + { + var _ int // blank (_) identifiers never shadow since they are in no scope + return + } +} + func switches0() { var x int @@ -566,13 +597,13 @@ func typeswitch2() { func typeswitch3(x interface{}) { switch x.(type) { - case int /* ERROR previous case int */ : + case int: case float64: case int /* ERROR duplicate case */ : } switch x.(type) { - case nil /* ERROR previous case */ /* ERROR previous case */ : + case nil: case int: case nil /* ERROR duplicate case */ , nil /* ERROR duplicate case */ : } @@ -580,7 +611,7 @@ func typeswitch3(x interface{}) { type F func(int) switch x.(type) { case nil: - case int, func /* ERROR previous case */ (int): + case int, func(int): case float32, func /* ERROR duplicate case */ (x int): case F: } diff --git a/go/types/typexpr.go b/go/types/typexpr.go index 1cff6722..4d1a0cee 100644 --- a/go/types/typexpr.go +++ b/go/types/typexpr.go @@ -382,6 +382,7 @@ func (check *checker) collectParams(scope *Scope, list *ast.FieldList, variadicO return } + var named, anonymous bool for i, field := range list.List { ftype := field.Type if t, _ := ftype.(*ast.Ellipsis); t != nil { @@ -399,18 +400,29 @@ func (check *checker) collectParams(scope *Scope, list *ast.FieldList, variadicO if len(field.Names) > 0 { // named parameter for _, name := range field.Names { + if name.Name == "" { + check.invalidAST(name.Pos(), "anonymous parameter") + // ok to continue + } par := NewParam(name.Pos(), check.pkg, name.Name, typ) check.declare(scope, name, par) params = append(params, par) } + named = true } else { // anonymous parameter par := NewParam(ftype.Pos(), check.pkg, "", typ) check.recordImplicit(field, par) params = append(params, par) + anonymous = true } } + if named && anonymous { + check.invalidAST(list.Pos(), "list contains both named and anonymous parameters") + // ok to continue + } + // For a variadic function, change the last parameter's type from T to []T. if variadic && len(params) > 0 { last := params[len(params)-1]