search

uber-go / mock

GoMock is a mocking framework for the Go programming language.
Apache License 2.0
1.81k stars 103 forks source link

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

Open mtoader opened 4 months ago

mtoader commented 4 months ago

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
```go
// 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

Triage Notes for the Maintainers

mtoader commented 3 months ago

ping

JacobOaks commented 3 months ago

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 commented 3 months ago

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:

Then

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 commented 3 months ago

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 commented 3 months ago

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

mtoader commented 3 months ago

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

let me try

mtoader commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

mtoader commented 3 months ago

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: