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.Implements is true for embedded unexported methods #20633

Closed
abhinav opened this issue Jun 9, 2017 · 6 comments
Closed

reflect: StructOf.Implements is true for embedded unexported methods #20633

abhinav opened this issue Jun 9, 2017 · 6 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@abhinav
Copy link
Contributor

abhinav commented Jun 9, 2017

Please answer these questions before submitting your issue. Thanks!

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

$ go version
go version go1.8.3 darwin/amd64

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

$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/abg/dev/go"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.8.3/libexec"
GOTOOLDIR="/usr/local/Cellar/go/1.8.3/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/8z/qdzjsr3n5l72vdg67zr6xkjc0000gn/T/go-build554429180=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

If you create an interface with an unexported method and another type in the
same package that implements that interface, you can embed that type in a
struct to have that struct also implement that interface.

If you generate a struct with reflect.StructOf, the generated type claims to
also implement that interface but cannot be casted to it.

https://play.golang.org/p/QPlzjnwiZc demonstrates this by generating a type
which embeds *ast.Ident. The generated type claims to implement ast.Expr
but a value of that type cannot actually be casted to ast.Expr.

What did you expect to see?

foo implements ast.Expr: true
foo casts to ast.Expr: true

generated type implements ast.Expr: false
generated type casts to ast.Expr: false
(or alternatively, true, true)

Note that based on the documentation of reflect.StructOf, the expectation
is that the type does NOT implement ast.Expr:

StructOf currently does not generate wrapper methods for embedded fields.
This limitation may be lifted in a future version.

What did you see instead?

The type claims to implement the interface but instances of that type cannot
be cast into that interface.

foo implements ast.Expr: true
foo casts to ast.Expr: true

generated type implements ast.Expr: true
generated type casts to ast.Expr: false

Worth mentioning that if the method on the interface is exported, the
generated type claims to implement the interface and can be casted to it. This
goes against what the docs state but it's a separate issue. See #20632.

Possibly related: #20541

@bradfitz
Copy link
Contributor

bradfitz commented Jun 9, 2017

At tip the result is:

$ go run repro.go
foo implements ast.Expr: true
foo casts to ast.Expr: true

panic: reflect.StructOf: field 0 has no name

goroutine 1 [running]:
reflect.StructOf(0xc420041f08, 0x1, 0x1, 0x0, 0x0)
        /home/bradfitz/go/src/reflect/type.go:2374 +0x34e6
main.main()
        /home/bradfitz/repro.go:24 +0x24d
exit status 2

@bradfitz bradfitz added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 9, 2017
@bradfitz
Copy link
Contributor

bradfitz commented Jun 9, 2017

This isn't a dup of already-fixed #20541?

/cc @crawshaw @ianlancetaylor

@abhinav
Copy link
Contributor Author

abhinav commented Jun 9, 2017

The difference is that types in the original issue are hand-written and the types in this issue are generated by reflect.StructOf.

@ianlancetaylor
Copy link
Contributor

This is another aspect of #15924. Closing in favor of that one. Please comment if you disagree.

@abhinav
Copy link
Contributor Author

abhinav commented Jun 10, 2017

I think this issue is slightly different.

The problem here isn't the lack of wrapper methods but that the output of Type.Implements() is incorrect for these structs. This is a reflect.StructOf(..) version of #20541.

@ianlancetaylor
Copy link
Contributor

Ah, I see. In that case, I believe that this would be fixed on tip by the fix to #20541, if it weren't for the fact that the fix for #18780 means that you can no longer use reflect.StructOf to create types with unexported fields. If I give the field the name "Ident", then I get this output on tip:

foo implements ast.Expr: true
foo casts to ast.Expr: true

generated type implements ast.Expr: false
generated type casts to ast.Expr: false

Based on that I think this is fixed.

@golang golang locked and limited conversation to collaborators Jun 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants