Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
JamesWrigley wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions base/abstractdict.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,10 +406,10 @@ promoteK(K) = K
promoteV(V) = V
promoteK(K, d, ds...) = promoteK(promote_type(K, keytype(d)), ds...)
promoteV(V, d, ds...) = promoteV(promote_type(V, valtype(d)), ds...)
function _typeddict(d::AbstractDict, others::AbstractDict...)
function _typeddict(d::T, others::AbstractDict...) where {T <: AbstractDict}
K = promoteK(keytype(d), others...)
V = promoteV(valtype(d), others...)
Dict{K,V}(d)
T.name.wrapper{K, V}(d)
Copy link
Copy Markdown
Member

@vtjnash vtjnash Mar 15, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@JamesWrigley JamesWrigley Mar 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@JamesWrigley JamesWrigley Mar 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...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

Copy link
Copy Markdown
Member Author

@JamesWrigley JamesWrigley Mar 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔

Copy link
Copy Markdown
Member

@vtjnash vtjnash Mar 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@JamesWrigley JamesWrigley Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end

"""
Expand Down
4 changes: 2 additions & 2 deletions test/dict.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1349,8 +1349,8 @@ end
@test d == IdDict(1 => 6, 2 => 5, 3 => 10)
@inferred mergewith(+, d1, d2, d3)
d = mergewith(+, d1, d2, d3)
@test d isa Dict{Int, Float64}
@test d == Dict(1 => 6, 2 => 5, 3 => 10)
@test d isa IdDict{Int, Float64}
@test d == IdDict(1 => 6, 2 => 5, 3 => 10)
end

@testset "misc error/io" begin
Expand Down
Loading