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

reflect: StructOf forbids anonymous fields without name #24781

Closed
Merovius opened this issue Apr 9, 2018 · 8 comments
Closed

reflect: StructOf forbids anonymous fields without name #24781

Merovius opened this issue Apr 9, 2018 · 8 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. early-in-cycle A change that should be done early in the 3 month dev cycle. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@Merovius
Copy link
Contributor

Merovius commented Apr 9, 2018

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

go version go1.10.1 linux/amd64

Does this issue reproduce with the latest release?

Yes.

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

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/mero/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/mero"
GORACE=""
GOROOT="/home/mero/go"
GOTMPDIR=""
GOTOOLDIR="/home/mero/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
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-build786947304=/tmp/go-build -gno-record-gcc-switches"

What did you do?

https://play.golang.org/p/NL_aQrr6F3V

What did you expect to see?

true
true

What did you see instead?

true
false
interface conversion: interface {} is struct { X main.X }, not struct { main.X }


I believe A() and B() should behave the same, but they don't. StructOf seemingly creates a named field, even though Anonymous is set to true. It is not possible, not to pass a name either, though, because then StructOf panics:

true
panic: reflect.StructOf: field 0 has no name

goroutine 1 [running]:
reflect.StructOf(0x1052ff5c, 0x1, 0x1, 0x1e5e20, 0x0, 0x0)
/usr/local/go/src/reflect/type.go:2404 +0x3fa0
main.B(0x122660, 0x1052ffa4)
/tmp/sandbox310635867/main.go:22 +0xc0
main.main()
/tmp/sandbox310635867/main.go:14 +0x60

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 9, 2018
@bcmills bcmills added this to the Go1.11 milestone Apr 9, 2018
@bcmills
Copy link
Contributor

bcmills commented Apr 9, 2018

(CC: @griesemer @mdempsky @dsnet)

@griesemer
Copy link
Contributor

Possibly related to #24721.

@jimmyfrasche
Copy link
Member

I just ran into this: https://play.golang.org/p/rv5GDx0s02w

There seem to be three things going on

  1. If you create an anonymous field with no name, it should use the type name (feature request)
  2. Whether you give a name, it's not treating the field as anonymous (prints field name and type instead of just field name derived from type)
  3. cmd/compile: inconsistent behaviors in judging whether or not two types are identical #24721 as @griesemer said above

@infogulch
Copy link
Contributor

(Thanks Sean for triaging the recently mentioned issue.)

The problem seems to be an inconsistency in the way the abi and reflect packages represent structs with embedded members.

Consider this playground example where the reflected field types are equivalent, but the native type has no field name and the reflect type has the same name as its embedded type:

Note the types of the fields are exactly equivalent according to reflect:
anon: {Name:A PkgPath: Type:main.A Tag: Offset:0 Index:[0] Anonymous:true}, {Name:B PkgPath: Type:main.B Tag: Offset:16 Index:[1] Anonymous:true}
dyn:  {Name:A PkgPath: Type:main.A Tag: Offset:0 Index:[0] Anonymous:true}, {Name:B PkgPath: Type:main.B Tag: Offset:16 Index:[1] Anonymous:true}

Though the printed value looks the same, the printed type is different and they do not compare equal:
anon: {A:{AField:hello} B:{BField:world}} :: struct { main.A; main.B }
dyn:  {A:{AField:hello} B:{BField:world}} :: struct { A main.A; B main.B }
equal?: false

When the reflect package calls unsafe_New, for some reason the fact that the field is embedded is lost and the field is given an explicit name, though it retains its embedded status. This asymmetry is problematic because a type like this is unnameable natively, and also because reflect can't create a struct with embedded members.

I tried investigating further but I ran into a wall because I can't tell where unsafe_New is implemented; this name is not associated with a definition anywhere in the go source.

@ianlancetaylor
Copy link
Contributor

The declaration of unsafe_New in the reflect package says

// implemented in package runtime

//go:noescape
func unsafe_New(*abi.Type) unsafe.Pointer

Searching for unsafe_New in the runtime package turns up

//go:linkname reflect_unsafe_New reflect.unsafe_New
func reflect_unsafe_New(typ *_type) unsafe.Pointer {
	return mallocgc(typ.Size_, typ, true)
}

That is the implementation of reflect.unsafe_New.

That said I think that unsafe_New is a red herring here. unsafe_New takes a type descriptor as an argument. The issue here is that the type descriptors are different. That is, the algorithm used by StructOf to construct the type descriptor does not match the algorithm used by the compiler.

@gopherbot
Copy link

Change https://go.dev/cl/567897 mentions this issue: reflect: omit anonymous field name from StructOf type string

@infogulch
Copy link
Contributor

That is, the algorithm used by StructOf to construct the type descriptor does not match the algorithm used by the compiler.

Yes this is what I was trying to point out, though I misattributed the issue to a downstream effect (unsafe_New) instead of walking back farther to the root cause cause (StructOf). I had forgotten that the runtime sometimes implements internal functions for packages so I did not consider looking elsewhere, and I agree that unsafe_New is a red herring. Thank you for explaining anyway, and for reading through the inaccurate terminology to decipher what I meant.

Now I'm curious about the algorithm that the compiler uses to generate type descriptors.

What do you think of the format of this playground example, where it prints out a description of the issue inline with the code that demonstrates it?

@ianlancetaylor
Copy link
Contributor

The playground example was helpful, thanks. I sent a fix for the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. early-in-cycle A change that should be done early in the 3 month dev cycle. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Development

No branches or pull requests

9 participants