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

cmd/compile: short-circuiting interface-to-concrete comparisons misses panics #32187

Open
skinass opened this issue May 22, 2019 · 6 comments

Comments

Projects
None yet
5 participants
@skinass
Copy link

commented May 22, 2019

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

$ go version
go version go1.11.4 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/a.sulaev/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/a.sulaev/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.11.4/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.11.4/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/wb/q3d56j697hs6zfvjgt3n88bc0000gp/T/go-build625061947=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Here is the code + play.golang.org link on it :

package main

import (
	"errors"
)

type SomeType struct{}

func (SomeType) Error() string {
	return ""
}

func main() {
	var x error
	x = errors.New("aa")
	switch x {
	case x.(*SomeType):
		println("*SomeType")
	case x:
		println("x")
	}

	// println(x.(*SomeType))
}

What did you expect to see?

I was expecting to see panic at the line case x.(*SomeType): because of the bad type casting.

For example if i uncomment the line // println(x.(*SomeType)) (which casts x the same way as in case) i get the panic

What did you see instead?

This code successfully compiles and prints "x" when running.

@skinass

This comment has been minimized.

Copy link
Author

commented May 22, 2019

UPD:

package main

import (
	"errors"
)

type SomeType struct{}

func (SomeType) Error() string {
	return ""
}

func main() {
	var x error
	x = errors.New("aa")
	switch x {
	case x.(SomeType):
		println("SomeType")
	case x:
		println("x")
	}
}

if i cast to x.(SomeType) in place of x.(*SomeType) i get the panic

@randall77

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

Definitely weird. That should panic.

I suspect something in the compiler is getting mixed up between expression switches (which this is) and type switches.

Is this derived from some real code? It looks bizarre to me. The reason I ask is because it affects priority - if this happens in the real world we should probably get a fix in for 1.13; if not it can wait for 1.14.

Go gets this right up to and including 1.4. The bug starts in 1.5.
gccgo gets this right.

@skinass

This comment has been minimized.

Copy link
Author

commented May 22, 2019

Definitely weird. That should panic.

I suspect something in the compiler is getting mixed up between expression switches (which this is) and type switches.

Is this derived from some real code? It looks bizarre to me. The reason I ask is because it affects priority - if this happens in the real world we should probably get a fix in for 1.13; if not it can wait for 1.14.

Go gets this right up to and including 1.4. The bug starts in 1.5.
gccgo gets this right.

I got something similar while reviewing a pull request where the code works fine.
And i started wondering why this doesn't panic. I dont think we are going to use this "trick" in production, but it might happen to someone else.

@randall77

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

It just seems weird to compare an interface against a concrete type that's type-casted from an interface. You might as well compare them directly. In other words, if x and y are interfaces, then x == y.(T) is the same as x == y, except that the former would panic if y was not a T.

@bcmills bcmills changed the title Tricky type cast inside switch/case block cmd/compile: invalid type assertion in switch case fails to panic May 22, 2019

@bcmills bcmills added this to the Go1.14 milestone May 22, 2019

@mdempsky

This comment has been minimized.

Copy link
Member

commented May 23, 2019

Simpler repros:

package main

func main() {
    var x interface{}
    println(x == x.(*int)) // should panic, but prints false

    var p *int
    println(x == *p)  // should panic, but prints false

    var s []string
    println(x == s[10])  // should panic, but prints false
}

The problem is for interface-to-concrete comparisons, we're short circuiting on the interface value's dynamic type before evaluating the concrete expression for side effects.

The problem doesn't happen for x.(SomeType) because order.go calls copyExpr on non-direct interface types (i.e., non-pointer types):

case ODOTTYPE, ODOTTYPE2:
n.Left = o.expr(n.Left, nil)
// TODO(rsc): The isfat is for consistency with componentgen and walkexpr.
// It needs to be removed in all three places.
// That would allow inlining x.(struct{*int}) the same as x.(*int).
if !isdirectiface(n.Type) || isfat(n.Type) || instrumenting {
n = o.copyExpr(n, n.Type, true)
}

@mdempsky mdempsky changed the title cmd/compile: invalid type assertion in switch case fails to panic cmd/compile: short-circuiting interface-to-concrete comparisons misses panics May 23, 2019

@gopherbot

This comment has been minimized.

Copy link

commented May 24, 2019

Change https://golang.org/cl/178817 mentions this issue: cmd/compile: ensure interface-to-concrete comparison panics when it should

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.