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

spec: struct can implement an interface with an inherited method but attempting to call that method can cause an unexpected panic #39601

Closed
finnbear opened this issue Jun 15, 2020 · 7 comments
Assignees
Milestone

Comments

@finnbear
Copy link

@finnbear finnbear commented Jun 15, 2020

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

$ go version
go version go1.14.4 linux/amd64

Does this issue reproduce with the latest release?

I believe go1.14.4 is the latest stable release, so yes.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/finnb/.cache/go-build"
GOENV="/home/finnb/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/finnb/go:/home/finnb/git/finnbear/mazean"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/snap/go/5830"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/snap/go/5830/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build362984802=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Playground link: https://play.golang.org/p/Xf3mskprGZV

package main

import (
	"fmt"
)

type Nilable interface {
	IsNil() bool
}

type Child struct {

}

func (child *Child) IsNil() bool {
	return child == nil
}

type Parent struct {
	Child
}

func main() {
	var something Nilable

	var child *Child
	something = child
	fmt.Println(something.IsNil())

	var parent *Parent
	something = parent
	fmt.Println(something.IsNil()) // Panic here
}

What did you expect to see?

true
true

What did you see instead?

true
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x4919f5]

goroutine 1 [running]:
main.(*Parent).IsNil(0x0, 0xc0000b6008)
	<autogenerated>:1 +0x5
main.main()
	/tmp/sandbox080043792/prog.go:36 +0xad

I suspect what is happening is that calling IsNil on parent acts on parent.Child, thus dereferencing Child from a nil pointer parent, but this happens silently. I think one of the following should be done:

  1. Assuming my theory is correct, the panic message should be improved to not act as if the issue occurred in the (*Child) IsNil receiver but in the intermediate step of accessing parent.Child
  2. The documentation should be updated to warn about this behavior
  3. In the unlikely event that this is a bug in Go, it should be fixed
@andybons andybons changed the title interfaces: Struct can implement an interface with an inherited method but attempting to call that method can cause an unexpected panic spec: struct can implement an interface with an inherited method but attempting to call that method can cause an unexpected panic Jun 15, 2020
@andybons
Copy link
Member

@andybons andybons commented Jun 15, 2020

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 15, 2020

This is intended behavior. Dereferencing a nil pointer in order to access an embedded field panics.

But in a quick glance at the language spec I don't see that documented anywhere, so I guess we ought to have an additional sentence in the spec somewhere.

I think the panic message is as good as it is going to get. The IsNil method is promoted to become a method of *Parent, and that is where the crash happens. There isn't an intermediate step that could appear in the stack trace or the panic message.

@finnbear
Copy link
Author

@finnbear finnbear commented Jun 16, 2020

I now realize my statement

the panic message should be improved to not act as if the issue occurred in the (*Child) IsNil receiver

was wrong, since the panic actually states the location was in main.(*Parent).IsNil and it does say <autogenerated> which is a decent hint.

An additional line in the spec/doc would be nice though.

Also, how is it even possible that Parent *Parent can implement Nilable if it contains Child not *Child and only *Child has IsNil? I'm guessing that the receiver acts like a pointer receiver but there is no actual *Child?

@dotaheor
Copy link

@dotaheor dotaheor commented Jun 16, 2020

Parent doesn't implement Nilable, *Parent does.

related: #32021 #32035

@finnbear
Copy link
Author

@finnbear finnbear commented Jun 16, 2020

Parent doesn't implement Nilable, *Parent does.

I misspoke, my question was directed at why *Parent can implement Nilable. I have to do more research into whether *Parent has an internal/pseudo Child (like Parent does), *Child (which would be weird, I think), or something else...

@bcmills
Copy link
Member

@bcmills bcmills commented Jun 16, 2020

I believe this is exactly the case described in #18617.
(Note that that issue is a proposal for a backward-incompatible change to the language.)

@dotaheor
Copy link

@dotaheor dotaheor commented Jun 16, 2020

@finnbear
By the current rules,

  • type struct{T} gets all the methods of type T,
  • type *struct{T}, type struct{*T}, and type *struct{*T} get all the methods of type *T (as the method set of *T is always a super-set of T, so the three types also get all the methods of T)
@finnbear finnbear closed this Jul 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.