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

firstindex(r) and fist(axes(r, 1)) return different types #45223

Closed
LilithHafner opened this issue May 7, 2022 · 2 comments · Fixed by #45230
Closed

firstindex(r) and fist(axes(r, 1)) return different types #45223

LilithHafner opened this issue May 7, 2022 · 2 comments · Fixed by #45230
Labels
bug Indicates an unexpected problem or unintended behavior

Comments

@LilithHafner
Copy link
Member

I expect firstindex(v::AbstractVector) to be the same as as first(axes(v, 1)) and to have the same type as lastindex(v), but this example (adapted from sorting tests) violates that expectation:

struct T
    n::Int8
end

Base.zero(::T) = T(0)
Base.convert(::Type{T}, n::Integer) = T(Int8(n))
Base.isless(a::T, b::T) = isless(a.n, b.n)
Base.:(-)(a::T, b::T) = T(a.n - b.n)
Base.:(+)(a::T, b::T) = T(a.n + b.n)
Base.:(*)(n::Integer, a::T) = T(n * a.n)
Base.rem(a::T, b::T) = T(rem(a.n, b.n))

# The important part of this test is that the return type of length might be different from Int
Base.length(r::StepRange{T,T}) = Int8((last(r).n - first(r).n + 1) ÷ step(r).n)

x = T(1):T(3)
println(typeof(first(axes(x, 1)))) # Int8
println(typeof(last(axes(x, 1)))) # Int8
println(typeof(firstindex(x))) # Int64
println(typeof(lastindex(x))) # Int8

If this is indeed a bug, we can probably fix it by changing this

firstindex(::UnitRange) = 1

to

firstindex(r::UnitRange) = one(eltype(axes1(r)))

or similar (and doing the same for the subsequent two lines).

@N5N3
Copy link
Member

N5N3 commented May 8, 2022

IIUC, these methods was introduced for performance. Keeping the result is a better choice.

However, performance remains a problem:
one(eltype(axes1(r))) is almost equivalent with the the general firstindex for AbstractArray.
UnitRange and LinRange have simple length, thus no problem.
length(::StepRange) is somehow too complex. we might need

function firstindex(::StepRange{T1,T2}) where {T1<:HWReal,T2<:HWReal}
    sizeof(T1) < sizeof(Int) && return 1
    return 
end

to make sure there's no regression.

Edit: div(one(T1), one(T2)) might not be the correct way. as length(::StepRange) is not stable sometime.
For example, @code_warntype length(StepRange(1,Int128(1),1)). We need to fix that first.

@LilithHafner
Copy link
Member Author

LilithHafner commented May 8, 2022

Ah, I was surprised and disappointed to find that one(eltype(axes1(x))) does not compile to a constant.

Ideally, length, axes1, firstindex, and lastindex are all stable.

If length (and therefore axes1 and lastindex) are unstable, however, then firstindex should also be unstable (e.g. use the general firstindex for AbstractArray).

I think generic methods and the compiler already efficiently handle the type stable case, so these optimizations are no longer necessary in that case, and only make a difference when lastindex is unstable, a difference that is buggy in my opinion.

@LilithHafner LilithHafner added the bug Indicates an unexpected problem or unintended behavior label May 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants