Add `PyGcTraversable` derive by bschoenmaeckers · Pull Request #6330 · PyO3/pyo3

bschoenmaeckers · 2026-08-19T16:42:46Z

This adds a derive for the PyGcTraversable trait. This trait is not wired to the pyclass macro just yet but it can already be used manually by calling it in the __traverse__ & __clear__ methods.

It forces the user to implement PyGcTraversable on all fields or explicit disabling it using #[pyo3(gc = false)]. When setting #[pyo3(gc = false)] on a struct that implements PyGcTraversable will result in a compiler error.

To facilitate a escape hatch to prevent infinite recursion I've added a wrapper type PyGcOpaque that will stop visiting that type. This gives finer control on which parts of a (external) type should be traversed.

ref #5663

Person-93 · 2026-08-20T07:52:44Z

+        visit.call(self)
+    }
+
+    fn clear(&mut self) {}


This doesn't actually clear anything and may cause leaks.

I suppose that it can't really clear itself as Py unsafely assumes that it always references a live python object, should that change?

AFAICT, the only place where it's currently possible to see a cleared object is in its Drop impl, so maybe we can add an extra safety requirement to PyGcTraversable that implementors can't use Py fields in their Drop impl.

I recall a lua engine that did something like this by having an unsafe marker trait TrustedDrop and their GC trait depended on that.

If you derived their GC trait, it'd generate an empty drop impl by default and impl TrustedDrop as well. There was an option in the derive macro to leave this out and the user would have to unsafely implement the marker trait.

I guess you cannot create loops without a mutable type somewhere in the cycle. So a empty clear is fine here.

I guess you cannot create loops without a mutable type somewhere in the cycle. So a empty clear is fine here.

How do you know that the Py isn't a reference to a mutable object? Can't it be anything?

I'm unsure if we need to do anything here; my understanding is that as long as we allow the GC to see the full cycle it'll choose the point to call tp_clear to break it. Breaking any one edge in the cycle should be enough to collect it all.

But maybe it's possible to just set self to py.None() in order to be sure that the cycle gets broken? That seems like it could make a lot of clear implementations relatively inefficient, but maybe inefficient clear is a corner case we don't care about.

We should be able to set a Py<T> to None where T == PyAny. We could use TypeId for this. This requires a ’static bound which I think is fine?

For all other cases not clearing it is probably enough and if it’s not, users could always implement __clear__ manually.

Ah good point about type confusion, I think it'd be weird to have a special case for PyAny. Probably it's best to not clear and recommend using Option if tight cycles are possible?

For all other cases not clearing it is probably enough and if it’s not, users could always implement __clear__ manually.

What would a manual implementation of clear for the Node in my example even look like?

Probably it's best to not clear and recommend using Option if tight cycles are possible?

I suppose that'd be an improvement over the way traversal is handled now.

Person-93 · 2026-08-20T08:30:39Z

Why do all the containers drop their contained item(s)? Shouldn't they call clear on them?

I guess this is answered by my other comment about Py's clear being a noop.

I guess this is answered by my other comment about Py's clear being a noop.

You are right. Clear should break cycles, so the most effected way for a container is to drop all items.

Clear should break cycles, so the most effected way for a container is to drop all items.

From cpython docs:

Any non-trivial cleanup should be performed in tp_finalize instead of tp_clear.

Dropping an arbitrary type can do anything. That includes non-trivial things. I think it'd be better to call the contents' implementation of PyGcTraversable::clear

It seems like the difficulty with clearing a Py might be a reason to do it this way, but doing it this way isn't sufficient to prevent leaks from Py instances..

#[pyclass] #[derive(PyGcTraversable)] struct Thing { oh_no: Py<PyAny>, }

If a Thing::oh_no is set to a python reference to the Thing, the cycle will be detected, but not cleared.

Dropping an arbitrary type can do anything. That includes non-trivial things. I think it'd be better to call the contents' implementation of PyGcTraversable::clear

I don't think this will work, as we have to clear it somewhere. By just calling clear on the inner value wastes a possible break point.

It seems like the difficulty with clearing a Py might be a reason to do it this way, but doing it this way isn't sufficient to prevent leaks from Py instances..

Sorry you are right. Py<T> might hold a cycle. What about setting setting it to None when we have a Py?

CPython docs recommend setting PyObject pointers to NULL.

From the docs

A cleared object is a partially destroyed object; the object is not obligated to satisfy design invariants held during normal use.
....
Implementations of tp_clear should drop the instance’s references to those of its members that may be Python objects, and set its pointers to those members to NULL

Making Drop for Py<T> resilient to NULL pointers may be an option here.

Another insight; in most cases clear does not have to be perfect when traverse is implemented correctly. As Python will call clear on all nodes in a circle. So in most cases there will be at least one break point. For your example of just 1 pyo3 class referencing itself this does obviously not work.

I noticed this while looking at pydantic-core's gc integration. They only implement traverse and rely on other types in the cycle to break the loop. Without ever implementing clear on their own types.

I put similar thoughts in #6330 (comment).

I think Py<T> cannot safely hold NULL without a lot of breakage, so setting to Python None is the only "correct default" we can offer, I think.

