Fix var name generation to avoid conflict (#145)

When the type and the package name is the same for an anonymous
parameter (ex: time.Time), and there are more than 1 such parameters,
the generated name for both was the same. And the generated code would
not be valid.

Fix the bug by ensuring the parameter name does not conflict with
package imports first before checking against other parameter names.

internal/registry/method_scope.go

@@ -23,63 +23,46 @@ type MethodScope struct {
 // without conflict with other variables and imported packages. It also
 // adds the relevant imports to the registry for each added variable.
 func (m *MethodScope) AddVar(vr *types.Var, suffix string) *Var {
-	name := vr.Name()
-	if name == "" || name == "_" {
-		name = generateVarName(vr.Type())
-	}
-
-	name += suffix
+	imports := make(map[string]*Package)
+	m.populateImports(vr.Type(), imports)
+	m.resolveImportVarConflicts(imports)
 
-	switch name {
-	case "mock", "callInfo", "break", "default", "func", "interface", "select", "case", "defer", "go", "map", "struct",
-		"chan", "else", "goto", "package", "switch", "const", "fallthrough", "if", "range", "type", "continue", "for",
-		"import", "return", "var":
+	name := varName(vr, suffix)
+	// Ensure that the var name does not conflict with a package import.
+	if _, ok := m.registry.searchImport(name); ok {
 		name += "MoqParam"
 	}
-
 	if _, ok := m.searchVar(name); ok || m.conflicted[name] {
-		return m.addDisambiguatedVar(vr, name)
-	}
-
-	return m.addVar(vr, name)
-}
-
-func (m *MethodScope) addDisambiguatedVar(vr *types.Var, suggested string) *Var {
-	n := 1
-	for {
-		// Keep incrementing the suffix until we find a name which is unused.
-		if _, ok := m.searchVar(suggested + strconv.Itoa(n)); !ok {
-			break
-		}
-		n++
-	}
-
-	name := suggested + strconv.Itoa(n)
-	if n == 1 {
-		conflict, _ := m.searchVar(suggested)
-		conflict.Name += "1"
-		name = suggested + "2"
-		m.conflicted[suggested] = true
+		name = m.resolveVarNameConflict(name)
 	}
 
-	return m.addVar(vr, name)
-}
-
-func (m *MethodScope) addVar(vr *types.Var, name string) *Var {
-	imports := make(map[string]*Package)
-	m.populateImports(vr.Type(), imports)
-
 	v := Var{
 		vr:         vr,
 		imports:    imports,
 		moqPkgPath: m.moqPkgPath,
 		Name:       name,
 	}
 	m.vars = append(m.vars, &v)
-	m.resolveImportVarConflicts(&v)
 	return &v
 }
 
+func (m *MethodScope) resolveVarNameConflict(suggested string) string {
+	for n := 1; ; n++ {
+		_, ok := m.searchVar(suggested + strconv.Itoa(n))
+		if ok {
+			continue
+		}
+
+		if n == 1 {
+			conflict, _ := m.searchVar(suggested)
+			conflict.Name += "1"
+			m.conflicted[suggested] = true
+			n++
+		}
+		return suggested + strconv.Itoa(n)
+	}
+}
+
 func (m MethodScope) searchVar(name string) (*Var, bool) {
 	for _, v := range m.vars {
 		if v.Name == name {
@@ -139,15 +122,12 @@ func (m MethodScope) populateImports(t types.Type, imports map[string]*Package)
 	}
 }
 
-func (m MethodScope) resolveImportVarConflicts(v *Var) {
-	// Ensure that the newly added var does not conflict with a package import
-	// which was added earlier.
-	if _, ok := m.registry.searchImport(v.Name); ok {
-		v.Name += "MoqParam"
-	}
+// resolveImportVarConflicts ensures that all the newly added imports do not
+// conflict with any of the existing vars.
+func (m MethodScope) resolveImportVarConflicts(imports map[string]*Package) {
 	// Ensure that all the newly added imports do not conflict with any of the
 	// existing vars.
-	for _, imprt := range v.imports {
+	for _, imprt := range imports {
 		if v, ok := m.searchVar(imprt.Qualifier()); ok {
 			v.Name += "MoqParam"
 		}

internal/registry/var.go

@@ -38,7 +38,25 @@ func (v Var) packageQualifier(pkg *types.Package) string {
 	return v.imports[path].Qualifier()
 }
 
-// generateVarName generates a name for the variable using the type
+func varName(vr *types.Var, suffix string) string {
+	name := vr.Name()
+	if name != "" && name != "_" {
+		return name + suffix
+	}
+
+	name = varNameForType(vr.Type()) + suffix
+
+	switch name {
+	case "mock", "callInfo", "break", "default", "func", "interface", "select", "case", "defer", "go", "map", "struct",
+		"chan", "else", "goto", "package", "switch", "const", "fallthrough", "if", "range", "type", "continue", "for",
+		"import", "return", "var":
+		name += "MoqParam"
+	}
+
+	return name
+}
+
+// varNameForType generates a name for the variable using the type
 // information.
 //
 // Examples:
@@ -49,12 +67,12 @@ func (v Var) packageQualifier(pkg *types.Package) string {
 // - map[string]int -> stringToInt
 // - error -> err
 // - a.MyType -> myType
-func generateVarName(t types.Type) string {
+func varNameForType(t types.Type) string {
 	nestedType := func(t types.Type) string {
 		if t, ok := t.(*types.Basic); ok {
 			return deCapitalise(t.String())
 		}
-		return generateVarName(t)
+		return varNameForType(t)
 	}
 
 	switch t := t.(type) {
@@ -83,7 +101,7 @@ func generateVarName(t types.Type) string {
 		return "val"
 
 	case *types.Pointer:
-		return generateVarName(t.Elem())
+		return varNameForType(t.Elem())
 
 	case *types.Signature:
 		return "fn"

pkg/moq/testpackages/paramconflict/iface.go

@@ -1,5 +1,7 @@
 package paramconflict
 
+import "time"
+
 type Interface interface {
-	Method(string, bool, string, bool, int, int32, int64, float32, float64)
+	Method(string, bool, string, bool, int, int32, int64, float32, float64, time.Time, time.Time)
 }