code review 101330055: reflect: ArrayOf, FuncOf, InterfaceOf, and StructOf.

reflect: ArrayOf, FuncOf, InterfaceOf, and StructOf.

Add missing type creation functionality to reflect.
Fixes issue 5748.
Fixes issue 5996.

[crc, 2014-06-21 23:24:56]

Hello golang-codereviews@googlegroups.com (cc:
golang-codereviews@googlegroups.com),

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

[iant, 2014-06-23 03:31:31]

https://codereview.appspot.com/101330055/diff/40001/src/cmd/ld/go.c
File src/cmd/ld/go.c (right):

https://codereview.appspot.com/101330055/diff/40001/src/cmd/ld/go.c#newcode608
src/cmd/ld/go.c:608: // keep each beginning with 'typelink.'
I think this will bloat programs with unnecessary data, so it seems best to
avoid it.  Can you explain why it is necessary?

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/alg.go
File src/pkg/reflect/alg.go (right):

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/alg.go#ne...
src/pkg/reflect/alg.go:102: return !(a == astring || a == ainter || a ==
anilinter || a == aslice ||
Perhaps use a switch statement.

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/alg.go#ne...
src/pkg/reflect/alg.go:103: (a < amem || a >= amax)) // implementations outside
algarray
It seems to me that float and complex also require special treatment.

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

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/type.go#n...
src/pkg/reflect/type.go:1230: if tm.typ != vm.typ {
This does not look right.  If we find the method, but the type is wrong, then
the type does not match the interface.  Same comment below.

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/type.go#n...
src/pkg/reflect/type.go:2263: // Name returns a new Type with with the same
underlying type as t but named according to
It's not clear to me why this function is desirable.  Perhaps this could be
moved to a separate CL for separate discussion.

https://codereview.appspot.com/101330055/diff/40001/src/pkg/runtime/alg.goc
File src/pkg/runtime/alg.goc (right):

https://codereview.appspot.com/101330055/diff/40001/src/pkg/runtime/alg.goc#n...
src/pkg/runtime/alg.goc:500: *eq = 1;
This looks wrong if you have a struct that has a normal field followed by a
field that is a struct.  The normal field may compare non-equal, and set *eq to
0.  There is no early exit, so we come to the next field, which is a struct, and
here we set *eq to 1.  We've lost the fact that the first field compared
non-equal.

[gmk, 2014-06-23 09:18:26]

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

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/all_test....
src/pkg/reflect/all_test.go:3486: // even though it is not refered to by any
compiled symbols. It also tests
referred

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/all_test....
src/pkg/reflect/all_test.go:3693: runtime.GC()
why GC?

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/all_test....
src/pkg/reflect/all_test.go:3695: runtime.GC()
why GC?

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/all_test....
src/pkg/reflect/all_test.go:3731: shouldPanic(func() {
Is this testing that Name=="" panics or that Type=="" panics?  Please add a
comment.

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/all_test....
src/pkg/reflect/all_test.go:3742: })
Add tests for anonymous fields:  calling inherited methods and satisfying
interfaces.

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

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/type.go#n...
src/pkg/reflect/type.go:1424: // The lookupCache caches ChanOf, MapOf, and
SliceOf lookups.
also mention StructOf, FuncOf, InterfaceOf

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/type.go#n...
src/pkg/reflect/type.go:1487: func FuncOf(in, out []Type, dotdotdot bool) Type {
please name the parameter "variadic" instead of "dotdotdot"

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/type.go#n...
src/pkg/reflect/type.go:2010: // StructOf returns the struct type containing
fields. Field offsets are ignored
How are anonymous fields specified, with Anonymous==true or with Name==""?  In
the former case, can the name be empty or must it match the type name?

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/type.go#n...
src/pkg/reflect/type.go:2269: func Name(t Type, pkgPath, name string) Type {
Consider calling it NamedOf for consistency.  And put the Type parameter last.

Will it be possible to attach methods to these named types?  If, so shouldn't
they be provided at construction time?

[adonovan, 2014-06-26 16:39:19]

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

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/type.go#n...
src/pkg/reflect/type.go:2040: // Embedded field
I haven't looked at the whole CL yet, but supporting this case is tricky since a
type struct{...; T; ...} may have a non-empty method set consisting of wrapper
functions that delegate to methods of T.  Ordinarily these wrappers are created
by the compiler, but for a reflectively created type, the wrappers will need to
be created dynamically.  How do you propose to do that?

[crc, 2014-07-05 18:09:20]

Thank you for you comments. It seems I have not fully considered the case when
StructOf() is used with an anonymous field with a non-empty method set.

This is going to take some time to fix as I am currently employed and only have
weekends to look at this.

https://codereview.appspot.com/101330055/diff/40001/src/cmd/ld/go.c
File src/cmd/ld/go.c (right):

https://codereview.appspot.com/101330055/diff/40001/src/cmd/ld/go.c#newcode608
src/cmd/ld/go.c:608: // keep each beginning with 'typelink.'
On 2014/06/23 03:31:30, iant wrote:
> I think this will bloat programs with unnecessary data, so it seems best to
> avoid it.  Can you explain why it is necessary?

I have left an useless comment at the test
TestInterfaceOfContainingCompiledMethodType
https://codereview.appspot.com/101330055/patch/40001/50004#newcode3485
but the name of the test somewhat describes the problem.
Consider:
type X int
func (x X) Ident() X {
  return x
}
func main() {
  a := FuncOf(nil, []Type{TypeOf(X(0))}, false)    // a = func() X
  b := ValueOf(X(0)).MethodByName("Ident").Type()  // b = func() X
  // a == b ?
}

If Ident() remains unused throughout the program, the symbol marker won't be
able to reach it. However, with FuncOf I can create a, which has the same type
as b.Ident(). To avoid creating a duplicate type, I require the original symbol
be accessible in .typelink. This is true even though Ident() was not referenced
in the compiled code.

I would appreciate your insight for a better solution as I too realise this
bloats the executable. I believe this is only a problem for methods, so if all
methods could be marked that may solve the problem.

I will fix the comment on the test to reflect this reply.

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/alg.go
File src/pkg/reflect/alg.go (right):

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/alg.go#ne...
src/pkg/reflect/alg.go:102: return !(a == astring || a == ainter || a ==
anilinter || a == aslice ||
On 2014/06/23 03:31:30, iant wrote:
> Perhaps use a switch statement.

Agreed, thank you.

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/alg.go#ne...
src/pkg/reflect/alg.go:103: (a < amem || a >= amax)) // implementations outside
algarray
On 2014/06/23 03:31:30, iant wrote:
> It seems to me that float and complex also require special treatment.

Good catch, thank you.

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

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/all_test....
src/pkg/reflect/all_test.go:3486: // even though it is not refered to by any
compiled symbols. It also tests
On 2014/06/23 09:18:25, gmk wrote:
> referred

Thank you

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/all_test....
src/pkg/reflect/all_test.go:3695: runtime.GC()
On 2014/06/23 09:18:25, gmk wrote:
> why GC?

I based these tests on those for other kinds such as Slices. For example,
TestSliceOf
func TestSliceOf(t *testing.T) {
	// check construction and use of type not in binary
	type T int
	st := SliceOf(TypeOf(T(1)))
	v := MakeSlice(st, 10, 10)
	runtime.GC()

calls runtime.GC() immediately after MakeSlice. I assume this tests that no part
of the slice data structure is marked for collection by the GC. If I am wrong
here please elaborate. Thank you.

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/all_test....
src/pkg/reflect/all_test.go:3731: shouldPanic(func() {
On 2014/06/23 09:18:25, gmk wrote:
> Is this testing that Name=="" panics or that Type=="" panics?  Please add a
> comment.

This is testing that two fields named "string" produces a panic.

struct {
  string
  string string
}

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/all_test....
src/pkg/reflect/all_test.go:3742: })
On 2014/06/23 09:18:25, gmk wrote:
> Add tests for anonymous fields:  calling inherited methods and satisfying
> interfaces.

good catch, thank you.

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

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/type.go#n...
src/pkg/reflect/type.go:1230: if tm.typ != vm.typ {
On 2014/06/23 03:31:30, iant wrote:
> This does not look right.  If we find the method, but the type is wrong, then
> the type does not match the interface.  Same comment below.

The reason for this code change is that when all interface 'name's were compiled
constant strings, pointer comparison sufficed (same reason "abc" == "abc" works
with an optimising C compiler). Now that interfaces can be constructed at
runtime, a full strcmp is required, and writing all conditions on the same line
became messy.

I have broken apart the original condition into what I believe is an equivalent
set of continue conditions by applying De Morgan's and negating each condition.
If I have mucked this up let me know but it still looks ok to me.

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/type.go#n...
src/pkg/reflect/type.go:1424: // The lookupCache caches ChanOf, MapOf, and
SliceOf lookups.
On 2014/06/23 09:18:26, gmk wrote:
> also mention StructOf, FuncOf, InterfaceOf

good catch, thank you

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/type.go#n...
src/pkg/reflect/type.go:1487: func FuncOf(in, out []Type, dotdotdot bool) Type {
On 2014/06/23 09:18:25, gmk wrote:
> please name the parameter "variadic" instead of "dotdotdot"

agreed, thank you

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/type.go#n...
src/pkg/reflect/type.go:2010: // StructOf returns the struct type containing
fields. Field offsets are ignored
On 2014/06/23 09:18:26, gmk wrote:
> How are anonymous fields specified, with Anonymous==true or with Name==""?  In
> the former case, can the name be empty or must it match the type name?

This definitely needs better doc. Anonymous fields are specified with Name=="".

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/type.go#n...
src/pkg/reflect/type.go:2040: // Embedded field
On 2014/06/26 16:39:19, adonovan wrote:
> I haven't looked at the whole CL yet, but supporting this case is tricky since
a
> type struct{...; T; ...} may have a non-empty method set consisting of wrapper
> functions that delegate to methods of T.  Ordinarily these wrappers are
created
> by the compiler, but for a reflectively created type, the wrappers will need
to
> be created dynamically.  How do you propose to do that?

This is an Oh Crap! moment for me. Whilst obvious in hindsight I had not
considered the possibility of unnamed types having a non-empty method set. I
have created a minimal program which will fail due to this issue.

type X int

func (x X) Ident() X {
	return x
}

type Xy interface {
	Ident() X
}

func main() {
	xt := reflect.TypeOf(X(0))
	ft := reflect.StructField{Type: xt}
	st := reflect.StructOf([]reflect.StructField{ft})
	xy := reflect.ValueOf(new(Xy)).Elem()
	xy.Set(reflect.New(st).Elem())
	xy.MethodByName("Ident").Call(nil)
}

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/type.go#n...
src/pkg/reflect/type.go:2269: func Name(t Type, pkgPath, name string) Type {
On 2014/06/23 09:18:26, gmk wrote:
> Consider calling it NamedOf for consistency.  And put the Type parameter last.
Agreed
> 
> Will it be possible to attach methods to these named types?  If, so shouldn't
> they be provided at construction time?

As per the previous comment, It seems I am going to need to handle attaching a
method set to unnamed types. I suspect that once I have code for doing so this
wont be so difficult.

https://codereview.appspot.com/101330055/diff/40001/src/pkg/runtime/alg.goc
File src/pkg/runtime/alg.goc (right):

https://codereview.appspot.com/101330055/diff/40001/src/pkg/runtime/alg.goc#n...
src/pkg/runtime/alg.goc:500: *eq = 1;
On 2014/06/23 03:31:30, iant wrote:
> This looks wrong if you have a struct that has a normal field followed by a
> field that is a struct.  The normal field may compare non-equal, and set *eq
to
> 0.  There is no early exit, so we come to the next field, which is a struct,
and
> here we set *eq to 1.  We've lost the fact that the first field compared
> non-equal.

does the loop condition, *eq && i < s->fields.len, not provide this early exit
resulting in *eq == 0?

[adonovan, 2014-07-07 08:52:15]

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

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/type.go#n...
src/pkg/reflect/type.go:2040: // Embedded field
My advice would be simply to forbid anonymous fields in the arguments to
StructOf, i.e. panic.
We can always add support for them later, and the current functionality provides
most of the benefit.

[gmk, 2014-07-13 12:39:47]

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

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/all_test....
src/pkg/reflect/all_test.go:3695: runtime.GC()
On 2014/07/05 18:09:19, crc wrote:
> On 2014/06/23 09:18:25, gmk wrote:
> > why GC?
> 
> I based these tests on those for other kinds such as Slices. For example,
> TestSliceOf
> func TestSliceOf(t *testing.T) {
> 	// check construction and use of type not in binary
> 	type T int
> 	st := SliceOf(TypeOf(T(1)))
> 	v := MakeSlice(st, 10, 10)
> 	runtime.GC()
> 
> calls runtime.GC() immediately after MakeSlice. I assume this tests that no
part
> of the slice data structure is marked for collection by the GC. If I am wrong
> here please elaborate. Thank you.

I guess that makes sense.  Maybe you should ask the author of those other tests.
 And verify that the test fails if some part of the data structure is marked for
collection; if this is a reasonable concern then I suppose it won't be hard to
manage.  But I'm not familiar with the implementation.

I think these calls to runtime.GC deserve comments describing their purpose.

https://codereview.appspot.com/101330055/diff/40001/src/pkg/reflect/all_test....
src/pkg/reflect/all_test.go:3731: shouldPanic(func() {
On 2014/07/05 18:09:19, crc wrote:
> On 2014/06/23 09:18:25, gmk wrote:
> > Is this testing that Name=="" panics or that Type=="" panics?  Please add a
> > comment.
> 
> This is testing that two fields named "string" produces a panic.
> 
> struct {
>   string
>   string string
> }

Then shouldn't it read:

StructOf([]StructField{
        StructField{Type: TypeOf("string")},
        StructField{Name: "string", Type: TypeOf("string")},
})

?

[gobot, 2014-12-19 05:02:43]

R=close

To the author of this CL:

The Go project has moved to Gerrit Code Review.

If this CL should be continued, please see the latest version of
https://golang.org/doc/contribute.html for instructions on
how to set up Git and the Go project's Gerrit codereview plugin,
and then create a new change with your current code.

If there has been discussion on this CL, please give a link to it
(golang.org/cl/NNNNNNN is best) in the description in your
new CL.

Thanks very much.