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

sorting an array of Union{} fails on nightly #45280

Closed
simonbyrne opened this issue May 11, 2022 · 4 comments · Fixed by #46095
Closed

sorting an array of Union{} fails on nightly #45280

simonbyrne opened this issue May 11, 2022 · 4 comments · Fixed by #46095
Labels
regression Regression in behavior compared to a previous version sorting Put things in order

Comments

@simonbyrne
Copy link
Contributor

julia> sort(Union{}[])
ERROR: MethodError: convert(::Type{Union{}}, ::Bool) is ambiguous. Candidates:
  convert(::Type{T}, x::Number) where T<:AbstractChar in Base at char.jl:184
  convert(::Type{T}, x::Number) where T<:Number in Base at number.jl:7
  convert(::Type{Union{}}, x) in Base at essentials.jl:280
  convert(::Type{T}, arg) where T<:VecElement in Base at baseext.jl:19
Possible fix, define
  convert(::Type{Union{}}, ::Number)
Stacktrace:
  [1] fill!(A::SubArray{Union{}, 1, Vector{Union{}}, Tuple{UnitRange{Int64}}, true}, x::Bool)
    @ Base ./multidimensional.jl:1098
  [2] copyto!
    @ ./broadcast.jl:921 [inlined]
  [3] materialize!
    @ ./broadcast.jl:871 [inlined]
  [4] materialize!
    @ ./broadcast.jl:868 [inlined]
  [5] sort!(v::Vector{Union{}}, lo::Int64, hi::Int64, a::Base.Sort.AdaptiveSort{Base.Sort.QuickSortAlg}, o::Base.Order.ForwardOrdering)
    @ Base.Sort ./sort.jl:742
  [6] sort!(v::Vector{Union{}}, alg::Base.Sort.AdaptiveSort{Base.Sort.QuickSortAlg}, order::Base.Order.ForwardOrdering)
    @ Base.Sort ./sort.jl:864
  [7] sort!(v::Vector{Union{}}; alg::Base.Sort.AdaptiveSort{Base.Sort.QuickSortAlg}, lt::Function, by::Function, rev::Nothing, order::Base.Order.ForwardOrdering)
    @ Base.Sort ./sort.jl:913
  [8] sort!(v::Vector{Union{}})
    @ Base.Sort ./sort.jl:907
  [9] sort(v::Vector{Union{}}; kws::Base.Pairs{Symbol, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
    @ Base.Sort ./sort.jl:961
 [10] sort(v::Vector{Union{}})
    @ Base.Sort ./sort.jl:961
 [11] top-level scope
    @ REPL[1]:1
@oscardssmith
Copy link
Member

Does this matter? Why would you use an array that you can't put an element in?

@simonbyrne
Copy link
Contributor Author

It matters to me: it broke my package: https://github.com/simonbyrne/KeywordDispatch.jl/runs/6391666554?check_suite_focus=true

One case is if you sort a collected tuple: the edge case, collecting an empty tuple returns such an array:

julia> collect(())
Union{}[]

@vtjnash
Copy link
Member

vtjnash commented May 11, 2022

A fix?

diff --git a/base/sort.jl b/base/sort.jl
index 23579abd77..e4e80d49fe 100644
--- a/base/sort.jl
+++ b/base/sort.jl
@@ -732,7 +732,7 @@ end
 
 # For AbstractVector{Bool}, counting sort is always best.
 # This is an implementation of counting sort specialized for Bools.
-function sort!(v::AbstractVector{<:Bool}, lo::Integer, hi::Integer, a::AdaptiveSort, o::Ordering)
+function sort!(v::AbstractVector{Bool}, lo::Integer, hi::Integer, a::AdaptiveSort, o::Ordering)
     first = lt(o, false, true) ? false : lt(o, true, false) ? true : return v
     count = 0
     @inbounds for i in lo:hi

@vtjnash
Copy link
Member

vtjnash commented May 11, 2022

But then we will notice that the UIntMappable function is implemented wrong/broken/buggy too (added in #44230 recently). Fixing that bug seems to make this case work:

diff --git a/base/sort.jl b/base/sort.jl
index 23579abd77..750f696bd4 100644
--- a/base/sort.jl
+++ b/base/sort.jl
@@ -1350,7 +1350,7 @@ uint_unmap(::Type{T}, u::Unsigned, ::ForwardOrdering) where T <: Signed =
 
 # unsigned(Int) is not available during bootstrapping.
 for (U, S) in [(UInt8, Int8), (UInt16, Int16), (UInt32, Int32), (UInt64, Int64), (UInt128, Int128)]
-    @eval UIntMappable(::Type{<:Union{$U, $S}}, ::ForwardOrdering) = $U
+    @eval UIntMappable(::Union{Type{$U}, Type{$S}}, ::ForwardOrdering) = $U
 end
 
 # Floats are not UIntMappable under regular orderings because they fail on NaN edge cases.

@JeffBezanson JeffBezanson added the regression Regression in behavior compared to a previous version label May 12, 2022
@LilithHafner LilithHafner added the sorting Put things in order label Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression Regression in behavior compared to a previous version sorting Put things in order
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants