JuliaLang / julia

The Julia Programming Language
https://julialang.org/
MIT License
45.4k stars 5.45k forks source link

Base.show(::IO, ::AbstractArray) bug for non one based indexing #52320

Open m-wells opened 9 months ago

m-wells commented 9 months ago

For non 1-based arrays that don't get limited (...) the output is incorrect. Example

struct Foo{T} <: AbstractVector{T}
    x::Vector{T}
end

Base.getindex(x::Foo, i::Int) = x.x[i+1]
Base.size(x::Foo) = size(x.x)
Base.axes(x::Foo) = map(x -> x .- 1, axes(x.x))

Now if we go to a REPL

julia> x # this is correct
4-element Foo{Int64} with indices 0:3:
 1
 2
 3
 4

julia> (x,) # this is incorrect
([2, 3, 4, #undef],)

The issue is in Base.show_vector.

julia> Base.show_vector(stdout, x)
[2, 3, 4, #undef]

https://github.com/JuliaLang/julia/blob/5b2fcb68800875e570d7bb8c78ed00d360b6cfd5/base/arrayshow.jl#L512-L532

And specifically due to https://github.com/JuliaLang/julia/blob/5b2fcb68800875e570d7bb8c78ed00d360b6cfd5/base/arrayshow.jl#L529-L531

julia> Base.show_delim_array(stdout, x, '[', ",", ']', false)
[2, 3, 4, #undef]

julia> axs1 = axes1(x);

julia> f, l = first(axs1), last(axs1);

julia> Base.show_delim_array(stdout, x, '[', ",", ']', false, f, l)
[1, 2, 3, 4]

A simple fix is

function show_vector(io::IO, v, opn='[', cls=']')
    prefix, implicit = typeinfo_prefix(io, v)
    print(io, prefix)
    # directly or indirectly, the context now knows about eltype(v)
    if !implicit
        io = IOContext(io, :typeinfo => eltype(v))
    end
    limited = get(io, :limit, false)::Bool

    axs1 = axes1(v)
    f, l = first(axs1), last(axs1)
    if limited && length(v) > 20
        show_delim_array(io, v, opn, ",", "", false, f, f+9)
        print(io, "  …  ")
        show_delim_array(io, v, "", ",", cls, false, l-9, l)
    else
        show_delim_array(io, v, opn, ",", cls, false, f, l)
    end
end
jishnub commented 9 months ago

One point to note here is that the axes are not compliant with the interface requirements (although these weren't spelled out explicitly in prior Julia versions). Axes of arrays in Julia must be their own axes for consistency. Offset axes also need to respect this, so UnitRanges are usually not valid axes but Base.IndentityUnitRanges are. Does the issue still exist if you define the axes as map(x -> Base.IdentityUnitRange(x .- 1), axes(x.x))? I haven't checked this, but this might resolve the issue

m-wells commented 9 months ago

@jishnub While doing this does produce the desired output

julia> Base.axes(x::Foo) = map(x -> x .- 1, axes(x.x)); (k,)

julia> (x,)
([2, 3, 4, #undef],)

julia> Base.axes(x::Foo) = map(x -> Base.IdentityUnitRange(x .- 1), axes(x.x)); (k,)

julia> (x,)
([1, 2, 3, 4],)

You said

One point to note here is that the axes are not compliant with the interface requirements (although these weren't spelled out explicitly in prior Julia versions). Axes of arrays in Julia must be their own axes for consistency. Offset axes also need to respect this, so UnitRanges are usually not valid axes but Base.IndentityUnitRanges are.

but the documentation specifically uses the term UnitRange as a valid type to return

If you're writing a non-1 indexed array type, you will want to specialize axes so it returns a UnitRange, or (perhaps better) a custom AbstractUnitRange. The advantage of a custom type is that it "signals" the allocation type for functions like similar. If we're writing an array type for which indexing will start at 0, we likely want to begin by creating a new AbstractUnitRange, ZeroRange, where ZeroRange(n) is equivalent to 0:n-1.

jishnub commented 9 months ago

This should be updated. Using a UnitRange will lead to many other bugs aside from the show. See https://juliaarrays.github.io/OffsetArrays.jl/stable/internals/#Wrapping-other-offset-array-types