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

go/types: improve documentation on *types.Named identity #53914

Open
aykevl opened this issue Jul 16, 2022 · 4 comments
Open

go/types: improve documentation on *types.Named identity #53914

aykevl opened this issue Jul 16, 2022 · 4 comments
Labels
Documentation help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@aykevl
Copy link

aykevl commented Jul 16, 2022

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

$ go version
go version go1.19beta1 linux/amd64

Does this issue reproduce with the latest release?

It also reproduces in Go 1.18.3.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/ayke/.cache/go-build"
GOENV="/home/ayke/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/ayke/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/ayke:/home/ayke"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.18.3"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/ayke/src/github.com/tinygo-org/tinygo/go.mod"
GOWORK=""
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build898003955=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I am using golang.org/x/tools/go/ssa to build the SSA form for TinyGo.
We recently added generics support, which was surprisingly easy because the ssa package does all the hard work of instantiating generic functions.
Unfortunately, it resulted in an issue. Previously, we could rely on *types.Named to have pointer identity, meaning, that we could create a map[*types.Named]someOtherType and rely on the fact that every unique named type would have only one entry in the map.

What did you expect to see?

I expected this to remain the same while adding support for generics.

What did you see instead?

It broke. It works most of the time, but in some cases (in particular, methods on generic structs) there can be two *types.Named objects for the same named type.

Here is a reproducer:
https://go.dev/play/p/AIZZDnqqm0l?v=gotip

I've tried to look at the documentation and comments inside the package what the intended behavior is, but I see somewhat conflicting information. In any case, it would be very helpful if *types.Named would be unique again.

  1. This comment seems to suggest *types.Named is intended to have pointer identity but doesn't at the moment.
    // TODO(gri) Why is x == y not sufficient? And if it is,
    // we can just return false here because x == y
    // is caught in the very beginning of this function.
    return x.obj == y.obj
  2. types.Instantiate says that not all types may be uniqued:

    If ctxt is non-nil, it may be used to de-duplicate the instance against previous instances with the same identity. As a special case, generic *Signature origin types are only considered identical if they are pointer equivalent, so that instantiating distinct (but possibly identical) signatures will yield different instances. The use of a shared context does not guarantee that identical instances are deduplicated in all cases.

  3. The comment at the top of src/go/types/named.go says that *types.Named may not be unique:

    go/src/go/types/named.go

    Lines 74 to 81 in 2aa473c

    // Some invariants to keep in mind: each declared Named type has a single
    // corresponding object, and that object's type is the (possibly generic) Named
    // type. Declared Named types are identical if and only if their pointers are
    // identical. On the other hand, multiple instantiated Named types may be
    // identical even though their pointers are not identical. One has to use
    // Identical to compare them. For instantiated named types, their obj is a
    // synthetic placeholder that records their position of the corresponding
    // instantiation in the source (if they were constructed during type checking).

From this, it appears to be a design decision to change the uniqueness of *types.Named pointers for generic types.
This is a problem for me, as I need a way to create unique IDs per *types.Named instance (types.Identical is not a solution, because you can't use it in a map). I tried typ.Obj().Pkg().Path() + "." + typ.Obj().Name() but that doesn't work for named types inside functions (which create a new scope but may have the same name as a named type in the outer scope).

I'm reporting this as a bug as it feels like a regression to me, even though it's not technically a regression.

@dominikh
Copy link
Member

types.Identical is not a solution, because you can't use it in a map

https://pkg.go.dev/golang.org/x/tools/go/types/typeutil#Map exists.

@findleyr
Copy link
Contributor

As @dominikh suggests, the solution is to use something like typeutil.Map.

This was indeed a design decision: given that instances can be created concurrently in separate packages, the cost of guaranteeing that they are canonical was considered to be too high. We may be able to improve our guarantees in the future, but since this will never be possible without a shared context, which necessarily pins a lot of types in memory, we won't ever be able to guarantee that Named types are always canonical.

Documentation could be improved here.

@findleyr findleyr changed the title go/types: *types.Named does not have pointer identity anymore go/types: improve documentation on *types.Named identity Jul 17, 2022
@toothrot toothrot added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 19, 2022
@toothrot toothrot added this to the Backlog milestone Jul 19, 2022
@aykevl
Copy link
Author

aykevl commented Jul 30, 2022

Thanks for pointing us to typeutil.Map, that's what we use now and it solves the issue. Feel free to close it.

@ianlancetaylor
Copy link
Contributor

Thanks for following up. This issue can remain open for improving the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants