mockgen should not follow type aliases

[dsnet]

Suppose mockgen was run on a source file that specified proto.Message. Today, mockgen will generate a file that imports "github.com/golang/protobuf/proto", which is the current place that proto.Message is declared. However, suppose we were to move proto.Message to a separate package (e.g., "github.com/golang/protobuf/protoapi") using type aliases, then mockgen now uses the import for the "real" location that the type is declared. I'd argue that mockgen should not do that. It should produce a file that has a set of dependencies that matches (or is a subset of) the original input file.

[gertcuykens]

I am split on this I assume some people do and some people don't?

[dsnet]

Under what circumstance would people want it to follow the alias?

The whole point of type aliases is to be able to move a type in a way that is agnostic to the user of the old type. For that reason, tools like gomock should preserve the identifier as used in the original code that they are mocking.

I just moved another type via a type alias and a bunch of targets are broken again because their dependency graph changed since the generated mock file now has a different set of dependencies than before.

[dsnet]

Here's a real example of breakage:

• A type Foo is lives at path/to/my/package
• At some point, type Foo is moved to path/to/my/internal/package, and an alias is placed in path/to/my/package forwarding that identifier to the new location.
  • For all intents and purposes, the old location is the way users are supposed to interact with that type (the type is being moved to an internal package to work around a dependency cycle)
• The mock tool uses the aliased location and the generated code tries to import path/to/my/internal/package, which fails because they can't (and shouldn't) import an internal package.

[gertcuykens]

Ok you convinced me but is it actually possible to do so? Looking at the code I have no idea how to prevent this from happening as far as I understand only basic reflect stuff is being used?

[dsnet]

The approach taken in reflect.go seems fundamentally problematic since the type information needed has already been lost when the stub program is run. Probably the proper way to do this is through static analysis on the source code similar to parse.go.

[gertcuykens]

Ok let see what the developers have to say about this, isn't going to be trivial I think but I agree it's the way forward.

[balshetzer]

I am convinced that the right thing to do would be to not follow aliases. I'm not sure what exactly to do about it though.

We do have parse mode. I'm not sure if it correctly handles aliases. It precedes the existence of aliases.

Your system, dsnet, exclusively uses reflect mode. We could try changing it to use parse mode.

We can document that reflect mode follows aliases and direct users to parse mode instead.

I'm not sure if this warrants deprecating reflect mode. The two modes continue to exist because people keep running into situations where one works for them while the other does not.

[dsnet]

It seems to me that "reflect" is just one possible approach to implementing mockgen. Being an implementation detail, that seems to suggest that this detail should (as much as reasonable possible) not be surface-able to the user.

If I can understand better, what are the cases where the "reflect" approach is superior to the "parse" approach, and can those advantages be implemented in the "parse" approach such that "parse" mode has feature parity?

[linzhp]

I also don't want mockgen to follow alias. We are generating Bazel gomock rules, and have a hard time in figuring out the dependencies of generated mocks because it may import something that the mocked package doesn't import directly.

[linzhp]

Also, go.uber.org/cadence/encoded.Value is an alias of type in its internal package. When mockgen follows aliases, the generated mock of any interfaces that uses encoded.Value will depends on go.uber.org/cadence/internal, which is not allowed in Go

[linzhp]

@codyoss thoughts on this?

[codyoss]

I agree with the intent of this issue, and this is how is actually does work in source mode. I have not looked into the complexity of the issue, but at least there is a workaround for now. I will turn this into a feature request.

[linzhp]

Please make this a configurable flag, so we can choose whether to follow type aliases based on the scenarios. Because of https://github.com/jmhodges/bazel_gomock/issues/10, we rely on type aliases to mock system packages in Bazel

[codyoss]

@linzhp Having this configurable seems seems like a workaround for an issue in the Bazel ecosystem. Do you agree? I don't have great experience with Bazel though, so maybe I am missing something.

[linzhp]

I agree. It's a workaround. However, since this following alias behavior has been there for a while, other code may depends on it, it's probably better to make it configurable to maintain backward compatibility

[hsyed]

The GRPC library has refactored to type aliases delegating to internal packages as well recently. We can't generate mocks anymore.

[codyoss]

This topic requires more investigation as it does not appear to be as straight-forward to me as I once thought.

[cvgw]

It was mentioned earlier in the thread that parse mode might be a solution here, but I don't believe it was ever clarified whether it actually works.

Given the following example

somepkg/
  alias/
    alias.go
    internal/
      baz/
        baz.go

// alias.go
package alias

imports (
  "somepkg/alias/internal/baz"
)

type Foobar baz.Foobar

// baz.go
package baz

type Foobar interface {
  String() string
}

somepkg/alias$ mockgen -source=alias.go
=>
// Code generated by MockGen. DO NOT EDIT.
// Source: alias.go

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

import (
        gomock "github.com/golang/mock/gomock"
)

Which seems to be a bug

[kevindflynn]

This is also an issue for my team. We have to modify the generated code so that it doesn't import any internal packages whenever we re-generate.

[glukasiknuro]

Note that protobuf ApiV2 is using aliases from old api to new api for well known types (e.g. empty.pb.go), what changes the code generated by mockgen after upgrading protobuf package but still using the old API. In case of my team it caused some non-obvious strict dependency checks to fire - a mock has different deps than an interface.

[bobbyrullo]

This just bit me today. I'm in a BAZEL ecosystem as well. Are they any workarounds?

[dilyevsky]

As far as I can tell this happens bc reflect.Type.PkgPath follows the type alias and seems to me alias target information is totally lost during compilation. Here's a sample of prog intermediate:

func main() {
        flag.Parse()

        its := []struct {
                sym string
                typ reflect.Type
        }{

                {"SubscriberMessageHandler", reflect.TypeOf((*pkg_.SubscriberMessageHandler)(nil)).Elem()},
        }
        pkg := &model.Package{
                // NOTE: This behaves contrary to documented behaviour if the
                // package name is not the final component of the import path.
                // The reflect package doesn't expose the package name, though.
                Name: path.Base("github.com/test/pubsub"),
        }
        for _, it := range its {
                fmt.Println(it.typ.Method(0).Type.In(0).Elem().PkgPath())
...

Where SubscriberMessageHandler is an interface with a single method which has a single cloud.google.com/go/pubsub.Message argument:

package cloud

import (
        "cloud.google.com/go/pubsub"
)

type SubscriberMessageHandler interface {
        HandlePubSubMessage(*pubsub.Message) error
}

Println statement inside in the loop prints cloud.google.com/go/internal/pubsub instead of cloud.google.com/go/pubsub bc this is an alias type https://pkg.go.dev/cloud.google.com/go/pubsub#Message.

Using source mode (specifying source attribute in Bazel's gomock rule) totally works though. My guess would be this can't be fixed in this library without changing Go's reflection implementation wrt to alias types.