x/tools/internal/typesinternal/: regression inside v0.19.0 related to ReceiverNamed

[ldez]

Go version

go version go1.22.1 linux/amd64

Output of go env in your module/workspace:

GO111MODULE='on'
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/ldez/.cache/go-build'
GOENV='/home/ldez/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/ldez/sources/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/ldez/sources/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/lib/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/lib/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.22.0'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/ldez/sources/go/src/github.com/golangci/golangci-lint/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build1404820727=/tmp/go-build -gno-record-gcc-switches'

What did you do?

Using httpresponse pass through golangci-lint (current master branch) on this project golangci-repro-4482

./golangci-lint run ./tools/cmd/debug/...

Related to golangci/golangci-lint#4482

I bisected x/tools and the first commit with this problem is c111c4dfa

the value of n in n, ok := aliases.Unalias(t).(*types.Named) (inside IsNamedType) is nil and ok is true.

I think the problem is related to named, _ = aliases.Unalias(t).(*types.Named) inside ReceiverNamed.

The type assertion is "forcing" the type of named.

I'm still trying to find a minimal reproducible example, but if I extract the code the panic disappears.

What did you see happen?

A panic:

runtime/debug.Stack()
        /usr/lib/go/src/runtime/debug/stack.go:24 +0x5e
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyzeSafe.func1()
        /home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_action.go:108 +0x277
panic({0x169eda0?, 0x22c2360?})
        /usr/lib/go/src/runtime/panic.go:770 +0x132
go/types.(*Named).Obj(...)
        /usr/lib/go/src/go/types/named.go:295
golang.org/x/tools/go/analysis/passes/internal/analysisutil.IsNamedType({0x1aa5cf0?, 0x0?}, {0x180ef88, 0x8}, {0xc0022d7af8, 0x1, 0x6c61562a5d5b2073?})
        /home/ldez/sources/go/pkg/mod/golang.org/x/tools@v0.19.0/go/analysis/passes/internal/analysisutil/util.go:123 +0x49
golang.org/x/tools/go/analysis/passes/httpresponse.isHTTPFuncOrMethodOnClient(0xc015ef7380, 0xc0022d7ba0?)
        /home/ldez/sources/go/pkg/mod/golang.org/x/tools@v0.19.0/go/analysis/passes/httpresponse/httpresponse.go:122 +0xfb
golang.org/x/tools/go/analysis/passes/httpresponse.run.func1({0x1aa5110?, 0xc01642f700?}, 0x10?, {0xc007030200, 0x6, 0x20})
        /home/ldez/sources/go/pkg/mod/golang.org/x/tools@v0.19.0/go/analysis/passes/httpresponse/httpresponse.go:62 +0x73
golang.org/x/tools/go/ast/inspector.(*Inspector).WithStack(0xc0164ed200, {0xc0022d7cb8?, 0x22ee1c0?, 0xc001208008?}, 0xc002155cc8)
        /home/ldez/sources/go/pkg/mod/golang.org/x/tools@v0.19.0/go/ast/inspector/inspector.go:148 +0x188
golang.org/x/tools/go/analysis/passes/httpresponse.run(0xc01e3fc0d0)
        /home/ldez/sources/go/pkg/mod/golang.org/x/tools@v0.19.0/go/analysis/passes/httpresponse/httpresponse.go:57 +0x112
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyze(0xc00128e7e0)
        /home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_action.go:190 +0xa02
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyzeSafe.func2()
        /home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_action.go:112 +0x17
github.com/golangci/golangci-lint/pkg/timeutils.(*Stopwatch).TrackStage(0xc001b5c3c0, {0x18596d7, 0xc}, 0xc002138f48)
        /home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/timeutils/stopwatch.go:111 +0x44
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyzeSafe(0xc001037e60?)
        /home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_action.go:111 +0x7a
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*loadingPackage).analyze.func2(0xc00128e7e0)
        /home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_loadingpackage.go:80 +0xa8
created by github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*loadingPackage).analyze in goroutine 204
        /home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_loadingpackage.go:75 +0x205 
WARN [runner] Can't run linter goanalysis_metalinter: goanalysis_metalinter: httpresponse: package "main" (isInitialPkg: true, needAnalyzeSource: true): runtime error: invalid memory address or nil pointer dereference

What did you expect to see?

No panic.

[ldez]

The problem comes from here:

func ReceiverNamed(recv *types.Var) (isPtr bool, named *types.Named) { // <---
	t := recv.Type()
	if ptr, ok := aliases.Unalias(t).(*types.Pointer); ok {
		isPtr = true
		t = ptr.Elem()
	}
	named, _ = aliases.Unalias(t).(*types.Named) // <---
	return
}

If I replace the call to this function inside httpresponse by:

