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

Fixes for non-Int based lengths #37741

Merged
merged 15 commits into from
Jan 4, 2021
23 changes: 11 additions & 12 deletions base/abstractarray.jl
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ julia> axes(A)
"""
function axes(A)
@_inline_meta
map(OneTo, size(A))
map(oneto, size(A))
end

"""
Expand All @@ -107,10 +107,10 @@ require_one_based_indexing(A...) = !has_offset_axes(A...) || throw(ArgumentError
# in other applications.
axes1(A::AbstractArray{<:Any,0}) = OneTo(1)
axes1(A::AbstractArray) = (@_inline_meta; axes(A)[1])
axes1(iter) = OneTo(length(iter))
axes1(iter) = oneto(length(iter))

unsafe_indices(A) = axes(A)
unsafe_indices(r::AbstractRange) = (OneTo(unsafe_length(r)),) # Ranges use checked_sub for size
unsafe_indices(r::AbstractRange) = (oneto(unsafe_length(r)),) # Ranges use checked_sub for size

keys(a::AbstractArray) = CartesianIndices(axes(a))
keys(a::AbstractVector) = LinearIndices(a)
Expand Down Expand Up @@ -308,7 +308,7 @@ function eachindex(A::AbstractArray, B::AbstractArray...)
@_inline_meta
eachindex(IndexStyle(A,B...), A, B...)
end
eachindex(::IndexLinear, A::AbstractArray) = (@_inline_meta; OneTo(length(A)))
eachindex(::IndexLinear, A::AbstractArray) = (@_inline_meta; oneto(length(A)))
eachindex(::IndexLinear, A::AbstractVector) = (@_inline_meta; axes1(A))
function eachindex(::IndexLinear, A::AbstractArray, B::AbstractArray...)
@_inline_meta
Expand Down Expand Up @@ -1483,12 +1483,11 @@ vcat(V::AbstractVector{T}...) where {T} = typed_vcat(T, V...)
# but that solution currently fails (see #27188 and #27224)
AbstractVecOrTuple{T} = Union{AbstractVector{<:T}, Tuple{Vararg{T}}}

function _typed_vcat(::Type{T}, V::AbstractVecOrTuple{AbstractVector}) where T
n = 0
for Vk in V
n += Int(length(Vk))::Int
end
a = similar(V[1], T, n)
_typed_vcat_similar(V, T, n) = similar(V[1], T, n)
_typed_vcat(::Type{T}, V::AbstractVecOrTuple{AbstractVector}) where T =
_typed_vcat!(_typed_vcat_similar(V, T, mapreduce(length, +, V)), V)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the perspective of Base & stdlibs, this change seems safe because _typed_vcat has no backedges that extend deeper than typed_vcat---we expunged them from the system image (probably mostly in #37163). You can create more backedges with vcat(1:3, [1,2,3]), and so this may cause some worsening of package interactions, but I think that can argued to be their problem rather than Julia's.


function _typed_vcat!(a::AbstractVector{T}, V::AbstractVecOrTuple{AbstractVector}) where T
pos = 1
for k=1:Int(length(V))::Int
Vk = V[k]
Expand Down Expand Up @@ -1634,15 +1633,15 @@ _cat(dims, X...) = cat_t(promote_eltypeof(X...), X...; dims=dims)
@inline cat_t(::Type{T}, X...; dims) where {T} = _cat_t(dims, T, X...)
@inline function _cat_t(dims, ::Type{T}, X...) where {T}
catdims = dims2cat(dims)
shape = cat_shape(catdims, map(cat_size, X)::Tuple{Vararg{Union{Int,Dims}}})::Dims
shape = cat_shape(catdims, map(cat_size, X))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as comment for _vcat_t above: safe.

A = cat_similar(X[1], T, shape)
if count(!iszero, catdims)::Int > 1
fill!(A, zero(T))
end
return __cat(A, shape, catdims, X...)
end

function __cat(A, shape::NTuple{M,Int}, catdims, X...) where M
function __cat(A, shape::NTuple{M}, catdims, X...) where M
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

N = M::Int
offsets = zeros(Int, N)
inds = Vector{UnitRange{Int}}(undef, N)
Expand Down
12 changes: 8 additions & 4 deletions base/arrayshow.jl
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,9 @@ Alignment is reported as a vector of (left,right) tuples, one for each
column going across the screen.
"""
function alignment(io::IO, X::AbstractVecOrMat,
rows::AbstractVector, cols::AbstractVector,
cols_if_complete::Integer, cols_otherwise::Integer, sep::Integer)
a = Tuple{Int, Int}[]
rows::AbstractVector{T}, cols::AbstractVector{V},
cols_if_complete::Integer, cols_otherwise::Integer, sep::Integer) where {T,V}
a = Tuple{T, V}[]
Copy link
Member

@timholy timholy Dec 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is more concerning. I'm a little surprised that we don't @nospecialize(X). Julia ships with 19 precompiled MethodInstances whose backedges include print_matrix, and these are pretty expensive to recompile:

julia> include("/tmp/sv.jl")
2-element SquaresVector:
 1
 4  0.395123 seconds (1.06 M allocations: 58.098 MiB, 6.47% gc time, 99.91% compilation time)

where sv.jl is taken from https://docs.julialang.org/en/v1/manual/interfaces/#man-interface-array and then adds display:

struct SquaresVector <: AbstractArray{Int, 1}
    count::Int
end
Base.size(S::SquaresVector) = (S.count,)
Base.IndexStyle(::Type{<:SquaresVector}) = IndexLinear()
Base.getindex(S::SquaresVector, i::Int) = i*i

sv = SquaresVector(2)
@time show(stdout, MIME("text/plain"), sv)

0.4s is a long time, and we have to pay it if stuff gets invalidated.

Since the rows and cols are supposed to be lists of indices, I'll ask whether this change is truly needed or whether there might be another way? We'll do it if we must, but try looking at

code_warntype(Base.alignment, (IO, AbstractVecOrMat, AbstractVector, AbstractVector, Integer, Integer, Integer))

and

code_warntype(Base.print_matrix, (IO, AbstractVecOrMat, AbstractString, AbstractString, AbstractString, AbstractString, AbstractString, AbstractString, Integer, Integer))

to see the invalidation risks and how much the type-asserts currently save us. You might compare this branch against master and check for differences.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the rows and cols are supposed to be lists of indices, I'll ask whether this change is truly needed or whether there might be another way?

It seems what we want to say is that we are iterating over the "range" of a vector. A more descriptive way would be to change it to rowsA, colsA = union(axes(X,1)),union(axes(X,2)). Unfortunately, this won't work since union(::AbstractRange) is not specialised:

julia> union(Base.OneTo(5))
5-element Vector{Int64}:
 1
 2
 3
 4
 5

There's a very good argument for adding special overloads for union(::AbstractRange), though making the return type immutable would be a breaking change. And this wouldn't actually improve any of the invalidation issues.

for j in cols # need to go down each column one at a time
l = r = 0
for i in rows # plumb down and see what largest element sizes are
Expand Down Expand Up @@ -166,6 +166,11 @@ function print_matrix(io::IO, @nospecialize(X::AbstractVecOrMat),
vdots::AbstractString = "\u22ee",
ddots::AbstractString = " \u22f1 ",
hmod::Integer = 5, vmod::Integer = 5)
# use invokelatest to avoid backtracing in type invalidation, ref #37741
invokelatest(_print_matrix, io, X, pre, sep, post, hdots, vdots, ddots, hmod, vmod, unitrange(axes(X,1)), unitrange(axes(X,2)))
end

function _print_matrix(io, @nospecialize(X::AbstractVecOrMat), pre, sep, post, hdots, vdots, ddots, hmod, vmod, rowsA, colsA)
hmod, vmod = Int(hmod)::Int, Int(vmod)::Int
if !(get(io, :limit, false)::Bool)
screenheight = screenwidth = typemax(Int)
Expand All @@ -178,7 +183,6 @@ function print_matrix(io::IO, @nospecialize(X::AbstractVecOrMat),
postsp = ""
@assert textwidth(hdots) == textwidth(ddots)
sepsize = length(sep)::Int
rowsA, colsA = UnitRange{Int}(axes(X,1)), UnitRange{Int}(axes(X,2))
m, n = length(rowsA), length(colsA)
# To figure out alignments, only need to look at as many rows as could
# fit down screen. If screen has at least as many rows as A, look at A.
Expand Down
3 changes: 3 additions & 0 deletions base/range.jl
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,8 @@ unitrange_last(start::T, stop::T) where {T} =
ifelse(stop >= start, convert(T,start+floor(stop-start)),
convert(T,start-oneunit(stop-start)))

unitrange(x) = UnitRange(x)

if isdefined(Main, :Base)
# Constant-fold-able indexing into tuples to functionally expose Base.tail and Base.front
function getindex(@nospecialize(t::Tuple), r::UnitRange)
Expand Down Expand Up @@ -332,6 +334,7 @@ struct OneTo{T<:Integer} <: AbstractUnitRange{T}
end
OneTo(stop::T) where {T<:Integer} = OneTo{T}(stop)
OneTo(r::AbstractRange{T}) where {T<:Integer} = OneTo{T}(r)
oneto(r) = OneTo(r)

## Step ranges parameterized by length

Expand Down
4 changes: 2 additions & 2 deletions base/reshapedarray.jl
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ rdims(out::Tuple{}, inds::NTuple{M,Any}) where {M} = ()
rdims(out::Tuple{Any}, inds::Tuple{}) = out # N == 1, M == 0
rdims(out::NTuple{N,Any}, inds::Tuple{}) where {N} = out # N > 1, M == 0
rdims(out::Tuple{Any}, inds::Tuple{Any}) = inds # N == 1, M == 1
rdims(out::Tuple{Any}, inds::NTuple{M,Any}) where {M} = (OneTo(rdims_trailing(inds...)),) # N == 1, M > 1
rdims(out::Tuple{Any}, inds::NTuple{M,Any}) where {M} = (oneto(rdims_trailing(inds...)),) # N == 1, M > 1
rdims(out::NTuple{N,Any}, inds::NTuple{N,Any}) where {N} = inds # N > 1, M == N
rdims(out::NTuple{N,Any}, inds::NTuple{M,Any}) where {N,M} = (first(inds), rdims(tail(out), tail(inds))...) # N > 1, M > 1, M != N

Expand Down Expand Up @@ -207,7 +207,7 @@ size(A::ReshapedArray) = A.dims
similar(A::ReshapedArray, eltype::Type, dims::Dims) = similar(parent(A), eltype, dims)
IndexStyle(::Type{<:ReshapedArrayLF}) = IndexLinear()
parent(A::ReshapedArray) = A.parent
parentindices(A::ReshapedArray) = map(OneTo, size(parent(A)))
parentindices(A::ReshapedArray) = map(oneto, size(parent(A)))
reinterpret(::Type{T}, A::ReshapedArray, dims::Dims) where {T} = reinterpret(T, parent(A), dims)
elsize(::Type{<:ReshapedArray{<:Any,<:Any,P}}) where {P} = elsize(P)

Expand Down
6 changes: 3 additions & 3 deletions base/subarray.jl
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ viewindexing(I::Tuple{Vararg{Any}}) = IndexCartesian()
viewindexing(I::Tuple{AbstractArray, Vararg{Any}}) = IndexCartesian()

# Simple utilities
size(V::SubArray) = (@_inline_meta; map(n->Int(unsafe_length(n)), axes(V)))
size(V::SubArray) = (@_inline_meta; map(unsafe_length, axes(V)))

similar(V::SubArray, T::Type, dims::Dims) = similar(V.parent, T, dims)

Expand Down Expand Up @@ -90,7 +90,7 @@ julia> parentindices(V)
(1, Base.Slice(Base.OneTo(2)))
```
"""
parentindices(a::AbstractArray) = map(OneTo, size(a))
parentindices(a::AbstractArray) = map(oneto, size(a))

## Aliasing detection
dataids(A::SubArray) = (dataids(A.parent)..., _splatmap(dataids, A.indices)...)
Expand All @@ -107,7 +107,7 @@ function unaliascopy(V::SubArray{T,N,A,I,LD}) where {T,N,A<:Array,I<:Tuple{Varar
end
# Transform indices to be "dense"
_trimmedindex(i::Real) = oftype(i, 1)
_trimmedindex(i::AbstractUnitRange) = oftype(i, OneTo(length(i)))
_trimmedindex(i::AbstractUnitRange) = oftype(i, oneto(length(i)))
_trimmedindex(i::AbstractArray) = oftype(i, reshape(eachindex(IndexLinear(), i), axes(i)))

## SubArray creation
Expand Down
5 changes: 5 additions & 0 deletions test/abstractarray.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1234,6 +1234,11 @@ end
@test Base.rest(a, st) == [3, 2, 4]
end

@testset "issue #37741, non-int cat" begin
@test [1; 1:BigInt(5)] == [1; 1:5]
@test [1:BigInt(5); 1] == [1:5; 1]
end

@testset "Base.isstored" begin
a = rand(3, 4, 5)
@test Base.isstored(a, 1, 2, 3)
Expand Down
14 changes: 14 additions & 0 deletions test/subarray.jl
Original file line number Diff line number Diff line change
Expand Up @@ -698,3 +698,17 @@ import InteractiveUtils
@test M*v == copy(M)*v
@test (InteractiveUtils.@which M*v) == (InteractiveUtils.@which copy(M)*v)
end


isdefined(Main, :InfiniteArrays) || @eval Main include("testhelpers/InfiniteArrays.jl")
using .Main.InfiniteArrays, Base64

@testset "PR #37741: non-Int sizes" begin
r = BigInt(1):BigInt(100_000_000)^100
v = SubArray(r, (r,))
@test size(v) == (last(r),)

v = SubArray(OneToInf(), (OneToInf(),))
@test size(v) == (Infinity(),)
@test stringmime("text/plain", v; context=(:limit => true)) == "$(Infinity())-element view(::$(OneToInf{Int}), 1:1:$(Infinity())) with eltype $Int with indices 1:1:$(Infinity()):\n 1\n 2\n 3\n 4\n 5\n 6\n 7\n 8\n 9\n 10\n ⋮"
end
51 changes: 51 additions & 0 deletions test/testhelpers/InfiniteArrays.jl
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
# This file is a part of Julia. License is MIT: https://julialang.org/license

# InfiniteArrays (arrays with infinite size)

# This test file is designed to exercise support for generic sizing,
# even though infinite arrays aren't implemented in Base.

module InfiniteArrays

export OneToInf, Infinity

"""
Infinity()

represents infinite cardinality. Note that `Infinity <: Integer` to support
being treated as an index.
"""
struct Infinity <: Integer end

Base.:(==)(::Infinity, ::Int) = false
Base.:(==)(::Int, ::Infinity) = false
Base.:(<)(::Int, ::Infinity) = true
Base.:(≤)(::Int, ::Infinity) = true
Base.:(≤)(::Infinity, ::Int) = false
Base.:(≤)(::Infinity, ::Infinity) = true
Base.:(-)(::Infinity, ::Int) = Infinity()
Base.:(+)(::Infinity, ::Int) = Infinity()
Base.:(:)(::Infinity, ::Infinity) = 1:0

"""
OneToInf(n)

Define an `AbstractInfUnitRange` that behaves like `1:∞`, with the added
distinction that the limits are guaranteed (by the type system) to
be 1 and ∞.
"""
struct OneToInf{T<:Integer} <: AbstractUnitRange{T} end

OneToInf() = OneToInf{Int}()

Base.axes(r::OneToInf) = (r,)
Base.unsafe_indices(r::OneToInf) = (r,)
Base.unsafe_length(r::OneToInf) = Infinity()
Base.size(r::OneToInf) = (Infinity(),)
Base.first(r::OneToInf{T}) where {T} = oneunit(T)
Base.length(r::OneToInf{T}) where {T} = Infinity()
Base.last(r::OneToInf{T}) where {T} = Infinity()
Base.unitrange(r::OneToInf) = r
Base.oneto(::Infinity) = OneToInf()

end