Add keytype+valtype method for AbstractArray and AbstractVector

[tkluck]

The function keytype(a) returns eltype(keys(a)) for associative structures; it should do the same for AbstractArray and AbstractVector. Looks like this was overlooked(?) in #25577.

#25104 is a related pull request, but renaming is less urgent than adding expected behaviour.

[Keno]

eltype(keys(a))

Wouldn't literally this definition be a better generic fallback and cover these cases?

[andyferris]

Agree that keytype should work but @Keno’s suggestion seems pretty fool-proof to me.

I think one stumbling block for arrays in the past is that they have at least two kinds of keys (linear and Cartesian) plus whatever we call it when you can getindex with an (arbitrarily long) varargs of integers. But I think of the definitions here as the “real” keys, and the others are “convenience” keys. But maybe that’s just me.

[tkluck]

eltype(keys(a))

Wouldn't literally this definition be a better generic fallback and cover these cases?

Here's what made me not do that: keys may be implemented by doing an allocation. For example, it returns a KeySet for Dict. In this particular case, the compiler may be able to optimize it out, but that may not be true for user-defined containers.

And we'd need to implement it case-by-case for keytype on the type anyway, as we can't call keys on the type. (I now realize this pull request doesn't address that yet and it probably should.)

julia> d = Dict(1=>2)
Dict{Int64,Int64} with 1 entry:
  1 => 2

julia> keytype(d), keytype(typeof(d))
(Int64, Int64)

julia> a = [1 2]
1×2 Array{Int64,2}:
 1  2

julia> keytype(a), keytype(typeof(a))
ERROR: MethodError: no method matching keytype(::Type{Array{Int64,2}})
Closest candidates are:
  keytype(::AbstractArray{T,1} where T) at abstractarray.jl:100
  keytype(::AbstractArray) at abstractarray.jl:99
  keytype(::Type{#s56} where #s56<:AbstractDict{K,V}) where {K, V} at abstractdict.jl:237
  ...
Stacktrace:
 [1] top-level scope at none:0

julia> keytype(a)
CartesianIndex{2}

Thoughts?

[andyferris]

I feel the fallback can be allowed to have allocations, whether or not the compler can elide them. If we have specific performance problems with Base types we can simply add some specializations.

And we'd need to implement it case-by-case for keytype on the type anyway, as we can't call keys on the type.

This is more convincing.

[tkluck]

Reviving this pull request as I recently found myself reaching for this. Merged back master (still a clean merge) and added valtype.

[mbauman]

Let's make the type-based methods the "canonical" endpoints so a custom type would only need to override that one (instead of both).

[tkluck]

All feedback is now incorporated, including the needs-* labels. The CI failures seem to be about dependency artifacts and not related to this PR.

[tkluck]

Oooh let's get this in 1.2: https://discourse.julialang.org/t/julia-1-2-feature-freeze-april-4th/22478

@mbauman or @andyferris , do either of you have commit rights? Or should we bring in someone else?

[StefanKarpinski]

It appears that this no longer needs NEWS or tests but still needs docs with compatibility annotation indicating that it was added in version 1.2. If @mbauman approves, we can merge. It would be great to get the docs with this PR but if time is an issue if you promise to add docs after the feature freeze...

[mbauman]

Thanks @tkluck! This looks great to me now. Let's merge once tests pass.

[tkluck]

@StefanKarpinski good catch, apologies. (FWIW, I (mis)understood that that label was related to the Compat.jl package, and that it would need the specific commit that introduced this feature -- i.e. that it wasn't actionable until this was merged.)

Just pushed those docs with a compatibility note.

[StefanKarpinski]

Looks great! I made a suggestion to change the valtype example to use a string array since it seemed more obviously different than the types of the keys of an array which could be Int32 on some systems.

[tkluck]

Travis uncovers failing

keytype_is_correct(Int64(1):Int64(5))

on 32-bits architecture. I'll check.

[tkluck]

buildbot is happy now, including the new test cases introduced by this pull request. Appveyor and travis complain about unrelated things and are broken on master too.

I say, good to go :)

[StefanKarpinski]

Good to go as soon as CI passes. Please squash when merging!