Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/compile: interface definition cannot contain itself inside an anonymous interface method argument #16369

Closed
juamedgod opened this issue Jul 14, 2016 · 12 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@juamedgod
Copy link

Please answer these questions before submitting your issue. Thanks!

  1. What version of Go are you using (go version)?

go1.6.2

  1. What operating system and processor architecture are you using (go env)?
GOARCH="amd64"
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
  1. What did you do?

I tried to define an interface that expects itself as the argument of one of its methods:

type Nestable interface {
   AddChild(interface{Nestable})
}

https://play.golang.org/p/oB091DHIS2

  1. What did you expect to see?

I would expect it to simply look for the desired signature of the argument passed in and accept it as valid

  1. What did you see instead?

It seems it recursively walk the full interface definition so it enters som kind of loop

panic: interface conversion: *main.TreeNode is not interface { AddChild(interface { AddChild(interface { AddChild(interface { AddChild(interface { AddChild(interface { AddChild<...> }) }) }) }) }) }: missing method AddChild
@mvdan
Copy link
Member

mvdan commented Jul 14, 2016

Error on tip (go version devel +fc80387 Mon Jul 11 21:03:09 2016 +0000 linux/amd64) is different:

runtime: goroutine stack exceeds 1000000000-byte limit
fatal error: stack overflow

runtime stack:
runtime.throw(0x8f8c67, 0xe)
        /home/mvdan/tip/src/runtime/panic.go:566 +0x95
runtime.newstack()
        /home/mvdan/tip/src/runtime/stack.go:1061 +0x416
runtime.morestack()
        /home/mvdan/tip/src/runtime/asm_amd64.s:366 +0x7f

goroutine 1 [running]:
encoding/binary.PutVarint(0xc440484316, 0xa, 0xa, 0xfffffffffffffff2, 0x0)
        /home/mvdan/tip/src/encoding/binary/varint.go:78 fp=0xc4404842d8 sp=0xc4404842d0
cmd/compile/internal/gc.(*exporter).rawInt64(0xc460483890, 0xfffffffffffffff2)
        /home/mvdan/tip/src/cmd/compile/internal/gc/bexport.go:1712 +0x5d fp=0xc440484330 sp=0xc4404842d8
[...]

@josharian josharian changed the title Interface definition cannot contain itself as a method argument cmd/compile: interface definition cannot contain itself as a method argument Jul 14, 2016
@josharian
Copy link
Contributor

Not a regression from 1.6, so I'm marking as 1.8.

@josharian josharian added this to the Go1.8 milestone Jul 14, 2016
@ianlancetaylor
Copy link
Contributor

Are you sure it's not a regression? It seems to compile for me with 1.6.

@josharian
Copy link
Contributor

Hmm. It fails at run time on 1.6, but compile time at tip. Not sure whether that counts as a regression or not.

@ianlancetaylor
Copy link
Contributor

Oh, I see. I agree: not a regression.

The test case seems to work with gccgo.

@griesemer
Copy link
Contributor

The new importer/exporter can handle this data structure w/o problem but we had to turn off the mechanism a while back because the compiler's internal type representation cannot handle such a recursive type (see trackAllTypes flag in bexport.go of the compiler). Unfortunately this broke cycle detection and we get a stack overflow.

This should probably report an error, or perhaps silently pass as was the case with 1.6.

Note also while this "seems to work" with 1.6, the compiler actually cannot import the resulting package anymore (syntax error in import). Any package with such an interface-recursive exported type cannot be imported - it's only useful at the top-level (main package), but in that case such a type doesn't need to be exported.

With 1.6, for:

package main

type T interface {
    M(interface {
        T
    })
}

the export data looks like:

$$
package main
    type @"".T interface { M(? interface { M(? interface { M(? interface { M(? interface { M(? interface { M<...> }) }) }) }) }) }
    func @"".init ()

$$

(Note the recursion that's "terminated" with interface { M<...> } which cannot be parsed by the importer.)

Probably not a release blocker, but I'll see if there's a low-risk fix that could prevent the crash.

@griesemer
Copy link
Contributor

PS: The reason the compiler cannot handle this type is that it "flattens" interface types: interface{T} becomes an anonymous interface with the method M of T and when exported, we have cycle that doesn't use a named type. The compiler's data structures cannot handle such types when read in.

@gopherbot
Copy link

CL https://golang.org/cl/24979 mentions this issue.

gopherbot pushed a commit that referenced this issue Aug 16, 2016
For #16369.

Change-Id: I4c9f5a66b95558adcc1bcface164b9b2b4382d2f
Reviewed-on: https://go-review.googlesource.com/24979
Reviewed-by: Alan Donovan <adonovan@google.com>
@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 11, 2016
@quentinmit quentinmit changed the title cmd/compile: interface definition cannot contain itself as a method argument cmd/compile: interface definition cannot contain itself inside an anonymous interface method argument Oct 11, 2016
@rsc
Copy link
Contributor

rsc commented Oct 21, 2016

Currently:

$ go run /tmp/x.go
# command-line-arguments
/tmp/x.go:5: cannot export unnamed recursive interface
$ 

That's at least very clear.

@rsc rsc modified the milestones: Go1.9Early, Go1.8 Oct 21, 2016
@mdempsky mdempsky self-assigned this Mar 19, 2017
@gopherbot
Copy link

CL https://golang.org/cl/38392 mentions this issue.

@gopherbot
Copy link

CL https://golang.org/cl/38391 mentions this issue.

gopherbot pushed a commit that referenced this issue Mar 21, 2017
Previously, we handled recursive interfaces by deferring typechecking
of interface methods, while eagerly expanding interface embeddings.

This CL switches to eagerly evaluating interface methods, and
deferring expanding interface embeddings to dowidth. This allows us to
detect recursive interface embeddings with the same mechanism used for
detecting recursive struct embeddings.

Updates #16369.

Change-Id: If4c0320058047f8a2d9b52b9a79de47eb9887f95
Reviewed-on: https://go-review.googlesource.com/38391
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
gopherbot pushed a commit to golang/tools that referenced this issue Mar 21, 2017
Backports golang.org/cl/38392 from go/internal/gcimporter.

Updates golang/go#16369.

Change-Id: Ic805f96e6565590987a5dae9f0f76c206fceab05
Reviewed-on: https://go-review.googlesource.com/38429
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
@gopherbot
Copy link

CL https://golang.org/cl/38490 mentions this issue.

gopherbot pushed a commit that referenced this issue Mar 23, 2017
exporter.nesting was added in c7b9bd7 to mitigate #16369 which was
closed in ee272bb. Remove the exporter.nesting field as it is now unused.

Change-Id: I07873d1a07d6a08b11994b817a1483ffc2f5e45f
Reviewed-on: https://go-review.googlesource.com/38490
Run-TryBot: Dave Cheney <dave@cheney.net>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@golang golang locked and limited conversation to collaborators Mar 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

9 participants