Skip to content

Commit

Permalink
cmd/compile: reintroduce work-around for cyclic alias declarations
Browse files Browse the repository at this point in the history
This change re-introduces (temporarily) a work-around for recursive
alias type declarations, originally in https://golang.org/cl/35831/
(intended as fix for golang#18640). The work-around was removed later
for a more comprehensive cycle detection check. That check
contained a subtle error which made the code appear to work,
while in fact creating incorrect types internally. See golang#25838
for details.

By re-introducing the original work-around, we eliminate problems
with many simple recursive type declarations involving aliases;
specifically cases such as golang#27232 and golang#27267. However, the more
general problem remains.

This CL also fixes the subtle error (incorrect variable use when
analyzing a type cycle) mentioned above and now issues a fatal
error with a reference to the relevant issue (rather than crashing
later during the compilation). While not great, this is better
than the current status. The long-term solution will need to
address these cycles (see golang#25838).

As a consequence, several old test cases are not accepted anymore
by the compiler since they happened to work accidentally only.
This CL disables parts or all code of those test cases. The issues
are: golang#18640, golang#23823, and golang#24939.

One of the new test cases (fixedbugs/issue27232.go) exposed a
go/types issue. The test case is excluded from the go/types test
suite and an issue was filed (golang#28576).

Updates golang#18640.
Updates golang#23823.
Updates golang#24939.
Updates golang#25838.
Updates golang#28576.

Fixes golang#27232.
Fixes golang#27267.

Change-Id: I6c2d10da98bfc6f4f445c755fcaab17fc7b214c5
Reviewed-on: https://go-review.googlesource.com/c/147286
Reviewed-by: Matthew Dempsky <[email protected]>
  • Loading branch information
griesemer committed Nov 5, 2018
1 parent 3377b46 commit e630538
Show file tree
Hide file tree
Showing 8 changed files with 68 additions and 10 deletions.
8 changes: 6 additions & 2 deletions src/cmd/compile/internal/gc/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -491,13 +491,17 @@ 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.
// TODO(gri) Remove this again once we have a fix for #25838.
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 {
if op := n.Op; op != ODCL && op != OAS && op != OAS2 && (op != ODCLTYPE || !n.Left.Name.Param.Alias) {
xtop[i] = typecheck(n, Etop)
}
}
Expand All @@ -509,7 +513,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 {
if op := n.Op; op == ODCL || op == OAS || op == OAS2 || op == ODCLTYPE && n.Left.Name.Param.Alias {
xtop[i] = typecheck(n, Etop)
}
}
Expand Down
12 changes: 10 additions & 2 deletions src/cmd/compile/internal/gc/typecheck.go
Original file line number Diff line number Diff line change
Expand Up @@ -255,8 +255,16 @@ func typecheck(n *Node, top int) (res *Node) {
// since it would expand indefinitely when aliases
// are substituted.
cycle := cycleFor(n)
for _, n := range cycle {
if n.Name != nil && !n.Name.Param.Alias {
for _, n1 := range cycle {
if n1.Name != nil && !n1.Name.Param.Alias {
// Cycle is ok. But if n is an alias type and doesn't
// have a type yet, we have a recursive type declaration
// with aliases that we can't handle properly yet.
// Report an error rather than crashing later.
if n.Name != nil && n.Name.Param.Alias && n.Type == nil {
lineno = n.Pos
Fatalf("cannot handle alias type declaration (issue #25838): %v", n)
}
lineno = lno
return n
}
Expand Down
1 change: 1 addition & 0 deletions src/go/types/stdlib_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@ func TestStdFixed(t *testing.T) {
"issue22200b.go", // go/types does not have constraints on stack size
"issue25507.go", // go/types does not have constraints on stack size
"issue20780.go", // go/types does not have constraints on stack size
"issue27232.go", // go/types has a bug with alias type (issue #28576)
)
}

Expand Down
5 changes: 2 additions & 3 deletions test/fixedbugs/issue18640.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,7 @@ type (
d = c
)

// The compiler reports an incorrect (non-alias related)
// type cycle here (via dowith()). Disabled for now.
// The compiler cannot handle these cases. Disabled for now.
// See issue #25838.
/*
type (
Expand All @@ -32,7 +31,6 @@ type (
i = j
j = e
)
*/
type (
a1 struct{ *b1 }
Expand All @@ -45,3 +43,4 @@ type (
b2 = c2
c2 struct{ *b2 }
)
*/
8 changes: 6 additions & 2 deletions test/fixedbugs/issue23823.go
Original file line number Diff line number Diff line change
@@ -1,15 +1,19 @@
// errorcheck
// 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 p

// The compiler cannot handle this. Disabled for now.
// See issue #25838.
/*
type I1 = interface {
I2
}
type I2 interface { // ERROR "invalid recursive type"
type I2 interface {
I1
}
*/
4 changes: 3 additions & 1 deletion test/fixedbugs/issue24939.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ type M interface {
}

type P = interface {
I() M
// The compiler cannot handle this case. Disabled for now.
// See issue #25838.
// I() M
}

func main() {}
19 changes: 19 additions & 0 deletions test/fixedbugs/issue27232.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// 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 p

type F = func(T)

type T interface {
m(F)
}

type t struct{}

func (t) m(F) {}

var _ T = &t{}
21 changes: 21 additions & 0 deletions test/fixedbugs/issue27267.go
Original file line number Diff line number Diff line change
@@ -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 p

// 1st test case from issue
type F = func(E) // compiles if not type alias or moved below E struct
type E struct {
f F
}

var x = E{func(E) {}}

// 2nd test case from issue
type P = *S
type S struct {
p P
}

0 comments on commit e630538

Please sign in to comment.