-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
x/tools/go/ssa: panic with Go 1.17 slice-to-array conversions #46987
Comments
CC @timothy-king, @findleyr via owners. |
This looks like a bug in the
I think the |
Change https://golang.org/cl/332049 mentions this issue: |
Thanks, I can confirm that the current |
[Apologies for the late reply and re-opening a closed issue.] The current solution is definitely a short-term improvement and definitely fixed a bug. I am not sure this is a good long term solution for ssa though. Having an ssa.Convert that can panic feels inconsistent with other parts of the ssa IR. The ssa IR distinguishes Go conversions into Convert, ChangeInterface, MakeInterface, and ChangeType. The slice to array conversion is the most similar to ChangeInterface IMO as this can panic. ChangeInterface got its own instruction type. Some possible alternatives for ssa to encode this change:
FWIW the comment on Convert is now inaccurate after 332049 "One or both of those types is basic (but possibly named)." Slices and arrays are not basic. We can update this comment to be correct if we want to keep this change. @bcmills I don't see how "[the Convert operation] cannot fail dynamically” was inaccurate. The ssa IR is not the same as conversion operators in Go. (It is inaccurate after 332049 though.) |
@timothy-king The doc of ChangeInterface claims that the operation cannot fails, though. |
@cuonglm That is a really good point. I think I was confused between ChangeInterface and TypeAssert's interface case. I feel like this makes having Convert maybe panic in one of its cases even more of an outlier. |
Change https://golang.org/cl/333749 mentions this issue: |
My first impression is that adding a new instruction will be a lot more disruptive to existing users of go/ssa than changing the dynamic behavior of the Convert instruction. Adding a new instruction will require most clients to be updated to handle the new instruction, whereas only few clients will care that Convert can now panic – and the clients that do care can inspect the types involved. Note that my argument is purely based on the expected impact on users, not what's theoretically a cleaner abstraction. |
I am happy to factor the expected impact on users into this decision. For example, I am not supportive of deleting existing instructions to get a cast instruction that merges these cases. Adding a new instruction definitely requires more boilerplate and we should take this into account. A concern with combining the slice-to-array-pointer into Convert is that people out there will have code that has violated assumptions. Off the top of head this can happen either because of:
A data point is that https://golang.org/cl/333749 uncovered at least 1 instance of the 3rd case in go/ssa/interp/ops.go . That code would have silently failed until somebody ran it on code containing one of these casts. OTOH adding a new instruction would mean that ssa users would now have an unmatched case in type switches. If they want to support the new operator they need to hunt down these spots. This seems somewhat reasonable if you want to support a new construct added in Go 1.17. Not the best, but maybe better? So I don't think the solutions proposed so far won't require some action for ssa users to support it. Happy to hear from users if they have a clear preference in either direction. |
I don't have strong opinions on this. Both solutions have their pros and cons. Changing the behavior of Convert has the benefit that more code will keep working without change, but may introduce subtle bugs. Adding a new instruction means that ~all code needs to be updated to support the new instruction, but the change in behavior will be obvious, and it's the overall cleaner change.
is an interesting, subtle point, and something that hasn't been changed in Convert's documentation in the first CL. The documentation still claims that at least one of the types is basic. To be perfectly honest, I also mixed up Convert and ChangeType, and thinking more about it, slice to array conversion seems to sit somewhere between the two; another point in favour of introducing a new instruction. |
@timothy-king @dominikh I'm leaning to add new instruction, as now I realize that mixing slice to array pointer in For reference, the compiler SSA backend also add new op for this conversion. |
It has been a few days without more input. The conversation seems to be heading towards adding a SliceToArrayPointer instruction. Please speak up in the next couple of days if you would prefer a different solution. |
Adding a new instruction seems reasonable to me. (Caveat: I don't use x/tools/go/ssa much.) |
Looks like there are no [dissenting] updates here. I +2'd the change. #47326 includes a list of golang.org packages that may need to be updated. |
Change https://golang.org/cl/336489 mentions this issue: |
Converting from nil slice to a zero length array pointer must be nil. Updates golang/go#46987 Change-Id: I8894b92bd85fae8ea77bf01b92ee56f1a215a75b Reviewed-on: https://go-review.googlesource.com/c/tools/+/336489 Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com> Trust: Tim King <taking@google.com> Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com> Run-TryBot: Tim King <taking@google.com> gopls-CI: kokoro <noreply+kokoro@google.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Tim King <taking@google.com> Reviewed-by: Bryan C. Mills <bcmills@google.com>
I just came across SliceToArrayPointer today, and my first reaction was that this could have been compiled to existing instructions something like this:
Of course this would need to be preceded by a bounds check that
The take approach of adding a new operator seems reasonable. |
I'm not sure if this is a bug or not, considering that Go 1.17 is not out yet, but then again, Go 1.17 Beta 1 is already out. The original issue is mvdan/unparam#55. Using
golang.org/x/tools
with the latestmaster
version (v0.1.5-0.20210629191230-72e4d1bb8d47
),ssa.(*Package).Build
seems to panic with:Full panic message:
I've tested it using two analysers that use SSA,
unparam
andnilness
, and both are panicking.The text was updated successfully, but these errors were encountered: