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

proposal: reflect: allow creation of recursive types atomically #39717

Open
TheCount opened this issue Jun 19, 2020 · 24 comments
Open

proposal: reflect: allow creation of recursive types atomically #39717

TheCount opened this issue Jun 19, 2020 · 24 comments
Labels
Projects
Milestone

Comments

@TheCount
Copy link

@TheCount TheCount commented Jun 19, 2020

Background and introduction

The proposal in #39528 has been criticised for the fact that it would introduce "incomplete" instances of reflect.Type. This alternative proposal addresses that shortcoming: here, newly created recursive reflect.Types can emerge only through a single function, reflect.Bind. This new function either returns a reflect.Type which can be used with the same expectations as any other reflect.Type, or it panics (hence, "atomically"). The remainder of the reflect API is not affected.

This convenience comes at the cost of building a partially parallel infrastructure to describe the type to be newly created. The central component is a new interface reflect.TypeDesc instances of which are immutable descriptions of a type (or type-to-be) and a number of functions for the construction of such descriptions.

Proposal

We provide the proposal in the form of go source code. For orientation, the following new objects parallel existing objects in the reflect package:

New Existing
DescArray Array
DescChan Chan
DescFunc Func
DescInterface Interface
DescInvalid Invalid
DescKind Kind
DescMap Map
DescPtr Ptr
DescSlice Slice
DescStruct Struct
DescribeArray ArrayOf
DescribeChan ChanOf
DescribeFunc FuncOf
DescribeMap MapOf
DescribePtr PtrTo
DescribeSlice SliceOf
DescribeStruct StructOf
MethodDesc Method
StructFieldDesc StructField
TypeDesc Type

The following new objects have no parallel:

New
Bind
Complete
DescComplete
DescIncomplete
DescribeInterface
Incomplete

Proposal as go code:

// A DescKind represents the specific kind of type description that a TypeDesc
// represents. The zero DescKind is not a valid type description kind.
type DescKind uint

const (
	DescInvalid             = DescKind(Invalid)
	DescArray               = DescKind(Array)
	DescChan                = DescKind(Chan)
	DescFunc                = DescKind(Func)
	DescInterface           = DescKind(Interface)
	DescMap                 = DescKind(Map)
	DescPtr                 = DescKind(Ptr)
	DescSlice               = DescKind(Slice)
	DescStruct              = DescKind(Struct)
	DescIncomplete DescKind = (1 << 5) - 2
	DescComplete   DescKind = (1 << 5) - 1
)

// String returns the name of dk.
func (dk DescKind) String() string

// TypeDesc represents a description of a (possibly incomplete) type. A type
// description is a safe way to describe the layout of a (possibly recursive)
// type with the Complete, Incomplete, DescribeArray, DescribeChan,
// DescribeFunc, DescribeInterface, DescribeMap, DescribePtr, DescribeSlice,
// and DescribeStruct functions before the actual Type is created with Bind.
type TypeDesc interface {
	// ChanDir returns a channel description's direction.
	// It panics if the description kind is not DescChan.
	ChanDir() ChanDir

	// Elem returns the element type description for this type description.
	// It panics if the description kind is not DescArray, DescChan, DescMap,
	// DescPtr, or DescSlice.
	Elem() TypeDesc

	// Field returns a struct type description's i'th field description.
	// It panics if the description kind is not DescStruct.
	// It panics if i is not in the range [0, NumField()).
	Field(i int) StructFieldDesc

	// FieldByName returns the struct field description with the given name
	// and a boolean indicating if the field description was found.
	// It panics if the description kind if not DescStruct.
	FieldByName(name string) (StructFieldDesc, bool)

	// Key returns a map type description's key type description.
	// It panics if the description kind is not DescMap.
	Key() TypeDesc

	// Kind returns the specific description kind of this type description.
	Kind() DescKind

	// In returns the type description of a function type descriptions's i'th
	// input parameter.
	// It panics if the description kind is not DescFunc.
	// It panics if i is not in the range [0, NumIn()).
	In(i int) TypeDesc

	// IsVariadic reports whether a function type description's final input
	// parameter is a "..." parameter. If so, td.In(td.NumIn() - 1) returns the
	// parameter's implicit actual type description []T.
	//
	// For concreteness, if td describes func(x int, y ... float64), then
	//
	//	td.NumIn() == 2
	//	td.In(0) is a reflect.TypeDesc for "int"
	//	td.In(1) is a reflect.TypeDesc for "[]float64"
	//	td.IsVariadic() == true
	//
	// IsVariadic panics if the description kind is not DescFunc.
	IsVariadic() bool

	// Len returns the length property of an array description.
	// It panics if the description kind is not DescArray.
	Len() int

	// Method returns the i'th method description in the type description's
	// method description set.
	// It panics if the description kind is not DescInterface,
	// or if i is not in the range [0, NumMethod()).
	//
	// In particular, Method does not work on descriptions with kind DescStruct.
	// Method returns only method descriptions explicitly added with
	// DescribeInterface, not methods implicitly acquired through embedded fields.
	//
	// The returned method description describes the method signature, without a
	// receiver. The Func field is nil.
	//
	// There is no guarantee that the methods of the resulting type will have the
	// same sort order as in the description.
	Method(int) MethodDesc

	// MethodByName returns the method description for the method with that name
	// in the type descriptions's method description set and a boolean indicating
	// if the method was found.
	// It panics if the description kind is not DescInterface.
	//
	// The returned method description describes the method signature, without a
	// receiver. The Func field is nil.
	MethodByName(string) (MethodDesc, bool)

	// Name returns the name of the described type.
	// It panics if the description kind is not DescIncomplete.
	Name() string

	// NumField returns a struct type description's field description count.
	// It panics if the description kind is not DescStruct.
	NumField() int

	// NumIn returns a function type description's input parameter count.
	// It panics if the description type is not DescFunc.
	NumIn() int

	// NumMethod returns the number of method descriptions in the type
	// description's method set.
	// It panics if the description kind is not DescInterface.
	NumMethod() int

	// NumOut returns a function type description's output parameter count.
	// It panics if the description kind is not DescFunc.
	NumOut() int

	// PkgPath returns the designated package path in a description of an
	// incomplete type. It panics if the description kind is not DescIncomplete.
	PkgPath() string

	// Out returns the type description of a function type description's i'th
	// output parameter.
	// It panics if the description kind is not DescFunc.
	// It panics if i is not in the range [0, NumOut()).
	Out(i int) TypeDesc

	// Underlying returns the actual type underlying this type description.
	// It panics if the description kind is not DescComplete.
	Underlying() Type
}

