From 5650a53dac73703a1bc095a277a194813519001f Mon Sep 17 00:00:00 2001
From: Robert Griesemer <gri@golang.org>
Date: Tue, 8 Oct 2019 15:45:14 -0700
Subject: [PATCH] go/types: don't skip defined types when reporting cycles

The newly introduced "late-stage" cycle detection for types
(https://golang.org/cl/196338/) "skips" named types on the
RHS of a type declaration when reporting a cycle. For instance,
for:

	type (
	   A B
	   B [10]C
	   C A
	)

the reported cycle is:

	illegal cycle in declaration of C
	       C refers to
	       C

because the underlying type of C resolves to [10]C (note that
cmd/compile does the same but simply says invalid recursive
type C).

This CL introduces the Named.orig field which always refers
to the RHS type in a type definition (and is never changed).
By using Named.orig rather than Named.underlying for the type
validity check, the cycle as written in the source code is
reported:

 	illegal cycle in declaration of A
 	       A refers to
 	       B refers to
 	       C refers to
 	       A

Fixes #34771.

Change-Id: I41e260ceb3f9a15da87ffae6a3921bd8280e2ac4
Reviewed-on: https://go-review.googlesource.com/c/go/+/199937
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
---
 src/go/types/decl.go             | 10 ++++++++--
 src/go/types/testdata/cycles.src | 19 +++++++++++++------
 src/go/types/type.go             |  3 ++-
 3 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/src/go/types/decl.go b/src/go/types/decl.go
index d0027aeb8e..a4fb2b81cc 100644
--- a/src/go/types/decl.go
+++ b/src/go/types/decl.go
@@ -311,10 +311,16 @@ func (check *Checker) validType(typ Type, path []Object) typeInfo {
 		}
 
 	case *Named:
+		// don't report a 2nd error if we already know the type is invalid
+		// (e.g., if a cycle was detected earlier, via Checker.underlying).
+		if t.underlying == Typ[Invalid] {
+			t.info = invalid
+			return invalid
+		}
 		switch t.info {
 		case unknown:
 			t.info = marked
-			t.info = check.validType(t.underlying, append(path, t.obj))
+			t.info = check.validType(t.orig, append(path, t.obj))
 		case marked:
 			// cycle detected
 			for i, tn := range path {
@@ -535,7 +541,7 @@ func (check *Checker) typeDecl(obj *TypeName, typ ast.Expr, def *Named, alias bo
 		obj.typ = named // make sure recursive type declarations terminate
 
 		// determine underlying type of named
-		check.definedType(typ, named)
+		named.orig = check.definedType(typ, named)
 
 		// The underlying type of named may be itself a named type that is
 		// incomplete:
diff --git a/src/go/types/testdata/cycles.src b/src/go/types/testdata/cycles.src
index 7f9fc8945e..b2ee8ecd5f 100644
--- a/src/go/types/testdata/cycles.src
+++ b/src/go/types/testdata/cycles.src
@@ -23,10 +23,8 @@ type (
 	A0 /* ERROR cycle */ [10]A0
 	A1 [10]*A1
 
-	// 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
+	A2 /* ERROR cycle */ [10]A3
+	A3 [10]A4
 	A4 A2
 
 	A5 [10]A6
@@ -41,8 +39,8 @@ type (
 	S2 struct{ _ *S2 }
 	S3 struct{ *S3 }
 
-	S4 struct{ S5 }
-	S5 /* ERROR cycle */ struct{ S6 }
+	S4 /* ERROR cycle */ struct{ S5 }
+	S5 struct{ S6 }
 	S6 S4
 
 	// pointers
@@ -73,6 +71,15 @@ type (
 	C0 chan C0
 )
 
+// test case for issue #34771
+type (
+	AA /* ERROR cycle */ B
+	B C
+	C [10]D
+	D E
+	E AA
+)
+
 func _() {
 	type (
 		t1 /* ERROR cycle */ t1
diff --git a/src/go/types/type.go b/src/go/types/type.go
index a490d92009..087cda429d 100644
--- a/src/go/types/type.go
+++ b/src/go/types/type.go
@@ -450,6 +450,7 @@ func (c *Chan) Elem() Type { return c.elem }
 type Named struct {
 	info       typeInfo  // for cycle detection
 	obj        *TypeName // corresponding declared object
+	orig       Type      // type (on RHS of declaration) this *Named type is derived of (for cycle reporting)
 	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
 }
@@ -461,7 +462,7 @@ func NewNamed(obj *TypeName, underlying Type, methods []*Func) *Named {
 	if _, ok := underlying.(*Named); ok {
 		panic("types.NewNamed: underlying type must not be *Named")
 	}
-	typ := &Named{obj: obj, underlying: underlying, methods: methods}
+	typ := &Named{obj: obj, orig: underlying, underlying: underlying, methods: methods}
 	if obj.typ == nil {
 		obj.typ = typ
 	}