Skip to content
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.

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

Fix duplicate error when using generator in Dict #53151

Merged
merged 3 commits into from
Feb 10, 2024
Merged

Fix duplicate error when using generator in Dict #53151

merged 3 commits into from
Feb 10, 2024

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Feb 1, 2024

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

Fixes: #33147
Replaces/Closes: #40445
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
@vtjnash vtjnash added error handling Handling of exceptions by Julia or the user error messages Better, more actionable error messages labels Feb 1, 2024
@vtjnash
Copy link
Member Author

vtjnash commented Feb 1, 2024

This implements the improved version proposed originally by @mbauman in #12453 (comment)

base/abstractdict.jl Outdated Show resolved Hide resolved
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Feb 8, 2024
@vtjnash vtjnash merged commit 5547305 into master Feb 10, 2024
5 of 8 checks passed
@vtjnash vtjnash deleted the jn/40445 branch February 10, 2024 03:03
@inkydragon inkydragon removed the merge me PR is reviewed. Merge when all tests are passing label Feb 10, 2024
fingolfin added a commit to fingolfin/OrderedCollections.jl that referenced this pull request Nov 24, 2024
fingolfin added a commit to fingolfin/OrderedCollections.jl that referenced this pull request Nov 24, 2024
fingolfin added a commit to fingolfin/OrderedCollections.jl that referenced this pull request Nov 24, 2024
fingolfin added a commit to JuliaCollections/OrderedCollections.jl that referenced this pull request Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error handling Handling of exceptions by Julia or the user error messages Better, more actionable error messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dict construction calls generator side effects twice on failure
4 participants