fix invalid code generation when interface method parameter's name is the same as interface name

[kozmod]

Description

look at #648

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Version of Golang used when building/testing:

• 1.11
• 1.12
• 1.13
• 1.14
• 1.15
• 1.16
• 1.17
• 1.18
• 1.19
• 1.20

How Has This Been Tested?

Try to generate mocks fot interfaces with the same names of a method argument and an argument's type

//go:generate mockery --inpackage --dir . --outpkg service --testonly --filename mock_interfaceA_config_test.go --name "interfaceA"
//go:generate mockery --inpackage --dir . --outpkg service --testonly --filename mock_interfaceB_config_test.go --name "interfaceB"

type (
	interfaceA interface {
		// the name of the parameter like as interface name
		SomeMethod(interfaceB interfaceB) (interfaceA interfaceA)
	}

	interfaceB interface {
		GetData() int
	}
)

Expected result

// SomeMethod provides a mock function with given fields: interfaceB0
func (_m *mockInterfaceA) SomeMethod(interfaceB0 interfaceB) interfaceA {
	ret := _m.Called(interfaceB0)

	var r0 interfaceA
	if rf, ok := ret.Get(0).(func(interfaceB) interfaceA); ok {
		r0 = rf(interfaceB0)
	} else {
		if ret.Get(0) != nil {
			r0 = ret.Get(0).(interfaceA)
		}
	}

	return r0
}

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[LandonTClipp]

Hi, thank you very much for the PR. This solution appears to me to be incomplete. What if you have a separate interface in your package named InterfaceB0? The solution proposed here doesn't do proper name collision resolution. The parser itself probably has the information about what types are used in the package (https://github.com/vektra/mockery/blob/v2.30.1/cmd/mockery.go#L256), we might be able to leverage something like that here. Possibly by just sending the generator a list of used names, and we keep appending an integer to the argument until there are no more collisions.

[kozmod]

Hi, thank for notes.

This solution appears to me to be incomplete. What if you have a separate interface in your package named InterfaceB0?

I tried to reproduce generation with names conflicts and got following result:

//go:generate mockery --inpackage --dir . --outpkg mockery --testonly  --name "interfaceA|interfaceB|interfaceB0"

type (
	interfaceA interface {
		// the name of the parameter like as interface name
		SomeMethod(interfaceB interfaceB) (interfaceA interfaceA)
		Do(interfaceB interfaceB0) interfaceB0
		Do2(interfaceB0 interfaceB0) interfaceB0
	}

	interfaceB interface {
		GetData() int
	}

	interfaceB0 interface {
		DoB0(interfaceB0 interfaceB0) interfaceB0
	}
)

// Do provides a mock function with given fields: interfaceB
func (_m *mockInterfaceA) Do(interfaceB interfaceB0) interfaceB0 {
	ret := _m.Called(interfaceB) // 👈️ `interfaceB` is the arg name 

	var r0 interfaceB0
	if rf, ok := ret.Get(0).(func(interfaceB0) interfaceB0); ok { // 👈️ `interfaceB0 ` is the type
		r0 = rf(interfaceB)
	} else {
		if ret.Get(0) != nil {
			r0 = ret.Get(0).(interfaceB0)
		}
	}

	return r0
}

// Do2 provides a mock function with given fields: interfaceB00
func (_m *mockInterfaceA) Do2(interfaceB00 interfaceB0) interfaceB0 {
	ret := _m.Called(interfaceB00)

	var r0 interfaceB0
	if rf, ok := ret.Get(0).(func(interfaceB0) interfaceB0); ok {
		r0 = rf(interfaceB00)
	} else {
		if ret.Get(0) != nil {
			r0 = ret.Get(0).(interfaceB0)
		}
	}

	return r0
}

// SomeMethod provides a mock function with given fields: interfaceB0
func (_m *mockInterfaceA) SomeMethod(interfaceB0 interfaceB) interfaceA {
	ret := _m.Called(interfaceB0)

	var r0 interfaceA
	if rf, ok := ret.Get(0).(func(interfaceB) interfaceA); ok {
		r0 = rf(interfaceB0)
	} else {
		if ret.Get(0) != nil {
			r0 = ret.Get(0).(interfaceA)
		}
	}

	return r0
}

I have not found any conflicts.
Maybe I didn't understand the case. Please, add the example of the case with conflicts.

Test example:

import (
	"testing"
	
	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/mock"
)

type testStruct struct {
	interfaceA interfaceA
}

func (s *testStruct) Exec() interfaceB0 {
	var in interfaceB0 = nil
	return s.interfaceA.Do(in)
}

