Bug: Cannot return nil for generic type T, when T is an interface

[mwittie]

Description

For the interfaces

type Builder[T any] interface {
	Build() (T, error)
}

type Thing interface {
	DoStuff()
}

I'd like to mock Builder behavior as

thingBuilder := new(mocks.Builder[Thing])
thingBuilder.On("Build").Return(nil, errors.New("some error"))

But I get an error like:

panic: interface conversion: interface is nil, not Thing

Shouldn't mockery know that Thing is an interface and so Builder[Thing].Build() should be able to return (nil, error)?

Mockery Version

$ mockery --version
19 Oct 22 17:49 MDT INF Starting mockery dry-run=false version=v2.14.0
v2.14.0

Golang Version

$ go version
go version go1.19 linux/amd64

Installation Method

• Binary Distribution
• Docker
• brew
• go install
• Other: [specify]

Expected Behavior

Can mock generic return types as nil when they are in fact interfaces.

Actual Behavior

Tests panic

[dlwyatt]

This is a workaround to get the test working with the current version of mockery:

thingBuilder.On("Build").Return(func() Thing { return nil }, errors.New("some error"))

[dlwyatt]

Possible fix: Generate the following code in all non-error cases, instead of the current isNillable check:

			v := ret.Get(0)
			if !reflect.ValueOf(&v).Elem().IsZero() && !reflect.ValueOf(v).IsZero() {
				r0 = v.(T)
			}

r0 is already the zero value for whatever its type is. The first call to IsZero (on ValueOf(&v).Elem()) covers the case where v is a nil, invalid interface (avoiding the panic in this issue), and the second call (on ValueOf(v) looks at whatever concrete type was passed into Return, assuming it's not the invalid nil interface. If both of those IsZero checks are false, then r0 needs to have a value assigned, and I think the type assertion v.(WhateverType) should always work.

This also requires an addition of g.addPackageImportWithName(ctx, "reflect", "reflect") up in NewGenerator

(The second check may not be strictly necessary, I was just trying to match the behavior of the code that mockery currently generates. It seems to work fine with just if !reflect.ValueOf(&v).Elem().IsZero() { even if the type is a pointer, map, slice, etc and the value is nil.)

[dlwyatt]

Even just putting if v != nil in all cases (not bothering to check for nillable or using reflect) might work, tbh.

[sergisimo]

Hi I've had the same issue and changing the generated code to this makes this usecase work:

if ret.Get(0) != nil {
    r0 = ret.Get(0).(R)
}

I'm not familiar with the code on this project so it would take me a while to create a PR to add this, if I have time I'll look into it an create a PR with a proposition.

[LandonTClipp]

nil interface{} doesn't implement Thing so how could this work? How would you return nil from a function that returns (Thing, error)?

[dlwyatt]

Interface values can be assigned nil in go. That's their zero value. (You can also assign nil of a concrete type that implements the interface, cast that to the interface type, and return it. The difference is that calling a method on the nil interface panics, but calling a method in the second scenario will actually call the concrete type's implementation, with a nil receiver. The distinction probably isn't important here, for the purpose of generating mock code; you'd likely only be returning nil when error is non-nil anyway, and callers should be checking for that.)

[LandonTClipp]

I understand that, but in this particular case:

thingBuilder.On("Build").Return(nil, errors.New("some error"))

nil gets stored as untyped (or rather, an empty interface) so the solution is either to:

1. Send a nil-valued but properly typed interface to Return
2. Use the .EXPECT() methods.

Another case why .EXPECT() is always recommended!

[dlwyatt]

I'm not understanding the problem here. The only thing that fails is the type conversion inside the mock, which can be avoided by checking for nil first. What the caller does with the nil interface is up to them.

[LandonTClipp]

Ah okay, on first glance I misinterpreted what was happening. Once thing to note is that using the RunAndReturn methods results in expected behavior:

thingBuilder.EXPECT().Build().RunAndReturn(func() (Thing, error) { return nil, nil })

To be honest, this is the cleanest way to handle this because the core issue is that when you do On("Builder").Return(nil, nil), the first return value gets stored as an uninitialized interface{}. There is no type information regarding Thing that is retained. That's the crux of this complication, and why we have to create these weird workarounds. Using RunAndReturn gets around this because the uninitialized T/Thing is always stored in the type system as T/Thing, not as interface{}.

To be honest, this is one of the warts of mockery: the fact that we have to store expected arguments and return types inside interface{}. Using RunAndReturn kind-of-sort-of gets around this fact. Going back to the generated mock for Builder, you can see why:

	var r0 T
	var r1 error
	if rf, ok := ret.Get(0).(func() (T, error)); ok {
		return rf()
	}
	if rf, ok := ret.Get(0).(func() T); ok {
		r0 = rf()
	} else {
		r0 = ret.Get(0).(T)
	}

And this is kind of the same thing you suggested in #513 (comment).

Going back to the post you made almost 2 (!) years ago:

			v := ret.Get(0)
			if !reflect.ValueOf(&v).Elem().IsZero() && !reflect.ValueOf(v).IsZero() {
				r0 = v.(T)
			}

This seems kind of redundant? reflect.ValueOf(&v).Elem() will return the reflect.Value of the thing the pointer points to, so in essence aren't you just taking the pointer and then immediately dereferencing it?

[dlwyatt]

Yep, I was seriously overthinking it in that first reply. Just a simple "v != nil" check is fine, no need for reflection at all.

[dlwyatt]

Also made those comments before #538 was merged. RunAndReturn should just work at this point.

[jfragosoperez]

Hello everyone, I've a fix for the issue mentioned here, the PR's been linked in the thread. Hope it helps