CC @timothy-king, @findleyr via owners.

This looks like a bug in the ssa package. Per https://tip.golang.org/doc/go1.17#reflect:

Checking the Type.ConvertibleTo method is no longer sufficient to guarantee that a call to Value.Convert will not panic. It may panic when converting []T to *[N]T if the slice's length is less than N.

I think the ssa package needs a similar change: an extra case in emitConv and the removal of some now-inaccurate claims that “[the Convert operation] cannot fail dynamically.”

Change https://golang.org/cl/332049 mentions this issue: go/ssa: allow conversion from slice to array pointer

Thanks, I can confirm that the current master fixes unparam. Are there any plans to release a tagged version of the golang.org/x/tools module to make automatic updates easier?

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

1. Add a SliceToArray instruction and revert the changes to Convert.
2. Merge some subset of Convert, ChangeInterface, and ChangeType into one Convert instruction and have helper functions to distinguish cases. (This will probably break users.)

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: all: add SliceToArrayPointer instruction

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:

• the panic behavior,
• the conversions are no longer Basic types, or
• the list of conversions explicitly mentioned in the documentation expanded.

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.

the conversions are no longer Basic types

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 Convert seems make the current code more complicated.

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: go/ssa/interp: handle nil slice convert to array pointer

I just came across SliceToArrayPointer today, and my first reaction was that this could have been compiled to existing instructions something like this:

return (*[n]T)(&slice[0]) // use *ssa.Convert here, since it can't fail

Of course this would need to be preceded by a bounds check that len(slice) >= n, but this neglects two edge cases:

1. If slice is nil, the operation shouldn't fail, but should return a nil array pointer. This requires an additional preceding check.
2. If n=0 and the slice is empty but non-nil, the &slice[0] operation will fail.

The take approach of adding a new operator seems reasonable.