New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reflect: MakeInterface #4146

Open
bradfitz opened this Issue Sep 25, 2012 · 59 comments

Comments

Projects
None yet
@bradfitz
Member

bradfitz commented Sep 25, 2012

Feature request for MakeInterface, to go along with the newly-arrived MakeFunc (Issue
1765).

Good goal would be gomock without source code generation.
@rsc

This comment has been minimized.

Contributor

rsc commented Sep 25, 2012

Comment 1:

Labels changed: added priority-later, go1.2.

Status changed to Accepted.

@dsymonds

This comment has been minimized.

Member

dsymonds commented Sep 25, 2012

Comment 2:

Yes, this would be marvellous for GoMock.
@bradfitz

This comment has been minimized.

Member

bradfitz commented Sep 25, 2012

Comment 3:

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.
@rsc

This comment has been minimized.

Contributor

rsc commented Sep 25, 2012

Comment 4:

It is unclear to me why this would be enough to eliminate Go file
generation from GoMock. The implementations of preexisting interfaces
could be generated this way but not the mocking stubs that people call
to describe the expected call sequences.
Russ
@dsymonds

This comment has been minimized.

Member

dsymonds commented Sep 25, 2012

Comment 5:

It wouldn't eliminate the code generation, but it would halve it. That would be nice.
Eliminating the rest would require run-time type construction, which is outside the
scope of this issue.
@rsc

This comment has been minimized.

Contributor

rsc commented Sep 25, 2012

Comment 6:

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
@dsymonds

This comment has been minimized.

Member

dsymonds commented Sep 25, 2012

Comment 7:

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.
@rsc

This comment has been minimized.

Contributor

rsc commented Sep 25, 2012

Comment 8:

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.
@dsymonds

This comment has been minimized.

Member

dsymonds commented Sep 25, 2012

Comment 9:

Aah, I see where you're going. I was imagining that those methods
could be changed to take (...interface{}), and do type checks at run
time, but that still doesn't solve the very existence of EXPECT, and
nor does the compiler know about the existence of its return type's
Put method.
@extemporalgenome

This comment has been minimized.

extemporalgenome commented Jun 22, 2013

Comment 10:

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).
@rsc

This comment has been minimized.

Contributor

rsc commented Jul 30, 2013

Comment 11:

Not going to make the Go 1.2 cut. Not clear there's a use case anyway.

Labels changed: added go1.3maybe, removed go1.2.

@robpike

This comment has been minimized.

Contributor

robpike commented Aug 20, 2013

Comment 12:

Labels changed: removed go1.3maybe.

@rsc

This comment has been minimized.

Contributor

rsc commented Nov 27, 2013

Comment 13:

Labels changed: added go1.3maybe.

@rsc

This comment has been minimized.

Contributor

rsc commented Dec 4, 2013

Comment 14:

Labels changed: added release-none, removed go1.3maybe.

@rsc

This comment has been minimized.

Contributor

rsc commented Dec 4, 2013

Comment 15:

Labels changed: added repo-main.

@gopherbot

This comment has been minimized.

gopherbot commented May 11, 2014

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.
@anacrolix

This comment has been minimized.

Contributor

anacrolix commented Mar 8, 2015

@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

@bradfitz

This comment has been minimized.

Member

bradfitz commented Mar 31, 2016

@crawshaw, any interest? :)

@crawshaw

This comment has been minimized.

Contributor

crawshaw commented Apr 1, 2016

@bradfitz I don't have time for this in the 1.7 window.

@bcmills

This comment has been minimized.

Member

bcmills commented Aug 14, 2017

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.)

@jimmyfrasche

This comment has been minimized.

Member

jimmyfrasche commented Aug 14, 2017

@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.

@bcmills

This comment has been minimized.

Member

bcmills commented Aug 14, 2017

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.

@jimmyfrasche

This comment has been minimized.

Member

jimmyfrasche commented Aug 14, 2017

@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.

@bcmills

This comment has been minimized.

Member

bcmills commented Aug 14, 2017

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.

@jimmyfrasche

This comment has been minimized.

Member

jimmyfrasche commented Aug 14, 2017

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.

@stevenblenkinsop

This comment has been minimized.

stevenblenkinsop commented Aug 14, 2017

@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.

@jimmyfrasche

This comment has been minimized.

Member

jimmyfrasche commented Aug 15, 2017

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?

@bcmills

This comment has been minimized.

Member

bcmills commented Aug 15, 2017

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.)

@jimmyfrasche

This comment has been minimized.

Member

jimmyfrasche commented Aug 15, 2017

