deduplicate methods to allow overlapping methods on embedded interfaces

[marten-seemann]

Fixes #497.

Description

This PR deduplicates methods to allow overlapping methods on embedded interfaces (which are valid Go since version 1.14). We're using the fact that if the name of the method matches, the parameters need to match as well (e.g. you can't combine two interfaces that both define a method sayHello(), where one signature is function sayHello() and the other one is function sayHello() error).

Regarding the implementation: I did consider using a map (map[method name]*method) for deduplication, however, this would lead to a non-deterministic order when iterating over this map. I'm therefore deduplicating every time a new method is added to the interface. Clearly, the scaling behavior of this is not ideal for interfaces with a lot of methods.
I could use an additional set to keep track of function names, but I figured that most interfaces will have a lot less than 20 or so methods, so the performance difference should be negligible. lmk if you disagree with my choice here.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

• Includes tests

Reviewer Notes

• The code flow looks good.
• Tests added.

Release Notes

deduplicate methods to allow overlapping methods on embedded interfaces

- Deduplicate methods in order to allow overlapping methods when using embedded interfaces

[marten-seemann]

The tests are failing since the overlap package I added for testing purpose contains overlapping methods. Apparently Go doesn't accept this (even when using Go 1.14+), if go.mod defines a lower Go version (go.mod uses 1.11 here), even if the offending files are protected by build flags specifying a minimum Go version.

Any suggestions how to proceed here?

[marten-seemann]

@codyoss Any thoughts on this PR?

[marten-seemann]

@codyoss Any chance you could have a look at this PR any time soon?

[codyoss]

@marten-seemann Sorry for the slow response here. I am just catching up after being on a sabbatical. I will get to reviewing in the next day or so. Thanks for your understanding.

[codyoss]

The tests are failing since the overlap package I added for testing purpose contains overlapping methods. Apparently Go
doesn't accept this (even when using Go 1.14+), if go.mod defines a lower Go version (go.mod uses 1.11 here), even if the
offending files are protected by build flags specifying a minimum Go version.
Any suggestions how to proceed here?

Will need to make an implementation for go1.14+ and <go1.14. You call a method named the same thing that exists in two different files. Once file has a build tag of // +build go1.14 and the other should be // +build !go1.14

[marten-seemann]

Will need to make an implementation for go1.14+ and <go1.14. You call a method named the same thing that exists in two different files. Once file has a build tag of // +build go1.14 and the other should be // +build !go1.14

@codyoss The problem is not the compiler version in use. The problem is that the Go version set in go.mod determines what the compiler will complain about. Therefore, // +build go1.14 won't have any effect.
I created the following gist to demonstrate this: https://gist.github.com/marten-seemann/ea3b81a175844ce00e2e4f155845b57c. If you try to go build this (using Go 1.15.x), it will still fail with ./main.go:15:2: duplicate method Close. Only when you change the Go version in go.mod to Go 1.14 or newer the compilation will succeed.

An easy solution would be to increase the Go version number in the go.mod in this repository. As I'm not very familiar with the code base, I'm not sure if this would have any unwanted side effects somewhere else, or what your policy regarding support for old Go versions is. If it's similar to Go's "last but one major release", changing it to 1.14 should be fine.

[codyoss]

Sorry for the slow response here. If I am understanding you correctly it is the code that consumes mockgen must be 1.14. If this is the case we could define a go.mod file in the given test directory. Would that solve the issue? I would prefer not to drop support for older languages versions if not needed.

[marten-seemann]

If I am understanding you correctly it is the code that consumes mockgen must be 1.14.

Sorry for not making my point clear. The problem is not the version of the Go compiler here. Please have a look at my example in https://gist.github.com/marten-seemann/ea3b81a175844ce00e2e4f155845b57c. This is perfectly valid code from Go 1.14 on, but it will still fail because the version in go.mod is 1.13. The compiler will behave differently depending on what version it sees in go.mod.

If this is the case we could define a go.mod file in the given test directory.

As far as I'm aware, you can't have multiple go.mods in one repository. I tried adding one in the overlapping_methods directory, and I'm getting the same error.

I would prefer not to drop support for older languages versions if not needed.

May I ask why? Is there any use case for supporting older Go version than the Go project itself (which supports only the last two major versions)? To the best of my knowledge, most Go projects have adopted a similar policy for their own code.

[marten-seemann]

@codyoss Is there anything I can do to help make progress on this PR?

[marten-seemann]

@codyoss I added the go.mod file in the overlapping_methods directory. The Travis build is now succeeding. The GitHub Action build is failing, but this doesn't seem related to this PR (it's failing on master as well).

[marten-seemann]

@codyoss I rebased the PR (thank you for getting rid of Travis, GH Actions is so much smoother!), and now all tests are passing.

[codyoss]

Thanks for the updates. Would you mind adding the push/pop logic to both check_go_mod.sh and check_go_generate.sh? Those go commands there will also only be executed in the current module context. Other than that it looks good to me!

[marten-seemann]

@codyoss Done.

Can we get a new release once this PR is merged? Would love to use this feature in quic-go soon.

[codyoss]

LGTM, thank you for your contribution 🎆 . Sorry for the delays!

[codyoss]

I will try to get a release out in the next couple of days. In the meantime you can always pin to head temporarily: go get github.com/golang/mock/mockgen@HEAD

[joaodrp]

I will try to get a release out in the next couple of days. In the meantime you can always pin to head temporarily: go get github.com/golang/mock/mockgen@HEAD

@codyoss, do you know when you'll be able to make a new release?

[codyoss]

https://github.com/golang/mock/releases/tag/v1.5.0