cmd/compile: wrappers for methods incorrectly shared

[randall77]

It looks like we're sometimes oversharing wrappers for methods, when the methods have the same name but different argument signatures. This is almost certainly a bug in CL 790740.

Reproducer:
go.mod:

module foo.bar

go 1.26.5

a/a.go:

package a

type W struct {
}

func (t W) M(x int) {
	println("a.W", x)
}

b/b.go:

package b

type W struct {
}

func (t W) M() {
	println("b.W")
}

main.go

package main

import (
	"foo.bar/a"
	"foo.bar/b"
)

type I interface {
	M(int)
}

type J interface {
	M()
}

var i I
var j J

type A struct {
	a.W
}
type B struct {
	b.W
}

func init() {
	i = A{}
	j = B{}
}

func main() {
	i.M(9)
	j.M()
}

In 1.27, it prints

$ go run .
a.W 9
b.W

At tip, it prints

a.W 9
a.W 0

Discovered in Google's internal test suite.

It looks like we're somehow conflating the wrappers for the two different types. I'm not sure exactly how yet.

@jakebailey

[jakebailey]

I found the bug; will have a CL shortly

[gopherbot]

Change https://go.dev/cl/826084 mentions this issue: cmd/compile: qualify shared wrapper receiver types

[prattmic]

This CL also breaks the test case at https://github.com/google/go-cmp/blob/master/cmp/internal/function/func_test.go#L40.

Their NameOf function does a bunch of post-processing on runtime.Func.Name() to (I guess) return a nicer looking name. It seems to trip on on the name symbol names. The failing case has runtime.Func.Name() == github.com/google/go-cmp/cmp/internal/function..embed.*Buffer.Write.1.0, which seems to sufficiently trip up NameOf such that it returns the empty string.

Ultimately parsing symbol names is a problem for that package and not a regression, but I do wonder if this naming scheme is what we want/intend? The double .. looks odd to me. As does *Buffer without parenthesis, or a package path (what if there is bytes.Buffer and notbytes.Buffer?). This package is never going to get a name it really likes out of this because it doesn't mention the container type, but that is a known trade-off of this CL.

Given a shared method wrapper world, I suppose what this package would want to return in this case is bytes.Buffer.Write, which is what their final test case already expects (that case still passes with this CL). They can't ever get that with the current naming scheme because it drops the package path.

It looks like https://go.dev/cl/826084 at least resolves the package path.

[jakebailey]

I think it's just the suffix that matters; I think I can just flip the order and preserve the full LinkString() as the suffix and go-cmp would still work and also this bug would be fixed.

[jakebailey]

Oh, I see what you're saying, yeah, I don't think I can satisfy everything here ☹️

[jakebailey]

The double .. looks odd to me.

Just to note it, the first dot is always present when naming things in a package, but then compiler-generated names typically have a . prefix to make sure they don't shadow other names, e.g. .inittask or .typeAssert.1 etc net archive/zip..inittask or bufio..typeAssert.1

[jakebailey]

I updated the CL to instead generate a name like bytes..embed.1.0.bytes.(*Buffer).Write which go-cmp's NameOf can parse, though is a little duplicitive.

[prattmic]

To be clear, I don't think go-cmp necessarily needs to be able to parse these symbols with no changes (symbol names don't have a compatibility guarantee), but at least in this case, it seems reasonable to me that they should be able to get the information they want somehow.