SetArg on slice silently fails

[kaedys]

When SetArg is used to set a slice argument, it silently fails to set the argument when the call occurs. This is because setting a slice by reflection cannot be done with a one-shot Set() call.

The diff below adds in support for setting of slices. A similar method will probably be needed for setting arrays, maps, and possible other types (maybe structs?), though my project doesn't have a need for these, so I didn't investigate their implementation.

--- github.com/golang/mock/gomock/call.go   2016-03-09 12:34:55.000000000 -0600
+++ github.com/golang/mock/gomock/call.go   2016-03-09 12:34:56.000000000 -0600
@@ -39,6 +39,13 @@
@@ -212,9 +233,38 @@
        }
        action = func() { c.doFunc.Call(doArgs) }
    }

    for n, v := range c.setArgs {
-       reflect.ValueOf(args[n]).Elem().Set(v)
-   }
+       switch reflect.TypeOf(args[n]).Kind() {
+       case reflect.Slice:
+           setSlice(args[n], v)
+       default:
+           reflect.ValueOf(args[n]).Elem().Set(v)
+       }
+   }

    rets = c.rets
    if rets == nil {
@@ -229,6 +279,15 @@
    return
 }

+func setSlice(arg interface{}, v reflect.Value) {
+   arg = reflect.MakeSlice(v.Type(), v.Len(), v.Cap())
+   av := reflect.ValueOf(arg)
+   for i := 0; i < av.Len(); i++ {
+       av.Index(i).Set(v.Index(i))
+   }
+}
+
 func (c *Call) methodType() reflect.Type {
    recv := reflect.ValueOf(c.receiver)
    for i := 0; i < recv.Type().NumMethod(); i++ {

[pasztorpisti]

SetArg is the sibling of Return and meant to be used only with pointer arguments ("out" args) that have a similar purpose as func return values.

While "pointer like objects" (slices, maps) seem to be similar to a normal pointers they can't serve as return values in such a clean way as a pointer arg or a normal func return value - you can't replace the underlying array pointed by the slice at the call site, can't replace the map object referenced by the call site, etc... - you can only modify them. For this reason if someone wants to return a slice or a map then the arg should be a slice pointer (e.g.: *[]int) or map pointer - or better - just a plain func return value.

I think the correct way to fix this bug is explicitly checking whether the given arg is a pointer (instead of assuming) and failing if it isn't.

[kaedys]

The standard UI uses slice inputs and in-place modification of the underlying backing array in a number of places, though. The io.Reader interface is an excellent example of this, in which a slice of some preallocated size is passed in, and the reader sets the read data to the underlying backing array without altering the slice header (which thus does not need to be a pointer).

Maps don't have this effect, since there's no way to alter a map while guaranteeing that the runtime won't reallocate (and thus invalidate other copies of that map header) during the operation. For slices, though, this is both easily done and very commonly done.

Beyond that, this particular update is specific to the use case where you pass in a pointer to a slice, so it's most certainly still being used with "out" args, as you put it. You just can't set those the same way as pointers to other types.

[kaedys]

For example, currently, this succeeds:

// func Foo(*int)
fmock.EXPECT().Foo(gomock.Any()).SetArg(0, int(42))
var f int
Foo(&f) // f -> 42

This, on the other hand, fails:

// func Bar(*[]string)
fmock.EXPECT().Bar(gomock.Any()).SetArg(0, []slice{"hello", "world"})
var s []string
Bar(&s)

There's nothing logically different about the two. We're passing in a pointer to the destination variable in both cases, but the latter will fail, because you can't just use reflections to assign a slice to a slice pointer in a single shot. You need to build it and apply it as in the OP instead.

[pasztorpisti]

Based on the OP I thought you were talking about mocking something like func Bar([]string).

With slice pointers I see no design problems and it seems to work for me with gomock. I've worked with the go reflect package before and I know that it doesn't behave differently with different types in case of setting/getting the referenced value this is why I was surprised about the mentioned bug.

Here is my test:

interface.go

//go:generate mockgen -source interface.go -destination interface_mock.go -package pkg
package pkg

type I interface {
	M(*[]string)
}

pkg.go

package pkg

func f(i I) []string {
	var a []string
	i.M(&a)
	return a
}

pkg_test.go

package pkg

import (
	"github.com/golang/mock/gomock"
	"testing"
)

func TestF(t *testing.T) {
	ctrl := gomock.NewController(t)
	m := NewMockI(ctrl)
	m.EXPECT().M(gomock.Any()).SetArg(0, []string{"s0", "s1"})

	a := f(m)

	if len(a) != 2 || a[0] != "s0" || a[1] != "s1" {
		t.Errorf(`a == %#v, want []string{"s0", "s1"}`, a)
	}
}

[pasztorpisti]

I forgot to add ctrl.Finish() to the end of the test but it should be no problem in our case.

Running:

go generate && go test -v

[kaedys]

Hmm, you're correct, it is in fact working in that case. Perhaps I was looking at a non-reference use case (such as an io.Reader), it's been a while since I submitted this ticket. I wonder if something changed in the Reflect package in the recent versions of Go that affected this...