Delete unnecessary convert() method for OrderedDict

[JamesWrigley]

This causes invalidations and already has a definition in Base: https://github.com/JuliaLang/julia/blob/8c145c1e0e4676ce57a32d7125cae05a2ece3ac6/base/abstractdict.jl#L577

Fixes these invalidations:

 inserting convert(::Type{OrderedCollections.OrderedDict{K, V}}, d::AbstractDict) where {K, V} @ OrderedCollections ~/.julia/packages/OrderedCollections/Xihhq/src/ordered_dict.jl:104 invalidated:                                            
   backedges: 1: superseding convert(::Type{T}, x::T) where T<:AbstractDict @ Base abstractdict.jl:575 with MethodInstance for convert(::Type{T}, ::T) where T<:AbstractDict (3 children)                                                      
              2: superseding convert(::Type{T}, x::AbstractDict) where T<:AbstractDict @ Base abstractdict.jl:577 with MethodInstance for convert(::Type{<:AbstractDict}, ::AbstractDict) (470 children)                                       

The original loop seems to be quite old so I don't think we'll miss anything by removing it. Would it be possible to make a release with this?

[JamesWrigley]

(bump)

[JamesWrigley]

(bump)

[JamesWrigley]

@fingolfin, @Tokazama, apologies for the direct ping, but would it be possible to review this? 🙏

[Tokazama]

Are we worried about that depwarn disappearing? If it doesn't break code I think this is great

[JamesWrigley]

Good point, I'm not sure about the depwarn 🤔 One could argue that since the conversion is defined in Base we should support it too, as long as there's no technical reason not to.

[Tokazama]

I guess the question is if we still care about associative containers being sorted before conversion. Our direct constructors consume unordered associated containers, so I'm not sure why people decided convert was the place to draw the line. The commit for this warning is 9 years old. @cstjean wrote that commit, but I don't see any reasoning for this and doubt he remembers all of his code over a decade.

We don't have this warning/error tested, good documentation for why it's there, or any reason to believe its absence will cause errors downstream. On top of that it looks like it resolves a good chunk of invalidations. I'd say add a minor version bump and we release it. Unfortunately, I'm back on the hospital floors full time in a couple days, so I'd like someone else to be following this repo for issues over the next couple months in case some problems arise.

[JamesWrigley]

I just enabled notifications for the repo, I can commit to fixing any issues from this in the near future 👍

[cstjean]

Thank you for the ping. The original discussion is here. I stand by the general point that non-deterministic behaviour (like conversion from Dict to OrderedDict) is a footgun in Julia, and that to the extent possible it's good to avoid it.

I won't mind too much if this definition is taken out, but personally I would vote against.

I'm curious how this causes invalidation. This package owns OrderedCollections.OrderedDict, so what code gets invalidated?

[JamesWrigley]

I see 🤔 The reason it causes invalidations is because the package defines a more specific method of convert() for dicts that overrides the previous default in Base. Because of world-splitting that causes Julia to recompile methods calling convert() that it could previously assume would always hit the Base default.

[cstjean]

That's too bad. Thank you for looking around for invalidations, it's important work. I don't know how else that could be tackled. Should some of that logic be moved to Base? If it's easy, can you give an example of code that gets invalidated? If the code is type stable, then it shouldn't get invalidated no?

An OrderedDict has an ordering. AFAIC converting an unordered dict into an ordered one should be a MethodError, just like you can't meaningfully convert a Date into a DateTime object.

Our direct constructors consume unordered associated containers,

That's a bug to me. OrderedDict's entire raison d'etre is to have a meaningful order. If you don't care about the order, why are you not using a plain Dict?

[cstjean]

A pragmatic solution to this issue might be to change convert(::Type{OrderedDict{K,V}}, d::AbstractDict) into convert(::Type{OrderedDict{K,V}}, d::Dict). Obviously it's "not quite as general", but as far as I'm concerned it's good enough, at a lower complexity cost. I think it would resolve the invalidation problem?

BTW I saw JuliaLang/julia#61329, and I think it's great (though I'm not quite sure how it would help here)

[JamesWrigley]

A pragmatic solution to this issue might be to change convert(::Type{OrderedDict{K,V}}, d::AbstractDict) into convert(::Type{OrderedDict{K,V}}, d::Dict).

Sadly that did not fix it 🥲 I think because it's still inserting a new method that 'overlaps' with AbstractDict. I don't think there's a good way to fix it without modifying Base unfortunately, so I'll close this.

BTW I saw JuliaLang/julia#61329, and I think it's great (though I'm not quite sure how it would help here)

Yeah it's not related, just wanted to give you a heads-up since it would allow changing OrderedCollections.