`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.