append!(Int64[], [1.1, 0.1]) results in unwanted array growth

[gasagna]

Consider the following code demonstrating the bug in append!

julia> a = Int64[]
0-element Array{Int64,1}

julia> append!(a, [1.1, 2.2])
ERROR: InexactError()
 in copy! at abstractarray.jl:344
 in append! at array.jl:447

julia> a
2-element Array{Int64,1}:
 13128344792
  4521590816

Now, in this case the program terminates with an error, but if the append statement is within a try/catch block the growth of the array can go unnoticed.

I actually discovered this bug in testing code, where I was testing that some code involving an append! raised an appropriate error. As soon as I added this test, all other tests in my code started to fail miserably.

[StefanKarpinski]

I believe this came up once before and this behavior was chosen because it's technically acceptable since the operation errored and this avoids and expensive try-catch, but I still think it would be better to at least try to make append! behave atomically w.r.t. known failure modes.

[mbauman]

Ref #7671

[JeffBezanson]

In the Midori terminology this would be an unrecoverable error. As in #7671 I still don't think operations need to be atomic with respect to these.

[nalimilan]

Could we at least reset the array to its original size before raising the error? I don't see any drawback in doing so, and it completely hides the bug.

[ivarne]

The previous discussion in #7642 and #7671 has all most relavant arguments.

This is a choice between safety and the speed of appending arrays of different types.

Suggested solutions:

1. Document that append! consideres conversion errors as an unrecoverable error and that the orignal array will be left in a undefined state. The current documentation is wrong, because append(A,b) isn't equivalent to push!(A,b...) in this regard.
2. Limit append to only work on arrays with the same element type (MethodError if the types don't match)
3. Implement the missing method after adding the restriction in (2.). This will be a slower method, because it uses try...catch to restore the array to after a failed conversion.
4. Various ideas that tries to catch a limited set of issues.

Off topic: Should probably consider #7798 (extending append! to iterables)

[StefanKarpinski]

In the Midori terminology this would be an unrecoverable error. As in #7671 I still don't think operations need to be atomic with respect to these.

Right, except that we don't have unrecoverable errors.

[StefanKarpinski]

Doesn't @nalimilan's suggesting solve this issue without introducing any overhead?

[ivarne]

I don't see how to implement @nalimilan's suggestion without a try...catch.

[StefanKarpinski]

Hmm. That's a fair point. If we could pass in custom error handlers, then it would be straightforward. But we can't do that currently.

[JeffBezanson]

One question that still interests me is why append! needs to be error-atomic, but multi-element setindex! doesn't. It seems to me the only difference is cost, which is part of what makes me think this is not really a safety issue.

Or imagine that append! really were implemented with repeated push!, as indeed it might be for some data structures (e.g. merge! on dicts). Then the array would end up with elements appended up to the point of the error. I suspect this would strike fewer people as a bug, since the resulting array wouldn't look weird. So I think at least part of this is not a need for atomicity, but just the uninitialized array looking weird.

[StefanKarpinski]

Could this be handled by calling sizehint on the array first and then pushing individual elements? That way the array would only end up containing successfully converted and pushed elements.

[nalimilan]

My bad. Unless we introduce a can_convert() -> Bool function, my suggestion doesn't work.

Pushing elements one-by-one is an interesting idea, but it probably incurs some performance hit too, and in the end the array is still modified (even if the result is much better). Only benchmarks will tell.

[non-Jedi]

Are there really that many situations where one might want to append an iterable that HasLength() or HasShape() but doesn't HasEltype()? Currently the definition of append!(::AbstractVector, iter) requires the former; couldn't we just require the latter as well?

Of course, this would be a breaking change so would require waiting until 2.0.

Edit: I guess the downside is that you lose the ability to append! an Any eltype iterable that happens to contain only items of the appropriate type.

[rapus95]

As this is still open I'm curious to ask why the following doesn't solve the original problem:

function append!(a, b)
  oldlen = length(a)
  resize!(a, length(a)+length(b)) #ensures enough space & contiguous memory
  a, newb = split!(a, oldlen) #splits into two arrays
  #a now is equivalent to the original a
  newb::typeof(similar(b, eltype(a))) #-> newb is uninitialized
  copyto!(newb, b) #errors on convert error
  contiguousmerge!(a, newb) #fast path which atomically forgets newb and resizes a
  return a
end

Here we only need to guarantee that the gc won't delete newb in mid of the function (which currently is guaranteed as I understand it) and that a and newb are not moved meanwhile. But that's already guaranteed aswell I guess.
It should also be noted that calling contiguousmerge! out of those very restricted circumstances can render the whole instance unrecoverable. Maybe even better replace it with some intrinsics.

Phrased differently: Why can't we allocate the targetarray in the right place in the first place?

[MasonProtter]

One option would be to do something like this:

function append!(x, y)
    if eltype(y) <: eltype(x)
        unsafe_append!(x, y)
    else
        try_append!(x, y)
    end
end

where unsafe_append! is basically what we do now, but try_append! has the try; catch block and will autoresize if it hits an error.

[StefanKarpinski]

I like it! Would you make a PR?

[MasonProtter]

Yeah I can give it a go.