I think we need MakeValue(methods []Method) reflect.Value with the following semantics:

  • the returned Value's type has an arbitrary, unique, illegal name (say "1"), an invalid PkgPath, and the specified method set
  • the underlying type is struct{}
  • when methods are called on the value they are not passed a receiver—they're treated like ordinary function calls
  • type assertions on unexported methods respect the PkgPath of the method not the type

That 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

  • collect and compute the method set,
  • use MethodValue to turn the methods we're keeping into functions,
  • create a Method for each copying the Name, PkgPath from the original Method, and setting Func to the result of MethodValue,
  • and pass those to 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)
  }
}
@jimmyfrasche

This comment has been minimized.

Member

jimmyfrasche commented Aug 15, 2017

@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.

@stevenblenkinsop

This comment has been minimized.

stevenblenkinsop commented Aug 15, 2017

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()
@jimmyfrasche

This comment has been minimized.

Member

jimmyfrasche commented Aug 15, 2017

@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.

@jimmyfrasche

This comment has been minimized.

Member

jimmyfrasche commented Aug 17, 2017

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.

@jimmyfrasche

This comment has been minimized.

Member

jimmyfrasche commented Aug 21, 2017

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.

@jimmyfrasche

This comment has been minimized.

Member

jimmyfrasche commented Aug 21, 2017

If something like that was added to the reflect package itself, then fmt.Errorf could be rewritten to something like:

// Errorf formats according to a format specifier and returns the string
// as a value that satisfies error.
//
// The methods of the first error in the format args are still there
// [editor's note: I shouldn't write this docstring]
func Errorf(format string, a ...interface{}) error {
  msg := errors.New(Sprintf(format, a...))
  if err := firstError(a); err != nil {
    return reflect.Combine(err, msg)
  }
  return msg
}
func firstError(a []interface{}) error {
  for _, i := range a {
    if e, ok := i.(error); ok {
      return e
    }
  }
  return nil
}

That's not a light change to consider, it could have some weird consequences, but it would mean that a lot of code that hides optional methods on errors would stop doing so with no modification.

@jimmyfrasche

This comment has been minimized.

Member

jimmyfrasche commented Aug 21, 2017

Another issue: (should have tested this earlier) StructOf can't create an unexported field, though maybe that's not an issue, since you could only get to the fields with reflect anyway and they're fairly arbitrary. If you use it to created exported fields with an unexported type from another package you can set that field, which is mildly surprising, but that is what we want to do here: https://play.golang.org/p/wQ8dC45dmI

@jimmyfrasche

This comment has been minimized.

Member

jimmyfrasche commented Aug 23, 2017

Let's say for the moment that reflect can create new types with methods, something equivalent to what I wrote above is added to the stdlib, and that it becomes the blessed way, by convention, of composing errors.

This approach works well for errors with additional methods but not so well for errors with additional fields or sentinel errors, like io.EOF. IsX style funcs, like os.IsPermission, that dispatch on the identity of the type don't play well with this scheme.

Errors with additional fields can be made to work with this approach by adding an unexported method and an exported helper. For example, let's examine os.PathError. If it adds a method like

func (p *PathError) pathError() *PathError {
  return p
}

and the OS package adds a func like

func IsPathError(err error) *PathError { // result == nil implies false
  if p, ok := err.(interface{pathError() *PathError}); ok {
    return p.pathError()
  }
  return nil
}

then an error, under any number of combinations (with non-PathErrors), is still fully inspectable. Helpers like IsPermission could use similar tactics behind the scenes to extract the correct underlying error for their tests.

There's a small increase in API and implementation but everything works and is Go1 compatible.

Sentinel errors are a bit more annoying. They could follow the same basic approach, but each one would need its own type, like

type eof struct{}
func (e eof) Error() string { return "EOF" }
func (e eof) isEOF() {}
var EOF = eof{}
func IsEOF(err error) bool {
  _, eof := err.(interface{isEOF()})
  return eof
}

I don't think that's strictly Go1 compatible, but the only code I can think of that that would break is something using reflect. Perhaps it would be permissible as part of the transition to Go2?

@jimmyfrasche

This comment has been minimized.

Member

jimmyfrasche commented Sep 28, 2017

