cmd/compile: correct alias cycle detection

The original fix (https://go-review.googlesource.com/c/go/+/35831)
for this issue was incorrect as it reported cycles in cases where
it shouldn't.

Instead, use a different approach: A type cycle containing aliases
is only a cycle if there are no type definitions. As soon as there
is a type definition, alias expansion terminates and there is no
cycle.

Approach: Split sprint_depchain into two non-recursive and more
easily understandable functions (cycleFor and cycleTrace),
and use those instead for cycle reporting. Analyze the cycle
returned by cycleFor before issueing an alias cycle error.

Also: Removed original fix (main.go) which introduced a separate
crash (#23823).

Fixes #18640.
Fixes #23823.
Fixes #24939.

Change-Id: Ic3707a9dec40a71dc928a3e49b4868c5fac3d3b7
Reviewed-on: https://go-review.googlesource.com/118078
Reviewed-by: Matthew Dempsky <mdempsky@google.com>

diff --git a/src/cmd/compile/internal/gc/main.go b/src/cmd/compile/internal/gc/main.go
index e8b33008b45751a4e99663d4201b64544d73233f..9f1ea2ab4bf772b6417e85c81af17aeeab76b2a4 100644 (file)
--- a/src/cmd/compile/internal/gc/main.go
+++ b/src/cmd/compile/internal/gc/main.go
@@ -481,15 +481,13 @@ func Main(archInit func(*Arch)) {
        // Phase 1: const, type, and names and types of funcs.
        //   This will gather all the information about types
        //   and methods but doesn't depend on any of it.
-       //   We also defer type alias declarations until phase 2
-       //   to avoid cycles like #18640.
        defercheckwidth()
 
        // Don't use range--typecheck can add closures to xtop.
        timings.Start("fe", "typecheck", "top1")
        for i := 0; i < len(xtop); i++ {
                n := xtop[i]
-               if op := n.Op; op != ODCL && op != OAS && op != OAS2 && (op != ODCLTYPE || !n.Left.Name.Param.Alias) {
+               if op := n.Op; op != ODCL && op != OAS && op != OAS2 {
                        xtop[i] = typecheck(n, Etop)
                }
        }
@@ -501,7 +499,7 @@ func Main(archInit func(*Arch)) {
        timings.Start("fe", "typecheck", "top2")
        for i := 0; i < len(xtop); i++ {
                n := xtop[i]
-               if op := n.Op; op == ODCL || op == OAS || op == OAS2 || op == ODCLTYPE && n.Left.Name.Param.Alias {
+               if op := n.Op; op == ODCL || op == OAS || op == OAS2 {
                        xtop[i] = typecheck(n, Etop)
                }
        }

diff --git a/src/cmd/compile/internal/gc/typecheck.go b/src/cmd/compile/internal/gc/typecheck.go
index 483be32d6e9ccc186042c9aae57bd56ec9e40955..fd134e9f1206098aa6ad4dd30afa666518b14d20 100644 (file)
--- a/src/cmd/compile/internal/gc/typecheck.go
+++ b/src/cmd/compile/internal/gc/typecheck.go
@@ -110,19 +110,35 @@ func typekind(t *types.Type) string {
        return fmt.Sprintf("etype=%d", et)
 }
 
-// sprint_depchain prints a dependency chain of nodes into trace.
-// It is used by typecheck in the case of OLITERAL nodes
-// to print constant definition loops.
-func sprint_depchain(trace *string, stack []*Node, cur *Node, first *Node) {
-       for i := len(stack) - 1; i >= 0; i-- {
-               if n := stack[i]; n.Op == cur.Op {
-                       if n != first {
-                               sprint_depchain(trace, stack[:i], n, first)
-                       }
-                       *trace += fmt.Sprintf("\n\t%v: %v uses %v", n.Line(), n, cur)
-                       return
+func cycleFor(start *Node) []*Node {
+       // Find the start node in typecheck_tcstack.
+       // We know that it must exist because each time we mark
+       // a node with n.SetTypecheck(2) we push it on the stack,
+       // and each time we mark a node with n.SetTypecheck(2) we
+       // pop it from the stack. We hit a cycle when we encounter
+       // a node marked 2 in which case is must be on the stack.
+       i := len(typecheck_tcstack) - 1
+       for i > 0 && typecheck_tcstack[i] != start {
+               i--
+       }
+
+       // collect all nodes with same Op
+       var cycle []*Node
+       for _, n := range typecheck_tcstack[i:] {
+               if n.Op == start.Op {
+                       cycle = append(cycle, n)
                }
        }
+
+       return cycle
+}
+
+func cycleTrace(cycle []*Node) string {
+       var s string
+       for i, n := range cycle {
+               s += fmt.Sprintf("\n\t%v: %v uses %v", n.Line(), n, cycle[(i+1)%len(cycle)])
+       }
+       return s
 }
 
 var typecheck_tcstack []*Node
@@ -174,10 +190,20 @@ func typecheck(n *Node, top int) *Node {
                        }
 
                case OTYPE:
+                       // Only report a type cycle if we are expecting a type.
+                       // Otherwise let other code report an error.
                        if top&Etype == Etype {
-                               var trace string
-                               sprint_depchain(&trace, typecheck_tcstack, n, n)
-                               yyerrorl(n.Pos, "invalid recursive type alias %v%s", n, trace)
+                               // A cycle containing only alias types is an error
+                               // since it would expand indefinitely when aliases
+                               // are substituted.
+                               cycle := cycleFor(n)
+                               for _, n := range cycle {
+                                       if n.Name != nil && !n.Name.Param.Alias {
+                                               lineno = lno
+                                               return n
+                                       }
+                               }
+                               yyerrorl(n.Pos, "invalid recursive type alias %v%s", n, cycleTrace(cycle))
                        }
 
                case OLITERAL:
@@ -185,9 +211,7 @@ func typecheck(n *Node, top int) *Node {
                                yyerror("%v is not a type", n)
                                break
                        }
-                       var trace string
-                       sprint_depchain(&trace, typecheck_tcstack, n, n)
-                       yyerrorl(n.Pos, "constant definition loop%s", trace)
+                       yyerrorl(n.Pos, "constant definition loop%s", cycleTrace(cycleFor(n)))
                }
 
                if nsavederrors+nerrors == 0 {

diff --git a/test/fixedbugs/issue18640.go b/test/fixedbugs/issue18640.go
index c4f948b706efe86ae440f9cd1e521fb95d98b82b..60abd31f760cfaa835cbbe808414f30e2d740da0 100644 (file)
--- a/test/fixedbugs/issue18640.go
+++ b/test/fixedbugs/issue18640.go
@@ -11,12 +11,20 @@ type (
        b struct {
                *a
        }
+)
 
+type (
        c struct {
                *d
        }
        d = c
+)
 
+// The compiler reports an incorrect (non-alias related)
+// type cycle here (via dowith()). Disabled for now.
+// See issue #25838.
+/*
+type (
        e = f
        f = g
        g = []h
@@ -24,3 +32,16 @@ type (
        i = j
        j = e
 )
+*/
+
+type (
+       a1 struct{ *b1 }
+       b1 = c1
+       c1 struct{ *b1 }
+)
+
+type (
+       a2 struct{ b2 }
+       b2 = c2
+       c2 struct{ *b2 }
+)

diff --git a/test/fixedbugs/issue23823.go b/test/fixedbugs/issue23823.go
new file mode 100644 (file)
index 0000000..2f802d0
--- /dev/null
+++ b/test/fixedbugs/issue23823.go
@@ -0,0 +1,15 @@
+// errorcheck
+
+// Copyright 2018 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package p
+
+type I1 = interface {
+       I2
+}
+
+type I2 interface { // ERROR "invalid recursive type"
+       I1
+}

diff --git a/test/fixedbugs/issue24939.go b/test/fixedbugs/issue24939.go
new file mode 100644 (file)
index 0000000..26530e9
--- /dev/null
+++ b/test/fixedbugs/issue24939.go
@@ -0,0 +1,21 @@
+// compile
+
+// Copyright 2018 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package main
+
+type T interface {
+       M(P)
+}
+
+type M interface {
+       F() P
+}
+
+type P = interface {
+       I() M
+}
+
+func main() {}