Skip to content

Commit

Permalink
deprecate unsafe_length for length
Browse files Browse the repository at this point in the history
This seems to be a fairly arbitrary case for throwing exceptions, when
the user might often use this value in arithmetic afterwards, which is
not checked. It leads to awkward complexity in the API however, where it
may be unclear which function to reach for, with no particular
justification for why a particular usage is "safe". And it inhibits
optimization and performance due to the additional checks and error
cases (and is not even entirely type-stable).
  • Loading branch information
vtjnash committed Apr 8, 2021
1 parent 6117388 commit 6c549a9
Show file tree
Hide file tree
Showing 11 changed files with 90 additions and 93 deletions.
15 changes: 6 additions & 9 deletions base/abstractarray.jl
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,6 @@ axes1(A::AbstractArray{<:Any,0}) = OneTo(1)
axes1(A::AbstractArray) = (@_inline_meta; axes(A)[1])
axes1(iter) = oneto(length(iter))

unsafe_indices(A) = axes(A)
unsafe_indices(r::AbstractRange) = (oneto(unsafe_length(r)),) # Ranges use checked_sub for size

keys(a::AbstractArray) = CartesianIndices(axes(a))
keys(a::AbstractVector) = LinearIndices(a)

Expand Down Expand Up @@ -538,14 +535,14 @@ end
function trailingsize(inds::Indices, n)
s = 1
for i=n:length(inds)
s *= unsafe_length(inds[i])
s *= length(inds[i])
end
return s
end
# This version is type-stable even if inds is heterogeneous
function trailingsize(inds::Indices)
@_inline_meta
prod(map(unsafe_length, inds))
prod(map(length, inds))
end

## Bounds checking ##
Expand Down Expand Up @@ -646,7 +643,7 @@ function checkbounds_indices(::Type{Bool}, ::Tuple{}, I::Tuple)
@_inline_meta
checkindex(Bool, OneTo(1), I[1])::Bool & checkbounds_indices(Bool, (), tail(I))
end
checkbounds_indices(::Type{Bool}, IA::Tuple, ::Tuple{}) = (@_inline_meta; all(x->unsafe_length(x)==1, IA))
checkbounds_indices(::Type{Bool}, IA::Tuple, ::Tuple{}) = (@_inline_meta; all(x->length(x)==1, IA))
checkbounds_indices(::Type{Bool}, ::Tuple{}, ::Tuple{}) = true

throw_boundserror(A, I) = (@_noinline_meta; throw(BoundsError(A, I)))
Expand Down Expand Up @@ -2051,8 +2048,8 @@ function _sub2ind_recurse(inds, L, ind, i::Integer, I::Integer...)
end

nextL(L, l::Integer) = L*l
nextL(L, r::AbstractUnitRange) = L*unsafe_length(r)
nextL(L, r::Slice) = L*unsafe_length(r.indices)
nextL(L, r::AbstractUnitRange) = L*length(r)
nextL(L, r::Slice) = L*length(r.indices)
offsetin(i, l::Integer) = i-1
offsetin(i, r::AbstractUnitRange) = i-first(r)

Expand All @@ -2078,7 +2075,7 @@ end
_lookup(ind, d::Integer) = ind+1
_lookup(ind, r::AbstractUnitRange) = ind+first(r)
_div(ind, d::Integer) = div(ind, d), 1, d
_div(ind, r::AbstractUnitRange) = (d = unsafe_length(r); (div(ind, d), first(r), d))
_div(ind, r::AbstractUnitRange) = (d = length(r); (div(ind, d), first(r), d))

# Vectorized forms
function _sub2ind(inds::Indices{1}, I1::AbstractVector{T}, I::AbstractVector{T}...) where T<:Integer
Expand Down
2 changes: 1 addition & 1 deletion base/broadcast.jl
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,7 @@ an `Int`.
"""
Base.@propagate_inbounds newindex(arg, I::CartesianIndex) = CartesianIndex(_newindex(axes(arg), I.I))
Base.@propagate_inbounds newindex(arg, I::Integer) = CartesianIndex(_newindex(axes(arg), (I,)))
Base.@propagate_inbounds _newindex(ax::Tuple, I::Tuple) = (ifelse(Base.unsafe_length(ax[1])==1, ax[1][1], I[1]), _newindex(tail(ax), tail(I))...)
Base.@propagate_inbounds _newindex(ax::Tuple, I::Tuple) = (ifelse(length(ax[1]) == 1, ax[1][1], I[1]), _newindex(tail(ax), tail(I))...)
Base.@propagate_inbounds _newindex(ax::Tuple{}, I::Tuple) = ()
Base.@propagate_inbounds _newindex(ax::Tuple, I::Tuple{}) = (ax[1][1], _newindex(tail(ax), ())...)
Base.@propagate_inbounds _newindex(ax::Tuple{}, I::Tuple{}) = ()
Expand Down
3 changes: 3 additions & 0 deletions base/deprecated.jl
Original file line number Diff line number Diff line change
Expand Up @@ -238,4 +238,7 @@ end
@deprecate cat_shape(dims, shape::Tuple{}, shapes::Tuple...) cat_shape(dims, shapes)
cat_shape(dims, shape::Tuple{}) = () # make sure `cat_shape(dims, ())` do not recursively calls itself

@deprecate unsafe_indices(A) axes(A)
@deprecate unsafe_length(r) length(r)

# END 1.6 deprecations
8 changes: 1 addition & 7 deletions base/indices.jl
Original file line number Diff line number Diff line change
Expand Up @@ -352,17 +352,14 @@ struct Slice{T<:AbstractUnitRange} <: AbstractUnitRange{Int}
end
Slice(S::Slice) = S
axes(S::Slice) = (IdentityUnitRange(S.indices),)
unsafe_indices(S::Slice) = (IdentityUnitRange(S.indices),)
axes1(S::Slice) = IdentityUnitRange(S.indices)
axes(S::Slice{<:OneTo}) = (S.indices,)
unsafe_indices(S::Slice{<:OneTo}) = (S.indices,)
axes1(S::Slice{<:OneTo}) = S.indices

first(S::Slice) = first(S.indices)
last(S::Slice) = last(S.indices)
size(S::Slice) = (length(S.indices),)
length(S::Slice) = length(S.indices)
unsafe_length(S::Slice) = unsafe_length(S.indices)
getindex(S::Slice, i::Int) = (@_inline_meta; @boundscheck checkbounds(S, i); i)
getindex(S::Slice, i::AbstractUnitRange{<:Integer}) = (@_inline_meta; @boundscheck checkbounds(S, i); i)
getindex(S::Slice, i::StepRange{<:Integer}) = (@_inline_meta; @boundscheck checkbounds(S, i); i)
Expand All @@ -383,17 +380,14 @@ end
IdentityUnitRange(S::IdentityUnitRange) = S
# IdentityUnitRanges are offset and thus have offset axes, so they are their own axes
axes(S::IdentityUnitRange) = (S,)
unsafe_indices(S::IdentityUnitRange) = (S,)
axes1(S::IdentityUnitRange) = S
axes(S::IdentityUnitRange{<:OneTo}) = (S.indices,)
unsafe_indices(S::IdentityUnitRange{<:OneTo}) = (S.indices,)
axes1(S::IdentityUnitRange{<:OneTo}) = S.indices

first(S::IdentityUnitRange) = first(S.indices)
last(S::IdentityUnitRange) = last(S.indices)
size(S::IdentityUnitRange) = (length(S.indices),)
length(S::IdentityUnitRange) = length(S.indices)
unsafe_length(S::IdentityUnitRange) = unsafe_length(S.indices)
getindex(S::IdentityUnitRange, i::Int) = (@_inline_meta; @boundscheck checkbounds(S, i); i)
getindex(S::IdentityUnitRange, i::AbstractUnitRange{<:Integer}) = (@_inline_meta; @boundscheck checkbounds(S, i); i)
getindex(S::IdentityUnitRange, i::StepRange{<:Integer}) = (@_inline_meta; @boundscheck checkbounds(S, i); i)
Expand Down Expand Up @@ -475,7 +469,7 @@ convert(::Type{LinearIndices{N,R}}, inds::LinearIndices{N}) where {N,R} =
# AbstractArray implementation
IndexStyle(::Type{<:LinearIndices}) = IndexLinear()
axes(iter::LinearIndices) = map(axes1, iter.indices)
size(iter::LinearIndices) = map(unsafe_length, iter.indices)
size(iter::LinearIndices) = map(length, iter.indices)
function getindex(iter::LinearIndices, i::Int)
@_inline_meta
@boundscheck checkbounds(iter, i)
Expand Down
2 changes: 1 addition & 1 deletion base/multidimensional.jl
Original file line number Diff line number Diff line change
Expand Up @@ -843,7 +843,7 @@ function _unsafe_getindex(::IndexStyle, A::AbstractArray, I::Vararg{Union{Real,
# This is specifically not inlined to prevent excessive allocations in type unstable code
shape = index_shape(I...)
dest = similar(A, shape)
map(unsafe_length, axes(dest)) == map(unsafe_length, shape) || throw_checksize_error(dest, shape)
map(length, axes(dest)) == map(length, shape) || throw_checksize_error(dest, shape)
_unsafe_getindex!(dest, A, I...) # usually a generated function, don't allow it to impact inference result
return dest
end
Expand Down
71 changes: 33 additions & 38 deletions base/range.jl
Original file line number Diff line number Diff line change
Expand Up @@ -585,9 +585,11 @@ end

## interface implementations

length(r::AbstractRange) = error("length implementation missing") # catch mistakes
size(r::AbstractRange) = (length(r),)

isempty(r::StepRange) =
# steprange_last_empty(r.start, r.step, r.stop) == r.stop
(r.start != r.stop) & ((r.step > zero(r.step)) != (r.stop > r.start))
isempty(r::AbstractUnitRange) = first(r) > last(r)
isempty(r::StepRangeLen) = length(r) == 0
Expand All @@ -614,66 +616,59 @@ julia> step(range(2.5, stop=10.9, length=85))
```
"""
step(r::StepRange) = r.step
step(r::AbstractUnitRange{T}) where{T} = oneunit(T) - zero(T)
step(r::AbstractUnitRange{T}) where {T} = oneunit(T) - zero(T)
step(r::StepRangeLen) = r.step
step(r::StepRangeLen{T}) where {T<:AbstractFloat} = T(r.step)
step(r::LinRange) = (last(r)-first(r))/r.lendiv

step_hp(r::StepRangeLen) = r.step
step_hp(r::AbstractRange) = step(r)

unsafe_length(r::AbstractRange) = length(r) # generic fallback

function unsafe_length(r::StepRange)
n = Integer(div((r.stop - r.start) + r.step, r.step))
isempty(r) ? zero(n) : n
end
length(r::StepRange) = unsafe_length(r)
unsafe_length(r::AbstractUnitRange) = Integer(last(r) - first(r) + step(r))
unsafe_length(r::OneTo) = Integer(r.stop - zero(r.stop))
length(r::AbstractUnitRange) = unsafe_length(r)
length(r::OneTo) = unsafe_length(r)
length(r::StepRangeLen) = r.len
length(r::LinRange) = r.len
axes(r::AbstractRange) = (oneto(length(r)),)

# Needed to fold the `firstindex` call in SimdLoop.simd_index
firstindex(::UnitRange) = 1
firstindex(::StepRange) = 1
firstindex(::LinRange) = 1

function length(r::StepRange{T}) where T<:Union{Int,UInt,Int64,UInt64,Int128,UInt128}
isempty(r) && return zero(T)
if r.step > 1
return checked_add(convert(T, div(unsigned(r.stop - r.start), r.step)), one(T))
elseif r.step < -1
return checked_add(convert(T, div(unsigned(r.start - r.stop), -r.step)), one(T))
elseif r.step > 0
return checked_add(div(checked_sub(r.stop, r.start), r.step), one(T))
else
return checked_add(div(checked_sub(r.start, r.stop), -r.step), one(T))
length(r::AbstractUnitRange) = Integer(last(r) - first(r) + step(r))
length(r::OneTo) = Integer(r.stop - zero(r.stop))
length(r::StepRangeLen) = r.len
length(r::LinRange) = r.len
length(r::StepRange) = Integer(div(r.stop - r.start + r.step, r.step))

# compile optimizations for which promote_type(T, Int) = T
let bigint = Union{Int,UInt,Int64,UInt64,Int128,UInt128}
global length
function length(r::StepRange{T}) where T<:bigint
step = r.step
diff = r.stop - r.start
step == 0 && return zero(T) # unreachable, by construction, but avoids the error case here later
isempty(r) && return zero(T)
if -1 <= step <= 1 || step == -step || step isa Unsigned # n.b. !(step isa T)
# if |step| > 1, diff might have overflowed, but unsigned(diff)÷step should
# therefore still be valid (if the result is representable at all)
return div(diff, step) % T + one(T)
elseif step < 0
return div(unsigned(-diff), -step) % T + one(T)
else
return div(unsigned(diff), step) % T + one(T)
end
end
end

function length(r::AbstractUnitRange{T}) where T<:Union{Int,Int64,Int128}
@_inline_meta
checked_add(checked_sub(last(r), first(r)), one(T))
function length(r::AbstractUnitRange{T}) where T<:bigint
@_inline_meta
return last(r) - first(r) + one(T) # even when isempty, by construction (with overflow)
end
length(r::OneTo{T}) where {T<:bigint} = r.stop
end
length(r::OneTo{T}) where {T<:Union{Int,Int64}} = T(r.stop)

length(r::AbstractUnitRange{T}) where {T<:Union{UInt,UInt64,UInt128}} =
r.stop < r.start ? zero(T) : checked_add(last(r) - first(r), one(T))

# some special cases to favor default Int type
let smallint = (Int === Int64 ?
Union{Int8,UInt8,Int16,UInt16,Int32,UInt32} :
Union{Int8,UInt8,Int16,UInt16})
global length

function length(r::StepRange{<:smallint})
isempty(r) && return Int(0)
div(Int(r.stop)+Int(r.step) - Int(r.start), Int(r.step))
end

length(r::StepRange{<:smallint}) = div(Int(r.stop) - Int(r.start) + r.step, r.step) # n.b. !(step isa T)
length(r::AbstractUnitRange{<:smallint}) = Int(last(r)) - Int(first(r)) + 1
length(r::OneTo{<:smallint}) = Int(r.stop)
end
Expand Down
10 changes: 5 additions & 5 deletions base/subarray.jl
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ viewindexing(I::Tuple{Vararg{Any}}) = IndexCartesian()
viewindexing(I::Tuple{AbstractArray, Vararg{Any}}) = IndexCartesian()

# Simple utilities
size(V::SubArray) = (@_inline_meta; map(unsafe_length, axes(V)))
size(V::SubArray) = (@_inline_meta; map(length, axes(V)))

similar(V::SubArray, T::Type, dims::Dims) = similar(V.parent, T, dims)

