deprecated reflect mode has been replaced with import mode by tulzke · Pull Request #198 · uber-go/mock

tulzke · 2024-08-09T11:43:44Z

resolves #175
resolves #197
resolves #128

It is impossible to create an mock for a generic interface via reflect mode, because it is impossible to compile a generic type without instantiation.
This PR replaces the reflect mod for parsing using go/types.

All exists mocks have been regenerated and the tests have been passed. But since this radically changes the behavior of reflect mode, I would be grateful if there are those who want to add additional test cases that I did not provide.

We can also come up with another name instead of import mode.

benefits:

generation mocks for generic interfaces
generation mocks for aliases to interfaces
correct names for method arguments

JacobOaks

Hey! Thank you for this PR! We're excited about this change. We took a first pass and have some initial comments. We're going to do some additional testing on this and come back soon for more review.

One high level comment I wanted to raise was - with Go 1.23, go/types introduced Alias. Can we add test cases that try to load aliases to interfaces and verify this will work?

mockgen/import_mode.go

mockgen/internal/tests/import_mode/guns/guns.go

tulzke · 2024-08-28T17:56:33Z

One high level comment I wanted to raise was - with Go 1.23, go/types introduced Alias. Can we add test cases that try to load aliases to interfaces and verify this will work?

This already works without explicitly specifying an alias. This works thanks to the Underlying method.

Example of generated mock for alias:

mock/mockgen/internal/tests/import_mode/mock/interfaces.go

Lines 297 to 434 in 703a35f

    
           type MockHuman struct { 
        
           	ctrl     *gomock.Controller 
        
           	recorder *MockHumanMockRecorder 
        
           } 
        
           // MockHumanMockRecorder is the mock recorder for MockHuman. 
        
           type MockHumanMockRecorder struct { 
        
           	mock *MockHuman 
        
           } 
        
           // NewMockHuman creates a new mock instance. 
        
           func NewMockHuman(ctrl *gomock.Controller) *MockHuman { 
        
           	mock := &MockHuman{ctrl: ctrl} 
        
           	mock.recorder = &MockHumanMockRecorder{mock} 
        
           	return mock 
        
           } 
        
           // EXPECT returns an object that allows the caller to indicate expected use. 
        
           func (m *MockHuman) EXPECT() *MockHumanMockRecorder { 
        
           	return m.recorder 
        
           } 
        
           // ISGOMOCK indicates that this struct is a gomock mock. 
        
           func (m *MockHuman) ISGOMOCK() struct{} { 
        
           	return struct{}{} 
        
           } 
        
           // Breathe mocks base method. 
        
           func (m *MockHuman) Breathe() { 
        
           	m.ctrl.T.Helper() 
        
           	m.ctrl.Call(m, "Breathe") 
        
           } 
        
           // Breathe indicates an expected call of Breathe. 
        
           func (mr *MockHumanMockRecorder) Breathe() *MockHumanBreatheCall { 
        
           	mr.mock.ctrl.T.Helper() 
        
           	call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Breathe", reflect.TypeOf((*MockHuman)(nil).Breathe)) 
        
           	return &MockHumanBreatheCall{Call: call} 
        
           } 
        
           // MockHumanBreatheCall wrap *gomock.Call 
        
           type MockHumanBreatheCall struct { 
        
           	*gomock.Call 
        
           } 
        
           // Return rewrite *gomock.Call.Return 
        
           func (c *MockHumanBreatheCall) Return() *MockHumanBreatheCall { 
        
           	c.Call = c.Call.Return() 
        
           	return c 
        
           } 
        
           // Do rewrite *gomock.Call.Do 
        
           func (c *MockHumanBreatheCall) Do(f func()) *MockHumanBreatheCall { 
        
           	c.Call = c.Call.Do(f) 
        
           	return c 
        
           } 
        
           // DoAndReturn rewrite *gomock.Call.DoAndReturn 
        
           func (c *MockHumanBreatheCall) DoAndReturn(f func()) *MockHumanBreatheCall { 
        
           	c.Call = c.Call.DoAndReturn(f) 
        
           	return c 
        
           } 
        
           // Eat mocks base method. 
        
           func (m *MockHuman) Eat(foods ...import_mode.Food) { 
        
           	m.ctrl.T.Helper() 
        
           	varargs := []any{} 
        
           	for _, a := range foods { 
        
           		varargs = append(varargs, a) 
        
           	} 
        
           	m.ctrl.Call(m, "Eat", varargs...) 
        
           } 
        
           // Eat indicates an expected call of Eat. 
        
           func (mr *MockHumanMockRecorder) Eat(foods ...any) *MockHumanEatCall { 
        
           	mr.mock.ctrl.T.Helper() 
        
           	call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Eat", reflect.TypeOf((*MockHuman)(nil).Eat), foods...) 
        
           	return &MockHumanEatCall{Call: call} 
        
           } 
        
           // MockHumanEatCall wrap *gomock.Call 
        
           type MockHumanEatCall struct { 
        
           	*gomock.Call 
        
           } 
        
           // Return rewrite *gomock.Call.Return 
        
           func (c *MockHumanEatCall) Return() *MockHumanEatCall { 
        
           	c.Call = c.Call.Return() 
        
           	return c 
        
           } 
        
           // Do rewrite *gomock.Call.Do 
        
           func (c *MockHumanEatCall) Do(f func(...import_mode.Food)) *MockHumanEatCall { 
        
           	c.Call = c.Call.Do(f) 
        
           	return c 
        
           } 
        
           // DoAndReturn rewrite *gomock.Call.DoAndReturn 
        
           func (c *MockHumanEatCall) DoAndReturn(f func(...import_mode.Food)) *MockHumanEatCall { 
        
           	c.Call = c.Call.DoAndReturn(f) 
        
           	return c 
        
           } 
        
           // Sleep mocks base method. 
        
           func (m *MockHuman) Sleep(duration time.Duration) { 
        
           	m.ctrl.T.Helper() 
        
           	m.ctrl.Call(m, "Sleep", duration) 
        
           } 
        
           // Sleep indicates an expected call of Sleep. 
        
           func (mr *MockHumanMockRecorder) Sleep(duration any) *MockHumanSleepCall { 
        
           	mr.mock.ctrl.T.Helper() 
        
           	call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Sleep", reflect.TypeOf((*MockHuman)(nil).Sleep), duration) 
        
           	return &MockHumanSleepCall{Call: call} 
        
           } 
        
           // MockHumanSleepCall wrap *gomock.Call 
        
           type MockHumanSleepCall struct { 
        
           	*gomock.Call 
        
           } 
        
           // Return rewrite *gomock.Call.Return 
        
           func (c *MockHumanSleepCall) Return() *MockHumanSleepCall { 
        
           	c.Call = c.Call.Return() 
        
           	return c 
        
           } 
        
           // Do rewrite *gomock.Call.Do 
        
           func (c *MockHumanSleepCall) Do(f func(time.Duration)) *MockHumanSleepCall { 
        
           	c.Call = c.Call.Do(f) 
        
           	return c 
        
           } 
        
           // DoAndReturn rewrite *gomock.Call.DoAndReturn 
        
           func (c *MockHumanSleepCall) DoAndReturn(f func(time.Duration)) *MockHumanSleepCall { 
        
           	c.Call = c.Call.DoAndReturn(f) 
        
           	return c 
        
           }

And, yes, i tested it in compile-time:

mock/mockgen/internal/tests/import_mode/mock/interfaces_test.go

Line 21 in 703a35f

var human import_mode.Human = &MockHuman{}

Generated mock successfully implements alias.

JacobOaks · 2024-09-09T16:55:58Z

One high level comment I wanted to raise was - with Go 1.23, go/types introduced Alias. Can we add test cases that try to load aliases to interfaces and verify this will work?

This already works without explicitly specifying an alias. This works thanks to the Underlying method.

@tulzke, when I run mockgen's tests on go1.23.1, or with GODEBUG=gotypesalias=1, I get the following failure:

$ GODEBUG=gotypesalias=1 go test ./...
...
                Error:          Received unexpected error:
                                extract interfaces from package: parse interface: error parsing type go.uber.org/mock/mockgen/internal/tests/import_mode.Human = go.uber.org/mock/mockgen/internal/tests/import_mode.Primate: Human is not an interface. it is a interface{Breathe(); Sleep(duration time.Duration); go.uber.org/mock/mockgen/internal/tests/import_mode.Eater}
...

This is because obj in parseInterface is a *types.Alias, but being asserted to a *types.Named. types.Unalias can be applied here to get to the underlying *types.Named

JacobOaks

Hey @tulzke - thanks for the updates and sorry for the delay. Took another pass.

mockgen/import_mode.go

JacobOaks · 2024-09-09T17:25:01Z

Forgot to add - since types.Unalias was added in 1.22, we will probably want to hide usages of it behind a build tag.

tulzke · 2024-09-12T10:01:18Z

Forgot to add - since types.Unalias was added in 1.22, we will probably want to hide usages of it behind a build tag.

Why we can't just update go up to 1.23 in go.mod? This is the latest stable version. If we make different implementations for different versions of Go, it will be noticeably more difficult to maintain.

JacobOaks · 2024-09-12T15:07:25Z

Why we can't just update go up to 1.23 in go.mod? This is the latest stable version. If we make different implementations for different versions of Go, it will be noticeably more difficult to maintain.

I'm gonna ask other maintainers their thoughts here because I'm actually not super sure myself. cc: @sywhang @r-hang @tchung1118.

I believe using 1.23 in go.mod would force anybody who wants to use the next release to upgrade to go1.23. We can technically bump the version of go.mod to 1.22, since that aligns with our release policy of supporting the two latest versions, and then we can reference types.Alias freely here. But since gomock is used widely and this is a simple enough difference to hide behind build tags, it would be nice to use them instead of alienating pre-1.22 users from our next release. I don't think we would need to duplicate a lot of code, I think both references to types.Alias/types.Unalias can be one-line functions with different definitions between the versions, but I agree it is technically a maintenance burden, especially for the switch case.

ebilling · 2024-09-16T17:53:47Z

As moving to go1.23 would be considered a breaking change, it might make sense to release as v2 which would force users to move forward manually. That would allow the old system to work with older versions but force users to move forward with a "modern" version. I've been watching/waiting/using this change for a few weeks now. For users that want to stay up-to-date with Go versions and latest protoc changes, not merging this change is causing real headaches.

This change updates all `go.mod` and ci workflows to only support go1.22 and go1.23. Although this will force users to upgrade to go1.22 to use the next release, it is technically in line with our release policy (see: https://github.com/uber-go/mock?tab=readme-ov-file#supported-go-versions) and will allow us to merge uber-go#198 without using build tags for references to the new `*types.Alias` and `types.Unalias()`.

JacobOaks · 2024-09-18T15:49:27Z

Chatted about build tags vs. go.mod update internally - we think it's fine to update go.mod to 1.22 which will allow this PR to merge without build tags.

This is technically in line with our release policy and will avoid some ugly code in the big switch statement as @tulzke mentioned.

As moving to go1.23 would be considered a breaking change, it might make sense to release as v2 which would force users to move forward manually. That would allow the old system to work with older versions but force users to move forward with a "modern" version.

As a policy, we don't typically do 2.0 releases. If something is truly a breaking change we don't usually even consider it. In this case however - I'm not sure dropping support for Go versions that aren't even supported themselves upstream qualifies as a breaking change, so I think it's fine to update go.mod.

This change updates all `go.mod` and ci workflows to only support go1.22 and go1.23. Although this will force users to upgrade to go1.22 to use the next release, it is technically in line with our release policy (see: https://github.com/uber-go/mock?tab=readme-ov-file#supported-go-versions) and will allow us to merge #198 without using build tags for references to the new `*types.Alias` and `types.Unalias()`.

JacobOaks

Hey @tulzke - this is looking great! Thanks for your updates. I did a final fine-toothed review over this, but other than these last couple of comments/questions this LGTM.

mockgen/import_mode.go

JacobOaks · 2024-09-25T17:51:32Z

ci/test.sh

 #!/bin/bash
 # This script is used to ensure that the go.mod file is up to date.

+# Compatibility for Go 1.22


Can you expand on why this is needed? Ideally, we'd want to support Go 1.22 w/o the GODEBUG variable set.

I have already generated mocks for aliases. Without gotypesalias=1, mocks are generated without aliases.
Example from failed test:

< func (c *MockEarthHumanPopulationCall) Return(arg0 int) *MockEarthHumanPopulationCall { --- > func (c *MockEarthHumanPopulationCall) Return(arg0 import_mode.HumansCount) *MockEarthHumanPopulationCall {

HumansCount here - alias for int. On 1.23 and 1.22 with GODEBUG=gotypealias=1 mockgen wiil generate mocks with correct types. On native 1.22 it wil generate int instead of import_mode.HumansCount.

Tests on 1.22 and 1.23 have different behavior now due to aliases.

Understood - let's keep this here then. Can you just add this brief explanation into the test script?

I have tested it now. And with go 1.22 in go mod it does not work with aliases as i expected.

$ go version go version go1.23.1 darwin/arm64 $ cat go.mod | grep "go 1." go 1.22 $ cd mockgen $ go build . && cp mockgen /Users/USERNAME/go/bin/mockgen # copy binary to PATH $ cat internal/tests/import_mode/go.mod | grep "go 1." #checking go version in nested go module go 1.23 $ cd internal/tests/import_mode $ cat interfaces.go| grep "AddHumans" AddHumans(HumansCount) []Human $ go generate ./... $ cat mock/interfaces.go | grep "AddHumans(arg0" #must have HumansCount and Human instead of int and Primate func (m *MockEarth) AddHumans(arg0 int) []import_mode.Primate { func (mr *MockEarthMockRecorder) AddHumans(arg0 any) *MockEarthAddHumansCall {

In my case it works only after i changed go version in go.mod and rebuild binary.

Do you have any ideas how to support go 1.22 and alias types together?

mockgen/import_mode.go

mockgen/internal/tests/import_mode/interfaces.go

JacobOaks · 2024-09-27T15:04:05Z

Hey @tulzke - I'm sorry, I mistakenly overrode your remote branch's main with the origin's main while trying to fix the go1.22 alias issue.

I think I'm able to re-instate it/re-open the PR how it was by pushing the correct main in my local state to your branch if you grant me permission to do so - or you can force-push your local main to tulzke/main if you have your local state still correct and then I think I can re-open/reinstate the PR. Again, really sorry about the mistake.

tulzke · 2024-09-27T15:12:52Z

Hey @tulzke - I'm sorry, I mistakenly overrode your remote branch's main with the origin's main while trying to fix the go1.22 alias issue.

I think I'm able to re-instate it/re-open the PR how it was by pushing the correct main in my local state to your branch if you grant me permission to do so - or you can force-push your local main to tulzke/main if you have your local state still correct and then I think I can re-open/reinstate the PR. Again, really sorry about the mistake.

I can't re-open this PR, so i created the new one:
#207

JacobOaks reviewed Aug 27, 2024

View reviewed changes

tulzke requested a review from JacobOaks September 2, 2024 06:30

wawan93 approved these changes Sep 3, 2024

View reviewed changes

JacobOaks reviewed Sep 9, 2024

View reviewed changes

mockgen/import_mode.go Outdated Show resolved Hide resolved

mockgen/import_mode.go Outdated Show resolved Hide resolved

mockgen/import_mode.go Outdated Show resolved Hide resolved

mockgen/import_mode.go Outdated Show resolved Hide resolved

JacobOaks mentioned this pull request Sep 18, 2024

Support Go 1.22 and Go 1.23 #203

Merged

ary1992 mentioned this pull request Sep 20, 2024

Upgrade k8s.io/* to v0.31, sigs.k8s.io/controller-runtime to v0.19 gardener/gardener#10459

Merged

3 tasks

JacobOaks reviewed Sep 25, 2024

View reviewed changes

JacobOaks closed this Sep 27, 2024

JacobOaks force-pushed the main branch from 5b309e7 to 6d5eb71 Compare September 27, 2024 14:54

tulzke mentioned this pull request Sep 27, 2024

deprecated reflect mode has been replaced with package mode #207

Merged

Conversation

tulzke commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

benefits:

Uh oh!

JacobOaks left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tulzke commented Aug 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JacobOaks commented Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JacobOaks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JacobOaks commented Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tulzke commented Sep 12, 2024

Uh oh!

JacobOaks commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ebilling commented Sep 16, 2024

Uh oh!

JacobOaks commented Sep 18, 2024

Uh oh!

JacobOaks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JacobOaks Sep 25, 2024

Choose a reason for hiding this comment

Uh oh!

tulzke Sep 26, 2024

Choose a reason for hiding this comment

Uh oh!

JacobOaks Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tulzke Sep 26, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JacobOaks commented Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tulzke commented Sep 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tulzke commented Aug 9, 2024 •

edited

Loading

JacobOaks left a comment •

edited

Loading

tulzke commented Aug 28, 2024 •

edited

Loading

JacobOaks commented Sep 9, 2024 •

edited

Loading

JacobOaks commented Sep 9, 2024 •

edited

Loading

JacobOaks commented Sep 12, 2024 •

edited

Loading

JacobOaks Sep 26, 2024 •

edited

Loading

JacobOaks commented Sep 27, 2024 •

edited

Loading