go/types: use color-marking based cycle detection at package level

The existing cycle detection scheme passes around a (type name)
path; when a type name re-appears in the path, a cycle is reported.
Indirections (as in *T, func(T), etc.) are broken by starting a new
(nil) path. The problem with this approach is that it doesn't work
for cycles involving alias type names since they may be invalid
even if there is an indirection. Furthermore, the path must be
passed around through all functions which is currently not the
case, which leads to less optimial error reporting.

The new code is using the previously introduced color marking
scheme and global object path for package-level cycle detection
(function-local cycle detection doesn't use the same code path
yet but is also much less important as cycles can only be created
using the type being declared).

The new code is guarded with an internal flag (useCycleMarking)
so it can be disabled in short notice if this change introduced
unexpected new issues.

Fixes #23139.
Fixes #25141.

For #18640.
For #24939.

Change-Id: I1bbf2d2d61a375cf5885b2de1df0a9819d63e5fa
Reviewed-on: https://go-review.googlesource.com/115455
Reviewed-by: Alan Donovan <[email protected]>

Author: griesemer

src/go/types/decl.go

@@ -49,6 +49,13 @@ func pathString(path []*TypeName) string {
 	return s
 }
 
+// useCycleMarking enables the new coloring-based cycle marking scheme
+// for package-level objects. Set this flag to false to disable this
+// code quickly and revert to the existing mechanism (and comment out
+// some of the new tests in cycles5.src that will fail again).
+// TODO(gri) remove this for Go 1.12
+const useCycleMarking = true
+
 // objDecl type-checks the declaration of obj in its respective (file) context.
 // See check.typ for the details on def and path.
 func (check *Checker) objDecl(obj Object, def *Named, path []*TypeName) {
@@ -117,12 +124,32 @@ func (check *Checker) objDecl(obj Object, def *Named, path []*TypeName) {
 		switch obj := obj.(type) {
 		case *Const:
 			visited = obj.visited
+
 		case *Var:
 			visited = obj.visited
-		default:
+
+		case *TypeName:
+			assert(obj.Type() != nil)
+			if useCycleMarking {
+				check.typeCycle(obj)
+			}
+			return
+
+		case *Func:
+			// Cycles involving functions require variables in
+			// the cycle; they are pretty esoteric. For now we
+			// handle this as before (for grey functions, the
+			// function type is set to an empty signature which
+			// makes it impossible to initialize a variable with
+			// the function).
 			assert(obj.Type() != nil)
 			return
+
+		default:
+			unreachable()
 		}
+
+		// we have a *Const or *Var
 		if obj.Type() != nil {
 			return
 		}
@@ -176,6 +203,60 @@ func (check *Checker) objDecl(obj Object, def *Named, path []*TypeName) {
 	}
 }
 
+// indir is a sentinel type name that is pushed onto the object path
+// to indicate an "indirection" in the dependency from one type name
+// to the next. For instance, for "type p *p" the object path contains
+// p followed by indir, indicating that there's an indirection *p.
+// Indirections are used to break type cycles.
+var indir = new(TypeName)
+
+// typeCycle checks if the cycle starting with obj is valid and
+// reports an error if it is not.
+func (check *Checker) typeCycle(obj *TypeName) {
+	d := check.objMap[obj]
+	if d == nil {
+		check.dump("%v: %s should have been declared", obj.Pos(), obj)
+		unreachable()
+	}
+
+	// A cycle must have at least one indirection and one defined
+	// type to be permitted: If there is no indirection, the size
+	// of the type cannot be computed (it's either infinite or 0);
+	// if there is no defined type, we have a sequence of alias
+	// type names which will expand ad infinitum.
+	var hasIndir, hasDefType bool
+	assert(obj.color() >= grey)
+	start := obj.color() - grey // index of obj in objPath
+	cycle := check.objPath[start:]
+	for _, obj := range cycle {
+		// Cycles may contain various objects; for now only look at type names.
+		if tname, _ := obj.(*TypeName); tname != nil {
+			if tname == indir {
+				hasIndir = true
+			} else if !check.objMap[tname].alias {
+				hasDefType = true
+			}
+			if hasIndir && hasDefType {
+				return // cycle is permitted
+			}
+		}
+	}
+
+	// break cycle
+	// (without this, calling underlying() below may lead to an endless loop)
+	obj.typ = Typ[Invalid]
+
+	// report cycle
+	check.errorf(obj.Pos(), "illegal cycle in declaration of %s", obj.Name())
+	for _, obj := range cycle {
+		if obj == indir {
+			continue // don't print indir sentinels
+		}
+		check.errorf(obj.Pos(), "\t%s refers to", obj.Name()) // secondary error, \t indented
+	}
+	check.errorf(obj.Pos(), "\t%s", obj.Name())
+}
+
 func (check *Checker) constDecl(obj *Const, typ, init ast.Expr) {
 	assert(obj.typ == nil)
 
@@ -353,7 +434,7 @@ func (check *Checker) addMethodDecls(obj *TypeName) {
 		return
 	}
 	delete(check.methods, obj)
-	assert(!obj.IsAlias())
+	assert(!check.objMap[obj].alias) // don't use TypeName.IsAlias (requires fully set up object)
 
 	// use an objset to check for name conflicts
 	var mset objset

src/go/types/testdata/cycles5.src

@@ -97,12 +97,12 @@ var _ = err.Error()
 // more esoteric cases
 
 type (
-	T1 interface { T2 /* ERROR not an interface */ }
+	T1 interface { T2 }
 	T2 /* ERROR cycle */ T2
 )
 
 type (
-	T3 interface { T4 /* ERROR not an interface */ }
+	T3 interface { T4 }
 	T4 /* ERROR cycle */ T5
 	T5 = T6
 	T6 = T7
@@ -117,3 +117,38 @@ const n = unsafe.Sizeof(func(){})
 type I interface {
 	m([unsafe.Sizeof(func() { I.m(nil, [n]byte{}) })]byte)
 }
+
+
+// test cases for varias alias cycles
+
+type T10 /* ERROR cycle */ = *T10                 // issue #25141
+type T11 /* ERROR cycle */ = interface{ f(T11) }  // issue #23139
+
+// issue #18640
+type (
+	aa = bb
+	bb struct {
+		*aa
+	}
+)
+
+type (
+	a struct{ *b }
+	b = c
+	c struct{ *b }
+)
+
+// issue #24939
+type (
+	_ interface {
+		M(P)
+	}
+
+	M interface {
+		F() P
+	}
+
+	P = interface {
+		I() M
+	}
+)

src/go/types/typexpr.go

@@ -71,6 +71,12 @@ func (check *Checker) ident(x *operand, e *ast.Ident, def *Named, path []*TypeNa
 
 	case *TypeName:
 		x.mode = typexpr
+		// package-level alias cycles are now checked by Checker.objDecl
+		if useCycleMarking {
+			if check.objMap[obj] != nil {
+				break
+			}
+		}
 		if check.cycle(obj, path, true) {
 			// maintain x.mode == typexpr despite error
 			typ = Typ[Invalid]
@@ -132,7 +138,11 @@ func (check *Checker) cycle(obj *TypeName, path []*TypeName, report bool) bool {
 // If def != nil, e is the type specification for the named type def, declared
 // in a type declaration, and def.underlying will be set to the type of e before
 // any components of e are type-checked. Path contains the path of named types
-// referring to this type.
+// referring to this type; i.e. it is the path of named types directly containing
+// each other and leading to the current type e. Indirect containment (e.g. via
+// pointer indirection, function parameter, etc.) breaks the path (leads to a new
+// path, and usually via calling Checker.typ below) and those types are not found
+// in the path.
 //
 func (check *Checker) typExpr(e ast.Expr, def *Named, path []*TypeName) (T Type) {
 	if trace {
@@ -152,6 +162,12 @@ func (check *Checker) typExpr(e ast.Expr, def *Named, path []*TypeName) (T Type)
 }
 
 func (check *Checker) typ(e ast.Expr) Type {
+	// typExpr is called with a nil path indicating an indirection:
+	// push indir sentinel on object path
+	if useCycleMarking {
+		check.push(indir)
+		defer check.pop()
+	}
 	return check.typExpr(e, nil, nil)
 }