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: seemingly identical anonymous types lead to confusing error messages #18911

Closed
zzz125 opened this Issue Feb 3, 2017 · 15 comments

Comments

Projects
None yet
7 participants
@zzz125
Copy link

zzz125 commented Feb 3, 2017

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

1.8rc3 (broken, also in rc1)
1.7.5 (works)

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/k/dev/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build538144559=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"

What did you do?

https://play.golang.org/p/rQzEbb-u6Y

also, this is my test code with modules (i cant simulate it in playground):
test.zip

to run it, set GOPATH and run it with:
$ go run src/a.go

the output for go1.7 is:
-> main.X main.X
Hello, playground {2 3}

for go1.8:
panic: interface conversion: interface {} is struct { x int; y int }, not struct { x int; y int }

it does't work in my another project either now, after switching to 1.8rc

What did you expect to see?

old behaviour as in go 1.7 where it works

What did you see instead?

panic: interface conversion: interface {} is struct { x int; y int }, not struct { x int; y int }

@randall77

This comment has been minimized.

Copy link
Contributor

randall77 commented Feb 3, 2017

Thanks for the issue. This looks like two anonymous types (declared in two separate packages, but identical) are being given different type pointers.

Here's a cleaner repro:

a.go:

package main

import "b"

func main() {
	_ = b.GetY().(struct{ x, y int })

b/b.go:

package b

func GetY() interface{} {
	return struct{ x, y int }{2, 3}
}

GetY gets inlined, so the type test boils down to (in main.main):

 104c52f:       48 8d 05 8a e5 00 00    lea    0xe58a(%rip),%rax        # 105aac0 <runtime.rodata+0xe500>
 104c536:       48 8d 0d 03 e6 00 00    lea    0xe603(%rip),%rcx        # 105ab40 <runtime.rodata+0xe580>
 104c53d:       48 39 c8                cmp    %rcx,%rax
 104c540:       75 0a                   jne    104c54c <main.main+0x4c>

Which seems wrong. Both of those LEAs should be of the same address.
I thought we had fixed all the non-unique type problems. Guess not.

I'll look more in the morning.

@crawshaw @rsc

@randall77 randall77 added this to the Go1.8 milestone Feb 3, 2017

@josharian josharian changed the title go 1.8rc3 anonymous structs comparing silly error (works in 1.7) cmd/compile: anonymous types in separate packages have different type pointers Feb 3, 2017

@josharian josharian changed the title cmd/compile: anonymous types in separate packages have different type pointers cmd/compile: identical anonymous types in separate packages have different type pointers Feb 3, 2017

@crawshaw

This comment has been minimized.

Copy link
Contributor

crawshaw commented Feb 3, 2017

These are different types.

The fields x and y are unexported, and so cannot be accessed outside of package b. If you export them, X and Y, this code works as you would expect.

@crawshaw crawshaw closed this Feb 3, 2017

@zzz125

This comment has been minimized.

Copy link
Author

zzz125 commented Feb 3, 2017

@crawshaw

even if so, i think the error should be more clear, because the folowing is unreadable:

panic: interface conversion: interface {} is struct { x int; y int }, not struct { x int; y int }
                                                 ^------------- same thing ---------^

probably it should be
panic: interface conversion: interface {} is b. struct { x int; y int }, not main. struct { x int; y int }

@crawshaw

This comment has been minimized.

Copy link
Contributor

crawshaw commented Feb 3, 2017

A better error would be better.

@crawshaw crawshaw reopened this Feb 3, 2017

@crawshaw crawshaw changed the title cmd/compile: identical anonymous types in separate packages have different type pointers cmd/compile: seemingly identical anonymous types lead to confusing error messages Feb 3, 2017

@crawshaw crawshaw modified the milestones: Go1.9, Go1.8 Feb 3, 2017

@randall77

This comment has been minimized.

Copy link
Contributor

randall77 commented Feb 3, 2017

Hm, so is the bug in 1.7 then? It ran the repro successfully.

@crawshaw

This comment has been minimized.

Copy link
Contributor

crawshaw commented Feb 3, 2017

I would say so, yes. Probably something I introduced when working on binary size reductions for 1.7. I elided some unnecessary pkgpath values, and seem to remember at least once getting overenthusiastic.

@randall77

This comment has been minimized.

Copy link
Contributor

randall77 commented Feb 3, 2017

Repro runs successfully for all of 1.2 through 1.7...

@crawshaw

This comment has been minimized.

Copy link
Contributor

crawshaw commented Feb 3, 2017

Oh. Hmm.

It still fits my mental model that these are separate types, and you shouldn't be able to reach the unexported fields of another package's type, even if it's unnamed.

@ianlancetaylor, @griesemer, opinions?

@randall77

This comment has been minimized.

Copy link
Contributor

randall77 commented Feb 3, 2017

Spec seems pretty clear that 1.8 is right and 1.2-1.7 are wrong. As part of type identity:

Lower-case field names from different packages are always different.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Feb 3, 2017

It looks to me like Go 1.8 is correct and earlier versions of Go were not. My guess is that Robert's new export data fixed the bug.

(I also agree that a clearer error would be nice.)

@griesemer

This comment has been minimized.

Copy link
Contributor

griesemer commented Feb 3, 2017

This is clearly correct in Go 1.8. There were several subtle bugs in the old export data (though I don't know for sure what fixed this. I'll assign it to me for a better error message.

@griesemer griesemer self-assigned this Feb 3, 2017

@griesemer

This comment has been minimized.

Copy link
Contributor

griesemer commented Jun 6, 2017

Leaving for 1.10 for improved error message (should probably mentioned declaration positions). Clearly not urgent as it's a rare case.

@griesemer griesemer modified the milestones: Go1.10, Go1.9 Jun 6, 2017

@mdempsky

This comment has been minimized.

Copy link
Member

mdempsky commented Nov 29, 2017

Related: #17283 and #21282.

@mdempsky mdempsky modified the milestones: Go1.10, Go1.11 Nov 29, 2017

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Jun 4, 2018

Change https://golang.org/cl/116255 mentions this issue: runtime: slightly better error message for assertion panics with identical looking types

@gopherbot gopherbot closed this in 75c1aed Jun 5, 2018

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Jul 11, 2018

Change https://golang.org/cl/123395 mentions this issue: runtime: don't say "different packages" if they may not be different

gopherbot pushed a commit that referenced this issue Jul 11, 2018

runtime: don't say "different packages" if they may not be different
Fix the panic message produced for an interface conversion error to
only say "types from different packages" if they are definitely from
different packges. If they may be from the same package, say "types
from different scopes."

Updates #18911
Fixes #26094

Change-Id: I0cea50ba31007d88e70c067b4680009ede69bab9
Reviewed-on: https://go-review.googlesource.com/123395
Reviewed-by: Austin Clements <austin@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.