Inference relies on eltype(::AbstractArray) being correct

[martinholters]

Demo:

julia> using DataArrays

julia> da = @data([1, NA])
WARNING: Array{T}(::Type{T}, m::Int) is deprecated, use Array{T}(m) instead.
# irrelevant stackstrace omitted
2-element DataArrays.DataArray{Int64,1}:
 1  
  NA

julia> foo(xs::Any...) = -1
foo (generic function with 1 method)

julia> foo(xs::Int...) = [0]
foo (generic function with 2 methods)

julia> bar(xs) = sum(foo(xs...))
bar (generic function with 1 method)

julia> bar(da)

signal (11): Segmentation fault
while loading no file, in expression starting on line 0
mapreduce_impl at ./reduce.jl:173
# stacktrace going through jl_call_method_internal and jl_apply_generic

Why is that?

julia> using DataArrays

julia> da = @data([1, NA])
2-element DataArrays.DataArray{Int64,1}:
 1  
  NA

julia> eltype(da)
Int64

julia> da[2] isa eltype(da)
false

But inference relies on splatting an instance of T<:AbstractArray giving a Vararg{eltype(T)}. I.e. inference determines that foo(::Int...) would be called in the example, inferring a return type of Vector{Int}, but the actual call goes to foo(::Any...), returning an Int instead. Thus the wrong sum specialization gets called.

Now what?

Possible ways forward:

1. Clearly document with a big exclamation mark that eltype must not lie. (Or probably even better, that one has to subtype the correct AbstractArray, i.e. AbstractArray{Union{DataArrays.NAtype, Int64}} for the above example.) Go and fix DataArrays.
2. Make splatting assert that all arguments obtained from splatting an AbstractArray are in fact of the expected type. Would throw TypeError: typeassert: expected Int64, got DataArrays.NAtype or similar for the example above.
3. Always treat a splatted AbstractArray as a Vararg{Any}, ignore its eltype.
4. Use ordinary type inference to figure out the possible types after splatting.

My opinion: I don't like 1. I can't judge 2; I think the behavior would be ok, but I have no idea what an implementation would look like in terms of code complexity and run-time cost. 3. would be an easy fix, but at the cost of worsening inference precision. My favorite is 4, although I'm not sure it's actually feasible with a reasonable amount of code and extra time for inference.

Thoughts?

[JeffBezanson]

We should just make the inference code more conservative. It can use eltype for array types in base, and otherwise Any.

[vtjnash]

Maybe we should merge #12579?

[martinholters]

Ah, yes, #12579 is exactly what I meant with option 4. Thanks for the pointer!

[martinholters]

It can use eltype for array types in base, and otherwise Any.

In practice, restricting this to Array would probably be good enough. (Note: "Array types in base" is too broad; view-like types would have to be excluded, unless recursively checking their parents, too.) Esthetically, giving built-in types a better treatment than external ones is not very pleasing.