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

Jump to bottom

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

Closed
stapelberg opened this issue Dec 10, 2020 · 8 comments
Closed

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

stapelberg opened this issue Dec 10, 2020 · 8 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.17

Comments

@stapelberg
Copy link
Contributor

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 griesemer added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 10, 2020
@griesemer griesemer modified the milestones: Go1.17, Go1.16 Dec 10, 2020
@griesemer
Copy link
Contributor

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

@griesemer griesemer self-assigned this Dec 10, 2020
@findleyr
Copy link
Contributor

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

@gopherbot
Copy link

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

@findleyr
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

@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 😕

@findleyr findleyr modified the milestones: Go1.16, Go1.17 Dec 22, 2020
@findleyr findleyr mentioned this issue Mar 10, 2021
Open
@gopherbot
Copy link

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

@gopherbot
Copy link

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

@golang golang locked and limited conversation to collaborators Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Go1.17
Development

No branches or pull requests
4 participants
@stapelberg @griesemer @gopherbot @findleyr