New #[pyclass] system that is aware of its concrete layout

[kngwyu]

This PR introduces a new #[pyclass] system that uses struct PyClassShell instead of offset access.
I'll add a more detailed explanation later.
cc: @davidhewitt

TODO

• Modify derive-backend
• Renew #[new] API
• Renew super().__init()__
• Implement weakref and dict support
• Give correct ConcreteLayouts for C-FFI types
  • bool
  • bytearray
  • complex
  • datetime
  • dict
  • float
  • int
  • list
  • module
  • set
  • slice
  • string
  • tuple
• Fix tests
• Fix guides
• Fix examples
• Proof read docs
• Check naming issues
• CHANGELOG

[kngwyu]

Still incomplete, but here I note the rough sketch.
This PR introduces PyClassShell, which represents the concrete layout of #[pyclass] in Python heap.

#[repr(C)]
pub struct PyClassShell<T: PyClass> {
    ob_base: <T::BaseType as PyTypeInfo>::ConcreteLayout,
    pyclass: ManuallyDrop<T>,
    ...
}

Currently, we use a hacky way to deal with #[pyclass].
Specifically, we use OFFSET for getting &T: PyClass from *mut ffi::PyObject, like this.

(value.as_ptr() as *const u8).offset(T::OFFSET) as *const T

(From https://github.com/PyO3/pyo3/blob/v0.8.4/src/conversion.rs#L399)

By using PyClassShell, we can write this operation by (roughly)

(*(value.as_ptr() as *const PyClassShell<T>)).pyclass

Additionally, this PR does

• Replace PyRef with &PyClassShell
• Replace PyRefMut with &mut PyClassShell
• Remove PyTypeCreate and PyObjectAlloc
• Introduce PyClassAlloc, PyConcreteObject for tp_alloc and tp_dealloc
• Remove PyRawObject
• Introduce PyClassInitializer, which changes initialization API dramatically

Now #[new] doesn't take PyRawObject.
It instead is a roughly FnOnce(*args, **kwargs) -> impl IntoInitializer.

#[pymethods]
impl BaseClass {
    #[new]
    fn new() -> Self {
        BaseClass { val1: 10 }
    }
}

#[pyclass(extends=BaseClass)]
struct SubClass {
    #[pyo3(get)]
    val2: usize,
}

#[pymethods]
impl SubClass {
    #[new]
    fn new() -> PyClassInitializer<Self> {
        let mut init = PyClassInitializer::from_value(SubClass { val2: 5 });
        init.get_super().init(BaseClass { val1: 10 });
        init
    }
}

The benefit of new PyClassInitializer is that it can detect uninitialized object.
I.e., if there is an uninitialized #[pyclass] in base classes, it raises an exception, which resolves #664.
Indeed we need more discussion, but I believe this API is much easier to use.

[programmerjake]

One thing that should be supported (I didn't check) is the ability to skip initialization and throw away the new object when raising an exception from #[new]. The code would need to handle destroying a partially initialized object (some rust base classes could have been initialized when the failure is detected).

It would also be nice, but not required, to support conditionally returning an unrelated object from #[new], rather than the newly allocated object, similar to how Python's enum.Enum constructor works (it always returns the selected enumerant object instead of constructing a new object).

[davidhewitt]

Sorry for the time taken for me to pick this up. (I'd been out travelling and hadn't the time to give this a proper read through.)

Overall, this looks nice. While we're here, I think there's two main things we should be aiming for:

• It would be great if we can keep PyClassShell<T> out of the pyo3 api, so that users can just use &T / &mut T. I don't know how easy this is.
• pybind11 has mechanisms to be able to wrap references for Python, without copying. I've made a comment on PyClassShell which hints how the references are stored in there. Doing this safely also requires a "keep-alive" mechanism so that Python knows to keep the original data alive (so we don't end up with dangling references).

[davidhewitt]

Is it necessary for the ConcreteLayout to be part of the TypeInfo trait? It seems like this is only needed for #[pyclass] types. Having it here creates a lot of work to get the right definitions for all the py native types, but I'm not sure if it helps.

[kngwyu]

If we want to give a fixed size to PyClassShell, we need it.

It seems like this is only needed for #[pyclass] types.

We also need it to derive PyDict or so.

Having it here creates a lot of work to get the right definitions for all the py native types,

Yeah, but we can use some scripts to get them.

[davidhewitt]

This PyClassShell idea is nice, and is heading in the direction that pybind11 goes. There's two main differences between this idea and what pybind11 does:

1. The pybind11 equivalent just stores *mut void instead of being generic over T. This means that the python type allocation for all pybind11 classes is more or less exactly the same. It also means smaller binary sizes, because there's only one copy of the PyClassShell methods instead of one for each T.

2. pybind11 allows references to be stored in the PyClassShell as well as owned values. In Rust speak, instead of pyclass: ManuallyDrop<T>, we'd have something like:

   enum PyClassValue<T> {
       Borrowed(*const T),
       MutBorrowed(*mut T),
       Owned(ManuallyDrop<T>),
   }
   

The great thing about storing references in PyClassShell is that it allows us to do a lot less copying when going Rust -> Python -> Rust.

For example, the code below doesn't currently compile with pyo3 because the &Address return type. If we could store references in the PyClassShell, then this should be fairly straightforward to support:

#[pyclass]
struct Address {
    value: String
}

#[pyclass]
struct Employee {
    address: Address
}

#[pymethods]
impl Employee {
    pub fn address(&self) -> &Address {
        &self.address
   }
}

[programmerjake]

• It would be great if we can keep PyClassShell<T> out of the pyo3 api, so that users can just use &T / &mut T. I don't know how easy this is.

We need to make sure that we never can simultaneously hand out more than one &mut T or both &mut T and &T, so there should be some method of tracking that (like RefCell).

This is important since &mut T means unique, not just mutable.

[programmerjake]

Also, since python modifies refcounts and stuff, that part needs to be wrapped in UnsafeCell so Rust doesn't assume it's immutable.

[davidhewitt]

We need to make sure that we never can simultaneously hand out more than one &mut T or both &mut T and &T, so there should be some method of tracking that (like RefCell).

Agreed.

[kngwyu]

we'd have something like:

enum PyClassValue<T> {
    Borrowed(*const T),
    MutBorrowed(*mut T),
    Owned(ManuallyDrop<T>),
}

We never should do so.
If we are to expose Rust's references to Python world, we should handle the lifetime explicitly, or we'd seriously break Rust's safe rule(=no dangling pointer, no memory leak).

[davidhewitt]

If we are to expose Rust's references to Python world, we should handle the lifetime explicitly, or we'd seriously break Rust's safe rule(=no dangling pointer, no memory leak).

I agree with this in principle, but I don't think it's possible. As far as I know, Rust can't express the lifetime of the borrow in Python.

That's why a keep-alive mechanism is necessary. I can demonstrate a PR for this if you like.

[kngwyu]

@davidhewitt
Thank you for the proposal, but we should discuss that keep-alive feature in the other thread.
This PR does not intend to add new features to PyO3, but to clean up some dirty parts.

[davidhewitt]

👍 that makes total sense.

It's for reasons like this that I recommend we keep PyClassShell out of the public pyo3 API, if possible.

Then we can consider adding features like reference support without more breaking changes. I could then write a PR for that another time.

[kngwyu]

I added some additional features to this PR.

1. Safe inheritance of native types
   Currently this struct causes SIGSEGV when we use it as dict.

#[pyclass(extends=PyDict)]
struct DictWithName {
    #[pyo3(get(name))]
    _name: &'static str,
}

This SIGSEGV is fixed by properly using baseclass's tp_new.

2. Inhibits inheriting PyVarObject
   PyVarObject has a variable length so it can be problematic to inherit it with our current fixed layout strategy.
   E.g., now we cannot do

#[pyclass(extends=PyTuple)]
struct TupleWithName {
    _name: &'static str,
}

[programmerjake]

is the condition reversed here, erroring when NEED_INIT is false seems backwards.

[kngwyu]

erroring when NEED_INIT is false seems backwards

Sorry I'm not sure what you are saying..., but you mean we should not raise an error for redundant initialization?

I understand, thanks.

[programmerjake]

looks good!

[programmerjake]

shouldn't this be unsafe, it seems likely to be UB to invoke twice on the same shell object

[kngwyu]

Hmm 🤔 ... , I don't think it's UB.
init just do shell.pyclass = value.

[programmerjake]

doesn't this call the CPython init functions for the base classes? Those are likely to not be allowed to be called twice on the same object.

[kngwyu]

No, it's a part of tp_new and doesn't call init function.

[programmerjake]

ah, ok. Related, if shell is partially uninitialized, it would be UB for safe code to read from it, so it should have MaybeUninitialized and/or be passed in as *mut T::ConcreteLayout. Also, if later code depends on shell being initialized instead of being uninit, then that's also a good reason for init_class to be unsafe.

[davidhewitt]

PyVarObject has a variable length so it can be problematic to inherit it with our current fixed layout strategy.

We would presumably be able to work around this if the layout contained a pointer to the actual tuple object, e.g. *const PyTupleObject ?

This is actually how C++'s pybind11 does all its layout. The PyClassShell contains a pointer to the actual struct, as well as a pointer for each Python base.

[pganssle]

It is not for performance.
Current OFFSET based design has some problems

• Corner cases that cause SIGSEGV(e.g., #[pyclass(extends=PyDict)])
• We have to use specialization to distinguish Rust-native type and Python-native type
  So this PR mainly resolves these 2 problems, but it removes an if OFFSET == 0 branch from conversion codes.

It would still be good to know how it affects performance. Does this add overhead, and if so, how much? One thing to consider is that the more overhead PyO3 adds, the less likely it is to be adopted (at least from the Python side), since many people are expicitly looking for a safer way to get speed in their Python files. If PyO3 adds a ton of overhead over C or Cython, people will simply say, "Oh well it would be nice if we could have safety and speed, but I guess we can't."

If this sort of thing is super expensive but necessary to provide safety guarantees mainly in strange edge cases, it may make sense to have a middle-of-the-road "fast but possibly unsafe" interface for people who really care about speed and a "slow but safe" version for people whose bottlenecks are elsewhere.

[kngwyu]

Does this add overhead,

No.
This abstraction is zero-cost and does more things at compile time than the current implementation.
E.g., it removes if OFFSET == 0 branches from all conversion codes.

[kngwyu]

Important change for PyClassInitializer

I pushed the new PyClassInitializer that can prevent uninitialized base class at compile time.
Though this adds another boiler plate to PyTypeInfo, the implementation is quite simple and clean.
The interface employs std::process::Command like initializer pattern:

#[pymethods]
impl SubSubClass {
    #[new]
    fn new() -> PyClassInitializer<Self> {
        let base_init = PyClassInitializer::from(BaseClass{basename: "base"});
        base_init.add_subclass(SubClass{subname: "sub"})
                 .add_subclass(SubSubClass{subsubname: "subsub"})
    }
}

For simple inheritance where B inherits PyAny and S inherits B, we allow (S, B) as return type of #[new].

#[pymethods]
impl SubClass {
    #[new]
    fn new() -> (Self, BaseClass) {
        (SubClass { val2: 5 }, BaseClass { val1: 10 })
    }
}

cc: @pganssle

[davidhewitt]

I very much like the new PyClassInitializer, though have concerns about IntoInitializer as implemented. It's got a strange coupling to PyResult which I don't think we need.

I opened a PR against your fork to propose a simpler API for IntoInitializer. Further elaboration on my thoughts are also there. kngwyu#1

[davidhewitt]

Further to my thoughts about IntoInitializer and PyResult, the new (S, B) return type of #[new] is also creeping into the API for Py::new:

#[pyclass]
struct Base { }

#[pyclass(extends=Base)]
struct Sub { }

let sub = pyo3::Py::new(py, (Sub {}, Base {})).unwrap()

This doesn't seem desirable to me. What do others think?

(If we merge my suggested PR, then it would be as simple as changing Py::new to take impl Into<PyClassInitializer<T>> instead of impl IntoInitializer<T>.)

[kngwyu]

Following the line of @davidhewitt's simplification, I replaced IntoInitializer<T> with Into<PyClassInitializer<T>>.

[davidhewitt]

This new pub use crate::pyclass_init::PyClassInitializer; has split up a comment from the line beneath it.

[kngwyu]

👍

[davidhewitt]

Is it necessary to box super_init? This seems to be an allocation we may not need.

[kngwyu]

You're right, thank you.