Circular dependency in go 1.23 when mock is used in the package where it's defined

max-melentyev commented 5 days ago

Description

Hey, looks like #808 results in a circular dependency if mock is used in the package where it's defined:

// ./p/iface.go
package p

type T = int

//go:generate mockery --quiet --name I
type I interface{ F(T) }

// this generates ./p/mocks/I.go
package mocks
import "p"

func (_m *I) F(_a0 p.T) {
...

// ./p/i_test.go
import "p/mocks" // fails because of circular dependency

Here is a sample repo that reproduces the issue: https://github.com/max-melentyev/mockery-issue

Mockery Version

2.46.3

Go Version

1.23

Installation Method

[ ] Binary Distribution
[ ] Docker
[ ] brew
[x] go install
[ ] Other: [specify]

Steps to Reproduce

git clone https://github.com/max-melentyev/mockery-issue
make test

Expected Behavior

Mock uses original type (int) rather than its local alias (p.T) and this allows using mock in the same package (p).

Actual Behavior

p/mocks uses type p.T and this doesn't allow to use this mock from the package p:

go test -v ./p
# mockery-issue/p
package mockery-issue/p
        imports mockery-issue/p/mocks
        imports mockery-issue/p: import cycle not allowed in test
FAIL    mockery-issue/p [setup failed]

LandonTClipp commented 4 days ago

Oh gosh, this is a tricky one. Sadly, the behavior where mockery would resolve down to the underlying aliased type was incorrect for a long time. Mockery having been changed to refer to the alias name instead of the underlying type fixed a lot of deal-breaking issues. So this fix highlighted a latent problem with the way you generate mocks.

This is generally why I recommend generating mocks in the same package as the tests because of this exact problem. But, I recognize I can't just ask you to change your codebase and ignore the issue altogether.

I can offer a solution of adding a parameter that when enabled will tell mockery to resolve back to the underlying type. Otherwise, I don't know what else to do, we won't revert the change.

LandonTClipp commented 4 days ago

Actually, another fix that might help you is to set GODEBUG=gotypesalias=0 mockery [...]. This will disable the Go runtime from exposing the type alias in the AST.

max-melentyev commented 3 days ago

Looks like mocks are generated in a separate package by default.

If I understand correctly GODEBUG=gotypesalias=0 is a temporary workaround and it can be removed in future versions of go. Are there any pitfalls if mockery provides --resolve-type-alias flag?

LandonTClipp commented 3 days ago

gotypesalias is enabled by default starting from Go 1.23 and it will be completely removed/permanently disabled sometime after Go 1.27. We can still tell mockery to resolve type aliases to their underlying type with a small feature addition.

Are there any pitfalls if mockery provides --resolve-type-alias flag?

Not really no. It would just revert mockery to the old behavior. The only reason for us to add it is to get around the fact that your mock generation is now broken on the latest version of mockery.

max-melentyev commented 3 days ago

Could you please share links to issues that were related to old alias resolution behaviour? For me it looks like if mockery is no longer resolving aliases by default, then it should place mocks to the same package by default, because otherwise these 2 defaults just don't work together. So it'd be a breaking change, and probably old behaviour should be restored within the current major version. WDYT?

LandonTClipp commented 3 days ago

There's a whole bunch of them, I'll post some related pulls/issues:

For me it looks like if mockery is no longer resolving aliases by default, then it should place mocks to the same package by default, because otherwise these 2 defaults just don't work together.

This is fair. However, there are many cases where a mock does not have to import anything from the original package, in which case placing it in a separate package wouldn't result in circular dependencies.

As far as considering this a breaking change, I think I agree. I'll look into adding this parameter.

LandonTClipp commented 22 hours ago

@max-melentyev can you confirm you are not using the packages feature of mockery? If not, I am considering setting resolve-type-alias to default to true only for the legacy config semantics.

vektra / mockery