deprecated reflect mode has been replaced with package mode #207

tulzke · 2024-09-27T15:11:53Z

re-open of: #198

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

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

* added detailed parsing error * error more succinct * removed call of iface.Complete() * renamed checkInterfaceIsNotConstraint -> isConstraint * reduced nesting at parse named segment * add_generate_directive test fixed after I accidentally changed it early

* go updated up to 1.23 for correct aliases support * loading of package was changed from fake package to direct load by pattern * types.Alias added to same case as types.Named * added test case for interface containing aliases in parameters and returns

# Conflicts: # mockgen/reflect.go

* changed errors formatting with less details * regenerated mocks

* fixed type conversion for support go1.22 * simplified loops by using range * removed unused method "generateSalt" * renamed method of test interface: GiveBirth -> AddHumans

tulzke · 2024-09-27T15:14:34Z

@JacobOaks
What do you think about this problem?
#198 (comment)

JacobOaks · 2024-09-27T15:22:52Z

@tulzke - thanks for re-opening this and apologies again for the git mistakes on the other PR.

I think for now it makes sense to just use the unaliased names until we're able to update go.mod to 1.23. From the perspective of type-checking, types and their aliases are interchangeable so there shouldn't be any missing functionality/bugs introduced by just uniformly using the unaliased names for now. This also keeps the generated code consistent between the two versions. Once 1.24 is out and we can upgrade go.mod to 1.23, we can definitely then switch over to using the alias names.

I think the changes to do this aren't too bad, we would just have a separate case in parseType like so:

case *types.Alias:
    return p.parseType(types.Unalias(t))

What are your thoughts?

tulzke · 2024-09-27T16:48:47Z

we would just have a separate case

I think, that alias for us is the same thing as a *types.Named. We need only his name and package.

types.Unalias(t) returns original type, so in mocks we will see original types. It is not completely correct behavior.

tulzke · 2024-09-27T16:57:00Z

So, if we have agreed on the functionality, can we think about the names? What is the name of the new mode to replace reflect?
Maybe Package mode? Source mode and Package mode. That sounds good.

JacobOaks · 2024-09-27T18:36:06Z

I don't have a strong opinion on a name. "package mode" or "types mode" is fine.

* tests * help message * README.md

tulzke · 2024-09-27T19:19:25Z

I don't have a strong opinion on a name. "package mode" or "types mode" is fine.

I choose package mode. Check last commit changes please: 69210e9

If everything is OK, i'm waiting for a merge 🙂

tulzke · 2024-09-27T19:29:41Z

I realized that I had made a mistake.
I deleted 3 unused flags, and this may break the existing generation methods. Since now these flags have no effect, I think they can be removed from the readme and help, and when using write a WARNING message to the console.

@JacobOaks What do you think about this?

//UPD: flags cannot be removed from help, so i added "DEPRECATED" to them usages

JacobOaks · 2024-10-01T18:37:16Z

Hey @tulzke, for --exec_only and --prog_only, I think we can do what you're suggesting.

For build flags, what do you think about maintaining backwards compatibility and passing them to packages.Load via Config which has a BuildFlags field? My understanding is these will get passed to the go list command, which accepts normal go build flags. This will keep things like build tags specified by --tag to be respected when deciding which Go files to load, for example.

tulzke · 2024-10-02T18:06:05Z

@JacobOaks
I returned support for build_flags and added additional test case for it

JacobOaks · 2024-10-03T14:12:48Z

README.md

- `-build_flags`: (reflect mode only) Flags passed verbatim to `go build`.
-


Should we keep these lines now since we are keeping the flag?

You're right, i forgot to do this

JacobOaks

Thanks @tulzke for all your hard work on this PR! LGTM.

tulzke · 2024-10-04T11:13:23Z

Hi @JacobOaks
I'm glad that PR was merged. So, when new release?

