Aliasing problem with `SparseMatrixCSC` and `SparseVector`

[KlausC]

In v1.2.0 and v1.5.0 it is easy to create a corrupted sparse object unintentionally:

julia> B = sparse([1 1;0 0]);
julia> A = SparseMatrixCSC{Float64,Int}(B);
julia> A[2,1] = 1
julia> A
2×2 SparseMatrixCSC{Float64,Int64} with 3 stored entries:
  [1, 1]  =  1.0
  [2, 1]  =  1.0
  [1, 2]  =  1.0
julia> B
2×2 SparseMatrixCSC{Int64,Int64} with 3 stored entries:
  [1, 1]  =  1
  [2, 1]  =  1
  [1, 2]  =  #undef

julia> b = sparse([1;0]);
julia> a = SparseVector{Float64,Int}(b);
julia> a[2] = 1;
julia> a
2-element SparseVector{Float64,Int64} with 2 stored entries:
  [1]  =  1.0
  [2]  =  1.0
julia> b
2-element SparseVector{Int64,Int64} with 1 stored entry:
  [1]  =  1
  [2]  =  #undef

Reason seems to be that the constructors SparseMatrixCSC{Tv,Ti}(S::SparseMatrixCSC) and SparseVector{Tv,Ti}(s:SparseVector)
may produce objects, which share some but not all of their fields with the source objects.
That is for example the case, if Ti == eltype(S.rowval) and Tv != eltype(S.nzval). That results in inconsistent objects when one of them is modified. For example:

julia> dump(b)
SparseVector{Int64,Int64}
  n: Int64 2
  nzind: Array{Int64}((2,)) [1, 2]
  nzval: Array{Int64}((1,)) [1]

where the sizes of the component vectors of b diverge.
IMO the constructors should be changed to not use convert, but make copies.

The issue surfaced in v1.5 (but not in v1.2) when doing a calculation like follows, where the
corruption of B was rather unexpected:

julia> B = Symmetric(sparse([1 1;0 0]));
julia> B \ ones(2);
ERROR: MethodError: no method matching lu! ...

julia> B
2×2 Symmetric{Int64,SparseMatrixCSC{Int64,Int64}}:
   1     #undef
 #undef    0

[ViralBShah]

The original idea was that end-users are not supposed to use the SparseMatrixCSC constructor directly, and it is meant to aid developers. Maybe we should document it better.

[KristofferC]

Yes, these constructors are not documented are not user facing (cf #31724 (comment) and discussion)

This is why I didn't really understand #31724 (comment) because SparseMatrixCSC is already the non-user facing "expert" version, but now we have different degree of expertness with the difference between the SparseMatrixCSC constructor and SparseMatrixCSC{Tv, Ti} constructor.

[KlausC]

If even the authors of LinearAlgebra are not aware of the consequences of their doing (see the described way the issue was detected), and therefore are no "experts", I don't know who else.
Nevertheless, if the issue shouldn't be against the sparse constructors, it must be raised against convert(AbstractArray,) or something in the call stack up to A \ b, which was user-facing in the first place. I just wanted to reveal the root cause of this.

[KlausC]

that end-users are not supposed to use the SparseMatrixCSC constructor directly

that may be true. But they are supposed to use convert(SparseMatrixCSC{Float64,Int}, B) according to this logic, because the function convert is documented and the type SparseMatrixCSC{Tv,Ti}, too.

If I replace the constructor call with this call to convert, I yield exactly the same behaviour.
Does that mean, we have to repair convert and not the constructor?

[ViralBShah]

Right, if you are not messing around with the constructors directly, you should get "good behaviour". I guess this all stems from the treatment of stored zeros, which at first glance appears to not be consistent.

[KlausC]

I don't see the relationship to the storage of zeros. This issue appears much more serious to me.

EDIT:
The problem is, that A.colptr === B.colptr, but A.nzval !== B.nzval. The assignment A[2,1] = 1 changes A.colptr and A.nzval. A is conistent.
By the aliasing B.colptr and B.rowval change implicitly, and B.nzval remains unchanged. So the indices stored in B.colptr refer to positions in B.nzval, which do not exist. B is corrupted.

[ViralBShah]

Thanks for the analysis. Is the aliasing happening in a call to convert? I agree the issue is serious and should be fixed and ideally also backported to 1.0.

[KlausC]

The aliasing happens in the constructors SparseMatrixCSC{Tv,Ti}(::SparseMatrixCSC) and SparseVector{Tv,Ti}(::SparseVector), which @KristofferC is not willing to change with that "is not public interface because not documented" argument. The public convert simply calls the constructor.

[ViralBShah]

Would making a copy in convert before calling the constructor do the trick?

[KlausC]

It would solve the issue, but at cost of inefficiency.
I would prefer to change the constructors, if that is not desirable, to call new internal functions, which are creating the sparse objects in a safe way.
Under no circumstances copying should be done for all types, but only in specialized versions of convert for sparse arrays.

[KristofferC]

I do think it makes sense for SparseMatrixCSC{Float64,Int}(B); to copy all internal vectors. That is similar to what we do for Array:

julia/base/array.jl

Lines 539 to 541 in 3720edf

	# constructors should make copies
	Array{T,N}(x::AbstractArray{S,N}) where {T,N,S} = copyto!(Array{T,N}(undef, size(x)), x)
	AbstractArray{T,N}(A::AbstractArray{S,N}) where {T,N,S} = copyto!(similar(A,T), A)