Expand Down Expand Up @@ -362,7 +362,7 @@ compute_stride1(parent::AbstractArray, I::NTuple{N,Any}) where {N} =
compute_stride1(s, inds, I::Tuple{}) = s
compute_stride1(s, inds, I::Tuple{Vararg{ScalarIndex}}) = s
compute_stride1(s, inds, I::Tuple{ScalarIndex, Vararg{Any}}) =
(@_inline_meta; compute_stride1(s*unsafe_length(inds[1]), tail(inds), tail(I)))
(@_inline_meta; compute_stride1(s*length(inds[1]), tail(inds), tail(I)))
compute_stride1(s, inds, I::Tuple{AbstractRange, Vararg{Any}}) = s*step(I[1])
compute_stride1(s, inds, I::Tuple{Slice, Vararg{Any}}) = s
compute_stride1(s, inds, I::Tuple{Any, Vararg{Any}}) = throw(ArgumentError("invalid strided index type $(typeof(I[1]))"))
Expand Down Expand Up @@ -407,12 +407,12 @@ end
function compute_linindex(f, s, IP::Tuple, I::Tuple{ScalarIndex, Vararg{Any}})
@_inline_meta
Δi = I[1]-first(IP[1])
compute_linindex(f + Δi*s, s*unsafe_length(IP[1]), tail(IP), tail(I))
compute_linindex(f + Δi*s, s*length(IP[1]), tail(IP), tail(I))
end
function compute_linindex(f, s, IP::Tuple, I::Tuple{Any, Vararg{Any}})
@_inline_meta
Δi = first(I[1])-first(IP[1])
compute_linindex(f + Δi*s, s*unsafe_length(IP[1]), tail(IP), tail(I))
compute_linindex(f + Δi*s, s*length(IP[1]), tail(IP), tail(I))
end
compute_linindex(f, s, IP::Tuple, I::Tuple{}) = f

Expand Down Expand Up @@ -447,5 +447,5 @@ _indices_sub(::Real, I...) = (@_inline_meta; _indices_sub(I...))
_indices_sub() = ()
function _indices_sub(i1::AbstractArray, I...)
@_inline_meta
(unsafe_indices(i1)..., _indices_sub(I...)...)
(axes(i1)..., _indices_sub(I...)...)
end
2 changes: 1 addition & 1 deletion stdlib/Dates/test/ranges.jl
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,7 @@ end
@test length(Dates.Year(1):Dates.Year(1):Dates.Year(10)) == 10
@test length(Dates.Year(10):Dates.Year(-1):Dates.Year(1)) == 10
@test length(Dates.Year(10):Dates.Year(-2):Dates.Year(1)) == 5
@test_throws OverflowError length(typemin(Dates.Year):Dates.Year(1):typemax(Dates.Year))
@test length(typemin(Dates.Year):Dates.Year(1):typemax(Dates.Year)) == 0 # overflow
@test_throws MethodError Dates.Date(0):Dates.DateTime(2000)
@test_throws MethodError Dates.Date(0):Dates.Year(10)
@test length(range(Dates.Date(2000), step=Dates.Day(1), length=366)) == 366
Expand Down
60 changes: 35 additions & 25 deletions test/ranges.jl
Original file line number Diff line number Diff line change
Expand Up @@ -499,19 +499,28 @@ end
@test length(1:4:typemax(Int)) == div(typemax(Int),4) + 1

@testset "overflow in length" begin
Tset = Int === Int64 ? (Int,UInt,Int128,UInt128) :
(Int,UInt,Int64,UInt64,Int128, UInt128)
Tset = Int === Int64 ? (Int, UInt, Int128, UInt128) :
(Int, UInt, Int64, UInt64, Int128, UInt128)
for T in Tset
@test_throws OverflowError length(zero(T):typemax(T))
@test_throws OverflowError length(typemin(T):typemax(T))
@test_throws OverflowError length(zero(T):one(T):typemax(T))
@test_throws OverflowError length(typemin(T):one(T):typemax(T))
@test length(zero(T):typemax(T)) == typemin(T)
@test length(typemin(T):typemax(T)) == T(0)
@test length(zero(T):one(T):typemax(T)) == typemin(T)
@test length(typemin(T):one(T):typemax(T)) == T(0)
@test length(one(T):typemax(T)) == typemax(T)
if T <: Signed
@test_throws OverflowError length(-one(T):typemax(T)-one(T))
@test_throws OverflowError length(-one(T):one(T):typemax(T)-one(T))
@test length(-one(T):typemax(T)-one(T)) == typemin(T)
@test length(-one(T):one(T):typemax(T)-one(T)) == typemin(T)
@test length(-one(T):typemax(T)) == typemin(T) + T(1)
@test length(zero(T):typemin(T):typemin(T)) == 2
@test length(one(T):typemin(T):typemin(T)) == 2
@test length(typemax(T):typemin(T):typemin(T)) == 2
@test length(-one(T):typemin(T):typemin(T)) == 1
@test length(zero(T):typemin(T):zero(T)) == 1
@test length(zero(T):typemin(T):one(T)) == 0
end
end
end

