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).

davidhewitt

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

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

Should be fixed in: 4f2510b

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?)

Fixed in 4f2510b

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

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


Can we maybe unify with structs using the fields here?

Please see 836340e, is this moving in the right direction?

codspeed · 2026-09-07T12:15:06Z

Merging this PR will degrade performance by 7.37%

⚠️

Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 1 improved benchmark
❌ 3 regressed benchmarks
✅ 137 untouched benchmarks
⏩ 6 skipped benchmarks¹

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Benchmark	`BASE`	`HEAD`	Efficiency
❌	`extract_biguint_small`	1.4 µs	1.6 µs	-13.92%
❌	`extract_biguint_zero`	1.4 µs	1.6 µs	-13.46%
❌	`into_u128_small`	820.5 ns	930.2 ns	-11.79%
⚡	`into_biguint_small`	1.5 µs	1.4 µs	+12.06%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.

_{Comparing bschoenmaeckers:gc-integration (c8c2a49) with main (ff1316b)}

6 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

bschoenmaeckers · 2026-09-07T12:53:09Z

This is ready for final review

Person-93

Looking forward to actually using this new feature.

Person-93 · 2026-09-10T16:05:05Z

-use crate::{ffi, Py};
+/// Trait describing how values participate in Python's cyclic garbage collector.
+///
+/// # Safety


Are there any requirements on the clear method?

I don't think so. But I might be wrong here.

I don't think so. But I might be wrong here.

Can it call arbitrary python code? Isn't there some subset that it's restricted to? Is there any chance that some of the python objects it has access to might not be valid any more?

My understanding is that it is allowed but should be avoided. As clearing python objects may run their destructor it will call python code is some cases anyway. This should be safe because we only allow clearing for mutable types that can be safely cleared like Option.

Person-93 · 2026-09-10T16:25:13Z

+/// ```
+#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
+#[repr(transparent)]
+pub struct PyGcOpaque<T>(T);


Bike-shedding comment ahead; ignore if it's too soon for this.

The Py type is a smart pointer, PyGcOpaque might be taken to mean this is some kind of smart pointer as well.

… match statement

davidhewitt

Thanks, very much hoping we can ship this in 0.30 and start migrating towards a more correct / complete GC story. A number of review points.

davidhewitt · 2026-09-14T14:07:54Z

+// SAFETY: traversing `PyErr` only reports the normalized exception object and does
+// not execute arbitrary Python code.


I think there's a potential sad case here that the in the lazy state the boxed callback might be hiding a cycle... but IMO there's not much we can reasonably do about that and yet another reason the lazy normalization must die.

This reverts commit d6217e4.

This reverts commit b1c74a6.

This reverts commit 7a7af89.

This reverts commit 0b91496.

This reverts commit dc228de.

Tpt

naive questions to try simplify the implementation.

Tpt · 2026-09-16T19:20:50Z

+
+    #[inline]
+    fn traverse(&self, visit: PyVisit<'_>) -> Result<(), PyTraverseError> {
+        if T::MAY_CONTAIN_CYCLES {


Likely dumb question (please ignore): is it worth it to do this check here and in some other implementations? If the nested clear() optimization is not doing anything like it is for type that do not contain cycles then the compiler is likely to optimize calls away.

Tpt · 2026-09-16T19:22:27Z

+
+    #[inline]
+    fn clear(&mut self) {
+        if T::MAY_CONTAIN_CYCLES {


Similarly, is it explicit if worth it? I guess that clear() is called most often before dropping the object so, e.g. clearing a vec explicitly then dropping it even if it does not contain any python object is not likely to be much more costly than just dropping it full (code needs to iterate on destructors either way).

Person-93 · 2026-09-17T02:32:24Z

    #[cfg(wip_feature_std)]
-    pub use std::sync::MutexGuard;
+    pub use {
+        crate::{PyGcTraversable, PyTraverseError, PyVisit},


I don't think these should be re-exported. They should have a regular use, not be part of the existing pub use.

Person-93 · 2026-09-17T02:42:13Z

+    }
+
+    #[inline]
+    fn clear(&mut self) {


I am not sure if clearing the reference is the correct behavior. The reference might refer to data that is outside of the python heap (arguably a GC root), and clearing it would be surprising to the user.

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

gate eprintln calls for no_std #6261

Merged

This was referenced Aug 26, 2026

0.30 release #6126

Open

internal: no std mutex #6058

Merged

bschoenmaeckers added 7 commits September 1, 2026 18:15

Implement PyGcTraversable for Mutex & RwLock

caf2e4a

Merge remote-tracking branch 'origin/main' into gc-integration

89a757e

Implement PyGcTraversable for Arc<T>

0b91496

Remove PyGcTraversable and PyGcOpaque from prelude

ff540b5

Add PyGcOpaque doc examples for map keys and cycle breakpoints

7761f0e

Allow deriving PyGcTraversable for empty enums

3e2e9bf

Implement PyGcTraversable for Weak<T>

7a7af89

bschoenmaeckers force-pushed the gc-integration branch from 53fd0a8 to 7a7af89 Compare September 3, 2026 10:50

bschoenmaeckers added 6 commits September 3, 2026 11:52

Restore derive_attributes.rs line endings

d1b482e

Clarify PyGcTraversable safety docs for Python::attach

f5f10e5

Merge remote-tracking branch 'origin/main' into gc-integration

6d1e459

impl PyGcTraversable for PyBackedStr & PyBackedBytes

d1a3550

Impl PyGcTraversable for Rc<T> and rc::Weak<T>

b1c74a6

Add PyGcTraversable impl for PyErr

d8f3763

bschoenmaeckers added 2 commits September 7, 2026 14:43

Add helpers for build_derive_py_gc_integration

a5b8e22

Fix clippy

aa0d095

bschoenmaeckers force-pushed the gc-integration branch from 836340e to aa0d095 Compare September 7, 2026 12:46

Ignore unknown attributes

4f2510b

bschoenmaeckers marked this pull request as ready for review September 7, 2026 12:52

bschoenmaeckers added 2 commits September 7, 2026 16:02

Fix recursive example

e714ba8

Update PyGcOpaque docs

12af2ce

Person-93 suggested changes Sep 10, 2026

View reviewed changes

bschoenmaeckers added 3 commits September 10, 2026 20:13

PyBackedStr & PyBackedBytes will never contain cycles

d6217e4

Reduce code duplication when we already check MAY_CONTAIN_CYCLES in…

ea36eaf

… match statement

Document that poisoned locks will be traversed

a184234

davidhewitt mentioned this pull request Sep 14, 2026

implement Copy for PyVisit #6412

Merged

davidhewitt reviewed Sep 14, 2026

View reviewed changes

bschoenmaeckers added 6 commits September 14, 2026 16:38

Revert "PyBackedStr & PyBackedBytes will never contain cycles"

eef1b47

This reverts commit d6217e4.

Revert "Impl PyGcTraversable for Rc<T> and rc::Weak<T>"

bdc0c36

This reverts commit b1c74a6.

Revert "Implement PyGcTraversable for Weak<T>"

ea23291

This reverts commit 7a7af89.

Revert "Implement PyGcTraversable for Arc<T>"

672dd90

This reverts commit 0b91496.

Revert "Add Cow impl for PyGcTraversable"

73d0850

This reverts commit dc228de.

Do not lock in PyErr impl

a0ddbe2

Person-93 suggested changes Sep 14, 2026

View reviewed changes

Comment thread src/platform/sync.rs Outdated

bschoenmaeckers added 6 commits September 15, 2026 09:49

Merge remote-tracking branch 'origin/main' into gc-integration

cc5c889

Fix doc example when Arc does not implement PyGcTraversable

cd44aaf

Do not clone PyVisit

ec1b362

Bless UI test

833b527

Impl PyGcTraversable for parking_lot::Mutex<T>

dc995e7

Add ?Sized to some impl's

c8c2a49

Tpt reviewed Sep 16, 2026

View reviewed changes

Viicos mentioned this pull request Sep 16, 2026

Refcount corruption during GC when a mode='wrap' validator's handler outlives validation (shared validator tree traversed twice) pydantic/pydantic#13817

Open

1 task

Person-93 suggested changes Sep 17, 2026

View reviewed changes

		// SAFETY: traversing `PyErr` only reports the normalized exception object and does
		// not execute arbitrary Python code.

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!

Uh oh!

davidhewitt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codspeed Bot commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will degrade performance by 7.37%

Performance Changes

Footnotes

Uh oh!

bschoenmaeckers commented Sep 7, 2026

Uh oh!

Person-93 left a comment

Choose a reason for hiding this comment

Uh oh!

bschoenmaeckers commented Aug 19, 2026 •

edited

Loading

Person-93 Aug 20, 2026 •

edited

Loading

bschoenmaeckers Aug 22, 2026 •

edited

Loading

codspeed Bot commented Sep 7, 2026 •

edited

Loading