PyO3 / rust-numpy

PyO3-based Rust bindings of the NumPy C-API
BSD 2-Clause "Simplified" License
1.11k stars 106 forks source link

Freeze the slice container pyclass as it does not need mutable access #367

Closed adamreichold closed 1 year ago

adamreichold commented 1 year ago

Or rather any access at all.

@davidhewitt @mejrs I think we forgot this when switching from an open-coded #[pyclass] to using PyO3's macros to handle the complications implies by the multiple-pymethods feature. Maybe yet another data point that frozen-by-default would be a good thing (if the diagnostic of how to gain &mut self are good enough).

adamreichold commented 1 year ago

Tbh the UI tests for PyO3 show the &mut case with frozen - it's pretty good in my opinion.

I did go looking for it but admittedly could not find it. Which UI test you mean?

What I would have expected was something like adding

#[pymethods]
impl Foo {
    fn bar(&mut self) {
        self.field = 42;
    }
}

to invalid_frozen_pyclass_borrow.rs. If I do that, I get an error message like

error[E0271]: type mismatch resolving `<Foo as PyClass>::Frozen == False`
 --> tests/ui/invalid_frozen_pyclass_borrow.rs:9:1
  |
9 | #[pymethods]
  | ^^^^^^^^^^^^ expected struct `False`, found struct `True`
  |
note: required by a bound in `PyCell::<T>::try_borrow_mut`
 --> src/pycell.rs
  |
  |         T: PyClass<Frozen = False>,
  |                    ^^^^^^^^^^^^^^ required by this bound in `PyCell::<T>::try_borrow_mut`
  = note: this error originates in the attribute macro `pymethods` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0614]: type `PyRefMut<'_, Foo>` cannot be dereferenced
 --> tests/ui/invalid_frozen_pyclass_borrow.rs:9:1
  |
9 | #[pymethods]
  | ^^^^^^^^^^^^
  |
  = note: this error originates in the attribute macro `pymethods` (in Nightly builds, run with -Z macro-backtrace for more info)
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈

where the <Foo as PyClass>::Frozen == False is admittedly nice, but the error does not reference the &mut self at all and the reference to PyRefMut is relatively opaque if one has never used PyCell manually before.

I understand that this is not possible using proc macros, but if I were to make things up, I would aim for something like

error[E1234]: cannot borrow frozen pyclass as mutable
 --> tests/ui/invalid_frozen_pyclass_borrow.rs:11:1
   |
11 | fn bar(&mut self) {
   |        ^^^^^^^^^ expected `&self`, found `&mut self`
   |
note: pyclasses are frozen by default, add `mutable` to the definition of the pyclass, e.g. `#[pyclass(mutable)]`
adamreichold commented 1 year ago

Actually I think we only added frozen in 0.17, so when we made the change here frozen wasn't yet available.

Yes, you are right. So I forgot, that I could not have forgotten at that time. Or I forgot to add this when preparing rust-numpy 0.17.0. I think I appear somewhat confused in any possible interpretation. ;)