Constrain UnitRanges to integer types

[jishnub]

Change UnitRange to UnitRange{<:Integer} in the function signature of fill, zeros, ones, trues and falses, keeping in mind that OffsetArrays accept only integer offsets.

Fixes #117

[codecov]

Codecov Report

Merging #118 into master will decrease coverage by 1.78%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##           master     #118      +/-   ##
==========================================
- Coverage   83.69%   81.91%   -1.79%     
==========================================
  Files           2        2              
  Lines         184      188       +4     
==========================================
  Hits          154      154              
- Misses         30       34       +4     

Impacted Files	Coverage Δ
src/OffsetArrays.jl	85.90% <57.14%> (-0.59%)	⬇️
src/axes.jl	66.66% <0.00%> (-5.56%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7294f3d...c93688b. Read the comment docs.

[johnnychen94]

Not sure if this is wanted, I believe the main reason for this loose type annotation is for consistency to the Base implementation

Although it seems not the case according to the test report, adding such constrain "might" introduce incremental compilations and method ambiguities when collaborating with other packages(not only Base), so I prefer to get a practical need first(e.g., when any particular downstream package raises such need), and then make such changes.

This will also allow other packages to define these methods for various other UnitRange types.

Do you have an example of this to see how things could be work together?

[jishnub]

I see. I was thinking of OffsetArrays as a fallback type if arrays are indexed using types that are supersets of Integers, eg. HalfIntegers. For example, this is the behavior that I'm looking for

fill(x, UnitRange{HalfInt}, UnitRange{HalfInt}) -> Array indexed by HalfInts
fill(x, UnitRange{HalfInt}, UnitRange{Int}) -> Array indexed by HalfInts
fill(x, UnitRange{Int}, UnitRange{HalfInt}) -> Array indexed by HalfInts
fill(x, UnitRange{Int}, UnitRange{Int}) -> OffsetArray indexed by Ints (defined in OffsetArrays, external to the package)

This is an extension of the scheme followed in OffsetArrays, where

fill(x, UnitRange{Int}, UnitRange{Int}) -> OffsetArray indexed by Ints
fill(x, Int, UnitRange{Int}) -> OffsetArray indexed by Ints
fill(x, UnitRange{Int}, Int) -> OffsetArray indexed by Ints
fill(x, Int, Int) -> Array indexed by Ints (defined in Base, external to the package)

Getting this behavior to work requires me to dispatch on Tuple{Vararg{Union{UnitRange{HalfInt},UnitRange{Int}}}}. This however doesn't work as OffsetArrays dispatches on Tuple{UnitRange,Vararg{UnitRange}} which is more specific.

[timholy]

For the record, I think this is a very good change. I am a bit worried about the very point that @johnnychen94 raises. Have you played with it extensively on your own machine? Does it introduce any problems?

[jishnub]

This is potentially breaking if one defines a type that subtypes Real but behaves like an integer. Consider something like this:

module CustomInt

struct MyInt <: Real
   x :: Int
end

MyInt(m::MyInt) = m

Base.Int(m::MyInt) = m.x
Base.Integer(m::MyInt) = m.x

Base.:(<=)(m::MyInt, n::MyInt) = m.x <= n.x

Base.:(+)(m::MyInt, n::MyInt) = MyInt(m.x + n.x)
Base.:(-)(m::MyInt, n::MyInt) = MyInt(m.x - n.x)

Base.round(m::MyInt, ::RoundingMode) = m

Base.promote_rule(::Type{MyInt}, ::Type{Int}) = Int

end

Now, on master:

julia> r = UnitRange(CustomInt.MyInt(2), CustomInt.MyInt(3))
Main.CustomInt.MyInt(2):Main.CustomInt.MyInt(3)

julia> zeros(r)
2-element OffsetArray(::Array{Float64,1}, 2:3) with eltype Float64 with indices 2:3:
 0.0
 0.0

while after this PR

julia> r = UnitRange(CustomInt.MyInt(2), CustomInt.MyInt(3))
Main.CustomInt.MyInt(2):Main.CustomInt.MyInt(3)

julia> zeros(r)
ERROR: MethodError: no method matching zeros(::Type{Float64}, ::Tuple{UnitRange{Main.CustomInt.MyInt}})

However this does seem to be a good direction to proceed in, as it limits the extent of type-piracy in this package. The question now is -- should we consider this for OffsetArrays v2.0? I'm not sure about invalidations and incremental compilations, and would like to hear more about it.