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: decide, document whether custom Type implementations are supported #32303

Open
bradfitz opened this issue May 29, 2019 · 11 comments

Comments

Projects
None yet
8 participants
@bradfitz
Copy link
Member

commented May 29, 2019

Does the reflect package support users who implement Type themselves?

https://golang.org/pkg/reflect/#Type has unexported methods, but it's still possible to implement it by embedding a reflect.Type and using the normal (*rtype) type.

The documentation doesn't say that it's not allowed, but apparently people do it:

 https://godoc.org/gorgonia.org/tensor#Dtype

This apparently worked (enough? kinda?) at some point, and then we broke it, probably because we never imagined somebody would do this. That led to sending https://golang.org/cl/179338

This bug is to decide whether that's supported and to document. If it's supported, it needs tests.

/cc @ianlancetaylor @rsc @bcmills @randall77 @aclements @cherrymui

@bradfitz bradfitz added this to the Go1.13 milestone May 29, 2019

@bcmills

This comment has been minimized.

Copy link
Member

commented May 29, 2019

The documentation does say:

Type values are comparable, such as with the == operator, so they can be used as map keys. Two Type values are equal if they represent identical types.

That was added for #16348 in 2016.

It's not obvious to me that that can be reconciled with user-defined Type implementations.

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented May 29, 2019

It's not obvious to me that you couldn't argue that your own type was its own type, and couldn't just be comparable (to itself).

I don't think that sentence is enough documentation.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

I don't think we should try to support people who define their own implementation of reflect.Type. It's too tied into the details of the compiler.

@chewxy

This comment has been minimized.

Copy link

commented May 30, 2019

hi,

Author of said naughty functions here. I think it should be supported, albeit in a slightly different form. First I provide a high level motivation, followed by examples, a rationalization and two possible solutions.

The Gorgonia tensor library provides high-ish performance generic multidimensional slices. What this means is that I am able to perform tensor operations across multiple different data types in a high performance way. The high performance part is crucial for deep learning applications. So is the generic part. However the generics part allowed for explorations into different territories where tensor operations come in handy.

For example, I have written a CKY-style parser that relies on tensor operations of spans of texts; formal logics systems where tensor operations performed on logical forms; and many more.

The Extension example in https://godoc.org/gorgonia.org/tensor provides the most concrete use case of Dtype. Further motivations and explanations may be found in my Gophercon Singapore 2017 talk and the Tensor Refactor Experience Report.

Where it's used is mainly in the .Data() method, where the reflect.NewAt() function is used to create a new backing slice for the abstract tensor data structure.

So why is Dtype needed, instead of users just passing in a reflect.Type? The simple answer is that it's convenient as Dtype implements a hm.Type, which is used for Hindley-Milner style type inference for not just Gorgonia but a bunch of other things.

Solutions to the current situation is quite easy to fix: we can just destructure the Dtype and pass in the embedded reflect.Type when calling reflect.NewAt(). From this point of view, there is no need for @owulveryck's patch. Having said that, the patch is a simple change of a type assertion to use .common() which is a better way of handling things anyway.

The whole situation can further be remedied by having an unexported sealing method in reflect.Type (say, have *reflect.rtype implement a isReflectionType() method, which would be part of the reflect.Type methodset) - thus one may embed a reflect.Type but not create a reflect.Type. This preserves the usefulness of reflect.Type being used in the way Gorgonia does it to create generic data types, while disallowing other more nefarious uses

How this plays with different implementations, I have no idea.

@cherrymui

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

Solutions to the current situation is quite easy to fix: we can just destructure the Dtype and pass in the embedded reflect.Type when calling reflect.NewAt().

Yes, I think this is better.

@aclements

This comment has been minimized.

Copy link
Member

commented May 30, 2019

@chewxy, I think I understand why it's useful to have Dtype, but why is it useful that Dtype itself implement reflect.Type?

The whole situation can further be remedied by having an unexported sealing method in reflect.Type

This is already the case. reflect.Type has a common() *rtype method, so the only way for a type outside reflect to implement reflect.Type is by embedding reflect.Type.

@bradfitz said:

This apparently worked (enough? kinda?) at some point, and then we broke it, probably because we never imagined somebody would do this.

I don't really understand in what sense this has ever worked. I believe every exported function of reflect that works on reflect.Type asserts it to an *rtype. I can't even imagine how to make most of these work without doing that.

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented May 30, 2019

I don't really understand in what sense this has ever worked

That was a big assumption on my part because: a) that tensor code exists, and I assumed it wouldn't have been submitted somewhere if it didn't work, b) a CL was sent to fix it. So I assumed it used to work and recently stopped.

@cherrymui

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

So I assumed it used to work and recently stopped.

CL https://go-review.googlesource.com/c/go/+/96115

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented May 30, 2019

Good find.

@chewxy

This comment has been minimized.

Copy link

commented May 31, 2019

I think the issue is pretty settled now, I'm happy with the current solution (@cherrymui 's comment). Allow me however to answer a query from @aclements

I think I understand why it's useful to have Dtype, but why is it useful that Dtype itself implement reflect.Type

Dtype embeds reflect.Type because several of reflect.Type methods are useful in creating a runtime parametric polymorphic system. Specifically the Align(), Size() methods are useful for determining sizes to allocate dynamically. This is so that the library can know how much memory to allocate for custom types. This, used in concert with the implementation of hm.Type allows for type inference to happen in a way subject to progress and preservation

@rsc

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

decide, document whether custom Type implementations are supported

We never intended custom Type implementations to be supported. If there were some way to disallow reflect.Type from being embedded, I would do it. I don't think we should make any promises that that's OK.

chewxy added a commit to gorgonia/tensor that referenced this issue Jun 1, 2019

chewxy added a commit to gorgonia/tensor that referenced this issue Jun 1, 2019

Fixed Travis (#44)
* Fixed Travis

* Fixed reflect type issue per golang/go#32303
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.