@testset "loops involving typemin/typemax" begin
n = 0
s = 0
Expand Down Expand Up @@ -866,17 +875,18 @@ end
end
# issue #2959
@test 1.0:1.5 == 1.0:1.0:1.5 == 1.0:1.0
#@test 1.0:(.3-.1)/.1 == 1.0:2.0
@test_broken 1.0:(.3-.1)/.1 == 1.0:2.0 # (this is just shy of 2.0)

@testset "length with typemin/typemax" begin
let r = typemin(Int64):2:typemax(Int64), s = typemax(Int64):-2:typemin(Int64)
let r = typemin(Int64):2:typemax(Int64)
@test first(r) == typemin(Int64)
@test last(r) == (typemax(Int64)-1)
@test_throws OverflowError length(r)

@test last(r) == typemax(Int64) - 1
@test length(r) == typemin(Int64)
end
let s = typemax(Int64):-2:typemin(Int64)
@test first(s) == typemax(Int64)
@test last(s) == (typemin(Int64)+1)
@test_throws OverflowError length(s)
@test last(s) == typemin(Int64) + 1
@test length(s) == typemin(Int64)
end

@test length(typemin(Int64):3:typemax(Int64)) == 6148914691236517206
Expand All @@ -892,6 +902,7 @@ end
@test length(typemax(UInt):UInt(2):(typemax(UInt)-1)) == 0
@test length((typemin(Int)+3):5:(typemin(Int)+1)) == 0
end

# issue #6364
@test length((1:64)*(pi/5)) == 64

Expand Down Expand Up @@ -1042,13 +1053,9 @@ end
@test reverse(LinRange{Int}(0,3,4)) === LinRange{Int}(3,0,4)
@test reverse(LinRange{Float64}(0.,3.,4)) === LinRange{Float64}(3.,0.,4)
end
@testset "Issue #11245" begin
io = IOBuffer()
show(io, range(1, stop=2, length=3))
str = String(take!(io))
# @test str == "range(1.0, stop=2.0, length=3)"
@test str == "1.0:0.5:2.0"
end

# issue #11245
@test repr(range(1, stop=2, length=3)) == "1.0:0.5:2.0"

@testset "issue 10950" begin
r = 1//2:3
Expand Down Expand Up @@ -1262,8 +1269,11 @@ end
end

r = 1f8-10:1f8
@test_broken argmin(f) == argmin(collect(r))
@test_broken argmax(f) == argmax(collect(r))
rv = collect(r)
@test argmin(r) == argmin(rv) == 1
@test r[argmax(r)] == r[argmax(rv)] == 1f8
@test argmax(r) == lastindex(r)
@test argmax(rv) != lastindex(r)
end

@testset "OneTo" begin
Expand Down Expand Up @@ -1515,7 +1525,7 @@ Base.Unsigned(x::Displacement) = Unsigned(x.val)
Base.rem(x::Displacement, y::Displacement) = Displacement(rem(x.val, y.val))
Base.div(x::Displacement, y::Displacement) = Displacement(div(x.val, y.val))

# required for collect (summing lengths); alternatively, should unsafe_length return Int by default?
# required for collect (summing lengths); alternatively, should length return Int by default?
Base.promote_rule(::Type{Displacement}, ::Type{Int}) = Int
Base.convert(::Type{Int}, x::Displacement) = x.val

Expand Down
Loading

0 comments on commit 6c549a9

Please sign in to comment.