fix python GC traversal for validators and serializers #787
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #787 +/- ##
==========================================
- Coverage 93.67% 93.53% -0.15%
==========================================
Files 99 100 +1
Lines 14322 14376 +54
Branches 25 25
==========================================
+ Hits 13416 13446 +30
- Misses 900 924 +24
Partials 6 6
Continue to review full report in Codecov by Sentry.
|
|
please review |
|
(I also removed the |
CodSpeed Performance ReportMerging #787 will not alter performanceComparing Summary
|
| impl<T: PyGcTraverse> PyGcTraverse for Vec<T> { | ||
| fn py_gc_traverse(&self, visit: &PyVisit<'_>) -> Result<(), PyTraverseError> { | ||
| for item in self { | ||
| item.py_gc_traverse(visit)?; | ||
| } | ||
| Ok(()) | ||
| } | ||
| } | ||
|
|
||
| impl<T: PyGcTraverse> PyGcTraverse for AHashMap<String, T> { | ||
| fn py_gc_traverse(&self, visit: &PyVisit<'_>) -> Result<(), PyTraverseError> { | ||
| for item in self.values() { | ||
| item.py_gc_traverse(visit)?; | ||
| } | ||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Seems like these might even make sense in PyO3 at some point, right?
There was a problem hiding this comment.
Yes, though probably not in this form. In PyO3 I'd like (if it's possible) to do it automatically with a #[derive] macro, however the challenge is how to make most types noops and enable gc traversal for just the ones that matter. I'm sure there's an issue about this in the PyO3 backlog but I can't find it right now.
|
In principle we would want to test all validators and serializers for all fields they accept which can contain python objects (to check that those fields are properly traversed), however that's a long set and wouldn't catch any changes to the implementations. At least the tests I added should cover the cases which are most likely to produce a cycle. |
Change Summary
This PR separates the
py_gc_traversemethods from serializers and validators out into a separatePyGcTraversetrait, forcing all types to provide an implementation.This gave me a mechanical way to check the traversal for all validators and serializers and implement them fully.
I also added a spattering of tests which attempt to generate cycles and leak them. They fail without this patch which gives me confidence this fixes the original
pydanticissue listed below.There is still no guarantee that the traversal implementations are correct, as they're hand-written, but this should make them prevalent enough to force us to remember to look at them.
Related issue number
pydantic/pydantic#6727
Checklist
pydantic-core(except for expected changes)Selected Reviewer: @adriangb