-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 eltype of flatten of tuple with non-2 length #56802
Conversation
In 4c076c8, eltype of flatten of tuple was improved by computing a refined eltype at compile time. However, this implementation only worked for length-2 tuples, and errored for all others. Generalize this to all tuples.
Turns out we need to think about bootstrapping - |
I second @martinholters suggestion for using |
That requires splatting the tuple, which might not be a good idea. |
Splatting is only an issue for large tuples, in any case pretty sure splatting directly is preferable to calling Lines 430 to 445 in ece1c70
|
EDIT: this entire comment is incorrect. I misinterpreted/misused
Actually, it doesn't seem to be an issue at all. Here are two variants on @martinholters' suggestion: function f(::Type{Iterators.Flatten{I}}) where {I<:Union{Tuple,NamedTuple}}
Base.afoldl((T, i) -> Base.promote_typejoin(T, eltype(i)), Union{}, fieldtypes(I)...)
end
function g(::Type{Iterators.Flatten{I}}) where {I<:Union{Tuple,NamedTuple}}
function reducer(t::Type, i::Int)
Base.promote_typejoin(t, eltype(fieldtype(I, i)))
end
Base.afoldl(reducer, Union{}, 1:fieldcount(I)...)
end Both are foldable independently of tuple length: julia> Base.infer_effects(f, Tuple{Type{Iterators.Flatten{<:Tuple}}})
(+c,+e,!n,+t,+s,+m,+u,+o,+r)
julia> Base.infer_effects(g, Tuple{Type{Iterators.Flatten{<:Tuple}}})
(+c,+e,!n,+t,+s,+m,+u,+o,+r)
julia> Base.infer_effects(f, Tuple{Type{Iterators.Flatten{<:NamedTuple}}})
(+c,+e,!n,+t,+s,+m,+u,+o,+r)
julia> Base.infer_effects(g, Tuple{Type{Iterators.Flatten{<:NamedTuple}}})
(+c,+e,!n,+t,+s,+m,+u,+o,+r) Also note that the new variant works without splatting a tuple. |
Co-authored-by: Neven Sajko <[email protected]>
Ah, I did not mean to merge this PR without getting @martinholters's approval first. @martinholters Could you do a post-merge review of this PR? If there are any changes you suggest, we can either make those changes in a follow-up PR, or revert this PR if there are major changes needed. |
FWIW the implementation that was merged is what @martinholters suggested: #56802 (comment) |
Just made a PR to test foldability inference of |
In 4c076c8, eltype of flatten of tuple was improved by computing a refined eltype at compile time. However, this implementation only worked for length-2 tuples, and errored for all others.
Generalize this to all tuples.
Closes #56783