Fix no_offset_view for Slice (#376)

Currently, the implementation of `no_offset_view` for a `Slice` doesn't
actually ensure that there is no offset. This is because
`Base.Slice(UnitRange(a))` has `UnitRange(a)` as its axes, which are
usually offset. This PR changes the behavior to return a `UnitRange(a)`
instead.

Fixes #375 

The flip side of the current implementation is that the `Slice`
information is lost, so `IndexStyle` of 2D views might become
`IndexCartesian` once they are routed through `no_offset_view`.

```julia
julia> arr = OffsetArray(reshape(1:15, 3, 5), 2, 3)
3×5 OffsetArray(reshape(::UnitRange{Int64}, 3, 5), 3:5, 4:8) with eltype Int64 with indices 3:5×4:8:
 1  4  7  10  13
 2  5  8  11  14
 3  6  9  12  15

julia> V = view(arr, :, :)
3×5 view(OffsetArray(reshape(::UnitRange{Int64}, 3, 5), 3:5, 4:8), :, :) with eltype Int64 with indices 3:5×4:8:
 1  4  7  10  13
 2  5  8  11  14
 3  6  9  12  15

julia> IndexStyle(V)
IndexLinear()

julia> V_nooff = OffsetArrays.no_offset_view(V)
3×5 view(OffsetArray(reshape(::UnitRange{Int64}, 3, 5), 3:5, 4:8), 3:5, 4:8) with eltype Int64:
 1  4  7  10  13
 2  5  8  11  14
 3  6  9  12  15

julia> IndexStyle(V_nooff)
IndexCartesian()
```

An alternate approach might be to shift the parent to one-based indices,
and use `Base.Slice{Base.OneTo{Int}}` axes, which preserve this
information. This, however, departs from the current approach of
preserving the `parentindices` of the `SubArray`.

Edit: the alternate approach is implemented now.

Author: jishnub

src/OffsetArrays.jl

@@ -693,19 +693,60 @@ julia> A
 no_offset_view(A::OffsetArray) = no_offset_view(parent(A))
 if isdefined(Base, :IdentityUnitRange)
     # valid only if Slice is distinguished from IdentityUnitRange
-    no_offset_view(a::Base.Slice{<:Base.OneTo}) = a
-    no_offset_view(a::Base.Slice) = Base.Slice(UnitRange(a))
-    no_offset_view(S::SubArray) = view(parent(S), map(no_offset_view, parentindices(S))...)
+    _onebasedslice(S::Base.Slice) = Base.Slice(Base.OneTo(length(S)))
+    _onebasedslice(S::Base.Slice{<:Base.OneTo}) = S
+    _onebasedslice(S) = S
+    _isoffsetslice(::Any) = false
+    _isoffsetslice(::Base.Slice) = true
+    _isoffsetslice(::Base.Slice{<:Base.OneTo}) = false
+    function no_offset_view(S::SubArray)
+        #= If a view contains an offset Slice axis,
+        i.e. it is a view of an offset array along the offset axis,
+        we shift the axis to a 1-based one.
+        E.g. Slice(2:3) -> Slice(Base.OneTo(2))
+        We transform the `parent` as well as the `parentindices`,
+        so that the view still points to the same elements, even though the indices have changed.
+        This way, we retain the axis of the view as a `Slice`
+        =#
+        P = parent(S)
+        pinds = parentindices(S)
+        #=
+        Check if all the axes are `Slice`s and the parent has `OneTo` axes,
+        in which case we may unwrap the `OffsetArray` and forward the view to the parent.
+        =#
+        may_pop_parent = all(_isoffsetslice, pinds) && P isa OffsetArray && all(x -> x isa Base.OneTo, axes(parent(P)))
+        if may_pop_parent
+            return no_offset_view(P)
+        end
+        #=
+        we convert offset `Slice`s to 1-based ones using `_onebasedslice`.
+        The next call, `no_offset_view`, is a no-op on a `Slice{<:OneTo}`,
+        while it converts the offset axes to 1-based ones.
+        Eventually, we end up with a `Tuple` comprising `Slice{<:OneTo}`s and other 1-based axes.
+
+        The difference between `_onebasedslice` and `no_offset_view` is that
+        the latter does not change the value of the range, while the former does.
+        =#
+        newviewinds = map(no_offset_view ∘ _onebasedslice, pinds)
+        needs_shifting = any(_isoffsetslice, pinds)
+        P_maybeshiftedinds = if needs_shifting
+            t = Origin(parent(S)).index
+            neworigin = ntuple(i -> _isoffsetslice(pinds[i]) ? 1 : t[i], length(t))
+            Origin(neworigin)(P)
+        else
+            P
+        end
+        view(P_maybeshiftedinds, newviewinds...)
+    end
 end
 no_offset_view(a::Array) = a
 no_offset_view(i::Number) = i
 no_offset_view(A::AbstractArray) = _no_offset_view(axes(A), A)
 _no_offset_view(::Tuple{}, A::AbstractArray{T,0}) where T = A
 _no_offset_view(::Tuple{Base.OneTo, Vararg{Base.OneTo}}, A::AbstractArray) = A
-# the following method is needed for ambiguity resolution
-_no_offset_view(::Tuple{Base.OneTo, Vararg{Base.OneTo}}, A::AbstractUnitRange) = A
-_no_offset_view(::Any, A::AbstractArray) = OffsetArray(A, Origin(1))
-_no_offset_view(::Any, A::AbstractUnitRange) = UnitRange(A)
+_no_offset_view(::Any, A::AbstractArray) = _no_offset_view(A)
+_no_offset_view(A::AbstractArray) = OffsetArray(A, Origin(1))
+_no_offset_view(A::AbstractUnitRange) = UnitRange(A)
 
 #####
 # center/centered

test/runtests.jl

@@ -2367,6 +2367,12 @@ Base.getindex(x::PointlessWrapper, i...) = x.parent[i...]
     @test OffsetArrays.no_offset_view(V) == collect(V)
     V = @view O[:,:]
     @test IndexStyle(A) == IndexStyle(O) == IndexStyle(V) == IndexStyle(OffsetArrays.no_offset_view(V)) == IndexLinear()
+
+    @testset "issue #375" begin
+        arr = OffsetArray(reshape(1:15, 3, 5), 2, 3)
+        arr_no_offset = OffsetArrays.no_offset_view(@view arr[:, 4])
+        @test all(!Base.has_offset_axes, axes(arr_no_offset))
+    end
 end
 
 @testset "no nesting" begin