Type piracy breaks Base.similar on JuMP containers

[CasBex]

Hi, I've seen that type piracy has already been discussed in the issues here (#87). This issue is to highlight a specific case which is bugging me. I will refrain from philosophizing on #87 here.

julia> import Pkg; Pkg.activate("/home/cas/tmp/bugreport_OffsetArrays.jl/")
  Activating project at `~/tmp/bugreport_OffsetArrays.jl`

julia> import Pkg

julia> Pkg.status()
Status `~/tmp/bugreport_OffsetArrays.jl/Project.toml`
  [4076af6c] JuMP v1.23.5
  [6fe1bfb0] OffsetArrays v1.15.0

julia> using JuMP

julia> tmp = Containers.DenseAxisArray(rand(2, 3), 1:2, 0.0:0.1:0.2)
2-dimensional DenseAxisArray{Float64,2,...} with index sets:
    Dimension 1, 1:2
    Dimension 2, 0.0:0.1:0.2
And data, a 2×3 Matrix{Float64}:
 0.859161  0.444293  0.328422
 0.277883  0.582098  0.209645

julia> similar(tmp[:, 0.0])
1-dimensional DenseAxisArray{Float64,1,...} with index sets:
    Dimension 1, 1:2
And data, a 2-element Vector{Float64}:
 6.91582553623834e-310
 6.91581336938644e-310

julia> using OffsetArrays

julia> similar(tmp[:, 0.0])
ERROR: MethodError: similar(::JuMP.Containers.DenseAxisArray{Float64, 1, Tuple{…}, Tuple{…}}, ::Type{Float64}, ::Tuple{UnitRange{…}}) is ambiguous.

Candidates:
  similar(A::JuMP.Containers.DenseAxisArray{T, N, Ax, L} where L<:NTuple{N, JuMP.Containers._AxisLookup}, ::Type{S}, axes::Ax) where {T, N, Ax<:(Tuple{var"#s31"} where var"#s31"<:(AbstractVector)), S}
    @ JuMP.Containers ~/.julia/packages/JuMP/i68GU/src/Containers/DenseAxisArray.jl:282
  similar(A::AbstractArray, ::Type{T}, shape::Tuple{Union{Integer, AbstractUnitRange}, Vararg{Union{Integer, AbstractUnitRange}}}) where T
    @ OffsetArrays ~/.julia/packages/OffsetArrays/HLmxQ/src/OffsetArrays.jl:320

Possible fix, define
  similar(::JuMP.Containers.DenseAxisArray{…} where L<:NTuple{…}, ::Type{…}, ::Tuple{…}) where {…}

Stacktrace:
 [1] similar(a::JuMP.Containers.DenseAxisArray{Float64, 1, Tuple{…}, Tuple{…}}, ::Type{Float64})
   @ Base ./abstractarray.jl:821
 [2] similar(a::JuMP.Containers.DenseAxisArray{Float64, 1, Tuple{UnitRange{…}}, Tuple{JuMP.Containers._AxisLookup{…}}})
   @ Base ./abstractarray.jl:820
 [3] top-level scope
   @ REPL[8]:1
Some type information was truncated. Use `show(err)` to see complete types.

julia> show(err)
1-element ExceptionStack:
MethodError: similar(::JuMP.Containers.DenseAxisArray{Float64, 1, Tuple{UnitRange{Int64}}, Tuple{JuMP.Containers._AxisLookup{Tuple{Int64, Int64}}}}, ::Type{Float64}, ::Tuple{UnitRange{Int64}}) is ambiguous.

Candidates:
  similar(A::JuMP.Containers.DenseAxisArray{T, N, Ax, L} where L<:NTuple{N, JuMP.Containers._AxisLookup}, ::Type{S}, axes::Ax) where {T, N, Ax<:(Tuple{var"#s31"} where var"#s31"<:(AbstractVector)), S}
    @ JuMP.Containers ~/.julia/packages/JuMP/i68GU/src/Containers/DenseAxisArray.jl:282
  similar(A::AbstractArray, ::Type{T}, shape::Tuple{Union{Integer, AbstractUnitRange}, Vararg{Union{Integer, AbstractUnitRange}}}) where T
    @ OffsetArrays ~/.julia/packages/OffsetArrays/HLmxQ/src/OffsetArrays.jl:320

