Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
1 of 5 tasks
max-melentyev opened this issue Nov 11, 2024 · 10 comments · Fixed by #843
Closed
1 of 5 tasks
Labels
approved Feature request approved for development bug

Comments

@max-melentyev
Copy link

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
  • go install
  • Other: [specify]

Steps to Reproduce

  1. git clone https://github.com/max-melentyev/mockery-issue
  2. 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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

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 LandonTClipp added bug approved Feature request approved for development labels Nov 14, 2024
@LandonTClipp
Copy link
Collaborator

@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.

@max-melentyev
Copy link
Author

I don't think I use it: here is a repo reproducing the issue, it doesn't have any custom config: https://github.com/max-melentyev/mockery-issue.

Regarding the issues, for me it looks like #331 is the initial issue that required the change to not resolve aliases. I wonder if it's possible to make mockery resolve only local aliases. For example:

package parent

type T1 = int
type T2 = int

/////////////////////////////////////
package app

import "parent"

type A1 = parent.T1

//go:generate mockery --quiet --name I
type I interface{ F(A1, parent.T2) }

///////////////////////////////////
// result:
func (_m *I) F(_a0 parent.T1, _a1 parent.T2) {

// So A1 is resolved because it's local alias (don't have package prefix).
// And it's resolved only in the scope of current package.

// parent.T2 is not resolved to `int` because it's not local (has package prefix).

@max-melentyev
Copy link
Author

Though if we have a new type like type A1 parent.T1, it should not be resolved, and mockery will use app.A1 in the mock, which will create a circular dependency 🤔

So seems like putting all mocks to the current package rather than to ./mocks is a proper approach, but it's a breaking change.

@LandonTClipp
Copy link
Collaborator

@max-melentyev PR #843 adds a new parameter that by default will revert mockery to its previous behavior, which should fix your issue.

I added a deprecation warning to mockery that this parameter needs to be set to False as type alias resolution will be permanently disabled in mockery v3.

I will double check that it fixes your reproducer at some point soon, but let me know if this looks good to you in the meantime.

LandonTClipp added a commit to LandonTClipp/mockery that referenced this issue Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Feature request approved for development bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants