From 2d20248ddbd03c1189c945e711215b312743f55c Mon Sep 17 00:00:00 2001 From: Tor Erlend Fjelde Date: Tue, 24 Oct 2023 11:56:16 +0100 Subject: [PATCH 1/9] fixed type instability in setindex! with array values --- src/base.jl | 15 ++++++++++++ test/test_setindex.jl | 55 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+) diff --git a/src/base.jl b/src/base.jl index be631e5..9b22e9d 100644 --- a/src/base.jl +++ b/src/base.jl @@ -488,6 +488,21 @@ possible(::typeof(_setindex!), ::C, ::V, ::K) where {C <: AbstractDict, V, K} = promote_type(keytype(C), K) <: keytype(C) && promote_type(valtype(C), V) <: valtype(C) +# Need to support a wide range of indexing behaviors. +# - Tuples of indices +# - Colon +# - AbstractUnitRange +# - Indexing with arrays +function possible( + ::typeof(_setindex!), + ::C, + ::T, + ::Vararg +) where {C <: AbstractArray{<:Real}, T <: AbstractArray{<:Real}} + return implements(setindex!, C) && + promote_type(eltype(C), eltype(T)) <: eltype(C) +end + """ resize!!(vector::AbstractVector, n::Integer) -> vector′ """ diff --git a/test/test_setindex.jl b/test/test_setindex.jl index 2224484..d20a216 100644 --- a/test/test_setindex.jl +++ b/test/test_setindex.jl @@ -2,6 +2,25 @@ module TestSetIndex include("preamble.jl") +# Some utility methods for testing `setindex!`. +replace_colon_with_axis(inds::Tuple, x) = ntuple(length(inds)) do i + inds[i] isa Colon ? axes(x, i) : inds[i] +end +replace_colon_with_vector(inds::Tuple, x) = ntuple(length(inds)) do i + inds[i] isa Colon ? collect(axes(x, i)) : inds[i] +end +replace_colon_with_range(inds::Tuple, x) = ntuple(length(inds)) do i + inds[i] isa Colon ? (1:size(x, i)) : inds[i] +end +replace_colon_with_booleans(inds::Tuple, x) = ntuple(length(inds)) do i + inds[i] isa Colon ? trues(size(x, i)) : inds[i] +end + +replace_colon_with_range_linear(inds::NTuple{1}, x::AbstractArray) = inds[1] isa Colon ? (1:length(x),) : inds + +# TODO: Do we handle expansive indexing, e.g. `x[:, :]` for `x isa Vector` correctly? +is_linear_index(inds::Tuple, x::AbstractArray) = length(inds) == 1 + @testset begin @test setindex!!((1, 2, 3), :two, 2) === (1, :two, 3) @test setindex!!((a=1, b=2, c=3), :two, :b) === (a=1, b=:two, c=3) @@ -21,4 +40,40 @@ end end end +@testset "slices" begin + @testset "$(typeof(x)) with $(src_idx)" for (x, src_idx) in [ + # Vector. + (randn(2), (:,)), + (randn(2), (1:2,)), + # Matrix. + (randn(2, 3), (:,)), + (randn(2, 3), (:, 1)), + (randn(2, 3), (:, 1:3)), + # 3D array. + (randn(2, 3, 4), (:, 1, :)), + (randn(2, 3, 4), (:, 1:3, :)), + (randn(2, 3, 4), (1, 1:3, :)), + ] + # Base case. + @test @inferred(setindex!!(x, x[src_idx...], src_idx...)) === x + + # If we have `Colon` in the index, we replace this with other equivalent indices. + if any(Base.Fix2(isa, Colon), src_idx) + if is_linear_index(src_idx, x) + # With range instead of `Colon`. + @test @inferred(setindex!!(x, x[src_idx...], replace_colon_with_range_linear(src_idx, x)...)) === x + else + # With axis instead of `Colon`. + @test @inferred(setindex!!(x, x[src_idx...], replace_colon_with_axis(src_idx, x)...)) === x + # With range instead of `Colon`. + @test @inferred(setindex!!(x, x[src_idx...], replace_colon_with_range(src_idx, x)...)) === x + # With vectors instead of `Colon`. + @test @inferred(setindex!!(x, x[src_idx...], replace_colon_with_vector(src_idx, x)...)) === x + # With boolean index instead of `Colon`. + @test @inferred(setindex!!(x, x[src_idx...], replace_colon_with_booleans(src_idx, x)...)) === x + end + end + end +end + end # module From 90ffd4ffffc75c3c1f5145c501b943fdd4dae4c5 Mon Sep 17 00:00:00 2001 From: Tor Erlend Fjelde Date: Tue, 24 Oct 2023 12:01:19 +0100 Subject: [PATCH 2/9] removed TODO comment --- test/test_setindex.jl | 1 - 1 file changed, 1 deletion(-) diff --git a/test/test_setindex.jl b/test/test_setindex.jl index d20a216..8d551ad 100644 --- a/test/test_setindex.jl +++ b/test/test_setindex.jl @@ -18,7 +18,6 @@ end replace_colon_with_range_linear(inds::NTuple{1}, x::AbstractArray) = inds[1] isa Colon ? (1:length(x),) : inds -# TODO: Do we handle expansive indexing, e.g. `x[:, :]` for `x isa Vector` correctly? is_linear_index(inds::Tuple, x::AbstractArray) = length(inds) == 1 @testset begin From 9f9c00f8a3476b7156e9325dff2b0bf930840cd3 Mon Sep 17 00:00:00 2001 From: Tor Erlend Fjelde Date: Tue, 24 Oct 2023 12:05:09 +0100 Subject: [PATCH 3/9] renamed one is_linear_index to test_linear_index_only + return false for vector --- test/test_setindex.jl | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/test_setindex.jl b/test/test_setindex.jl index 8d551ad..5cede87 100644 --- a/test/test_setindex.jl +++ b/test/test_setindex.jl @@ -3,6 +3,10 @@ module TestSetIndex include("preamble.jl") # Some utility methods for testing `setindex!`. +test_linear_index_only(::Tuple, ::AbstractArray) = false +test_linear_index_only(inds::NTuple{1}, ::AbstractArray) = true +test_linear_index_only(inds::NTuple{1}, ::AbstractVector) = false + replace_colon_with_axis(inds::Tuple, x) = ntuple(length(inds)) do i inds[i] isa Colon ? axes(x, i) : inds[i] end @@ -18,8 +22,6 @@ end replace_colon_with_range_linear(inds::NTuple{1}, x::AbstractArray) = inds[1] isa Colon ? (1:length(x),) : inds -is_linear_index(inds::Tuple, x::AbstractArray) = length(inds) == 1 - @testset begin @test setindex!!((1, 2, 3), :two, 2) === (1, :two, 3) @test setindex!!((a=1, b=2, c=3), :two, :b) === (a=1, b=:two, c=3) @@ -58,7 +60,7 @@ end # If we have `Colon` in the index, we replace this with other equivalent indices. if any(Base.Fix2(isa, Colon), src_idx) - if is_linear_index(src_idx, x) + if test_linear_index_only(src_idx, x) # With range instead of `Colon`. @test @inferred(setindex!!(x, x[src_idx...], replace_colon_with_range_linear(src_idx, x)...)) === x else From 764e74473566b15879ba62f4df7001dee4449c31 Mon Sep 17 00:00:00 2001 From: Tor Erlend Fjelde Date: Thu, 26 Oct 2023 07:59:04 +0100 Subject: [PATCH 4/9] fix issue with `possible` for `_setindex!` not handling invalid array values --- src/base.jl | 19 ++++++++++++++++--- test/test_setindex.jl | 1 + 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/base.jl b/src/base.jl index 9b22e9d..c76a8f1 100644 --- a/src/base.jl +++ b/src/base.jl @@ -493,14 +493,27 @@ possible(::typeof(_setindex!), ::C, ::V, ::K) where {C <: AbstractDict, V, K} = # - Colon # - AbstractUnitRange # - Indexing with arrays +# +# Also need to ensure that the dimensionality of the index is +# valid, i.e. that we're not returning `true` in cases such as +# +# setindex!!([1, 2, 3], 1, [4, 5]) +# +# which should return `false`. +_index_dimension(::Any) = 0 +_index_dimension(::Colon) = 1 +_index_dimension(::AbstractVector) = 1 +_index_dimension(indices::Tuple) = sum(map(_index_dimension, indices)) + function possible( ::typeof(_setindex!), ::C, ::T, - ::Vararg -) where {C <: AbstractArray{<:Real}, T <: AbstractArray{<:Real}} + indices::Vararg + ) where {M, C <: AbstractArray{<:Real}, T <: AbstractArray{<:Real,M}} return implements(setindex!, C) && - promote_type(eltype(C), eltype(T)) <: eltype(C) + promote_type(eltype(C), eltype(T)) <: eltype(C) && + (_index_dimension(indices) == M || _index_dimension(indices) == 1) end """ diff --git a/test/test_setindex.jl b/test/test_setindex.jl index 5cede87..161aa0d 100644 --- a/test/test_setindex.jl +++ b/test/test_setindex.jl @@ -30,6 +30,7 @@ replace_colon_with_range_linear(inds::NTuple{1}, x::AbstractArray) = inds[1] isa Dict(:a=>10, :b=>2) @test setindex!!(Dict{Symbol,Int}(:a=>1, :b=>2), 3, "c") == Dict(:a=>1, :b=>2, "c"=>3) + @testset setindex!!([1, 2, 3], [4, 5], 1) === [[4, 5], 2, 3] end @testset "mutation" begin From 2d34ad8f17e99150c71cafdbaf33c59644acbf67 Mon Sep 17 00:00:00 2001 From: Tor Erlend Fjelde Date: Thu, 26 Oct 2023 08:09:22 +0100 Subject: [PATCH 5/9] move things around and add further description to the code --- src/base.jl | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/src/base.jl b/src/base.jl index c76a8f1..e5b95bc 100644 --- a/src/base.jl +++ b/src/base.jl @@ -481,23 +481,27 @@ Base.@propagate_inbounds _setindex!(xs, v, I...) = (setindex!(xs, v, I...); xs) pure(::typeof(_setindex!)) = NoBang._setindex possible(::typeof(_setindex!), ::Union{Tuple,NamedTuple,Empty}, ::Vararg) = false -possible(::typeof(_setindex!), ::C, ::T, ::Vararg) where {C <: AbstractArray, T} = - implements(setindex!, C) && promote_type(eltype(C), T) <: eltype(C) possible(::typeof(_setindex!), ::C, ::V, ::K) where {C <: AbstractDict, V, K} = implements(setindex!, C) && promote_type(keytype(C), K) <: keytype(C) && promote_type(valtype(C), V) <: valtype(C) -# Need to support a wide range of indexing behaviors. -# - Tuples of indices -# - Colon -# - AbstractUnitRange -# - Indexing with arrays +# Default implementation for `_setindex!` with `AbstractArray`. +# But this will return `false` even in cases such as +# +# setindex!!([1, 2, 3], [4, 5, 6], :) +# +# because `promote_type(eltype(C), T) <: eltype(C)` is `false`. +possible(::typeof(_setindex!), ::C, ::T, ::Vararg) where {C <: AbstractArray, T} = + implements(setindex!, C) && promote_type(eltype(C), T) <: eltype(C) + +# To address this, we specialize on the case where `T<:AbstractArray`. +# In addition, we need to support a wide range of indexing behaviors: # -# Also need to ensure that the dimensionality of the index is +# We also need to ensure that the dimensionality of the index is # valid, i.e. that we're not returning `true` in cases such as # -# setindex!!([1, 2, 3], 1, [4, 5]) +# setindex!!([1, 2, 3], [4, 5], 1) # # which should return `false`. _index_dimension(::Any) = 0 @@ -513,6 +517,11 @@ function possible( ) where {M, C <: AbstractArray{<:Real}, T <: AbstractArray{<:Real,M}} return implements(setindex!, C) && promote_type(eltype(C), eltype(T)) <: eltype(C) && + # This will still return `false` for scenarios such as + # + # setindex!!([1, 2, 3], [4, 5, 6], :, 1) + # + # which are in fact valid. However, this case is rare. (_index_dimension(indices) == M || _index_dimension(indices) == 1) end From 4f3ae19055e07854b85d03f05135c65c69976ff6 Mon Sep 17 00:00:00 2001 From: Tor Erlend Fjelde Date: Thu, 26 Oct 2023 08:10:29 +0100 Subject: [PATCH 6/9] fixed comment typo --- src/base.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/base.jl b/src/base.jl index e5b95bc..84f07c5 100644 --- a/src/base.jl +++ b/src/base.jl @@ -521,7 +521,7 @@ function possible( # # setindex!!([1, 2, 3], [4, 5, 6], :, 1) # - # which are in fact valid. However, this case is rare. + # which are in fact valid. However, this cases are rare. (_index_dimension(indices) == M || _index_dimension(indices) == 1) end From e0e6aa29800e38be008f85c5db94211737e75f48 Mon Sep 17 00:00:00 2001 From: Tor Erlend Fjelde Date: Thu, 26 Oct 2023 08:21:55 +0100 Subject: [PATCH 7/9] fix typos in tests --- test/test_setindex.jl | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/test/test_setindex.jl b/test/test_setindex.jl index 161aa0d..d4aec44 100644 --- a/test/test_setindex.jl +++ b/test/test_setindex.jl @@ -30,15 +30,21 @@ replace_colon_with_range_linear(inds::NTuple{1}, x::AbstractArray) = inds[1] isa Dict(:a=>10, :b=>2) @test setindex!!(Dict{Symbol,Int}(:a=>1, :b=>2), 3, "c") == Dict(:a=>1, :b=>2, "c"=>3) - @testset setindex!!([1, 2, 3], [4, 5], 1) === [[4, 5], 2, 3] end @testset "mutation" begin - @testset for args in [ - ([1, 2, 3], 20, 2), - (Dict(:a=>1, :b=>2), 10, :a), - ] - @test setindex!!(args...) === args[1] + @testset "without type expansion" begin + for args in [ + ([1, 2, 3], 20, 2), + (Dict(:a => 1, :b => 2), 10, :a), + ] + @test setindex!!(args...) === args[1] + end + end + + @testset "with type expansion" begin + @test setindex!!([1, 2, 3], [4, 5], 1) == [[4, 5], 2, 3] + @test setindex!!([1, 2, 3], [4, 5, 6], :, 1) == [4, 5, 6] end end From 44c932533579068ab23801bb96c6d3e1dca289df Mon Sep 17 00:00:00 2001 From: Mason Protter Date: Sat, 27 Jan 2024 11:26:37 +0100 Subject: [PATCH 8/9] drop eltype restrictions --- src/base.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/base.jl b/src/base.jl index 84f07c5..ef5e2eb 100644 --- a/src/base.jl +++ b/src/base.jl @@ -514,7 +514,7 @@ function possible( ::C, ::T, indices::Vararg - ) where {M, C <: AbstractArray{<:Real}, T <: AbstractArray{<:Real,M}} + ) where {M, C <: AbstractArray{<:Any}, T <: AbstractArray{<:Any,M}} return implements(setindex!, C) && promote_type(eltype(C), eltype(T)) <: eltype(C) && # This will still return `false` for scenarios such as From 18d5f6b29d8da08d456d6ad14ef4c1143869f1aa Mon Sep 17 00:00:00 2001 From: Mason Protter Date: Sat, 27 Jan 2024 11:30:09 +0100 Subject: [PATCH 9/9] add a non-<:Real test case --- test/test_setindex.jl | 1 + 1 file changed, 1 insertion(+) diff --git a/test/test_setindex.jl b/test/test_setindex.jl index d4aec44..7a6c780 100644 --- a/test/test_setindex.jl +++ b/test/test_setindex.jl @@ -37,6 +37,7 @@ end for args in [ ([1, 2, 3], 20, 2), (Dict(:a => 1, :b => 2), 10, :a), + ([(1,),(2,),(3,)], [(4,), (5,), (6,)], :, 1) ] @test setindex!!(args...) === args[1] end