fix bounds checking and unaliasing of copyto! for PermutedDimsArray

[adienes]

there were two issues with the old method(s):

• bounds were only checked when the eltype and dimensionality matched, leading to potential OOB writes
• we did not unalias dest and src. I know there are not specific contracts that either of PermutedDimsArray nor copyto! promise w.r.t. aliasing, but by convention it seems copyto! intends to unalias its arguments. see also: Some copyto! methods don’t check for aliasing #39460
  the new method matches the pattern of copyto! fallback for AbstractArray, only replacing copyto_unaliased! with its specific _copy!

[adienes]

bump

[adienes]

found and fixed another bug on master

julia> P = PermutedDimsArray(zeros(2, 2), (2, 1));

# should copy the whole thing, but only does the first column
julia> copyto!(P, [1.0, 2.0, 3.0, 4.0])
2×2 PermutedDimsArray(::Matrix{Float64}, (2, 1)) with eltype Float64:
 1.0  0.0
 2.0  0.0

# should BoundsError, but doesn't and writes OOB
julia> copyto!(P, [1.0, 2.0, 3.0, 4.0, 5.0])

I think these are quite "serious" bugs (relative to the frequency with with people directly call copyto! on PermutedDimsArrays, which is likely not all that much in the bigger picture) so I think if this doesn't get any blocking comment within the next week or so I'd like to merge it.

[adienes]

there is another inconsistency on master that this PR does not fix

julia> src = [1 2; 3 4]
2×2 Matrix{Int64}:
 1  2
 3  4

julia> copyto!(PermutedDimsArray(zeros(3, 3), (1, 2)), src)
3×3 PermutedDimsArray(::Matrix{Float64}, (1, 2)) with eltype Float64:
 1.0  4.0  0.0
 3.0  0.0  0.0
 2.0  0.0  0.0

# WRONG
julia> copyto!(PermutedDimsArray(zeros(3, 3), (2, 1)), src)
3×3 PermutedDimsArray(::Matrix{Float64}, (2, 1)) with eltype Float64:
 1.0  2.0  0.0
 3.0  4.0  0.0
 0.0  0.0  0.0

but I think this is best left for another PR. it also touches on #45430 . I am experimenting with a more general refactor to copyto! that can hopefully fix all these things, but it's currently blocked by a few other PRs so I'd leave this one as a more narrow fix to the potential OOB writes and aliasing behavior.

[mbauman]

Suggested change

	function Base.copyto!(dest::PermutedDimsArray{<:Any,N}, src::AbstractArray{<:Any,N}) where {N}
	function Base.copyto!(dest::PermutedDimsArray, src::AbstractArray)

This is the signature you deleted; I think checkbounds should behave as we want in the dim-mismatch case, yeah?

[adienes]

the problem is that checkbounds will accept just same-length when there is a dimension mismatch

julia> P = PermutedDimsArray(zeros(2, 2), (2, 1));

julia> checkbounds(Bool, P, 1:4)
true

but _copy! will write OOB on this, see the behavior on master

julia> copyto!(P, [1.0, 2.0, 3.0, 4.0])
2×2 PermutedDimsArray(::Matrix{Float64}, (2, 1)) with eltype Float64:
 1.0  0.0
 2.0  0.0

[mbauman]

Oh I see. And that's complicated to fix, looks like?

Could this use a generic copyto! in the meantime? It seems not great to just delete a method, even if it is buggy in some situations. Will this just fall back to the generic copyto! in the meantime?

[adienes]

the vector src case will hit copyto!(dest::AbstractArray, src::AbstractArray) which forwards to copyto_unaliased!, so on this PR we have (correctly)

julia> P = PermutedDimsArray(zeros(2, 2), (2, 1));

julia> copyto!(P, [1, 2, 3, 4])
2×2 PermutedDimsArray(::Matrix{Float64}, (2, 1)) with eltype Float64:
 1.0  3.0
 2.0  4.0

so I don't think there are any cases where we hit a new MethodError. however there are some new BoundsError

julia> copyto!(P, [1 2 3 4])
ERROR: BoundsError: attempt to access 2×2 PermutedDimsArray(::Matrix{Float64}, (2, 1)) with eltype Float64 at index [1:1, 1:4]
Stacktrace:
 [1] _throw_boundserror_indices(A::PermutedDimsArray{Float64, 2, (2, 1), (2, 1), Matrix{Float64}},

however note that the behavior on master is incorrect, so the error is an improvement IMO

julia> copyto!(P, [1 2 3 4])
2×2 PermutedDimsArray(::Matrix{Float64}, (2, 1)) with eltype Float64:
 1.0  2.0
 3.0  4.0

note the reverse is true too. some things error on master that this PR fixes

julia> P = PermutedDimsArray(zeros(2, 2), (1, 2));

# PR
julia> copyto!(P, reshape(1:4, 2, 2, 1))
2×2 PermutedDimsArray(::Matrix{Float64}, (1, 2)) with eltype Float64:
 1.0  3.0
 2.0  4.0

# master
julia> copyto!(P, reshape(1:4, 2, 2, 1))
ERROR: BoundsError: attempt to access Tuple{Int64, Int64} at index [3]

... but master actually works, as long as P is NOT the identity permutation. so it's pretty inconsistent

julia> P = PermutedDimsArray(zeros(2, 2), (2, 1));

julia> copyto!(P, reshape(1:4, 2, 2, 1))
2×2 PermutedDimsArray(::Matrix{Float64}, (2, 1)) with eltype Float64:
 1.0  3.0
 2.0  4.0

I can only think of one case where master was "accidentally" correct, but we introduce a BoundsError, namely when the dimensionalities match and are > 1, the axes don't match, the eltypes don't match, and perm is the identity. example on master, but note that if we use the same shape checks as in #59025 this should error in the end

julia> P = PermutedDimsArray(zeros(2, 2), (1, 2));

# master
julia> copyto!(P, [1 2 3 4])
2×2 PermutedDimsArray(::Matrix{Float64}, (1, 2)) with eltype Float64:
 1.0  3.0
 2.0  4.0

# PR
julia> copyto!(P, Float64[1 2 3 4])
ERROR: BoundsError: attempt to access 2×2 PermutedDimsArray(::Matrix{Float64}, (1, 2)) with eltype Float64 at index [1:1, 1:4]

[adienes]

I think it will be a bit complicated to fix _copy!, or at least I'd rather not do it in this PR, until I can couple it with broader cleanups & a firmer contract about exactly which bounds / axes need to be checked and when, make things more consistent across copyto!(P, _) vs copyto!(collect(P), _), when the linear fallback is acceptable, etc.