Skip to content

Commit

Permalink
Make no-index A[] check bounds like everyone else (#24236)
Browse files Browse the repository at this point in the history
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`.
  • Loading branch information
mbauman authored and JeffBezanson committed Oct 23, 2017
1 parent fc6b2ba commit 0f498e7
Show file tree
Hide file tree
Showing 6 changed files with 40 additions and 39 deletions.
29 changes: 8 additions & 21 deletions base/abstractarray.jl
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -943,24 +942,18 @@ _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
@inbounds r = getindex(A, _to_linear_index(A, I...))
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
Expand All @@ -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)
Expand Down Expand Up @@ -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...)
Expand All @@ -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...)
Expand Down Expand Up @@ -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)
Expand Down
5 changes: 2 additions & 3 deletions base/array.jl
Original file line number Diff line number Diff line change
Expand Up @@ -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})
Expand Down Expand Up @@ -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})
Expand Down Expand Up @@ -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]
Expand Down
2 changes: 1 addition & 1 deletion base/subarray.jl
Original file line number Diff line number Diff line change
Expand Up @@ -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...)
Expand Down
28 changes: 18 additions & 10 deletions test/abstractarray.jl
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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})
Expand All @@ -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})
Expand Down
13 changes: 10 additions & 3 deletions test/bitarray.jl
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
2 changes: 1 addition & 1 deletion test/core.jl
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down

0 comments on commit 0f498e7

Please sign in to comment.