func Test(t *testing.T) {
	mockInterfaceB0 := new(mockInterfaceB0)

	mockInterfaceA := new(mockInterfaceA)
	mockInterfaceA.On("Do", mock.Anything).Return(mockInterfaceB0)

	s := testStruct{
		interfaceA: mockInterfaceA,
	}
	res := s.Exec()
	assert.Equal(t, mockInterfaceB0, res)

	mockInterfaceA.AssertExpectations(t)
}

[LandonTClipp]

I will have to toy around with this myself, I'm surprised the mock for the InterfaceB0 interface compiles.

[kozmod]

I have tried different combinations of flags and interfaces, but haven't found the problem.
I also will continue researching and try to find problem cases.
Tell me please, if you will find something.

[LandonTClipp]

@kozmod could you create a fixture in https://github.com/vektra/mockery/tree/master/pkg/fixtures using some examples (like what you posted here), add config to generate it in mockery.yaml, then add tests for that mock file? That should be sufficient proof to me that it works.

[kozmod]

@LandonTClipp Some tests was added)

[LandonTClipp]

This looks good, let's see if the tests pass.

[LandonTClipp]

These two parameters actually aren't recognized by the packages feature. Best to remove these.

[kozmod]

removed testonly: True -> ok.

But after was removed keeptree: False, mocks generate with invalid package arguments imports (method_argssame_name_arg_and_type):

// DoB0 provides a mock function with given fields: interfaceB0
func (_m *interfaceB0Mock) DoB0(interfaceB0 method_argssame_name_arg_and_type.interfaceB0) method_argssame_name_arg_and_type.interfaceB0 { // 👈️ the  interesting behavior
	ret := _m.Called(interfaceB0)

	var r0 method_argssame_name_arg_and_type.interfaceB0
	if rf, ok := ret.Get(0).(func(method_argssame_name_arg_and_type.interfaceB0) method_argssame_name_arg_and_type.interfaceB0); ok {
		r0 = rf(interfaceB0)
	} else {
		if ret.Get(0) != nil {
			r0 = ret.Get(0).(method_argssame_name_arg_and_type.interfaceB0)
		}
	}

	return r0
}

[kozmod]

Going to continue research the problem...

[kozmod]

It's happened because configuration file contains keeptree: True(.mockery.yaml:2). And in package block I override it useing keeptree: False.

[LandonTClipp]

Ah okay, that seems to be a mistake, mockery shouldn't be using keeptree.

[LandonTClipp]

I will have to review the usage of keeptree in mockery, my intention was for this to not be used anywhere in the packages config. We can just keep it if it makes the tests pass.

[codecov]

Codecov Report

Patch coverage has no change and project coverage change: -0.11562 ⚠️

Comparison is base (ec88c00) 76.65601% compared to head (15a4f69) 76.54039%.

Additional details and impacted files

@@                 Coverage Diff                 @@
##              master        #649         +/-   ##
===================================================
- Coverage   76.65601%   76.54039%   -0.11562%     
===================================================
  Files              8           8                 
  Lines           2189        2191          +2     
===================================================
- Hits            1678        1677          -1     
- Misses           376         378          +2     
- Partials         135         136          +1     

Impacted Files	Coverage Δ
pkg/generator.go	92.91339% <0.00000%> (-0.37608%)	⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[kozmod]

What about tests?
Now coverage decreased about 0.1%.
But testing Generator.genList bit difficult due to design of the method (look at ones).

I will able to write tests like (through all generating algorithm)

func (s *GeneratorSuite) TestGeneratorSameMethodArgsNameAndType() {
	iface := s.getInterfaceFromFile("method_args/same_name_arg_and_type/entity.go", "interfaceA")
	pkg := iface.QualifiedName
	gen := NewGenerator(s.ctx, GeneratorConfig{
		InPackage: true,
	}, iface, pkg)
	err := gen.Generate(s.ctx)
	s.NoError(err)
	generatedData := gen.buf.String()
	s.Contains(generatedData,
		`func (_m *mockInterfaceA) DoB(interfaceB0 interfaceB) interfaceB {ret := _m.Called(interfaceB0)`,
	)
	s.Contains(generatedData,
		`	var r0 interfaceB
	if rf, ok := ret.Get(0).(func(interfaceB) interfaceB); ok {
		r0 = rf(interfaceB0)
	} else {if ret.Get(0) != nil {
			r0 = ret.Get(0).(interfaceB)
		}}

	return r0
}`,
	)
}

[LandonTClipp]

I think we can ignore the test coverage failures. I am really against comparing the mock against a string because it tests the wrong thing, and it because really difficult to manage if you have 100 string comparison tests and you ever change what your mock looks like!

This is good, I will approve.