func ReceiverNamedTemp(recv *types.Var) (isPtr bool, named types.Type) {
	t := recv.Type()
	if ptr, ok := aliases.Unalias(t).(*types.Pointer); ok {
		isPtr = true
		t = ptr.Elem()
	}

	named, ok := aliases.Unalias(t).(*types.Named)
	if !ok {
		named = aliases.Unalias(t)
	}

	return
}

The problem disappears.

The type assertion, inside ReceiverNamed, is forcing the type of named, and then when IsNamedType checks the same assertion the result is true instead of false.

https://go.dev/play/p/woJnLMGAgdR

package typesinternal

import (
	"fmt"
	"go/token"
	"go/types"
	"testing"

	"golang.org/x/tools/internal/aliases"
)

func Test_ReceiverNamed(t *testing.T) {
	v := types.NewVar(token.NoPos, nil, "", types.NewPointer(types.NewArray(types.Typ[types.Int], 10)))
	ptr, named := ReceiverNamed(v)

	println(ptr)
	fmt.Printf("%T\n", named)

	// true
	// *types.Named

	_, ok := aliases.Unalias(named).(*types.Named)

	println(ok)

	// true
}

func Test_ReceiverNamedTemp(t *testing.T) {
	v := types.NewVar(token.NoPos, nil, "", types.NewPointer(types.NewArray(types.Typ[types.Int], 10)))
	ptr, named := ReceiverNamedTemp(v)

	println(ptr)
	fmt.Printf("%T\n", named)

	// true
	// *types.Array

	_, ok := aliases.Unalias(named).(*types.Named)

	println(ok)

	// false
}

func ReceiverNamedTemp(recv *types.Var) (isPtr bool, named types.Type) {
	t := recv.Type()
	if ptr, ok := aliases.Unalias(t).(*types.Pointer); ok {
		isPtr = true
		t = ptr.Elem()
	}

	named, ok := aliases.Unalias(t).(*types.Named)
	if !ok {
		named = aliases.Unalias(t)
	}

	return
}

The problem affects at least httpresponse, httpmux, loopclosure.

[ldez]

The problem is the function itself because it's designed to return a *types.Named.
I don't know what the right solution is. 🤔

[ldez]

It's a critical problem for, at least, us (golangci-lint maintainers) and, I think, for go vet.

I cannot downgrade because some dependencies already use v0.19.0 and I cannot use replace directive because go install doesn't support it.

I will postpone the release of golangci-lint for now.

I am interested in creating a fix, but I think this should be discussed before doing something.

[timothy-king]

If ReceiverNamedTemp is helping, that looks like a *types.Named to a types.Type conversion might be happening.

If this is the case, the desired solution is probably to audit the users of the function. Not update the signature.

[ldez]

ReceiverNamedTemp was just an example to illustrate the problem, not a suggestion.

I think you suggest that the root of the problem is not here, but at least, the expectations of the function seem optimistic about the type.
Maybe panic can be added?

[ldez]

I compiled the unitchecker from v0.19.0.

$ go vet -vettool=unitchecker ./tools/cmd/debug/...                                
# gitlab.com/accumulatenetwork/accumulate/tools/cmd/debug
# [gitlab.com/accumulatenetwork/accumulate/tools/cmd/debug]
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x68c589]

goroutine 53 [running]:
go/types.(*Named).Obj(...)
        /usr/lib/go/src/go/types/named.go:295
golang.org/x/tools/go/analysis/passes/internal/analysisutil.IsNamedType({0x7b6958?, 0x0?}, {0x73cd99, 0x8}, {0xc002718a48, 0x1, 0x10?})
        ../../golangci-lint/x-tools/go/analysis/passes/internal/analysisutil/util.go:123 +0x49
golang.org/x/tools/go/analysis/passes/httpresponse.isHTTPFuncOrMethodOnClient(0xc00051ce40, 0xc002718af0?)
        ../../golangci-lint/x-tools/go/analysis/passes/httpresponse/httpresponse.go:122 +0xfb
golang.org/x/tools/go/analysis/passes/httpresponse.run.func1({0x7b6548?, 0xc0003b3800?}, 0x18?, {0xc002d4e000, 0x5, 0x20})
        ../../golangci-lint/x-tools/go/analysis/passes/httpresponse/httpresponse.go:62 +0x73
golang.org/x/tools/go/ast/inspector.(*Inspector).WithStack(0xc0025e96b0, {0xc002718c08?, 0x9688e0?, 0xc000600008?}, 0xc002718c18)
        ../../golangci-lint/x-tools/go/ast/inspector/inspector.go:148 +0x188
golang.org/x/tools/go/analysis/passes/httpresponse.run(0xc002d3a0d0)
        ../../golangci-lint/x-tools/go/analysis/passes/httpresponse/httpresponse.go:57 +0x112
golang.org/x/tools/go/analysis/unitchecker.run.func2.1()
        ../../golangci-lint/x-tools/go/analysis/unitchecker/unitchecker.go:382 +0x8ce
sync.(*Once).doSlow(0x6edb40?, 0xc001f72ff0?)
        /usr/lib/go/src/sync/once.go:74 +0xc2
sync.(*Once).Do(...)
        /usr/lib/go/src/sync/once.go:65
golang.org/x/tools/go/analysis/unitchecker.run.func2(0x9676e0)
        ../../golangci-lint/x-tools/go/analysis/unitchecker/unitchecker.go:333 +0x19e
golang.org/x/tools/go/analysis/unitchecker.run.func3.1(0x0?)
        ../../golangci-lint/x-tools/go/analysis/unitchecker/unitchecker.go:404 +0x23
created by golang.org/x/tools/go/analysis/unitchecker.run.func3 in goroutine 1
        ../../golangci-lint/x-tools/go/analysis/unitchecker/unitchecker.go:403 +0x4

[gopherbot]

Change https://go.dev/cl/571057 mentions this issue: all: audit ReceiverNamed usage

[ldez]

A minimal reproducer

.
├── base.go
├── go.mod
├── go.sum
└── pkg
    └── json.go

base.go

package main

import (
	"net/http"

	"github.com/golangci/sandbox/pkg"
)

func Bar(_ *http.Server) {
	pkg.ChainFromJSON()
}

json.go

package pkg

func ChainFromJSON() (*[32]byte, error) {
	return nil, nil
}

$ go vet -vettool=unitchecker .
# github.com/golangci/sandbox
# [github.com/golangci/sandbox]
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x68c649]

goroutine 55 [running]:
go/types.(*Named).Obj(...)
        /usr/lib/go/src/go/types/named.go:295
golang.org/x/tools/go/analysis/passes/internal/analysisutil.IsNamedType({0x7b6978?, 0x0?}, {0x73cd9d, 0x8}, {0xc0000a2a48, 0x1, 0x10?})
        ../../../..langci-lint/x-tools/go/analysis/passes/internal/analysisutil/util.go:129 +0x49
golang.org/x/tools/go/analysis/passes/httpresponse.isHTTPFuncOrMethodOnClient(0xc0000005b0, 0xc0000a2af0?)
        ../../../..langci-lint/x-tools/go/analysis/passes/httpresponse/httpresponse.go:139 +0xff
golang.org/x/tools/go/analysis/passes/httpresponse.run.func1({0x7b6568?, 0xc0001d2e40?}, 0x18?, {0xc00003a100, 0x5, 0x8})
        ../../../..langci-lint/x-tools/go/analysis/passes/httpresponse/httpresponse.go:65 +0x6f
golang.org/x/tools/go/ast/inspector.(*Inspector).WithStack(0xc0003b04c8, {0xc0000a2c08?, 0x9688e0?, 0xc000700008?}, 0xc0000a2c18)
        ../../../..langci-lint/x-tools/go/ast/inspector/inspector.go:148 +0x188
golang.org/x/tools/go/analysis/passes/httpresponse.run(0xc0000005b0)
        ../../../..langci-lint/x-tools/go/analysis/passes/httpresponse/httpresponse.go:57 +0x112
golang.org/x/tools/go/analysis/unitchecker.run.func2.1()
        ../../../..langci-lint/x-tools/go/analysis/unitchecker/unitchecker.go:382 +0x8ce
sync.(*Once).doSlow(0x6edb40?, 0xc0003bb230?)
        /usr/lib/go/src/sync/once.go:74 +0xc2
sync.(*Once).Do(...)
        /usr/lib/go/src/sync/once.go:65
golang.org/x/tools/go/analysis/unitchecker.run.func2(0x9676e0)
        ../../../..langci-lint/x-tools/go/analysis/unitchecker/unitchecker.go:333 +0x19e
golang.org/x/tools/go/analysis/unitchecker.run.func3.1(0x0?)
        ../../../..langci-lint/x-tools/go/analysis/unitchecker/unitchecker.go:404 +0x23
created by golang.org/x/tools/go/analysis/unitchecker.run.func3 in goroutine 1
        ../../../..langci-lint/x-tools/go/analysis/unitchecker/unitchecker.go:403 +0x47

Maybe you will be able to use it inside a test.

[ldez]

It looks like a bug that comes from somewhere else because the type of the return parameter depends on the position of the function:

• if the function ChainFromJSON is inside the base.go the type is *types.Named
• if the function ChainFromJSON is inside another package the type is *types.Array

Maybe I missed something but it's an unexpected behavior.