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.

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

Fix regression in map and collect #43120

Merged
merged 3 commits into from
Nov 18, 2021
Merged

Fix regression in map and collect #43120

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ffef2c1
Fix regression in `map` and `collect`
nalimilan Nov 17, 2021
c4e26b7
Proper fix
nalimilan Nov 18, 2021
f2d8519
Fix 32-bit
nalimilan Nov 18, 2021
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
2 changes: 1 addition & 1 deletion base/promotion.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ function typejoin_union_tuple(T::Type)
elseif U isa Union
ci = typejoin(U.a, U.b)
else
ci = U
ci = promote_typejoin_union(U)
Copy link
Member Author

@nalimilan nalimilan Nov 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@vtjnash According to what you noted at #30485 (comment), it's not correct to call promote_typejoin_union from here given that the function is marked as @pure. But I don't know how to fix this otherwise, as without @pure the result isn't inferred, which defeats the point of all this code.

EDIT: in the worst case we could do ci = Any (at least to get a quick fix for 1.7), as we only need an upper bound for the type. But better be more precise if we can.

Copy link
Member

@vtjnash vtjnash Nov 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, exactly as your edit says. Note that promote_typejoin_union(U) here is already defined to be ci = Any, so that is the identical change too. And we could perhaps simplify the structure a bit:

diff --git a/base/promotion.jl b/base/promotion.jl
index 21245f0e05..70bb4c3b53 100644
--- a/base/promotion.jl
+++ b/base/promotion.jl
@@ -168,19 +168,17 @@ function promote_typejoin_union(::Type{T}) where T
         return Any # TODO: compute more precise bounds
     elseif T isa Union
         return promote_typejoin(promote_typejoin_union(T.a), promote_typejoin_union(T.b))
-    elseif T <: Tuple
-        return typejoin_union_tuple(T)
-    else
+    elseif T isa DataType
+        T <: Tuple && return typejoin_union_tuple(T)
         return T
+    else
+        error("unreachable") # not a type??
     end
 end
 
-function typejoin_union_tuple(T::Type)
+function typejoin_union_tuple(T::DataType)
     @_pure_meta
     u = Base.unwrap_unionall(T)
-    u isa Union && return typejoin(
-            typejoin_union_tuple(Base.rewrap_unionall(u.a, T)),
-            typejoin_union_tuple(Base.rewrap_unionall(u.b, T)))
     p = (u::DataType).parameters
     lr = length(p)::Int
     if lr == 0
@@ -192,6 +190,8 @@ function typejoin_union_tuple(T::Type)
         U = Core.Compiler.unwrapva(pi)
         if U === Union{}
             ci = Union{}
+        elseif U isa UnionAll
+            return Any # TODO: compute more precise bounds
         elseif U isa Union
             ci = typejoin(U.a, U.b)
         else

end
if i == lr && Core.Compiler.isvarargtype(pi)
c[i] = isdefined(pi, :N) ? Vararg{ci, pi.N} : Vararg{ci}
Expand Down
2 changes: 2 additions & 0 deletions test/broadcast.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1011,6 +1011,8 @@ end
@test typeof.([iszero, iszero]) == [typeof(iszero), typeof(iszero)]
@test isequal(identity.(Vector{<:Union{Int, Missing}}[[1, 2],[missing, 1]]),
[[1, 2],[missing, 1]])
@test broadcast(i -> ((x=i, y=(i==1 ? 1 : "a")), 3), 1:4) isa
Vector{Tuple{NamedTuple{(:x, :y)}, Int64}}
end

@testset "Issue #28382: eltype inconsistent with getindex" begin
Expand Down
2 changes: 2 additions & 0 deletions test/generic_map_tests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ function generic_map_tests(mapf, inplace_mapf=nothing)
@test isequal(map(identity, Vector{<:Union{Int, Missing}}[[1, 2],[missing, 1]]),
[[1, 2],[missing, 1]])
@test map(x -> x < 0 ? false : x, Int[]) isa Vector{Integer}
@test map(i -> ((x=i, y=(i==1 ? 1 : "a")), 3), 1:4) isa
Vector{Tuple{NamedTuple{(:x, :y)}, Int64}}
end

function testmap_equivalence(mapf, f, c...)
Expand Down