// StructFieldDesc describes a struct field.
type StructFieldDesc struct {
	// Name is the field name. It must start with an uppercase letter.
	Name string

	// Type is the description of the field type.
	Type TypeDesc

	// Tag is the field tag string.
	Tag StructTag

	// Anonymous indicates whether or not this struct field description
	// describes an embedded field.
	Anonymous bool
}

// MethodDesc describes a method.
type MethodDesc struct {
	// Name is the method name. It must start with an uppercase letter.
	Name string

	// Type describes the method type.
	//
	// For interface descriptions, Type should describe the signature of the
	// method.
	//
	// For descriptions of incomplete types, Type should describe a function
	// whose first argument is the receiver.
	Type TypeDesc

	// Func is nil if this description describes an interface method.
	//
	// For descriptions of methods of incomplete types, Func is the method
	// implementation with receiver, arguments and return values in terms of
	// reflect.Value.
	// Methods for incomplete types are not implemented yet.
	Func func(recv Value, in []Value) []Value
}

// Incomplete creates a type description for an incomplete named type.
// It panics if name is not a valid identifier starting with an uppercase
// letter. The package name may be empty but it is recommended that each
// package use a unique string here (such as its fully qualified package name)
// to avoid naming clashes with other packages.
func Incomplete(pkgName, name string) TypeDesc

// Complete creates a description of the given already complete type.
func Complete(typ Type) TypeDesc

// DescribeArray creates a description of an array type with the given count and
// element type described by elem.
// It panics if elem's description kind is DescIncomplete.
func DescribeArray(count int, elem TypeDesc) TypeDesc

// DescribeChan creates a description of a channel type with the given
// direction and element type described by t.
//
// The gc runtime currently imposes a limit of 64 kB on channel element types.
// If t is not of description kind DescIncomplete and describes a type whose
// size is equal to or exceeds this limit, DescribeChan panics. If t describes
// an incomplete type, a later call to Bind will panic if the resulting type
// would violate this limit.
func DescribeChan(dir ChanDir, t TypeDesc) TypeDesc

// DescribeFunc creates a description of the function type with the argument
// and result types described by in and out.
//
// The variadic argument controls whether the description of a variadic function
// is returned. If variadic is true and in[len(in)-1] does not represent a
// slice (i. e., is either of description kind DescSlice, or DescComplete and
// describes a slice type), DescribeFunc panics.
func DescribeFunc(in, out []TypeDesc, variadic bool) TypeDesc

// DescribeInterface creates a description of the interface type with the
// methods described by methods.
func DescribeInterface(methods []MethodDesc) TypeDesc

// DescribeMap creates a description of the map type with key and
// element types described by key and elem, respectively.
// It panics if key's description kind is DescIncomplete.
//
// Map key types must be comparable (i. e., implement Go's == operator).
// DescribeMap does not perform this check, but a later call to Bind will
// panic if the resulting type would not be a valid key type.
func DescribeMap(key, elem TypeDesc) TypeDesc

