pymethods: support gc protocol

[davidhewitt]

This is the last step for #1884. Copies the same design for __traverse__ and __clear__ from PyGCProtocol onto #[pymethods].

It's not the cleanest implementation, but it works fine. Once merged, we can mark #[pyproto] as deprecated and get on with releasing 0.16 😄

[davidhewitt]

This check isn't needed on Python 3.11, however it is needed on older Python versions else #[pyclass(gc)] without __traverse__ will cause segfaults.

I think a better solution might be to deprecate #[pyclass(gc)] and then in src/pyclass.rs we can check for the presence of __traverse__ to decide whether to set the Py_TPFLAGS_HAVE_GC flag.

[davidhewitt]

I pushed a commit which deprecates #[pyclass(gc)] as suggested.

[davidhewitt]

I'll leave this one up over the weekend in case anyone wants to review! Then some time next week I'll merge, deprecate #[pyproto], and begin preparing the 0.16 release.

[birkenfeld]

It seems like you're generating a normal slot for __clear__, while under pyproto, pyo3::class::gc::clear did not have the callback mechanism, and no way to transport exceptions. Are you sure that exceptions can be raised normally in tp_clear? If yes, why do we have the special case for tp_traverse (PyTraverseError could be a normal PyError)?

[davidhewitt]

Good question. I'm not 100% on this; but what I do see is in the usage of tp_clear in the gc module, there's immediately following a check for errors.

https://github.com/python/cpython/blob/af32b3ef1fbad3c2242627a14398320960a0cb45/Modules/gcmodule.c#L1014

Whereas uses of tp_traverse never seem to check for errors. I get the impression that it's not expected for it to be possible to error during GC traversal.

[mejrs]

This piece of documentation still mentions pyclass(gc) and needs deleting: https://github.com/PyO3/pyo3/blob/main/pyo3-macros/src/lib.rs#L90

[davidhewitt]

Regarding error messages, it does indeed look like errors in tp_clear are allowed. With this test class

#[pyclass]
struct ErrorDuringClear {
    self_ref: PyObject,
}

#[pymethods]
impl ErrorDuringClear {
    fn __traverse__(&self, visit: PyVisit) -> Result<(), PyTraverseError> {
        visit.call(&self.self_ref)
    }

    fn __clear__(&mut self, py: Python) -> PyResult<()> {
        self.self_ref = py.None();
        Err(exceptions::PyValueError::new_err("clear failed"))
    }
}

I get the following output when the GC runs:

Exception ignored in tp_clear of: <class 'builtins.ErrorDuringClear'>
Traceback (most recent call last):
  File "<string>", line 1, in <module>
ValueError: clear failed

[davidhewitt]

This piece of documentation still mentions pyclass(gc) and needs deleting: https://github.com/PyO3/pyo3/blob/main/pyo3-macros/src/lib.rs#L90

Thanks, pushed!

[birkenfeld]

Ok, nice. The other question is then if tp_traverse can still execute any Python code (and then ignore the exception), otherwise it probably should be marked unsafe.

[davidhewitt]

Ok, nice. The other question is then if tp_traverse can still execute any Python code (and then ignore the exception), otherwise it probably should be marked unsafe.

I would argue even if it can't execute Python code, it doesn't need to be unsafe. I understand unsafe fn means it is not safe to call; the implementation could always use unsafe internally. In this case it isn't unsafe for other Rust code to call a correct implementation of __traverse__.

We should definitely document that a correct implementation of __traverse__ should do nothing more than use visit.call on all Python objects contained within the struct.

[davidhewitt]

I'm going to merge this shortly and proceed forwards with a final PR to deprecate #[pyproto].

[birkenfeld]

All the docs still need to be moved from proto-centric to method-centric, with (maybe?) mentioning the old proto stuff.

[davidhewitt]

Agreed, I'll do that in the next PR 👍

[birkenfeld]

Are you already working on the docs PR? If not I'd make a start today.

[davidhewitt]

See #2173 (comment) - please feel free to take over that branch!