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: incorrect type assertions on conflicting package names #21282

Open
dsnet opened this Issue Aug 2, 2017 · 5 comments

Comments

Projects
None yet
5 participants
@dsnet
Member

dsnet commented Aug 2, 2017

This is a bug that has existed since Go1.0.

Consider the following packages:

package iface // import "github.com/dsnet/example/iface1/iface"

import "fmt"
import "reflect"

func CheckInterface() {
	type iface interface { unexported() }
	var v interface{} = foo{}
	t := reflect.TypeOf(v)

	t1 := reflect.TypeOf((*iface)(nil)).Elem()
	t2 := reflect.TypeOf((*interface { unexported() })(nil)).Elem()
	fmt.Println("reflect implements named interface:  ", t.Implements(t1))
	fmt.Println("reflect implements interface literal:", t.Implements(t2))

	_, ok1 := v.(iface)
	_, ok2 := v.(interface { unexported() })
	fmt.Println("type assertion on named interface:   ", ok1)
	fmt.Println("type assertion on interface literal: ", ok2)

	fmt.Println()
}

type foo struct{}
func (foo) unexported() {}
package iface // import "github.com/dsnet/example/iface2/iface"

import "fmt"
import "reflect"

func CheckInterface() {
	type iface interface { unexported() }
	var v interface{} = foo{}
	t := reflect.TypeOf(v)

	t1 := reflect.TypeOf((*iface)(nil)).Elem()
	t2 := reflect.TypeOf((*interface { unexported() })(nil)).Elem()
	fmt.Println("reflect implements named interface:  ", t.Implements(t1))
	fmt.Println("reflect implements interface literal:", t.Implements(t2))

	_, ok1 := v.(iface)
	_, ok2 := v.(interface { unexported() })
	fmt.Println("type assertion on named interface:   ", ok1)
	fmt.Println("type assertion on interface literal: ", ok2)

	fmt.Println()
}

type foo struct{}
func (foo) unexported() {}

In the setup, we have two packages both named iface where the source code of both packages is identical, but they have different absolute package paths.

Running the following program:

package main
import (
	iface1 "github.com/dsnet/example/iface1/iface"
	iface2 "github.com/dsnet/example/iface2/iface"
)

func main() {
	iface1.CheckInterface()
	iface2.CheckInterface()
}

Prints the following (on Go1.0 to Go1.8):

reflect implements named interface:   true
reflect implements interface literal: true
type assertion on named interface:    true
type assertion on interface literal:  true

reflect implements named interface:   true
reflect implements interface literal: false
type assertion on named interface:    true
type assertion on interface literal:  false

The output slightly differs on Go1.9, but is still wrong.

We expect that calling iface1.CheckInterface() and iface2.CheckInterface() to produce the exact same results. However, that is not what we see. In iface2, type assertions via a interface literal on unexported fields no longer works properly, but it works as expected in iface1.

\cc @ianlancetaylor @josharian @mdempsky @neild

@dsnet dsnet added this to the Go1.10 milestone Aug 2, 2017

@go101

This comment has been minimized.

go101 commented Aug 5, 2017

Append the following two lines at the end of iface1.CheckInterface() is okay,

	var x interface { unexported() } = foo{}
	_ = reflect.TypeOf(x)

but append them at the end of iface2.CheckInterface() will cause run time panic.

package iface // import "github.com/dsnet/example/iface2/iface"

import "fmt"
import "reflect"

type Iface interface { unexported() }

func CheckInterface() {
	var v interface{} = foo{}
	t := reflect.TypeOf(v)

	t1 := reflect.TypeOf((*Iface)(nil)).Elem()
	t2 := reflect.TypeOf((*interface { unexported() })(nil)).Elem()
	fmt.Println("reflect implements named interface:  ", t.Implements(t1))
	fmt.Println("reflect implements interface literal:", t.Implements(t2))

	_, ok1 := v.(Iface)
	_, ok2 := v.(interface { unexported() })
	fmt.Println("type assertion on named interface:   ", ok1)
	fmt.Println("type assertion on interface literal: ", ok2)

	fmt.Println()
	
	//>> panic
	var x interface { unexported() } = foo{}
	_ = reflect.TypeOf(x)
	//<<
}

type foo struct{}
func (foo) unexported() {}

The panic is some weird, when it is triggered, the texts printed in iface1.CheckInterface() are not shown (go 1.9 alpha 1).

$ go run main.go 
panic: interface conversion: iface.foo is not interface { iface.unexported() }: missing method unexported
fatal error: panic on system stack

runtime stack:
runtime.throw(0x4ba54a, 0x15)
	/go/src/runtime/panic.go:605 +0x95 fp=0x7fff6c62f698 sp=0x7fff6c62f678 pc=0x4272e5
panic(0x4a0d60, 0xc42000e000)
	/go/src/runtime/panic.go:420 +0x7b6 fp=0x7fff6c62f740 sp=0x7fff6c62f698 pc=0x427126
runtime.additab(0x51c120, 0x1)
	/go/src/runtime/iface.go:131 +0x63e fp=0x7fff6c62f820 sp=0x7fff6c62f740 pc=0x40c3ee
runtime.itabsinit()
	/go/src/runtime/iface.go:156 +0x84 fp=0x7fff6c62f870 sp=0x7fff6c62f820 pc=0x40c4c4
runtime.schedinit()
	/go/src/runtime/proc.go:476 +0x73 fp=0x7fff6c62f8b0 sp=0x7fff6c62f870 pc=0x429c53
runtime.rt0_go(0x7fff6c62f8e8, 0x1, 0x7fff6c62f8e8, 0x0, 0x0, 0x1, 0x7fff6c6303db, 0x0, 0x7fff6c630417, 0x7fff6c63042d, ...)
	/go/src/runtime/asm_amd64.s:175 +0x1eb fp=0x7fff6c62f8b8 sp=0x7fff6c62f8b0 pc=0x44e9db
exit status 2
@mdempsky

This comment has been minimized.

Member

mdempsky commented Nov 29, 2017

The issue is interface { unexported() } describes different types when written in different packages, because unexported is an unexported identifier.

However, the runtime type descriptor we produce for both is given a linker symbol of type.interface { iface.unexported() }. I.e., it's package name qualified, but not package path qualified. It's also marked dupok, so the linker doesn't detect the collision.

The fix here is we need to qualify these method names by their package path, not just their package name.

@mdempsky

This comment has been minimized.

Member

mdempsky commented Nov 29, 2017

This is clearly undesirable, but I think the fix is too delicate for this late in the release cycle. We've evidently lived with it since 1.0, so I think it can wait another release.

@gopherbot

This comment has been minimized.

gopherbot commented Apr 10, 2018

Change https://golang.org/cl/106175 mentions this issue: cmd/compile: omit unnecessary interface method expression wrappers

gopherbot pushed a commit that referenced this issue Apr 10, 2018

cmd/compile: omit unnecessary interface method expression wrappers
We'll always generate method expression wrappers for declared
interface types in their own package, so no need to generate them in
downstream packages.

Noticed by gri@ while looking into #21282.

Change-Id: I4fb7051b4e15297933da05fdd2b111d6b8f4178e
Reviewed-on: https://go-review.googlesource.com/106175
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
@griesemer

This comment has been minimized.

Contributor

griesemer commented Apr 23, 2018

This has been around forever. Clearly we should fix it but it's also not a release blocker.

@griesemer griesemer added NeedsFix and removed release-blocker labels Apr 23, 2018

@griesemer griesemer modified the milestones: Go1.11, Go1.12 Jun 4, 2018

@griesemer griesemer modified the milestones: Go1.12, Go1.13 Dec 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment