-
Notifications
You must be signed in to change notification settings - Fork 18k
spec: order of evaluation vs panicking #23735
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
Comments
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.
My reading of the spec would allow a panic with a "division by zero" error. |
I'm ok with adding clarifying examples to the spec. |
Change https://golang.org/cl/91557 mentions this issue: |
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>
Change https://go.dev/cl/418097 mentions this issue: |
In this test, traditionally the comparison `*l == r[0]` was left as a comparison between `*l` (type `any`) and `r[0]` (type `*int`), and the rest of the compiler needed to handle mixed-typed comparisons. However, this means more complexity for wiring up explicit rtypes. To simplify rtype handling, the next CL will change unified IR to instead handle the expression as `*l == any(r[0])`. However, a consequence of this currently is that walk will now sequence the `any(r[0])` expression first, because it involves a concrete-to-interface conversion. And in turn, this means the `r[0]` panic ("index out of bounds") will take priority over the `*l` panic ("nil pointer dereference"). This is a change in user-visible semantics in some cases, but the Go spec leaves this unspecified, so it shouldn't be an issue. Note also: gccgo has the same behavior (i.e., panicking on index out of bounds, not nil pointer dereference), and cmd/compile also already has the same behavior when the interface conversion is explicit (as in the added "nil pointer dereference #3" test case). Updates #23735. Updates #32187. Change-Id: I49e5dcca85b4680f9c8780ef0013e64254d38fe5 Reviewed-on: https://go-review.googlesource.com/c/go/+/418097 Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com> Reviewed-by: David Chase <drchase@google.com>
In this test, traditionally the comparison `*l == r[0]` was left as a comparison between `*l` (type `any`) and `r[0]` (type `*int`), and the rest of the compiler needed to handle mixed-typed comparisons. However, this means more complexity for wiring up explicit rtypes. To simplify rtype handling, the next CL will change unified IR to instead handle the expression as `*l == any(r[0])`. However, a consequence of this currently is that walk will now sequence the `any(r[0])` expression first, because it involves a concrete-to-interface conversion. And in turn, this means the `r[0]` panic ("index out of bounds") will take priority over the `*l` panic ("nil pointer dereference"). This is a change in user-visible semantics in some cases, but the Go spec leaves this unspecified, so it shouldn't be an issue. Note also: gccgo has the same behavior (i.e., panicking on index out of bounds, not nil pointer dereference), and cmd/compile also already has the same behavior when the interface conversion is explicit (as in the added "nil pointer dereference golang#3" test case). Updates golang#23735. Updates golang#32187. Change-Id: I49e5dcca85b4680f9c8780ef0013e64254d38fe5 Reviewed-on: https://go-review.googlesource.com/c/go/+/418097 Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com> Reviewed-by: David Chase <drchase@google.com>
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:
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:
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:
Today, the last statement is compiled into roughly:
It would be nice to optimize that to
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.
The text was updated successfully, but these errors were encountered: