traverse and clear#6780
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThis PR systematically refactors the garbage collection traversal system across RustPython, transitioning from an older "trace" mechanism to a "traverse" model with explicit support for circular reference clearing via new HAS_TRAVERSE, HAS_CLEAR flags and optional clear methods on types. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Sorry, something went wrong.
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin traverse |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/vm/src/object/core.rs (1)
95-100: Doc comment still saystry_trace.Please update the comment to
try_traverseto match the new function name.✏️ Suggested doc fix
-/// Call `try_trace` on payload +/// Call `try_traverse` on payload
🤖 Fix all issues with AI agents
In `@crates/derive-impl/src/pyclass.rs`:
- Around line 588-595: The code currently allows `clear` to be true while
`traverse` is absent which leaves `HAS_TRAVERSE` false and makes GC traversal a
no-op; update the logic around `has_traverse` and `has_clear` in the block using
`class_meta.inner()._has_key` and `class_meta.inner()._bool` to validate that if
`clear` is present and true then `traverse` must also be present (or return an
error) — e.g. after computing `has_clear` check `if has_clear && !has_traverse {
return Err(...) }` referencing the same symbols (`has_traverse`, `has_clear`,
`class_meta.inner()._has_key`, `_bool`, `HAS_TRAVERSE`) so the parser fails the
derive when `clear` is enabled without `traverse`.
- Around line 643-644: The generated signature for try_traverse is using
::rustpython_vm::object::TraverseFn without its required lifetime; update the
generated code so all uses of TraverseFn include the lifetime parameter (e.g.
TraverseFn<'a>) and propagate that 'a into the impl/generics for try_traverse
and any surrounding impl blocks where tracer_fn is referenced (look for the
try_traverse function and references to ::rustpython_vm::object::TraverseFn and
add the 'a lifetime consistently).
In `@crates/vm/src/builtins/function.rs`:
- Around line 54-58: The traversal currently grabs blocking locks (e.g.
self.type_params.lock().traverse(...), self.annotations.lock().traverse(...),
self.module.lock().traverse(...), self.doc.lock().traverse(...)) which can block
the GC and deadlock; instead call the Traverse implementation on the PyMutex
fields directly (use the PyMutex's non-blocking traverse method or
try_lock-based impl) — replace usages of .lock().traverse(tracer_fn) with the
PyMutex/field's traverse(tracer_fn) so traversal uses the non-blocking path for
type_params, annotations, module and doc.
🧹 Nitpick comments (1)
crates/vm/src/object/traverse_object.rs (1)
63-75: Stale comments reference old "trace" terminology.The doc comment on line 64 and inline comment on line 67 still reference
try_tracewhile the actual code callsT::try_traverse. Consider updating for consistency with the refactor.📝 Suggested comment updates
unsafe impl<T: MaybeTraverse> Traverse for PyInner<T> { - /// Type is known, so we can call `try_trace` directly instead of using erased type vtable + /// Type is known, so we can call `try_traverse` directly instead of using erased type vtable fn traverse(&self, tracer_fn: &mut TraverseFn<'_>) { // 1. trace `dict` and `slots` field(`typ` can't trace for it's a AtomicRef while is leaked by design) - // 2. call corresponding `try_trace` function to trace payload + // 2. call corresponding `try_traverse` function to traverse payload // (No need to call vtable's trace function because we already know the type) // self.typ.trace(tracer_fn); self.dict.traverse(tracer_fn);
Sorry, something went wrong.
9301ae2
into
RustPython:main
Jan 18, 2026
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.