Rename or outsource iswriteable

[ToucheSir]

Through a typo, I found that Base exports iswritable (one char difference). Were we to outsource this, possible candidates include ChainRulesCore.is_inplaceable_destination (used by add!!) and ArrayInterfaceCore.ismutable.

[mcabbott]

The near-collision is unfortunate, we should at least re-name it.

The goal was to do the dead-simple minimal thing, erring on the side of safety (i.e. not writing unelss quite sure), and revisit when someone had a compelling case to do otherwise.

At least we depend on ChainRulesCore; I'm very reluctant to entangle this little package with the huge ArrayInterface. ChainRulesCore has more description of what its rules are, ArrayInterface doesn't try to explain:

julia> using ChainRulesCore, ArrayInterface, StaticArrays, ReadOnlyArrays, FillArrays

## CRC

julia> ChainRulesCore.is_inplaceable_destination(transpose(1:3))  # ok!
false

julia> ChainRulesCore.is_inplaceable_destination(Diagonal([1,2,3]))  # but you can only write some places, "an appropriate tangent"
true

julia> ChainRulesCore.is_inplaceable_destination(SA[1,2,3])  # ok
false

julia> ChainRulesCore.is_inplaceable_destination(ReadOnlyArray([1,2,3]))  # fooled, fails the wrong way
true

## AI

julia> ArrayInterface.ismutable(ReadOnlyArray([1,2,3]))  # likewise fooled
true

julia> ArrayInterface.ismutable(SA[1,2,3])  # isn't there literally a package registered to handle this?
true

julia> ArrayInterface.ismutable(Fill(1,2,3))  # issue 77 from 2020, still broken
true

[ToucheSir]

ArrayInterface's own dependencies were gutted recently, so one needs auxiliary packages like ArrayInterfaceStaticArrays for make ismutable(SA[...]) work. is_inplaceable_destination fails on ReadOnlyArray because https://github.com/JuliaDiff/ChainRulesCore.jl/blob/f9304b32adbd911a7fb6c428cddcdaacd4d3a6ed/src/accumulation.jl#L53 calls https://github.com/bkamins/ReadOnlyArrays.jl/blob/master/src/ReadOnlyArrays.jl#L66.

In an ideal world, ArrayInterface would adopt a method more like is_inplaceable_destination (which IMO makes more sense than ismutable in this case) and Array libraries/CRC would pick up on it. In the meantime, perhaps just a rename on our side?

[ToucheSir]

Also note that iswriteable itself lets a couple immutable types through:

julia> subtypes(DenseArray)
6-element Vector{Any}:
 Array
 Base.CodeUnits
 Base.Experimental.Const
 Random.UnsafeView
 SharedArrays.SharedArray
 SuiteSparse.CHOLMOD.Dense

It's unlikely we'll see Base.CodeUnits show up in a model and AFAICT nobody uses Base.Experimental.Const these days, but just goes to show how many edge cases are out there!

[mcabbott]

Sure, these internal types seem much more obscure though.

I am surprised by this failure to the wrong side. Surely the question this function is useful for is "can be sure that writing won't give an error?", so it should return false when uncertain (or if you haven't loaded some magic package). Or have some documented 3-value logic & return missing perhaps.

CRC at least documents that it's going to trust whatever parent returns. But what parent means is pretty vague. I think ReadOnlyArrays is not wrong to define it, there really is an underlying Array.

[ToucheSir]

I'd say that's what makes having an interface so important. ReadOnlyArrays is correct in defining parent, but the issue is that is_inplaceable_destination(parent(x)) is not the same semantically as is_inplaceable_destination(x). It would be better if ReadOnlyArray could override is_inplaceable_destination directly, but that requires an interface package other than CRC (which many JuliaArrays packages are wary of taking on as a dep) + downstream buy-in.

[mcabbott]

Yes. Ideally Base would have some such notion, clearly described, which everyone would extend.

I am somewhat skeptical of ArrayInterface's claims to take up this role. In part because of the complete lack of documentation about what on earth it's actually supposed to return, which side does it fail towards, what's the intended use case -- here's all you get:

help?> ArrayInterface.ismutable
  ismutable(::Type{T}) -> Bool

  Query whether instances of type T are mutable or not, see
  https://github.com/JuliaDiffEq/RecursiveArrayTools.jl/issues/19.

The link doesn't help much, and "mutable" of course already has a meaning:

help?> Base.ismutable
search: ismutable ismutabletype isimmutable

  ismutable(v) -> Bool

  Return true if and only if value v is mutable. See Mutable Composite Types for a discussion of
  immutability. Note that this function works on values, so if you give it a type, it will tell
  you that a value of DataType is mutable.

Sadly ChainRulesCore is also not immune to bloat.

[ToucheSir]

I very much agree. My thought was that ArrayInterfaceCore still seems to be the most promising host package for mostly non-technical reasons (i.e. chance of adoption). ismutable is arguably a bad fit as you've articulated, so it would either be a new function name or a revamp of an existing one like can_setindex.

[mcabbott]

I think I overheard somewhere that this one is supposed to be about whether scalar setindex! is allowed, i.e. not for GPU arrays? But its documentation is just its name, and examples are (again) not very illuminating:

help?> ArrayInterface.can_setindex
  can_setindex(::Type{T}) -> Bool

  Query whether a type can use setindex!.

julia> ArrayInterface.can_setindex(Fill(1,2))  # wrong
true

julia> using JLArrays  # reference implementation, https://github.com/JuliaGPU/GPUArrays.jl/pull/418

julia> jl([1,2,3]) isa JLArrays.AbstractGPUArray
true

julia> ArrayInterface.can_setindex(jl([1,2,3]))  # really?
true

julia> jl([1,2,3])[1] = 4
┌ Warning: Performing scalar indexing on task Task (runnable) @0x000000010b0c4010.

julia> using ArrayInterfaceGPUArrays  # maybe you need a magic package?

julia> ArrayInterface.can_setindex(jl([1,2,3]))  # nope
true

It seems sad that indeed this thing appears to be spreading, as you say for non-technical reasons.

ps. There was apparently a breaking release made over which way ismutable should fail: https://github.com/JuliaArrays/ArrayInterface.jl#breaking-release-notes From which I still can't tell what "tends to not work out in a way one would expect" means.