Incorrect argument count validation for Do/DoReturn

[joshblum]

Actual behavior A clear and concise description of what the bug is.

Do and DoReturn can report a false positive for argument matching for variadic functions:

wrong number of arguments in Do func for (...) got 1, want 3

Expected behavior A clear and concise description of what you expected to
happen.

no error

To Reproduce Steps to reproduce the behavior

From #558 / #71

I think that the argument validation checks should change from if c.methodType.NumIn() != ft.NumIn() { to
if c.methodType.NumIn() != ft.NumIn() && !ft.IsVariadic() {

Happy to supply a PR if this is the desired fix.

Additional Information

https://github.com/keybase/client/blob/master/go/kbfs/libkbfs/subscription_manager_test.go#L153-L155 has an example of this failure case.

• gomock mode (reflect or source):
• gomock version or git ref: 1.6.0
• golang version: 1.17.3

Triage Notes for the Maintainers

[joshblum]

Ah I caught up on the discussion in these issues a bit more and see this has already been reported and closed as 'by design'. The original issue #71 was to improve an error message, I think that breaking compatibility is a worse outcome in the end. If #601 won't be accepted as-is, perhaps we can just improve the error messaging to save other folks the github dive?

[codyoss]

In this case I think we should improve error messaging. As detailed in #571, although this was a breaking change it fixed the semantics for the Do/DoReturn API to actually match. Another thing that could possibly done in addition I think is a new matcher like Any but works in place of variadic arguments. This way things could still be explicit and have more flexibility for those that want to opt-in to that. Thoughts?

[joshblum]

@codyoss I'll update #601 to improve the error messaging. I don't have a strong opinion either way about adding a new matcher!