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

x/tools/go/loader: program with nested interfaces works in Go but not with the loader #22866

Closed
matthewmueller opened this issue Nov 24, 2017 · 8 comments
Milestone

Comments

@matthewmueller
Copy link

matthewmueller commented Nov 24, 2017

This took me awhile to boil down, but the link below is the smallest possible program that illustrates this bug. The program models the DOM, which has lots of interconnected interfaces.

The weird part is that it works with go run, so it seems there must be some case missing in the loader.

I noticed some other open issues around cgo and vendoring – this program doesn't use cgo or vendoring, it's just Go code with some interfaces.

Runnable example: https://github.com/matthewmueller/deep-interface-bug

Thanks in advance for your help!


UPDATE: I tried to make a callgraph visualization to help illustrate the problem better, but it uses the loader, which can't compile this program 😅


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

go version go1.9 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

GOOS="darwin"
GOARCH="amd64"

What did you do?

Runnable example at: https://github.com/matthewmueller/deep-interface-bug

What did you expect to see?

A loaded program

What did you see instead?

invalid operation: html (variable of type github.com/matthewmueller/deep-interface-bug/window.HTMLElement) has no field or method DispatchEvent

@matthewmueller matthewmueller changed the title tools/go/loader: program with nested interfaces works in Go but not with loader tools/go/loader: program with nested interfaces works in Go but not with the loader Nov 24, 2017
@mvdan mvdan changed the title tools/go/loader: program with nested interfaces works in Go but not with the loader x/tools/go/loader: program with nested interfaces works in Go but not with the loader Nov 24, 2017
@gopherbot gopherbot added this to the Unreleased milestone Nov 24, 2017
@matthewmueller
Copy link
Author

matthewmueller commented Dec 4, 2017

FWIW, here's the example that doesn't work:

window.go

package window

// EventTarget interface
type EventTarget interface {
	DispatchEvent() Event
}

// Node interface
type Node interface {
	EventTarget

	ParentElement() HTMLElement
}

// Element interface
type Element interface {
	Node
}

// HTMLElement interface
type HTMLElement interface {
	Element
}

// Event interface
type Event interface {
	SrcElement() Element
}

input.go

package main

import "github.com/matthewmueller/deep-interface-bug/window"

func Element() window.Element {
	return nil
}

func main() {
	html := Element().(window.HTMLElement)
	html.DispatchEvent()
}

main.go

package main

import (
	"github.com/golang/tools/go/loader"
)

func main() {
	var conf loader.Config
	conf.Import("./input.go")

	program, err := conf.Load()
	if err != nil {
		panic(err)
	}

	println("got program!")
	_ = program
}

Here's the output when running main.go:

input.go:11:2: invalid operation: 
html (variable of type window.HTMLElement) has no field or method DispatchEvent

Full example: https://github.com/matthewmueller/deep-interface-bug

@ianlancetaylor
Copy link
Contributor

This is probably a dup of #18395.

CC @griesemer

@griesemer griesemer self-assigned this Dec 5, 2017
@griesemer griesemer modified the milestones: Unreleased, Go1.11 Dec 5, 2017
@griesemer
Copy link
Contributor

@matthewmueller Could you please run your code using the latest (tip) version of go/types and report back if you see the same error? If it is indeed a duplicate of #18395 go/types will now complain with a respective error.

@griesemer griesemer added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Dec 5, 2017
@griesemer
Copy link
Contributor

@matthewmueller I just confirmed myself that this is indeed a duplicate of #18395. A fix is in the works, but unfortunately it's too late to get that into Go 1.10. For:

package p

// EventTarget interface
type EventTarget interface {
	DispatchEvent() Event
}

// Node interface
type Node interface {
	EventTarget

	ParentElement() HTMLElement
}

// Element interface
type Element interface {
	Node
}

// HTMLElement interface
type HTMLElement interface {
	Element
}

// Event interface
type Event interface {
	SrcElement() Element
}

var _ = HTMLElement.DispatchEvent

gotype reports:

$ gotype x.go
x.go:22:2: internal error: incomplete embedded interface Element (issue #18395)
x.go:30:9: invalid operation: HTMLElement (type) has no field or method DispatchEvent

As a temporary work-around in this case you could manually inline the methods that the Element interface embeds via the Node type. This is always possible to do manually, and in fact the fix for go/types is to just do this automatically. But it's a larger fix which is why it's not going to make it anymore this round. Sorry.

Closing as duplicate of #18395.

@griesemer griesemer removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Dec 5, 2017
@matthewmueller
Copy link
Author

matthewmueller commented Dec 6, 2017

Thanks for the response and workaround!

@griesemer I'll definitely give inlining a shot. It will definitely blow up the file sizes a lot because the DOM interfaces are huge – hopefully that doesn't slow down the compiler.

@griesemer
Copy link
Contributor

@matthewmueller Inlining all methods manually will always work but you may not have to go to that extreme to avoid the issue. You may be able to factor our a significant subset of the methods into a new interface that you then can embed.

Again, a fix is in the works ( https://go-review.googlesource.com/c/go/+/79575 ) and you could try the patch, but it's not the final version quite yet.

@matthewmueller
Copy link
Author

matthewmueller commented Dec 15, 2017

Thanks for the comments, I'm not sure factoring out by hand is an option because the DOM APIs are pretty massive and I'm not sure where all the cycles lie.

Again, a fix is in the works ( https://go-review.googlesource.com/c/go/+/79575 ) and you could try the patch, but it's not the final version quite yet.

Would this require a fork to Go itself or could I bring this in as a library?

Update Also a bit off topic. I'm not sure this would be helpful to the Go team, but I have a really massive generated 200kloc, working implementation (minus this bug) of the DOM in Go. Might be useful for testing + performance tuning, happy to share it if it'd be of value.

@griesemer
Copy link
Contributor

@matthewmueller You can just apply that patch ( https://go-review.googlesource.com/c/go/+/79575 ) to go/types (it's only affecting go/types) and use the updated library.

DOM implementation: Sure, let us know how to get it.

@golang golang locked and limited conversation to collaborators Dec 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants