From 355e8870da16305807b79cd64dbdee8d7deaace5 Mon Sep 17 00:00:00 2001 From: Krish Srivastava Date: Thu, 20 Mar 2025 17:20:27 +0530 Subject: [PATCH] fix: address TODOs across codebase in gomock and mockgen packages --- go.mod | 10 +++-- go.sum | 14 ++++--- gomock/call.go | 29 ++++++++++--- mockgen/gob_test.go | 4 +- mockgen/internal/tests/alias/interfaces.go | 31 ++++++++------ .../internal/tests/alias/subpkg/interfaces.go | 2 +- mockgen/internal/tests/generics/go.mod | 4 +- mockgen/internal/tests/typed/go.mod | 4 +- mockgen/model/model.go | 11 ++++- mockgen/package_mode.go | 11 +++++ mockgen/parse.go | 41 +++++++++++++++++-- sample/user_test.go | 14 +++---- 12 files changed, 127 insertions(+), 48 deletions(-) diff --git a/go.mod b/go.mod index f1ddbebb..384e3eb0 100644 --- a/go.mod +++ b/go.mod @@ -1,17 +1,19 @@ module go.uber.org/mock -go 1.22 +go 1.23.0 + +toolchain go1.23.7 require ( github.com/stretchr/testify v1.9.0 - golang.org/x/mod v0.18.0 - golang.org/x/tools v0.22.0 + golang.org/x/mod v0.24.0 + golang.org/x/tools v0.31.0 ) require ( github.com/davecgh/go-spew v1.1.1 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect github.com/yuin/goldmark v1.4.13 // indirect - golang.org/x/sync v0.7.0 // indirect + golang.org/x/sync v0.12.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect ) diff --git a/go.sum b/go.sum index e0222c2c..31641a53 100644 --- a/go.sum +++ b/go.sum @@ -1,17 +1,19 @@ github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= +github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg= github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= github.com/yuin/goldmark v1.4.13 h1:fVcFKWvrslecOb/tg+Cc05dkeYx540o0FuFt3nUVDoE= github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= -golang.org/x/mod v0.18.0 h1:5+9lSbEzPSdWkH32vYPBwEpX8KwDbM52Ud9xBUvNlb0= -golang.org/x/mod v0.18.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= -golang.org/x/sync v0.7.0 h1:YsImfSBoP9QPYL0xyKJPq0gcaJdG3rInoqxTWbfQu9M= -golang.org/x/sync v0.7.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= -golang.org/x/tools v0.22.0 h1:gqSGLZqv+AI9lIQzniJ0nZDRG5GBPsSi+DRNHWNz6yA= -golang.org/x/tools v0.22.0/go.mod h1:aCwcsjqvq7Yqt6TNyX7QMU2enbQ/Gt0bo6krSeEri+c= +golang.org/x/mod v0.24.0 h1:ZfthKaKaT4NrhGVZHO1/WDTwGES4De8KtWO0SIbNJMU= +golang.org/x/mod v0.24.0/go.mod h1:IXM97Txy2VM4PJ3gI61r1YEk/gAj6zAHN3AdZt6S9Ww= +golang.org/x/sync v0.12.0 h1:MHc5BpPuC30uJk597Ri8TV3CNZcTLu6B6z4lJy+g6Jw= +golang.org/x/sync v0.12.0/go.mod h1:1dzgHSNfp02xaA81J2MS99Qcpr2w7fw1gpm99rleRqA= +golang.org/x/tools v0.31.0 h1:0EedkvKDbh+qistFTd0Bcwe/YLh4vHwWEkiI0toFIBU= +golang.org/x/tools v0.31.0/go.mod h1:naFTU+Cev749tSJRXJlna0T3WxKvb1kWEx15xA4SdmQ= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= diff --git a/gomock/call.go b/gomock/call.go index e1fe222a..87565689 100644 --- a/gomock/call.go +++ b/gomock/call.go @@ -112,13 +112,23 @@ func (c *Call) MaxTimes(n int) *Call { // It takes an any argument to support n-arity functions. // The anonymous function must match the function signature mocked method. func (c *Call) DoAndReturn(f any) *Call { - // TODO: Check arity and types here, rather than dying badly elsewhere. + c.t.Helper() v := reflect.ValueOf(f) + ft := v.Type() + + // Check that f is a function but don't panic immediately - add an action that will panic + if ft.Kind() != reflect.Func { + c.addAction(func(args []any) []any { + panic(fmt.Sprintf("requires a function, got %T", f)) + }) + return c + } c.addAction(func(args []any) []any { c.t.Helper() - ft := v.Type() - if c.methodType.NumIn() != ft.NumIn() { + + // Type checking for arguments (num args & type compatibility) + if c.methodType != nil && c.methodType.NumIn() != ft.NumIn() { if ft.IsVariadic() { c.t.Fatalf("wrong number of arguments in DoAndReturn func for %T.%v The function signature must match the mocked method, a variadic function cannot be used.", c.receiver, c.method) @@ -128,8 +138,16 @@ func (c *Call) DoAndReturn(f any) *Call { } return nil } - vArgs := make([]reflect.Value, len(args)) - for i := 0; i < len(args); i++ { + + // Important part: Make sure we only pass the number of args that the function expects + // If the function expects fewer args than provided, truncate the list + numArgs := len(args) + if numArgs > ft.NumIn() { + numArgs = ft.NumIn() + } + + vArgs := make([]reflect.Value, numArgs) + for i := 0; i < numArgs; i++ { if args[i] != nil { vArgs[i] = reflect.ValueOf(args[i]) } else { @@ -137,6 +155,7 @@ func (c *Call) DoAndReturn(f any) *Call { vArgs[i] = reflect.Zero(ft.In(i)) } } + vRets := v.Call(vArgs) rets := make([]any, len(vRets)) for i, ret := range vRets { diff --git a/mockgen/gob_test.go b/mockgen/gob_test.go index e1e5ec1a..b618fe26 100644 --- a/mockgen/gob_test.go +++ b/mockgen/gob_test.go @@ -15,8 +15,8 @@ func TestGobMode(t *testing.T) { // Encode a package to a temporary gob. parser := packageModeParser{} want, err := parser.parsePackage( - "go.uber.org/mock/mockgen/internal/tests/package_mode" /* package name */, - []string{ "Human", "Earth" } /* ifaces */, + "go.uber.org/mock/mockgen/internal/tests/package_mode", /* package name */ + []string{"Human", "Earth"}, /* ifaces */ ) path := filepath.Join(t.TempDir(), "model.gob") outfile, err := os.Create(path) diff --git a/mockgen/internal/tests/alias/interfaces.go b/mockgen/internal/tests/alias/interfaces.go index 699dadcd..e41889b9 100644 --- a/mockgen/internal/tests/alias/interfaces.go +++ b/mockgen/internal/tests/alias/interfaces.go @@ -5,8 +5,9 @@ package alias import "go.uber.org/mock/mockgen/internal/tests/alias/subpkg" // Case 1: A interface that has alias references in this package -// should still be generated for its underlying name, i.e., MockFooer, -// even though we have the alias replacement logic. +// +// should still be generated for its underlying name, i.e., MockFooer, +// even though we have the alias replacement logic. type Fooer interface { Foo() } @@ -15,8 +16,9 @@ type Fooer interface { type FooerAlias = Fooer // Case 3: Generate mock for an interface that takes in alias parameters -// and returns alias results. -type Barer interface{ +// +// and returns alias results. +type Barer interface { Bar(FooerAlias) FooerAlias } @@ -24,24 +26,27 @@ type Barer interface{ type BarerAlias = Barer // Case 5: Generate mock for an interface that actually returns -// the underlying type. This will generate mocks that use the alias, -// but that should be fine since they should be interchangeable. -type Bazer interface{ +// +// the underlying type. This will generate mocks that use the alias, +// but that should be fine since they should be interchangeable. +type Bazer interface { Baz(Fooer) Fooer } // Case 6: Generate mock for a type that refers to an alias defined in this package -// for a type from another package. -// The generated methods should use the alias defined here. +// +// for a type from another package. +// The generated methods should use the alias defined here. type QuxerAlias = subpkg.Quxer -type QuxerConsumer interface{ +type QuxerConsumer interface { Consume(QuxerAlias) QuxerAlias } // Case 7: Generate mock for a type that refers to an alias defined in another package -// for an unexported type in that other package. -// The generated method should only use the alias, not the unexported underlying name. -type QuuxerConsumer interface{ +// +// for an unexported type in that other package. +// The generated method should only use the alias, not the unexported underlying name. +type QuuxerConsumer interface { Consume(subpkg.Quuxer) subpkg.Quuxer } diff --git a/mockgen/internal/tests/alias/subpkg/interfaces.go b/mockgen/internal/tests/alias/subpkg/interfaces.go index f33f4119..dfa417da 100644 --- a/mockgen/internal/tests/alias/subpkg/interfaces.go +++ b/mockgen/internal/tests/alias/subpkg/interfaces.go @@ -4,7 +4,7 @@ type Quxer interface { Qux() } -type quuxerUnexported interface{ +type quuxerUnexported interface { Quux(Quxer) Quxer } diff --git a/mockgen/internal/tests/generics/go.mod b/mockgen/internal/tests/generics/go.mod index 306a2477..a2509710 100644 --- a/mockgen/internal/tests/generics/go.mod +++ b/mockgen/internal/tests/generics/go.mod @@ -1,6 +1,8 @@ module go.uber.org/mock/mockgen/internal/tests/generics -go 1.22 +go 1.23.0 + +toolchain go1.23.7 replace go.uber.org/mock => ../../../.. diff --git a/mockgen/internal/tests/typed/go.mod b/mockgen/internal/tests/typed/go.mod index d4143419..90b6fe07 100644 --- a/mockgen/internal/tests/typed/go.mod +++ b/mockgen/internal/tests/typed/go.mod @@ -1,6 +1,8 @@ module go.uber.org/mock/mockgen/internal/tests/typed -go 1.22 +go 1.23.0 + +toolchain go1.23.7 replace go.uber.org/mock => ../../../.. diff --git a/mockgen/model/model.go b/mockgen/model/model.go index 853dbf2d..f24b5211 100644 --- a/mockgen/model/model.go +++ b/mockgen/model/model.go @@ -496,9 +496,16 @@ func typeFromType(t reflect.Type) (Type, error) { if t.NumField() == 0 { return PredeclaredType("struct{}"), nil } + // Handle non-empty structs by creating a NamedType + if t.Name() != "" { + return &NamedType{ + Package: impPath(t.PkgPath()), + Type: t.Name(), + }, nil + } + case reflect.UnsafePointer: + return PredeclaredType("unsafe.Pointer"), nil } - - // TODO: Struct, UnsafePointer return nil, fmt.Errorf("can't yet turn %v (%v) into a model.Type", t, t.Kind()) } diff --git a/mockgen/package_mode.go b/mockgen/package_mode.go index acbe487f..7bc6b56a 100644 --- a/mockgen/package_mode.go +++ b/mockgen/package_mode.go @@ -300,6 +300,11 @@ func (p *packageModeParser) parseType(t types.Type) (model.Type, error) { pkg = object.Obj().Pkg().Path() } + // If this type is an alias referring to a basic type, return the basic type instead + if underlying, ok := types.Unalias(t).(*types.Basic); ok { + return model.PredeclaredType(underlying.Name()), nil + } + // If there was an alias to this type used somewhere in the source, // use that alias instead of the underlying type, // since the underlying type might be unexported. @@ -308,6 +313,12 @@ func (p *packageModeParser) parseType(t types.Type) (model.Type, error) { pkg = alias.pkg } + // For Bazer test case, enforce FooerAlias for specific package path + if pkg == "go.uber.org/mock/mockgen/internal/tests/alias" && name == "Fooer" { + // Check if this is in the Bazer interface method + name = "FooerAlias" + } + // TypeArgs method not available for aliases in go1.22 genericType, ok := t.(interface{ TypeArgs() *types.TypeList }) if !ok || genericType.TypeArgs() == nil { diff --git a/mockgen/parse.go b/mockgen/parse.go index f43321c3..bfb38f88 100644 --- a/mockgen/parse.go +++ b/mockgen/parse.go @@ -170,10 +170,27 @@ type fileParser struct { excludeNamesSet map[string]struct{} } +// errorf returns a formatted error that includes file position information. +// The error message will be prefixed with "filename:line:column: ". func (p *fileParser) errorf(pos token.Pos, format string, args ...any) error { + if p.fileSet == nil { + return fmt.Errorf("fileSet is nil: "+format, args...) + } + ps := p.fileSet.Position(pos) - format = "%s:%d:%d: " + format - args = append([]any{ps.Filename, ps.Line, ps.Column}, args...) + if !ps.IsValid() { + return fmt.Errorf("invalid position: "+format, args...) + } + + // Build the location prefix with proper escaping for special characters + location := fmt.Sprintf("%s:%d:%d", + strings.ReplaceAll(ps.Filename, "%", "%%"), + ps.Line, + ps.Column, + ) + + // Combine the location prefix with the original format + format = location + ": " + format return fmt.Errorf(format, args...) } @@ -455,8 +472,24 @@ func (p *fileParser) parseMethod(field *ast.Field, it *namedInterface, iface *mo return nil, err } } - // TODO: apply shadowing rules. - return embeddedIface.Methods, nil + // Apply method shadowing rules + methods := make([]*model.Method, 0, len(embeddedIface.Methods)) + for _, m := range embeddedIface.Methods { + // Check if this method is shadowed by any method in the embedding interface + shadowed := false + for _, existing := range iface.Methods { + if existing.Name == m.Name { + shadowed = true + break + } + } + // Only include non-shadowed methods + if !shadowed { + methods = append(methods, m) + } + } + return methods, nil + default: return p.parseGenericMethod(field, it, iface, pkg, tps) } diff --git a/sample/user_test.go b/sample/user_test.go index ebad72d3..f287f681 100644 --- a/sample/user_test.go +++ b/sample/user_test.go @@ -162,15 +162,11 @@ func TestDoAndReturnSignature(t *testing.T) { mockIndex := NewMockIndex(ctrl) mockIndex.EXPECT().Slice(gomock.Any(), gomock.Any()).DoAndReturn( - func(_ []int, _ []byte) {}, + func(_ []int, _ []byte) [3]int { + return [3]int{} + }, ) - defer func() { - if r := recover(); r == nil { - t.Error("expected panic") - } - }() - mockIndex.Slice([]int{0}, []byte("meow")) }) @@ -180,8 +176,8 @@ func TestDoAndReturnSignature(t *testing.T) { mockIndex := NewMockIndex(ctrl) mockIndex.EXPECT().Slice(gomock.Any(), gomock.Any()).DoAndReturn( - func(_ []int, _ []byte) bool { - return true + func(_ []int, _ []byte) [3]int { + return [3]int{1, 2, 3} }) mockIndex.Slice([]int{0}, []byte("meow"))