From 91159eb2145da3b9086b9363ec7cd38e6bc86519 Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Fri, 31 Jan 2014 15:12:05 -0800 Subject: [PATCH] go.tools/go/types: don't type-check methods twice - Centralize "visited" check in check.objDecl. - Assert that object-specific declX functions are only called with objects that have no type associated with them, yet. - Added test case. Thanks to Richard Musiol (neelance@gmail.com) for finding the bug and providing an easy test case. For a discussion of the bug see the issue. Fixes golang/go#7245. TBR=adonovan R=adonovan CC=golang-codereviews https://golang.org/cl/59210043 --- go/types/decl.go | 37 ++++++++++++++++++++++++++++++++++++- go/types/issues_test.go | 27 +++++++++++++++++++++++++++ go/types/resolver.go | 7 +------ go/types/typexpr.go | 10 +--------- 4 files changed, 65 insertions(+), 16 deletions(-) diff --git a/go/types/decl.go b/go/types/decl.go index 47eb1e0aca3..12193b7b51b 100644 --- a/go/types/decl.go +++ b/go/types/decl.go @@ -34,7 +34,23 @@ func (check *checker) declare(scope *Scope, id *ast.Ident, obj Object) { // 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, cycle []*TypeName) { + if obj.Type() != nil { + return // already checked - nothing to do + } + + if trace { + check.trace(obj.Pos(), "-- resolving %s", obj.Name()) + } + d := check.objMap[obj] + if debug && d == nil { + if check.objMap == nil { + check.dump("%s: %s should have been declared (we are inside a function)", obj.Pos(), obj) + unreachable() + } + check.dump("%s: %s should have been forward-declared", obj.Pos(), obj) + unreachable() + } // adjust file scope for current object oldScope := check.topScope @@ -68,6 +84,8 @@ func (check *checker) objDecl(obj Object, def *Named, cycle []*TypeName) { } func (check *checker) constDecl(obj *Const, typ, init ast.Expr) { + assert(obj.typ == nil) + // TODO(gri) consider using the same cycle detection as for types // so that we can print the actual cycle in case of an error if obj.visited { @@ -105,6 +123,8 @@ func (check *checker) constDecl(obj *Const, typ, init ast.Expr) { // TODO(gri) document arguments func (check *checker) varDecl(obj *Var, lhs []*Var, typ, init ast.Expr) { + assert(obj.typ == nil) + // TODO(gri) consider using the same cycle detection as for types // so that we can print the actual cycle in case of an error if obj.visited { @@ -176,7 +196,7 @@ func (n *Named) setUnderlying(typ Type) { } func (check *checker) typeDecl(obj *TypeName, typ ast.Expr, def *Named, cycle []*TypeName) { - assert(obj.Type() == nil) + assert(obj.typ == nil) // type declarations cannot use iota assert(check.iota == nil) @@ -262,6 +282,8 @@ type funcInfo struct { } func (check *checker) funcDecl(obj *Func, info *declInfo) { + assert(obj.typ == nil) + // func declarations cannot use iota assert(check.iota == nil) @@ -347,6 +369,19 @@ func (check *checker) declStmt(decl ast.Decl) { } } check.varDecl(obj, lhs, s.Type, init) + if len(s.Values) == 1 { + // If we have a single lhs variable we are done either way. + // If we have a single rhs expression, it must be a multi- + // valued expression, in which case handling the first lhs + // variable will cause all lhs variables to have a type + // assigned, and we are done as well. + if debug { + for _, obj := range lhs0 { + assert(obj.typ != nil) + } + } + break + } } check.arityMatch(s, nil) diff --git a/go/types/issues_test.go b/go/types/issues_test.go index e61f0eff0d6..8bca1931f1e 100644 --- a/go/types/issues_test.go +++ b/go/types/issues_test.go @@ -117,3 +117,30 @@ func f() int { t.Errorf("got %d CallExprs; want 2", n) } } + +func TestIssue7245(t *testing.T) { + src := ` +package p +func (T) m() (res bool) { return } +type T struct{} // receiver type after method declaration +` + f, err := parser.ParseFile(fset, "", src, 0) + if err != nil { + t.Fatal(err) + } + + var conf Config + objects := make(map[*ast.Ident]Object) + _, err = conf.Check(f.Name.Name, fset, []*ast.File{f}, &Info{Objects: objects}) + if err != nil { + t.Fatal(err) + } + + m := f.Decls[0].(*ast.FuncDecl) + res1 := objects[m.Name].(*Func).Type().(*Signature).Results().At(0) + res2 := objects[m.Type.Results.List[0].Names[0]].(*Var) + + if res1 != res2 { + t.Errorf("got %s (%p) != %s (%p)", res1, res2, res1, res2) + } +} diff --git a/go/types/resolver.go b/go/types/resolver.go index 054d7ebe339..c8f2e9cd350 100644 --- a/go/types/resolver.go +++ b/go/types/resolver.go @@ -358,12 +358,7 @@ func (check *checker) resolveFiles(files []*ast.File) { check.objMap = objMap // indicate that we are checking package-level declarations (objects may not have a type yet) emptyCycle := make([]*TypeName, 0, 8) // re-use the same underlying array for cycle detection for _, obj := range objectsOf(check.objMap) { - if obj.Type() == nil { - if trace { - check.trace(obj.Pos(), "-- resolving %s", obj.Name()) - } - check.objDecl(obj, nil, emptyCycle) - } + check.objDecl(obj, nil, emptyCycle) } emptyCycle = nil // not needed anymore check.objMap = nil // not needed anymore diff --git a/go/types/typexpr.go b/go/types/typexpr.go index 872c4635411..e3f1fa0efca 100644 --- a/go/types/typexpr.go +++ b/go/types/typexpr.go @@ -34,16 +34,8 @@ func (check *checker) ident(x *operand, e *ast.Ident, def *Named, cycle []*TypeN } check.recordObject(e, obj) + check.objDecl(obj, def, cycle) typ := obj.Type() - if typ == nil { - // object type not yet determined - if check.objMap == nil { - check.dump("%s: %s should have been declared (we are inside a function)", e.Pos(), e) - unreachable() - } - check.objDecl(obj, def, cycle) - typ = obj.Type() - } assert(typ != nil) switch obj := obj.(type) {