exp/types: enable cycle checks again

Process a package's object in a reproducible
order (rather then in map order) so that we
get error messages in reproducible order.

R=r
CC=golang-dev
https://golang.org/cl/6449076

src/pkg/exp/types/check.go

@@ -30,6 +30,7 @@ func (c *checker) errorf(pos token.Pos, format string, args ...interface{}) stri
 
 // collectFields collects struct fields tok = token.STRUCT), interface methods
 // (tok = token.INTERFACE), and function arguments/results (tok = token.FUNC).
+//
 func (c *checker) collectFields(tok token.Token, list *ast.FieldList, cycleOk bool) (fields ObjList, tags []string, isVariadic bool) {
 	if list != nil {
 		for _, field := range list.List {
@@ -104,7 +105,7 @@ func (c *checker) makeType(x ast.Expr, cycleOk bool) (typ Type) {
 		obj := t.Obj
 		if obj == nil {
 			// unresolved identifier (error has been reported before)
-			return &Bad{Msg: "unresolved identifier"}
+			return &Bad{Msg: fmt.Sprintf("%s is unresolved", t.Name)}
 		}
 		if obj.Kind != ast.Typ {
 			msg := c.errorf(t.Pos(), "%s is not a type", t.Name)
@@ -112,10 +113,7 @@ func (c *checker) makeType(x ast.Expr, cycleOk bool) (typ Type) {
 		}
 		c.checkObj(obj, cycleOk)
 		if !cycleOk && obj.Type.(*Name).Underlying == nil {
-			// TODO(gri) Enable this message again once its position
-			// is independent of the underlying map implementation.
-			// msg := c.errorf(obj.Pos(), "illegal cycle in declaration of %s", obj.Name)
-			msg := "illegal cycle"
+			msg := c.errorf(obj.Pos(), "illegal cycle in declaration of %s", obj.Name)
 			return &Bad{Msg: msg}
 		}
 		return obj.Type.(Type)
@@ -227,11 +225,25 @@ func (c *checker) checkObj(obj *ast.Object, ref bool) {
 // there are errors.
 //
 func Check(fset *token.FileSet, pkg *ast.Package) (types map[ast.Expr]Type, err error) {
+	// Sort objects so that we get reproducible error
+	// positions (this is only needed for testing).
+	// TODO(gri): Consider ast.Scope implementation that
+	// provides both a list and a map for fast lookup.
+	// Would permit the use of scopes instead of ObjMaps
+	// elsewhere.
+	list := make(ObjList, len(pkg.Scope.Objects))
+	i := 0
+	for _, obj := range pkg.Scope.Objects {
+		list[i] = obj
+		i++
+	}
+	list.Sort()
+
 	var c checker
 	c.fset = fset
 	c.types = make(map[ast.Expr]Type)
 
-	for _, obj := range pkg.Scope.Objects {
+	for _, obj := range list {
 		c.checkObj(obj, false)
 	}
 

src/pkg/exp/types/testdata/test0.src

@@ -44,15 +44,15 @@ type (
 type (
 	Pi pi /* ERROR "not a type" */
 
-	a /* DISABLED "illegal cycle" */ a
+	a /* ERROR "illegal cycle" */ a
 	a /* ERROR "redeclared" */ int
 
 	// where the cycle error appears depends on the
 	// order in which declarations are processed
 	// (which depends on the order in which a map
 	// is iterated through)
-	b c
-	c /* DISABLED "illegal cycle" */ d
+	b /* ERROR "illegal cycle" */ c
+	c d
 	d e
 	e b
 
@@ -79,13 +79,13 @@ type (
 	S3 struct {
 		x S2
 	}
-	S4/* DISABLED "illegal cycle" */ struct {
+	S4/* ERROR "illegal cycle" */ struct {
 		S4
 	}
-	S5 struct {
+	S5 /* ERROR "illegal cycle" */ struct {
 		S6
 	}
-	S6 /* DISABLED "illegal cycle" */ struct {
+	S6 struct {
 		field S7
 	}
 	S7 struct {
@@ -96,8 +96,8 @@ type (
 	L2 []int
 
 	A1 [10]int
-	A2 /* DISABLED "illegal cycle" */ [10]A2
-	A3 /* DISABLED "illegal cycle" */ [10]struct {
+	A2 /* ERROR "illegal cycle" */ [10]A2
+	A3 /* ERROR "illegal cycle" */ [10]struct {
 		x A4
 	}
 	A4 [10]A3
@@ -132,17 +132,21 @@ type (
 		I1
 		I1
 	}
-	I8 /* DISABLED "illegal cycle" */ interface {
+	I8 /* ERROR "illegal cycle" */ interface {
 		I8
 	}
-	I9 /* DISABLED "illegal cycle" */ interface {
+	// 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 {
 		I10
 	}
 	I10 interface {
 		I11
 	}
 	I11 interface {
-		I9
+		I09
 	}
 
 	C1 chan int