.Return() mutates its caller's slice

[Groxx]

https://github.com/golang/mock/blob/112dfb85f71efc679eef2a9763162fa83fbee449/gomock/call.go#L184

If someone uses Return with an interface slice, like .Return(rets...), this line mutates the caller's slice. How it mutates it is semi-harmless, usually, but rather importantly this means tests that share a return slice for convenience can encounter order-dependent behavior and race conditions:

WARNING: DATA RACE
Read at 0x00c0002da8f0 by goroutine 88:
  github.com/golang/mock/gomock.(*Call).Return()
      /Users/stevenl/gocode/pkg/mod/github.com/golang/[email protected]/gomock/call.go:171 +0x122
  go.uber.org/cadence/internal/common/metrics.runTest.func1()
      /Users/stevenl/gocode/src/go.uber.org/cadence/internal/common/metrics/service_wrapper_test.go:171 +0x316f
  testing.tRunner()
      /usr/local/Cellar/go/1.13.4/libexec/src/testing/testing.go:909 +0x199

Previous write at 0x00c0002da8f0 by goroutine 89:
  github.com/golang/mock/gomock.(*Call).Return()
      /Users/stevenl/gocode/pkg/mod/github.com/golang/[email protected]/gomock/call.go:188 +0x2f3
  go.uber.org/cadence/internal/common/metrics.runTest.func1()
      /Users/stevenl/gocode/src/go.uber.org/cadence/internal/common/metrics/service_wrapper_test.go:171 +0x316f
  testing.tRunner()
      /usr/local/Cellar/go/1.13.4/libexec/src/testing/testing.go:909 +0x199

Evidence that it mutates a spread-ed slice is pretty simple: https://play.golang.org/p/mz_FRDnPJHB

Anyone free to fix? There's really no reason to (ab)use the original slice like this. I noticed other rets-use while skimming, but I'm not sure if they have the same issue. It'd be good to make sure all instances of arg-slice mutating are cleaned up.

[codyoss]

I am not sure there is a good solution for this. If you read the comment above that block of code this is done for a reason. Thoughts?

[Groxx]

That seems to just be intending "convert concretes to the intended interface", not "mutate the caller's var to match the mock expectations".

Though I suspect it'll break someone's tests, if they're doing value-equality: is there a reason this can't just be done to a new slice? e.g. at the beginning of the func, rets := append(make([]interface{}, 0, len(rets)), rets...) would make this go away since you'd be mutating the rets-to-return rather than the rets-that-may-mutate-the-caller.

[cvgw]

That seems to just be intending "convert concretes to the intended interface", not "mutate the caller's var to match the mock expectations".

Though I suspect it'll break someone's tests, if they're doing value-equality: is there a reason this can't just be done to a new slice? e.g. at the beginning of the func, rets := append(make([]interface{}, 0, len(rets)), rets...) would make this go away since you'd be mutating the rets-to-return rather than the rets-that-may-mutate-the-caller.

That seems pretty reasonable to me.

I think you're pointing out that it would be a breaking change for the following use case?

mockFoo.EXPECT().Bar().Return(baz)

// someFunc returns the value of mockFoo.Bar()
if someFunc() != baz {
  t.FailNow()
}