From 94e0fb612d9efb5ff80f0851d34c8857db226cd7 Mon Sep 17 00:00:00 2001 From: Matt Bauman Date: Fri, 20 Oct 2017 10:48:05 -0500 Subject: [PATCH 1/3] Make no-index A[] check bounds like everyone else In #23628, we deprecated omitting indices over dimensions that are not of length 1. Unfortunately the 0-index case got left behind. This brings it into consistency. In short, once this deprecation is removed, `A[]` will _also_ assert that there is only one element in `A`. --- base/abstractarray.jl | 29 ++++++++--------------------- base/array.jl | 5 ++--- base/subarray.jl | 2 +- 3 files changed, 11 insertions(+), 25 deletions(-) diff --git a/base/abstractarray.jl b/base/abstractarray.jl index 3ffb9657e3e34..d6b32e16c59a8 100644 --- a/base/abstractarray.jl +++ b/base/abstractarray.jl @@ -399,7 +399,6 @@ function checkbounds(A::AbstractArray, I...) checkbounds(Bool, A, I...) || throw_boundserror(A, I) nothing end -checkbounds(A::AbstractArray) = checkbounds(A, 1) # 0-d case """ checkbounds_indices(Bool, IA, I) @@ -943,7 +942,6 @@ _getindex(::IndexStyle, A::AbstractArray, I...) = ## IndexLinear Scalar indexing: canonical method is one Int _getindex(::IndexLinear, A::AbstractArray, i::Int) = (@_propagate_inbounds_meta; getindex(A, i)) -_getindex(::IndexLinear, A::AbstractArray) = (@_propagate_inbounds_meta; getindex(A, _to_linear_index(A))) function _getindex(::IndexLinear, A::AbstractArray, I::Vararg{Int,M}) where M @_inline_meta @boundscheck checkbounds(A, I...) # generally _to_linear_index requires bounds checking @@ -951,16 +949,11 @@ function _getindex(::IndexLinear, A::AbstractArray, I::Vararg{Int,M}) where M r end _to_linear_index(A::AbstractArray, i::Int) = i -_to_linear_index(A::AbstractVector, i::Int, I::Int...) = i # TODO: DEPRECATE FOR #14770 -_to_linear_index(A::AbstractArray{T,N}, I::Vararg{Int,N}) where {T,N} = (@_inline_meta; sub2ind(A, I...)) -_to_linear_index(A::AbstractArray) = 1 # TODO: DEPRECATE FOR #14770 -_to_linear_index(A::AbstractArray, I::Int...) = (@_inline_meta; sub2ind(A, I...)) # TODO: DEPRECATE FOR #14770 +_to_linear_index(A::AbstractVector, i::Int, I::Int...) = i +_to_linear_index(A::AbstractArray) = 1 +_to_linear_index(A::AbstractArray, I::Int...) = (@_inline_meta; sub2ind(A, I...)) ## IndexCartesian Scalar indexing: Canonical method is full dimensionality of Ints -function _getindex(::IndexCartesian, A::AbstractArray) - @_propagate_inbounds_meta - getindex(A, _to_subscript_indices(A)...) -end function _getindex(::IndexCartesian, A::AbstractArray, I::Vararg{Int,M}) where M @_inline_meta @boundscheck checkbounds(A, I...) # generally _to_subscript_indices requires bounds checking @@ -972,11 +965,11 @@ function _getindex(::IndexCartesian, A::AbstractArray{T,N}, I::Vararg{Int, N}) w getindex(A, I...) end _to_subscript_indices(A::AbstractArray, i::Int) = (@_inline_meta; _unsafe_ind2sub(A, i)) -_to_subscript_indices(A::AbstractArray{T,N}) where {T,N} = (@_inline_meta; fill_to_length((), 1, Val(N))) # TODO: DEPRECATE FOR #14770 -_to_subscript_indices(A::AbstractArray{T,0}) where {T} = () # TODO: REMOVE FOR #14770 -_to_subscript_indices(A::AbstractArray{T,0}, i::Int) where {T} = () # TODO: REMOVE FOR #14770 -_to_subscript_indices(A::AbstractArray{T,0}, I::Int...) where {T} = () # TODO: DEPRECATE FOR #14770 -function _to_subscript_indices(A::AbstractArray{T,N}, I::Int...) where {T,N} # TODO: DEPRECATE FOR #14770 +_to_subscript_indices(A::AbstractArray{T,N}) where {T,N} = (@_inline_meta; fill_to_length((), 1, Val(N))) +_to_subscript_indices(A::AbstractArray{T,0}) where {T} = () +_to_subscript_indices(A::AbstractArray{T,0}, i::Int) where {T} = () +_to_subscript_indices(A::AbstractArray{T,0}, I::Int...) where {T} = () +function _to_subscript_indices(A::AbstractArray{T,N}, I::Int...) where {T,N} @_inline_meta J, Jrem = IteratorsMD.split(I, Val(N)) _to_subscript_indices(A, J, Jrem) @@ -1019,7 +1012,6 @@ _setindex!(::IndexStyle, A::AbstractArray, v, I...) = ## IndexLinear Scalar indexing _setindex!(::IndexLinear, A::AbstractArray, v, i::Int) = (@_propagate_inbounds_meta; setindex!(A, v, i)) -_setindex!(::IndexLinear, A::AbstractArray, v) = (@_propagate_inbounds_meta; setindex!(A, v, _to_linear_index(A))) function _setindex!(::IndexLinear, A::AbstractArray, v, I::Vararg{Int,M}) where M @_inline_meta @boundscheck checkbounds(A, I...) @@ -1032,10 +1024,6 @@ function _setindex!(::IndexCartesian, A::AbstractArray{T,N}, v, I::Vararg{Int, N @_propagate_inbounds_meta setindex!(A, v, I...) end -function _setindex!(::IndexCartesian, A::AbstractArray, v) - @_propagate_inbounds_meta - setindex!(A, v, _to_subscript_indices(A)...) -end function _setindex!(::IndexCartesian, A::AbstractArray, v, I::Vararg{Int,M}) where M @_inline_meta @boundscheck checkbounds(A, I...) @@ -1072,7 +1060,6 @@ end get(A::AbstractArray, I::AbstractRange, default) = get!(similar(A, typeof(default), index_shape(I)), A, I, default) -# TODO: DEPRECATE FOR #14770 (just the partial linear indexing part) function get!(X::AbstractArray{T}, A::AbstractArray, I::RangeVecIntList, default::T) where T fill!(X, default) dst, src = indcopy(size(A), I) diff --git a/base/array.jl b/base/array.jl index 8647a64714379..3b7096fdd254b 100644 --- a/base/array.jl +++ b/base/array.jl @@ -756,7 +756,7 @@ function getindex end # This is more complicated than it needs to be in order to get Win64 through bootstrap @eval getindex(A::Array, i1::Int) = arrayref($(Expr(:boundscheck)), A, i1) -@eval getindex(A::Array, i1::Int, i2::Int, I::Int...) = (@_inline_meta; arrayref($(Expr(:boundscheck)), A, i1, i2, I...)) # TODO: REMOVE FOR #14770 +@eval getindex(A::Array, i1::Int, i2::Int, I::Int...) = (@_inline_meta; arrayref($(Expr(:boundscheck)), A, i1, i2, I...)) # Faster contiguous indexing using copy! for UnitRange and Colon function getindex(A::Array, I::UnitRange{Int}) @@ -795,7 +795,7 @@ function setindex! end @eval setindex!(A::Array{T}, x, i1::Int) where {T} = arrayset($(Expr(:boundscheck)), A, convert(T,x)::T, i1) @eval setindex!(A::Array{T}, x, i1::Int, i2::Int, I::Int...) where {T} = - (@_inline_meta; arrayset($(Expr(:boundscheck)), A, convert(T,x)::T, i1, i2, I...)) # TODO: REMOVE FOR #14770 + (@_inline_meta; arrayset($(Expr(:boundscheck)), A, convert(T,x)::T, i1, i2, I...)) # These are redundant with the abstract fallbacks but needed for bootstrap function setindex!(A::Array, x, I::AbstractVector{Int}) @@ -2231,7 +2231,6 @@ end findin(a, b) = _findin(a, b) # Copying subregions -# TODO: DEPRECATE FOR #14770 function indcopy(sz::Dims, I::Vector) n = length(I) s = sz[n] diff --git a/base/subarray.jl b/base/subarray.jl index 2f083abe5e399..608a2395d0963 100644 --- a/base/subarray.jl +++ b/base/subarray.jl @@ -100,7 +100,7 @@ parentindexes(a::AbstractArray) = ntuple(i->OneTo(size(a,i)), ndims(a)) _maybe_reshape_parent(A::AbstractArray, ::NTuple{1, Bool}) = reshape(A, Val(1)) _maybe_reshape_parent(A::AbstractArray{<:Any,1}, ::NTuple{1, Bool}) = reshape(A, Val(1)) _maybe_reshape_parent(A::AbstractArray{<:Any,N}, ::NTuple{N, Bool}) where {N} = A -_maybe_reshape_parent(A::AbstractArray, ::NTuple{N, Bool}) where {N} = reshape(A, Val(N)) # TODO: DEPRECATE FOR #14770 +_maybe_reshape_parent(A::AbstractArray, ::NTuple{N, Bool}) where {N} = reshape(A, Val(N)) """ view(A, inds...) From c3789a54e359a3605aa75f91c7f4ee46e4403fcc Mon Sep 17 00:00:00 2001 From: Matt Bauman Date: Fri, 20 Oct 2017 13:48:41 -0500 Subject: [PATCH 2/3] Fixup tests --- test/abstractarray.jl | 28 ++++++++++++++++++---------- test/bitarray.jl | 13 ++++++++++--- test/core.jl | 2 +- 3 files changed, 29 insertions(+), 14 deletions(-) diff --git a/test/abstractarray.jl b/test/abstractarray.jl index 9c6b0d51e207f..4da890c701197 100644 --- a/test/abstractarray.jl +++ b/test/abstractarray.jl @@ -301,7 +301,7 @@ function test_scalar_indexing(::Type{T}, shape, ::Type{TestAbstractArray}) where end end # Test zero-dimensional accesses - @test A[] == B[] == A[1] == B[1] == 1 + @test A[1] == B[1] == 1 # Test multidimensional scalar indexed assignment C = T(Int, shape) D1 = T(Int, shape) @@ -361,9 +361,17 @@ function test_scalar_indexing(::Type{T}, shape, ::Type{TestAbstractArray}) where end @test C == B == A # Test zero-dimensional setindex - A[] = 0; B[] = 0 - @test A[] == B[] == 0 - @test A == B + if length(A) == 1 + A[] = 0; B[] = 0 + @test A[] == B[] == 0 + @test A == B + else + # TODO: Re-enable after PLI deprecation + # @test_throws BoundsError A[] = 0 + # @test_throws BoundsError B[] = 0 + # @test_throws BoundsError A[] + # @test_throws BoundsError B[] + end end function test_vector_indexing(::Type{T}, shape, ::Type{TestAbstractArray}) where T @@ -489,10 +497,10 @@ function test_getindex_internals(::Type{T}, shape, ::Type{TestAbstractArray}) wh A = reshape(collect(1:N), shape) B = T(A) - @test getindex(A) == 1 - @test getindex(B) == 1 - @test Base.unsafe_getindex(A) == 1 - @test Base.unsafe_getindex(B) == 1 + @test getindex(A, 1) == 1 + @test getindex(B, 1) == 1 + @test Base.unsafe_getindex(A, 1) == 1 + @test Base.unsafe_getindex(B, 1) == 1 end function test_getindex_internals(::Type{TestAbstractArray}) @@ -509,8 +517,8 @@ function test_setindex!_internals(::Type{T}, shape, ::Type{TestAbstractArray}) w A = reshape(collect(1:N), shape) B = T(A) - Base.unsafe_setindex!(B, 1) - @test B[1] == 1 + Base.unsafe_setindex!(B, 2, 1) + @test B[1] == 2 end function test_setindex!_internals(::Type{TestAbstractArray}) diff --git a/test/bitarray.jl b/test/bitarray.jl index 3263dd612e4dc..4700eec1e7a17 100644 --- a/test/bitarray.jl +++ b/test/bitarray.jl @@ -206,9 +206,16 @@ timesofar("constructors") @testset "Indexing" begin @testset "0d for size $sz" for (sz,T) in allsizes b1 = rand!(falses(sz...)) - @check_bit_operation getindex(b1) Bool - @check_bit_operation setindex!(b1, true) T - @check_bit_operation setindex!(b1, false) T + if length(b1) == 1 + @check_bit_operation getindex(b1) Bool + @check_bit_operation setindex!(b1, true) T + @check_bit_operation setindex!(b1, false) T + else + # TODO: Re-enable after PLI deprecation is removed + # @test_throws getindex(b1) + # @test_throws setindex!(b1, true) + # @test_throws setindex!(b1, false) + end end @testset "linear for size $sz" for (sz,T) in allsizes[2:end] diff --git a/test/core.jl b/test/core.jl index 44bf53f7abb8d..d1b29021f87fd 100644 --- a/test/core.jl +++ b/test/core.jl @@ -2332,7 +2332,7 @@ end # pull request #9534 @test try; a,b,c = 1,2; catch ex; (ex::BoundsError).a === (1,2) && ex.i == 3; end -@test try; [][]; catch ex; isempty((ex::BoundsError).a::Array{Any,1}) && ex.i == (1,); end +# @test try; [][]; catch ex; isempty((ex::BoundsError).a::Array{Any,1}) && ex.i == (1,); end # TODO: Re-enable after PLI @test try; [][1,2]; catch ex; isempty((ex::BoundsError).a::Array{Any,1}) && ex.i == (1,2); end @test try; [][10]; catch ex; isempty((ex::BoundsError).a::Array{Any,1}) && ex.i == (10,); end f9534a() = (a=1+2im; getfield(a, -100)) From 2ad31e9cb76e45f3d92826d7b733edc3ba2eeb4a Mon Sep 17 00:00:00 2001 From: Matt Bauman Date: Fri, 20 Oct 2017 14:26:57 -0500 Subject: [PATCH 3/3] Whitespace (SQUASH ME!) --- test/abstractarray.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/abstractarray.jl b/test/abstractarray.jl index 4da890c701197..a1d698d2ac933 100644 --- a/test/abstractarray.jl +++ b/test/abstractarray.jl @@ -301,7 +301,7 @@ function test_scalar_indexing(::Type{T}, shape, ::Type{TestAbstractArray}) where end end # Test zero-dimensional accesses - @test A[1] == B[1] == 1 + @test A[1] == B[1] == 1 # Test multidimensional scalar indexed assignment C = T(Int, shape) D1 = T(Int, shape)