Possible fix, define
  similar(::JuMP.Containers.DenseAxisArray{T, N, Ax, L} where L<:NTuple{N, JuMP.Containers._AxisLookup}, ::Type{T}, ::Tuple{AbstractUnitRange}) where {T, T, N, Ax<:(Tuple{var"#s31"} where var"#s31"<:(AbstractVector))}

Stacktrace:
 [1] similar(a::JuMP.Containers.DenseAxisArray{Float64, 1, Tuple{UnitRange{Int64}}, Tuple{JuMP.Containers._AxisLookup{Tuple{Int64, Int64}}}}, ::Type{Float64})
   @ Base ./abstractarray.jl:821
 [2] similar(a::JuMP.Containers.DenseAxisArray{Float64, 1, Tuple{UnitRange{Int64}}, Tuple{JuMP.Containers._AxisLookup{Tuple{Int64, Int64}}}})
   @ Base ./abstractarray.jl:820
 [3] top-level scope
   @ REPL[8]:1
julia> 

The JuMP implementation for the Base.similar(a::JuMP.Containers.DenseAxisArray, ::Type, axes) seems pretty reasonable to me (pasted below) but breaks due to the type piracy in this package.

# We specify `Ax` for the type of `axes` to avoid conflict where `axes` has type
# `Tuple{Vararg{Int,N}}`.
function Base.similar(
    A::DenseAxisArray{T,N,Ax},
    ::Type{S},
    axes::Ax,
) where {T,N,Ax<:Tuple{<:AbstractVector},S}
    return construct_undef_array(S, axes)
end

# Avoid conflict with method defined in Julia Base when the axes of the
# `DenseAxisArray` are all `Base.OneTo`:
function Base.similar(
    ::DenseAxisArray{T,N,Ax},
    ::Type{S},
    axes::Ax,
) where {T,N,Ax<:Tuple{Base.OneTo,Vararg{Base.OneTo}},S}
    return construct_undef_array(S, axes)
end

[aplavin]

MethodError: similar(...) is ambiguous.

Not defending type piracy here, but ambiguity errors like this can arise without any piracy at all. And this happens regularly!
For a recent example, see
https://github.com/JuliaArrays/StructArrays.jl/blob/master/ext/StructArraysStaticArraysExt.jl#L43-L45
that we had to add to StructArrays to avoid similar and reshape ambiguity with StaticArrays. Neither package pirates reshape.

[CasBex]

I would argue that StaticArrays is doing some fishy stuff with the type unions there. I don't want to go into the rabbit hole of getting the exact type piracy definition as mentioned in JuliaArrays/StaticArrays.jl#1248 where you could say that the wider Union does not affect dispatch in Base.

It seems reasonable to me that neither of below should be defined in package A:

function packageB.func(data::packageC.sometype) end
function packageB.func(data::Union{packageA.sometype,packageC.sometype}) end
function packageB.func(data::packageD.sometype{T}) where {T<:packageC.sometype} end

[aplavin]

It seems reasonable to me that neither of below should be defined in package A: <...>

Yes, as I said I'm not defending piracy here :)

[jishnub]

Type-piracy aside, the method defined in JuMP should define the method for AbstractUnitRanges anyway.

function Base.similar(
    A::DenseAxisArray{T,N,Ax},
    ::Type{S},
    axes::Ax,
-) where {T,N,Ax<:Tuple{<:AbstractVector},S}
+) where {T,N,Ax<:Tuple{AbstractUnitRange{<:Integer}},S}
    return construct_undef_array(S, axes)
end

These are the only types valid as axes of an AbstractArray. Incidentally, this would also avoid the method ambiguity seen here.

[CasBex]

I think the point of the DenseAxisArray in JuMP is that you can have more flexible indices than just the AbstractUnitRange (see their documentation). At that point the DenseAxisArray becomes a regular Array I guess?

[pulsipher]

Adding to the conversation, I have also hit this issue trying to use Interpolations.jl (which requires OffestArrays) with JuMP. @CasBex is correct that simply adding AbstractUnitRange to JuMP is not sufficient since it is more general and can accept arbitrary axis types. It is clear that OffsetArrays is committing type piracy.