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: order of evaluation vs panicking #23735

Open
mdempsky opened this issue Feb 7, 2018 · 4 comments

Comments

@mdempsky
Copy link
Member

commented Feb 7, 2018

It's possible to write expressions that, depending on order of evaluation, may panic for different reasons. I think this is fine, but it might be worth mentioning or providing examples to make clearer / less surprising to users.

For example, consider:

var p *[0]int
var i int

(*p)[i] = 0

Under cmd/compile, this snippet produces an "invalid memory address or nil pointer dereference" panic; whereas under gccgo, it produces an "index out of range" panic.

--

Another example is:

var p *int
var x int

_ = *p / x

Both cmd/compile and gccgo produce a nil pointer dereference panic, but the Go spec appears to also permit division by zero.

--

The latter example is relevant to #23661, because of:

var m = map[interface{}]int{0: 0}  // non-empty map to workaround #23734
var k []int
var x int

m[k] /= x

Today, the last statement is compiled into roughly:

tmp := *mapaccess1(m, k)
if x == 0 {
    panicdivide()
}
*mapassign(m, k) = tmp / x

It would be nice to optimize that to

if x == 0 {
    panicdivide()
}
*mapassign(m, k) /= x

But that would change the panic from "hash of unhashable type []int" to "division by zero". This change appears valid under the Go spec, but it seems worth clarifying whether that's the case.

@mdempsky

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2018

@randall77

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2018

The Run-time panics section of the spec seems to imply to me that the nature of the panic is irrelevant, only whether there is one or not.
i.e. for the code:

var p *int
_ = *p

My reading of the spec would allow a panic with a "division by zero" error.
Not that we would want to do that, but no one should be programmatically depending on the error text.

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2018

I'm ok with adding clarifying examples to the spec.

@griesemer griesemer self-assigned this Feb 12, 2018

@griesemer griesemer added this to the Go1.11 milestone Feb 12, 2018

@gopherbot

This comment has been minimized.

Copy link

commented Mar 1, 2018

Change https://golang.org/cl/91557 mentions this issue: cmd/compile: avoid extra mapaccess in "m[k] op= r"

gopherbot pushed a commit that referenced this issue Mar 12, 2018
cmd/compile: avoid extra mapaccess in "m[k] op= r"
Currently, order desugars map assignment operations like

    m[k] op= r

into

    m[k] = m[k] op r

which in turn is transformed during walk into:

    tmp := *mapaccess(m, k)
    tmp = tmp op r
    *mapassign(m, k) = tmp

However, this is suboptimal, as we could instead produce just:

    *mapassign(m, k) op= r

One complication though is if "r == 0", then "m[k] /= r" and "m[k] %=
r" will panic, and they need to do so *before* calling mapassign,
otherwise we may insert a new zero-value element into the map.

It would be spec compliant to just emit the "r != 0" check before
calling mapassign (see #23735), but currently these checks aren't
generated until SSA construction. For now, it's simpler to continue
desugaring /= and %= into two map indexing operations.

Fixes #23661.

Change-Id: I46e3739d9adef10e92b46fdd78b88d5aabe68952
Reviewed-on: https://go-review.googlesource.com/91557
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>

@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Unplanned Jun 29, 2018

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