Support different subtypes of AbstractDict in merge()/mergewith()

[JamesWrigley]

The merge functions internally use the _typeddict() helper function, which returns a new Dict with the correct promoted key/value types. But the fact that it always returns a Dict means that the default implementations of merge()/mergewith() cannot be used for new AbstractDict subtypes that wish those functions to return their own custom type.

For example, OrderedDict: https://github.com/JuliaCollections/OrderedCollections.jl/blob/1eaa98084bd5c6e1f6519527c8f9e856a2fd2047/src/ordered_dict.jl#L488
And on 1.13 nightly that causes a ton of invalidations when loaded through CurveFit.jl:

julia> using SnoopCompileCore

julia> invs, trees = begin
           invs = @snoop_invalidations using CurveFit
           using SnoopCompile, AbstractTrees
           trees = invalidation_trees(invs)
           invs, trees
       end;

julia> trees[end]
inserting merge(d::OrderedCollections.OrderedDict, others::AbstractDict...) @ OrderedCollections ~/.julia/dev/OrderedCollections/src/ordered_dict.jl:488 invalidated:
   mt_backedges: <38 invalidations I deleted because they have enormous types>
   backedges: 1: superseding merge(d::AbstractDict, others::AbstractDict...) @ Base abstractdict.jl:359 with MethodInstance for merge(::AbstractDict, ::Base.Pairs{Symbol, Union{}, Nothing, @NamedTuple{}}) (21 children)
              2: superseding merge(d::AbstractDict, others::AbstractDict...) @ Base abstractdict.jl:359 with MethodInstance for merge(::AbstractDict, ::Base.Pairs{Symbol, _A, Nothing, A} where {_A, A<:NamedTuple}) (2489 children)

By allowing the Base functions to work with other dict types we could delete the methods from OrderedCollections and fix the invalidations. But, this is definitely breaking. A non-breaking alternative would be to just set max_methods for merge()/mergewith() to 1. I think the arguments for doing it anyway are:

1. Intuitively, merge(::MyDict, ::MyDict) -> MyDict is the behaviour I would expect with a custom dict. It's odd that this is supported for any AbstractDict but always returns a Dict, which is more like the behaviour of collect().
2. The docstrings for merge()/mergewith() do not specify the type of the return value so it's kinda-sorta legal to change it. Admittedly this is a weak argument, we would probably need to do a PkgEval to see how breaking it actually is.
3. Supporting other dict types in Base allows more code reuse. Other packages currently need to implement their own type-promotion for keys/values to implement merge()/mergewith() properly.

Hardcoding Dict was done years ago, I suspect unintentionally: ebbf1dd
Calling T.name.wrapper to get the original type is to preserve this optimization: #22737

(CC @cstjean)

[vtjnash]

This is quite dangerous, since T.name is not a stable result and name.wrapper is not a public API and the result of apply_type {K, V} is user-defined, and does not make a dictionary of K => V

[JamesWrigley]

I couldn't find another way of doing it, is there a better way?

[JamesWrigley]

...the result of apply_type {K, V} is user-defined, and does not make a dictionary of K => V

Do you mean if a constructor returns something other than its struct type? I would consider that a bug in user code. We can assume that AbstractDict subtypes have at least two type parameters since that's part of the definition:

julia/base/essentials.jl

Line 34 in 393f698

abstract type AbstractDict{K,V} end

FWIW, an example of having more than two type parameters is SortedDict: https://github.com/JuliaCollections/DataStructures.jl/blob/06eb26e60e9d8dd01dd5f416722994a783ba5ae8/src/sorted_dict.jl#L50

[JamesWrigley]

Hmm though I see SortedDict also does some optimizations for constructors and would likely need to keep its custom merge functions anyway, which is a point in favour of just setting max_methods to 1 🤔

[vtjnash]

No, that is a faulty assumption. Many subtypes do not have parameters, or have more or have them in other orders

[JamesWrigley]

Right, so I guess the requirement is that they have a constructor that does take in key/value type parameters. That is more invasive than I thought :( I still think it would be an improvement but I'm not sure if it's too breaking.

[vtjnash]

IIRC, this is spelled empty(d, K, V)?

[JamesWrigley]

IIRC, this is spelled empty(d, K, V)?

That is indeed what I initially did, but that would require an extra merge!() call, which would hurt performance: #22737

[JamesWrigley]

I'll close this since the invalidations issue is fixed by #61330.