go/types: improve assertion when PkgName is found in Package.Scope

[etnz]

Go version

go version 1.24

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/etnz/.cache/go-build'
GOENV='/home/etnz/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/etnz/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/etnz/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.23.5'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/home/etnz/.config/go/telemetry'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/etnz/picsou/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-build2593118196=/tmp/go-build -gno-record-gcc-switches'

What did you do?

Here is a way to reproduce the bug:

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

go/types.Eval function panics with "unreachable" instead of an error.
You can switch the bool "bug" to false to see the "expected" normal error.

It is allowed in Go to import a package with an exported name (see https://go.dev/play/p/ruGWMm9SRLI )

When reproducing this schema with go sources it doesn't panic: https://go.dev/play/p/T22H349IOYc

What did you see happen?

Code is panicking.

What did you expect to see?

an error should have been returned.

[gabyhelp]

Related Issues

• go/types: panic: unreachable #17716 (closed)
• types2, go/types: panic when using reflect.TypeOf in unexported struct #48351 (closed)
• go/types: recursive type aliases might cause a panic #71284 (closed)
• x/tools/go/packages: panic loading with LoadTypes #31163 (closed)
• go.tools/go.types: goto _ crashes type checker #7537 (closed)
• go.tools/go/types: crash in type checker #8518 (closed)
• go/types: assertion failure in expr.go:978 (Checker.rawExpr) #20837 (closed)
• cmd/compile: unreachable panic with GODEBUG=gotypesalias=1 #65778 (closed)
• go/types: panic: invalid package name "_" #20231 (closed)
• cmd/go: panic with empty import #61224 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[adonovan]

Although the wording of the assertion could be improved (e.g. by adding a case for PkgName, or printing the type of exp), I think the logic is sound. The problem is in these lines of your program:

func importPackage(pack *types.Package, name string, child *types.Package) {
	pkgName := types.NewPkgName(token.NoPos, pack, name, child)
	pack.Scope().Insert(pkgName)
}

A PkgName object should never appear at package scope. PkgName objects are created only by import declarations, and they create objects in the file scope. Any packages created by go/types (from syntax) or go/importer (from serialized packages) will preserve this invariant.

I'm curious: why are you creating entire packages and their objects using the go/types New* API? I'm skeptical that much good can come from it as the invariants are just too complex. This API exists primarily to make it possible to write an importer in a separate package. Aside from NewVar and NewFunc, which are needed to construct Signature and Interface types, the others are almost never needed even in applications as complex as gopls. (The few exceptions where gopls creates a Package--other than for type-checking syntax--are all local tactical hacks where we need to call some existing function that needs a Package, but they are not really first-class packages.)

[etnz]

Hi Alan,

thanks for the insights.

What I am really trying to do is to allow imported constants in types.Eval. So that the following work

types.Eval(token.NewFileSet(), pack, token.NoPos, "foo.A+foo.B")

Where foo.A and foo.B are defined in another package "foo".

Like this: https://go.dev/play/p/_V7qZ0KW6mU

( see https://github.com/etnz/calc/ for the full small project, that I stripped out to reproduce the bug )

I wonder if there is a more normal/safer way to import packages so that "foo.A + foo.B" works?

[adonovan]

I wonder if there is a more normal/safer way to import packages so that "foo.A + foo.B" works?

What you're doing seems reasonable, but unfortunately go/types doesn't yet provide quite the necessary API functions for this kind of evaluation of constructed fragments, useful though it would be. types.Eval and types.CheckExpr are quite limited in what they can express. In the past we have taken the approach of wrapping an expression E in a file, such as package p; import(...); var _ = E, which gives you control over the imported environment, but one downside is that the line/column numbers don't reflect E. Another issue is that there's no one-size-fits-all way to type-check EXPR: if it's a large constant such as 1<<100, you need to use const _ = E; if it's a statement S, you need func _() { S }, and so on. There's definitely room for API improvement.

[etnz]

It seems that the Checker is actually handling and expecting PkgName so this seem to be a correct approach:

go/src/go/types/call.go

Line 689 in 644b984

if pname, _ := obj.(*PkgName); pname != nil {

So the bug is probably here:

go/src/go/types/call.go

Line 729 in 644b984

if !exp.Exported() {

When the lookup finds an exported pkgName it is wrongly returned.
Fix could be to add a special case in this function, or to make Exported method of PkgName type to always return false.

The latter seems cleaner, but might have more side effects.

I'll be happy to implement the fix if it is agreed on.

WDYT?