From 5547305a7d571a9abc6119589689a64cc7c98da6 Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Fri, 9 Feb 2024 22:03:26 -0500 Subject: [PATCH] Fix duplicate error when using generator in Dict (#53151) Fixes: #33147 Replaces/Closes: #40445 The difference here, compared to past implementations, is that we use the zero-cost `isiterable` check on every intermediate step, instead of wrapping the call in a try/catch and then trying to re-approximate the `isiterable` afterwards. Some samples: ```julia julia> Dict(i for i in 1:3) ERROR: ArgumentError: AbstractDict(kv): kv needs to be an iterator of 2-tuples or pairs Stacktrace: [1] _throw_dict_kv_error() @ Base ./dict.jl:118 [2] grow_to! @ ./dict.jl:132 [inlined] [3] dict_with_eltype @ ./abstractdict.jl:592 [inlined] [4] Dict(kv::Base.Generator{UnitRange{Int64}, typeof(identity)}) @ Base ./dict.jl:120 [5] top-level scope @ REPL[1]:1 julia> Dict(i => error("$i") for i in 1:3) ERROR: 1 Stacktrace: [1] error(s::String) @ Base ./error.jl:35 [2] (::var"#3#4")(i::Int64) @ Main ./none:0 [3] iterate @ ./generator.jl:48 [inlined] [4] grow_to! @ ./dict.jl:124 [inlined] [5] dict_with_eltype @ ./abstractdict.jl:592 [inlined] [6] Dict(kv::Base.Generator{UnitRange{Int64}, var"#3#4"}) @ Base ./dict.jl:120 [7] top-level scope @ REPL[2]:1 ``` The other unrelated change here is that `dest = empty(dest, typeof(k), typeof(v))` is made conditional, so we do not unconditionally construct an empty Dict in order to discard it and allocate an exact duplicate of it, but only do so if inference wasn't precise originally. Co-authored-by: Curtis Vogt --- base/abstractdict.jl | 49 ++++++++++++++++++++++++++++++++++++++++++++ base/dict.jl | 40 +----------------------------------- base/iddict.jl | 13 +----------- base/weakkeydict.jl | 12 +---------- test/dict.jl | 24 ++++++++++++++++++++++ 5 files changed, 76 insertions(+), 62 deletions(-) diff --git a/base/abstractdict.jl b/base/abstractdict.jl index 5ca485610f053..f73b518930a07 100644 --- a/base/abstractdict.jl +++ b/base/abstractdict.jl @@ -579,6 +579,55 @@ _tablesz(x::T) where T <: Integer = x < 16 ? T(16) : one(T)<<(top_set_bit(x-one( TP{K,V} = Union{Type{Tuple{K,V}},Type{Pair{K,V}}} +# This error is thrown if `grow_to!` cannot validate the contents of the iterator argument to it, which it does by testing the iteration protocol (isiterable) on it each time it is about to start iteration on it +_throw_dict_kv_error() = throw(ArgumentError("AbstractDict(kv): kv needs to be an iterator of 2-tuples or pairs")) + +function grow_to!(dest::AbstractDict, itr) + applicable(iterate, itr) || _throw_dict_kv_error() + y = iterate(itr) + y === nothing && return dest + kv, st = y + applicable(iterate, kv) || _throw_dict_kv_error() + k = iterate(kv) + k === nothing && _throw_dict_kv_error() + k, kvst = k + v = iterate(kv, kvst) + v === nothing && _throw_dict_kv_error() + v, kvst = v + iterate(kv, kvst) === nothing || _throw_dict_kv_error() + if !(dest isa AbstractDict{typeof(k), typeof(v)}) + dest = empty(dest, typeof(k), typeof(v)) + end + dest[k] = v + return grow_to!(dest, itr, st) +end + +function grow_to!(dest::AbstractDict{K,V}, itr, st) where {K, V} + y = iterate(itr, st) + while y !== nothing + kv, st = y + applicable(iterate, kv) || _throw_dict_kv_error() + kst = iterate(kv) + kst === nothing && _throw_dict_kv_error() + k, kvst = kst + vst = iterate(kv, kvst) + vst === nothing && _throw_dict_kv_error() + v, kvst = vst + iterate(kv, kvst) === nothing || _throw_dict_kv_error() + if isa(k, K) && isa(v, V) + dest[k] = v + else + new = empty(dest, promote_typejoin(K, typeof(k)), promote_typejoin(V, typeof(v))) + merge!(new, dest) + new[k] = v + return grow_to!(new, itr, st) + end + y = iterate(itr, st) + end + return dest +end + + dict_with_eltype(DT_apply, kv, ::TP{K,V}) where {K,V} = DT_apply(K, V)(kv) dict_with_eltype(DT_apply, kv::Generator, ::TP{K,V}) where {K,V} = DT_apply(K, V)(kv) dict_with_eltype(DT_apply, ::Type{Pair{K,V}}) where {K,V} = DT_apply(K, V)() diff --git a/base/dict.jl b/base/dict.jl index 2b6fd41d8d075..c924e10ab4d6e 100644 --- a/base/dict.jl +++ b/base/dict.jl @@ -114,45 +114,7 @@ const AnyDict = Dict{Any,Any} Dict(ps::Pair{K,V}...) where {K,V} = Dict{K,V}(ps) Dict(ps::Pair...) = Dict(ps) -function Dict(kv) - try - dict_with_eltype((K, V) -> Dict{K, V}, kv, eltype(kv)) - catch - if !isiterable(typeof(kv)) || !all(x->isa(x,Union{Tuple,Pair}),kv) - throw(ArgumentError("Dict(kv): kv needs to be an iterator of tuples or pairs")) - else - rethrow() - end - end -end - -function grow_to!(dest::AbstractDict{K, V}, itr) where V where K - y = iterate(itr) - y === nothing && return dest - ((k,v), st) = y - dest2 = empty(dest, typeof(k), typeof(v)) - dest2[k] = v - grow_to!(dest2, itr, st) -end - -# this is a special case due to (1) allowing both Pairs and Tuples as elements, -# and (2) Pair being invariant. a bit annoying. -function grow_to!(dest::AbstractDict{K,V}, itr, st) where V where K - y = iterate(itr, st) - while y !== nothing - (k,v), st = y - if isa(k,K) && isa(v,V) - dest[k] = v - else - new = empty(dest, promote_typejoin(K,typeof(k)), promote_typejoin(V,typeof(v))) - merge!(new, dest) - new[k] = v - return grow_to!(new, itr, st) - end - y = iterate(itr, st) - end - return dest -end +Dict(kv) = dict_with_eltype((K, V) -> Dict{K, V}, kv, eltype(kv)) empty(a::AbstractDict, ::Type{K}, ::Type{V}) where {K, V} = Dict{K, V}() diff --git a/base/iddict.jl b/base/iddict.jl index 73f532af9bba6..0b12ae86b2c6a 100644 --- a/base/iddict.jl +++ b/base/iddict.jl @@ -53,18 +53,7 @@ IdDict(ps::Pair{K}...) where {K} = IdDict{K,Any}(ps) IdDict(ps::(Pair{K,V} where K)...) where {V} = IdDict{Any,V}(ps) IdDict(ps::Pair...) = IdDict{Any,Any}(ps) -function IdDict(kv) - try - dict_with_eltype((K, V) -> IdDict{K, V}, kv, eltype(kv)) - catch - if !applicable(iterate, kv) || !all(x->isa(x,Union{Tuple,Pair}),kv) - throw(ArgumentError( - "IdDict(kv): kv needs to be an iterator of tuples or pairs")) - else - rethrow() - end - end -end +IdDict(kv) = dict_with_eltype((K, V) -> IdDict{K, V}, kv, eltype(kv)) empty(d::IdDict, ::Type{K}, ::Type{V}) where {K, V} = IdDict{K,V}() diff --git a/base/weakkeydict.jl b/base/weakkeydict.jl index b827f0d1495da..1a98bf1ee4333 100644 --- a/base/weakkeydict.jl +++ b/base/weakkeydict.jl @@ -54,17 +54,7 @@ WeakKeyDict(ps::Pair{K}...) where {K} = WeakKeyDict{K,Any}(ps) WeakKeyDict(ps::(Pair{K,V} where K)...) where {V} = WeakKeyDict{Any,V}(ps) WeakKeyDict(ps::Pair...) = WeakKeyDict{Any,Any}(ps) -function WeakKeyDict(kv) - try - Base.dict_with_eltype((K, V) -> WeakKeyDict{K, V}, kv, eltype(kv)) - catch - if !isiterable(typeof(kv)) || !all(x->isa(x,Union{Tuple,Pair}),kv) - throw(ArgumentError("WeakKeyDict(kv): kv needs to be an iterator of tuples or pairs")) - else - rethrow() - end - end -end +WeakKeyDict(kv) = Base.dict_with_eltype((K, V) -> WeakKeyDict{K, V}, kv, eltype(kv)) function _cleanup_locked(h::WeakKeyDict) if h.dirty diff --git a/test/dict.jl b/test/dict.jl index f44c6d696bc74..ca8a598de0b81 100644 --- a/test/dict.jl +++ b/test/dict.jl @@ -162,6 +162,30 @@ end # issue #39117 @test Dict(t[1]=>t[2] for t in zip((1,"2"), (2,"2"))) == Dict{Any,Any}(1=>2, "2"=>"2") + + @testset "issue #33147" begin + expected = try; Base._throw_dict_kv_error(); catch e; e; end + @test_throws expected Dict(i for i in 1:2) + @test_throws expected Dict(nothing for i in 1:2) + @test_throws expected Dict(() for i in 1:2) + @test_throws expected Dict((i, i, i) for i in 1:2) + @test_throws expected Dict(nothing) + @test_throws expected Dict((1,)) + @test_throws expected Dict(1:2) + @test_throws expected Dict(((),)) + @test_throws expected IdDict(((),)) + @test_throws expected WeakKeyDict(((),)) + @test_throws expected IdDict(nothing) + @test_throws expected WeakKeyDict(nothing) + @test Dict(1:0) isa Dict + @test Dict(()) isa Dict + try + Dict(i => error("$i") for i in 1:3) + catch ex + @test ex isa ErrorException + @test length(Base.current_exceptions()) == 1 + end + end end @testset "empty tuple ctor" begin