Aliased imports don't propagate well in the generated mocks (at least in source mode)

[mtoader]

Generating a mock, in source mode, for an interface that references types from an imported package that is aliased will not generate the same alias in the generated output. This creates problems when you want to generate within the same package since other tools, mockgen included, can't deal with packages that import other package with different aliases.

Expected behavior A clear and concise description of what you expected to
happen.

The generated mock should use the same aliases as the source package.

To Reproduce Steps to reproduce the behavior

1. Try to generate a mock for a file like this:

package import_aliased

import (
	definition_alias "github.com/package/definition"
)

//go:generate mockgen -package import_aliased -destination source_mock.go -source=source.go -imports definition_alias=github.com/package/definition

type S interface {
	M(definition_alias.X)
}

2. The generated file preamble will look like

// Code generated by MockGen. DO NOT EDIT.
// Source: source.go
//
// Generated by this command:
//
//	mockgen -package import_aliased -destination source_mock.go -source=source.go -imports definition_alias=github.com/package/definition
//

// Package import_aliased is a generated GoMock package.
package import_aliased

import (
	reflect "reflect"

	definition "github.com/package/definition"
	gomock "go.uber.org/mock/gomock"
)

// MockS is a mock of S interface.
type MockS struct {
	ctrl     *gomock.Controller
	recorder *MockSMockRecorder
}
...

notice that now the import_aliased package imports the github.com/package/definition with two aliases: definition and definition_alias. This is not a real problem for the go compiler but mockgen itself can't handle it. The right thing here is for generated file to reference the import with the same alias.

Additional Information

• gomock mode (reflect or source): source
• gomock version or git ref: 986b50e
• golang version: 1.20 - 1.22

Triage Notes for the Maintainers

[mtoader]

ping

[JacobOaks]

Hey @mtoader thanks for the PR.

I understand it does create friction to have two different import aliases within the same package for the same import, but can you elaborate on what you mean by "mockgen itself can't handle it"?

[mtoader]

Hey @mtoader thanks for the PR.

I understand it does create friction to have two different import aliases within the same package for the same import, but can you elaborate on what you mean by "mockgen itself can't handle it"?

I can definitely try,

Assume this:

• a package p with two files a.go and b.go
• an aliased import in a.go like import imp_alias_a "github.com/package/definition"
• another aliased import in b.go with a different name. like import imp_alias_b "github.com/package/definition"
• an interface/type Definition defined in a.go (this Definition might need to refer imp_alias_a)
• a client package client_p that has an interface definition like type Int interface { F(p.Definition) }

Then

• if you try to generate a mock in client_p for Int, mockgen will fail.

This triggered for us because we do in-package mock generations for interfaces that reference proto definitions and we decided to have the proto generated packages aliased. One level of generation works but when you import a package that already has a generated mock with a different alias the generation fails.

Let me know if this is clear. If not i can try to make a repro case.

[mtoader]

Also .. please advice on how to write those tests better.

Right now they fail with this:

go: downloading golang.org/x/tools v0.2.0
go: downloading golang.org/x/mod v0.11.0
go: downloading github.com/yuin/goldmark v1.4.13
go: downloading golang.org/x/sys v0.1.0
Error: mockgen/internal/tests/import_aliased/source.go:4:2: no required module provides package github.com/package/definition; to add it:
	go get github.com/package/definition
Error: Process completed with exit code 1.

i assumed that the internal tree there is only used as a mock source to showcase thing, but i seem to have been wrong.

[r-hang]

@mtoader, would you be able to provide a repro example and an error message for the situation?

[mtoader]

@mtoader, would you be able to provide a repro example and an error message for the situation?

let me try

[mtoader]

@mtoader, would you be able to provide a repro example and an error message for the situation?

let me try

@r-hang see https://github.com/mtoader/sample-gomock. Checkout and do run.sh (assuming a working go setup) should show an error message similar to:

2024/03/19 13:30:35 Loading input failed: /Users/mihai/sample-gomock/pack/r.go:10:13: failed parsing arguments: /Users/mihai/sample-gomock/pack/r.go:10:17: unknown package "proto_pb"
client.go:8: running "mockgen": exit status 1

there are some comments in the run.sh explaining the relatively narrow edge case here :)

[mtoader]

@mtoader, would you be able to provide a repro example and an error message for the situation?

let me try

@r-hang see https://github.com/mtoader/sample-gomock. Checkout and do run.sh (assuming a working go setup) should show an error message similar to:

2024/03/19 13:30:35 Loading input failed: /Users/mihai/sample-gomock/pack/r.go:10:13: failed parsing arguments: /Users/mihai/sample-gomock/pack/r.go:10:17: unknown package "proto_pb"
client.go:8: running "mockgen": exit status 1

there are some comments in the run.sh explaining the relatively narrow edge case here :)

ping ?

[r-hang]

Sorry for the delay and thank you for the repro. I'm starting to take a look now, i'll have an update by Monday.

[mtoader]

Sorry for the delay and thank you for the repro. I'm starting to take a look now, i'll have an update by Monday.

Perfect, hopefully you can get the right fix in place.

[r-hang]

Hey @mtoader,

I think the cause of this issue has to do with mockgen's attempt to merge all the files of a package with ast.MergepackageFiles before parsing it for imports. Only one package name is recognized by gomock at the conclusion of this step. While parsing, if an imported package alias is not in this map produced by this process, mock generation will fail. As your repro case points out, the order of the merge matters.

This also seemed to happen when dealing when mocking embedded interfaces in a different package because mockgen initializes a new parser for that package that does not carry over state from the root file we're looking to mock.

From my understanding, when parsing a package embedded somewhere in a package we want to mock, any difference in package aliases for the a particular package could cause this issue. In your example, a generated mock does trigger this case but mock generated code is not required.

If I patch your proposed fix here https://github.com/uber-go/mock/pull/165/files, but modify your github repro case with the patch below -- i'll still get a similar error on run.

new file mode 100644
index 0000000..8b2c385
--- /dev/null
+++ b/pack/a.go
@@ -0,0 +1,7 @@
+package pack
+
+import (
+	proto_name "github.com/sample/sample-gomock/proto"
+)
+
+func x(p *proto_name.X) { return }
diff --git a/pack/r.go b/pack/r.go
index 5d2c2fd..cca477d 100644
--- a/pack/r.go
+++ b/pack/r.go
@@ -1,11 +1,11 @@
 package pack
 
 import (
-	proto_pb "github.com/sample/sample-gomock/proto"
+	"github.com/sample/sample-gomock/proto"
 )
 
 //go:generate mockgen -source r.go -destination mock_pack.gen.go -package pack
 
 type I interface {
-	DoSomething(x *proto_pb.X) (int, error)
+	DoSomething(x *proto.X) (int, error)
 }

Where the error is src/github.com/mtoader/sample-gomock/pack/r.go:10:13: failed parsing arguments: /Users/rhang/gocode/src/github.com/mtoader/sample-gomock/pack/r.go:10:17: unknown package "proto"

As you alluded to in this issue, I think mockgen should be able to handle different aliases to the same package since the Go compiler is able to do so, i'm trying to see what changes that would require.

[mtoader]

Yea .. my patch just attempts to manage the badness, the real solution is more complicated.

[mtoader]

Yea .. my patch just attempts to manage the badness, the real solution is more complicated.

Thinking about what would be a good default behavior here, i think the following rules should hold:

• when in source mode, any imports in the generated files should use the same aliases as the imports used in the corresponding input files. E.g. if source.go containing type X interface imports a package with some alias and that import is needed for the target mock_source.go the import within mock_source.go should use the same as the original file.
• When parsing a package the import aliases should be tracked per input files
• have the ability to force a particular alias for some import by hand (currently the -imports flag seems kind of broken for some reason but it should always work regardless of how the import package path looks like).

[mtoader]

ping ?

[A13xB0]

We are also facing this issue... Any update on the fix as this has not been updates since April?

[r-hang]

Hey @mtoader sorry for the delay.

Let's merge https://github.com/uber-go/mock/pull/165/files because it fixes what looks like a mistake where definedImports is never indexed by the importpath it is actually populated with.

As for the long term fix with the rules you specified above, I agree with your points!

[mtoader]

I need to rebase / merge things. Gimme some little to do this.