From 7717106c05ec4194e4c646343e83f77e23e3b4d2 Mon Sep 17 00:00:00 2001 From: Andy Dienes Date: Sun, 24 Nov 2024 11:34:05 -0500 Subject: [PATCH 1/4] fix map! undefined value exposure --- base/abstractarray.jl | 7 +++++-- test/abstractarray.jl | 3 +++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/base/abstractarray.jl b/base/abstractarray.jl index 6102a0a8a00fa..b85177256aaad 100644 --- a/base/abstractarray.jl +++ b/base/abstractarray.jl @@ -3462,8 +3462,11 @@ function ith_all(i, as) end function map_n!(f::F, dest::AbstractArray, As) where F - idxs1 = LinearIndices(As[1]) - @boundscheck LinearIndices(dest) == idxs1 && all(x -> LinearIndices(x) == idxs1, As) + idxs1 = eachindex(LinearIndices(As[1])) + @boundscheck begin + idxs1 = intersect(map(eachindex ∘ LinearIndices, As)...) + checkbounds(dest, idxs1) + end for i = idxs1 @inbounds I = ith_all(i, As) val = f(I...) diff --git a/test/abstractarray.jl b/test/abstractarray.jl index 2a2ec8e8e432c..fd01dd036f03d 100644 --- a/test/abstractarray.jl +++ b/test/abstractarray.jl @@ -910,6 +910,9 @@ include("generic_map_tests.jl") generic_map_tests(map, map!) @test_throws ArgumentError map!(-, [1]) +# Issue #30624 +@test map!(+, [0,0,0], [1,2], [10,20,30], [100]) == [111,0,0] + test_UInt_indexing(TestAbstractArray) test_13315(TestAbstractArray) test_checksquare() From 592d233d0775e2aa640dd20895b6c3b30cd761d5 Mon Sep 17 00:00:00 2001 From: Andy Dienes Date: Wed, 27 Nov 2024 22:43:58 -0500 Subject: [PATCH 2/4] apply code review --- base/abstractarray.jl | 27 +++++++++++---------------- test/abstractarray.jl | 21 +++++++++++++++++++-- 2 files changed, 30 insertions(+), 18 deletions(-) diff --git a/base/abstractarray.jl b/base/abstractarray.jl index b85177256aaad..30989defba83b 100644 --- a/base/abstractarray.jl +++ b/base/abstractarray.jl @@ -3400,9 +3400,9 @@ concatenate_setindex!(R, X::AbstractArray, I...) = (R[I...] = X) ## 1 argument function map!(f::F, dest::AbstractArray, A::AbstractArray) where F - for (i,j) in zip(eachindex(dest),eachindex(A)) - val = f(@inbounds A[j]) - @inbounds dest[i] = val + inds = LinearIndices(A) + for i = inds + dest[i] = f(A[i]) end return dest end @@ -3445,10 +3445,10 @@ map(f, ::AbstractSet) = error("map is not defined on sets") ## 2 argument function map!(f::F, dest::AbstractArray, A::AbstractArray, B::AbstractArray) where F - for (i, j, k) in zip(eachindex(dest), eachindex(A), eachindex(B)) - @inbounds a, b = A[j], B[k] - val = f(a, b) - @inbounds dest[i] = val + inds = intersect(eachindex(LinearIndices(A)), eachindex(LinearIndices(B))) + @boundscheck checkbounds(dest, inds) + for i = inds + dest[i] = f(A[i], B[i]) end return dest end @@ -3462,15 +3462,10 @@ function ith_all(i, as) end function map_n!(f::F, dest::AbstractArray, As) where F - idxs1 = eachindex(LinearIndices(As[1])) - @boundscheck begin - idxs1 = intersect(map(eachindex ∘ LinearIndices, As)...) - checkbounds(dest, idxs1) - end - for i = idxs1 - @inbounds I = ith_all(i, As) - val = f(I...) - @inbounds dest[i] = val + inds = mapreduce(eachindex ∘ LinearIndices, intersect, As) + @boundscheck checkbounds(dest, inds) + for i = inds + dest[i] = f(ith_all(i, As)...) end return dest end diff --git a/test/abstractarray.jl b/test/abstractarray.jl index fd01dd036f03d..7bf77ee50515e 100644 --- a/test/abstractarray.jl +++ b/test/abstractarray.jl @@ -910,8 +910,25 @@ include("generic_map_tests.jl") generic_map_tests(map, map!) @test_throws ArgumentError map!(-, [1]) -# Issue #30624 -@test map!(+, [0,0,0], [1,2], [10,20,30], [100]) == [111,0,0] +function test_30624() + ### unstructured + @test map!(+, ones(3), ones(3), ones(3), [1]) == [3, 1, 1] + @test map!(+, ones(3), [1], ones(3), ones(3)) == [3, 1, 1] + @test map!(+, [1], [1], [], []) == [1] + @test map!(+, [[1]], [1], [], []) == [[1]] + + @test_throws BoundsError map!(+, ones(1), ones(2)) + @test_throws BoundsError map!(+, ones(1), ones(2, 2)) + + @test map!(+, ones(3), view(ones(2, 3), 1:2, 2:3), ones(3)) == [2, 2, 2] + @test map!(+, ones(3), ones(2, 2), ones(3)) == [2, 2, 2] + + ### structured (all mapped arguments are <:AbstractArray equal ndims > 1) + @test map!(+, ones(4), ones(2, 2), ones(2, 2)) == [2, 2, 2, 2] + @test map!(+, ones(4), ones(2, 2), ones(1, 2)) == [2, 2, 1, 1] + @test_throws BoundsError map!(+, ones(3), ones(2, 2), ones(2, 2)) +end +test_30624() test_UInt_indexing(TestAbstractArray) test_13315(TestAbstractArray) From c6230931d118e8c5763a7b41dede2fd8bd5d4c4e Mon Sep 17 00:00:00 2001 From: Andy Dienes Date: Wed, 16 Apr 2025 10:07:52 -0400 Subject: [PATCH 3/4] update for offsetarrays --- base/abstractarray.jl | 5 +++-- base/reducedim.jl | 2 +- test/abstractarray.jl | 3 +-- test/offsetarray.jl | 2 ++ 4 files changed, 7 insertions(+), 5 deletions(-) diff --git a/base/abstractarray.jl b/base/abstractarray.jl index ee9d35604b82e..a123aee131a1b 100644 --- a/base/abstractarray.jl +++ b/base/abstractarray.jl @@ -3400,7 +3400,8 @@ map(f, ::AbstractSet) = error("map is not defined on sets") ## 2 argument function map!(f::F, dest::AbstractArray, A::AbstractArray, B::AbstractArray) where F - inds = intersect(eachindex(LinearIndices(A)), eachindex(LinearIndices(B))) + IL = IndexLinear() + inds = intersect(eachindex(IL, A), eachindex(IL, B)) @boundscheck checkbounds(dest, inds) for i = inds dest[i] = f(A[i], B[i]) @@ -3417,7 +3418,7 @@ function ith_all(i, as) end function map_n!(f::F, dest::AbstractArray, As) where F - inds = mapreduce(eachindex ∘ LinearIndices, intersect, As) + inds = mapreduce(Fix1(eachindex, IndexLinear()), intersect, As) @boundscheck checkbounds(dest, inds) for i = inds dest[i] = f(ith_all(i, As)...) diff --git a/base/reducedim.jl b/base/reducedim.jl index 0478afe1a46b6..b48aeeefb9205 100644 --- a/base/reducedim.jl +++ b/base/reducedim.jl @@ -238,7 +238,7 @@ copyfirst!(R::AbstractArray, A::AbstractArray) = mapfirst!(identity, R, A) function mapfirst!(f::F, R::AbstractArray, A::AbstractArray{<:Any,N}) where {N, F} lsiz = check_reducedims(R, A) t = _firstreducedslice(axes(R), axes(A)) - map!(f, R, view(A, t...)) + map!(f, view(R, firstindex(R):lastindex(R)), view(A, t...)) end # We know that the axes of R and A are compatible, but R might have a different number of # dimensions than A, which is trickier than it seems due to offset arrays and type stability diff --git a/test/abstractarray.jl b/test/abstractarray.jl index 62ce38ce6e707..92aec49492eea 100644 --- a/test/abstractarray.jl +++ b/test/abstractarray.jl @@ -910,7 +910,7 @@ include("generic_map_tests.jl") generic_map_tests(map, map!) @test map!(-, [1]) == [-1] -function test_30624() +@testset "#30624" begin ### unstructured @test map!(+, ones(3), ones(3), ones(3), [1]) == [3, 1, 1] @test map!(+, ones(3), [1], ones(3), ones(3)) == [3, 1, 1] @@ -928,7 +928,6 @@ function test_30624() @test map!(+, ones(4), ones(2, 2), ones(1, 2)) == [2, 2, 1, 1] @test_throws BoundsError map!(+, ones(3), ones(2, 2), ones(2, 2)) end -test_30624() test_UInt_indexing(TestAbstractArray) test_13315(TestAbstractArray) diff --git a/test/offsetarray.jl b/test/offsetarray.jl index 8e2ee33c49ed6..b04e233c8df33 100644 --- a/test/offsetarray.jl +++ b/test/offsetarray.jl @@ -344,6 +344,8 @@ map!(+, dest, am, am) am = map(identity, a) @test isa(am, OffsetArray) @test am == a +@test_throws BoundsError map!(identity, fill(0, 1:3), fill(1, 2:4)) + # https://github.com/JuliaArrays/OffsetArrays.jl/issues/106 @test isequal(map(!, OffsetArray([true,missing],2)), OffsetArray([false, missing], 2)) From f9d5fa79e03d35825539c016670f38295c639349 Mon Sep 17 00:00:00 2001 From: Andy Dienes Date: Fri, 18 Apr 2025 16:17:14 -0400 Subject: [PATCH 4/4] apply code review --- base/abstractarray.jl | 32 ++++++++++++++++++++------------ base/reducedim.jl | 2 +- test/abstractarray.jl | 7 ++++--- test/offsetarray.jl | 2 -- 4 files changed, 25 insertions(+), 18 deletions(-) diff --git a/base/abstractarray.jl b/base/abstractarray.jl index a123aee131a1b..1c1e7ebb5c3ee 100644 --- a/base/abstractarray.jl +++ b/base/abstractarray.jl @@ -3355,9 +3355,9 @@ concatenate_setindex!(R, X::AbstractArray, I...) = (R[I...] = X) ## 1 argument function map!(f::F, dest::AbstractArray, A::AbstractArray) where F - inds = LinearIndices(A) - for i = inds - dest[i] = f(A[i]) + for (i,j) in zip(eachindex(dest),eachindex(A)) + val = f(@inbounds A[j]) + @inbounds dest[i] = val end return dest end @@ -3400,11 +3400,10 @@ map(f, ::AbstractSet) = error("map is not defined on sets") ## 2 argument function map!(f::F, dest::AbstractArray, A::AbstractArray, B::AbstractArray) where F - IL = IndexLinear() - inds = intersect(eachindex(IL, A), eachindex(IL, B)) - @boundscheck checkbounds(dest, inds) - for i = inds - dest[i] = f(A[i], B[i]) + for (i, j, k) in zip(eachindex(dest), eachindex(A), eachindex(B)) + @inbounds a, b = A[j], B[k] + val = f(a, b) + @inbounds dest[i] = val end return dest end @@ -3418,10 +3417,19 @@ function ith_all(i, as) end function map_n!(f::F, dest::AbstractArray, As) where F - inds = mapreduce(Fix1(eachindex, IndexLinear()), intersect, As) - @boundscheck checkbounds(dest, inds) - for i = inds - dest[i] = f(ith_all(i, As)...) + idxs = LinearIndices(dest) + if all(x -> LinearIndices(x) == idxs, As) + for i in idxs + @inbounds as = ith_all(i, As) + val = f(as...) + @inbounds dest[i] = val + end + else + for (i, Is...) in zip(eachindex(dest), map(eachindex, As)...) + as = ntuple(j->getindex(As[j], Is[j]), length(As)) + val = f(as...) + dest[i] = val + end end return dest end diff --git a/base/reducedim.jl b/base/reducedim.jl index b48aeeefb9205..0478afe1a46b6 100644 --- a/base/reducedim.jl +++ b/base/reducedim.jl @@ -238,7 +238,7 @@ copyfirst!(R::AbstractArray, A::AbstractArray) = mapfirst!(identity, R, A) function mapfirst!(f::F, R::AbstractArray, A::AbstractArray{<:Any,N}) where {N, F} lsiz = check_reducedims(R, A) t = _firstreducedslice(axes(R), axes(A)) - map!(f, view(R, firstindex(R):lastindex(R)), view(A, t...)) + map!(f, R, view(A, t...)) end # We know that the axes of R and A are compatible, but R might have a different number of # dimensions than A, which is trickier than it seems due to offset arrays and type stability diff --git a/test/abstractarray.jl b/test/abstractarray.jl index 92aec49492eea..b882778e4b152 100644 --- a/test/abstractarray.jl +++ b/test/abstractarray.jl @@ -917,8 +917,9 @@ generic_map_tests(map, map!) @test map!(+, [1], [1], [], []) == [1] @test map!(+, [[1]], [1], [], []) == [[1]] - @test_throws BoundsError map!(+, ones(1), ones(2)) - @test_throws BoundsError map!(+, ones(1), ones(2, 2)) + # TODO: decide if input axes & lengths should be validated + # @test_throws BoundsError map!(+, ones(1), ones(2)) + # @test_throws BoundsError map!(+, ones(1), ones(2, 2)) @test map!(+, ones(3), view(ones(2, 3), 1:2, 2:3), ones(3)) == [2, 2, 2] @test map!(+, ones(3), ones(2, 2), ones(3)) == [2, 2, 2] @@ -926,7 +927,7 @@ generic_map_tests(map, map!) ### structured (all mapped arguments are <:AbstractArray equal ndims > 1) @test map!(+, ones(4), ones(2, 2), ones(2, 2)) == [2, 2, 2, 2] @test map!(+, ones(4), ones(2, 2), ones(1, 2)) == [2, 2, 1, 1] - @test_throws BoundsError map!(+, ones(3), ones(2, 2), ones(2, 2)) + # @test_throws BoundsError map!(+, ones(3), ones(2, 2), ones(2, 2)) end test_UInt_indexing(TestAbstractArray) diff --git a/test/offsetarray.jl b/test/offsetarray.jl index b04e233c8df33..8e2ee33c49ed6 100644 --- a/test/offsetarray.jl +++ b/test/offsetarray.jl @@ -344,8 +344,6 @@ map!(+, dest, am, am) am = map(identity, a) @test isa(am, OffsetArray) @test am == a -@test_throws BoundsError map!(identity, fill(0, 1:3), fill(1, 2:4)) - # https://github.com/JuliaArrays/OffsetArrays.jl/issues/106 @test isequal(map(!, OffsetArray([true,missing],2)), OffsetArray([false, missing], 2))