1.2: SparseArrays commits type piracy in override of copy

[dlfivefifty]

The following is an instance of type-piracy:

julia/stdlib/SparseArrays/src/sparseconvert.jl

Line 80 in 7b18a30

copy(A::SubArray{T,2}) where T = getindex(unwrap(parent(A)), A.indices...)

It was introduced by @KlausC in 7a75d6b

[dlfivefifty]

Note it is impacting BlockBandedMatrices.jl Julia v1.2 compatibility, though this can be fixed by modifying unwrap or copy. But there is definitely wrong with the design here I think beyond type piracy: choosing the matrix type should be done in an extendable way, not via iswrsparse(A) check.

[KlausC]

The following is an instance of type-piracy

julia/stdlib/SparseArrays/src/sparseconvert.jl

Line 80 in 7b18a30

copy(A::SubArray{T,2}) where T = getindex(unwrap(parent(A)), A.indices...)

Agree, there is an unfortunate collateral damage. This variant of copy should only affect SparseMatrixCSC and wrapped derivatives. A method with the used signature should be defined in Base, to avoid type piracy, if at all.

A fixed definition of copy could look like:

function copy(A::SubArray{T,2,P}) where {T,P}
    if iswrsparse(P)
        getindex(unwrap(parent(A)), A.indices...)
   else
        invoke(copy, Tuple{AbstractArray}, A)
    end
end

It falls back to the AbstractArray case for all types, which are not targeted by this PR.

But there is definitely wrong with the design here I think beyond type piracy: choosing the matrix type should be done in an extendable way, not via iswrsparse(A) check.

You are referring to the definition of unwrap, I think:

julia/stdlib/SparseArrays/src/sparseconvert.jl

Line 77 in 7b18a30

unwrap(A::AbstractMatrix) = iswrsparse(A) ? convert(SparseMatrixCSC, A) : convert(Array, A)

In spite of the unconventional style, the use of iswrsparse has no runtime effect, due to the smart compiler, evaluating this type-dependant function at compile time, however deeply the wrappers are nested.
If we could code similar to

    unwrap(A::Any) = A
    unwrap(A::AbstractWrappedMatrix{T,SparseMatrixCSC}) where T = convert(SparseMatrixCSC, A)

that would obsolete the current solution. (See PR #31563, triage for 1.3 or later).

[dlfivefifty]

The type restricted unwrap is less worrying so I think your proposed change is fine

[KlausC]

@dlfivefifty, you mean the change for copy? The above proposal for unwrap is only hypothetical, because AbstractWrappedMatrix is what I would like to have, but does not exist.

I would also like to see your case, where the change of copy breaks your code.

[dlfivefifty]

https://travis-ci.org/JuliaMatrices/BlockBandedMatrices.jl/jobs/540770283

It’s because BlockArrays has custom indexing types BlockSlice which isn’t playing well with the change

[KlausC]

MethodError: no method matching getindex(::Nothing, ::BlockArrays.BlockSlice{Block{1,Int64}}, ::BlockArrays.BlockSlice{Block{1,Int64}})
  Stacktrace:
   [1] copy(::SubArray{Float64,2,BlockSkylineMatrix{Float64,Array{Float64,1},BlockSkylineSizes{BlockSizes{2,Tuple{Array{Int64,1},Array{Int64,1}}},Fill{Int64,1,Tuple{Base.OneTo{Int64}}},Fill{Int64,1,Tuple{Base.OneTo{Int64}}},BandedMatrix{Int64,Array{Int64,2},Base.OneTo{Int64}},Array{Int64,1}}},Tuple{BlockArrays.BlockSlice{Block{1,Int64}},BlockArrays.BlockSlice{Block{1,Int64}}},false}) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.3/SparseArrays/src/sparseconvert.jl:80

This stack seems to indicate, that Array(::BlockSkylineMatrix{Float64,Array{Float64,1}) = nothing, which is against the expectation, that Array(::AbstractArray{T,N}) returns an Array{T,N}.

[mbauman]

Regardless of the failure mode in BlockBandedMatrices, this method needs to be further restricted for 1.2. Even if every matrix were convertable to Array, it's quite unnecessary to do so to copy a view of it.

I know it's unsatisfactory, but the temporary fix here is likely to just use a union of the most common cases. It won't be able to catch everything, but that's ok for now.

[JeffBezanson]

If #32249 is enough to fix the most urgent breakage, triage thinks we should merge that while we keep thinking about the design here.

[KristofferC]

Fixed by #32266.