code review 6500065: reflect: add Convert (API CHANGE)

reflect: add Type.ConvertibleTo, Value.Convert (API CHANGE)

Fixes issue 4047.

[rsc, 2012-09-21 04:55:11]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[dsymonds, 2012-09-21 04:56:37]

Should there be a corresponding CanConvert?

[rsc, 2012-09-21 04:56:59]

On Fri, Sep 21, 2012 at 12:56 AM,  <dsymonds@golang.org> wrote:
> Should there be a corresponding CanConvert?

Yes, it's Type.ConvertibleTo. Will fix description.

[iant, 2012-09-21 13:59:00]

https://codereview.appspot.com/6500065/diff/4001/src/pkg/reflect/all_test.go
File src/pkg/reflect/all_test.go (right):

https://codereview.appspot.com/6500065/diff/4001/src/pkg/reflect/all_test.go#...
src/pkg/reflect/all_test.go:2383: {V(float64(169)), V(float64(169))},
Is it worth testing things like int(float32(1.4))?

https://codereview.appspot.com/6500065/diff/4001/src/pkg/reflect/all_test.go#...
src/pkg/reflect/all_test.go:2410: {V(int(-1)), V(string("\uFFFD"))},
Do these int("\u") tests really pass?  It looks like the values are not the
same.

https://codereview.appspot.com/6500065/diff/4001/src/pkg/reflect/all_test.go#...
src/pkg/reflect/all_test.go:2434: {V((***int)(nil)), V((***int)(nil))},
Can you introduce another pointer type, to verify that different pointer types
do not convert?

Similarly for chan, map, [], etc.

https://codereview.appspot.com/6500065/diff/4001/src/pkg/reflect/type.go
File src/pkg/reflect/type.go (right):

