remove eltype piracy from SortedDict

[vtjnash]

No description provided.

[StephenVavasis]

Could someone explain how this is type piracy? The offending statements refer to types that are defined in this module, namely, SortedDict, etc., so they would not affect someone else's eltype.

[vtjnash]

The meaning of eltype is defined for AbstractDict already, which this declaring itself a subtype of, then pirating the definitions for it

[KristofferC]

I think the argument is that you don't own Type.

[vtjnash]

You could own some of Type{<:YourObject}, but you don't own any of Type{<:AbstractDict}, since that is already owned by the interface for AbstractDict.

[StephenVavasis]

Could you give me an example how this declaration could cause problems for some other code? I tried below to cause trouble unsuccessfully.

julia> VERSION
v"1.8.2"

julia> using DataStructures

julia> s = SortedDict("a" => 4)
SortedDict{String, Int64, Base.Order.ForwardOrdering} with 1 entry:
  "a" => 4

julia> eltype(s)
Pair{String, Int64}

julia> @which eltype(s)
eltype(m::SortedDict{K, D, Ord}) where {K, D, Ord<:Base.Order.Ordering} in DataStructures at C:\Users\vavasis\.julia\packages\DataStructures\59MD0\src\sorted_dict.jl:280

julia> struct C{S,T} <: AbstractDict{S,T}
       end

julia> Base.iterate(::C{S,T}, state=0) where S where T = nothing

julia> v = C{String,Int}()
C{String, Int64}()

julia> eltype(v)
Pair{String, Int64}

julia> @which eltype(v)
eltype(x) in Base at abstractarray.jl:206

julia> eltype(C{String,Int})
Pair{String, Int64}

julia> @which eltype(C{String,Int})
eltype(::Type{<:AbstractDict{K, V}}) where {K, V} in Base at abstractdict.jl:479

[KristofferC]

I think one issue is that all code that has previously inferred eltype(::Type{<:AbstractDict{S,T}}) needs to recompile since this method could have returned something different.

[vtjnash]

Correct. You are force it to inject an inference barrier here, so that the compiler can handle the piracy. It knows how to keep working, even though it will cost users performance after it is pushed off the fast path.

[timholy]

Are any of those eltype/keytype/valtype methods required? Doesn't

mutable struct SortedDict{K, D, Ord <: Ordering} <: AbstractDict{K,D}
    bt::BalancedTree23{K,D,Ord}
end

mean that everything would work if you deleted all six of them? (I.e., deleting even the ones that Jameson didn't delete.)

A reason to consider that is that defining new eltype etc. methods may in principle invalidate other code; if you would have gotten the same answer without defining those methods, you're far, far better off deleting them.

[vtjnash]

Looks like there is another similar one on values, if you want to continue fixing similar issues to this:

 inserting values(ba::DataStructures.SortedDict) @ DataStructures ~/.julia/packages/DataStructures/59MD0/src/container_loops.jl:245 invalidated:
   mt_backedges: 1: signature values(a::AbstractDict) @ Base abstractdict.jl:131 (formerly values(a::AbstractDict) @ Base abstractdict.jl:131) triggered MethodInstance for CSV.Context(::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg) (0 children)

 inserting values(ct::DataStructures.Accumulator) @ DataStructures ~/.julia/packages/DataStructures/59MD0/src/accumulator.jl:60 invalidated:
   mt_backedges: 1: signature values(a::AbstractDict) @ Base abstractdict.jl:131 (formerly values(a::AbstractDict) @ Base abstractdict.jl:131) triggered MethodInstance for CSV.Context(::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg, ::CSV.Arg) (0 children)

(Tim is also right, but I previously left them because they cause fewer problems. But it still would make sense to delete them also and DRY the code.)

[StephenVavasis]

Thanks for the explanation-- I understand better now. Yes, all of those can be deleted. You can also get rid of the eltype in sorted_set.jl for the same reason (inherited from AbstractSet).

[chriselrod]

Can this be merged?

[StephenVavasis]

Before this is merged, is the OP okay with making a similar edit to sorted_set.jl in order to fix the problem more thoroughly?

[vtjnash]

I don't generally have a ton of time to improve packages unfortunately, since I am spread rather thin among a lot of them

[andyferris]

a similar edit to sorted_set.jl in order to fix the problem more thoroughly

FYI I made an attempt on this in #863.