Skip to content

Fix PyList.remove #4326

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jan 4, 2023
Merged

Fix PyList.remove #4326

merged 4 commits into from
Jan 4, 2023

Conversation

yt2b
Copy link
Contributor

@yt2b yt2b commented Dec 10, 2022

#4315
I added comparison between index and list size in PyList.remove.

@DimitrisJim
Copy link
Member

could you add the code snippet supplied in the issue as a test case in extra_tests/snippets? there's already be a file for lists (extra_tests/snippets/builtin_list.py), you could just add the snippet there.

@DimitrisJim
Copy link
Member

also, this does actually fix one of the tests I mentioned in the issue, if you remove the skip decorator from test_count_index_remove_crashes it should pass just fine.

@yt2b
Copy link
Contributor Author

yt2b commented Dec 11, 2022

@DimitrisJim
Code fix completed.

@@ -300,7 +300,8 @@ impl PyList {

if let Some(index) = index.into() {
// defer delete out of borrow
Ok(self.borrow_vec_mut().remove(index))
let mut elements = self.borrow_vec_mut();
Ok((index < elements.len()).then(move || elements.remove(index)))
} else {
Err(vm.new_value_error(format!("'{}' is not in list", needle.str(vm)?)))
}
Copy link
Member

@youknowone youknowone Dec 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect fix. If size differs, it will silently skip remove operation - which is hard-to-recognize error.

I suggest to lock both index and remove operation using self.borrow_vec_mut() if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I implemented it this way because list.remove returns None in CPython.

>>> class rewrite_list_eq(list):
...     pass
... 
>>> 
>>> class poc:
...     def __eq__(self, other):
...         list1.clear()
...         return self
... 
>>> 
>>> list1 = rewrite_list_eq([poc()])
>>> print(list1.remove(list1))
None

But as you said, this is hard-to-recognize error.
Should I fix this so that list.remove returns ValueError?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bug is broken sync between index and remove. out of index is one of the visible example of it. e.g. when l = [0, 1, 2, 3] and tried l.remove(3), del l[0] can be done between index and remove. Then it is the case you described. When user ran l.remove(3), they expect 3 is gone. This is the reason I said hard-to-recognize.
But what if l.insert(0, 0) happened instead of del l[0]? Then it will remove 2 instead of 3. This is the original bug. Your fix is about only the case l.remove(3), but not applicable to other cases.
Instead of it, we'd better to synchronize index and remove.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then it will remove 2 instead of 3.

Unless I'm misunderstanding your comment, this is also how CPython handles it, a user doing weird things in __eq__ can't really expect sensible behavior:

>>> class BadEq:
...     def __eq__(self, other):
...             if isinstance(self, type(other)):
...                     l.insert(0, 0)
...                     return True
...             return NotImplemented
... 
>>> l = [10, 20, BadEq()]
>>> l.remove(BadEq())
>>> l
[0, 10, <__main__.BadEq object at 0x7f450686e070>]

As far as I can tell from list_ass_slice they check/force that the value for the index (ilow and ihigh) is in bounds for the list and then perform the operation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thank you. And sorry for wrong comment. I needed to check how cpython handle it first.

So is this version compatible to CPython @DimitrisJim ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It definitely gets us closer, we definitely shouldn't panic when the list changes size underneath our feet.

@yt2b
Copy link
Contributor Author

yt2b commented Dec 18, 2022

@DimitrisJim @youknowone
I changed to lock in both index and remove operation.
Could you please review again?

Copy link
Member

@DimitrisJim DimitrisJim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, forgot about this, looks good to me!

@DimitrisJim DimitrisJim merged commit cad2adc into RustPython:main Jan 4, 2023
@yt2b yt2b deleted the fix_list_remove branch January 4, 2023 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants