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: 1.18 produces different type descriptor for identical struct in different packages #52856

Closed
pfi79 opened this issue May 11, 2022 · 14 comments
Labels
NeedsInvestigation release-blocker
Milestone

Comments

@pfi79
Copy link

@pfi79 pfi79 commented May 11, 2022

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

$ go version
1.17.10 and 1.18.1

Does this issue reproduce with the latest release?

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

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/......./Library/Caches/go-build"
GOENV="/Users/......./Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/......./go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/......./go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.18.1"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/....../go/src/github.com/......./qqqqq/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 -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/6v/kmxqr0yn61588pbz646cfdyc0000gq/T/go-build1526426409=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

go 1.18
go 1.17

What did you expect to see?

identical behaviour

What did you see instead?

different behavior

If the change in behavior is normal, please show the comit in which it has changed

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented May 11, 2022

A possibly minified repro (with struct { int }): https://go.dev/play/p/F_PIyGVxd0E.

@ianlancetaylor ianlancetaylor changed the title reflect: different behavior in versions 1.17.10 and 1.18.1 cmd/compile: 1.18 produces different type descriptor for identical struct in different packages May 11, 2022
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 11, 2022

Although both values have type struct { int }, in 1.18 the type descriptors for the two values are different. In 1.17 they are the same. This appears to be a bug in 1.18. I don't see any difference in the field values of the descriptors, we seem to just have two different identical descriptors.

CC @randall77 @cherrymui

@ianlancetaylor ianlancetaylor added NeedsInvestigation release-blocker labels May 11, 2022
@ianlancetaylor ianlancetaylor added this to the Go1.19 milestone May 11, 2022
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 11, 2022

@gopherbot Please open a backport for 1.18.

This should be fixed in a 1.18 minor release. The bug does not exist in earlier releases.

@gopherbot
Copy link

@gopherbot gopherbot commented May 11, 2022

Backport issue(s) opened: #52858 (for 1.18).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@cherrymui
Copy link
Member

@cherrymui cherrymui commented May 11, 2022

The spec says https://go.dev/ref/spec#Type_identity

Two struct types are identical if they have the same sequence of fields, and if corresponding fields have the same names, and identical types, and identical tags. Non-exported field names from different packages are always different.

Here the field name (int) is non-exported. I would think the new behavior is actually correct.

The embedding makes it an interesting case. I guess one could argue that the field name for both types is from the builtin package, therefore same? If it were non-embedded type (like struct { x int } or even struct { int int }), it would be clearly different, and the compiler always emits different type descriptors.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 12, 2022

Thanks. You're right: with 1.18 the Field(0).StructField.PkgPath field is different for the two types. With 1.17 it's main for both.

I guess this may be technically correct. Anybody want to argue otherwise?

CC @griesemer @mdempsky

@mdempsky
Copy link
Member

@mdempsky mdempsky commented May 12, 2022

I agree the 1.18 behavior is correct. struct { int } denotes different types when it appears in different packages, because the field names are non-exported and therefore different.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented May 12, 2022

FWIW, I suspect https://go-review.googlesource.com/c/go/+/378434/ is the CL that introduced the changed (fixed) behavior here.

@griesemer
Copy link
Contributor

@griesemer griesemer commented May 12, 2022

I'd also say the 1.18 behavior is correct. The name of an embedded field is simply the (unqualified) name of the embedded type, there's no knowledge of that name coming from a built-in (predeclared) type. The usual export rules apply. The behavior for struct{int} should be the same as for struct{int int} with respect to DeepEqual.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented May 12, 2022

Working as intended.

@pfi79
Copy link
Author

@pfi79 pfi79 commented May 12, 2022

To finally clarify the question

If you replace the example with one like this, it is true everywhere.

func T() interface{} {
return struct{ X int }{123}
}

And if you replace the example with one like this, it is false everywhere.

type Q struct{ I int }

func T() interface{} {
return Q{123}
}

Is this behavior correct?

Two struct types are identical if they have the same sequence of fields, and if corresponding fields have the same names, and identical types, and identical tags. Non-exported field names from different packages are always different.

@mdempsky

@mdempsky
Copy link
Member

@mdempsky mdempsky commented May 12, 2022

@pfi79 The code at https://go.dev/play/p/aAwW6Hhyous appears to behaving correctly, yes.

Edit: To elaborate, defined types declared by different type declarations are always different, and reflect.DeepEqual documents that values of different types are always unequal.

@C0rWin
Copy link

@C0rWin C0rWin commented May 12, 2022

Working as intended.

I have to say it's a little bit unexpected, while I understand why it should be true speaking of unexported fields, IMO with embedding, it is not that obvious, and breaking backwards compatibility.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented May 12, 2022

@C0rWin I'm sorry for surprise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation release-blocker
Projects
None yet
Development

No branches or pull requests

8 participants