Please consider clarifying use of unsafe in README example

[grothesque]

Thanks for providing this amazing library!

The code example given in this project's README file demonstrates the use of PyO3. I guess that it should also serve as a pedagogical model of how PyO3 should be used. After all, this is often going to be the first bit of PyO3-using code that prospective users are going to see.

Now that example features a short unsafe block without any explanation. But the usage of unsafe in Rust code means that the safety of the featured block has been carefully verified and the compiler should trust that.

In practice the usage of unsafe is quite often accompanied by a comment that explains why it is safe. This should certainly be the case in pedagogical code. I think that such a comment would be very helpful here.

Perhaps this particular use of unsafe is obvious to seasoned PyO3 users, but it certainly isn't to newbies for whom this code will often be the first contact with PyO3. Here are some question answers that may come up:

• Why is this particular use of unsafe safe after all?
• Are there any assumptions without which this would no longer be the case?
• The first function of the module takes PyReadonlyArrayDyn<'py, f64> arguments. It seems that the second function could similarly take a PyReadwriteArrayDyn<'py, f64> argument (which provides a safe as_array_mut method), but it takes &Bound<'py, PyArrayDyn<f64>> instead. Is this indeed a choice and what is the reason behind it?

I'm not suggesting that a long discussion of this issue should be added to the README. Probably a short comment and a few pointers into the documentation would be enough.

[grothesque]

I did some experimenting.

The function mult_py of the example can be indeed rewritten in a safe way as follows:

#[pyfn(m)]
fn mult_safe<'py>(a: f64, mut x: PyReadwriteArrayDyn<'py, f64>) {
    let x = x.as_array_mut();
    mult(a, x);
}

But this seems to come at a slight cost in terms of performance (release mode):

% python3 -m timeit -s 'import rust_python as rp; import numpy as np; a = np.identity(3)' 'rp.mult(3, a)'
1000000 loops, best of 5: 289 nsec per loop
% python3 -m timeit -s 'import rust_python as rp; import numpy as np; a = np.identity(3)' 'rp.mult_safe(3, a)'
500000 loops, best of 5: 406 nsec per loop

Is this the reason why the variant using unsafe is shown in the README example? If yes, then this deserves a comment IMHO. Otherwise, shouldn't the other variant be shown?

I wonder about the reason for the performance difference. After all the checking that prevents mutable aliasing with PyReadwriteArrayDyn is done at compile time. The following does not compile:

#[pyfn(m)]
fn mult_bad<'py>(a: f64, mut x: PyReadwriteArrayDyn<'py, f64>) {
    let y = x.as_array_mut();
    let x = x.as_array_mut();
    mult(a, x);
    mult(a, y);
}

While the following does:

#[pyfn(m)]
fn mult_dangerous<'py>(a: f64, x: &Bound<'py, PyArrayDyn<f64>>) {
    let y = unsafe { x.as_array_mut() };
    let x = unsafe { x.as_array_mut() };
    mult(a, x);
    mult(a, y);
}

[grothesque]

I think I finally understand now, but this took me many hours of studying https://github.com/PyO3/pyo3/pull/2885 and the like. Would a patch that introduces some hints into the documentation (notably the README file) be welcome?

Here is my current understanding (I am grateful for any corrections): PyReadwriteArrayDyn (and other similar types in numpy::borrow) is not just a zero-runtime-cost wrapper that allows for compile-time borrow checking. In addition it uses an internal "database" (implemented as a hashtable from NumPy array data pointers to borrow flags) ` to do dynamic borrow checking. This functionality is even available via a C-API in the hope that it will be used outside of rust-numpy.

So, if the above mult_safe is called from two different Python threads on the same array, the dynamic borrow checking will work and enforce Rust's discipline. This is even true if two separate CPython extensions (both implemented in Rust and using rust-numpy) try to simultaneously access NumPy arrays that share memory.

But, and this is a huge but, this borrow checking API (to my knowledge) is not used by other extensions, most notably it's not used by NumPy itself. So it is possible to simultaneously mutate a NumPy array using just rust-numpy and pure Python/Numpy. (Just like it's possible to simultaneously mutate a NumPy array using exclusively pure Python without rust-numpy.)

Rust-numpy takes the view that this is the most that it can do:

In summary, this crate takes the position that all unchecked code - unsafe Rust, Python, C, Fortran, etc. - must be checked for correctness by its author. Safe Rust code can then rely on this correctness, but should not be able to introduce memory safety issues on its own.

I think that's OK if we can be sure that this will never trigger undefined behavior. Are we?

[grothesque]

In summary, unless my understanding is flawed, the following changes seem appropriate:

(1) In the README example, no longer opt-out unsafely and without comment from the dynamic borrow checking that this crate itself provides. I.e. replace

let x = unsafe { x.as_array_mut() };

by

let mut x = x.readwrite();
let x = x.as_array_mut();

(or by a function that takes mut x: PyReadwriteArrayDyn<'py, f64> instead of x: &Bound<'py, PyArrayDyn<f64>>.

(2) In numpy::array::PyArray::as_array (and other similar methods there) explain clearly what needs to be respected to be able to use these methods safely: namely that some form of locking must be used to exclude illegal borrows. (Are there any cases where such locking might not be necessary?)