https://codereview.appspot.com/6500065/diff/4001/src/pkg/reflect/type.go#newc...
src/pkg/reflect/type.go:1196: if Bool <= kind && kind <= Complex128 || kind ==
UnsafePointer {
I wouldn't describe this as return true for "identical underlying types." 
Perhaps this function should have a different name.  Or maybe this condition
should be omitted altogether, I'm not sure what it is doing.

https://codereview.appspot.com/6500065/diff/4001/src/pkg/reflect/value.go
File src/pkg/reflect/value.go (right):

https://codereview.appspot.com/6500065/diff/4001/src/pkg/reflect/value.go#new...
src/pkg/reflect/value.go:1995: if dst.Kind() == Slice && dst.Name() == "" &&
dst.Elem().PkgPath() == "" {
The spec shows that converting a string to a named slice type is a valid
conversion.  It looks like that is being rejected here.

https://codereview.appspot.com/6500065/diff/4001/src/pkg/reflect/value.go#new...
src/pkg/reflect/value.go:2005: if dst.Kind() == String && src.Name() == "" &&
src.Elem().PkgPath() == "" {
Again, the spec permits converting a named slice type to string.

https://codereview.appspot.com/6500065/diff/4001/src/pkg/reflect/value.go#new...
src/pkg/reflect/value.go:2041: *(*uint64)(unsafe.Pointer(ptr)) = bits
Comment: Assume pointers are at least 4 bytes.

https://codereview.appspot.com/6500065/diff/4001/src/pkg/reflect/value.go#new...
src/pkg/reflect/value.go:2062: *(*float64)(unsafe.Pointer(ptr)) = v
Comment: Assume pointers are at least 4 bytes.

https://codereview.appspot.com/6500065/diff/4001/src/pkg/reflect/value.go#new...
src/pkg/reflect/value.go:2117: return makeInt(v.flag&flagRO, uint64(v.Int()), t)
Why do you need to preserve the flagRO field?  You are creating a new value. 
What's the harm in permitting the program to change that value?

Whatever the decision is here, should there be tests for the way this flag is
handled?

[rsc, 2012-09-21 14:43:17]

(Still other comments left to process.)

https://codereview.appspot.com/6500065/diff/4001/src/pkg/reflect/all_test.go
File src/pkg/reflect/all_test.go (right):

https://codereview.appspot.com/6500065/diff/4001/src/pkg/reflect/all_test.go#...
src/pkg/reflect/all_test.go:2410: {V(int(-1)), V(string("\uFFFD"))},
On 2012/09/21 13:59:00, iant wrote:
> Do these int("\u") tests really pass?  It looks like the values are not the
> same.

All runtime string conversions of invalid runes turn into the Unicode
replacement rune U+FFFD. Compile-time conversions too, it turns out, although
that wasn't entirely intentional.

https://codereview.appspot.com/6500065/diff/4001/src/pkg/reflect/type.go
File src/pkg/reflect/type.go (right):

https://codereview.appspot.com/6500065/diff/4001/src/pkg/reflect/type.go#newc...
src/pkg/reflect/type.go:1196: if Bool <= kind && kind <= Complex128 || kind ==
UnsafePointer {
On 2012/09/21 13:59:00, iant wrote:
> I wouldn't describe this as return true for "identical underlying types." 
> Perhaps this function should have a different name.  Or maybe this condition
> should be omitted altogether, I'm not sure what it is doing.

If this is a basic type, the underlying type is the builtin, so as long as kinds
agree they have the same underlying type.  I am using "identical underlying
type" in the sense from the spec. 
Will add a comment to that effect.

[rsc, 2012-09-21 18:39:06]

PTAL

https://codereview.appspot.com/6500065/diff/4001/src/pkg/reflect/value.go
File src/pkg/reflect/value.go (right):

https://codereview.appspot.com/6500065/diff/4001/src/pkg/reflect/value.go#new...
src/pkg/reflect/value.go:1995: if dst.Kind() == Slice && dst.Name() == "" &&
dst.Elem().PkgPath() == "" {
On 2012/09/21 13:59:00, iant wrote:
> The spec shows that converting a string to a named slice type is a valid
> conversion.  It looks like that is being rejected here.

That's what I thought too, but it's not what I found in the spec. Looks like I
found the wrong section. I filed issue 4123 for the spec.

Fixed code and added tests.

https://codereview.appspot.com/6500065/diff/4001/src/pkg/reflect/value.go#new...
src/pkg/reflect/value.go:2005: if dst.Kind() == String && src.Name() == "" &&
src.Elem().PkgPath() == "" {
On 2012/09/21 13:59:00, iant wrote:
> Again, the spec permits converting a named slice type to string.

Done.

https://codereview.appspot.com/6500065/diff/4001/src/pkg/reflect/value.go#new...
src/pkg/reflect/value.go:2041: *(*uint64)(unsafe.Pointer(ptr)) = bits
On 2012/09/21 13:59:00, iant wrote:
> Comment: Assume pointers are at least 4 bytes.

Done.

https://codereview.appspot.com/6500065/diff/4001/src/pkg/reflect/value.go#new...
src/pkg/reflect/value.go:2062: *(*float64)(unsafe.Pointer(ptr)) = v
On 2012/09/21 13:59:00, iant wrote:
> Comment: Assume pointers are at least 4 bytes.

Done.

https://codereview.appspot.com/6500065/diff/4001/src/pkg/reflect/value.go#new...
src/pkg/reflect/value.go:2117: return makeInt(v.flag&flagRO, uint64(v.Int()), t)
On 2012/09/21 13:59:00, iant wrote:
> Why do you need to preserve the flagRO field?  You are creating a new value. 
> What's the harm in permitting the program to change that value?
> 
> Whatever the decision is here, should there be tests for the way this flag is
> handled?

Any values obtained by reading an unexported field are tainted RO, to avoid
writes to package-local data via reflect. The RO applies not just to this value
but to any value found using it. For example if you grab an unexported p *int
from a struct, the RO field on the value for p keeps you also from writing to
what it points at. Because it's a taint, it needs to be preserved.

Actually the Convert method was requiring that the thing being converted must be
exported, which means not RO, so all this tracking was unnecessary. However, I
think it makes sense to allow the conversion and just propagate the flag. A
Printf for example might want to Convert to a set of standard types.

I removed the check in Convert (which wasn't documented to be there anyway) and
added tests that all the conversions preserve the RO.

[iant, 2012-09-21 18:48:18]

LGTM

[rsc, 2012-09-21 19:10:01]

Rob, please sign off on the API change.

[r, 2012-09-21 19:24:42]

https://codereview.appspot.com/6500065/diff/13002/src/pkg/reflect/type.go
File src/pkg/reflect/type.go (right):

https://codereview.appspot.com/6500065/diff/13002/src/pkg/reflect/type.go#new...
src/pkg/reflect/type.go:1182: return identicalUnderlyingTypes(T, V)
maybe verb this. haveIdenticalUnderlyingTypes

https://codereview.appspot.com/6500065/diff/13002/src/pkg/reflect/value.go
File src/pkg/reflect/value.go (right):

https://codereview.appspot.com/6500065/diff/13002/src/pkg/reflect/value.go#ne...
src/pkg/reflect/value.go:306: // It panics if v's underlying value is not a
slice of runes.
slice of runes (int32).

https://codereview.appspot.com/6500065/diff/13002/src/pkg/reflect/value.go#ne...
src/pkg/reflect/value.go:1236: // It panics if v's underlying value is not a
slice of runes.
ditto

https://codereview.appspot.com/6500065/diff/13002/src/pkg/reflect/value.go#ne...
src/pkg/reflect/value.go:1955: func convertOp(dst, src *commonType) func(Value,
Type) Value {
// convertOp returns the function than can convert src to dst.

https://codereview.appspot.com/6500065/diff/13002/src/pkg/reflect/value.go#ne...
src/pkg/reflect/value.go:2036: func makeInt(f flag, bits uint64, t Type) Value {
doc comments on these make functions. they're not simple to understand.

[rsc, 2012-09-22 04:58:51]

PTAL

[r, 2012-09-22 06:05:34]

LGTM

http://codereview.appspot.com/6500065/diff/5005/src/pkg/reflect/type.go
File src/pkg/reflect/type.go (right):

http://codereview.appspot.com/6500065/diff/5005/src/pkg/reflect/type.go#newco...
src/pkg/reflect/type.go:1182: return haveIdenticalUnderlyingTypes(T, V)
nit: should probably be haveIdenticalUnderlyingType (singular) since if true
there is only one such type.

[rsc, 2012-09-22 12:52:35]

*** Submitted as http://code.google.com/p/go/source/detail?r=a1dbdb805e2e ***

reflect: add Type.ConvertibleTo, Value.Convert (API CHANGE)

Fixes issue 4047.

R=iant, r
CC=golang-dev
http://codereview.appspot.com/6500065

[chalfant, 2012-09-22 15:46:23]

On 2012/09/22 12:52:35, rsc wrote:
> *** Submitted as http://code.google.com/p/go/source/detail?r=a1dbdb805e2e ***
> 
> reflect: add Type.ConvertibleTo, Value.Convert (API CHANGE)
> 
> Fixes issue 4047.
> 
> R=iant, r
> CC=golang-dev
> http://codereview.appspot.com/6500065

I think you may have a typo in this panic in type.go ("AssignableTo" vs
"ConvertibleTo"):

panic("reflect: nil type passed to Type.AssignableTo")

Sorry if this isn't the best way to provide you with this feedback (or if it is
unhelpful); I'm unfamiliar with this reviewing tool.

[rsc, 2012-09-22 16:07:46]

> I think you may have a typo in this panic in type.go ("AssignableTo" vs
> "ConvertibleTo"):
>
> panic("reflect: nil type passed to Type.AssignableTo")
>
> Sorry if this isn't the best way to provide you with this feedback (or
> if it is unhelpful); I'm unfamiliar with this reviewing tool.

Thanks for catching that. Email replies are great.

Russ