RFC: automatic garbage collector derives

[davidhewitt]

One of the things I want to see improved most about the current PyO3 implementation is our support for the Python GC. It is currently implemented entirely by hand by users via the __traverse__ and __clear__ functions in #[pymethods].

Both of these functions are both relatively simple to implement as they have quite well specified behaviour. Yet, we ask users to do this manually and worse, if users don't do this properly, it leads to memory leaks. My experience working on pydantic-core (with complex object trees) is that it's easy to get a __traverse__ or clear implementation wrong.

I think one major step we can do to improve this is to introduce a new (automatically-derivable) trait which defines the correct behaviour, and require all pyclasses to implement this trait. We can probably even go further and automatically derive it as part of #[pyclass] (maybe with an opt-opt #[pyclass(derive_gc = false)].

Historically, I have thought that we needed specialization to make this trait performant and easily usable. Given that specialization is unlikely to be a solution for the foreseeable future, I've been wondering what we can do without it.

I think the following trait definition might be sufficient to allow us to move forward without specialization.

/// Unsafe to implement because `traverse` is not allowed to execute arbitrary Python code
unsafe trait PyGcIntegration {
    /// Helper constant which allows optimization by eliminating needless calls to sub-structures
    /// 
    /// Setting this to `false` will cause many implemenations to never call this type's implementation, which
    /// will lead to memory leaks if set incorrectly.
    const MAY_CONTAIN_CYCLES: bool;

    fn traverse(&self, visit: PyVisit) -> Result<(), PyTraverseError>;

    // TBC: is the `&mut self` a problem? Seems necessary, but `frozen` pyclasses may not
    // support mutable access. There _might_ be something we can do which automatically locks
    // mutexes etc.
    fn clear(&mut self);
}

Example derive

#[derive(PyGcIntegration)]
struct Foo {
   a: A,
   b: B,
}

unsafe impl PyGcIntegration for Foo {
    const MAY_CONTAIN_CYCLES: bool = A::MAY_CONTAIN_CYCLES || B::MAY_CONTAIN_CYCLES;
    
    fn traverse(&self, visit: PyVisit) -> Result<(), PyTraverseError> {
        if A::MAY_CONTAIN_CYCLES {
            self.a.traverse(visit)?;
        }
        if B::MAY_CONTAIN_CYCLES {
            self.b.traverse(visit)?;
        }
        Ok(())
    }

    fn clear(&mut self, visit: PyVisit) {
        if A::MAY_CONTAIN_CYCLES {
            self.a.clear()
        }
        if B::MAY_CONTAIN_CYCLES {
            self.b.clear()
        }
    }
}

I think clear might be "shallow" at the first level of mutability, e.g. for Option:

unsafe impl PyGcIntegration for Option<T> {
    const MAY_CONTAIN_CYCLES: bool = T::MAY_CONTAIN_CYCLES;

    fn traverse(&self, visit: PyVisit) -> Result<(), PyTraverseError> {
        if T::MAY_CONTAIN_CYCLES && let Some(inner) = self {
            inner.traverse(visit)?;
        }
        Ok(())
    }

    fn clear(&mut self, visit: PyVisit) {
        // eliminate all state in the `Option`, but only if the values might be part of cycles
        if T::MAY_CONTAIN_CYCLES {
            *self = None;
        }
    }
}

... while I think there's a ton of semantics which need to be clarified, I'm curious to see if we can make this work and replace hand-written __traverse__ and __clear__ with reliable derived functionality.

[Tpt]

Sounds great! I got a bit puzzled with how to support containers but it seems one can just implement PyGcIntegration on them. For example if PyGcIntegration is implemented on Vec<T> where T: PyGcIntegration then the derivation on this struct should work properly:

struct MyContainer {
    inner: Vec<Py<PyAny>>
}

Edit: a concern if we implement it inside of pyclass: how to distinguish between types on which traverse and clear should be called? If I have

#[pyclass]
struct MyClass {
    field: usize
}

Adding derive_gc = false just because PyGcIntegration is not implemented on usize would be sad.

[davidhewitt]

Yep, exactly.

Adding derive_gc = false just because PyGcIntegration is not implemented on usize would be sad.

I think we would derive PyGcIntegration on usize and make it a noop? I think that in general we would want users to require all types to implement GC integration so they're forced to make a choice.

We might allow:

#[pyclass]
struct MyClass {
    #[pyo3(gc = false)]
    field: usize
}

to opt-out on individual fields, but I think if we did that I would want to set it up so that #[pyo3(gc = false)] on a field which does implement PyGcIntegration is an error.

[Tpt]

I think we would derive PyGcIntegration on usize and make it a noop?

If we do that doesn't it mean we require users to make sure that PyGcIntegration is implemented on all types they use as pyclass fields? It might lead to a cumbersome experience when these types are from other crates.

but I think if we did that I would want to set it up so that #[pyo3(gc = false)] on a field which does implement PyGcIntegration is an error.

Yes!

[davidhewitt]

If we do that doesn't it mean we require users to make sure that PyGcIntegration is implemented on all types they use as pyclass fields? It might lead to a cumbersome experience when these types are from other crates.

Agreed, I think this is why we would allow types without an implementation to be opt-out using the #[pyo3(gc = false)] annotation.

I think our choice would be between making users explicitly mark everything (more correct, more verbose), which is what I propose here, or to automatically skip fields without PyGcIntegration. That is simpler for users intially but makes it easy for types like struct A(Py<PyAny>) to forget a PyGcIntegration implementation and end up with gc leaks...

[Tpt]

think our choice would be between making users explicitly mark everything (more correct, more verbose), which is what I propose here

Seems like a good approach if we can setup a good error message pointing users into the good direction (either write #[pyo3(gc = false)] or implement the trait)