From 6052ac0558290743acb729b22ff3d92ce1318c28 Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Wed, 6 Jun 2018 10:21:15 -0700 Subject: [PATCH] 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 --- src/cmd/compile/internal/gc/main.go | 6 +-- src/cmd/compile/internal/gc/typecheck.go | 58 +++++++++++++++++------- test/fixedbugs/issue18640.go | 21 +++++++++ test/fixedbugs/issue23823.go | 15 ++++++ test/fixedbugs/issue24939.go | 21 +++++++++ 5 files changed, 100 insertions(+), 21 deletions(-) create mode 100644 test/fixedbugs/issue23823.go create mode 100644 test/fixedbugs/issue24939.go diff --git a/src/cmd/compile/internal/gc/main.go b/src/cmd/compile/internal/gc/main.go index e8b33008b45751..9f1ea2ab4bf772 100644 --- 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 483be32d6e9ccc..fd134e9f120609 100644 --- 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 c4f948b706efe8..60abd31f760cfa 100644 --- 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 index 00000000000000..2f802d09886064 --- /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 index 00000000000000..26530e95b29ee4 --- /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() {}