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

Support GODEBUG=gotypesalias=1 #248

Merged
merged 2 commits into from
Jul 2, 2024

Conversation

JacobOaks
Copy link
Contributor

Currently, the go/types.Type for a declared type alias is the same type as the that which it is aliased too.

In Go 1.23, when GODEBUG=gotypesalias=1 is enabled by default, it is a go/types.Alias instead, which is a unique type node for aliases that points to the type it is aliased to via go/types.Unalias().
See golang/go#63223 for more details.

errcheck's tests do not currently fail with GODEBUG=gotypesalias=1, but that is because the tests themselves do not exercise the problematic situations, such as the following:

type t struct{}

func (x t) a() error { /* ... */ }

type embedt struct {
	t
}

type embedtalias = embedt

func foo() {
	embedtalias.a()
}

With GODEBUG=gotypesalias=1, errcheck will panic when inspecting embedtalias.a() to see if it should be excluded from checking:

panic: cannot get Field of a type that is not a struct, got _/home/user/go/src/github.com/kisielk/errcheck/errcheck/testdata/src/a.embedtalias (*types.Alias)

goroutine 2962 [running]:
github.com/kisielk/errcheck/errcheck.getTypeAtFieldIndex({0x7779c0?, 0xc002eceab0?}, 0x4c?)
        /home/user/go/src/github.com/kisielk/errcheck/errcheck/embedded_walker.go:96 +0xf9
github.com/kisielk/errcheck/errcheck.walkThroughEmbeddedInterfaces(0x6b83a0?)
        /home/user/go/src/github.com/kisielk/errcheck/errcheck/embedded_walker.go:53 +0x8e
github.com/kisielk/errcheck/errcheck.(*visitor).namesForExcludeCheck(0xc0025eeff0, 0xc002596140?)
        /home/user/go/src/github.com/kisielk/errcheck/errcheck/errcheck.go:364 +0x165
github.com/kisielk/errcheck/errcheck.(*visitor).excludeCall(0xc0025eeff0, 0x4d?)
        /home/user/go/src/github.com/kisielk/errcheck/errcheck/errcheck.go:403 +0x72
github.com/kisielk/errcheck/errcheck.(*visitor).ignoreCall(0xc0025eeff0, 0xc002596140)
        /home/user/go/src/github.com/kisielk/errcheck/errcheck/errcheck.go:415 +0x2c
github.com/kisielk/errcheck/errcheck.(*visitor).Visit(0xc0025eeff0, {0x776d48?, 0xc0066e08a0})
        /home/user/go/src/github.com/kisielk/errcheck/errcheck/errcheck.go:564 +0x137

This change uses types.Unalias to follow through this extra level of indirection, which fixes the panic and ensures the same behavior with both settings.

This change also adds these cases to the test suite, and runs the tests using both settings of gotypesalias. Without the calls to types.Unalias, the tests fail.

@JacobOaks JacobOaks marked this pull request as ready for review June 14, 2024 17:50
Currently, the go/types.Type for a declared type alias
is the same type as the that which it is aliased too.

In Go 1.23, when GODEBUG=gotypesalias=1 is enabled by default,
it is a go/types.Alias instead, which is a unique type node
for aliases that points to the type it is aliased to via go/types.Unalias().
(see golang/go#63223 for more details).

errcheck's tests do not currently fail with GODEBUG=gotypesalias=1,
but that is because the tests themselves do not exercise the problematic situations,
such as the following:

```go
type t struct{}

func (x t) a() error { /* ... */ }

type embedt struct {
	t
}

type embedtalias = embedt

func foo() {
	embedtalias.a()
}
```

With GODEBUG=gotypesalias=1, errcheck will panic when inspecting `embedtalias.a()`
to see if it should be excluded from checking:

```
panic: cannot get Field of a type that is not a struct, got _/home/user/go/src/github.com/kisielk/errcheck/errcheck/testdata/src/a.embedtalias (*types.Alias)

goroutine 2962 [running]:
github.com/kisielk/errcheck/errcheck.getTypeAtFieldIndex({0x7779c0?, 0xc002eceab0?}, 0x4c?)
        /home/user/go/src/github.com/kisielk/errcheck/errcheck/embedded_walker.go:96 +0xf9
github.com/kisielk/errcheck/errcheck.walkThroughEmbeddedInterfaces(0x6b83a0?)
        /home/user/go/src/github.com/kisielk/errcheck/errcheck/embedded_walker.go:53 +0x8e
github.com/kisielk/errcheck/errcheck.(*visitor).namesForExcludeCheck(0xc0025eeff0, 0xc002596140?)
        /home/user/go/src/github.com/kisielk/errcheck/errcheck/errcheck.go:364 +0x165
github.com/kisielk/errcheck/errcheck.(*visitor).excludeCall(0xc0025eeff0, 0x4d?)
        /home/user/go/src/github.com/kisielk/errcheck/errcheck/errcheck.go:403 +0x72
github.com/kisielk/errcheck/errcheck.(*visitor).ignoreCall(0xc0025eeff0, 0xc002596140)
        /home/user/go/src/github.com/kisielk/errcheck/errcheck/errcheck.go:415 +0x2c
github.com/kisielk/errcheck/errcheck.(*visitor).Visit(0xc0025eeff0, {0x776d48?, 0xc0066e08a0})
        /home/user/go/src/github.com/kisielk/errcheck/errcheck/errcheck.go:564 +0x137
```

This change uses `types.Unalias` to follow through this extra level of indirection,
which fixes the panic and ensures the same behavior with both settings.

This change also adds these cases to the test suite, and runs the tests
using both settings of gotypesalias. Without the calls to `types.Unalias`,
the tests fail.
Copy link
Collaborator

@dtcaciuc dtcaciuc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@dtcaciuc dtcaciuc merged commit b832de3 into kisielk:master Jul 2, 2024
5 checks passed
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.

3 participants