cmd/compile: method call on interface panics, but works on the contained struct

[hubslave]

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

go version go1.10 linux/amd64

Does this issue reproduce with the latest release?

Yes.

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

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/a/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/a"
GORACE=""
GOROOT="/home/a/go"
GOTMPDIR=""
GOTOOLDIR="/home/a/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
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-build961933609=/tmp/go-build"

What did you do?

https://play.golang.org/p/TjVzZGtL8zz

package main

import (
	"bytes"
	"fmt"
	"log"
)

type mystruct struct {
	f int
}

func (t mystruct) String() string {
	return "foo"
}

func main() {
	type deep struct {
		mystruct
	}
	s := struct {
		deep
		*bytes.Buffer
	}{
		deep{},
		bytes.NewBufferString("TEST"),
	}
	log.Print(s.String()) // ok
	var i fmt.Stringer = s
	log.Print(i.String)   // ok
	log.Print(i.String()) // panic
}

What did you expect to see?

2018/03/27 03:04:27 TEST
2018/03/27 03:04:27 0x48dc70
2018/03/27 03:04:27 TEST

What did you see instead?

2018/03/27 03:04:27 TEST
2018/03/27 03:04:27 0x48dc70
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x0]

goroutine 1 [running]:
main.main()
        /tmp/test.go:31 +0x25c

Are my expectations correct? I have reread the Struct types and the Selectors paragraphs of the specs but I can't find an explanation for the panic, I hope I haven't overlooked something obvious.
It seems related to the method collision (at different depths) and some specific combination of pointers and values.

[ianlancetaylor]

That's an interesting test case. I get different results from different versions of Go.

Go 1.1 through 1.6:

2018/03/26 19:32:57 TEST
panic: interface conversion: struct { main.deep; *bytes.Buffer } is not fmt.Stringer: missing method String

goroutine 1 [running]:
main.main()
	/home/iant/foo8.go:29 +0x19f

goroutine 2 [runnable]:
exit status 2

Go 1.7 through 1.9:

panic: interface conversion: struct { main.deep; *bytes.Buffer } is not fmt.Stringer: missing method String
fatal error: panic on system stack

runtime stack:
runtime.throw(0x4bbe09, 0x15)
	/home/iant/go1.7/src/runtime/panic.go:566 +0x95 fp=0x7ffe13da9030 sp=0x7ffe13da9010
panic(0x4a31c0, 0xc42000a000)
	/home/iant/go1.7/src/runtime/panic.go:389 +0x6d2 fp=0x7ffe13da90c0 sp=0x7ffe13da9030
runtime.additab(0x511200, 0x7ffe13da0001)
	/home/iant/go1.7/src/runtime/iface.go:131 +0x443 fp=0x7ffe13da91b0 sp=0x7ffe13da90c0
runtime.itabsinit()
	/home/iant/go1.7/src/runtime/iface.go:149 +0x79 fp=0x7ffe13da91f0 sp=0x7ffe13da91b0
runtime.schedinit()
	/home/iant/go1.7/src/runtime/proc.go:444 +0x6e fp=0x7ffe13da9230 sp=0x7ffe13da91f0
runtime.rt0_go(0x7ffe13da9268, 0x1, 0x7ffe13da9268, 0x0, 0x0, 0x1, 0x7ffe13daa80a, 0x0, 0x7ffe13daa846, 0x7ffe13daa85c, ...)
	/home/iant/go1.7/src/runtime/asm_amd64.s:145 +0x14f fp=0x7ffe13da9238 sp=0x7ffe13da9230
exit status 2

Go 1.10:

2018/03/26 19:35:40 TEST
2018/03/26 19:35:40 0x48dc70
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x0]

goroutine 1 [running]:
main.main()
	/home/iant/foo8.go:31 +0x25c
exit status 2

Current tip:

# command-line-arguments
../../foo8.go:29:6: cannot use s (type struct { deep; *bytes.Buffer }) as type fmt.Stringer in assignment:
	struct { deep; *bytes.Buffer } does not implement fmt.Stringer (String method has pointer receiver)

Go 1.10 is what the playground currently uses. It does seem that every version other than 1.10 agrees that the interface conversion does not succeed. Before 1.10 it is reported at run time. On tip it is reported at compile time.

With gccgo, the program works as expected:

2018/03/26 19:37:20 TEST
2018/03/26 19:37:20 0x405380
2018/03/26 19:37:20 TEST

When I look at the type of the anonymous struct assigned to s, I see that the *bytes.Buffer has a String method, and that deep has a String method, but the depth of the *bytes.Buffer method is less, so it seems tome that calling String on the anonymous struct ought to call (*bytes.Buffer).String. I don't understand why the error says that the method has a pointer receiver, since a pointer method of an embedded field should be a value method of the outer struct.

So it seems to me that gccgo is correct, and that gc is incorrect, in different ways for different versions.

CC @griesemer @mdempsky

[mdempsky]

It looks like the problem is in expandmeth. We call expand1 to build up a list of all unique method names, and then we use dotpath to figure out which actual methods each of those names resolve to.

However, when we record whether the promoted method is a "follows-ptr" method, we followed any pointers to discover the name in the first place. We should really be using whether dotpath had to follow any pointers.

The compilation regression was caused by CL 100845 (91bbe53) because it changed interface satisfaction from using dotpath directly to using the cached results computed by expandmeth.

[zigo101]

The behaviors of gc are really incorrect.
Whether or not the behavior of gccgo is correct depends on how to explain the depths of method selectors.

By the example of the following program,

package main

type mystruct struct {
	field int
}

func (t mystruct) String() string {
	return "foo"
}

type deep struct {
	mystruct
}

func main() {
	var d deep
	_ = d.mystruct.String
	_ = d.String
	_ = d.mystruct.field
	_ = d.field
}

Is d.field a shorthand of d.mystruct.field?
I think the answer is surely yes.
The reason is when we use reflection to list the fields of type deep,
there is not a field called field, but only a field called mystruct.

Is d.String a shorthand of d.mystruct.String?
I think the answer is ambiguous.
If we use reflection to list the methods of type deep,
there is really a method called String.
Compiler will delcare the String method for type deep (and type *deep) implicitly.
Spec doesn't give a clear answer the depth of d.String should be 0 or 1.

If the depth of d.String is 0, a.k.a, d.String is not a shorthand of d.mystruct.String,
then the behaviors of both gc and gccgo are wrong,
for the selector s.String in OP's code is ambiguous,
OP's code should fail to compile.

[mdempsky]

@go101 OP's code is valid. It should behave as they described: s.String should refer to s.Buffer.String, as that has a lesser depth than s.deep.mystruct.String.

[zigo101]

Go spec is a little ambiguous here:

For a value x of type T or *T where T is not a pointer or interface type,
x.f denotes the field or method at the shallowest depth in T
where there is such an f. ...

So there is such an f exactly means there is such an f declared explicitly?

[gopherbot]

Change https://golang.org/cl/102618 mentions this issue: cmd/compile: fix method set computation for shadowed methods

[mdempsky]

So there is such an f exactly means there is such a f declared explicitly?

Correct.

[zigo101]

It looks the tip only fix the fail-to-compile problem, but not the panic problem.

[zigo101]

sorry, my mistake. It is fixed on tip now.