reshape and push! inconsistent behavior

[cossio]

Consider the following example:

xmat = randn(5,5)
xvec = reshape(xmat, 25)
push!(xvec, 3)
xvec[1] = 0.0
xmat[1] # not zero

Here push! has decoupled xmat and xvec, though one would expect that a reshaped array always point to the same memory as the original array. I think the push! here should raise an error.

Contrast the example above with the following (source: https://discourse.julialang.org/t/puzzling-reshape-and-push/28326/2):

xvec = rand(4)
xmat = reshape(xvec, 2, 2)
push!(xvec, 1.0)  # ERROR: cannot resize array with shared data

To be consistent it seems to me that my first example above should also be an error.

[yuyichao]

This is an intentional hehavior. Changing this would be a breaking change.

Even though the implementation is different, just like subarray and reinterpreted arrays, you can't expect the original array and the new array to be completely symmetric so "consistency" isn't really an argument here. The real question then is which one is a better behavior.

In fact, both can be implemented from the other (the push does a copy implicitly) so the two behavior are basically equivalent (edit: apart from that the user code currently don't have enough information to do this copy automatically). Having a more explicit error could be a good thing since aliasing issue are a bit hard to debug so a change in 2.0 would probably be fine.

[KristofferC]

This is an intentional behavior.

According to what? This really feels like implementation detail leaking out and should be fair game to change. E.g. push! doesn't document that is unshares data when the buffer is owned by another array.

[yuyichao]

According to what?

According to this being explicitly implemented in the code. It's not a property of a naive implementation that didn't take this into account and it's definitely not a leak of implementation detail.

should be fair game to change.

No. This behavior is something someone can depend on. Failure to mention in the doc is just an argument to fix the doc. Raising an error in a case that previous doesn't is not going to be a minor change for sure.

[yuyichao]

Also, similar to many other aliasing issues, there are difference between users that care and users that doesn't. A user that just want to create an interpreted array before using it as a normal one is not going to care if the alias breaks or not and should be allowed to resize the new array. If the error is raised on the new array for resizing, there should be a way to support resizing a possibly shared array without making unnecessary copy when the array isn't shared.

[StefanKarpinski]

We can and do make technically breaking changes in minor releases under the term “minor change” so long as they are 1) undocumented, 2) unlikely to be relied upon on real code, 3) not actually relied upon in any packages according to PkgEval. This seems likely to qualify on all three counts. Especially since any code that relies on this is either broken already or will break with a fairly clear error message.

[yuyichao]

Especially since any code that relies on this is either broken already

Why? Not everyone cares about the old array after the new array is created.

or will break with a fairly clear error message.

But doesn't necessarily have an easy fix. See my comment above.

[yuyichao]

And FWIW, this is tested.

[StefanKarpinski]

If you want to join the triage call and discuss it, it’s on Thursday at 2:15 pm Eastern. It’s hard to come up with a legitimate use case for this “feature”.