// DescribePtr creates a description of the pointer type with element type
// described by t.
func DescribePtr(t TypeDesc) TypeDesc

// DescribeSlice creates a description of the slice type with element type
// described by t.
func DescribeSlice(t TypeDesc) TypeDesc

// DescribeStruct creates a description of the struct type with the fields
// described by fields.
func DescribeStruct(fields []StructFieldDesc) TypeDesc

// Bind creates a new type by binding the type name given by incomp to the type
// described by def.
//
// The methods argument must be nil. A later version may lift this restriction.
//
// It panics if the (package name, name) combination given incomp has been used
// to create a type with Bind before. This does not mean the name of the
// resulting type is unique as this restriction only applies to types created
// with Bind.
//
// It panics if the description kind of def is DescIncomplete, or if def
// indirectly references a description of an incomplete type other than incomp,
// or if a resulting type would be illegal for some reason (for example, a
// channel type whose element size would be too big, or a map key type not
// implementing Go's == operator).
func Bind(incomp, def TypeDesc, methods []Method) Type

Open questions

  • The proposal above does not explicitly allow creation of interfaces with embedded interfaces. If deemed necessary, the DescribeInterface function can get an additional embedded []TypeDesc parameter. This would allow the creation of interfaces with unexported methods if the embedded interfaces have unexported methods.
@cosmos72
Copy link

@cosmos72 cosmos72 commented Jun 19, 2020

I am continuing my implementation of #39528, and the incompleteness and mutability of the few reflect.Type created with the proposed new function Named is spreading to all kinds of types, through the functions ArrayOf ChanOf MapOf FuncOf SliceOf and StructOf. It starts looking like a major refactoring of reflect.Type methods.

So I welcome a different API that avoids this problem. Out of curiosity: is there a reason why you did not try to leverage go/types.Type API?

@TheCount
Copy link
Author

@TheCount TheCount commented Jun 20, 2020

Out of curiosity: is there a reason why you did not try to leverage go/types.Type API?

I briefly considered it but discarded the idea for the simple reason that I'm not terribly familiar with that API, which, at first sight, seemed unnecessarily complex to me. My interest in the reflect package comes from writing generic pieces of code, and while I might be wrong, I suspect it's similar for most users, so I tried to make the TypeDesc API as similar as possible to the familiar Type API. I understand that for people whose primary interest in reflect comes from parsing actual Go code, priorities might be different.

I'm not sure how important that actually is, but another aspect to consider might be the dependencies of the reflect package. I might be misinterpreting this, but it seems to me the implementers went to certain lengths to avoid pulling in "convenience" packages such as fmt. The biggest dependencies currently appear to be Unicode related. go/types with its own dependencies would significantly increase the size of reflect dependencies.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 20, 2020

The reflect package can't depend on go/types, because go/types depends on fmt and fmt depends on reflect. It would be quite awkward to rewrite go/types such that it does not depend on reflect in any way.

@rsc
Copy link
Contributor

@rsc rsc commented Jun 24, 2020

This is certainly a complex API to design.
I understand the need in a Go interpreter, but it's unclear that the benefit outweighs the very significant API cost.
We should keep thinking about whether there are ways to simplify the cost (API surface, complexity of implementation).

Maybe it would make sense to have a separate package with the same constructors as reflect but that operates only on these incomplete types:

package incomplete // import "reflect/incomplete"

type Type ...

func Complete(list []Type) []reflect.Type
func Of(reflect.Type) Type
func StructOf
PointerTo
ChanOf
MapOf
...

That would at least solve the "Desc everywhere" problem. I'm not sure how much that would end up being a second copy of reflect. And I'm not sure how methods get attached, if anyone wants to try to figure that out.

@cosmos72
Copy link

@cosmos72 cosmos72 commented Jun 24, 2020

An external package that allows building recursive reflect.Types could in theory depend on go/types.

Then the most minimal API is probably:

package convert // import "reflect/convert"
import (
    "go/types"
    "reflect"
)
func ToGoType(reflect.Type) *types.Named
func ToReflect([]*types.Named) []reflect.Type

where for simplicity ToGoType only accepts named (i.e. defined) reflect.Types.

To be implementable, it probably also needs this constraints: ToReflect can only convert types.Type which satisfy one of the two conditions:

  • either returned by a previous call of ToGoType - for symmetry and invertibility
  • or has an underlying type that, when traversed recursively, contains only unnamed types (which are themselves traversed recursively), plus named types returned by a previous call of ToGoType, plus named types passed to the same call of ToReflect - the idea is: you can convert mutually recursive named types A and B by calling ToReflect([]*types.Named{A, B})

This would allow a user to collect the named reflect.Types he wants, convert them to *types.Named, build a new *types.Named (say X) from them with the go/types API - which supports creating new named types and recursive types - then convert such new named type X to a reflect.Type

