Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reflect: TypeOf interface with methods using generic types includes package path in type #55147

Closed
daolis opened this issue Sep 19, 2022 · 6 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@daolis
Copy link

daolis commented Sep 19, 2022

GO Version: 1.19.1

reflect.TypeOf from an interface containing methods using generic types includes the complete package path within the type.

This functionality is use in gomock (reflect mode) and generates not compile-able code.

Example see https://go.dev/play/p/0P8R4Au1Wer

The string representation of the interfaces method type from the playground example looks like the following.
func() test.TestGenericType[play.ground/test.TestType]

Effect in gomock: golang/mock#677

@randall77
Copy link
Contributor

randall77 commented Sep 19, 2022

The paths in the instantiating types is there to disambiguate the package of the instantiating type.

Consider this program: https://go.dev/play/p/cGmCAwJzn9I

foo/a/x.I and foo/b/x.I are different types, but if we didn't include the package path they would look identical.

For the top-level type, that's ok, as PkgPath can be used to disambiguate the types (as shown in my example). But for instantiating types there is no such mechanism.

One thing we could do is remove the path from the name and just have ambiguous names. There still needs to be underlying uniqueness of the names for the linker+runtime, which means we'd need 2 names for everything.

Another thing we could do is provide a reflect API for accessing the instantiating types, so you could call PkgPath on them. Then the names themselves being unique is less important.

Things to ponder. I'm not convinced that the current state is all that bad, partly because I don't understand what gomock does and why this extra path trips it up.

I'm going to mark for 1.20 so that we make a decision one way or another before the release.

@randall77 randall77 added this to the Go1.20 milestone Sep 19, 2022
@cherrymui cherrymui changed the title reflection: typeof interface with methods using generic types includes package path in type reflect: typeOf interface with methods using generic types includes package path in type Sep 19, 2022
@cherrymui cherrymui changed the title reflect: typeOf interface with methods using generic types includes package path in type reflect: TypeOf interface with methods using generic types includes package path in type Sep 19, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Sep 19, 2022
@cherrymui cherrymui added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 19, 2022
@cherrymui
Copy link
Member

cherrymui commented Sep 19, 2022

Yeah, I think that the current state is not bad. The question is why this is a problem for gomock. It looks like it is used for generating code? It probably could be more careful about the instantiation names before writing it to source file.

@daolis
Copy link
Author

daolis commented Sep 20, 2022

gomock use the type directly from the reflect type (test.TestGenericType[play.ground/test.TestType]) and generate the mocks with this path.

see https://github.com/golang/mock/blob/73266f9366fcf2ccef0b880618e5a9266e4136f4/mockgen/model/model.go#L396

So, if the behavior from reflect is 'ok' then gomock should check the generic type by adding the correct import and modify the type itself?

like

import (
    test_ "play.ground/test"
)

func() test.TestGenericType[test_.TestType] {
    ...
}

Repo to reproduce:
https://github.com/daolis/gomocktest

@randall77
Copy link
Contributor

randall77 commented Sep 20, 2022

Yes, you'd have to do something like that. Certainly you will need to generate that import statement somehow, as you can't describe Foo[pkg.Bar] without having the correct import statement for pkg. This is a problem that only comes up with generics because normally you know the package of Foo, it's the one you're running gomock for, and you don't need an import statement because the code gomock generates is within Foos package. But the package of Bar could be some arbitrary other package.

@apparentlymart
Copy link

apparentlymart commented Sep 20, 2022

If I'm understanding correctly "what we could do is provide a reflect API for accessing the instantiating types" means, I think #54393 is a relevant proposal in that direction.

@mknyszek mknyszek modified the milestones: Go1.20, Backlog, Unplanned Sep 28, 2022
@mknyszek mknyszek assigned mknyszek and unassigned mknyszek Sep 28, 2022
@mdempsky
Copy link
Member

mdempsky commented Sep 28, 2022

One thing we could do is remove the path from the name and just have ambiguous names. There still needs to be underlying uniqueness of the names for the linker+runtime, which means we'd need 2 names for everything.

FWIW, we already have separate type names for reflection and linker+runtime, so I think it's already feasible to change the reflect.Type.String result from test.TestGenericType[play.ground/test.TestType] to test.TestGenericType[test.TestType].

Arguably that would be more consistent with how reflection has historically handled defined types, but it would also be a change from Go 1.18/1.19. I've filed #55924 to discuss this.

I agree that #54393 is the proper way for gomock to handle this case. In the mean time, I think gomock will have to do best-effort parsing of the link names, sorry. See #55924 for a few other special cases it may need to be careful about.

Closing this issue because I don't think there's further action to take on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
Status: Done
Development

No branches or pull requests

7 participants