go/types: correctly compute cycle length

The existing algorithm assumed that the length of a cycle was simply
the number of elements in the cycle slice starting at the start object.
However, we use a special "indir" indirection object to indicate
pointer and other indirections that break the inline placement of
types in composite types. These indirection objects don't exist as
true named type objects, so don't count them anymore.

This removes an unnecessary cycle error in one of the existing tests
(testdata/issues.src:100).

Also:
- added more tracing support (only active if tracing is enabled)
- better documentation in parts
- r/check.typ/check.typExpr/ in a few of places where we don't
  need to record a type indirection

Found while investigating #26124.

Change-Id: I45341743225d979a72af3fbecfa05012b32fab67
Reviewed-on: https://go-review.googlesource.com/121755
Run-TryBot: Robert Griesemer <gri@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>

src/go/types/check.go

@@ -162,14 +162,7 @@ func (check *Checker) pop() Object {
 
 // pathString returns a string of the form a->b-> ... ->g for an object path [a, b, ... g].
 func (check *Checker) pathString() string {
-	var s string
+	return objPathString(check.objPath)
-	for i, p := range check.objPath {
-		if i > 0 {
-			s += "->"
-		}
-		s += p.Name()
-	}
-	return s
 }
 
 // NewChecker returns a new Checker instance for a given package.

src/go/types/decl.go

@@ -38,6 +38,8 @@ func (check *Checker) declare(scope *Scope, id *ast.Ident, obj Object, pos token
 }
 
 // pathString returns a string of the form a->b-> ... ->g for a path [a, b, ... g].
+// TODO(gri) remove once we don't need the old cycle detection (explicitly passed
+//           []*TypeName path) anymore
 func pathString(path []*TypeName) string {
 	var s string
 	for i, p := range path {
@@ -49,6 +51,19 @@ func pathString(path []*TypeName) string {
 	return s
 }
 
+// objPathString returns a string of the form a->b-> ... ->g for a path [a, b, ... g].
+// TODO(gri) s/objPathString/pathString/ once we got rid of pathString above
+func objPathString(path []Object) string {
+	var s string
+	for i, p := range path {
+		if i > 0 {
+			s += "->"
+		}
+		s += p.Name()
+	}
+	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
@@ -224,16 +239,18 @@ var indir = NewTypeName(token.NoPos, nil, "*", nil)
 // reports an error if it is not.
 // TODO(gri) rename s/typeCycle/cycle/ once we don't need the other
 // cycle method anymore.
-func (check *Checker) typeCycle(obj Object) bool {
+func (check *Checker) typeCycle(obj Object) (isCycle bool) {
 	d := check.objMap[obj]
 	if d == nil {
 		check.dump("%v: %s should have been declared", obj.Pos(), obj)
 		unreachable()
 	}
 
-	// We distinguish between cycles involving only constants and variables
+	// Given the number of constants and variables (nval) in the cycle
-	// (nval = len(cycle)), cycles involving types (and functions) only
+	// and the cycle length (ncycle = number of named objects in the cycle),
-	// (nval == 0), and mixed cycles (nval != 0 && nval != len(cycle)).
+	// we distinguish between cycles involving only constants and variables
+	// (nval = ncycle), cycles involving types (and functions) only
+	// (nval == 0), and mixed cycles (nval != 0 && nval != ncycle).
 	// We ignore functions at the moment (taking them into account correctly
 	// is complicated and it doesn't improve error reporting significantly).
 	//
@@ -242,17 +259,19 @@ func (check *Checker) typeCycle(obj Object) bool {
 	// cannot be computed (it's either infinite or 0); if there is no type
 	// definition, we have a sequence of alias type names which will expand
 	// ad infinitum.
-	var nval int
+	var nval, ncycle int
 	var hasIndir, hasTDef bool
 	assert(obj.color() >= grey)
 	start := obj.color() - grey // index of obj in objPath
 	cycle := check.objPath[start:]
+	ncycle = len(cycle) // including indirections
 	for _, obj := range cycle {
 		switch obj := obj.(type) {
 		case *Const, *Var:
 			nval++
 		case *TypeName:
 			if obj == indir {
+				ncycle-- // don't count (indirections are not objects)
 				hasIndir = true
 			} else if !check.objMap[obj].alias {
 				hasTDef = true
@@ -264,10 +283,20 @@ func (check *Checker) typeCycle(obj Object) bool {
 		}
 	}
 
+	if trace {
+		check.trace(obj.Pos(), "## cycle detected: objPath = %s->%s (len = %d)", objPathString(cycle), obj.Name(), ncycle)
+		check.trace(obj.Pos(), "## cycle contains: %d values, has indirection = %v, has type definition = %v", nval, hasIndir, hasTDef)
+		defer func() {
+			if isCycle {
+				check.trace(obj.Pos(), "=> error: cycle is invalid")
+			}
+		}()
+	}
+
 	// A cycle involving only constants and variables is invalid but we
 	// ignore them here because they are reported via the initialization
 	// cycle check.
-	if nval == len(cycle) {
+	if nval == ncycle {
 		return false
 	}
 
@@ -305,7 +334,7 @@ func (check *Checker) constDecl(obj *Const, typ, init ast.Expr) {
 
 	// determine type, if any
 	if typ != nil {
-		t := check.typ(typ)
+		t := check.typExpr(typ, nil, nil)
 		if !isConstType(t) {
 			// don't report an error if the type is an invalid C (defined) type
 			// (issue #22090)
@@ -331,7 +360,7 @@ func (check *Checker) varDecl(obj *Var, lhs []*Var, typ, init ast.Expr) {
 
 	// determine type, if any
 	if typ != nil {
-		obj.typ = check.typ(typ)
+		obj.typ = check.typExpr(typ, nil, nil)
 		// We cannot spread the type to all lhs variables if there
 		// are more than one since that would mark them as checked
 		// (see Checker.objDecl) and the assignment of init exprs,

src/go/types/expr.go

@@ -1431,7 +1431,7 @@ func (check *Checker) exprInternal(x *operand, e ast.Expr, hint Type) exprKind {
 			check.invalidAST(e.Pos(), "use of .(type) outside type switch")
 			goto Error
 		}
-		T := check.typ(e.Type)
+		T := check.typExpr(e.Type, nil, nil)
 		if T == Typ[Invalid] {
 			goto Error
 		}

src/go/types/testdata/issues.src

@@ -97,7 +97,7 @@ func issue10979() {
 
 // issue11347
 // These should not crash.
-var a1, b1 /* ERROR cycle */ /* ERROR cycle */ , c1 /* ERROR cycle */ b1 = 0 > 0<<""[""[c1]]>c1
+var a1, b1 /* ERROR cycle */ , c1 /* ERROR cycle */ b1 = 0 > 0<<""[""[c1]]>c1
 var a2, b2 /* ERROR cycle */ = 0 /* ERROR cannot initialize */ /* ERROR cannot initialize */ > 0<<""[b2]
 var a3, b3 /* ERROR cycle */ = int /* ERROR cannot initialize */ /* ERROR cannot initialize */ (1<<""[b3])