go/types: include import path in type error messages (2nd edition)

[stapelberg]

What version of Go are you using (go version)?

$ go version
go version go1.15.5 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/stapelberg/.cache/go-build"
GOENV="/home/stapelberg/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/stapelberg/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/stapelberg/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/google-golang"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/google-golang/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build777997064=/tmp/go-build -gno-record-gcc-switches"

What did you do?

This is a repeat of #35895. Maybe it wasn’t fixed correctly back then, or maybe it regressed? Either way, with current Go, users still encounter the same problem.

The steps to reproduce are exactly the same. For convenience, here is a zip file with the relevant directories and files:
pkgerror.zip

When using go run demo/demo.go, I get:

# command-line-arguments
demo/demo.go:10:17: cannot use h (type *"pkgerror/nested/pkg1".Client) as type *"pkgerror/pkg1".Client in argument to third.Something

But when using go run ast/ast.go, I get:

/home/stapelberg/go/src/pkgerror/demo/demo.go:10:18: cannot use h (variable of type *pkg1.Client) as *pkg1.Client value in argument to third.Something
exit status 1

The latter message does not include the import path, only the package, and is hence very confusing especially for newcomers to the language: the two *pkg1.Client look exactly the same, so the error message looks non-sensical.

Note that some users in some environments might be exposed to error messages from go/types before they see the error message from the Go compiler because of how IDE integration works.

Could we include the import path in the error messages produced by go/types as well please?

cc @griesemer who worked on this in my first report (thanks!)

[griesemer]

1.16 if it's straigt-forward. If there's a substantial change that needs to be made, probably 1.17.

[findleyr]

@griesemer, since you assigned us both: I can look into this today to see if it's eligible for 1.16.

[gopherbot]

Change https://golang.org/cl/277213 mentions this issue: go/types: enforce unambiguous package qualification within errors

[findleyr]

Took a look at this. The specific example cited here and in #35895 was not fixed by https://golang.org/cl/209578, because that CL only addressed ambiguities in imported packages. In this example one of the ambiguous packages is not imported.

Fundamentally it's hard to know which package names are in scope for an error message until after formatting its arguments, so https://golang.org/cl/277213 contains a fix that tracks packages encountered during formatting, and does a second pass if any were ambiguously named. This felt correct to me, but is unfortunately not a narrow change. I'm leaning toward not fixing it for go1.16, but will discuss with @griesemer. Perhaps this is OK, or there is something simpler that I missed.

[stapelberg]

Thanks for the quick fix!

I think it’d be great if we could land this soon, but up to you all ultimately of course :)

[findleyr]

@griesemer and I have been discussing this, and it has sort of opened up a can of worms.

The gc compiler handles this ~correctly by keeping a global import map, so package names will be fully qualified in error strings if there are two "reachable" packages with the same name. It's not necessary for two different packages with the same name appear within the error message, packages will be fully qualified if it two packages with the same name appear anywhere in the list of reachable symbols, even if the symbols themselves are not ambiguous, and even if the ambiguity is not found in file imports. For example:
https://play.golang.org/p/NU-iRFent_A

There is an argument to be made for fully qualifying any possible ambiguity, but also for only qualifying ambiguities that actually appear within the error message. In my opinion, it is cleaner to only fully qualify package names when they are ambiguous within the context of the error message -- see for example the user report in #41044.

Additionally, for go/types as well as the future type checker being worked on in the dev.typeparams branch, it is not obvious how to implement behavior equivalent to the current compiler behavior. Whereas the compiler mutates a global map, the state that go/types operates on is either contained within the Checker, passed in via Info, or returned in the Package. We could, for example, move pkgCnt to the Package and then aggregate when importing, but this bookkeeping could easily be a source of bugs, and adds additional complexity that is only needed in the presence of errors (generally we try to optimize for the happy path).

All this to say that we're thinking about changing the qualification logic to only fully qualify symbols when they are ambiguous within the scope of the individual error message (c.f. https://golang.org/cl/279237). With this uncertainty, and with the beta being cut last week, this is not going to get fixed in 1.16, sorry 😕

[gopherbot]

Change https://golang.org/cl/313035 mentions this issue: go/types: walk all imports when determining package name ambiguity

[gopherbot]

Change https://golang.org/cl/330253 mentions this issue: go/types: in (*Checker).qualifier, mark the current package even if it is not the first