Fix #71 Do + DoAndReturn signature change error msg (#395)

Update the error handling for Call.Do and Call.DoAndReturn in the case where
the argument passed does not match expectations.

* panic if the argument is not a function
* panic if the number of input arguments do not match those expected by Call
* panic if the types of the input arguments do not match those expected
by Call

Call.DoAndReturn has additional validations on the return signature

* panic if the number of return arguments do not match those expected by
Call
* panic if the types of return arguments do not match those expected by
Call

Author: cvgw

gomock/call.go

@@ -19,6 +19,8 @@ import (
 	"reflect"
 	"strconv"
 	"strings"
+
+	"github.com/golang/mock/gomock/internal/validate"
 )
 
 // Call represents an expected call to a mock.
@@ -105,10 +107,24 @@ func (c *Call) MaxTimes(n int) *Call {
 // DoAndReturn declares the action to run when the call is matched.
 // The return values from this function are returned by the mocked function.
 // It takes an interface{} argument to support n-arity functions.
+// If the method signature of f is not compatible with the mocked function a
+// panic will be triggered. Both the arguments and return values of f are
+// validated.
 func (c *Call) DoAndReturn(f interface{}) *Call {
-	// TODO: Check arity and types here, rather than dying badly elsewhere.
 	v := reflect.ValueOf(f)
 
+	switch v.Kind() {
+	case reflect.Func:
+		mt := c.methodType
+
+		ft := v.Type()
+		if err := validate.InputAndOutputSig(ft, mt); err != nil {
+			panic(fmt.Sprintf("DoAndReturn: %s", err))
+		}
+	default:
+		panic("DoAndReturn: argument must be a function")
+	}
+
 	c.addAction(func(args []interface{}) []interface{} {
 		vargs := make([]reflect.Value, len(args))
 		ft := v.Type()
@@ -134,10 +150,24 @@ func (c *Call) DoAndReturn(f interface{}) *Call {
 // return values are ignored to retain backward compatibility. To use the
 // return values call DoAndReturn.
 // It takes an interface{} argument to support n-arity functions.
+// If the method signature of f is not compatible with the mocked function a
+// panic will be triggered. Only the arguments of f are validated; not the return
+// values.
 func (c *Call) Do(f interface{}) *Call {
-	// TODO: Check arity and types here, rather than dying badly elsewhere.
 	v := reflect.ValueOf(f)
 
+	switch v.Kind() {
+	case reflect.Func:
+		mt := c.methodType
+
+		ft := v.Type()
+		if err := validate.InputSig(ft, mt); err != nil {
+			panic(fmt.Sprintf("Do: %s", err))
+		}
+	default:
+		panic("Do: argument must be a function")
+	}
+
 	c.addAction(func(args []interface{}) []interface{} {
 		vargs := make([]reflect.Value, len(args))
 		ft := v.Type()

gomock/call_test.go

@@ -492,9 +492,11 @@ func TestDo(t *testing.T) {
 	doCalled := false
 	var argument string
 	ctrl.RecordCall(subject, "FooMethod", "argument").Do(
-		func(arg string) {
+		func(arg string) int {
 			doCalled = true
 			argument = arg
+
+			return 0
 		})
 	if doCalled {
 		t.Error("Do() callback called too early.")

gomock/controller_test.go

@@ -0,0 +1,199 @@
+// Copyright 2020 Google Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package validate
+
+import (
+	"fmt"
+	"reflect"
+)
+
+// InputAndOutputSig compares the argument and return signatures of actualFunc
+// against expectedFunc. It returns an error unless everything matches.
+func InputAndOutputSig(actualFunc, expectedFunc reflect.Type) error {
+	if err := InputSig(actualFunc, expectedFunc); err != nil {
+		return err
+	}
+
+	if err := outputSig(actualFunc, expectedFunc); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+// InputSig compares the argument signatures of actualFunc
+// against expectedFunc. It returns an error unless everything matches.
+func InputSig(actualFunc, expectedFunc reflect.Type) error {
+	// check number of arguments and type of each argument
+	if actualFunc.NumIn() != expectedFunc.NumIn() {
+		return fmt.Errorf(
+			"expected function to have %d arguments not %d",
+			expectedFunc.NumIn(), actualFunc.NumIn())
+	}
+
+	lastIdx := expectedFunc.NumIn()
+
+	// If the function has a variadic argument validate that one first so that
+	// we aren't checking for it while we iterate over the other args
+	if expectedFunc.IsVariadic() {
+		if ok := variadicArg(lastIdx, actualFunc, expectedFunc); !ok {
+			i := lastIdx - 1
+			return fmt.Errorf(
+				"expected function to have"+
+					" arg of type %v at position %d"+
+					" not type %v",
+				expectedFunc.In(i), i, actualFunc.In(i),
+			)
+		}
+
+		lastIdx--
+	}
+
+	for i := 0; i < lastIdx; i++ {
+		expectedArg := expectedFunc.In(i)
+		actualArg := actualFunc.In(i)
+
+		if err := arg(actualArg, expectedArg); err != nil {
+			return fmt.Errorf("input argument at %d: %s", i, err)
+		}
+	}
+
+	return nil
+}
+
+func outputSig(actualFunc, expectedFunc reflect.Type) error {
+	// check number of return vals and type of each val
+	if actualFunc.NumOut() != expectedFunc.NumOut() {
+		return fmt.Errorf(
+			"expected function to have %d return vals not %d",
+			expectedFunc.NumOut(), actualFunc.NumOut())
+	}
+
+	for i := 0; i < expectedFunc.NumOut(); i++ {
+		expectedArg := expectedFunc.Out(i)
+		actualArg := actualFunc.Out(i)
+
+		if err := arg(actualArg, expectedArg); err != nil {
+			return fmt.Errorf("return argument at %d: %s", i, err)
+		}
+	}
+
+	return nil
+}
+
+func variadicArg(lastIdx int, actualFunc, expectedFunc reflect.Type) bool {
+	if actualFunc.In(lastIdx-1) != expectedFunc.In(lastIdx-1) {
+		if actualFunc.In(lastIdx-1).Kind() != reflect.Slice {
+			return false
+		}
+
+		expectedArgT := expectedFunc.In(lastIdx - 1)
+		expectedElem := expectedArgT.Elem()
+		if expectedElem.Kind() != reflect.Interface {
+			return false
+		}
+
+		actualArgT := actualFunc.In(lastIdx - 1)
+		actualElem := actualArgT.Elem()
+
+		if ok := actualElem.ConvertibleTo(expectedElem); !ok {
+			return false
+		}
+
+	}
+
+	return true
+}
+
+func interfaceArg(actualArg, expectedArg reflect.Type) error {
+	if !actualArg.ConvertibleTo(expectedArg) {
+		return fmt.Errorf(
+			"expected arg convertible to type %v not type %v",
+			expectedArg, actualArg,
+		)
+	}
+
+	return nil
+}
+
+func mapArg(actualArg, expectedArg reflect.Type) error {
+	expectedKey := expectedArg.Key()
+	actualKey := actualArg.Key()
+
+	switch expectedKey.Kind() {
+	case reflect.Interface:
+		if err := interfaceArg(actualKey, expectedKey); err != nil {
+			return fmt.Errorf("map key: %s", err)
+		}
+	default:
+		if actualKey != expectedKey {
+			return fmt.Errorf("expected map key of type %v not type %v",
+				expectedKey, actualKey)
+		}
+	}
+
+	expectedElem := expectedArg.Elem()
+	actualElem := actualArg.Elem()
+
+	switch expectedElem.Kind() {
+	case reflect.Interface:
+		if err := interfaceArg(actualElem, expectedElem); err != nil {
+			return fmt.Errorf("map element: %s", err)
+		}
+	default:
+		if actualElem != expectedElem {
+			return fmt.Errorf("expected map element of type %v not type %v",
+				expectedElem, actualElem)
+		}
+	}
+
+	return nil
+}
+
+func arg(actualArg, expectedArg reflect.Type) error {
+	switch expectedArg.Kind() {
+	// If the expected arg is an interface we only care if the actual arg is convertible
+	// to that interface
+	case reflect.Interface:
+		if err := interfaceArg(actualArg, expectedArg); err != nil {
+			return err
+		}
+	default:
+		// If the expected arg is not an interface then first check to see if
+		// the actual arg is even the same reflect.Kind
+		if expectedArg.Kind() != actualArg.Kind() {
+			return fmt.Errorf("expected arg of kind %v not %v",
+				expectedArg.Kind(), actualArg.Kind())
+		}
+
+		switch expectedArg.Kind() {
+		// If the expected arg is a map then we need to handle the case where
+		// the map key or element type is an interface
+		case reflect.Map:
+			if err := mapArg(actualArg, expectedArg); err != nil {
+				return err
+			}
+		default:
+			if actualArg != expectedArg {
+				return fmt.Errorf(
+					"Expected arg of type %v not type %v",
+					expectedArg, actualArg,
+				)
+			}
+		}
+	}
+
+	return nil
+}