Disallow overflow for empty ranges in the OffsetArray constructor (#226)

* Disallow overflow in the OffsetArray constructor for empty ranges

* Overflow check in summing offsets in the nested OffsetArray constructor

Author: jishnub

src/OffsetArrays.jl

@@ -117,7 +117,7 @@ struct OffsetArray{T,N,AA<:AbstractArray{T,N}} <: AbstractArray{T,N}
     end
 end
 
-function OffsetArray{T, N, AA}(parent::AA, offsets::NTuple{N, <:Integer}) where {T, N, AA<:AbstractArray{T,N}}
+function OffsetArray{T, N, AA}(parent::AA, offsets::NTuple{N, Integer}) where {T, N, AA<:AbstractArray{T,N}}
     OffsetArray{T, N, AA}(parent, map(x -> convert(Int, x)::Int, offsets))
 end
 
@@ -135,16 +135,34 @@ Type alias and convenience constructor for two-dimensional [`OffsetArray`](@ref)
 """
 const OffsetMatrix{T,AA<:AbstractMatrix{T}} = OffsetArray{T,2,AA}
 
-function overflow_check(r, offset::T) where T
+# checks if the offset may be added to the range without overflowing
+function overflow_check(r::AbstractUnitRange{T}, offset::T) where T<:Integer
     # This gives some performance boost https://github.com/JuliaLang/julia/issues/33273
-    throw_upper_overflow_error() = throw(OverflowError("Boundary overflow detected: offset should be <= $(typemax(T) - last(r)) for offsets of type $T, received $offset"))
-    throw_lower_overflow_error() = throw(OverflowError("Boundary overflow detected: offset should be >= $(typemin(T) - first(r)) for offsets of type $T, received $offset"))
+    throw_upper_overflow_error(val) = throw(OverflowError("offset should be <= $(typemax(T) - val) corresponding to the axis $r, received an offset $offset"))
+    throw_lower_overflow_error(val) = throw(OverflowError("offset should be >= $(typemin(T) - val) corresponding to the axis $r, received an offset $offset"))
 
-    if offset > 0 && last(r) > typemax(T) - offset
+    # With ranges in the picture, first(r) might not necessarily be < last(r)
+    # we therefore use the min and max of first(r) and last(r) to check for overflow
+    firstlast_min, firstlast_max = minmax(first(r), last(r))
+
+    if offset > 0 && firstlast_max > typemax(T) - offset
+        throw_upper_overflow_error(firstlast_max)
+    elseif offset < 0 && firstlast_min < typemin(T) - offset
+        throw_lower_overflow_error(firstlast_min)
+    end
+    return nothing
+end
+# checks if the two offsets may be added together without overflowing
+function overflow_check(offset1::T, offset2::T) where {T<:Integer}
+    throw_upper_overflow_error() = throw(OverflowError("offset should be <= $(typemax(eltype(offset1)) - offset1) given a pre-existing offset of $offset1, received an offset $offset2"))
+    throw_lower_overflow_error() = throw(OverflowError("offset should be >= $(typemin(eltype(offset1)) - offset1) given a pre-existing offset of $offset1, received an offset $offset2"))
+
+    if offset1 > 0 && offset2 > typemax(T) - offset1
         throw_upper_overflow_error()
-    elseif offset < 0 && first(r) < typemin(T) - offset
+    elseif offset1 < 0 && offset2 < typemin(T) - offset1
         throw_lower_overflow_error()
     end
+    return nothing
 end
 
 # Tuples of integers are treated as offsets
@@ -170,10 +188,16 @@ end
 ## OffsetArray constructors
 for FT in (:OffsetArray, :OffsetVector, :OffsetMatrix)
     # Nested OffsetArrays may strip off the wrapper and collate the offsets
-    @eval @inline function $FT(A::OffsetArray, offsets::Tuple{Vararg{Integer}})
+    # empty tuples are handled here
+    @eval @inline function $FT(A::OffsetArray, offsets::Tuple{Vararg{Int}})
         _checkindices(A, offsets, "offsets")
+        # ensure that the offsets may be added together without an overflow
+        foreach(overflow_check, A.offsets, offsets)
         $FT(parent(A), map(+, A.offsets, offsets))
     end
+    @eval @inline function $FT(A::OffsetArray, offsets::Tuple{Integer,Vararg{Integer}})
+        $FT(A, map(x -> convert(Int, x)::Int, offsets))
+    end
 
     # In general, indices get converted to AbstractUnitRanges.
     # CartesianIndices{N} get converted to N ranges

test/runtests.jl

@@ -321,6 +321,14 @@ Base.convert(::Type{Int}, a::WeirdInteger) = a
             @test axes(a) === (IdOffsetRange(Base.OneTo(4), 0), )
         end
 
+        # nested OffsetVectors
+        for inds in Any[offsets, offsets_big]
+            a = OffsetVector{Float64}(undef, inds)
+            b = OffsetVector(a, inds); b2 = OffsetVector(a, inds...);
+            @test eltype(b) === eltype(b2) === Float64
+            @test axes(b, 1) === axes(b2, 1) === IdOffsetRange(Base.OneTo(4), 4)
+        end
+
         # offset indexing
         for inds in offset_axes
             # test offsets
@@ -385,6 +393,8 @@ Base.convert(::Type{Int}, a::WeirdInteger) = a
         @test_nowarn OffsetArray{Float64, 1, typeof(ao)}(ao, (-1, ))
         @test_throws OverflowError OffsetArray{Float64, 1, typeof(ao)}(ao, (-2, )) # inner Constructor
         @test_throws OverflowError OffsetArray(ao, (-2, )) # convinient constructor accumulate offsets
+        @test_throws OverflowError OffsetVector(1:0, typemax(Int))
+        @test_throws OverflowError OffsetVector(OffsetVector(1:0, 0), typemax(Int))
 
         @testset "OffsetRange" begin
             local r = 1:100
@@ -471,6 +481,15 @@ Base.convert(::Type{Int}, a::WeirdInteger) = a
         end
         @test_throws Union{ArgumentError, ErrorException} OffsetMatrix{Float64}(undef, 2, -2) # only positive numbers works
 
+        # nested OffsetMatrices
+        for inds in Any[offsets, offsets_big]
+            a = OffsetMatrix{Float64}(undef, inds)
+            b = OffsetMatrix(a, inds); b2 = OffsetMatrix(a, inds...);
+            @test eltype(b) === eltype(b2) === Float64
+            @test axes(b, 1) === axes(b2, 1) === IdOffsetRange(Base.OneTo(4), 4)
+            @test axes(b, 2) === axes(b2, 2) === IdOffsetRange(Base.OneTo(3), 3)
+        end
+
         for inds in offset_axes
             # test offsets
             a = OffsetMatrix{Float64}(undef, inds)
@@ -578,6 +597,15 @@ Base.convert(::Type{Int}, a::WeirdInteger) = a
         @test OffsetArray(a, 0:1, :,  CartesianIndices(()), 2:3) == OffsetArray(a, 0:1, :, 2:3)
         @test OffsetArray(a, 0:1, :, 2:3, CartesianIndices(())) == OffsetArray(a, 0:1, :, 2:3)
 
+        # nested OffsetArrays
+        for offsets in [(1,1,1), big.((1,1,1))]
+            ob = OffsetArray(oa, offsets); ob2 = OffsetArray(oa, offsets...);
+            @test eltype(ob) === eltype(ob2) === Float64
+            @test axes(ob, 1) === axes(ob2, 1) === IdOffsetRange(Base.OneTo(2), 0)
+            @test axes(ob, 2) === axes(ob2, 2) === IdOffsetRange(Base.OneTo(2), 3)
+            @test axes(ob, 3) === axes(ob2, 3) === IdOffsetRange(Base.OneTo(2), 2)
+        end
+
         indices = (-1:1, -7:7, -1:2, -5:5, -1:1, -3:3, -2:2, -1:1)
         y = OffsetArray{Float64}(undef, indices...);
         @test axes(y) === axes(OffsetArray{Float64}(undef, indices))