Perhaps we could give users a #[pyo3(clear = false)] attribute which would enable them to avoid the overhead of setting to None (if it turns out to be meaningful).

Person-93 · 2026-08-20T08:34:40Z

I notice this doesn't implement it for Mutex or RwLock, is that intentional?

I am not 100% sure of the implications, as they may introduce deadlocks. So I left them out for now.

I am not 100% sure of the implications, as they may introduce deadlocks. So I left them out for now.

The traverse methods can call try_{lock/read/write} so they won't block. It's incomplete, but better than nothing.

The call methods take &mut self so they can access the data without locking. https://doc.rust-lang.org/std/sync/struct.Mutex.html#method.get_mut

davidhewitt

Thanks very much for driving this forward, I've really wanted this but not been able to make progress myself.

davidhewitt · 2026-08-21T10:19:40Z

Do we really want these to be in the prelude?

davidhewitt · 2026-08-21T10:20:06Z

I think can revert this as it's fixed on main

davidhewitt · 2026-08-21T10:22:14Z

+
+// SAFETY: Shared references do not own data; forwarding traversal is correct and
+// clear is a no-op because `&T` cannot clear through immutable access.
+unsafe impl<T: ?Sized + PyGcTraversable> PyGcTraversable for &T {


Is there value on traversal for references? Python types cannot safely hold these.

davidhewitt · 2026-08-21T10:22:59Z

+///
+/// # Safety
+///
+/// Implementations must not execute arbitrary Python code from `traverse`.


Maybe we should be more direct and say that this includes calling Python::attach.

davidhewitt · 2026-08-22T08:57:39Z

+        visit.call(self)
+    }
+
+    fn clear(&mut self) {}


Ah good point about type confusion, I think it'd be weird to have a special case for PyAny. Probably it's best to not clear and recommend using Option if tight cycles are possible?

davidhewitt · 2026-08-22T10:08:42Z

+            Err(lookahead.error())
+        }
+    }
+}


From memory we have to accept and ignore IntoPyObject and FromPyObject derive attributes here (maybe re-emit them)? See #5070

davidhewitt · 2026-08-22T10:11:05Z

+    ensure_spanned!(
+        options.transparent.is_none(),
+        options.transparent.span() => "`transparent` is not supported for `#[derive(PyGcTraversable)]`"
+    );
+    ensure_spanned!(
+        options.from_item_all.is_none(),
+        options.from_item_all.span() => "`from_item_all` is not supported for `#[derive(PyGcTraversable)]`"
+    );
+    ensure_spanned!(
+        options.annotation.is_none(),
+        options.annotation.span() => "`annotation` is not supported for `#[derive(PyGcTraversable)]`"
+    );
+    ensure_spanned!(
+        options.rename_all.is_none(),
+        options.rename_all.span() => "`rename_all` is not supported for `#[derive(PyGcTraversable)]`"
+    );


I think we have to ignore these without asserting due to conflicts with e.g. FromPyObject / IntoPyObject on the same type (maybe have a test?)

davidhewitt · 2026-08-22T10:11:33Z

+            ensure_spanned!(
+                !data.variants.is_empty(),
+                tokens.span() => "cannot derive `PyGcTraversable` for empty enum"
+            );


I guess we can derive as a noop?

davidhewitt · 2026-08-22T10:12:29Z

+                match &variant.fields {
+                    Fields::Named(named) => {


Can we maybe unify with structs using the fields here?

davidhewitt · 2026-08-22T10:13:46Z

+/// Wrapper to explicitly opt out of GC traversal for a type.
+///
+/// This is useful for intentional recursion breakpoints where traversing a
+/// reference would recurse indefinitely. Only use this when the wrapped value
+/// is known to be traversed through another path.


Could we please add an example doc here to help explain when this is necessary? (For me, as well as users 😂 )

bschoenmaeckers force-pushed the gc-integration branch from 122ca0f to 06cd6d4 Compare August 19, 2026 17:18

Person-93 suggested changes Aug 20, 2026

View reviewed changes

davidhewitt reviewed Aug 22, 2026

View reviewed changes

bschoenmaeckers added 7 commits August 22, 2026 17:52

Add PyGcTraversable derive

965eb0c

Fix fmt/clippy

99fb5b7

Add changelog

b67e5cd

Fix ci

c553839

Add Cow impl for PyGcTraversable

dc228de

Impl for Atomic types

d117edc

Remove &T impl

b99d641

bschoenmaeckers force-pushed the gc-integration branch from 4b4ebc8 to b99d641 Compare August 22, 2026 16:53

davidhewitt mentioned this pull request Aug 25, 2026

internal: schedule buffer to release later instead of leaking #6344

Closed

Person-93 mentioned this pull request Aug 25, 2026

use PySys_WriteStderr instead of eprintln #6261

Open

Uh oh!

Conversation

bschoenmaeckers commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Person-93 Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bschoenmaeckers Aug 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bschoenmaeckers Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidhewitt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

bschoenmaeckers commented Aug 19, 2026 •

edited

Loading

Person-93 Aug 20, 2026 •

edited

Loading

bschoenmaeckers Aug 22, 2026 •

edited

Loading

bschoenmaeckers Aug 20, 2026 •

edited

Loading