It looks like the specific approach taken in the gist I posted relied on a bug ( #22073 ).

If it wants to expose unexported methods, it would need to use embedding to handle the method promotion (once that is in place) and only create proxy methods as tie-breakers for exported methods.

However that has several unhappy implications:

  1. it cannot remove any "useless" values as it cannot detect when they do not contribute to the resultant method set
  2. it cannot create tie-breakers for unexported methods which implicitly removes the method from the method set due to the ambiguity

1 is unfortunate but non-essential.

However, 2 means it just cannot handle unexported methods correctly. It needs to stick to only exported methods and always hide all unexported methods.

So the approach in the gist is correct and it can discard values that don't add to the method set, but it's less useful than I'd hoped because it can't handle unexported methods.

In order to handle unexported methods in the same manner as exported methods, it would need to either be a builtin or a function defined in the reflect package that circumvents the usual protections.

@dsnet

This comment has been minimized.

Member

dsnet commented Apr 18, 2018

I hit a use case for this while working on google/go-cmp#88. The gist of the problem is that TypeOf and ValueOf can never directly return a type that is an interface. I understand why it is the case, but it goes against user expectation:

b := new(bytes.Reader)
reflect.TypeOf(b)            // *bytes.Reader
reflect.TypeOf(io.Reader(b)) // io.Reader expected, but got *bytes.Reader

This causes an issue when doing something like:

equateNotExist := cmp.Comparer(func(x, y error) bool { ... })
var e1, e2 error = syscall.ENOENT, os.ErrNotExist
cmp.Equal(e1, e2, equateNotExist) // false

The user expected this to be true since there is a custom comparer that knows how to compare error interfaces. However, this returns false because under the hood, cmp.Equal uses reflect.ValueOf and loses the fact that the user intended to pass in two error interfaces. cmp.Equal see two different types syscall.Errno and *errors.errorString and immediately returns false without even giving the custom comparer a chance to work.

If you so much as pass even a pointer to the interface, then it works as expected:

cmp.Equal(&e1, &e2, equateNotExist) // true

However, doing so is not intuitive at all.

In an effort to make this more user-friendly, my approach was to try to promote the initial top-level value into an interface with the common set of methods to both values:

// Determine the set of common methods.
type method struct {
	Name string
	Type reflect.Type
}
mm := make(map[method]int)
for i := 0; i < vx.NumMethod(); i++ {
	mm[method{vx.Type().Method(i).Name, vx.Method(i).Type()}]++
}
for i := 0; i < vy.NumMethod(); i++ {
	mm[method{vy.Type().Method(i).Name, vy.Method(i).Type()}]++
}
var ms []reflect.Method
for m, n := range mm {
	if n == 2 {
		ms = append(ms, reflect.Method{Name: m.Name, Type: m.Type})
	}
}

// Promote to interface type with common method set.
iface := reflect.InterfaceOf(ms)
return vx.Convert(iface), vy.Convert(iface)

but this does not work since there is no reflect.InterfaceOf method.

@Merovius

This comment has been minimized.

Merovius commented Apr 18, 2018

@dsnet I might misread this, but the documentation says that

The comparer f must be a function "func(T, T) bool" and is implicitly filtered to input values assignable to T. If T is an interface, it is possible that f is called with two values of different concrete types that both implement T.

That would seem to cover the case you mention? In particular this works.

@dsnet

This comment has been minimized.

Member

dsnet commented Apr 18, 2018

@Merovius, so as to keep this thread about InterfaceOf, I'll comment on google/go-cmp#88 regarding your comment. My point in my previous comment is that I had use-case for InterfaceOf to create a dynamic interface that contains the common set of methods for 2 concrete types.

@matttproud

This comment has been minimized.

Contributor

matttproud commented Apr 18, 2018

I don't want to be a party pooper, but adding a feature like reflect.MakeInterface — or any other way of creating arbitrary types to fulfill interfaces at runtime — pushes the Go complexity clock quite close to midnight. My fear hearkens from building, maintaining, and diagnosing production Java servers that pushed the JRE through things like cglib and similar machinations. I am really not trying to sound like reactionary, but these systems were an absolute nightmare and enabled bounds of complexity in Inversion of Control harnesses and other obtuse stacks of indirection. The only thing that could — barely — tame JRE dynamic code generation is Java's superb runtime debugger ecosystem, which we don't have.

I skimmed over the issue's feedback and wondered: is there value in having a discussion 1. whether we should have this feature at all and 2. whether the benefits disproportionately outweigh the likely costs.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Apr 18, 2018

@matttproud I assume you are talking about the case of adding methods to a type created by using the reflect package. E.g., adding methods to a type created via reflect.StructOf. I clarify because I think that the function that does that operation should not be called reflect.MakeInterface. I think that reflect.MakeInterface, possibly better named reflect.InterfaceOf, is a function that creates a new interface type, just as reflect.StructOf creates a new struct type. I don't see any particular harm in being able to do that.

Personally I'm also not too worried about being able to add methods to a type created by reflect. That is nothing like Java's cglib. In Go, the code for the actual methods will be, essentially, a function literal. Adding methods to a type will amount to a different way of calling a function literal. Yes, the stack trace will be complex, but this is still much less powerful and much less complex than cglib.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment