golang / mock

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

Incorrectly treating generic types as exported types #658

Open alexandreLamarre opened 2 years ago

alexandreLamarre commented 2 years ago

Consider a source file notifier.go we want to mock, with the following pattern :

type Clonable[T any] interface {
    Clone() T
}

type Finder[T Clonable[T]] interface {
    Find(ctx context.Context) ([]T, error)
}

type UpdateNotifier[T any] interface {

    NotifyC(ctx context.Context) <-chan []T

    Refresh(ctx context.Context)
}

Actual behavior A clear and concise description of what the bug is.

The three following functions are generated

// MockFinder is a mock of Finder interface.
type MockFinder[T notifier.Clonable[notifier.T]] struct {
    ctrl     *gomock.Controller
    recorder *MockFinderMockRecorder[T]
}

// MockFinderMockRecorder is the mock recorder for MockFinder.
type MockFinderMockRecorder[T notifier.Clonable[notifier.T]] struct {
    mock *MockFinder[T]
}

// NewMockFinder creates a new mock instance.
func NewMockFinder[T notifier.Clonable[notifier.T]](ctrl *gomock.Controller) *MockFinder[T] {
    mock := &MockFinder[T]{ctrl: ctrl}
    mock.recorder = &MockFinderMockRecorder[T]{mock}
    return mock
}

Note that in the above the generic type T is treated as an exported type from notifier


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

notifier.Clonable[notifier.T] should be notifier.Clonable[T] in the function signatures

// MockFinder is a mock of Finder interface.
type MockFinder[T notifier.Clonable[T]] struct {
    /* ... */
}

// MockFinderMockRecorder is the mock recorder for MockFinder.
type MockFinderMockRecorder[T notifier.Clonable[T]] struct {
    /* ... */
}

// NewMockFinder creates a new mock instance.
func NewMockFinder[T notifier.Clonable[T]](ctrl *gomock.Controller) *MockFinder[T] {
    /* ... */
}

To Reproduce Steps to reproduce the behavior

  1. mockgen -source notifier.go -output notifier_mock.go

Additional Information

Triage Notes for the Maintainers

Seems like a fun one

alexandreLamarre commented 2 years ago

In mockgen.go GenerateMockInterface:

longTp, shortTp := g.formattedTypeParams(intf, outputPackagePath)

longTp is initialized as [T notifier.Clonable[notifier.T]] because :

for i, v := range it.TypeParams {
        if i != 0 {
            long.WriteString(", ")
            short.WriteString(", ")
        }
        long.WriteString(v.Name)
        short.WriteString(v.Name)
        long.WriteString(fmt.Sprintf(" %s", v.Type.String(g.packageMap, pkgOverride)))
    }

the nested T in [T notifier.Clonable[T]] is parsed into ast.NamedType in mockgen/generic_go118.go and Name() on NamedType always prefixes the NamedType with the package the NamedType is declared in.

Not very familiar with the codebase, but I would assume that to support nested generic interfaces, we would need to introduce another TypeParam implementation, e.g, something along the lines of PlaceholderType or GenericType. Let me know what you think, I'd be happy to work on this

tra4less commented 1 year ago

set tps before parse it.TypeParams[i].Type src

tp, err := p.parseFieldList(pkg, it.typeParams, tps)
if err != nil {
    return nil, fmt.Errorf("unable to parse interface type parameters: %v", name)
}
iface.TypeParams = tp
for _, v := range tp {
    tps[v.Name] = true
}