Fix nested IdOffsetRange constructor (#185)

* Fix nested IdOffsetRange constructor

* fix for 2D views

* fix compute_offset1

* add type assertions to convert

* Fix comment

Author: jishnub

Project.toml

@@ -1,6 +1,6 @@
 name = "OffsetArrays"
 uuid = "6fe1bfb0-de20-5000-8ca7-80f57d26f881"
-version = "1.5.0"
+version = "1.5.1"
 
 [deps]
 Adapt = "79e6a3ab-5dfb-504d-930d-738a2a938a0e"

docs/make.jl

@@ -1,6 +1,8 @@
 using Documenter, JSON
 using OffsetArrays
 
+DocMeta.setdocmeta!(OffsetArrays, :DocTestSetup, :(using OffsetArrays); recursive=true)
+
 makedocs(
     sitename = "OffsetArrays",
     format = Documenter.HTML(prettyurls = get(ENV, "CI", nothing) == "true"),

src/axes.jl

@@ -78,31 +78,38 @@ struct IdOffsetRange{T<:Integer,I<:AbstractUnitRange{T}} <: AbstractUnitRange{T}
     offset::T
 
     IdOffsetRange{T,I}(r::I, offset::T) where {T<:Integer,I<:AbstractUnitRange{T}} = new{T,I}(r, offset)
+    
+    #= This method is necessary to avoid a StackOverflowError in IdOffsetRange{T,I}(r::IdOffsetRange, offset::Integer). 
+    The type signature in that method is more specific than IdOffsetRange{T,I}(r::I, offset::T), 
+    so it ends up calling itself if I <: IdOffsetRange. 
+    =#
+    function IdOffsetRange{T,IdOffsetRange{T,I}}(r::IdOffsetRange{T,I}, offset::T) where {T<:Integer,I<:AbstractUnitRange{T}}
+        new{T,IdOffsetRange{T,I}}(r, offset)
+    end
 end
 
 # Construction/coercion from arbitrary AbstractUnitRanges
 function IdOffsetRange{T,I}(r::AbstractUnitRange, offset::Integer = 0) where {T<:Integer,I<:AbstractUnitRange{T}}
     rc, o = offset_coerce(I, r)
-    return IdOffsetRange{T,I}(rc, convert(T, o+offset))
+    return IdOffsetRange{T,I}(rc, convert(T, o+offset)::T)
 end
 function IdOffsetRange{T}(r::AbstractUnitRange, offset::Integer = 0) where T<:Integer
-    rc = convert(AbstractUnitRange{T}, r)
-    return IdOffsetRange{T,typeof(rc)}(rc, convert(T, offset))
+    rc = convert(AbstractUnitRange{T}, r)::AbstractUnitRange{T}
+    return IdOffsetRange{T,typeof(rc)}(rc, convert(T, offset)::T)
 end
 IdOffsetRange(r::AbstractUnitRange{T}, offset::Integer = 0) where T<:Integer =
-    IdOffsetRange{T,typeof(r)}(r, convert(T, offset))
+    IdOffsetRange{T,typeof(r)}(r, convert(T, offset)::T)
 
 # Coercion from other IdOffsetRanges
 IdOffsetRange{T,I}(r::IdOffsetRange{T,I}) where {T<:Integer,I<:AbstractUnitRange{T}} = r
 function IdOffsetRange{T,I}(r::IdOffsetRange, offset::Integer = 0) where {T<:Integer,I<:AbstractUnitRange{T}}
     rc, offset_rc = offset_coerce(I, r.parent)
-    return IdOffsetRange{T,I}(rc, r.offset + offset + offset_rc)
+    return IdOffsetRange{T,I}(rc, convert(T, r.offset + offset + offset_rc)::T)
 end
 function IdOffsetRange{T}(r::IdOffsetRange, offset::Integer = 0) where T<:Integer
     return IdOffsetRange{T}(r.parent, r.offset + offset)
 end
 IdOffsetRange(r::IdOffsetRange) = r
-IdOffsetRange(r::IdOffsetRange, offset::Integer) = typeof(r)(r.parent, offset + r.offset)
 
 # TODO: uncomment these when Julia is ready
 # # Conversion preserves both the values and the indexes, throwing an InexactError if this
@@ -123,12 +130,12 @@ end
 
 # Fallback, specialze this method if `convert(I, r)` doesn't do what you need
 offset_coerce(::Type{I}, r::AbstractUnitRange) where I<:AbstractUnitRange =
-    convert(I, r), 0
+    convert(I, r)::I, 0
 
 @inline Base.parent(r::IdOffsetRange) = r.parent
 @inline Base.axes(r::IdOffsetRange) = (Base.axes1(r),)
 @inline Base.axes1(r::IdOffsetRange) = IdOffsetRange(Base.axes1(r.parent), r.offset)
-@inline Base.unsafe_indices(r::IdOffsetRange) = (r,)
+@inline Base.unsafe_indices(r::IdOffsetRange) = (Base.axes1(r),)
 @inline Base.length(r::IdOffsetRange) = length(r.parent)
 Base.reduced_index(i::IdOffsetRange) = typeof(i)(first(i):first(i))
 # Workaround for #92 on Julia < 1.4
@@ -176,7 +183,7 @@ if VERSION < v"1.5.2"
     # issue 100, 133: IdOffsetRange as another index-preserving case shouldn't comtribute offsets
     # fixed by https://github.com/JuliaLang/julia/pull/37204
     @inline Base.compute_offset1(parent, stride1::Integer, dims::Tuple{Int}, inds::Tuple{IdOffsetRange}, I::Tuple) =
-        Base.compute_linindex(parent, I) - stride1*first(inds[1])
+        Base.compute_linindex(parent, I) - stride1*first(Base.axes1(inds[1]))
 end
 
 # This was deemed "too private" to extend: see issue #184

src/utils.jl

@@ -73,4 +73,4 @@ function _checkindices(N::Integer, indices, label)
 end
 
 _unwrap(r::IdOffsetRange) = r.parent .+ r.offset
-_unwrap(r::IdentityUnitRange) = r.indices
+_unwrap(r::IdentityUnitRange) = r.indices

test/runtests.jl

@@ -9,6 +9,8 @@ using EllipsisNotation
 using Adapt
 using StaticArrays
 
+DocMeta.setdocmeta!(OffsetArrays, :DocTestSetup, :(using OffsetArrays); recursive=true)
+
 # https://github.com/JuliaLang/julia/pull/29440
 if VERSION < v"1.1.0-DEV.389"
     Base.:(:)(I::CartesianIndex{N}, J::CartesianIndex{N}) where N =
@@ -22,6 +24,14 @@ struct TupleOfRanges{N}
     x ::NTuple{N, UnitRange{Int}}
 end
 
+function same_value(r1, r2)
+    length(r1) == length(r2) || return false
+    for (v1, v2) in zip(r1, r2)
+        v1 == v2 || return false
+    end
+    return true
+end
+
 @testset "Project meta quality checks" begin
     # Not checking compat section for test-only dependencies
     Aqua.test_all(OffsetArrays; project_extras=true, deps_compat=true, stale_deps=true, project_toml_formatting=true)
@@ -31,13 +41,7 @@ end
 end
 
 @testset "IdOffsetRange" begin
-    function same_value(r1, r2)
-        length(r1) == length(r2) || return false
-        for (v1, v2) in zip(r1, r2)
-            v1 == v2 || return false
-        end
-        return true
-    end
+    
     function check_indexed_by(r, rindx)
         for i in rindx
             r[i]
@@ -98,8 +102,16 @@ end
     @test same_value(r, 3:5)
     check_indexed_by(r, 3:5)
 
-    r = IdOffsetRange(IdOffsetRange(3:5, 2), 1)
-    @test parent(r) isa UnitRange
+    rp = Base.OneTo(3)
+    r = IdOffsetRange(rp)
+    r2 = IdOffsetRange{Int,typeof(r)}(r, 1)
+    @test same_value(r2, 2:4)
+    check_indexed_by(r2, 2:4)
+
+    r2 = IdOffsetRange{Int32,IdOffsetRange{Int32,Base.OneTo{Int32}}}(r, 1)
+    @test typeof(r2) == IdOffsetRange{Int32,IdOffsetRange{Int32,Base.OneTo{Int32}}}
+    @test same_value(r2, 2:4)
+    check_indexed_by(r2, 2:4)
 
     # conversion preserves both the values and the axes, throwing an error if this is not possible
     @test @inferred(oftype(ro, ro)) === ro
@@ -876,6 +888,35 @@ end
     @test S[0, 2, 2] == A[0, 4, 2]
     @test S[1, 1, 2] == A[1, 3, 2]
     @test axes(S) == (OffsetArrays.IdOffsetRange(0:1), Base.OneTo(2), OffsetArrays.IdOffsetRange(2:5))
+
+    # issue #186
+    a = reshape(1:12, 3, 4)
+    r = OffsetArrays.IdOffsetRange(3:4)
+    av = view(a, :, r)
+    @test av == a[:, 3:4]
+    @test axes(av) == (axes(a,1), axes(r,1))
+    r = OffsetArrays.IdOffsetRange(1:2,2)
+    av = view(a, :, r)
+    @test no_offset_view(av) == a[:, 3:4]
+    @test axes(av) == (axes(a,1), axes(r,1))
+    r = OffsetArrays.IdOffsetRange(2:3)
+    av1d = view(a, r, 3)
+    @test av1d == a[2:3, 3]
+    @test axes(av1d) == (axes(r,1),)
+    r = OffsetArrays.IdOffsetRange(Base.OneTo(2), 1)
+    av1d = view(a, r, 3)
+    @test no_offset_view(av1d) == a[2:3, 3]
+    @test axes(av1d) == (axes(r,1),)
+
+    # fix IdOffsetRange(::IdOffsetRange, offset) nesting from #178
+    b = 1:20
+    bov = OffsetArray(view(b, 3:4), 3:4)
+    c = @view b[bov]
+    @test same_value(c, 3:4)
+    @test axes(c,1) == 3:4
+    d = OffsetArray(c, 1:2)
+    @test same_value(d, c)
+    @test axes(d,1) == 1:2
 end
 
 @testset "iteration" begin