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 |
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin traverse |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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);
| let has_traverse = class_meta.inner()._has_key("traverse")?; | ||
| let has_clear = if class_meta.inner()._has_key("clear")? { | ||
| // If clear attribute is present, use its value | ||
| class_meta.inner()._bool("clear")? | ||
| } else { | ||
| // If clear attribute is absent, default to same as traverse | ||
| has_traverse | ||
| }; |
There was a problem hiding this comment.
Require traverse when clear is enabled.
If clear is set without traverse, HAS_TRAVERSE stays false and GC traversal becomes a no-op.
🔧 Proposed fix
let has_traverse = class_meta.inner()._has_key("traverse")?;
let has_clear = if class_meta.inner()._has_key("clear")? {
// If clear attribute is present, use its value
class_meta.inner()._bool("clear")?
} else {
// If clear attribute is absent, default to same as traverse
has_traverse
};
+if has_clear && !has_traverse {
+ bail_span!(
+ item,
+ "`clear` requires `traverse` (use `traverse` or `traverse = \"manual\"`)"
+ );
+}🤖 Prompt for AI Agents
In `@crates/derive-impl/src/pyclass.rs` around lines 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`.
| fn try_traverse(&self, tracer_fn: &mut ::rustpython_vm::object::TraverseFn) { | ||
| #try_traverse_body |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for TraverseFn definition
rg -n "type TraverseFn|struct TraverseFn|trait TraverseFn" crates/vm/src/object -A 3 -B 1Repository: RustPython/RustPython
Length of output: 186
🏁 Script executed:
# Check current code at lines 643-644 in the file
sed -n '640,650p' crates/derive-impl/src/pyclass.rsRepository: RustPython/RustPython
Length of output: 186
🏁 Script executed:
# Search for other uses of TraverseFn in the codebase to see patterns
rg -n "TraverseFn" crates/ --type rs | head -20Repository: RustPython/RustPython
Length of output: 186
🌐 Web query:
RustPython TraverseFn lifetime generic type definition
💡 Result:
Definition (from rustpython-vm crate):
pub type TraverseFn<'a> = dyn FnMut(&PyObject) + 'a. [1][2]
Sources:
- rustpython-vm API docs — TraverseFn type alias. [1]
- rustpython-vm source (object/mod.rs) showing the type alias. [2]
🌐 Web query:
RustPython vm object TraverseFn signature
💡 Result:
I can look that up in the RustPython source (to give the exact current Rust signature). Do you want me to fetch the signature from the repository (GitHub/docs.rs) now?
Fix the missing lifetime on TraverseFn in generated signatures.
The TraverseFn type is defined as dyn FnMut(&PyObject) + 'a and requires a lifetime parameter. Generated code must include it for type-checking.
🔧 Proposed fix
- fn try_traverse(&self, tracer_fn: &mut ::rustpython_vm::object::TraverseFn) {
+ fn try_traverse(&self, tracer_fn: &mut ::rustpython_vm::object::TraverseFn<'_>) {
`#try_traverse_body`
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn try_traverse(&self, tracer_fn: &mut ::rustpython_vm::object::TraverseFn) { | |
| #try_traverse_body | |
| fn try_traverse(&self, tracer_fn: &mut ::rustpython_vm::object::TraverseFn<'_>) { | |
| `#try_traverse_body` |
🤖 Prompt for AI Agents
In `@crates/derive-impl/src/pyclass.rs` around lines 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).
| // Traverse additional fields that may contain references | ||
| self.type_params.lock().traverse(tracer_fn); | ||
| self.annotations.lock().traverse(tracer_fn); | ||
| self.module.lock().traverse(tracer_fn); | ||
| self.doc.lock().traverse(tracer_fn); |
There was a problem hiding this comment.
Avoid blocking locks in traverse to prevent GC deadlocks.
PyMutex::lock() can block if the lock is held when GC runs, which can deadlock traversal. Prefer the Traverse impl for PyMutex (uses try_lock) instead of manual locking.
✅ Safer traversal (non-blocking)
- self.type_params.lock().traverse(tracer_fn);
- self.annotations.lock().traverse(tracer_fn);
- self.module.lock().traverse(tracer_fn);
- self.doc.lock().traverse(tracer_fn);
+ self.type_params.traverse(tracer_fn);
+ self.annotations.traverse(tracer_fn);
+ self.module.traverse(tracer_fn);
+ self.doc.traverse(tracer_fn);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Traverse additional fields that may contain references | |
| self.type_params.lock().traverse(tracer_fn); | |
| self.annotations.lock().traverse(tracer_fn); | |
| self.module.lock().traverse(tracer_fn); | |
| self.doc.lock().traverse(tracer_fn); | |
| // Traverse additional fields that may contain references | |
| self.type_params.traverse(tracer_fn); | |
| self.annotations.traverse(tracer_fn); | |
| self.module.traverse(tracer_fn); | |
| self.doc.traverse(tracer_fn); |
🤖 Prompt for AI Agents
In `@crates/vm/src/builtins/function.rs` around lines 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.
1 participant
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.