-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix PyList.remove
#4326
Conversation
could you add the code snippet supplied in the issue as a test case in |
also, this does actually fix one of the tests I mentioned in the issue, if you remove the skip decorator from |
@DimitrisJim |
@@ -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)?))) | |||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
@DimitrisJim @youknowone |
There was a problem hiding this 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!
#4315
I added comparison between index and list size in
PyList.remove
.