JuliaLang / Compat.jl

Compatibility across Julia versions
Other
142 stars 117 forks source link

Backport 3.x: `stack` (Julia PR 43334) #779

Closed ararslan closed 1 year ago

ararslan commented 1 year ago

Backport of #777 to the release-3 branch.

Closes #778

bkamins commented 1 year ago

Thank you (although there seem to be some backporting issues) CC @mcabbott

codecov[bot] commented 1 year ago

Codecov Report

Merging #779 (27ebabb) into release-3 (952300c) will increase coverage by 1.85%. The diff coverage is 98.70%.

@@              Coverage Diff              @@
##           release-3     #779      +/-   ##
=============================================
+ Coverage      79.69%   81.54%   +1.85%     
=============================================
  Files              4        4              
  Lines            714      791      +77     
=============================================
+ Hits             569      645      +76     
- Misses           145      146       +1     
Impacted Files Coverage Δ
src/iterators.jl 62.50% <ø> (ø)
src/Compat.jl 81.77% <98.70%> (+2.10%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

ararslan commented 1 year ago

Working on that now. The one I'm looking into currently is the lack of Iterators.peel but I think I have an idea...

mcabbott commented 1 year ago

Never tried this with ancient Julia, but notice the same error on 1.7 "LoadError: cannot assign a value to variable Base.IteratorSize from module Compat" which is weird.

My first guess was that it relied on @constprop but the Compat version of that does seem to have been backported to this branch.

ararslan commented 1 year ago

I fixed the IteratorSize one locally, that was an include order issue.

ararslan commented 1 year ago

Alright, the one remaining failure now is on Julia 1.0, something is causing a MethodError where the test expects an ArgumentError. My computer won't run my Julia 1.0 binaries anymore so I'm not able to diagnose.

mcabbott commented 1 year ago

On 1.0 with this PR I get this error:

julia> stack([(1,2), (3,4,5)]; dims=1)
ERROR: MethodError: no method matching UnitRange(::Int64)
Closest candidates are:
  UnitRange(::T<:Real, ::T<:Real) where T<:Real at range.jl:257
  UnitRange(::AbstractUnitRange) at range.jl:857
Stacktrace:
 [1] iterate at ./generator.jl:47 [inlined]
 [2] _collect(::Base.OneTo{Int64}, ::Base.Generator{Base.OneTo{Int64},Type{UnitRange}}, ::Base.EltypeUnknown, ::Base.HasShape{1}) at ./array.jl:632
 [3] collect_similar at ./array.jl:561 [inlined]
 [4] map at ./abstractarray.jl:1987 [inlined]
 [5] _stack_size_check at /Users/me/.julia/packages/Compat/Qd33g/src/Compat.jl:1613 [inlined]
 [6] _dim_stack!(::Val{1}, ::Array{Int64,2}, ::Tuple{Int64,Int64}, ::Base.Iterators.Rest{Array{Tuple{Int64,Int64,Vararg{Int64,N} where N},1},Int64}) at /Users/me/.julia/packages/Compat/Qd33g/src/Compat.jl:1604

from this:

julia> x = (1,2,3);

julia> axes(x)  # is not a tuple
Base.OneTo(3)

julia> map(UnitRange, axes(x))
ERROR: MethodError: no method matching UnitRange(::Int64)

I'm slightly alarmed that no other tests notice that axes has this weird behaviour on tuples.

ararslan commented 1 year ago

😳

ararslan commented 1 year ago

Incredible, I fixed it on 1.0 and broke it everywhere else

mcabbott commented 1 year ago

That looks right. I guess the inference tests don't like non-const _tuple_axes = axes.

ararslan commented 1 year ago

Finally!

bkamins commented 1 year ago

Thank you!