Fix Go 1.23 types.Alias handling #808

RangelReale · 2024-08-20T16:38:35Z

Description

Support the new types.Alias type, both in Go 1.22 and Go 1.23.

If types.Alias is supported, replace-types will not match the new type, so it is safe to leave it as it is until we stop supporting Go 1.22.

Go 1.22 includes a types.Alias type to ease the migration, but by default is is never returned unless gotypesalias=1. It has more fields in Go 1.23, so build constraints had to be used to cover these cases.

Fixes Go 1.23: Mock generation fails with alias types #803

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Version of Go used when building/testing:

How Has This Been Tested?

Added new tests.

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

LandonTClipp · 2024-08-22T19:00:08Z

pkg/generator.go

@@ -508,6 +508,8 @@ func (g *Generator) renderType(ctx context.Context, typ types.Type) string {
 			args = append(args, g.renderType(ctx, arg))
 		}
 		return fmt.Sprintf("%s[%s]", name, strings.Join(args, ","))
+	case *types.Alias:


I'm actually surprised that CI/CD is not failing in the go1.21 test. This didn't exist until 1.22 so technically this won't compile if someone tries to build with 1.21.

If I'm understanding that correctly, we will need to make sure people understand. But I'd also like to figure out why CICD didn't fail.

I see, go will download at least 1.22 because of our go.mod

$ /opt/homebrew/opt/[email protected]/bin/go version go: downloading go1.22.0 (darwin/arm64) go version go1.22.0 darwin/arm64

Also 1.21 is not supported anymore anyways, so we can probably remove it from the matrix and figure out how to get 1.23 to work in the test matrix.

Warning: [email protected] has been deprecated because it is not supported upstream! It will be disabled on 2025-08-16.

I did a CI test with go 1.23 enabled, but it requires the lastest golangci-lint, but installing it forces the library's go.mod to go 1.23.

LandonTClipp

@RangelReale we need to upgrade mockery to include 1.23 in the test matrix. I also think we'll need to bump go.mod to 1.23 as well considering that we'll be supporting type.Alias now.

Some of the depdencies like golangci-lint and go-task need to be updated as well.

Unfortunately it seems like golangci-lint updates cause a few new linter errors:

 $ go run github.com/go-task/task/v3/cmd/task test.ci
task: [lint] go run github.com/golangci/golangci-lint/cmd/golangci-lint run
task: [fmt] go fmt ./...
go: downloading github.com/prometheus/client_model v0.5.0
WARN The linter 'exportloopref' is deprecated (since v1.60.2) due to: Since Go1.22 (loopvar) this linter is no longer relevant. Replaced by copyloopvar. 
pkg/logging/logging.go:77:25: G115: integer overflow conversion uintptr -> int (gosec)
        if !term.IsTerminal(int(out.Fd())) || os.Getenv("TERM") == "dumb" {
                               ^
pkg/generator.go:440:11: printf: non-constant format string in call to (*github.com/vektra/mockery/v2/pkg.Generator).printf (govet)
        g.printf(prologue)
                 ^
pkg/config/config.go:191:33: printf: non-constant format string in call to fmt.Errorf (govet)
                return []string{}, fmt.Errorf(msg)
                                              ^
pkg/config/config.go:398:20: printf: non-constant format string in call to (*github.com/rs/zerolog.Event).Msgf (govet)
                log.Error().Msgf(msgString)
                                 ^
pkg/config/config.go:798:22: printf: non-constant format string in call to fmt.Errorf (govet)
                        return fmt.Errorf(msg)

Let me know if you would like to tackle the upgrades first. Once that is merged, you'll want to rebase this branch off of it to ensure the go1.23 changes work as expected. If you don't have time, perhaps I can look into the upgrades.

LandonTClipp · 2024-08-22T19:32:29Z

Another comment, I'm kind of doubting my mental model of how Go handles go.mod. It seems to be that if we specify go 1.22 in go.mod, Go will only use 1.22 to compile regardless of the installed version on the remote site. So if 1.22 is specified in go.mod, then files with //go:build go1.23 will never get compiled?? That is my question.

RangelReale · 2024-08-22T20:22:15Z

Another comment, I'm kind of doubting my mental model of how Go handles go.mod. It seems to be that if we specify go 1.22 in go.mod, Go will only use 1.22 to compile regardless of the installed version on the remote site. So if 1.22 is specified in go.mod, then files with //go:build go1.23 will never get compiled?? That is my question.

I thought that too, but no, surprisingly the //go:build tag is for the version that is being used for compilation, not the one that is in the go.mod files, I had to do some hacks because of it.

RangelReale · 2024-08-22T20:23:20Z

@RangelReale we need to upgrade mockery to include 1.23 in the test matrix. I also think we'll need to bump go.mod to 1.23 as well considering that we'll be supporting type.Alias now.

Some of the depdencies like golangci-lint and go-task need to be updated as well.

Unfortunately it seems like golangci-lint updates cause a few new linter errors:

 $ go run github.com/go-task/task/v3/cmd/task test.ci
task: [lint] go run github.com/golangci/golangci-lint/cmd/golangci-lint run
task: [fmt] go fmt ./...
go: downloading github.com/prometheus/client_model v0.5.0
WARN The linter 'exportloopref' is deprecated (since v1.60.2) due to: Since Go1.22 (loopvar) this linter is no longer relevant. Replaced by copyloopvar. 
pkg/logging/logging.go:77:25: G115: integer overflow conversion uintptr -> int (gosec)
        if !term.IsTerminal(int(out.Fd())) || os.Getenv("TERM") == "dumb" {
                               ^
pkg/generator.go:440:11: printf: non-constant format string in call to (*github.com/vektra/mockery/v2/pkg.Generator).printf (govet)
        g.printf(prologue)
                 ^
pkg/config/config.go:191:33: printf: non-constant format string in call to fmt.Errorf (govet)
                return []string{}, fmt.Errorf(msg)
                                              ^
pkg/config/config.go:398:20: printf: non-constant format string in call to (*github.com/rs/zerolog.Event).Msgf (govet)
                log.Error().Msgf(msgString)
                                 ^
pkg/config/config.go:798:22: printf: non-constant format string in call to fmt.Errorf (govet)
                        return fmt.Errorf(msg)

Let me know if you would like to tackle the upgrades first. Once that is merged, you'll want to rebase this branch off of it to ensure the go1.23 changes work as expected. If you don't have time, perhaps I can look into the upgrades.

I actually upgraded it, but reverted once I say that it changed go.mod, it's not too much things, I can do it again. But will we bump the go.mod version to 1.23? Isn't it too early?

LandonTClipp · 2024-08-22T20:55:22Z

I actually upgraded it, but reverted once I say that it changed go.mod, it's not too much things, I can do it again. But will we bump the go.mod version to 1.23? Isn't it too early?

Maybe so. I don't actually know the right thing to do. AFAIK go.mod go directive tells go which language semantics we use. Mockery does not use any semantics from 1.23 but it does probably need the updated toolchain so we can correctly parse these new features.

I would like if someone could educate me on the right way to do it. Much of how Go handles this is still mysterious to me.

RangelReale · 2024-08-22T20:58:13Z

I actually upgraded it, but reverted once I say that it changed go.mod, it's not too much things, I can do it again. But will we bump the go.mod version to 1.23? Isn't it too early?

Maybe so. I don't actually know the right thing to do. AFAIK go.mod go directive tells go which language semantics we use. Mockery does not use any semantics from 1.23 but it does probably need the updated toolchain so we can correctly parse these new features.

I would like if someone could educate me on the right way to do it. Much of how Go handles this is still mysterious to me.

I don't know it completely also, I know that go getting the newest golangci-lint, changed go.mod (actually not the go version, but the "toolchain" line was changed to 1.23, the go version stayed at 1.22, so it was even more confusing.

RangelReale · 2024-08-24T13:42:14Z

Upgraded golangci-lint and replace go 1.21 with go 1.23 in tests.

RangelReale · 2024-08-24T13:44:15Z

In the end it didn't change the go.mod in the root folder, so we should be good.

LandonTClipp · 2024-08-24T23:16:13Z

Wonderful! Looks great. I will give final approval and merge/deploy in a few days.

LandonTClipp · 2024-08-25T02:20:57Z

I looked more into this to make sure we're doing the right thing. Some notes:

`go.mod`

The root go.mod needs to have go 1.23 set. In my testing, I found that *types.Alias was not returned at all if this was not given, which means (unless I have mistaken something) that the generator_alias.go file would always be built (instead of the _go123.go variant) regardless of the installed go version of the end user. This also means that our testing matrix over 1.22 and 1.23 is not really asserting anything useful because the Go tool automatically snaps to whatever version we specify in go.mod.

Thus, if we really want to fix this for end users, we need go 1.23 set. Otherwise, they will need to continue to use gotypesalias=1.

`go:build`

The generator_alias*.go files probably don't need to be differentiated by build anymore. Because the behavior of Go is to snap to the version specified in go.mod, we can be guaranteed we'll always be built with at least the version specified therein. So if we're moving to 1.23, there is no sense in building for !go1.23...

tests

This brings me to the tests. Again, since we need go 1.23, the 1.23 tests don't need to be separated anymore, and we can remove the isTypeAliasEnabled check.

RangelReale · 2024-08-26T13:47:49Z

I did some other tests with a separate repo using go 1.23 in go.mod, and importing mockery which has go 1.22 in the go.mod, and it really don't generate the types.Alias type. I tried stepping in the code, but it is a lot of trickery, I was not able to understand how it works.

So, the only way to ensure aliases being generated without GODEBUG is making go 1.23 a requirement.

In my case I am using tools.go to manage the mockery version, so I won't be able to upgrade until I migrate by repo to 1.23 (we always wait for the ".1" patch release), so this will probably be a problem for some people. But won't if they only use the executable.

RangelReale · 2024-09-27T17:48:36Z

Well Go 1.23.1 was released and we upgraded our repo to it.

But I see you created a new major version, maybe you would want to do this in that version?

LandonTClipp · 2024-09-27T20:46:54Z

I'm going through an airport here shortly so I will have time to get to this. I think we can upgrade it in the current major version since AFAIK it is backwards compatible with the user's installed Go version.

LandonTClipp · 2024-09-28T02:24:38Z

I made a few changes and merged. Thanks @RangelReale for the great work!

I will do a release once I get better internet. Somehow I am able to browse the web on an airplane but I can't tag a git repo. Who knows why.

max-melentyev · 2024-11-05T21:11:29Z

Hey, looks like this change results in a circular dependency if mock is used in the package where it's defined:

// ./p/iface.go
package p

type T = int

//go:generate mockery --quiet --name I
type I interface{ F(T) }

// this generates ./p/mocks/I.go
package mocks
import "p"

func (_m *I) F(_a0 p.T) {
...

// ./p/i_test.go
import "p/mocks" // fails because of circular dependency

Is it possible to use target types instead of aliases in go 1.23?

RangelReale added 3 commits August 20, 2024 11:53

support types.Alias

66dba08

support types.Alias

42d905a

support types.Alias

6f7960f

RangelReale mentioned this pull request Aug 20, 2024

Go 1.23: Mock generation fails with alias types #803

Closed

5 tasks

remove Go 1.23 testing

c05a755

LandonTClipp reviewed Aug 22, 2024

View reviewed changes

LandonTClipp requested changes Aug 22, 2024

View reviewed changes

upgrade golangci-lint and add Go 1.23 to CI test

ea36ecc

upgrade go task

e4b095f

LandonTClipp mentioned this pull request Sep 18, 2024

Unable to generate interface type from another interface #787

Open

5 tasks

Update to go1.23

cf26d69

LandonTClipp approved these changes Sep 28, 2024

View reviewed changes

Undo go:build statements, make code a little DRY

720caa9

LandonTClipp mentioned this pull request Sep 28, 2024

Support go 1.23 from the docker image #818

Closed

12 tasks

LandonTClipp merged commit afe04a5 into vektra:master Sep 28, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Go 1.23 types.Alias handling #808

Fix Go 1.23 types.Alias handling #808

RangelReale commented Aug 20, 2024

LandonTClipp Aug 22, 2024

LandonTClipp Aug 22, 2024

RangelReale Aug 22, 2024

LandonTClipp left a comment •

edited

Loading

LandonTClipp commented Aug 22, 2024

RangelReale commented Aug 22, 2024

RangelReale commented Aug 22, 2024

LandonTClipp commented Aug 22, 2024

RangelReale commented Aug 22, 2024

RangelReale commented Aug 24, 2024

RangelReale commented Aug 24, 2024

LandonTClipp commented Aug 24, 2024

LandonTClipp commented Aug 25, 2024 •

edited

Loading

RangelReale commented Aug 26, 2024

RangelReale commented Sep 27, 2024

LandonTClipp commented Sep 27, 2024

LandonTClipp commented Sep 28, 2024

max-melentyev commented Nov 5, 2024 •

edited

Loading

Fix Go 1.23 types.Alias handling #808

Fix Go 1.23 types.Alias handling #808

Conversation

RangelReale commented Aug 20, 2024

Description

Type of change

Version of Go used when building/testing:

How Has This Been Tested?

Checklist

LandonTClipp Aug 22, 2024

Choose a reason for hiding this comment

LandonTClipp Aug 22, 2024

Choose a reason for hiding this comment

RangelReale Aug 22, 2024

Choose a reason for hiding this comment

LandonTClipp left a comment • edited Loading

Choose a reason for hiding this comment

LandonTClipp commented Aug 22, 2024

RangelReale commented Aug 22, 2024

RangelReale commented Aug 22, 2024

LandonTClipp commented Aug 22, 2024

RangelReale commented Aug 22, 2024

RangelReale commented Aug 24, 2024

RangelReale commented Aug 24, 2024

LandonTClipp commented Aug 24, 2024

LandonTClipp commented Aug 25, 2024 • edited Loading

go.mod

go:build

tests

RangelReale commented Aug 26, 2024

RangelReale commented Sep 27, 2024

LandonTClipp commented Sep 27, 2024

LandonTClipp commented Sep 28, 2024

max-melentyev commented Nov 5, 2024 • edited Loading

LandonTClipp left a comment •

edited

Loading

LandonTClipp commented Aug 25, 2024 •

edited

Loading

`go.mod`

`go:build`

max-melentyev commented Nov 5, 2024 •

edited

Loading