andyferris / Dictionaries.jl

An alternative interface for dictionaries in Julia, for improved productivity and performance
Other
282 stars 28 forks source link

Broadcasting `only` omits checks due to inbounds propagation #63

Closed c42f closed 3 years ago

c42f commented 3 years ago

Here's a dictionary where values contain collections of length not equal to 1. Broadcasting only should result in an error, but I get the first element instead:

julia> d = Dictionary([:a, :b], [[1,2], [3,4]])
2-element Dictionary{Symbol, Vector{Int64}}
 :a │ [1, 2]
 :b │ [3, 4]

julia> only.(d)
2-element Dictionary{Symbol, Int64}
 :a │ 1
 :b │ 3

Digging a little through the dispatch chain leads to

julia> bc = Base.broadcasted(only, d);

julia> out = similar(bc, Int)
2-element Dictionary{Symbol, Int64}
 :a │ 1
 :b │ 7

julia> map!(identity, out, bc)
2-element Dictionary{Symbol, Int64}
 :a │ 1
 :b │ 3

which further leads to the @inbounds assertion within the map! implementation, and the @propagate_inbounds within gettokenvalue(d::BroadcastedDictionary, t). This removes the @boundscheck within only when only is inlined. So while we have the expected:

julia> Dictionaries.gettokenvalue(bc, (0,1))
ERROR: ArgumentError: Collection has multiple elements, must contain exactly 1 element
Stacktrace:
 [1] only
   @ ./iterators.jl:1327 [inlined]

When this is optimized within an inbounds context (within map!), the bounds check disappears:

julia> foo(bc) = @inbounds Dictionaries.gettokenvalue(bc, (0,1))

julia> foo(bc)
1
andyferris commented 3 years ago

Good catch!