Clearly, reflect/convert would need to access some a lot of internal features and types of reflect: I'd say at least rtype, arrayType, chanType, mapType, funcType, interfaceType, ptrType, sliceType and structType.

Such API could even (if it's implementable) create new interface types - something that reflect cannot do yet.

And it would not need the proposed reflect.Type.Underlying()

@cosmos72
Copy link

@cosmos72 cosmos72 commented Jun 24, 2020

Update: or even

package convert // import "reflect/convert"
import (
    "go/types"
    "reflect"
)
func ToGoType(reflect.Type) types.Type
func ToReflect([]types.Type) []reflect.Type
@TheCount
Copy link
Author

@TheCount TheCount commented Jun 30, 2020

@rsc Separate package sounds good.

There would certainly be some overlap with existing reflect functionality. It seems unnatural exclude "incidentally" non-recursive types (maybe even unnamed types if there is a Complete API) from the new package, so the *Of functions of reflect would become redundant in the sense that the new API could be used to the same effect (the original *Of functions would still be easier to use for the quick creation of a non-recursive, unnamed type).

@cosmos72 your idea to leverage the go/types package sounds intriguing, especially given it comes with type checking facilities. I've never used it, though, and glancing at it, the API seems quite complex to me, and also not quite geared towards the kind of runtime type creation you'd want in reflect. For example, to create a named recursive type, you'd need a call to NewPackage, NewTypeName and NewNamed, and I don't know what would be sensible arguments for pos and path in a reflectesque context. Also, the type checker only works on an entire file set.

As for implementation complexity, I guess some stuff would probably have to be moved from reflect to some new reflect/internal package, in particular the lookup caches which reflect maintains in order to ensure that for two reflect.Types t1 and t2 we have t1 == t2 if and only if t1 and t2 are identical. The internal package would then be used by both reflect and the new package for incomplete types.

@cosmos72
Copy link

@cosmos72 cosmos72 commented Jun 30, 2020

The idea to leverage go/types is useful to understand what's the absolute minimum API needed to create recursive reflect.Types atomically. It has the downside of involving go/types, which:

  1. Has a verbose type creation API, slightly more complicated than strictly needed
  2. Is a large package - it also contains a lot of type-checking features, which are not used in this case
  3. Depends on fmt, which depends on reflect - the implementation of reflect/convert may become tricky, as it cannot depend on reflect

so that's the price to pay for the minimal API: offload the complexity to some other existing package, which does what's needed but also has other features (unused in this case)

Any other API which avoids go/types needs at least:

  1. A two-step process to create named and/or recursive types
  2. A whole series of functions ArrayOf ChanOf MapOf FuncOf PtrTo SliceOf StructOf to compose (possibly incomplete) types
  3. Convenience methods to examine the contents of (possibly incomplete) types: Key Elem ChanDir Field In Out Variadic etc.

thus it cannot be much simpler than your proposal or @rsc one.

@nrwiersma
Copy link

@nrwiersma nrwiersma commented Jul 1, 2020

Is it not possible to take a middle road, when we still have incomplete types, but only for a defined amount of time. Something like:

NamedOf(name Name, k Kind, m []Method, fn func(t Type) (underlying Type)) Type

This allows the incomplete type to only exist in the closure (as t) but it is an actual type of kind k and will panic if the underlying returned is not the same kind as k. It could be documented that functions like Elem on t would panic, and this no longer needs to be guarded. This seems simple enough to fit in the reflect package and does not cause any duplication of methods.

@rsc
Copy link
Contributor

@rsc rsc commented Jul 8, 2020

If you build on go/types you'd also have to build some kind of bridge between the types already built into the binary and the go/types API. Those are really meant to be separate. It doesn't make sense to cross-connect them just for reflection. Let's focus on non-go/types solutions.

Like @cosmos72 said above, it really does seem like the full API (as in #39717 (comment)) is needed, and we just want to get it as lightweight as possible.

Maybe someone would be interested to try to build the API sketched in that comment and see how well it works?

@TheCount
Copy link
Author

@TheCount TheCount commented Jul 12, 2020

Edit: updated according to remarks by @cosmos72.

@rsc

Attempt at expanding your sketch
// Package incomplete implements run-time reflection for incomplete types.
// Its purpose is the creation of recursive types.
//
// Invariants:
//
//     Complete([]Type{Of(t)}, nil)[0] == t
//     ArrayOf(n, Of(t)) == Of(reflect.ArrayOf(n, t))
//     ChanOf(d, Of(t)) == Of(reflect.ChanOf(d, t))
//     FuncOf([]Type{Of(in[0]), …, Of(in[len(in)-1])}, []Type{Of(out[0]), …, Of(out[len(out)-1]}, v) == Of(reflect.FuncOf(in, out, v))
//     MapOf(Of(k), Of(e)) == Of(reflect.MapOf(k, e))
//     PtrTo(Of(e)) == Of(reflect.PtrTo(e))
//     SliceOf(Of(e)) == Of(reflect.SliceOf(e))
//     StructOf([]StructField{
//       StructField{ Name: "Foo", Type: Of(ft) },
//       …,
//     }) == Of(reflect.StructOf([]reflect.StructField{
//       reflect.StructField{ Name: "Foo", Type: ft },
//       …,
//     })
package incomplete

import "reflect"

// Type represents an incomplete type, or part of an incomplete composite type.
// It is a safe way to define the layout of (possibly recursive) types
// with the Of, NamedOf, ArrayOf, ChanOf, FuncOf, InterfaceOf, MapOf, PtrTo,
// SliceOf, StructOf, and SetUnderlying functions before the actual types are
// created with Complete.
type Type interface {
	// interface shared with reflect.Type
	ChanDir() reflect.ChanDir
	IsVariadic() bool
	Len() int
	Name() string
	NumField() int
	NumIn() int
	NumMethod() int
	NumOut() int
	PkgPath() string

	// reflect.Type analogous methods
	Elem() Type
	Field(int) StructField
	FieldByName(string) (StructField, bool)
	Key() Type
	In(int) Type
	Method(int) Method
	MethodByName(string) (Method, bool)
	Out(int) Type

	// incomplete.Type-specific methods

	// Kind returns the specific kind of this type. It returns reflect.Invalid
	// if this type was returned by NamedOf and it has not been defined yet.
	Kind() reflect.Kind

	// Define defines a named type as the given type. It panics if the type's
	// kind is not reflect.Invalid, if the defining type's kind is
	// reflect.Invalid, or if the result would contain an invalid recursion.
	Define(Type)

	// AddMethod adds the given method to this type. It panics if there is a
	// method name clash, or if methods with distinct, non-empty PkgPath strings
	// are added. Furthermore, one of the following cases must apply:
	//
	// Case 1: this type was created with InterfaceOf.
	//
	// Case 2: this type was created with NamedOf and defined to a non-pointer,
	// non-interface type.
	//
	// Case 3: this type was created with PtrTo, with an element type which
	// Case 2 applies to.
	AddMethod(Method)

	// Underlying returns a type's underlying type.
	//
	// The underlying type of an interface type, an unnamed type, or a type with
	// a predeclared name is the type itself. For a user-defined non-interface
	// named type, the underlying type is the first unnamed or predeclared type
	// in its definition chain. For example, if the type represents A in
	//
	//     type A B
	//     type B byte
	//
	// then its underlying type is the type which represents byte (i. e., uint8).
	//
	// If a named type has not been defined yet, Underlying returns nil.
	Underlying() Type
}

// StructField is analogous to reflect.StructField, minus the Offset field.
type StructField struct {
	Name, PkgPath string
	Type          Type
	Tag           reflect.StructTag
	Index         []int
	Anonymous     bool
}

// Method represents an incomplete method.
// Unlike in reflect.Method, the implementing Func is not part of this
// structure.
type Method struct {
	Name, PkgPath string
	Type          Type // receiver = first arg, except for interface methods
}

// Of returns a Type representing the given complete reflect.Type.
func Of(reflect.Type) Type

// NamedOf creates the incomplete type with the specified package path and name.
// The name can be bound to an underlying type with the Define method.
func NamedOf(pkgPath, name string) Type

// ArrayOf creates an incomplete array type with the given count and
// element type described by elem.
func ArrayOf(count int, elem Type) Type

// ChanOf is analogous to reflect.ChanOf.
func ChanOf(dir reflect.ChanDir, elem Type) Type

// FuncOf is analogous to reflect.FuncOf.
func FuncOf(in, out []Type, variadic bool) Type

// InterfaceOf returns an incomplete interface type with the given list of
// named interface types. InterfaceOf panics if one of the given embedded types
// is unnamed or its kind is not reflect.Interface. It also panics if types
// with distinct, non-empty package paths are embedded.
//
// Explicit methods can be added with AddMethod.
func InterfaceOf(embedded []Type) Type

// MapOf creates an incomplete map type with the given key and element types.
func MapOf(key, elem Type) Type

// PtrTo is analogous to reflect.PtrTo.
func PtrTo(t Type) Type

// SliceOf is analogous to reflect.SliceOf.
func SliceOf(t Type) Type

// StructOf is analogous to reflect.StructOf.
func StructOf(fields []StructField) Type

// Complete completes the incomplete types in in, transforming them to a list
// of reflect.Type types. The function method is called once for each method
// added with AddMethod and should return an implementation of that method:
// a function whose first argument is the receiver.
// The list out contains the fully usable resulting types, except that methods
// can be called on them only after Complete has returned. The index indicates
// which type will be the method receiver, and stub indicates the method.
func Complete(
	in []Type,
	method func(out []reflect.Type, index int, stub Method) interface{},
) []reflect.Type

I've used @nrwiersma's idea to add method implementations in the Complete step, when the resulting types are completely constructed and usable, except for calling methods on their values.

This API is slightly more restrictive compared to what you can do at compile-time, but only as regards order of declarations. For example, given the run-time equivalent of the fragment

type A B
func (A) Print() { println("Foo") }
type B int

the API would not allow the declaration of Print before the declaration of B (because if B were bound to, say, *int instead, the method Print should not exist in the first place).

@cosmos72
Copy link

@cosmos72 cosmos72 commented Jul 13, 2020

Thanks for the effort :)
The 'invariants' part is useful, and the rest of the API looks quite complete.

Some quick comments:

	// Define defines a named type as the given type. It panics if the type's
	// kind is not reflect.Invalid, if the defining type's kind is
	// reflect.Invalid, or if the result would contain an invalid recursion.
	Define(Type)

what about SetUnderlying(Type)? It seems more intuitive to me

	Underlying() Type

Underlying may or may not be necessary, depending on the exact semantic of Define or SetUnderlying above.

// StructField is analogous to reflect.StructField, minus the Offset and Index
// fields.
type StructField struct {
	Name, PkgPath string
	Type          Type
	Tag           reflect.StructTag
	Anonymous     bool
}

Please keep the Index field, it costs basically nothing and makes programmers' life easier.

// MapOf creates an incomplete map type with the given key and element types.
// It panics if key's kind is reflect.Invalid.
func MapOf(key, elem Type) Type

MapOf also needs to accept key's kind = reflect.Invalid.
Otherwise you cannot create this type:

type Foo struct {
  Map *map[Foo]string
}

The check on whether key Type is acceptable as map key, and in general all non-recursion checks, must be delayed until Define or SetUnderlying gets invoked.

// StructOf is analogous to reflect.StructOf.
// It panics if one of the fields' type's kind is invalid.
func StructOf(fields []StructField) Type

Also, StructOf needs to accept fields whose type is incomplete, i.e. their kind is reflect.Invalid.
Otherwise you cannot create these mutually recursive types:

type Foo struct {
  Bar Bar
}
type Bar *struct { // notice the '*'
  Foo Foo
}
@TheCount
Copy link
Author

@TheCount TheCount commented Jul 13, 2020

Thanks for checking my post!

@cosmos72 wrote:

	// Define defines a named type as the given type. It panics if the type's
	// kind is not reflect.Invalid, if the defining type's kind is
	// reflect.Invalid, or if the result would contain an invalid recursion.
	Define(Type)

what about SetUnderlying(Type)? It seems more intuitive to me

I've decided against SetUnderlying and Bind because you can define a type A to a type B as in

type A B
type B int

but the underlying type of A is then int, and A is bound to that underlying type.

	Underlying() Type

Underlying may or may not be necessary, depending on the exact semantic of Define or SetUnderlying above.

Underlying may not return the same thing that was set with Define, so it may be useful.

// StructField is analogous to reflect.StructField, minus the Offset and Index
// fields.
type StructField struct {
	Name, PkgPath string
	Type          Type
	Tag           reflect.StructTag
	Anonymous     bool
}

Please keep the Index field, it costs basically nothing and makes programmers' life easier.

OK.

// MapOf creates an incomplete map type with the given key and element types.
// It panics if key's kind is reflect.Invalid.
func MapOf(key, elem Type) Type

MapOf also needs to accept key's kind = reflect.Invalid.
Otherwise you cannot create this type:

type Foo struct {
  Map *map[Foo]string
}

The check on whether key Type is acceptable as map key, and in general all non-recursion checks, must be delayed until Define or SetUnderlying gets invoked.

// StructOf is analogous to reflect.StructOf.
// It panics if one of the fields' type's kind is invalid.
func StructOf(fields []StructField) Type

Also, StructOf needs to accept fields whose type is incomplete, i.e. their kind is reflect.Invalid.
Otherwise you cannot create these mutually recursive types:

type Foo struct {
  Bar Bar
}
type Bar *struct { // notice the '*'
  Foo Foo
}

Yup, you're correct. I also removed the restriction that you cannot add methods to types which have already been embedded because, e. g., this here is legal:

type T struct {
  A
}
type A []T
func (A) Foo() {
  println("foo")
}
…
T{}.Foo()

So yes, we have to delay these checks.

Can the restriction not to declare methods on an t.Kind() == reflect.Invalid type remain? I think so, at the cost of the implementation of Type.Method* for interfaces and structs with embedded fields becoming somewhat more complex.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 13, 2020

I don't see an obvious reason to replicate reflect.Type in incomplete.Type. The program that constructs the incomplete type presumably does not need to interrogate it.

@cosmos72
Copy link

@cosmos72 cosmos72 commented Jul 13, 2020

@ianlancetaylor wrote:

I don't see an obvious reason to replicate reflect.Type in incomplete.Type. The program that constructs the incomplete type presumably does not need to interrogate it.

I guess you're talking about most methods of incomplete.Type in @TheCount #39717 (comment)
and my remark

  1. Convenience methods to examine the contents of (possibly incomplete) types: Key Elem ChanDir Field In Out Variadic etc.

I intentionally named them "convenience" methods: it's certainly possible to make incomplete.Types opaque (i.e. no way to inspect them) by removing most of their methods.

But using them it will not be pleasant: complicated code will need lots of them, and may even choose to keep them around, to avoid re-creating them every time. I think that making them opaque would often force to maintain the same information externally - duplicating it and risking discrepancies.

@cosmos72
Copy link

@cosmos72 cosmos72 commented Jul 13, 2020

I've decided against SetUnderlying and Bind because you can define a type A to a type B as in

type A B
type B int

but the underlying type of A is then int, and A is bound to that underlying type.

There are two alternatives here:

  1. the Type u passed to Type.Define(u Type) must be its own underlying type, i.e. u.Underlying() == u (or however you compare two incomplete.Types).
    The code type A B; type B int is then equivalent to:
import "reflect/incomplete"

var A = incomplete.NamedOf("A", "mypackage")
var B = incomplete.NamedOf("B", "mypackage")
B.Define(/*type of int*/) 
A.Define(B.Underlying()) // non-trivial dependency: A.Define() must be *after* B.Define()

This also requires Type.Underlying.

  1. the Type u passed to Type.Define(u Type) will not be used exactly - its underlying type will be used instead.
    The code type A B; type B int is then equivalent to:
import "reflect/incomplete"

var A = incomplete.NamedOf("A", "mypackage")
var B = incomplete.NamedOf("B", "mypackage")
A.Define(B)               // any order will do: A.Define() can be before or after B.Define()
B.Define(/*type of int*/) 

This does not strictly require Type.Underlying.

@TheCount
Copy link
Author

@TheCount TheCount commented Jul 14, 2020

@ianlancetaylor wrote:

I don't see an obvious reason to replicate reflect.Type in incomplete.Type. The program that constructs the incomplete type presumably does not need to interrogate it.

I don't think it's strictly necessary. A program which has from the beginning a clear plan from in its mind probably wouldn't interrogate the incomplete types. For something like a Go interpreter, where types may be constructed on the go, it might be convenient. In any case, it looks like a separable problem, so we could start with a minimal API and add interrogation methods later on.

FWIW, here is a stripped down version of the API.
package incomplete

import "reflect"

// Type represents an incomplete type, or part of an incomplete composite type.
// It is a safe way to define the layout of (possibly recursive) types
// with the Of, NamedOf, ArrayOf, ChanOf, FuncOf, InterfaceOf, MapOf, PtrTo,
// SliceOf, and StructOf functions before the actual types are created with
// Complete.
type Type interface {
	// Define defines a named type as the given type. It panics if the type
	// is not a named type, the type has already been defined, or if the result
	// would contain an invalid recursion.
	Define(Type)

	// AddMethod adds the given method to this type. The Index field of the given
	// method is ignored. It panics if there is a method name clash, or if
	// methods with distinct, non-empty PkgPath strings are added. Furthermore,
	// one of the following cases must apply:
	//
	// Case 1: this type was created with InterfaceOf.
	//
	// Case 2: this type was created with NamedOf and defined to a non-pointer,
	// non-interface type.
	//
	// Case 3: this type was created with PtrTo, with an element type which
	// Case 2 applies to.
	AddMethod(Method)
}

// StructField is analogous to reflect.StructField, minus the Index and Offset
// fields.
type StructField struct {
	Name, PkgPath string
	Type          Type
	Tag           reflect.StructTag
	Anonymous     bool
}

// Method represents an incomplete method.
// Unlike in reflect.Method, the implementing Func is not part of this
// structure.
type Method struct {
	Name, PkgPath string
	Type          Type // receiver = first arg, except for interface methods
	Index         int
}

// Of returns a Type representing the given complete reflect.Type.
func Of(reflect.Type) Type

// NamedOf creates the incomplete type with the specified name and package path.
// The name can be bound to an underlying type with the Define method.
func NamedOf(name, pkgPath string) Type

// ArrayOf creates an incomplete array type with the given count and
// element type described by elem.
func ArrayOf(count int, elem Type) Type

// ChanOf is analogous to reflect.ChanOf.
func ChanOf(dir reflect.ChanDir, elem Type) Type

// FuncOf is analogous to reflect.FuncOf.
func FuncOf(in, out []Type, variadic bool) Type

// InterfaceOf returns an incomplete interface type with the given list of
// named interface types. InterfaceOf panics if one of the given embedded types
// is unnamed or its kind is not reflect.Interface. It also panics if types
// with distinct, non-empty package paths are embedded.
//
// Explicit methods can be added with AddMethod.
func InterfaceOf(embedded []Type) Type

// MapOf creates an incomplete map type with the given key and element types.
func MapOf(key, elem Type) Type

// PtrTo is analogous to reflect.PtrTo.
func PtrTo(t Type) Type

// SliceOf is analogous to reflect.SliceOf.
func SliceOf(t Type) Type

// StructOf is analogous to reflect.StructOf.
func StructOf(fields []StructField) Type

// Complete completes the incomplete types in in, transforming them to a list
// of reflect.Type types. The function method is called once for each method
// added with AddMethod and should return an implementation of that method:
// a function whose first argument is the receiver.
// The list out contains the fully usable resulting types, except that methods
// can be called on them only after Complete has returned. The index indicates
// which type will be the method receiver, and stub indicates the method.
func Complete(
	in []Type,
	method func(out []reflect.Type, index int, stub Method) interface{},
) []reflect.Type

I've also swapped the order of arguments to NamedOf, so name and pkgPath match the order in StructField and Method (rather than the order in go/types).

@cosmos72: regarding type definitions, I would lean towards your 2., just so the behaviour is not too surprising for users of the API.

@rsc
Copy link
Contributor

@rsc rsc commented Jul 15, 2020

But using them it will not be pleasant: complicated code will need lots of them,

Why would code need them?
Those methods are for understanding an unknown reflect.Type that is passed to general-purpose code.
It's hard to see why you'd need them halfway through constructing a new type.
Wouldn't the code building the type know about the type?

It seems like it would be enough to just do a sentinel interface like:

type Type interface {
    isType() 
}

Unless you have a specific example of code where those extra methods would be needed?

Otherwise the sketch looks about right. Would you like to try implementing it and see if it satisfies your use case?

@tfieldmusic

This comment was marked as disruptive content.

@rsc
Copy link
Contributor

@rsc rsc commented Jul 22, 2020

@TheCount and/or @cosmos72, any interest in implementing the package sketched above and see if it serves your use case?

@cosmos72
Copy link

@cosmos72 cosmos72 commented Jul 22, 2020

My use case is a Go interpreter.

For this particular case, it can be modeled as a library that takes a slice of arbitrary go/types.TypeSpec as input, and uses reflect and reflect/incomplete to create a corresponding slice of reflect.Type.

I tried a mental experiment, and the proposed reflect/incomplete API seems powerful enough for such task, which can be summarized as "a library that creates reflect.Types from arbitrary source code".

About implementing reflect/incomplete: it's definitely non-trivial, in part because it needs access to reflect internal types, starting from *rtype

@mvertes
Copy link

@mvertes mvertes commented Jul 22, 2020

Hello, as developer of yaegi, also a go interpreter, I'm very interested in having 2 missing features in reflect:

  1. the ability to create recursive types (this is today poorly emulated in the interpreter),
  2. the ability to add methods to a created type, so it implements one or several interfaces (this is impossible to get it right without improving reflect).

To me, a problem is that the pre-compiled code must be able to check that an object instantiated from a such created type implements an existing interface. It seems not trivial to achieve.

Also the need to create a new interface type (not existing in the pre-compiled code) is less obvious to me, as only the interpreted code uses it, it can be handled internally in the interpreter (and is already, at least in gomacro and yaegi).

I understand that the proposed reflect/incomplete package is useful primarily to isolate the effort involved in the creation of a recursive type. It's not clear to me yet how it helps for the point 2. May be through the NamedOf ?

I agree also that this package is for reflect type creation only, not interrogation, as an interpreter has already its own type representation and spec.

Once successfully implemented, I don't get yet the interest of keeping this package exposed. It seems to rather belong to internal, and having the interesting functions accessible through reflect only.

But yes definitely interested to participate and help if I can, and put efforts on it. and for info, @nrwiersma an I work in the same team on yaegi.

@cosmos72
Copy link

@cosmos72 cosmos72 commented Jul 24, 2020

Question:
is it acceptable to move to a new package reflect/internal the types and functions from reflect that are also needed by this proposal?
Doing so would simplify the implementation and reduce duplicated code/types.

The types to move would be at least: rtype and its wrapper types: arrayType chanType funcType interfaceType mapType ptrType sliceType structType, plus structField and possibly others.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 24, 2020

@cosmos72 Perhaps you can use the already existing package internal/reflectlite. But yes, it would be OK to create a new internal package if required.

cosmos72 added a commit to cosmos72/go that referenced this issue Jul 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.