Should `unsafe_store!(ptr, x)` implicitly convert `x` to the pointer eltype?

[Tokazama]

@Seelengrab pointed out here that unsafe_store!s implicit conversion of the value passed to it may be somewhat unintuitive given we typically require any "unsafe" method to provide the proper arguments as is. Furthermore, it seems to provide some trouble for Jet.jl.

[Seelengrab]

Moving my comment about JET from the PR here as well:

My reasoning is this - JET can't find a broken convert, unless you turn on :sound mode, which can often give false positives IIRC:

julia> struct MyInt
           a::Int
       end

# oops, broken `convert`
julia> Base.convert(::Type{Int}, a::MyInt) = a

julia> a = [2]
1-element Vector{Int64}:
 2

julia> f(a) = unsafe_store!(pointer(a), MyInt(1))
f (generic function with 1 method)

# no worries, we're fine!
julia> JET.@report_call f(a)
No errors detected

# oops, we're not..
julia> f(a)
ERROR: TypeError: in pointerset, expected Int64, got a value of type MyInt
Stacktrace:
 [1] unsafe_store!(p::Ptr{Int64}, x::MyInt) (repeats 2 times)
   @ Base ./pointer.jl:126 [inlined]
 [2] f(a::Vector{Int64})
   @ Main ./REPL[10]:1
 [3] top-level scope
   @ REPL[12]:1

julia> JET.@report_call mode=:sound f(a)
═════ 1 possible error found ═════
┌ @ REPL[10]:1 unsafe_store!(pointer(a), MyInt(1))
│┌ @ pointer.jl:126 #self#(p, x, 1)
││┌ @ pointer.jl:126 Base.pointerset(p, convert(T, x), Base.Int(i), 1)
│││ this builtin function call may throw: Base.pointerset(p::Ptr{Int64}, convert(T::Type{Int64}, x::MyInt)::MyInt, Base.Int(i::Int64)::Int64, 1)
││└──────────────────

The error reported in :sound mode may be one of the others thrown by the intrinsic after all. If this type equality were required before calling the intrinsic, these kinds of mistakes would be found by JET without having to make sure the whole codebase (or at least the call people are actually interested in) is :sound 🤷

As far as I'm aware, the fact that we convert is undocumented. My guess is that the intention is to make unsafe_store!(::Ptr{Int64}, ::UInt8) and similar easier, but I'd argue that's just a buggy piece of code that's hard to debug.

[vtjnash]

this would be breaking

[Seelengrab]

Breaking in what sense? Is the conversion no longer implicit, and instead explicitly documented? There is also the breaking label for breaking stuff that can't be done now.