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

issue #298 - go 1.23 go/types alias change #301

Merged
merged 2 commits into from
Oct 10, 2024

Conversation

xanderflood
Copy link
Contributor

@xanderflood xanderflood commented Oct 9, 2024

From the go 1.23 changelog:

By default, go/types now produces Alias type nodes for type aliases. This behavior can be controlled by the GODEBUG gotypesalias flag. Its default has changed from 0 in Go 1.22 to 1 in Go 1.23.

Previously, the AST node for an alias-typed value would point directly at the underlying type, and counterfeiter would consequently see the underlying type and load the correct package(s) for that type.

In go 1.23 or newer, when processing such a node, we get the unrecognized *types.Alias type, and fall through to the default case, which prints an error and moves on. Then, when it comes time to render that type in (for instance) a method signature, it is rendered as a simple name, since the package alias was never added to the Imports struct.

This change ensures that we load the appropriate package for each alias type. This resolves a bug introduced by go 1.23, and also slightly changes the behavior from previous versions of counterfeiter, which implicitly resolved alias types and replaced them with the RHS (right-hand side) of the alias assignment. If we wanted to preserve that behavior, I believe we could instead use:

case *types.Alias:
    f.addImportsFor(t.Rhs())

However, I think using the actual alias (mimicking the interface being mocked) is preferable to resolving the alias.

@joefitzgerald
Copy link
Collaborator

Thanks for the contribution! I will take a look tonight.

@joefitzgerald joefitzgerald merged commit 241cc37 into maxbrunsfeld:main Oct 10, 2024
8 of 10 checks passed
@joefitzgerald
Copy link
Collaborator

joefitzgerald commented Oct 10, 2024

The change looks good to me. I added an integration test to prevent any regression. This is released in v6.10.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants