Allow SubArray dest

src/array.jl

@@ -389,11 +389,12 @@ end
 # Methods preserving levels and more efficient than AbstractArray fallbacks
 copy(A::CategoricalArray) = deepcopy(A)
 
-function copy!(dest::CategoricalArray{T, N}, dstart::Integer,
-               src::S, sstart::Integer,
-               n::Integer=length(src)-sstart+1) where
-               {T, N, S <: Union{CategoricalArray{<:Any, N},
-                                 SubArray{<:Any, N, <:CategoricalArray}}}
+CatArrOrSub{T, N} = Union{CategoricalArray{T, N},
+                          SubArray{<:Any, N, <:CategoricalArray{T}}} where {T, N}
+
+function copy!(dest::CatArrOrSub{T, N}, dstart::Integer,
+               src::CatArrOrSub{<:Any, N}, sstart::Integer,
+               n::Integer=length(src)-sstart+1) where {T, N}
     destinds, srcinds = linearindices(dest), linearindices(src)
     (dstart ∈ destinds && dstart+n-1 ∈ destinds) || throw(BoundsError(dest, dstart:dstart+n-1))
     (sstart ∈ srcinds  && sstart+n-1 ∈ srcinds)  || throw(BoundsError(src,  sstart:sstart+n-1))
@@ -402,36 +403,40 @@ function copy!(dest::CategoricalArray{T, N}, dstart::Integer,
 
     drefs = refs(dest)
     srefs = refs(src)
+    dpool = pool(dest)
     spool = pool(src)
 
     # try converting src to dest type to avoid partial copy corruption of dest
     # in the event that the src cannot be copied into dest
-    convert(Vector{T}, levels(src))
+    slevs = convert(Vector{T}, levels(src))
     if eltype(src) >: Null && !(eltype(dest) >: Null) && !all(x -> x > 0, srefs)
         throw(NullException())
     end
 
-    newlevels, ordered = mergelevels(isordered(dest), levels(dest), levels(src))
+    newlevels, ordered = mergelevels(isordered(dest), levels(dest), slevs)
     # Orderedness cannot be preserved if the source was unordered and new levels
     # need to be added: new comparisons would only be based on the source's order
     # (this is consistent with what happens when adding a new level via setindex!)
     ordered &= isordered(src) | (length(newlevels) == length(levels(dest)))
-    ordered!(dest, ordered)
+    if ordered != isordered(dest)
+        isa(dest, SubArray) && throw(ArgumentError("cannot set ordered=$ordered on dest SubArray as it would affect the parent. Found when trying to set levels to $newlevels."))
+        ordered!(dest, ordered)
+    end
 
     # Simple case: replace all values
-    if dstart == dstart == 1 && n == length(dest) == length(src)
+    if !isa(dest, SubArray) && dstart == dstart == 1 && n == length(dest) == length(src)
         # Set index to reflect refs
-        levels!(dest.pool, T[]) # Needed in case src and dest share some levels
-        levels!(dest.pool, index(spool))
+        levels!(dpool, T[]) # Needed in case src and dest share some levels
+        levels!(dpool, index(spool))
 
         # Set final levels in their visible order
-        levels!(dest.pool, newlevels)
+        levels!(dpool, newlevels)
 
         copy!(drefs, srefs)
     else # More work to do: preserve some values (and therefore index)
-        levels!(dest.pool, newlevels)
+        levels!(dpool, newlevels)
 
-        indexmap = indexin(index(spool), index(dest.pool))
+        indexmap = indexin(index(spool), index(dpool))
 
         @inbounds for i = 0:(n-1)
             x = srefs[sstart+i]
@@ -443,12 +448,10 @@ function copy!(dest::CategoricalArray{T, N}, dstart::Integer,
     dest
 end
 
-CatArrSrc = Union{CategoricalArray, SubArray{<:Any, <:Any, <:CategoricalArray}}
-
-copy!(dest::CategoricalArray, src::CatArrSrc) =
+copy!(dest::CatArrOrSub, src::CatArrOrSub) =
     copy!(dest, 1, src, 1, length(src))
 
-copy!(dest::CategoricalArray, dstart::Integer, src::CatArrSrc) =
+copy!(dest::CatArrOrSub, dstart::Integer, src::CatArrOrSub) =
     copy!(dest, dstart, src, 1, length(src))
 
 """

src/subarray.jl

@@ -2,6 +2,9 @@
 
 Nulls.levels(sa::SubArray{T,N,P}) where {T,N,P<:CategoricalArray} = levels(parent(sa))
 isordered(sa::SubArray{T,N,P}) where {T,N,P<:CategoricalArray} = isordered(parent(sa))
+# This method cannot support nullok=true since that would modify the parent
+levels!(sa::SubArray{T,N,P}, newlevels::Vector) where {T,N,P<:CategoricalArray} =
+    levels!(parent(sa), levels)
 
 function unique(sa::SubArray{T,N,P}) where {T,N,P<:CategoricalArray}
     A = parent(sa)

test/13_arraycommon.jl

@@ -277,6 +277,44 @@ end
             @test levels(dest) == levels(src)
         end
 
+        @testset "copy a src into viewed dest" begin
+            v = ["a", "b"]
+            src = levels!(CategoricalVector(v), reverse(v))
+            dest = CategoricalVector{String}(["e", "f", "g"])
+            vdest = view(dest, 1:2)
+            copy!(vdest, src)
+            @test dest[1:2] == src[1:2]
+            @test levels(dest) == levels(vdest) == ["e", "f", "g", "b", "a"]
+
+            dest = CategoricalVector{String}(["e", "f"])
+            vdest = view(dest, 1:2)
+            copy!(vdest, src)
+            @test vdest == src[1:2]
+            @test levels(dest) == levels(vdest) == ["e", "f", "b", "a"]
+        end
+
+        @testset "copy a src into viewed dest and breaking orderedness" begin
+            v = ["a", "b"]
+            src = levels!(CategoricalVector(v), reverse(v))
+            dest = CategoricalVector{String}(["e", "f", "g"], ordered=true)
+            vdest = view(dest, 1:2)
+            res = @test_throws ArgumentError copy!(vdest, src)
+            @test res.value.msg == "cannot set ordered=false on dest SubArray as it would affect the parent. " *
+                "Found when trying to set levels to String[\"e\", \"f\", \"g\", \"b\", \"a\"]."
+            @test dest[1:2] ==  ["e", "f"]
+            @test levels(dest) == levels(vdest) == ["e", "f", "g"]
+            @test isordered(dest) && isordered(vdest)
+
+            dest = CategoricalVector{String}(["e", "f"], ordered=true)
+            vdest = view(dest, 1:2)
+            res = @test_throws ArgumentError copy!(vdest, src)
+            @test res.value.msg == "cannot set ordered=false on dest SubArray as it would affect the parent. " *
+                "Found when trying to set levels to String[\"e\", \"f\", \"b\", \"a\"]."
+            @test dest == ["e", "f"]
+            @test levels(dest) == levels(vdest) == ["e", "f"]
+            @test isordered(dest) && isordered(vdest)
+        end
+
         @testset "viable mixed src and dest types" begin
             v = ["a", "b", "c"]
             src = CategoricalVector{Union{eltype(v), Null}}(v)