Unpirate Union{}[]

[tkf]

Union{}[] does not work after I import StaticArrays (cb46c2d) due to the new SA[...] constructor:

julia> Union{}[]
0-element Array{Union{},1}

julia> using StaticArrays

julia> Union{}[]
ERROR: ArgumentError: Union{} does not have elements
Stacktrace:
 [1] eltype(::Type{Union{}}) at ./array.jl:124
 [2] similar_type(::Type{Union{}}, ::Size{(0,)}) at /home/takafumi/.julia/dev/StaticArrays/src/abstractarray.jl:66
 [3] getindex(::Core.TypeofBottom) at /home/takafumi/.julia/dev/StaticArrays/src/initializers.jl:29
 [4] top-level scope at REPL[3]:1

This PR fixes it by overloading type piracy by yet another type piracy.

(Though I'm not sure if this is technically a type piracy. If you can't overload anything that can accept Type{Union{}}, you can't write Type{<:MyType}. But then getindex(::Type{Union{}}) = ... just totally looks like a type piracy...)

[andyferris]

I guess this is the correct way to do this?

Although would Vector{Union{}}() be clearer and avoid a run-time invoke?

[tkf]

It seems invoke(getindex, Tuple{Type}, Union{}) and Vector{Union{}}() are equivalent in terms of codegen:

julia> @inline f() = invoke(getindex, Tuple{Type}, Union{})
f (generic function with 1 method)

julia> @inline g() = Vector{Union{}}()
g (generic function with 1 method)

julia> @code_typed f()
CodeInfo(
1 ─ %1 = $(Expr(:foreigncall, :(:jl_alloc_array_1d), Array{Union{},1}, svec(Any, Int64), 0, :(:ccall), Array{Union{},1}, 0, 0))::Array{Union{},1}
└──      return %1
) => Array{Union{},1}

julia> @code_typed g()
CodeInfo(
1 ─ %1 = $(Expr(:foreigncall, :(:jl_alloc_array_1d), Array{Union{},1}, svec(Any, Int64), 0, :(:ccall), Array{Union{},1}, 0, 0))::Array{Union{},1}
└──      return %1
) => Array{Union{},1}

I personally prefer invoke(getindex, Tuple{Type}, Union{}) as it avoids us to refer to the implementation detail (Base can in principle do something different).

[tkf]

I should mention that importing StaticArrays still alters the type of error thrown by Union{}[1].

Before importing StaticArrays:

julia> Union{}[1]
ERROR: MethodError: convert(::Type{Union{}}, ::Int64) is ambiguous. Candidates:
  convert(::Type{Union{}}, x) in Base at essentials.jl:168
  convert(::Type{T}, x::Number) where T<:Number in Base at number.jl:7
  convert(::Type{T}, arg) where T<:VecElement in Base at baseext.jl:8
  convert(::Type{T}, x::Number) where T<:AbstractChar in Base at char.jl:179
Possible fix, define
  convert(::Type{Union{}}, ::Number)
Stacktrace:
 [1] setindex!(::Array{Union{},1}, ::Int64, ::Int64) at ./array.jl:796
 [2] getindex(::Type{Union{}}, ::Int64) at ./array.jl:366
 [3] top-level scope at REPL[1]:1

After importing StaticArrays (of this PR):

julia> Union{}[1]
ERROR: ArgumentError: Union{} does not have elements
Stacktrace:
 [1] eltype(::Type{Union{}}) at ./array.jl:124
 [2] similar_type(::Type{Union{}}, ::Size{(1,)}) at /home/takafumi/.julia/dev/StaticArrays/src/abstractarray.jl:66
 [3] getindex(::Core.TypeofBottom, ::Int64) at /home/takafumi/.julia/dev/StaticArrays/src/initializers.jl:30
 [4] top-level scope at REPL[14]:1

Typically exception type is not in the API domain so I'm not sure if we need to care, though.

[c42f]

Oh wow this is nasty :-/ Good catch.

I'm not sure this is the right way to go about fixing this. Adding a lower bound seems like it might work instead?

@inline Base.getindex(sa::Type{T}, xs...) where {E, SA{E}<:T<:SA} = similar_type(sa, Size(length(xs)))(xs)

We will need fixes for typed_vcat and typed_hcat too I guess to avoid these cases going into the static arrays code and changing the error...

[tkf]

Thanks, that's much more elegant.

[coveralls]

Coverage increased (+0.03%) to 81.718% when pulling 39fc862 on tkf:union into cb46c2d on JuliaArrays:master.

[c42f]

Looks good, cheers! I think we can merge this, though I added a suggested comment because having a lower bound is extremely unusual.

The fact that this was a problem at all is kind of concerning. I've written a lot of foo(::Type{<:X}) before.

[andyferris]

What about SA precisely?

[c42f]

There's nothing special about SA... It's just that ::Type{<:SA} is considered more specific to matching Union{} than a generic ::Type{T}:

julia> foo(::Type{T}) where {T} = "generic"
julia> foo(::Type{<:Int}) = "special"
julia> foo(Union{})
"special"

[c42f]

somewhat disturbing eh?

[andyferris]

Yes, I get that part.

I can see the above is fine for SA{Int64}[1,2,3]. But what about vanilla SA[1,2,3]? We don't have SA{E} <: SA for any (invariant) E, do we?

[c42f]

Oh right, I didn't understand your question. You're asking why T === SA works here?

We do have SA{E} <: SA for any choice of E, but then E doesn't have a definite value which is kind of weird. Actually this kind of thing has caused the inliner to fail in the past (see #665) for some reason.

The normalization of the following is also doing my head in a bit

julia> T where SA{E} <: T <: SA where E
SA

[c42f]

Uh oh. It turns out that the compiler (inliner?) doesn't like this implementation:

julia> foo() = SA[1,2,3]
foo (generic function with 1 method)

julia> @code_typed optimize=true foo()
CodeInfo(
1 ─ %1 = Base.getindex(Main.SA, 1, 2, 3)::Core.Compiler.Const([1, 2, 3], false)
└──      return %1
) => SArray{Tuple{3},Int64,1,3}

Strangely enough, inference seems to have worked really well and constant propagated everything, but the getindex call is still there. What.

[tkf]

I rollback to invoke-based solution.

[c42f]

I still feel bad about having a special case defined for Union{}, so I tweaked this another way by duplicating the function signatures. Does that look ok to you?

The lower bound optimization issue seems related to JuliaLang/julia#30713

[tkf]

Thanks! That's much better.

[tkf]

I just noticed another approach was

Type₁(U) = Type{T} where {P, U{P} <: T <: U}
@inline Base.getindex(sa::Type₁(SA), xs...) = similar_type(sa, Size(length(xs)))(xs)

Inliner does seem to work with this.

Maybe it's nice to have something like Type₊(U::UnionAll) that generalize Type₁ to arbitrary number of parameters. I think it makes sense to add it to Base as (AFAIU) it's not possible to implement this without referring to the internal representation of UnionAll.

[tkf]

I opened an issue here: JuliaLang/julia#33780

[c42f]

Thanks, excellent.

By the way do you need this fix urgently? We could release a 0.12.1 more or less immediately if that helps.

[tkf]

Thanks, but it's not urgent. I just excluded 0.12 in the projects that hits this bug.

But now that the registry is automated maybe you can go with more frequency release cycle?

[c42f]

Yes, I think I'll release soon anyway. I thought @oxinabox's blog post on continuous delivery had some good points (most especially for bug fixes) https://white.ucc.asn.au/2019/09/28/Continuous-Delivery-For-Julia-Packages.html though it would be nice to be a tad more automated.

I like your Type₁ solution from JuliaLang/julia#33780. I wanted to do something like that but forgot to include the Type on the inside of the UnionAll and got confused.

[tkf]

I think that's a great ideal. But I can see that you maybe need to be a bit more conservative for "almost-Base" package like StaticArrays.jl. I guess it'd be nice to do some reverse testing to make CD safer. JuliaCIBot was one of the rare things it was better before 1.0 in Julia....

[c42f]

Yeah, well 0.12.1 is very conservatively just patching this particular issue so I just released it.