From 0b23073b5c92207753940c0e89009a2bebbb9970 Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Wed, 2 Apr 2014 11:42:29 -0700 Subject: [PATCH] go.tools/go/types: type-check interfaces in reverse dependency order Side-effect: Because interfaces are now type-checked in reverse order, cycle errors in interface declarations appear at the "end" rather than at the "beginning" of the cycle in the source code. This is harmless. Eventually we may want to do dependency order determination and thus cycle detection for all types before fully type-checking them, which might simplify some code and also produce consistently positioned cycle errors again. Fixes golang/go#7158. LGTM=adonovan R=adonovan CC=golang-codereviews https://golang.org/cl/83640043 --- go/types/check.go | 14 +---- go/types/decl.go | 2 +- go/types/ordering.go | 108 ++++++++++++++++++++++++++++++++++ go/types/resolver.go | 13 ++-- go/types/testdata/cycles.src | 4 +- go/types/testdata/cycles4.src | 42 +++++++++++++ go/types/testdata/decls0.src | 10 +--- 7 files changed, 161 insertions(+), 32 deletions(-) create mode 100644 go/types/ordering.go diff --git a/go/types/check.go b/go/types/check.go index 3ad2889c..15eae5f2 100644 --- a/go/types/check.go +++ b/go/types/check.go @@ -10,7 +10,6 @@ import ( "fmt" "go/ast" "go/token" - "sort" "code.google.com/p/go.tools/go/exact" ) @@ -200,17 +199,6 @@ func (check *checker) handleBailout(err *error) { } } -func mapObjects(m map[Object]*declInfo) []Object { - list := make([]Object, len(m)) - i := 0 - for obj := range m { - list[i] = obj - i++ - } - sort.Sort(inSourceOrder(list)) - return list -} - // Files checks the provided files as part of the checker's package. func (check *checker) Files(files []*ast.File) (err error) { defer check.handleBailout(&err) @@ -219,7 +207,7 @@ func (check *checker) Files(files []*ast.File) (err error) { check.collectObjects() - objList := mapObjects(check.objMap) + objList := check.resolveOrder() check.packageObjects(objList) diff --git a/go/types/decl.go b/go/types/decl.go index a81feb90..8ef003d4 100644 --- a/go/types/decl.go +++ b/go/types/decl.go @@ -50,7 +50,7 @@ func (check *checker) objDecl(obj Object, def *Named, path []*TypeName) { d := check.objMap[obj] if d == nil { - check.dump("%s: %s should have been declared", obj.Pos(), obj) + check.dump("%s: %s should have been declared", obj.Pos(), obj.Name()) unreachable() } diff --git a/go/types/ordering.go b/go/types/ordering.go new file mode 100644 index 00000000..cc38cfed --- /dev/null +++ b/go/types/ordering.go @@ -0,0 +1,108 @@ +// Copyright 2014 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// This file implements resolveOrder. + +package types + +import ( + "go/ast" + "sort" +) + +// resolveOrder computes the order in which package-level objects +// must be type-checked. +// +// Interface types appear first in the list, sorted topologically +// by dependencies on embedded interfaces that are also declared +// in this package, followed by all other objects sorted in source +// order. +// +// TODO(gri) Consider sorting all types by dependencies here, and +// in the process check _and_ report type cycles. This may simplify +// the full type-checking phase. +// +func (check *checker) resolveOrder() []Object { + var ifaces, others []Object + + // collect interface types with their dependencies, and all other objects + for obj := range check.objMap { + if ityp := check.interfaceFor(obj); ityp != nil { + ifaces = append(ifaces, obj) + // determine dependencies on embedded interfaces + for _, f := range ityp.Methods.List { + if len(f.Names) == 0 { + // Embedded interface: The type must be a (possibly + // qualified) identifer denoting another interface. + // Imported interfaces are already fully resolved, + // so we can ignore qualified identifiers. + if ident, _ := f.Type.(*ast.Ident); ident != nil { + embedded := check.pkg.scope.Lookup(ident.Name) + if check.interfaceFor(embedded) != nil { + check.objMap[obj].addDep(embedded) + } + } + } + } + } else { + others = append(others, obj) + } + } + + // final object order + var order []Object + + // sort interface types topologically by dependencies, + // and in source order if there are no dependencies + sort.Sort(inSourceOrder(ifaces)) + if debug { + for _, obj := range ifaces { + assert(check.objMap[obj].mark == 0) + } + } + for _, obj := range ifaces { + check.appendInPostOrder(&order, obj) + } + + // sort everything else in source order + sort.Sort(inSourceOrder(others)) + + return append(order, others...) +} + +// interfaceFor returns the AST interface denoted by obj, or nil. +func (check *checker) interfaceFor(obj Object) *ast.InterfaceType { + tname, _ := obj.(*TypeName) + if tname == nil { + return nil // not a type + } + d := check.objMap[obj] + if d == nil { + check.dump("%s: %s should have been declared", obj.Pos(), obj.Name()) + unreachable() + } + if d.typ == nil { + return nil // invalid AST - ignore (will be handled later) + } + ityp, _ := d.typ.(*ast.InterfaceType) + return ityp +} + +func (check *checker) appendInPostOrder(order *[]Object, obj Object) { + d := check.objMap[obj] + if d.mark != 0 { + // We've already seen this object; either because it's + // already added to order, or because we have a cycle. + // In both cases we stop. Cycle errors are reported + // when type-checking types. + return + } + d.mark = 1 + + for _, obj := range orderedSetObjects(d.deps) { + check.appendInPostOrder(order, obj) + } + + *order = append(*order, obj) +} diff --git a/go/types/resolver.go b/go/types/resolver.go index 18f9b8b1..90217f1c 100644 --- a/go/types/resolver.go +++ b/go/types/resolver.go @@ -25,8 +25,8 @@ type declInfo struct { init ast.Expr // init expression, or nil fdecl *ast.FuncDecl // func declaration, or nil - deps map[Object]bool // init dependencies; lazily allocated - mark int // see check.dependencies + deps map[Object]bool // type and init dependencies; lazily allocated + mark int // for dependency analysis } // hasInitializer reports whether the declared object has an initialization @@ -437,7 +437,7 @@ func (check *checker) unusedImports() { } } -func setObjects(set map[Object]bool) []Object { +func orderedSetObjects(set map[Object]bool) []Object { list := make([]Object, len(set)) i := 0 for obj := range set { @@ -449,11 +449,6 @@ func setObjects(set map[Object]bool) []Object { return list } -// TODO(gri) Instead of this support function, introduce type -// that combines an map[Object]*declInfo and []Object so that -// we can encapsulate this and don't have to depend on correct -// Pos() information. - // inSourceOrder implements the sort.Sort interface. type inSourceOrder []Object @@ -526,7 +521,7 @@ func (check *checker) dependencies(obj Object, path []Object) { path = append(path, obj) // len(path) > 0 init.mark = len(path) // init.mark > 0 - for _, obj := range setObjects(init.deps) { + for _, obj := range orderedSetObjects(init.deps) { check.dependencies(obj, path) } init.mark = -1 // init.mark < 0 diff --git a/go/types/testdata/cycles.src b/go/types/testdata/cycles.src index 2a6a3ec9..621d83c9 100644 --- a/go/types/testdata/cycles.src +++ b/go/types/testdata/cycles.src @@ -52,9 +52,9 @@ type ( // interfaces I0 /* ERROR cycle */ interface{ I0 } - I1 /* ERROR cycle */ interface{ I2 } + I1 interface{ I2 } I2 interface{ I3 } - I3 interface{ I1 } + I3 /* ERROR cycle */ interface{ I1 } I4 interface{ f(I4) } diff --git a/go/types/testdata/cycles4.src b/go/types/testdata/cycles4.src index 528f2057..445babca 100644 --- a/go/types/testdata/cycles4.src +++ b/go/types/testdata/cycles4.src @@ -66,3 +66,45 @@ type T4 interface { func _(x T1, y T3) { x = y } + +// Check that interfaces are type-checked in order of +// (embedded interface) dependencies (was issue 7158). + +var x1 T5 = T7(nil) + +type T5 interface { + T6 +} + +type T6 interface { + m() T7 +} +type T7 interface { + T5 +} + +// Actual test case from issue 7158. + +func wrapNode() Node { + return wrapElement() +} + +func wrapElement() Element { + return nil +} + +type EventTarget interface { + AddEventListener(Event) +} + +type Node interface { + EventTarget +} + +type Element interface { + Node +} + +type Event interface { + Target() Element +} diff --git a/go/types/testdata/decls0.src b/go/types/testdata/decls0.src index 5ab15b4f..9da75b90 100644 --- a/go/types/testdata/decls0.src +++ b/go/types/testdata/decls0.src @@ -155,18 +155,14 @@ type ( I8 /* ERROR "illegal cycle" */ interface { I8 } - // Use I09 (rather than I9) because it appears lexically before - // I10 so that we get the illegal cycle here rather then in the - // declaration of I10. If the implementation sorts by position - // rather than name, the error message will still be here. - I09 /* ERROR "illegal cycle" */ interface { + I9 interface { I10 } I10 interface { I11 } - I11 interface { - I09 + I11 /* ERROR "illegal cycle" */ interface { + I9 } C1 chan int