Fix list.__repr__ deadlock#7275
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: Error: 'frozenset' object has no attribute 'discard' Legend:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/builtins/list.rs`:
- Around line 533-555: The loop captures a snapshot in elements and only
refreshes it when length changes, which yields stale items if an element's
__repr__ mutates contents without resizing; update the loop to re-fetch the
current element by index on every iteration instead of indexing into the stale
elements snapshot (use zelf.borrow_vec() each iteration or fetch a cloned
element via zelf.borrow_vec().get(i).cloned().unwrap() before calling
item.repr(vm)), and keep using zelf.__len__() to track size changes; adjust
variables: elements, item, i, and the repr call (item.repr(vm)) so the borrow
does not persist across the repr call.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Lib/test/test_list.pyis excluded by!Lib/**
📒 Files selected for processing (1)
crates/vm/src/builtins/list.rs
| let mut elements = zelf.borrow_vec().to_vec(); | ||
| let mut size = zelf.__len__(); | ||
| let mut first = true; | ||
| let mut i = 0; | ||
| while i < size { | ||
| if elements.len() != size { | ||
| // `repr` mutated the list. refetch it. | ||
| elements = zelf.borrow_vec().to_vec(); | ||
| } | ||
|
|
||
| let item = &elements[i]; | ||
|
|
||
| if first { | ||
| first = false; | ||
| } else { | ||
| writer.push_str(", "); | ||
| } | ||
|
|
||
| writer.push_wtf8(item.repr(vm)?.as_wtf8()); | ||
|
|
||
| size = zelf.__len__(); // Refetch list size as `repr` may mutate the list. | ||
| i += 1; | ||
| } |
There was a problem hiding this comment.
Refresh elements by index each iteration, not only on length changes.
The current logic only refetches when length changes. If an element __repr__ mutates list contents without changing length, repr can emit stale items (snapshot values), which is a correctness mismatch.
Suggested fix
- let mut elements = zelf.borrow_vec().to_vec();
- let mut size = zelf.__len__();
- let mut first = true;
- let mut i = 0;
- while i < size {
- if elements.len() != size {
- // `repr` mutated the list. refetch it.
- elements = zelf.borrow_vec().to_vec();
- }
-
- let item = &elements[i];
-
- if first {
- first = false;
- } else {
- writer.push_str(", ");
- }
-
- writer.push_wtf8(item.repr(vm)?.as_wtf8());
-
- size = zelf.__len__(); // Refetch list size as `repr` may mutate the list.
- i += 1;
- }
+ let mut i = 0;
+ while let Some(item) = {
+ let elements = zelf.borrow_vec();
+ elements.get(i).cloned()
+ } {
+ if i != 0 {
+ writer.push_str(", ");
+ }
+ writer.push_wtf8(item.repr(vm)?.as_wtf8());
+ i += 1;
+ }📝 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.
| let mut elements = zelf.borrow_vec().to_vec(); | |
| let mut size = zelf.__len__(); | |
| let mut first = true; | |
| let mut i = 0; | |
| while i < size { | |
| if elements.len() != size { | |
| // `repr` mutated the list. refetch it. | |
| elements = zelf.borrow_vec().to_vec(); | |
| } | |
| let item = &elements[i]; | |
| if first { | |
| first = false; | |
| } else { | |
| writer.push_str(", "); | |
| } | |
| writer.push_wtf8(item.repr(vm)?.as_wtf8()); | |
| size = zelf.__len__(); // Refetch list size as `repr` may mutate the list. | |
| i += 1; | |
| } | |
| let mut i = 0; | |
| while let Some(item) = { | |
| let elements = zelf.borrow_vec(); | |
| elements.get(i).cloned() | |
| } { | |
| if i != 0 { | |
| writer.push_str(", "); | |
| } | |
| writer.push_wtf8(item.repr(vm)?.as_wtf8()); | |
| i += 1; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/vm/src/builtins/list.rs` around lines 533 - 555, The loop captures a
snapshot in elements and only refreshes it when length changes, which yields
stale items if an element's __repr__ mutates contents without resizing; update
the loop to re-fetch the current element by index on every iteration instead of
indexing into the stale elements snapshot (use zelf.borrow_vec() each iteration
or fetch a cloned element via zelf.borrow_vec().get(i).cloned().unwrap() before
calling item.repr(vm)), and keep using zelf.__len__() to track size changes;
adjust variables: elements, item, i, and the repr call (item.repr(vm)) so the
borrow does not persist across the repr call.
2 participants
closes #6591
Summary by CodeRabbit