go/types: fix cycle detection

For Go 1.13, we rewrote the go/types cycle detection scheme. Unfortunately,
it was a bit too clever and introduced a bug (#34333). Here's an example:

type A struct {
	f1 *B
	f2 B
}

type B A

When type-checking this code, the first cycle A->*B->B->A (via field f1)
is ok because there's a pointer indirection. Though in the process B is
considered "type-checked" (and painted/marked from "grey" to black").
When type-checking f2, since B is already completely set up, go/types
doesn't complain about the invalid cycle A->B->A (via field f2) anymore.
On the other hand, with the fields f1, f2 swapped:

type A struct {
	f2 B
	f1 *B
}

go/types reports an error because the cycle A->B->A is type-checked first.
In general, we cannot know the "right" order in which types need to be
type-checked.

This CL fixes the issue as follows:

1) The global object path cycle detection does not take (pointer, function,
   reference type) indirections into account anymore for cycle detection.
   That mechanism was incorrect to start with and the primary cause for this
   issue. As a consequence we don't need Checker.indirectType and indir anymore.

2) After processing type declarations, Checker.validType is called to
   verify that a type doesn't expand indefinitively. This corresponds
   essentially to cmd/compile's dowidth computation (without size computation).

3) Cycles involving only defined types (e.g.: type (A B; B C; C A))
   require separate attention as those must now be detected when resolving
   "forward chains" of type declarations. Checker.underlying was changed
   to detect these cycles.

All three cycle detection mechanism use an object path ([]Object) to
report cycles. The cycle error reporting mechanism is now factored out
into Checker.cycleError and used by all three mechanisms. It also makes
an attempt to report the cycle starting with the "first" (earliest in the
source) object.

Fixes #34333.

Change-Id: I2c6446445e47344cc2cd034d3c74b1c345b8c1e6
Reviewed-on: https://go-review.googlesource.com/c/go/+/196338
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>

src/go/types/expr.go

@@ -1157,12 +1157,9 @@ func (check *Checker) exprInternal(x *operand, e ast.Expr, hint Type) exprKind {
 			}
 
 		case *Array:
