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
Labels
Documentation FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@finnbear
Copy link

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

@griesemer @ianlancetaylor

@andybons andybons added Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 15, 2020
@andybons andybons added this to the Unplanned milestone Jun 15, 2020
@ianlancetaylor
Copy link
Contributor

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

Parent doesn't implement Nilable, *Parent does.

related: #32021 #32035

@finnbear
Copy link
Author

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
Contributor

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 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)

@golang golang locked and limited conversation to collaborators Jul 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

7 participants