… `--exec_only` Change uber-go#207 replaced reflect mode with package mode. In doing so, we no longer had to create separate reflect programs that would encode the resulting package into a gob for the rest of mockgen to read from. Because of that, we removed the `--exec_only` flag. However, this flag was actually used to pass in programs that created gob encodings in other ways, allowing customization of the package loading and encoding portion of mockgen. As an alternative, this adds a new flag to mockgen `--model_gob`, which allows mockgen users to pass a path to a gob encoding of a `model.Package`, so that custom ways of loading package information can still be supported.

… `--exec_only` (#214) Change #207 replaced reflect mode with package mode. In doing so, we no longer had to create separate reflect programs that would encode the resulting package into a gob for the rest of mockgen to read from. Because of that, we removed the `--exec_only` flag. However, this flag was actually used to pass in programs that created gob encodings in other ways, allowing customization of the package loading and encoding portion of mockgen. As an alternative, this adds a new flag to mockgen `--model_gob`, which allows mockgen users to pass a path to a gob encoding of a `model.Package`, so that custom programs for encoding package information can still be used in conjunction with mockgen.

…eting-plugin-hetzner!135) This MR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [go.uber.org/mock](https://github.com/uber/mock) | require | minor | `v0.4.0` -> `v0.5.0` | --- ### Release Notes <details> <summary>uber/mock (go.uber.org/mock)</summary> ### [`v0.5.0`](https://github.com/uber-go/mock/releases/tag/v0.5.0) [Compare Source](uber-go/mock@v0.4.0...v0.5.0) #### 0.5.0 (15 Oct 2024) ##### Added - [#153][]: Add `--write_command_comment` flag to specify whether to include `Generated by this command` comment. - [#191][]: Add `--build_constraint` flag to add `//go:build` directives to generated mocks - [#214][]: Add gob mode to support custom package loading techniques in place of --exec_only ##### Changed - [#181][]: Made mockgen faster by changing flags passed to `go list`. - [#183][]: Made `Cond` matcher generic. - [#204][]: Removed `ISGOMOCK()` from generated mocks. - [#207][]: Deprecated reflect mode and replaced it with the new package mode. ##### Fixed - [#144][]: Fix a deadlock that can happen when mocking an interface that matches `fmt.Stringer`. - [#168][]: Fix an issue where the "generated by" comment was being included in the package comment of generated mocks. [#144]: uber-go/mock#144 [#153]: uber-go/mock#153 [#168]: uber-go/mock#168 [#181]: uber-go/mock#181 [#183]: uber-go/mock#183 [#191]: uber-go/mock#191 [#204]: uber-go/mock#204 [#207]: uber-go/mock#207 [#214]: uber-go/mock#214 Thanks to [@tulzke](https://github.com/tulzke) [@JacobOaks](https://github.com/JacobOaks) [@ARR4N](https://github.com/ARR4N) [@sashamelentyev](https://github.com/sashamelentyev) [@sywhang](https://github.com/sywhang) [@fasmat](https://github.com/fasmat) [@eyasy1217](https://github.com/eyasy1217) [@ghouscht](https://github.com/ghouscht) [@tie](https://github.com/tie) [@Neo2308](https://github.com/Neo2308) [@carson-brill](https://github.com/carson-brill) [@alexandear](https://github.com/alexandear) [@sodul](https://github.com/sodul) [@nbgraham](https://github.com/nbgraham) for their contributions this release. </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

v0.5.0 included uber-go#207, which replaced reflect mode with package mode. One issue with package mode that came up (ref: uber-go#216) was that generated mocks for interfaces that referred to alias types were referring to the aliases' underlying names instead. e.g., source: ```go import "github.com/tikv/client-go/v2/internal/apicodec" ... type Codec = apicodec.Codec type Foo interface{ Bar() Codec } ``` mock: ```go func (m *MockFoo) Bar() apicodec.Codec { // This is a problem, since apicodec is an internal package. // ... } ``` While technically this problem is solved in Go 1.23 with explicit alias types representation, (indeed, if you run mockgen on the example in the linked issue with `GODEBUG=gotypesalias=1`, you get the expected behavior) since we support the last two versions, we can't bump `go.mod` to 1.23 yet. This leaves us with the old behavior, where `go/types` does not track alias types. You can tell if an object is an alias, but not a type itself, and there is no way to retrieve the object of interest at the point where we are recursively parsing method types. This PR works around this issue (temporarily) by using syntax information to find all references to aliases in the source package. When we find one, we record it in a mapping of underlying type -> alias name. Later, while we parse the type tree, we replace any underlying types in the mapping with their alias names. The unexpected side effect of this is that _all_ references to the underlying type in the generated mocks will be replaced with the alias, even if the source used the underlying name. This is fine because: * If the alias is in the mapping, it was used at least once, which means its accessible. * From a type-checking perspective, aliases and their underlying types are equivalent. With this PR, the mocks get generated correctly now: ```go func (m *MockFoo) Bar() Codec { // ... } ``` Once we can bump `go.mod` to 1.23, we should definitely remove this, since the new type alias type nodes solve this problem automatically.

v0.5.0 included #207, which replaced reflect mode with package mode. One issue with package mode that came up (ref: #216) was that generated mocks for interfaces that referred to alias types were referring to the aliases' underlying names instead. e.g., some package: ```go package somgpkg import "somepkg/internal/apicodec" ... type Codec = apicodec.Codec ``` mockgen input: ```go type Foo interface{ Bar() somepkg.Codec } ``` mock: ```go func (m *MockFoo) Bar() apicodec.Codec { // This is a problem, since apicodec is an internal package. // ... } ``` While technically this problem is solved in Go 1.23 with explicit alias types representation, (indeed, if you run mockgen on the example in the linked issue with `GODEBUG=gotypesalias=1`, you get the expected behavior) since we support the last two versions, we can't bump `go.mod` to 1.23 yet. This leaves us with the old behavior, where `go/types` does not track alias types. You can tell if an object is an alias, but not a type itself, and there is no way to retrieve the object of interest at the point where we are recursively parsing method types. This PR works around this issue (temporarily) by using syntax information to find all references to aliases in the source package. When we find one, we record it in a mapping of underlying type -> alias name. Later, while we parse the type tree, we replace any underlying types in the mapping with their alias names. The unexpected side effect of this is that _all_ references to the underlying type in the generated mocks will be replaced with the alias, even if the source used the underlying name. This is fine because: * If the alias is in the mapping, it was used at least once, which means its accessible. * From a type-checking perspective, aliases and their underlying types are equivalent. The nice exception to the side effect is when we explicitly request mock generation for an alias type, since at that point we are dealing with the object, not the type. With this PR, the mocks get generated correctly now: ```go func (m *MockFoo) Bar() Codec { // ... } ``` Once we can bump `go.mod` to 1.23, we should definitely remove this, since the new type alias type nodes solve this problem automatically.

ref: uber-go/mock#207

tulzke and others added 12 commits August 9, 2024 14:34

deprecated reflect mode has been replaced with import mode. benefits:

3075423

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

tests has been reworked

2b8cc4c

review changes:

0643be9

* added detailed parsing error * error more succinct * removed call of iface.Complete() * renamed checkInterfaceIsNotConstraint -> isConstraint * reduced nesting at parse named segment * add_generate_directive test fixed after I accidentally changed it early

fixed misprint in the error

703a35f

added types.Unalias call

02bca6a

review:

07d869b

* go updated up to 1.23 for correct aliases support * loading of package was changed from fake package to direct load by pattern * types.Alias added to same case as types.Named * added test case for interface containing aliases in parameters and returns

Merge remote-tracking branch 'upstream/main'

2cb45fb

# Conflicts: # mockgen/reflect.go

Merge branch 'main' into main

6ac4347

review changes:

c630751

* changed errors formatting with less details * regenerated mocks

regenerated mock

c5cd86f

changed test for compatibility with type aliases

08f7db9

review changes:

5b309e7

* fixed type conversion for support go1.22 * simplified loops by using range * removed unused method "generateSalt" * renamed method of test interface: GiveBirth -> AddHumans

tulzke mentioned this pull request Sep 27, 2024

deprecated reflect mode has been replaced with import mode #198

Closed

behavior changed to default in go1.22 without GODEBUG features

6fc5cf5

import mode renamed to package mode in:

69210e9

* tests * help message * README.md

tulzke added 2 commits September 27, 2024 23:01

added warning message if deprecated flags are set

cbd7843

regenerated forgotten mock

b14e7ad

tulzke changed the title ~~deprecated reflect mode has been replaced with import mode~~ deprecated reflect mode has been replaced with package mode Sep 29, 2024

returned support for build_flags flag

dcf893d

JacobOaks reviewed Oct 3, 2024

View reviewed changes

returned build_flags description to README.md

3d7cfd3

JacobOaks approved these changes Oct 3, 2024

View reviewed changes

JacobOaks merged commit d01ed30 into uber-go:main Oct 3, 2024
3 checks passed

ebilling mentioned this pull request Oct 9, 2024

New Release? #210

Closed

JacobOaks mentioned this pull request Oct 17, 2024

Add gob mode to support custom package loading techniques in place of --exec_only #214

Merged

JacobOaks mentioned this pull request Oct 24, 2024

Package Mode: Use aliases when used in source #220

Merged

utgwkk added a commit to utgwkk/bulkmockgen that referenced this pull request Oct 29, 2024

doc: reflect mode is deprecated and package mode is introduced

fe0c5bf

ref: uber-go/mock#207

utgwkk mentioned this pull request Oct 29, 2024

doc: reflect mode is deprecated and package mode is introduced utgwkk/bulkmockgen#18

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deprecated reflect mode has been replaced with package mode #207

deprecated reflect mode has been replaced with package mode #207

tulzke commented Sep 27, 2024

tulzke commented Sep 27, 2024

JacobOaks commented Sep 27, 2024

tulzke commented Sep 27, 2024

tulzke commented Sep 27, 2024

JacobOaks commented Sep 27, 2024

tulzke commented Sep 27, 2024

tulzke commented Sep 27, 2024 •

edited

Loading

JacobOaks commented Oct 1, 2024 •

edited

Loading

tulzke commented Oct 2, 2024 •

edited

Loading

JacobOaks Oct 3, 2024

tulzke Oct 3, 2024

JacobOaks left a comment

tulzke commented Oct 4, 2024 •

edited

Loading

		- `-build_flags`: (reflect mode only) Flags passed verbatim to `go build`.

deprecated reflect mode has been replaced with package mode #207

deprecated reflect mode has been replaced with package mode #207

Conversation

tulzke commented Sep 27, 2024

tulzke commented Sep 27, 2024

JacobOaks commented Sep 27, 2024

tulzke commented Sep 27, 2024

tulzke commented Sep 27, 2024

JacobOaks commented Sep 27, 2024

tulzke commented Sep 27, 2024

tulzke commented Sep 27, 2024 • edited Loading

JacobOaks commented Oct 1, 2024 • edited Loading

tulzke commented Oct 2, 2024 • edited Loading

JacobOaks Oct 3, 2024

Choose a reason for hiding this comment

tulzke Oct 3, 2024

Choose a reason for hiding this comment

JacobOaks left a comment

Choose a reason for hiding this comment

tulzke commented Oct 4, 2024 • edited Loading

tulzke commented Sep 27, 2024 •

edited

Loading

JacobOaks commented Oct 1, 2024 •

edited

Loading

tulzke commented Oct 2, 2024 •

edited

Loading

tulzke commented Oct 4, 2024 •

edited

Loading