Skip to content

Use checked_length when collecting ranges #58251

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

jishnub
Copy link
Member

@jishnub jishnub commented Apr 28, 2025

This improves the status quo slightly, as it avoids a segfault if the length computation for the ranges overflow. This is only possible for known Integer types in Base, for which a checked_length method exists. However, this is enough to address common cases.

After this,

julia> r = typemin(Int):typemax(Int)
-9223372036854775808:9223372036854775807

julia> collect(r)
ERROR: OverflowError: 9223372036854775807 - -9223372036854775808 overflowed for type Int64

Fixes #58245

The performance of collect seems unaffected by this change, as this check should be inexpensive compared to the following setindex!.

@giordano
Copy link
Member

After this,

That sounds like a good test to add, no?

@mbauman
Copy link
Member

mbauman commented Apr 28, 2025

Why not always use checked_length? It already falls back to length in that last ::Any method:

julia> methods(Base.checked_length)
# 9 methods for generic function "checked_length" from Base:
 [1] checked_length(r::Base.OneTo{<:Union{Int16, Int32, Int8, UInt16, UInt32, UInt8}})
     @ range.jl:830
 [2] checked_length(r::StepRange{<:Dates.Period})
     @ Dates ~/.julia/juliaup/julia-1.10.9+0.aarch64.apple.darwin14/share/julia/stdlib/v1.10/Dates/src/ranges.jl:28
 [3] checked_length(r::AbstractUnitRange{T}) where T<:Rational
     @ rational.jl:582
 [4] checked_length(r::AbstractUnitRange{<:Union{Int16, Int32, Int8, UInt16, UInt32, UInt8}})
     @ range.jl:829
 [5] checked_length(r::AbstractUnitRange{T}) where T
     @ range.jl:730
 [6] checked_length(r::OrdinalRange{T}) where T<:Union{Int128, Int64, UInt128, UInt64}
     @ range.jl:798
 [7] checked_length(r::OrdinalRange{<:Union{Int16, Int32, Int8, UInt16, UInt32, UInt8}})
     @ range.jl:828
 [8] checked_length(r::OrdinalRange{T}) where T
     @ range.jl:713
 [9] checked_length(r)
     @ Base.Checked checked.jl:367

@mbauman mbauman added the ranges Everything AbstractRange label Apr 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ranges Everything AbstractRange
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segfault in edge case UnitRange{UInt64}
3 participants