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

runtime: in generic code, convert iface to type failed with "types from different scopes" #51250

Closed
rguilmont opened this issue Feb 18, 2022 · 15 comments
Labels
NeedsFix release-blocker
Milestone

Comments

@rguilmont
Copy link

@rguilmont rguilmont commented Feb 18, 2022

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

go version go1.18beta2 linux/amd64

Does this issue reproduce with the latest release?

yes

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

go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/romain/.cache/go-build"
GOENV="/home/romain/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/romain/go/pkg/mod"
GONOPROXY="XXXXXXXXXXXXXXXXXXX"
GONOSUMDB="XXXXXXXXXXXXXXXXXXX"
GOOS="linux"
GOPATH="/home/romain/go"
GOPRIVATE="XXXXXXXXXXXXXXXXX"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/romain/sdk/go1.18beta2"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/romain/sdk/go1.18beta2/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.18beta2"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/romain/Documents/git/github/replicate-issue/types-from-different-scopes-issue/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build838965849=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Using generics and interface conversion, i get the following error at runtime :

panic: interface conversion: either.Either[go.shape.string_0,go.shape.interface {}_1] is either.right[github.com/rguilmont/types-from-different-scopes-issue/pkg/pkga.Test,interface {}], not either.right[github.com/rguilmont/types-from-different-scopes-issue/pkg/pkga.Test,interface {}] (types from different scopes)

goroutine 1 [running]:
github.com/rguilmont/types-from-different-scopes-issue/pkg/either.Fold[...]({0x4b2c00, 0xc00009e210}, 0x49c850, 0x49c858)
        /home/romain/Documents/git/github/replicate-issue/types-from-different-scopes-issue/pkg/either/either.go:42 +0xc5
main.main()
        /home/romain/Documents/git/github/replicate-issue/types-from-different-scopes-issue/pkg/cmd/main.go:13 +0x65

To check the code, here it is with instruction to replicate : https://github.com/rguilmont/types-from-different-scopes-issue

What did you expect to see?

It works! test

Thanks !

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Feb 18, 2022

Thanks for the report. I don't know generics to say if this is expected or not, but I checked and this behavior reproduces with go1.18rc1 as well.

CC @findleyr, @mdempsky.

@dmitshur dmitshur added the NeedsInvestigation label Feb 18, 2022
@dmitshur dmitshur added this to the Go1.18 milestone Feb 18, 2022
@dmitshur dmitshur changed the title generics: convert iface to type failed with "types from different scopes" runtime: in generic code, convert iface to type failed with "types from different scopes" Feb 18, 2022
@mdempsky
Copy link
Member

@mdempsky mdempsky commented Feb 18, 2022

At the very least, I don't think "go.shape" should appear in any user visible error messages? I expect the panic message should mention the specific types instead.

/cc @danscales @randall77

@danscales
Copy link
Contributor

@danscales danscales commented Feb 18, 2022

In panicdottypeE, the have and want type descriptors look completely identical (I'm assuming the gcdata is identical too, didn't try to look past the first byte in gdb yet):

(dlv) p have
*runtime._type {size: 16, ptrdata: 8, hash: 2221142398, tflag: tflagUncommon|tflagExtraStar|tflagNamed (7), align: 8, fieldAlign: 8, kind: 25, equal: runtime.strequal, gcdata: *1, str: 41281, ptrToThis: 48864}
(dlv) p want
*runtime._type {size: 16, ptrdata: 8, hash: 2221142398, tflag: tflagUncommon|tflagExtraStar|tflagNamed (7), align: 8, fieldAlign: 8, kind: 25, equal: runtime.strequal, gcdata: *1, str: 41281, ptrToThis: 48864}

Seems like we maybe ended up with identical run-time types that should have been created as the same type from the start or deduplicated. The "types from different scopes" message is really just saying that's the guess, given that there is no other reason why the types aren't the same type (the other possibility printed is that they are in different packages).

@rfindley

This comment has been hidden.

@dmitshur

This comment has been hidden.

@rguilmont
Copy link
Author

@rguilmont rguilmont commented Feb 19, 2022

In panicdottypeE, the have and want type descriptors look completely identical (I'm assuming the gcdata is identical too, didn't try to look past the first byte in gdb yet):

