From 7bc647b29a9ab1dd589c97fa0322507d4785270d Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Tue, 23 Jul 2013 13:42:04 -0700 Subject: [PATCH] go.tools/go/types: internal cleanups - more consistent naming of some internal data types and methods - better factoring of some error reporting code - cleanup around association of methods with receiver base types - more tests R=adonovan CC=golang-dev https://golang.org/cl/11726043 --- go/types/check.go | 34 ++---- go/types/check_test.go | 4 +- go/types/errors.go | 4 +- go/types/resolver.go | 191 ++++++++++++++++++---------------- go/types/stmt.go | 6 +- go/types/testdata/decls2b.src | 10 ++ go/types/typexpr.go | 2 +- 7 files changed, 128 insertions(+), 123 deletions(-) diff --git a/go/types/check.go b/go/types/check.go index 84af263b089..6001d30896d 100644 --- a/go/types/check.go +++ b/go/types/check.go @@ -38,22 +38,22 @@ type exprInfo struct { type checker struct { conf *Config fset *token.FileSet - pkg *Package // current package + pkg *Package methods map[string][]*Func // maps type names to associated methods conversions map[*ast.CallExpr]bool // set of type-checked conversions (to distinguish from calls) untyped map[ast.Expr]exprInfo // map of expressions without final type - firsterr error // first error encountered + firstErr error // first error encountered Info // collected type info - objMap map[Object]*declInfo // if set we are in the package-level declaration phase (otherwise all objects seen must be declared) - topScope *Scope // topScope for lookups, non-global declarations - iota exact.Value // value of iota in a constant declaration; nil otherwise + objMap map[Object]declInfo // if set we are in the package-level declaration phase (otherwise all objects seen must be declared) + topScope *Scope // current topScope for lookups + iota exact.Value // value of iota in a constant declaration; nil otherwise // functions - funclist []function // list of functions/methods with correct signatures and non-empty bodies - funcsig *Signature // signature of currently type-checked function + funcList []funcInfo // list of functions/methods with correct signatures and non-empty bodies + funcSig *Signature // signature of currently type-checked function // debugging indent int // indentation for tracing @@ -96,24 +96,6 @@ func (check *checker) recordImplicit(node ast.Node, obj Object) { } } -type function struct { - file *Scope - obj *Func // for debugging/tracing only - sig *Signature - body *ast.BlockStmt -} - -// later adds a function with non-empty body to the list of functions -// that need to be processed after all package-level declarations -// are type-checked. -// -func (check *checker) later(f *Func, sig *Signature, body *ast.BlockStmt) { - // functions implemented elsewhere (say in assembly) have no body - if body != nil { - check.funclist = append(check.funclist, function{check.topScope, f, sig, body}) - } -} - // A bailout panic is raised to indicate early termination. type bailout struct{} @@ -121,7 +103,7 @@ func (check *checker) handleBailout(err *error) { switch p := recover().(type) { case nil, bailout: // normal return or early exit - *err = check.firsterr + *err = check.firstErr default: // unexpected panic: don't crash clients // TODO(gri) add a test case for this scenario diff --git a/go/types/check_test.go b/go/types/check_test.go index c7fbf52ffe0..69e8e404ac0 100644 --- a/go/types/check_test.go +++ b/go/types/check_test.go @@ -209,10 +209,10 @@ func checkFiles(t *testing.T, testfiles []string) { t.Error(err) return } - // Ignore error messages containing "previous declaration": + // Ignore error messages containing "other declaration": // They are follow-up error messages after a redeclaration // error. - if !strings.Contains(err.Error(), "previous declaration") { + if !strings.Contains(err.Error(), "other declaration") { errlist = append(errlist, err) } } diff --git a/go/types/errors.go b/go/types/errors.go index 502e76babea..3f7f97c2b46 100644 --- a/go/types/errors.go +++ b/go/types/errors.go @@ -55,8 +55,8 @@ func (check *checker) dump(format string, args ...interface{}) { } func (check *checker) err(err error) { - if check.firsterr == nil { - check.firsterr = err + if check.firstErr == nil { + check.firstErr = err } f := check.conf.Error if f == nil { diff --git a/go/types/resolver.go b/go/types/resolver.go index 166bbe2e010..ceee0884ddb 100644 --- a/go/types/resolver.go +++ b/go/types/resolver.go @@ -14,15 +14,22 @@ import ( "code.google.com/p/go.tools/go/exact" ) +func (check *checker) reportAltDecl(obj Object) { + if pos := obj.Pos(); pos.IsValid() { + // We use "other" rather than "previous" here because + // the first declaration seen may not be textually + // earlier in the source. + check.errorf(pos, "other declaration of %s", obj.Name()) + } +} + func (check *checker) declare(scope *Scope, id *ast.Ident, obj Object) { if obj.Name() == "_" { // blank identifiers are not declared obj.setParent(scope) } else if alt := scope.Insert(obj); alt != nil { check.errorf(obj.Pos(), "%s redeclared in this block", obj.Name()) - if pos := alt.Pos(); pos.IsValid() { - check.errorf(pos, "previous declaration of %s", obj.Name()) - } + check.reportAltDecl(alt) obj = nil // for callIdent below } if id != nil { @@ -47,6 +54,20 @@ type multiExpr struct { ast.Expr // dummy to satisfy ast.Expr interface } +type funcInfo struct { + obj *Func // for debugging/tracing only + sig *Signature + body *ast.BlockStmt +} + +// later appends a function with non-empty body to check.funcList. +func (check *checker) later(f *Func, sig *Signature, body *ast.BlockStmt) { + // functions implemented elsewhere (say in assembly) have no body + if body != nil { + check.funcList = append(check.funcList, funcInfo{f, sig, body}) + } +} + // arityMatch checks that the lhs and rhs of a const or var decl // have the appropriate number of names and init exprs. For const // decls, init is the value spec providing the init exprs; for @@ -88,33 +109,28 @@ func (check *checker) resolveFiles(files []*ast.File) { // independent of source order. Collect methods for a later phase. var ( - fileScope *Scope // current file scope, used by declare - scopes []*Scope // corresponding file scope per file - objList []Object // list of objects to type-check - objMap = make(map[Object]*declInfo) // declaration info for each object (incl. methods) + scopes []*Scope // corresponding file scope per file + scope *Scope // current file scope + objList []Object // list of package-level objects to type-check + objMap = make(map[Object]declInfo) // declaration info for each package-level object ) - declare := func(ident *ast.Ident, obj Object, typ, init ast.Expr, fdecl *ast.FuncDecl) { + // declare declares obj in the package scope, records its ident -> obj mapping, + // and updates objList and objMap. The object must not be a function or method. + declare := func(ident *ast.Ident, obj Object, typ, init ast.Expr) { assert(ident.Name == obj.Name()) // spec: "A package-scope or file-scope identifier with name init // may only be declared to be a function with this (func()) signature." if ident.Name == "init" { - f, _ := obj.(*Func) - if f == nil { - check.recordObject(ident, nil) - check.errorf(ident.Pos(), "cannot declare init - must be func") - return - } - // don't declare init functions in the package scope - they are invisible - f.parent = pkg.scope - check.recordObject(ident, obj) - } else { - check.declare(pkg.scope, ident, obj) + check.recordObject(ident, nil) + check.errorf(ident.Pos(), "cannot declare init - must be func") + return } + check.declare(pkg.scope, ident, obj) objList = append(objList, obj) - objMap[obj] = &declInfo{fileScope, typ, init, fdecl} + objMap[obj] = declInfo{scope, typ, init, nil} } importer := check.conf.Import @@ -126,11 +142,11 @@ func (check *checker) resolveFiles(files []*ast.File) { // the package identifier denotes the current package, but it is in no scope check.recordObject(file.Name, pkg) - fileScope = NewScope(pkg.scope) + scope = NewScope(pkg.scope) if retainASTLinks { - fileScope.node = file + scope.node = file } - scopes = append(scopes, fileScope) + scopes = append(scopes, scope) for _, decl := range file.Decls { switch d := decl.(type) { @@ -179,13 +195,13 @@ func (check *checker) resolveFiles(files []*ast.File) { if obj.IsExported() { // Note: This will change each imported object's scope! // May be an issue for types aliases. - check.declare(fileScope, nil, obj) + check.declare(scope, nil, obj) check.recordImplicit(s, obj) } } } else { // declare imported package object in file scope - check.declare(fileScope, nil, imp2) + check.declare(scope, nil, imp2) } case *ast.ValueSpec: @@ -208,7 +224,7 @@ func (check *checker) resolveFiles(files []*ast.File) { init = last.Values[i] } - declare(name, obj, last.Type, init, nil) + declare(name, obj, last.Type, init) } check.arityMatch(s, last) @@ -237,7 +253,7 @@ func (check *checker) resolveFiles(files []*ast.File) { } } - declare(name, obj, s.Type, init, nil) + declare(name, obj, s.Type, init) } check.arityMatch(s, nil) @@ -248,7 +264,7 @@ func (check *checker) resolveFiles(files []*ast.File) { case *ast.TypeSpec: obj := NewTypeName(s.Name.Pos(), pkg, s.Name.Name, nil) - declare(s.Name, obj, s.Type, nil, nil) + declare(s.Name, obj, s.Type, nil) default: check.invalidAST(s.Pos(), "unknown ast.Spec node %T", s) @@ -256,39 +272,36 @@ func (check *checker) resolveFiles(files []*ast.File) { } case *ast.FuncDecl: - obj := NewFunc(d.Name.Pos(), pkg, d.Name.Name, nil) + name := d.Name.Name + obj := NewFunc(d.Name.Pos(), pkg, name, nil) if d.Recv == nil { // regular function - declare(d.Name, obj, nil, nil, d) - continue + if name == "init" { + // don't declare init functions in the package scope - they are invisible + obj.parent = pkg.scope + check.recordObject(d.Name, obj) + } else { + check.declare(pkg.scope, d.Name, obj) + } + } else { + // Associate method with receiver base type name, if possible. + // Ignore methods that have an invalid receiver, or a blank _ + // receiver or method name. They will be type-checked later, + // with non-method functions. + if obj.name != "_" { + if list := d.Recv.List; len(list) > 0 { + typ := list[0].Type + if ptr, _ := typ.(*ast.StarExpr); ptr != nil { + typ = ptr.X + } + if base, _ := typ.(*ast.Ident); base != nil && base.Name != "_" { + check.methods[base.Name] = append(check.methods[base.Name], obj) + } + } + } } - - // Methods are associated with the receiver base type - // which we don't have yet. Instead, collect methods - // with receiver base type name so that they are known - // when the receiver base type is type-checked. - - // The receiver type must be one of the following - // - *ast.Ident - // - *ast.StarExpr{*ast.Ident} - // - *ast.BadExpr (parser error) - typ := d.Recv.List[0].Type - if ptr, _ := typ.(*ast.StarExpr); ptr != nil { - typ = ptr.X - } - - // Associate method with receiver base type name, if possible. - // Methods with receiver types that are not type names, that - // are blank _ names, or methods with blank names are ignored; - // the respective error will be reported when the method signature - // is type-checked. - if ident, _ := typ.(*ast.Ident); ident != nil && ident.Name != "_" && obj.name != "_" { - check.methods[ident.Name] = append(check.methods[ident.Name], obj) - } - - // Collect methods like other objects. objList = append(objList, obj) - objMap[obj] = &declInfo{fileScope, nil, nil, d} + objMap[obj] = declInfo{scope, nil, nil, d} default: check.invalidAST(d.Pos(), "unknown ast.Decl node %T", d) @@ -311,22 +324,25 @@ func (check *checker) resolveFiles(files []*ast.File) { check.objMap = objMap // indicate that we are checking global declarations (objects may not have a type yet) for _, obj := range objList { if obj.Type() == nil { - check.declareObject(obj, nil, false) + check.objDecl(obj, nil, false) } } - check.objMap = nil // done with global declarations + + // done with package-level declarations + check.objMap = nil + objList = nil // At this point we may have a non-empty check.methods map; this means that not all - // entries were deleted at the end of declareType, because the respective receiver - // base types were not declared. In that case, an error was reported when declaring - // those methods. We can now safely discard this map. + // entries were deleted at the end of typeDecl because the respective receiver base + // types were not declared. In that case, an error was reported when declaring those + // methods. We can now safely discard this map. check.methods = nil // Phase 4: Typecheck all functions bodies. - // (funclist may grow when checking statements - cannot use range clause) - for i := 0; i < len(check.funclist); i++ { - f := check.funclist[i] + // Note: funcList may grow while iterating through it - cannot use range clause. + for i := 0; i < len(check.funcList); i++ { + f := check.funcList[i] if trace { s := "" if f.obj != nil { @@ -335,17 +351,17 @@ func (check *checker) resolveFiles(files []*ast.File) { fmt.Println("---", s) } check.topScope = f.sig.scope // open the function scope - check.funcsig = f.sig + check.funcSig = f.sig check.stmtList(f.body.List) - if f.sig.results.Len() > 0 && f.body != nil && !check.isTerminating(f.body, "") { + if f.sig.results.Len() > 0 && !check.isTerminating(f.body, "") { check.errorf(f.body.Rbrace, "missing return") } } } -// declareObject completes the declaration of obj in its respective file scope. -// See declareType for the details on def and cycleOk. -func (check *checker) declareObject(obj Object, def *Named, cycleOk bool) { +// objDecl type-checks the declaration of obj in its respective file scope. +// See typeDecl for the details on def and cycleOk. +func (check *checker) objDecl(obj Object, def *Named, cycleOk bool) { d := check.objMap[obj] // adjust file scope for current object @@ -358,13 +374,13 @@ func (check *checker) declareObject(obj Object, def *Named, cycleOk bool) { switch obj := obj.(type) { case *Const: - check.declareConst(obj, d.typ, d.init) + check.constDecl(obj, d.typ, d.init) case *Var: - check.declareVar(obj, d.typ, d.init) + check.varDecl(obj, d.typ, d.init) case *TypeName: - check.declareType(obj, d.typ, def, cycleOk) + check.typeDecl(obj, d.typ, def, cycleOk) case *Func: - check.declareFunc(obj, d.fdecl) + check.funcDecl(obj, d.fdecl) default: unreachable() } @@ -373,7 +389,7 @@ func (check *checker) declareObject(obj Object, def *Named, cycleOk bool) { check.topScope = oldScope } -func (check *checker) declareConst(obj *Const, typ, init ast.Expr) { +func (check *checker) constDecl(obj *Const, typ, init ast.Expr) { if obj.visited { check.errorf(obj.Pos(), "illegal cycle in initialization of constant %s", obj.name) obj.typ = Typ[Invalid] @@ -400,7 +416,7 @@ func (check *checker) declareConst(obj *Const, typ, init ast.Expr) { check.iota = nil } -func (check *checker) declareVar(obj *Var, typ, init ast.Expr) { +func (check *checker) varDecl(obj *Var, typ, init ast.Expr) { if obj.visited { check.errorf(obj.Pos(), "illegal cycle in initialization of variable %s", obj.name) obj.typ = Typ[Invalid] @@ -435,7 +451,7 @@ func (check *checker) declareVar(obj *Var, typ, init ast.Expr) { check.initVar(obj, &x) } -func (check *checker) declareType(obj *TypeName, typ ast.Expr, def *Named, cycleOk bool) { +func (check *checker) typeDecl(obj *TypeName, typ ast.Expr, def *Named, cycleOk bool) { assert(obj.Type() == nil) // type declarations cannot use iota @@ -512,15 +528,11 @@ func (check *checker) declareType(obj *TypeName, typ ast.Expr, def *Named, cycle switch alt := alt.(type) { case *Var: check.errorf(m.pos, "field and method with the same name %s", m.name) - if pos := alt.pos; pos.IsValid() { - check.errorf(pos, "previous declaration of %s", m.name) - } + check.reportAltDecl(alt) m = nil case *Func: check.errorf(m.pos, "method %s already declared for %s", m.name, named) - if pos := alt.pos; pos.IsValid() { - check.errorf(pos, "previous declaration of %s", m.name) - } + check.reportAltDecl(alt) m = nil } } @@ -530,7 +542,7 @@ func (check *checker) declareType(obj *TypeName, typ ast.Expr, def *Named, cycle // If the method is valid, type-check its signature, // and collect it with the named base type. if m != nil { - check.declareObject(m, nil, true) + check.objDecl(m, nil, true) named.methods = append(named.methods, m) } } @@ -538,7 +550,7 @@ func (check *checker) declareType(obj *TypeName, typ ast.Expr, def *Named, cycle delete(check.methods, obj.name) // we don't need them anymore } -func (check *checker) declareFunc(obj *Func, fdecl *ast.FuncDecl) { +func (check *checker) funcDecl(obj *Func, fdecl *ast.FuncDecl) { // func declarations cannot use iota assert(check.iota == nil) @@ -549,6 +561,7 @@ func (check *checker) declareFunc(obj *Func, fdecl *ast.FuncDecl) { // ok to continue } obj.typ = sig + check.later(obj, sig, fdecl.Body) } @@ -585,7 +598,7 @@ func (check *checker) declStmt(decl ast.Decl) { init = last.Values[i] } - check.declareConst(obj, last.Type, init) + check.constDecl(obj, last.Type, init) } check.arityMatch(s, last) @@ -595,7 +608,7 @@ func (check *checker) declStmt(decl ast.Decl) { } case token.VAR: - // For declareVar called with a multiExpr we need the fully + // For varDecl called with a multiExpr we need the fully // initialized lhs. Compute it in a separate pre-pass. lhs := make([]*Var, len(s.Names)) for i, name := range s.Names { @@ -618,7 +631,7 @@ func (check *checker) declStmt(decl ast.Decl) { } } - check.declareVar(obj, s.Type, init) + check.varDecl(obj, s.Type, init) } check.arityMatch(s, nil) @@ -634,7 +647,7 @@ func (check *checker) declStmt(decl ast.Decl) { case *ast.TypeSpec: obj := NewTypeName(s.Name.Pos(), pkg, s.Name.Name, nil) check.declare(check.topScope, s.Name, obj) - check.declareType(obj, s.Type, nil, false) + check.typeDecl(obj, s.Type, nil, false) default: check.invalidAST(s.Pos(), "const, type, or var declaration expected") diff --git a/go/types/stmt.go b/go/types/stmt.go index 97b5af52e08..19e88fbf84c 100644 --- a/go/types/stmt.go +++ b/go/types/stmt.go @@ -79,10 +79,10 @@ func (check *checker) stmt(s ast.Stmt) { check.declStmt(s.Decl) case *ast.LabeledStmt: - scope := check.funcsig.labels + scope := check.funcSig.labels if scope == nil { scope = new(Scope) // no label scope chain - check.funcsig.labels = scope + check.funcSig.labels = scope } label := s.Label check.declare(scope, label, NewLabel(label.Pos(), label.Name)) @@ -219,7 +219,7 @@ func (check *checker) stmt(s ast.Stmt) { // TODO(gri) If a builtin is called, the builtin must be valid this context. case *ast.ReturnStmt: - sig := check.funcsig + sig := check.funcSig if n := sig.results.Len(); n > 0 { // determine if the function has named results named := false diff --git a/go/types/testdata/decls2b.src b/go/types/testdata/decls2b.src index f0e0351efbd..e7d9c76da99 100644 --- a/go/types/testdata/decls2b.src +++ b/go/types/testdata/decls2b.src @@ -41,3 +41,13 @@ func f_double /* ERROR "redeclared" */ () {} func (T /* ERROR "undeclared" */ ) _() {} func (T1) _(undeclared /* ERROR "undeclared" */ ) {} func (T1) _() int { return "foo" /* ERROR "cannot convert" */ } + +// Methods with undeclared receiver type can still be checked. +// Verify by checking that errors are reported. +func (Foo /* ERROR "undeclared" */ ) m() {} +func (Foo /* ERROR "undeclared" */ ) m(undeclared /* ERROR "undeclared" */ ) {} +func (Foo /* ERROR "undeclared" */ ) m() int { return "foo" /* ERROR "cannot convert" */ } + +func (Foo /* ERROR "undeclared" */ ) _() {} +func (Foo /* ERROR "undeclared" */ ) _(undeclared /* ERROR "undeclared" */ ) {} +func (Foo /* ERROR "undeclared" */ ) _() int { return "foo" /* ERROR "cannot convert" */ } diff --git a/go/types/typexpr.go b/go/types/typexpr.go index dbb56ba5794..e649be18091 100644 --- a/go/types/typexpr.go +++ b/go/types/typexpr.go @@ -40,7 +40,7 @@ func (check *checker) ident(x *operand, e *ast.Ident, def *Named, cycleOk bool) check.dump("%s: %s should have been declared (we are inside a function)", e.Pos(), e) unreachable() } - check.declareObject(obj, def, cycleOk) + check.objDecl(obj, def, cycleOk) typ = obj.Type() } assert(typ != nil)