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

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

cmd/compile: incorrect type assertions on conflicting package names #21282

dsnet opened this issue Aug 2, 2017 · 5 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dsnet
Copy link
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
Copy link

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
Copy link
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
Copy link
Member

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
Copy link

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
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
Copy link
Contributor

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

@griesemer griesemer added NeedsFix The path to resolution is known, but the work has not been done. 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
@griesemer griesemer modified the milestones: Go1.13, Go1.14 May 6, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants