Open bradfitz opened 12 years ago
While we're listing use cases, I'd also be able to take an arbitrary http.ResponseWriter value from a client (which may implement optional http.Hijacker and/or io.WriterTo and/or http.Flusher) and change a method or two on it, while still forwarding all other methods on it. Currently whenever I want to implement an http.ResponseWriter wrapper (which I continue to do regularly), I have to defensively implement all 3 optional interfaces, with the implementation bodies checking at runtime whether the wrapper ResponseWrapper implements those, otherwise I break people's flushing or sendfile or hijacking, when perhaps all I want to do is record Writes that go by, or change the underlying transport, or watch for errors, etc. Doing something like: type myWrapper struct { io.Writer http.ResponseWriter } ... is an error, due to Write ambiguity, and something like: type myWrapper struct { http.ResponseWriter } func (myWrapper) Write([]byte) (int, error) { ... } ... hides Flush, WriteTo, Hijack, etc. I could see where this road leads to abuse and performance problems, though. But it's painful either way.
Eliminating half the generation for gomock may be compelling for you as the author but it doesn't change at all the way people interact with gomock. It's still a separate preprocessor. I'd be much more excited if we could get rid of all the Go file generation entirely. That's a qualitative difference. However, even with runtime type construction I don't believe you can eliminate the other half. You need something at compile time for the mock user to import. Russ
You don't. The existing API for GoMock generated code only exports the mock implementation of the interface; the recorder is returned by an EXPECT method on that. I don't see any reason why something needs to be imported. Instead of foo := mock_foo.NewMockFoo(ctrl) you could write foo := gomock.NewMock(ctrl, (*Foo)(nil)) // passing a pointer to the interface and get back a dynamically created mock.
Yes but keep going. What does the code using foo look like? Suppose it says foo.EXPECT(). The compiler must see a definition of a struct or interface with an EXPECT method at compile time in order to build that. Where did that come from? Here's a line from sample/user_test.go: mockIndex.EXPECT().Put("b", gomock.Eq(2)) // matchers work too I don't see how the compiler can compile that line without a gomock-generated import defining something with a Put method that will accept gomock.Eq(2) as an argument.
That 'qualitative difference' can also be achieved by other means, such as build tool support for custom pre-compilation phases, possibly specified by build directives (e.g. a // +prebuild comment in GoMock could specify that the GoMock preprocessor be run on anything that imports the mocking package).
Comment 16 by martin@probst.io:
Wouldn't it be possible to write something like Mockito's API: ctrl := ... defer ctrl.Finish() mockFoo := gomock.NewMock(ctrl, (*Foo)(nil)) gomock.When(mockFoo.Put("b", gomock.Eq(2))). ThenReturn(PutResponse{}, nil) myTestCode() I.e. NewMock creates a mock that returns zero values for all calls. When() returns an Expectation interface that allows users to set expectations, e.g. using functions like "ThenReturn(retVals ...interface{})". NewMock internally tracks the last call to any method, calls on the returned Expectation interface set up the return values for any subsequent calls. You lose type safety on the values, i.e. ThenReturn is essentially untyped, but that might be acceptable in return for no code generation. In Java-land, that is solved using generics - When() could be parameterized as "func template <T> When(argument <T>) Expectation<T>" (pseudo syntax), so that "ThenReturn()" calls could require the correct types.
@bradfitz You mention having to implement wrappers for optional interfaces of http.ResponseWriter, are there any examples in http standard library? I've come across this problem as you've described it: https://groups.google.com/d/topic/golang-nuts/HKY3mI7Q2jY/discussion
@crawshaw, any interest? :)
@bradfitz I don't have time for this in the 1.7 window.
I have been considering implementing this for 1.8. There are some details in the runtime's itab code to work through, but that aside I think the API needs a little more consideration. It's not really MakeInterface because it's not making an abstract interface type. Instead the function is creating a new concrete type with a method set.
When writing Go we can add a method set to any named type. So I lean towards a function for creating a new named type, with an optional method set:
func NamedOf(t Type, name string, methodNames []string, methods []func(args []Value) results []Value) Type
This could reuse the reflect.Method struct for what looks like a simpler signature, but it will add a few new states to the Method struct (for example, will the Type field be used when passed to NamedOf?):
func NamedOf(t Type, name string, methods []Method) Type
@bradfitz does this work for your http.ResponseWriter case?
I'm fine with reflect.Named
, but it's not the same as reflect.MakeInterface
. The meaning of reflect.MakeInterface
(or reflect.InterfaceOf
) would be to create a new interface type that could then be used with reflect.Value.Convert
to produce a new interface value (and you could use Method(0)
to get the first method, etc.). I don't know why such a thing would be useful, but it is meaningful.
I agree that MakeInterface is different (and valid in its own right), and if someone can think of a good use for it I'll give it a go too. Brad's earlier example looked like it needed NamedOf instead, so I co-opted this issue. I'll make another.
The ability to create a new types with a method sets would greatly reduce the boilerplate in many use cases I have. I find myself forced to implement various interfaces(i.e. MarshalJSON/UnmarshalJSON, datastore.PropertyLoadSaver) when I need to unmarshal data into types with overlapping fields even if the process/body of the methods is basically same except the types involved. See a stub below[0]. This could be generalised [1] if there was a way to create types with method sets.
[1]
// NewQueryDecoder creates a virtual struct using StructOf and implements PropertyLoadSaver
dq := NewQueryDecoder(reflect.TypeOf(T1{}), reflect.TypeOf(T2{}), reflect.TypeOf(T3{}))
// Execute a query
// dq.Query wraps the datastore query, allocates a new value using the virtual struct
// and returns the value matching one of the types from NewQueryDecoder as interface{}
result, err := dq.Query("filter1=", true, "filter2=", false)
// type assert
switch result.(type){
case T1:
case T2:
case T3
}
type T1 struct{
F1 string
FT1 bool
}
type T2 struct{
F1 string
FT2 bool
}
type T2 struct{
F1 string
FT3 bool
}
[0]
type TOneOf struct{
Type string `datastore:"@type"`
*T1
*T2
*T3
}
func (x *TOneOf) Load(ps []datastore.Property) error {
For k := range ps{
if ps[k].Name != "@type"{
continue
}
x.Type = ps[k].Value.(string)
break
}
st := reflect.TypeOf(x)
switch x.Type{
case RefReflect(st.Field(1)):
val = reflect.New(st.Field(1))
if err = datastore.LoadStruct(val.Interface(), ps); err != nil{
return err
}
reflect.ValueOf(x).Field(1).Set(val.Elem())
return nil
case RefReflect(st.Field(2)):
val = reflect.New(st.Field(2))
if err = datastore.LoadStruct(val.Interface(), ps); err != nil{
return err
}
reflect.ValueOf(x).Field(2).Set(val.Elem())
return nil
case RefReflect(st.Field(3)):
val = reflect.New(st.Field(3))
if err = datastore.LoadStruct(val.Interface(), ps); err != nil{
return err
}
reflect.ValueOf(x).Field(3).Set(val.Elem())
return nil
}
return errors.New("Invalid type: "+ x.Type)
}
// RefReflect returns the type path (importPath.Type)
// e.g. encoding/json.Encoder
func RefReflect(t reflect.Type)string{
// stub
return t.String()
}
@crawshaw, you wrote:
and if someone can think of a good use for it I'll give it a go too.
I found in https://go-review.googlesource.com/#/c/27321/3/src/sort/sort.go (prototype for #16721) that the the interface wrapper around my funcs eats a ton of my time. If a MakeInterface
let me avoid that, I would love it. But that sort code couldn't depend on reflect anyway, so it probably wouldn't help me regardless. :-/
Oh, but it'd still help me if I decide to put this whole slice sorting helper in its own package.
You want to create methods, right? That's a job for #16522, which I'm having a hard time with. A function named MakeInterface would create a new interface type, like interface { F() Foo }
. Which is much easier but maybe not useful?
Yeah, sorry, I want to create an interface value with methods I define, implementing some existing interface. This bug is about creating an interface type now I guess? But #16522 also says its signature returns a type.
Which is about creating a value?
Well once you have the type you can use reflect.New(t).Elem() to get the value.
(Maybe it was a mistake to move the original meaning of the bug, perhaps they should have swapped titles.)
Oh, sure, to get it with its zero concrete value. I guess I just don't care about its concrete value so I was ignoring that.
I just want to set the itab values, basically. But now I realize from https://github.com/golang/go/issues/16522#issuecomment-237012698 that it's not what I want, since my function pointers are go11func pointers instead. And they're actually closures, so I can't cheat and hand you just the code half. I'll follow along in #16522
Hi,
I've written a user experience report that references this ticket in a potential solution to what I've seen as a larger problem. I hope it can either help prioritize this ticket, or give context to what I would want in an ideal solution.
https://medium.com/@cep21/interface-wrapping-method-erasure-c523b3549912
@cep21, great write-up!
@cep21
Hmm, that's an interesting take on it. I've hit the wrapping problem several times myself, but I hadn't really made the connection between that and MakeInterface
.
My takeaway from it was that we should stop using "magic methods" and replace them with some kind of compile-time introspection (e.g. metaprogramming support as part of #15292). All the complaints people have about the complexity of compile-time metaprogramming seem to apply equally well (if not more) to this kind of run-time metaprogramming.
I'm having trouble seeing how this would solve the interface wrapping method erasure problem.
Would the signature be something like MakeInterface(methods []struct { Name string; Fn func(args []Value) (results []Value)) Value
and the returned value would be an essentially anonymous type with the specified methods?
@jimmyfrasche I would expect something a bit more dynamic than that. By analogy with StructOf
and MakeFunc
, and interpreting MakeInterface
as roughly an inverse of (reflect.Value).Method
:
// InterfaceOf returns the interface type containing methods. The Func
// and Index fields are ignored and computed as they would be by the compiler.
func InterfaceOf(methods []Method) Type {
…
}
// MakeInterface returns a new interface of the given Type that wraps methods.
// len(methods) must equal typ.NumMethod(), and the Value for methods[i] must
// have a type assignable to typ.Method(i).Type.
func MakeInterface(typ Type, methods []Value) Value {
…
}
So the pseudocode for wrapping an error would be roughly:
get the method set of your wrapper
get the method set of dynamic value of the error to wrap
remove methods from the latter shared by the former
append the former to the latter
return MakeInterface(TypeOf(error(nil)), computedMethods).Interface().(error)
I was thinking len(computedMethods)
would need to match the length of the underlying type.
Forwarding unexported methods makes for an interesting problem, though: reflect.InterfaceOf
would presumably need to support unexported methods, but MakeInterface
shouldn't allow the caller to inject them (other than by forwarding to an existing implementation).
I think that implies an extra constraint on MakeInterface
:
// If typ.Method(i).PkgPath is not empty, methods[i] must be the result of a call
// to (Value).Method for a method with the same package-qualified name.
Then the forwarding code would be roughly:
func forwardMethods(wrapper, underlying error) error {
type methodName struct{Name, PkgPath string}
seen := make(map[methodName]bool)
var (
methods []reflect.Method
impls []reflect.Value
)
for _, v := range []reflect.Value{reflect.ValueOf(wrapper), reflect.ValueOf(underlying)} {
t := v.Type()
for i := 0; i < t.NumMethod(); i++ {
m := t.Method(i)
name := methodName{m.Name, m.PkgPath}
if seen[name] {
continue
}
seen[name] = true
methods = append(methods, m)
impls = append(impls, v.Method(i))
}
}
return reflect.MakeInterface(reflect.InterfaceOf(methods), impls).Interface().(error)
}
But for this API to be useful for wrapping type-asserted methods, it would need to have the (somewhat odd) property that reflect.MakeInterface
actually produces a value of a concrete (non-interface) type. (Otherwise there is nothing to type-assert against.)
@bcmills I think that's why I find this confusing. The name MakeInterface is kinda off.
MakeInterface, in your example above, is making a non-interface type which conforms to an interface (which seems redundant since it's given the Methods which contain all the necessary information). So what it's really doing is making a named type (or whatever they're called now) without a name but with a method set.
You could type assert that value against arbitrary interfaces (which is the goal) but not the underlying type since that only exists in the runtime.
I'd also assume that reflect.TypeOf(forwardMethods(a, b)) != reflect.TypeOf(forwardMethods(a, b))
since it would have to create a new type each time.
what it's really doing is making a named type (or whatever they're called now) without a name but with a method set.
That's an interesting observation! In that sense, you could imagine a function that generalizes MakeInterface
to construct an arbitrary named type with methods:
// NewTypeOf returns a new named type with the given underlying type and methods.
// The Index and Type fields of each Method are ignored and computed as they
// would be by the compiler.
// The Func field of each method must be a value of a function type,
// and the "underlying" type (or a pointer to that type) must be assignable to
// the function's first argument, which is treated as the method receiver.
// The Name of the returned type is an arbitrary string that is not the name of
// any other type, and its PkgPath is [...?].
func NewTypeOf(underlying Type, methods []Method) Type {
…
}
You could construct values of the returned type using the existing reflect.New
and reflect.Zero
functions.
I'd also assume that
reflect.TypeOf(forwardMethods(a, b)) != reflect.TypeOf(forwardMethods(a, b))
since it would have to create a new type each time.
I think they would need to be equal. I would expect them to be equal, any any rate: they are both the type "reflected instance of interface I
", where I
is itself the same (unnamed) interface type for both.
But we could make that explicit by adding both NewTypeOf
and InterfaceOf
, in which case you could store the concrete types in a sync.Map
indexed by interface type.
@bcmills how does that satisfy the use-case of avoiding interface method erasure?
That let's you frankenstein a type out of two types but you need a way to say "this method forwards invocation to this value and that method forwards invocation to that other value." In addition to building the method set you need to specify the value to use as the receiver for each method separately.
That's why I was thinking it would have to be more like a closure but for types (and hence why I assumed equality would not hold).
I'm starting to think that neither reflection or generics are good solutions to this problem and there just needs to be a way in the language to embed an interface in an "open" fashion where it transparently "forwards" type asserts to unmatched interfaces, like @cep21's struct{ wrap(error) }
construct.
you need a way to say "this method forwards invocation to this value and that method forwards invocation to that other value."
Presumably you'd do that the same way you would currently hand-write the wrapper: to merge N values the space-efficient way, you make a struct type with N fields, and define each method to forward the call to the appropriate field.
Or, to do it the less-code way, you define a struct type with a function field for each method, and define each method to call the function in the corresponding field.
It's admittedly quite a bit more boilerplate than a MakeInterface
that synthesizes a concrete type on its own, but it's a bit more consistent with the Go language proper.
That would be some annoying code to write but otoh it would only need to be written once and everyone could use it. There would be a lot of indirection when you called a method on it. That would be fine for an error, probably less so for an http.ResponseWriter
though.
Perhaps it could be implemented in the reflect package, even if the necessary primitives were all present, since it could likely cheat and do more optimizations than user code.
A pro for having it be in the language is that you could still assert to the concrete type of the wrapper should the need arise. If the type is built at runtime you'd have n versions, one for each wrapped type, and none exist at compile time.
@jimmyfrasche One option is that StructOf
left open the possibility of promoting methods from fields. You could create a struct type using StructOf
which embeds a concrete type, and then add methods using NewTypeOf
(or whatever it's called). The problem with this is that the new "methods" being closures means that the type would essentially have static members shared by all instances. The InterfaceOf
approach at least makes it so that the underlying value is the collection of values captured by the method closures.
Based on the idea that you should be able to cache pairs of types, I tried writing out a more complete implementation using NewTypeOf
.
I wanted to use reflect to do the equivalent of:
type createdAtRuntime struct {
s S
t T
}
func (c createdAtRuntime) SomeMethodNotOnT() {
c.s.SomeMethodNotOnT()
}
// other methods of S not on T
func (c createdAtRuntime) SomeMethodOnSandT() {
c.t.SomeMethodOnSandT()
}
// other methods of T
That way I could cache createdAtRuntime
and create new instances just by setting the fields.
The code was mostly straightforward.
Create a struct with two fields a
and b
of the appropriate types.
Collect the relevant methods and create proxies that call them on a
or b
appropriately.
I ran into a problem though.
In order to create the proxies I needed the result of NewTypeOf
but of course the proxies are the methods that I want to pass to NewTypeOf
. I'm not sure of a way around that. (The code for the proxies is here https://play.golang.org/p/xUSfI92a7O if it's of interest but the rest was certainly uninteresting).
Also, I'm not sure how unexported methods would work with this. If I have a "Wrap" func defined in package A and in package B I create a value from a type defined in package B and one in C, then can I assert for interfaces containing unexported methods in package B? What if I pass the value to package C?
In order to create the proxies I needed the result of NewTypeOf but of course the proxies are the methods that I want to pass to NewTypeOf.
Right, we'd need an explicit carve-out to allow the receivers of those methods to be the underlying type rather than the actual receiver type. (In my comment, I described that as 'the "underlying" type (or a pointer to that type) must be assignable to the function's first argument'.)
Also, I'm not sure how unexported methods would work with this.
I think you'd have to use embedding, which implies that NewTypeOf
(and by extension StructOf
) would have to handle embedded fields. (StructOf
currently does not, but explicitly documents that it may in the future.)
I think we need MakeValue(methods []Method) reflect.Value
with the following semantics:
Value
's type has an arbitrary, unique, illegal name (say "1"), an invalid PkgPath
, and the specified method setstruct{}
PkgPath
of the method not the typeThat let's us create a value with an arbitrary method set and type from regular functions.
Let's also say there was a way to create a method value† from a given reflect.Value
(there may be one hidden in reflect but I couldn't spot it). Just to have some notation let's say it's reflect.MethodValue(v reflect.Value, methodName string) reflect.Value
where the returned Value
is the function.
Then given two arbitrary Value
's to combine, we just need to
MethodValue
to turn the methods we're keeping into functions,Method
for each copying the Name
, PkgPath
from the original Method
, and setting Func
to the result of MethodValue
,MakeValue
.† I don't see this used much in the wild so if anyone reading is unfamiliar, given a type T
with a method func (t T) M(a int, b string) float64
and a value of that type v
then v.M
is equivalent to calling the below
func methodValueOfM(t T) func(int, string) float64 {
return func(a int, b string) float64 {
return t.M(a, b)
}
}
@bcmills yes, sorry—missed the point about the underlying type to break the loop. That does have the disadvantage that you couldn't create a method which invokes another method using reflection as the receiver doesn't have the method set yet. Nothing to do with the use-case under discussion, but a limitation nonetheless.
Maybe it needs to be a builder pattern where NewTypeOf(underlying Type)
returns some type that has an AddMethod(reflect.Method)
method and a Build() reflect.Type
method? Not especially idiomatic but without separating the steps in someway I think you're always going to have some edge to hit. The piecemeal construction avoids the issue.
Back to the use-case, as far as embedding, since we're, by definition, trying to combine two types with intersecting method sets we could only embed one of the two types so you'd have the same problems just with one less type to worry about except you'd also need to worry about a method colliding the embedded types name.
But that made me realize that in order for any of this to work we'd potentially need to create the equivalent of
package x //where the CombineValues func is defined
struct {
a y.typeName
b z.typeName
}
since the types we're combining come in interfaces so the underlying type could very well be unexported and is likely from another package. For that to work the constructed type would have to operate by different rules than ordinary Go types, which I'm sure would cause all kinds of subtle problems.
The MakeValue
approach suffers from the same issue but in a way that seems intuitively safer to me as the methods are siloed. You could do some weird stuff with MakeFunc
. I think it would be easier to reason about the interactions there. But I'm just speculating.
Field names conflicting with methods is potentially unfortunate. However, the solution already exists, since Anonymous bool
can be set separately from Name string
(the type aliases proposal describes this as a solved problem).
As for what the receiver type would need to be for NewTypeOf
methods, if it can be Value
or an interface type, that would work. It is unfortunate though.
As for unexported methods, the behaviour of promotion in the language is that unexported methods are promoted, so you should be able to assert to an interface containing those methods (provided the method name in the interface and the method name on the type originate in the same package).
MakeValue
could also work. An interesting potential is that your "two Values" could be
val1 := reflect.ValueOf(Wrap{ inner: err })
val2 := val1.Field(0).Elem()
@stevenblenkinsop good point about Anonymous
—with that it's just a matter of generating a name longer than any methods, aaaaaaaaaa
instead of just a
, and so on.
The issue with the receiver type is that instead of
func (f Foo) Bar() {...}
you have to create the methods like
func (f struct{ aaaaa S; bbbbb T}) Bar()
but that should actually be fine if NewTypeOf
goes through and fixes up the signatures (and it would have to) since when it's invoked the reflect.Value passed in as the receiver would be of the correct type, so my previous concern was simply due to insufficient blood-coffee levels.
The interesting thing about this wrt to unexported methods is that if you create a type from two types defined in separate packages, you could have two unexported methods on the created type with the same Name but different PkgPaths so it could, say, assert to interface{foo()}
in both packages but call different methods. That is how it should behave, but it is a bit weird.
Having thought about this more I think @bcmills solution could work. I'll write up a version using it and my MakeValue
later tonight to compare them and see if I run into any more issues.
Still playing with this, but I think NewTypeOf
is the only way forward.
All of the other alternatives (including first class support in the language) become O(n) when composed.
If you had a function for combining two values in some way, say Combine
, there's going to be cases where, for example, an error is returned, wrapped, returned, wrapped, and so on, several times resulting in something equivalent to Combine(Combine(Combine(err1, err2), err3), err4)
. Each layer adds another onion layer of indirection. For errors this isn't a big deal but for other uses like an http.ResponseWriter
it could be a deal breaker.
However, with NewTypeOf
, an implementation of Combine
could detect types it created and optimize it so that there was never more than one level of indirection. That makes construction of the type more involved but that can be easily cached.
I've created a gist of a rough implementation. Warning: a little over 300 LOC.
While it's an optimizing implementation, the implementation is far from optimized. I pessimized it fairly extensively to make it easier to understand what it's doing since I can't actually run it to verify. It should be relatively straightforward to reduce allocations and the number of passes over the data, though.
To avoid creating a tree of combined values that has to proxy and re-proxy method invocations up to the depth of the tree, it adds a marker field as the 0th field of the struct it creates. When it's called on a value whose type has that marker field, like Combine(Combine(a, b), Combine(c, d))
, it "flattens" everything out and computes the minimal method set as if it had been called like a hypothetical CombineVariadic(a, b, c, d)
. It also discards any fields, aside from the marker field, that do not contribute to the method set, allowing them to be garbage collected when they do not contribute anything.
A simpler, non-optimizing implementation would work, too, but it would have unfortunate performance implications in all but the simplest of cases making it less likely to be used.