Concerning `AbstractDict` inheritance and `push!`, is Base wrong?

I am implementing an struct that inherit AbstractDict.
To reduce the effort needed for a first working implementation I decided to let many methods to be defined by the fallback methods in abstracdict.jl.

push! would be one of these methods, as it is defined in terms of setindex! at line 479. However, seems to be some kind of inconsistency in the three definitions in lines 479–481 (quoted below).

push!(t::AbstractDict, p::Pair) = setindex!(t, p.second, p.first)
push!(t::AbstractDict, p::Pair, q::Pair) = push!(push!(t, p), q)
push!(t::AbstractDict, p::Pair, q::Pair, r::Pair...) = push!(push!(push!(t, p), q), r...)

If I call push!(d, 1 => 2.0) (where d is an instance of my type) it does not fail, but if I call push!(d, 1 => 2.0, 3 => 4.0) for example, it gives:

ERROR: MethodError: no method matching push!(::Float64, ::Pair{Int64,Float64})

What was kind of an strange error (I was sure d is not a Float64), but if we look at line 480 above, it is defined as:

push!(t::AbstractDict, p::Pair, q::Pair) = push!(push!(t, p), q)

So it expects push! to return the AbstractDict/collection to chain the two push! calls but it defines the push! for just one pair one line above as:

push!(t::AbstractDict, p::Pair) = setindex!(t, p.second, p.first)

Which used setindex!, and setindex! by convention do not return the indexable collection but the new value inserted/updated (otherwise all code that does something like a[1] = a[2] = 0 would fail).

As push! for Dicts, Vectors, and everything else returns the collection itself (and not the newly pushed value), and the definitions of lines 480 and 481 also seem to assume so, is the fallback definition of push! for a single pair in abstractdict.jl wrong? Should not it be:

function push!(t::AbstractDict, p::Pair)
  setindex!(t, p.second, p.first)
  t
end

This is not true,

julia> a = rand(5);

julia> b = setindex!(a, 1, 1);

julia> typeof(b)
Array{Float64,1}

The lowering for a[1] = 0 is not only to a setindex! call, cf:

julia> Meta.lower(Main, :(a[1] = x))
:($(Expr(:thunk, CodeInfo(
1 ─     (Base.setindex!)(a, x, 1)
└──     return x
))))
4 Likes

Ouch, this lowering caught me off guard. Now things make sense, my code has errors because I define my setindex! based on this incorrect assumption I had. I really not know how this has escaped me, as the manual is irreproachable:

setindex!(collection, value, key...)
Store the given value at the given key or index within a collection. The syntax a[i,j,...] = x is converted by the compiler to (setindex!(a, x, i, j, ...); x) .

Sorry for the noise.