-			// Prevent crash if the array referred to is not yet set up.
-			// This is a stop-gap solution; a better approach would use the mechanism of
-			// Checker.ident (typexpr.go) using a path of types. But that would require
-			// passing the path everywhere (all expression-checking methods, not just
-			// type expression checking), and we're not set up for that (quite possibly
-			// an indication that cycle detection needs to be rethought). Was issue #18643.
+			// Prevent crash if the array referred to is not yet set up. Was issue #18643.
+			// This is a stop-gap solution. Should use Checker.objPath to report entire
+			// path starting with earliest declaration in the source. TODO(gri) fix this.
 			if utyp.elem == nil {
 				check.error(e.Pos(), "illegal cycle in type declaration")
 				goto Error

src/go/types/testdata/cycles.src

@@ -23,8 +23,10 @@ type (
 	A0 /* ERROR cycle */ [10]A0
 	A1 [10]*A1
 
-	A2 /* ERROR cycle */ [10]A3
-	A3 [10]A4
+	// TODO(gri) It would be nicer to report the cycle starting
+	//           with A2 (also below, for S4). See issue #34771.
+	A2 [10]A3
+	A3 /* ERROR cycle */ [10]A4
 	A4 A2
 
 	A5 [10]A6
@@ -39,8 +41,8 @@ type (
 	S2 struct{ _ *S2 }
 	S3 struct{ *S3 }
 
-	S4 /* ERROR cycle */ struct{ S5 }
-	S5 struct{ S6 }
+	S4 struct{ S5 }
+	S5 /* ERROR cycle */ struct{ S6 }
 	S6 S4
 
 	// pointers
@@ -147,7 +149,7 @@ type (
 // test cases for issue 18643
 // (type cycle detection when non-type expressions are involved)
 type (
-	T14 /* ERROR cycle */ [len(T14{})]int
+	T14 [len(T14 /* ERROR cycle */ {})]int
 	T15 [][len(T15 /* ERROR cycle */ {})]int
 	T16 map[[len(T16 /* ERROR cycle */ {1:2})]int]int
 	T17 map[int][len(T17 /* ERROR cycle */ {1:2})]int

src/go/types/testdata/cycles5.src

@@ -188,3 +188,13 @@ func h() [h /* ERROR no value */ ()[0]]int { panic(0) }
 
 var c14 /* ERROR cycle */ T14
 type T14 [uintptr(unsafe.Sizeof(&c14))]byte
+
+// issue #34333
+type T15 /* ERROR cycle */ struct {
+	f func() T16
+	b T16
+}
+
+type T16 struct {
+	T15
+}
\ No newline at end of file

src/go/types/testdata/decls0.src

@@ -72,10 +72,6 @@ type (
 	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 /* ERROR "illegal cycle" */ c
 	c d
 	d e

src/go/types/type.go

@@ -448,6 +448,7 @@ func (c *Chan) Elem() Type { return c.elem }
 
 // A Named represents a named type.
 type Named struct {
+	info       typeInfo  // for cycle detection
 	obj        *TypeName // corresponding declared object
 	underlying Type      // possibly a *Named during setup; never a *Named once set up completely
 	methods    []*Func   // methods declared for this type (not the method set of this type); signatures are type-checked lazily

src/go/types/typexpr.go

@@ -142,16 +142,6 @@ func (check *Checker) definedType(e ast.Expr, def *Named) (T Type) {
 	return
 }
 
-// indirectType is like typ but it also breaks the (otherwise) infinite size of recursive
-// types by introducing an indirection. It should be called for components of types that
-// are not laid out in place in memory, such as pointer base types, slice or map element
-// types, function parameter types, etc.
-func (check *Checker) indirectType(e ast.Expr) Type {
-	check.push(indir)
-	defer check.pop()
-	return check.definedType(e, nil)
-}
-
 // funcType type-checks a function or method type.
 func (check *Checker) funcType(sig *Signature, recvPar *ast.FieldList, ftyp *ast.FuncType) {
 	scope := NewScope(check.scope, token.NoPos, token.NoPos, "function")
@@ -273,7 +263,7 @@ func (check *Checker) typInternal(e ast.Expr, def *Named) Type {
 		} else {
 			typ := new(Slice)
 			def.setUnderlying(typ)
-			typ.elem = check.indirectType(e.Elt)
+			typ.elem = check.typ(e.Elt)
 			return typ
 		}
 
@@ -286,7 +276,7 @@ func (check *Checker) typInternal(e ast.Expr, def *Named) Type {
 	case *ast.StarExpr:
 		typ := new(Pointer)
 		def.setUnderlying(typ)
-		typ.base = check.indirectType(e.X)
+		typ.base = check.typ(e.X)
 		return typ
 
 	case *ast.FuncType:
@@ -305,8 +295,8 @@ func (check *Checker) typInternal(e ast.Expr, def *Named) Type {
 		typ := new(Map)
 		def.setUnderlying(typ)
 
-		typ.key = check.indirectType(e.Key)
-		typ.elem = check.indirectType(e.Value)
+		typ.key = check.typ(e.Key)
+		typ.elem = check.typ(e.Value)
 
 		// spec: "The comparison operators == and != must be fully defined
 		// for operands of the key type; thus the key type must not be a
@@ -340,7 +330,7 @@ func (check *Checker) typInternal(e ast.Expr, def *Named) Type {
 		}
 
 		typ.dir = dir
-		typ.elem = check.indirectType(e.Value)
+		typ.elem = check.typ(e.Value)
 		return typ
 
 	default:
@@ -421,7 +411,7 @@ func (check *Checker) collectParams(scope *Scope, list *ast.FieldList, variadicO
 				// ignore ... and continue
 			}
 		}
-		typ := check.indirectType(ftype)
+		typ := check.typ(ftype)
 		// The parser ensures that f.Tag is nil and we don't
 		// care if a constructed AST contains a non-nil tag.
 		if len(field.Names) > 0 {
@@ -483,7 +473,7 @@ func (check *Checker) interfaceType(ityp *Interface, iface *ast.InterfaceType, d
 				continue // ignore
 			}
 
-			typ := check.indirectType(f.Type)
+			typ := check.typ(f.Type)
 			sig, _ := typ.(*Signature)
 			if sig == nil {
 				if typ != Typ[Invalid] {
@@ -508,8 +498,9 @@ func (check *Checker) interfaceType(ityp *Interface, iface *ast.InterfaceType, d
 			// it if it's a valid interface.
 			typ := check.typ(f.Type)
 
-			if _, ok := underlying(typ).(*Interface); !ok {
-				if typ != Typ[Invalid] {
+			utyp := check.underlying(typ)
+			if _, ok := utyp.(*Interface); !ok {
+				if utyp != Typ[Invalid] {
 					check.errorf(f.Type.Pos(), "%s is not an interface", typ)
 				}
 				continue
@@ -555,7 +546,12 @@ func (check *Checker) completeInterface(ityp *Interface) {
 		}()
 	}
 
-	ityp.allMethods = markComplete // avoid infinite recursion
+	// An infinitely expanding interface (due to a cycle) is detected
+	// elsewhere (Checker.validType), so here we simply assume we only
+	// have valid interfaces. Mark the interface as complete to avoid
+	// infinite recursion if the validType check occurs later for some
+	// reason.
+	ityp.allMethods = markComplete
 
 	// Methods of embedded interfaces are collected unchanged; i.e., the identity
 	// of a method I.m's Func Object of an interface I is the same as that of
@@ -599,7 +595,12 @@ func (check *Checker) completeInterface(ityp *Interface) {
 	posList := check.posMap[ityp]
 	for i, typ := range ityp.embeddeds {
 		pos := posList[i] // embedding position
-		typ := underlying(typ).(*Interface)
+		typ, ok := check.underlying(typ).(*Interface)
+		if !ok {
+			// An error was reported when collecting the embedded types.
+			// Ignore it.
+			continue
+		}
 		check.completeInterface(typ)
 		for _, m := range typ.allMethods {
 			addMethod(pos, m, false) // use embedding position pos rather than m.pos