fix: cleanup internal nparray_to_* methods

[LDeakin]

Addresses #78. I don't think there is/was any unsoundness, but doesn't hurt to get more eyes on it. nparray_to_slice and nparray_to_unsafe_cell_slice now validate internally.

[flying-sheep]

looks good, but oof unsafe is scary. That lifetime bug you fixed could have easily caused us to crash and burn.

we really should replace as many unsafe APIs numpy and zarrs with safe alternatives

[flying-sheep]

oof good catch. this is why unsafe is scary, this was extremely wrong.

[kylebarron]

I'm curious, is there a tl;dr for why this was wrong?

[flying-sheep]

Sure! I’ll rename the lifetimes for easier understanding.

First, Bound is a GIL binding, that means it’s a kind of reference¹ that lives as long as a certain part of our code holds Python’s GIL (Global Interpreter Lock). This guarantees that nothing in Python land tries to mutate that object while we’re accessing it at the same time. So:

• &'x Bound<'py, PyUntypedArray> means “a reference with lifetime 'x to a GIL binding with lifetime 'py to a PyArrayObject”. So the reference needs to live (most likely) shorter than the GIL binding² or maximally equally long.
• &'y PyArrayObject means “a reference with lifetime 'y to a PyArrayObject”.

The previous version returned a reference &'y PyArrayObject with 'y: 'py, which means that the returned reference is valid as long as the GIL binding is held. In reality the returned reference is derived from the input reference &'x. Remember that this &'x reference is probably shorter-lived than the GIL binding. So before this fix, nothing stopped us from dropping the &'x reference (because as far as rustc is concerned, nothing derives from it), creating a new, mutable reference to the same object, and mutating the object through that, while other parts of our code are still allowed to read that object through the &'y reference. Something like:

let mut value: Bound<'py, PyUntypedArray> = ...;

let array: &'py PyUntypedArray = {
    let readonly_ref = &value;
    py_untyped_array_to_array_object(readonly_ref)
}; // readonly_ref is dropped here

let mut_ref = value.borrow_mut();
thread::spawn(move || {
    write_into(mut_ref);  // race condition
}
thread::spawn(move || {
    println!("{?:array}");  // race condition
}

Footnotes

1. I’ll only call regular Rust references “reference” after this to avoid confusion ↩

2. because before you reference something, you need to create it, and the thing can only be destroyed after you dropped your reference ↩

[kylebarron]

I just wanted to say thanks for the detailed explanation! It still makes my head spin, which is a good reason to avoid unsafe in my own code 😅

[flying-sheep]

I approved, but maybe #80 (comment) should be addressed.

If the numpy crate grows a safe API we have to think about this anyway