go/types: track local cycles using same mechanism as for global objects

For Go 1.11, cycle tracking of global (package-level) objects was changed
to use a Checker-level object path rather than relying on the explicit
path parameter that is passed around to some (but not all) type-checker
functions.

This change now uses the same mechanism for the detection of local
type cycles (local non-type objects cannot create cycles by definition
of the spec).

As a result, local alias cycles are now correctly detected as well
(issue #27106).

The path parameter that is explicitly passed around to some type-checker
methods is still present and will be removed in a follow-up CL.

Also:
- removed useCycleMarking flag and respective dead code
- added a couple more tests
- improved documentation

Fixes #27106.
Updates #25773.

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

src/go/types/check.go

@@ -76,7 +76,7 @@ type Checker struct {
 	fset *token.FileSet
 	pkg  *Package
 	*Info
-	objMap map[Object]*declInfo   // maps package-level object to declaration info
+	objMap map[Object]*declInfo   // maps package-level objects and (non-interface) methods to declaration info
 	impMap map[importKey]*Package // maps (import path, source directory) to (complete or fake) package
 
 	// information collected during type-checking of a set of package files

src/go/types/decl.go

@@ -64,13 +64,6 @@ func objPathString(path []Object) 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) {
@@ -147,7 +140,7 @@ func (check *Checker) objDecl(obj Object, def *Named, path []*TypeName) {
 		// order code.
 		switch obj := obj.(type) {
 		case *Const:
-			if useCycleMarking && check.typeCycle(obj) {
+			if check.typeCycle(obj) {
 				obj.typ = Typ[Invalid]
 				break
 			}
@@ -156,7 +149,7 @@ func (check *Checker) objDecl(obj Object, def *Named, path []*TypeName) {
 			}
 
 		case *Var:
-			if useCycleMarking && check.typeCycle(obj) {
+			if check.typeCycle(obj) {
 				obj.typ = Typ[Invalid]
 				break
 			}
@@ -190,7 +183,7 @@ func (check *Checker) objDecl(obj Object, def *Named, path []*TypeName) {
 				}
 			}
 
-			if useCycleMarking && check.typeCycle(obj) {
+			if check.typeCycle(obj) {
 				// break cycle
 				// (without this, calling underlying()
 				// below may lead to an endless loop
@@ -200,7 +193,7 @@ func (check *Checker) objDecl(obj Object, def *Named, path []*TypeName) {
 			}
 
 		case *Func:
-			if useCycleMarking && check.typeCycle(obj) {
+			if check.typeCycle(obj) {
 				// Don't set obj.typ to Typ[Invalid] here
 				// because plenty of code type-asserts that
 				// functions have a *Signature type. Grey
@@ -273,10 +266,15 @@ var cutCycle = NewTypeName(token.NoPos, nil, "!", nil)
 // TODO(gri) rename s/typeCycle/cycle/ once we don't need the other
 // cycle method anymore.
 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()
+	// The object map contains the package scope objects and the non-interface methods.
+	if debug {
+		info := check.objMap[obj]
+		inObjMap := info != nil && (info.fdecl == nil || info.fdecl.Recv == nil) // exclude methods
+		isPkgObj := obj.Parent() == check.pkg.scope
+		if isPkgObj != inObjMap {
+			check.dump("%v: inconsistent object map for %s (isPkgObj = %v, inObjMap = %v)", obj.Pos(), obj, isPkgObj, inObjMap)
+			unreachable()
+		}
 	}
 
 	// Given the number of constants and variables (nval) in the cycle
@@ -312,8 +310,25 @@ func (check *Checker) typeCycle(obj Object) (isCycle bool) {
 				// that we type-check methods when we type-check their
 				// receiver base types.
 				return false
-			case !check.objMap[obj].alias:
-				hasTDef = true
+			default:
+				// Determine if the type name is an alias or not. For
+				// package-level objects, use the object map which
+				// provides syntactic information (which doesn't rely
+				// on the order in which the objects are set up). For
+				// local objects, we can rely on the order, so use
+				// the object's predicate.
+				// TODO(gri) It would be less fragile to always access
+				// the syntactic information. We should consider storing
+				// this information explicitly in the object.
+				var alias bool
+				if d := check.objMap[obj]; d != nil {
+					alias = d.alias // package-level object
+				} else {
+					alias = obj.IsAlias() // function local object
+				}
+				if !alias {
+					hasTDef = true
+				}
 			}
 		case *Func:
 			// ignored for now
@@ -552,15 +567,13 @@ func (check *Checker) addMethodDecls(obj *TypeName) {
 		}
 	}
 
-	if useCycleMarking {
-		// Suppress detection of type cycles occurring through method
-		// declarations - they wouldn't exist if methods were type-
-		// checked separately from their receiver base types. See also
-		// comment at the end of Checker.typeDecl.
-		// TODO(gri) Remove this once methods are type-checked separately.
-		check.push(cutCycle)
-		defer check.pop()
-	}
+	// Suppress detection of type cycles occurring through method
+	// declarations - they wouldn't exist if methods were type-
+	// checked separately from their receiver base types. See also
+	// comment at the end of Checker.typeDecl.
+	// TODO(gri) Remove this once methods are type-checked separately.
+	check.push(cutCycle)
+	defer check.pop()
 
 	// type-check methods
 	for _, m := range methods {
@@ -730,8 +743,10 @@ func (check *Checker) declStmt(decl ast.Decl) {
 				// the innermost containing block."
 				scopePos := s.Name.Pos()
 				check.declare(check.scope, s.Name, obj, scopePos)
+				// mark and unmark type before calling typeDecl; its type is still nil (see Checker.objDecl)
+				obj.setColor(grey + color(check.push(obj)))
 				check.typeDecl(obj, s.Type, nil, nil, s.Assign.IsValid())
-
+				check.pop().setColor(black)
 			default:
 				check.invalidAST(s.Pos(), "const, type, or var declaration expected")
 			}

src/go/types/resolver.go

@@ -420,6 +420,10 @@ func (check *Checker) collectObjects() {
 					check.recordDef(d.Name, obj)
 				}
 				info := &declInfo{file: fileScope, fdecl: d}
+				// Methods are not package-level objects but we still track them in the
+				// object map so that we can handle them like regular functions (if the
+				// receiver is invalid); also we need their fdecl info when associating
+				// them with their receiver base type, below.
 				check.objMap[obj] = info
 				obj.setOrder(uint32(len(check.objMap)))
 

src/go/types/testdata/cycles.src

@@ -158,6 +158,8 @@ type T20 chan [unsafe.Sizeof(func(ch T20){ _ = <-ch })]byte
 type T22 = chan [unsafe.Sizeof(func(ch T20){ _ = <-ch })]byte
 
 func _() {
-	type T1 chan [unsafe.Sizeof(func(ch T1){ _ = <-ch })]byte
-	type T2 = chan [unsafe.Sizeof(func(ch T2){ _ = <-ch })]byte
+	type T0 func(T0)
+	type T1 /* ERROR cycle */ = func(T1)
+	type T2 chan [unsafe.Sizeof(func(ch T2){ _ = <-ch })]byte
+	type T3 /* ERROR cycle */ = chan [unsafe.Sizeof(func(ch T3){ _ = <-ch })]byte
 }

src/go/types/typexpr.go

@@ -71,17 +71,6 @@ 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]
-			break
-		}
 
 	case *Var:
 		// It's ok to mark non-local variables, but ignore variables
@@ -169,10 +158,8 @@ 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()
-	}
+	check.push(indir)
+	defer check.pop()
 	return check.typExpr(e, nil, nil)
 }