Merge pull request RustPython#4326 from yt2b/fix_list_remove

DimitrisJim · web-flow · commit cad2adc299b3 · 2023-01-04T13:28:59.000+02:00
Fix `PyList.remove`
diff --git a/Lib/test/test_list.py b/Lib/test/test_list.py
@@ -198,7 +198,6 @@ def test_preallocation(self):
         self.assertEqual(iter_size, sys.getsizeof(list([0] * 10)))
         self.assertEqual(iter_size, sys.getsizeof(list(range(10))))
 
-    @unittest.skip("TODO: RUSTPYTHON; hang")
     def test_count_index_remove_crashes(self):
         # bpo-38610: The count(), index(), and remove() methods were not
         # holding strong references to list elements while calling
diff --git a/extra_tests/snippets/builtin_list.py b/extra_tests/snippets/builtin_list.py
@@ -655,4 +655,17 @@ def __eq__(self, other):
 l = [1, 2, 3, m, 4]
 m.list = l
 l.remove(4) 
-assert_raises(ValueError, lambda: l.index(4)) # element 4 must not be in the list
+assert_raises(ValueError, lambda: l.index(4)) # element 4 must not be in the list
+
+# Test no panic occured when list elements was deleted in __eq__
+class rewrite_list_eq(list):
+    pass
+
+class poc:
+    def __eq__(self, other):
+        list1.clear()
+        return self
+
+list1 = rewrite_list_eq([poc()])
+list1.remove(list1)
+assert list1 == []
diff --git a/vm/src/builtins/list.rs b/vm/src/builtins/list.rs
@@ -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 is_inside_range = index < self.borrow_vec().len();
+            Ok(is_inside_range.then(|| self.borrow_vec_mut().remove(index)))
         } else {
             Err(vm.new_value_error(format!("'{}' is not in list", needle.str(vm)?)))
         }

Original file line number	Diff line number	Diff line change
`@@ -300,7 +300,8 @@ impl PyList {`
`300`	`300`
`301`	`301`	`if let Some(index) = index.into() {`
`302`	`302`	`// defer delete out of borrow`
`303`		`- Ok(self.borrow_vec_mut().remove(index))`
	`303`	`+ let is_inside_range = index < self.borrow_vec().len();`
	`304`	`+ Ok(is_inside_range.then(\|\| self.borrow_vec_mut().remove(index)))`
`304`	`305`	`} else {`
`305`	`306`	`Err(vm.new_value_error(format!("'{}' is not in list", needle.str(vm)?)))`
`306`	`307`	`}`