Skip to content

Commit

Permalink
Add push! implementation for AbstractArray depending only on resize! (#…
Browse files Browse the repository at this point in the history
…55470)

Fix #55459

In Julia 1.10, `push!` and `append!` would be functional for
`AbstractVector` implementations
if `resize!` and `setindex!` were defined.

As of #51903 by @vtjnash as in Julia 1.11.0-rc2, `append!` now depends
on an implementation of
`sizehint!` and `push!`. Since `push!` also depends on `append!`, a
stack
overflow situation can easily be created.

To avoid this, this pull request defines the following

* Add generic versions of `push!(a::AbstractVector, x)` which do not
depend on `append!`
* Add default implementation of `sizehint!` that is a no-op

The implementation of `push!(a::AbstractVector, x)` is a generic version
based on the implementation
of `push!(a::Vector, x)` without depending on internals.

# Example for SimpleArray

Consider the `SimpleArray` example from test/abstractarray.jl:

```julia
mutable struct SimpleArray{T} <: AbstractVector{T}
    els::Vector{T}
end
Base.size(sa::SimpleArray) = size(sa.els)
Base.getindex(sa::SimpleArray, idx...) = getindex(sa.els, idx...)
Base.setindex!(sa::SimpleArray, v, idx...) = setindex!(sa.els, v, idx...)
Base.resize!(sa::SimpleArray, n) = resize!(sa.els, n)
Base.copy(sa::SimpleArray) = SimpleArray(copy(sa.els))
```

Note that `setindex!` and `resize!` are implemented for `SimpleArray`.

## Julia 1.10.4: push! is functional

On Julia 1.10.4, `push!` has a functional implementation for
`SimpleArray`

```julia-repl
julia> push!(SimpleArray{Int}(zeros(Int,5)), 6)
6-element SimpleArray{Int64}:
 0
 0
 0
 0
 0
 6
```

## Julia 1.11.0-rc2 and nightly: push! requires sizehint! and is prone
to stack overflow

Before this pull request, on Julia 1.11.0-rc2 and nightly, `push!` fails
for want of `sizehint!`.

```julia-repl
julia> push!(SimpleArray{Int}(zeros(Int,5)), 6)
ERROR: MethodError: no method matching sizehint!(::SimpleArray{Int64}, ::Int64)
The function `sizehint!` exists, but no method is defined for this combination of argument types.
...
```

After implementing `sizehint!`, `push!` still fails with a stack
overflow.

```julia-repl
julia> Base.sizehint!(a::SimpleArray, x) = a

julia> push!(SimpleArray{Int}(zeros(Int, 5)), 6)
Warning: detected a stack overflow; program state may be corrupted, so further execution might be unreliable.
ERROR: StackOverflowError:
Stacktrace:
      [1] _append!
        @ ./array.jl:1344 [inlined]
      [2] append!
        @ ./array.jl:1335 [inlined]
      [3] push!(a::SimpleArray{Int64}, iter::Int64)
        @ Base ./array.jl:1336
--- the above 3 lines are repeated 79982 more times ---
 [239950] _append!
        @ ./array.jl:1344 [inlined]
 [239951] append!
        @ ./array.jl:1335 [inlined]
```

This is because the new implementation of `append!` depends on `push!`.

## After this pull request, push! is functional.

After this pull request, there is a functional `push!` for `SimpleArray`
again as in Julia 1.10.4:

```julia-repl
julia> push!(SimpleArray{Int}(zeros(Int, 5), 6)
6-element SimpleArray{Int64}:
 0
 0
 0
 0
 0
 6
```

(cherry picked from commit cf4c30a)
  • Loading branch information
mkitti authored and KristofferC committed Aug 13, 2024
1 parent 99583cf commit 10b55db
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 0 deletions.
32 changes: 32 additions & 0 deletions base/abstractarray.jl
Original file line number Diff line number Diff line change
Expand Up @@ -3497,13 +3497,45 @@ julia> map(+, [1 2; 3 4], [1,10,100,1000], zeros(3,1)) # iterates until 3rd is
"""
map(f, it, iters...) = collect(Generator(f, it, iters...))

# Generic versions of push! for AbstractVector
# These are specialized further for Vector for faster resizing and setindexing
function push!(a::AbstractVector{T}, item) where T
# convert first so we don't grow the array if the assignment won't work
itemT = item isa T ? item : convert(T, item)::T
new_length = length(a) + 1
resize!(a, new_length)
a[new_length] = itemT
return a
end

# specialize and optimize the single argument case
function push!(a::AbstractVector{Any}, @nospecialize x)
new_length = length(a) + 1
resize!(a, new_length)
a[new_length] = x
return a
end
function push!(a::AbstractVector{Any}, @nospecialize x...)
@_terminates_locally_meta
na = length(a)
nx = length(x)
resize!(a, na + nx)
for i = 1:nx
a[na+i] = x[i]
end
return a
end

# multi-item push!, pushfirst! (built on top of type-specific 1-item version)
# (note: must not cause a dispatch loop when 1-item case is not defined)
push!(A, a, b) = push!(push!(A, a), b)
push!(A, a, b, c...) = push!(push!(A, a, b), c...)
pushfirst!(A, a, b) = pushfirst!(pushfirst!(A, b), a)
pushfirst!(A, a, b, c...) = pushfirst!(pushfirst!(A, c...), a, b)

# sizehint! does not nothing by default
sizehint!(a::AbstractVector, _) = a

## hashing AbstractArray ##

const hash_abstractarray_seed = UInt === UInt64 ? 0x7e2d6fb6448beb77 : 0xd4514ce5
Expand Down
9 changes: 9 additions & 0 deletions test/abstractarray.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1423,6 +1423,15 @@ using .Main.OffsetArrays
end
end

@testset "Check push!($a, $args...)" for
a in (["foo", "Bar"], SimpleArray(["foo", "Bar"]), OffsetVector(["foo", "Bar"], 0:1)),
args in (("eenie",), ("eenie", "minie"), ("eenie", "minie", "mo"))
orig = copy(a)
push!(a, args...)
@test length(a) == length(orig) + length(args)
@test all(a[end-length(args)+1:end] .== args)
end

@testset "splatting into hvcat" begin
t = (1, 2)
@test [t...; 3 4] == [1 2; 3 4]
Expand Down

0 comments on commit 10b55db

Please sign in to comment.