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

in 1.11, push! doesn't forward to append! as it should #56051

Open
aplavin opened this issue Oct 8, 2024 · 5 comments
Open

in 1.11, push! doesn't forward to append! as it should #56051

aplavin opened this issue Oct 8, 2024 · 5 comments
Labels
arrays [a, r, r, a, y, s]

Comments

@aplavin
Copy link
Contributor

aplavin commented Oct 8, 2024

push! docstring:

push!(collection, items...) -> collection

Insert one or more items in collection. If collection is an ordered container, the items are inserted at the end (in the given order).

Examples
≡≡≡≡≡≡≡≡

julia> push!([1, 2, 3], 4, 5, 6)
6-element Vector{Int64}:
1
2
3
4
5
6

If collection is ordered, use append! to add all the elements of another collection to it. The result of the preceding example is equivalent to append!([1, 2, 3], [4, 5, 6]). For AbstractSet objects, union! can be used instead.

Naturally, one would expect that defining append!(::MyArray, xs) would enable push! as well. And it does – on 1.10 and below.
But 1.11 breaks this.

MWE:

struct MyArray <: AbstractVector{Int}
    data::Vector{Int}
end

Base.size(a::MyArray) = size(a.data)
Base.getindex(a::MyArray, i...) = getindex(a.data, i...)

function Base.append!(a::MyArray, b)
    append!(a.data, b)
    return a
end

a = MyArray([1, 2, 3])
@show a
push!(a, 4)
@show a

Results:

❯ julia +1.6 tmp.jl 
a = [1, 2, 3]
a = [1, 2, 3, 4]

❯ julia +1.10 tmp.jl
a = [1, 2, 3]
a = [1, 2, 3, 4]

❯ julia +1.11 tmp.jl
a = [1, 2, 3]
ERROR: LoadError: MethodError: no method matching resize!(::MyArray, ::Int64)
The function `resize!` exists, but no method is defined for this combination of argument types.
@KristofferC
Copy link
Member

KristofferC commented Oct 8, 2024

Naturally, one would expect that defining append!(::MyArray, xs) would enable push! as well.

I have a bit of a hard time seeing this. If anything it feels like the opposite.

@aplavin
Copy link
Contributor Author

aplavin commented Oct 8, 2024

I personally expected both fallbacks to be honest :) Implemented append! for my array and indeed saw push! working.
Worked for quite some Julia versions, until today I ran tests on 1.11 and saw them broken.

@adienes
Copy link
Contributor

adienes commented Oct 8, 2024

one would expect that defining append!(::MyArray, xs) would enable push! as well

I can only speak for myself, but I would not have (and do not) expect this.

@oscardssmith
Copy link
Member

Ah this is an interesting one. It should be pretty easy to change this so that it works again, but I'm not sure that this was intended. As a side note, the vararg push! does seem very odd...

@jakobnissen
Copy link
Contributor

Perhaps related: #55459

FWIW, I don't see anything unexpected here. I interpret the docstring to mean that if both push and append is implemented, then append is equivalent to repeated pushing.

@nsajko nsajko added the arrays [a, r, r, a, y, s] label Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s]
Projects
None yet
Development

No branches or pull requests

6 participants