(dlv) p have
*runtime._type {size: 16, ptrdata: 8, hash: 2221142398, tflag: tflagUncommon|tflagExtraStar|tflagNamed (7), align: 8, fieldAlign: 8, kind: 25, equal: runtime.strequal, gcdata: *1, str: 41281, ptrToThis: 48864}
(dlv) p want
*runtime._type {size: 16, ptrdata: 8, hash: 2221142398, tflag: tflagUncommon|tflagExtraStar|tflagNamed (7), align: 8, fieldAlign: 8, kind: 25, equal: runtime.strequal, gcdata: *1, str: 41281, ptrToThis: 48864}

Seems like we maybe ended up with identical run-time types that should have been created as the same type from the start or deduplicated. The "types from different scopes" message is really just saying that's the guess, given that there is no other reason why the types aren't the same type (the other possibility printed is that they are in different packages).

From what i understand, and the "bypass" branch i've put to my repo, it seems that in a way ( i may be completely wrong, i don't have your understanding of the go runtime 😁 ) the error message is relevant : (types from different scopes) :

either.right[github.com/rguilmont/types-from-different-scopes-issue/pkg/pkga.Test,interface {}], not either.right[github.com/rguilmont/types-from-different-scopes-issue/pkg/pkga.Test,interface {}]

in the first case, the type was created inside the package ( namely github.com/rguilmont/types-from-different-scopes-issue/pkg/pkga ) and i tried to "cast" it with a type from an imported package ( same, github.com/rguilmont/types-from-different-scopes-issue/pkg/pkga ).

Once i move the type in another package, the issue disappear ( as you can see in the bypass branch ) because then in each package i work with types imported from another package. The issue looks related to scopes, maybe there's something lost in some way that prevent the casting though it should be valid.

I hope i was clear enough and i can help 🥲

@randall77
Copy link
Contributor

@randall77 randall77 commented Feb 24, 2022

Looks like we have two different itabs representing the same type/interface combo.

The itabs are almost identical. They have the same name according to the linker, and all their fields are the same except the hash field. Because the has field is different, the linker doesn't deduplicate like we would normally expect.

The types have different hashes, because one time they are computed as the hash of:

either.right[either.Test,interface {}]

or

either.right[github.com/rguilmont/types-from-different-scopes-issue/pkg/pkga.Test,interface {}]

I think the latter is the right thing to be hashing. Not sure how we get to the former. I think it has something to do with the LocalPkg being the package being compiled, not necessarily the package where the named type originates.

@randall77
Copy link
Contributor

@randall77 randall77 commented Feb 24, 2022

Hm, looks like when compiling pkga, we make a symbol right["".Test,interface {}] in package either. But the "" in that symbol refers to pkga. But right is in package either, so we use either to substitute into the symbol name, which is not always right.

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 25, 2022

Change https://go.dev/cl/387974 mentions this issue: cmd/compile: use package path instead of "" inside instantiation of nonlocal type

@cagedmantis
Copy link
Contributor

@cagedmantis cagedmantis commented Mar 2, 2022

What is the status of the CL linked to this issue? It is a release blocker and we are working our way to a release.

@randall77
Copy link
Contributor

@randall77 randall77 commented Mar 2, 2022

There is a CL that fixes this issue. I'm investigating a possibly better fix, should know one way or another by EOD.

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 2, 2022

Change https://go.dev/cl/389474 mentions this issue: cmd/compile: don't include instantiating types in type hash

@randall77
Copy link
Contributor

@randall77 randall77 commented Mar 3, 2022

Reopening for cherry-picking into 1.18 release branch.

@randall77 randall77 reopened this Mar 3, 2022
@dmitshur dmitshur added NeedsFix and removed NeedsInvestigation labels Mar 3, 2022
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 4, 2022

Change https://go.dev/cl/390017 mentions this issue: [release-branch.go1.18] cmd/compile: don't include instantiating types in type hash

gopherbot pushed a commit that referenced this issue Mar 4, 2022
…s in type hash

This CL is a bit overkill, but it is pretty safe for 1.18. We'll
want to revisit for 1.19 so we can avoid the hash collisions between
types, e.g. G[int] and G[float64], that will cause some slowdowns
(but not incorrect behavior). Thanks Cherry for the simple idea.

Fixes #51250

Change-Id: I68130e09ba68e7cc35687bc623f63547bc552867
Reviewed-on: https://go-review.googlesource.com/c/go/+/389474
Trust: Keith Randall <khr@golang.org>
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
(cherry picked from commit d367205)
Reviewed-on: https://go-review.googlesource.com/c/go/+/390017
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Mar 4, 2022

Cherry-picked to 1.18 release branch via CL 390017, closing.

@dmitshur dmitshur closed this as completed Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix release-blocker
Projects
None yet
Development

No branches or pull requests

8 participants