From e71a5fd8fad79c34c25695f03078a60f32eb117d Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Tue, 18 Aug 2026 07:09:40 +0900 Subject: [PATCH 01/27] Correct what the non-unix frame stack is for The comment said non-unix threading builds have no stop-the-world. CollectStopTheWorld is gated on `feature = "threading"` alone, and sys._current_frames stops the world on both paths; the field is the fallback a reader uses when there is no `top_iframe` to materialize from. Assisted-by: Claude --- crates/vm/src/vm/thread.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/vm/src/vm/thread.rs b/crates/vm/src/vm/thread.rs index 3f83d88fe70..4ba0d7ffada 100644 --- a/crates/vm/src/vm/thread.rs +++ b/crates/vm/src/vm/thread.rs @@ -53,7 +53,8 @@ pub struct ThreadSlot { pub top_iframe: AtomicUsize, /// Raw frame pointers, valid while the owning thread's call stack is active. /// Readers must hold the Mutex and convert to FrameObjectRef inside the lock. - /// Used on non-unix threading builds, which have no stop-the-world. + /// Stands in for `top_frame` where that field is not built, so a reader + /// that finds no `top_iframe` still has the frames to answer from. #[cfg(not(unix))] pub frames: parking_lot::Mutex>, pub exception: crate::PyAtomicRef>, From e981e03124596da839383613d44e83d9ef23219f Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Tue, 18 Aug 2026 07:09:41 +0900 Subject: [PATCH 02/27] Seek by the whole file position on Windows `SetFilePointer` answers with the low half of the new position and signals failure with INVALID_SET_FILE_POINTER, which is also that half of a position four gigabytes in; telling them apart takes the error code, which this did not read, so such a seek was reported as an error. Deciding seekability from it also called such a file unseekable. `SetFilePointerEx` returns the whole position and a success flag of its own, which also removes the transmute of the position into halves. Assisted-by: Claude --- crates/host_env/src/os.rs | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/crates/host_env/src/os.rs b/crates/host_env/src/os.rs index 7af8f586110..6c2521ca12a 100644 --- a/crates/host_env/src/os.rs +++ b/crates/host_env/src/os.rs @@ -25,9 +25,7 @@ use { std::{os::windows::io::AsRawHandle, path::Path}, windows_sys::Win32::{ Foundation::FILETIME, - Storage::FileSystem::{ - FILE_FLAG_BACKUP_SEMANTICS, INVALID_SET_FILE_POINTER, SetFilePointer, SetFileTime, - }, + Storage::FileSystem::{FILE_FLAG_BACKUP_SEMANTICS, SetFilePointerEx, SetFileTime}, System::SystemInformation::{GetSystemInfo, SYSTEM_INFO}, }, }; @@ -292,22 +290,24 @@ pub fn seek_fd( position: crt_fd::Offset, how: i32, ) -> io::Result { + use crate::windows::CheckWin32Bool; + let handle = crt_fd::as_handle(fd)?; - let mut distance_to_move: [i32; 2] = unsafe { core::mem::transmute(position) }; - let ret = unsafe { - SetFilePointer( + // `SetFilePointer` returns the low half of the new position and reports + // failure with the value a position four gigabytes in also has, so the two + // are only told apart through the error code. The `Ex` form answers with + // the whole position and a success flag of its own. + let mut new_position = 0; + unsafe { + SetFilePointerEx( handle.as_raw_handle(), - distance_to_move[0], - &mut distance_to_move[1], + position, + &mut new_position, how as _, ) - }; - if ret == INVALID_SET_FILE_POINTER { - Err(io::Error::last_os_error()) - } else { - distance_to_move[0] = ret as _; - Ok(unsafe { core::mem::transmute::<[i32; 2], i64>(distance_to_move) }) } + .check_win32_bool()?; + Ok(new_position) } #[cfg(any(unix, target_os = "wasi"))] From 9b4037334b1e107b891a9872d768304f9989ac66 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Tue, 18 Aug 2026 07:21:35 +0900 Subject: [PATCH 03/27] memoryview: tell a value of the wrong kind from one that does not fit Assigning an item reported every packing failure as a TypeError, so m[0] = 300 on a 'B' view said the value was the wrong type rather than out of range. Packing now says which of the two it was, and whether the value's own code raised, in which case that error is the answer as it is: m[0] = 300 ValueError: invalid value for format 'B' m[0] = "x" TypeError: invalid type for format 'B' m[0] = <__index__ that raises> the raised error `struct` reports both as `struct.error` and is unchanged; the kind travels beside the exception for the caller that tells them apart. Also: None was read as a deletion, so m[0] = None answered "cannot delete memory" instead of packing it; and deleting through the mapping protocol never reached the read-only check, which comes first. Assisted-by: Claude --- crates/vm/src/buffer.rs | 161 ++++++++++++++++----- crates/vm/src/builtins/memory.rs | 30 ++-- extra_tests/snippets/builtin_memoryview.py | 89 ++++++++++++ 3 files changed, 236 insertions(+), 44 deletions(-) diff --git a/crates/vm/src/buffer.rs b/crates/vm/src/buffer.rs index 038e7cae9f3..2dbc437a69c 100644 --- a/crates/vm/src/buffer.rs +++ b/crates/vm/src/buffer.rs @@ -1,5 +1,5 @@ use crate::{ - PyObjectRef, PyResult, TryFromObject, VirtualMachine, + AsObject, PyObjectRef, PyResult, TryFromObject, VirtualMachine, builtins::{PyBaseExceptionRef, PyBytesRef, PyTuple, PyTupleRef, PyTypeRef}, common::{static_cell, str::wchar_t}, convert::ToPyObject, @@ -16,9 +16,53 @@ use malachite_bigint::BigInt; use num_traits::{PrimInt, ToPrimitive}; use std::os::raw; -type PackFunc = fn(&VirtualMachine, FormatType, PyObjectRef, &mut [u8]) -> PyResult<()>; +type PackFunc = fn(&VirtualMachine, FormatType, PyObjectRef, &mut [u8]) -> Result<(), PackError>; type UnpackFunc = fn(&VirtualMachine, &[u8]) -> PyObjectRef; +/// Why a value could not be packed. +/// +/// `struct` reports both as `struct.error`, so the kind travels beside the +/// exception rather than in it; `memoryview`, which reports them as TypeError +/// and ValueError, is what needs to tell them apart. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum PackErrorKind { + /// The value was not the kind of thing the format takes. + Type, + /// The value was the right kind, and the format has no room for it. + Value, + /// The value's own code raised, and that error is the answer as it is. + Raised, +} + +pub struct PackError { + pub kind: PackErrorKind, + pub exception: PyBaseExceptionRef, +} + +impl PackError { + fn new>(kind: PackErrorKind, vm: &VirtualMachine, msg: T) -> Self { + Self { + kind, + exception: new_struct_error(vm, msg), + } + } + + /// An error raised by something other than the packing itself, such as a + /// conversion running the value's own code. + fn from_exception(exception: PyBaseExceptionRef, vm: &VirtualMachine) -> Self { + let kind = if exception.fast_isinstance(vm.ctx.exceptions.type_error) { + PackErrorKind::Type + } else if exception.fast_isinstance(vm.ctx.exceptions.overflow_error) + || exception.fast_isinstance(vm.ctx.exceptions.value_error) + { + PackErrorKind::Value + } else { + PackErrorKind::Raised + }; + Self { kind, exception } + } +} + static OVERFLOW_MSG: &str = "total struct size too long"; // not a const to reduce code size #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -438,22 +482,43 @@ impl FormatSpec { } pub fn pack(&self, args: Vec, vm: &VirtualMachine) -> PyResult> { + self.try_pack(args, vm).map_err(|e| e.exception) + } + + /// [`Self::pack`], keeping why a value could not be packed. + pub fn try_pack( + &self, + args: Vec, + vm: &VirtualMachine, + ) -> Result, PackError> { // Create data vector: let mut data = vec![0; self.size]; - self.pack_into(&mut data, args, vm)?; + self.try_pack_into(&mut data, args, vm)?; Ok(data) } pub fn pack_into( &self, - mut buffer: &mut [u8], + buffer: &mut [u8], args: Vec, vm: &VirtualMachine, ) -> PyResult<()> { + self.try_pack_into(buffer, args, vm) + .map_err(|e| e.exception) + } + + /// [`Self::pack_into`], keeping why a value could not be packed. + pub fn try_pack_into( + &self, + mut buffer: &mut [u8], + args: Vec, + vm: &VirtualMachine, + ) -> Result<(), PackError> { if self.arg_count != args.len() { - return Err(new_struct_error( + return Err(PackError::new( + PackErrorKind::Type, vm, format!( "pack expected {} items for packing (got {})", @@ -471,12 +536,14 @@ impl FormatSpec { match code.code { FormatType::Str => { let (buf, rest) = buffer.split_at_mut(code.repeat); - pack_string(vm, args.next().unwrap(), buf)?; + pack_string(vm, args.next().unwrap(), buf) + .map_err(|e| PackError::from_exception(e, vm))?; buffer = rest; } FormatType::Pascal => { let (buf, rest) = buffer.split_at_mut(code.repeat); - pack_pascal(vm, args.next().unwrap(), buf)?; + pack_pascal(vm, args.next().unwrap(), buf) + .map_err(|e| PackError::from_exception(e, vm))?; buffer = rest; } FormatType::Pad => { @@ -554,7 +621,7 @@ trait Packable { code: FormatType, arg: PyObjectRef, data: &mut [u8], - ) -> PyResult<()>; + ) -> Result<(), PackError>; fn unpack(vm: &VirtualMachine, data: &[u8]) -> PyObjectRef; } @@ -584,7 +651,7 @@ macro_rules! make_pack_prim_int { code: FormatType, arg: PyObjectRef, data: &mut [u8], - ) -> PyResult<()> { + ) -> Result<(), PackError> { let i: $T = get_int_or_index(vm, code, arg)?; i.pack_int::(data); Ok(()) @@ -598,13 +665,25 @@ macro_rules! make_pack_prim_int { }; } -fn get_int_or_index(vm: &VirtualMachine, code: FormatType, arg: PyObjectRef) -> PyResult +fn get_int_or_index( + vm: &VirtualMachine, + code: FormatType, + arg: PyObjectRef, +) -> Result where T: PrimInt + fmt::Display + for<'a> TryFrom<&'a BigInt>, { - let index = arg - .try_index_opt(vm) - .unwrap_or_else(|| Err(new_struct_error(vm, "required argument is not an integer")))?; + let index = match arg.try_index_opt(vm) { + None => { + return Err(PackError::new( + PackErrorKind::Type, + vm, + "required argument is not an integer", + )); + } + Some(Err(e)) => return Err(PackError::from_exception(e, vm)), + Some(Ok(index)) => index, + }; index.try_to_primitive(vm).map_err(|_| { // A pointer is converted rather than checked against the range of a // named format, so what it reports is the conversion failing. @@ -618,7 +697,7 @@ where T::max_value() ) }; - new_struct_error(vm, msg) + PackError::new(PackErrorKind::Value, vm, msg) }) } @@ -641,15 +720,20 @@ macro_rules! make_pack_float { _code: FormatType, arg: PyObjectRef, data: &mut [u8], - ) -> PyResult<()> { - let f_64 = ArgIntoFloat::try_from_object(vm, arg)?.into_float(); + ) -> Result<(), PackError> { + let f_64 = ArgIntoFloat::try_from_object(vm, arg) + .map_err(|e| PackError::from_exception(e, vm))? + .into_float(); let f = f_64 as $T; if f.is_infinite() != f_64.is_infinite() { - return Err(vm.new_overflow_error(concat!( - "float too large to pack with ", - $fmt, - " format" - ))); + return Err(PackError { + kind: PackErrorKind::Value, + exception: vm.new_overflow_error(concat!( + "float too large to pack with ", + $fmt, + " format" + )), + }); } f.to_bits().pack_int::(data); Ok(()) @@ -672,12 +756,17 @@ impl Packable for f16 { _code: FormatType, arg: PyObjectRef, data: &mut [u8], - ) -> PyResult<()> { - let f_64 = ArgIntoFloat::try_from_object(vm, arg)?.into_float(); + ) -> Result<(), PackError> { + let f_64 = ArgIntoFloat::try_from_object(vm, arg) + .map_err(|e| PackError::from_exception(e, vm))? + .into_float(); // "from_f64 should be preferred in any non-`const` context" except it gives the wrong result :/ let f_16 = Self::from_f64_const(f_64); if f_16.is_infinite() != f_64.is_infinite() { - return Err(vm.new_overflow_error("float too large to pack with e format")); + return Err(PackError { + kind: PackErrorKind::Value, + exception: vm.new_overflow_error("float too large to pack with e format"), + }); } f_16.to_bits().pack_int::(data); Ok(()) @@ -695,7 +784,7 @@ impl Packable for *mut raw::c_void { code: FormatType, arg: PyObjectRef, data: &mut [u8], - ) -> PyResult<()> { + ) -> Result<(), PackError> { usize::pack::(vm, code, arg, data) } @@ -710,8 +799,10 @@ impl Packable for bool { _code: FormatType, arg: PyObjectRef, data: &mut [u8], - ) -> PyResult<()> { - let v = ArgIntoBool::try_from_object(vm, arg)?.into_bool() as u8; + ) -> Result<(), PackError> { + let v = ArgIntoBool::try_from_object(vm, arg) + .map_err(|e| PackError::from_exception(e, vm))? + .into_bool() as u8; v.pack_int::(data); Ok(()) } @@ -727,13 +818,15 @@ fn pack_char( _code: FormatType, arg: PyObjectRef, data: &mut [u8], -) -> PyResult<()> { - let v = PyBytesRef::try_from_object(vm, arg)?; - let ch = *v - .as_bytes() - .iter() - .exactly_one() - .map_err(|_| new_struct_error(vm, "char format requires a bytes object of length 1"))?; +) -> Result<(), PackError> { + let v = PyBytesRef::try_from_object(vm, arg).map_err(|e| PackError::from_exception(e, vm))?; + let ch = *v.as_bytes().iter().exactly_one().map_err(|_| { + PackError::new( + PackErrorKind::Value, + vm, + "char format requires a bytes object of length 1", + ) + })?; data[0] = ch; Ok(()) } diff --git a/crates/vm/src/builtins/memory.rs b/crates/vm/src/builtins/memory.rs index 0b04de133e7..30ddd28222b 100644 --- a/crates/vm/src/builtins/memory.rs +++ b/crates/vm/src/builtins/memory.rs @@ -6,7 +6,7 @@ use crate::common::lock::LazyLock; use crate::{ AsObject, Context, Py, PyObject, PyObjectRef, PyPayload, PyRef, PyResult, TryFromBorrowedObject, TryFromObject, VirtualMachine, atomic_func, - buffer::FormatSpec, + buffer::{FormatSpec, PackErrorKind}, bytes_inner::{ByteInnerHexOptions, bytes_to_hex}, class::{PyClassImpl, StaticType}, common::{ @@ -329,11 +329,22 @@ impl PyMemoryView { // conversion runs `__index__` or `__float__`, which can read or write the // same buffer. // TODO: Optimize - let data = self.format_spec.pack(vec![value], vm).map_err(|_| { - vm.new_type_error(format!( - "memoryview: invalid type for format '{}'", + // A value of the wrong kind and a value the format has no room for are + // different errors here, though packing reports both the same way. + let data = self.format_spec.try_pack(vec![value], vm).map_err(|err| { + let what = match err.kind { + PackErrorKind::Type => "type", + PackErrorKind::Value => "value", + PackErrorKind::Raised => return err.exception, + }; + let msg = format!( + "memoryview: invalid {what} for format '{}'", self.desc.format - )) + ); + match err.kind { + PackErrorKind::Type => vm.new_type_error(msg), + _ => vm.new_value_error(msg), + } })?; // The conversion, and the index that produced `pos`, could have released // the view; `pos` addresses a buffer that is no longer there. @@ -842,6 +853,9 @@ impl PyMemoryView { } fn __delitem__(&self, _needle: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> { + self.try_not_released(vm)?; + // What cannot be written cannot be deleted from either, and that is + // the first thing answered. if self.desc.readonly { return Err(vm.new_type_error("cannot modify read-only memory")); } @@ -1133,10 +1147,6 @@ impl Py { if self.desc.readonly { return Err(vm.new_type_error("cannot modify read-only memory")); } - if value.is(&vm.ctx.none) { - return Err(vm.new_type_error("cannot delete memory")); - } - if self.desc.ndim() == 0 { // TODO: merge branches when we got conditional if let if needle.is(&vm.ctx.ellipsis) { @@ -1291,7 +1301,7 @@ impl AsMapping for PyMemoryView { if let Some(value) = value { zelf.__setitem__(needle.to_owned(), value, vm) } else { - Err(vm.new_type_error("cannot delete memory".to_owned())) + zelf.__delitem__(needle.to_owned(), vm) } }), }; diff --git a/extra_tests/snippets/builtin_memoryview.py b/extra_tests/snippets/builtin_memoryview.py index 979f584a2b1..a57f9c38ae5 100644 --- a/extra_tests/snippets/builtin_memoryview.py +++ b/extra_tests/snippets/builtin_memoryview.py @@ -850,3 +850,92 @@ def test_cast_bounds_the_dimensions(): test_cast_bounds_the_dimensions() + + +def test_setitem_error_kinds(): + import array + + # A value the format has no room for and a value of the wrong kind are + # different errors, the way they are for any other conversion. + for fmt, over, under in ( + ("B", 300, -1), + ("b", 128, -129), + ("i", 2**31, -(2**31) - 1), + ): + view = memoryview(array.array(fmt, [0, 0])) + for value in (over, under): + try: + view[0] = value + except ValueError as e: + assert str(e) == f"memoryview: invalid value for format '{fmt}'", e + else: + raise AssertionError(f"expected ValueError for {fmt!r} {value}") + for value in ("x", 1.5, None, [1]): + try: + view[0] = value + except TypeError as e: + assert str(e) == f"memoryview: invalid type for format '{fmt}'", e + else: + raise AssertionError(f"expected TypeError for {fmt!r} {value!r}") + + # A bytes item is a value error when it is the wrong length. + chars = memoryview(bytearray(b"ab")).cast("c") + for value in (b"", b"xy"): + try: + chars[0] = value + except ValueError as e: + assert str(e) == "memoryview: invalid value for format 'c'", e + else: + raise AssertionError(f"expected ValueError for {value!r}") + + +def test_setitem_propagates_index_errors(): + # An error raised by the value's own code is the answer, not a report that + # the value was the wrong kind. + class Boom: + def __index__(self): + raise ZeroDivisionError("boom") + + class NotAnInt: + def __index__(self): + return "not an int" + + view = memoryview(bytearray(b"ab")) + try: + view[0] = Boom() + except ZeroDivisionError as e: + assert str(e) == "boom", e + else: + raise AssertionError("expected ZeroDivisionError") + + try: + view[0] = NotAnInt() + except TypeError as e: + assert str(e) == "memoryview: invalid type for format 'B'", e + else: + raise AssertionError("expected TypeError") + + +def test_delete_answers_readonly_first(): + # Nothing can be deleted from a memoryview, but what cannot be written + # says so first. + try: + del memoryview(b"ab")[0] + except TypeError as e: + assert str(e) == "cannot modify read-only memory", e + else: + raise AssertionError("expected TypeError") + + view = memoryview(bytearray(b"abcd")) + for needle in (0, slice(0, 2)): + try: + del view[needle] + except TypeError as e: + assert str(e) == "cannot delete memory", e + else: + raise AssertionError(f"expected TypeError for {needle!r}") + + +test_setitem_error_kinds() +test_setitem_propagates_index_errors() +test_delete_answers_readonly_first() From 562dd8640060611a55f85e42d10118b715b25e50 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Tue, 18 Aug 2026 07:55:14 +0900 Subject: [PATCH 04/27] Raise MemoryError for allocations sized by Python input os.read, _RawIOBase.read, int.to_bytes, struct.pack, ctypes array creation and the _ssl RAND functions sized a Vec from a Python-supplied length with vec![], which calls handle_alloc_error and, under panic = "abort", ends the process. They now allocate through vm.new_zeroed_bytes and raise MemoryError. itertools.product built its pools without checking that len(iterables) * repeat is representable; it now raises OverflowError "repeat argument too large" and reserves the pool and index vectors fallibly. repeat is read as isize, so a negative one raises ValueError "repeat argument cannot be negative" instead of the conversion's message. _ssl.RAND_bytes and RAND_pseudo_bytes read n as i32, matching the int they are declared with. Assisted-by: Claude --- .cspell.dict/cpython.txt | 1 + crates/stdlib/src/openssl.rs | 4 +-- crates/stdlib/src/ssl.rs | 9 +++---- crates/vm/src/buffer.rs | 4 ++- crates/vm/src/builtins/int.rs | 10 +++---- crates/vm/src/stdlib/_ctypes/array.rs | 2 +- crates/vm/src/stdlib/_io.rs | 3 +-- crates/vm/src/stdlib/itertools.rs | 33 +++++++++++++++++++----- crates/vm/src/stdlib/os.rs | 2 +- extra_tests/snippets/builtin_int.py | 6 +++++ extra_tests/snippets/stdlib_ctypes.py | 10 +++++++ extra_tests/snippets/stdlib_itertools.py | 7 +++++ extra_tests/snippets/stdlib_struct.py | 6 +++++ 13 files changed, 73 insertions(+), 24 deletions(-) diff --git a/.cspell.dict/cpython.txt b/.cspell.dict/cpython.txt index da85c312898..c6a60a3a7f0 100644 --- a/.cspell.dict/cpython.txt +++ b/.cspell.dict/cpython.txt @@ -165,6 +165,7 @@ Nondescriptor noninteger nops noraise +npools nseen NSIGNALS numer diff --git a/crates/stdlib/src/openssl.rs b/crates/stdlib/src/openssl.rs index fe4a5298d12..ee9d9ae84e0 100644 --- a/crates/stdlib/src/openssl.rs +++ b/crates/stdlib/src/openssl.rs @@ -525,7 +525,7 @@ mod _ssl { if n < 0 { return Err(vm.new_value_error("num must be positive")); } - let mut buf = vec![0; n as usize]; + let mut buf = vm.new_zeroed_bytes(n as usize)?; openssl::rand::rand_bytes(&mut buf).map_err(|e| convert_openssl_error(vm, e))?; Ok(buf) } @@ -872,7 +872,7 @@ mod _ssl { if n < 0 { return Err(vm.new_value_error("num must be positive")); } - let mut buf = vec![0; n as usize]; + let mut buf = vm.new_zeroed_bytes(n as usize)?; let ret = unsafe { sys::RAND_bytes(buf.as_mut_ptr(), n) }; match ret { 0 | 1 => Ok((buf, ret == 1)), diff --git a/crates/stdlib/src/ssl.rs b/crates/stdlib/src/ssl.rs index b942e27fc69..262c3936e0a 100644 --- a/crates/stdlib/src/ssl.rs +++ b/crates/stdlib/src/ssl.rs @@ -3771,7 +3771,7 @@ mod _ssl { // Use compat layer for unified read logic with proper EOF handling // This matches SSL_read_ex() approach - let mut buf = vec![0u8; len]; + let mut buf = vm.new_zeroed_bytes(len)?; let read_result = { let mut conn_guard = self.connection.lock(); let conn = conn_guard @@ -5030,14 +5030,13 @@ mod _ssl { } #[pyfunction] - fn RAND_bytes(n: i64, vm: &VirtualMachine) -> PyResult { + fn RAND_bytes(n: i32, vm: &VirtualMachine) -> PyResult { // Validate n is not negative if n < 0 { return Err(vm.new_value_error("num must be positive")); } - let n_usize = n as usize; - let mut buf = vec![0u8; n_usize]; + let mut buf = vm.new_zeroed_bytes(n as usize)?; CryptoExt::get_provider() .secure_random .fill(&mut buf) @@ -5046,7 +5045,7 @@ mod _ssl { } #[pyfunction] - fn RAND_pseudo_bytes(n: i64, vm: &VirtualMachine) -> PyResult<(PyBytesRef, bool)> { + fn RAND_pseudo_bytes(n: i32, vm: &VirtualMachine) -> PyResult<(PyBytesRef, bool)> { // Rustls providers expose cryptographically strong random bytes. let bytes = RAND_bytes(n, vm)?; Ok((bytes, true)) diff --git a/crates/vm/src/buffer.rs b/crates/vm/src/buffer.rs index 2dbc437a69c..b3b33aa24fe 100644 --- a/crates/vm/src/buffer.rs +++ b/crates/vm/src/buffer.rs @@ -492,7 +492,9 @@ impl FormatSpec { vm: &VirtualMachine, ) -> Result, PackError> { // Create data vector: - let mut data = vec![0; self.size]; + let mut data = vm + .new_zeroed_bytes(self.size) + .map_err(|e| PackError::from_exception(e, vm))?; self.try_pack_into(&mut data, args, vm)?; diff --git a/crates/vm/src/builtins/int.rs b/crates/vm/src/builtins/int.rs index bb7b5128073..3a4c18a3cdd 100644 --- a/crates/vm/src/builtins/int.rs +++ b/crates/vm/src/builtins/int.rs @@ -593,7 +593,7 @@ impl PyInt { Sign::Minus if !signed => { return Err(vm.new_overflow_error("can't convert negative int to unsigned")); } - Sign::NoSign => return Ok(vec![0u8; byte_len].into()), + Sign::NoSign => return Ok(vm.new_zeroed_bytes(byte_len)?.into()), _ => {} } @@ -609,10 +609,10 @@ impl PyInt { return Err(vm.new_overflow_error("int too big to convert")); } - let mut append_bytes = match value.sign() { - Sign::Minus => vec![255u8; byte_len - origin_len], - _ => vec![0u8; byte_len - origin_len], - }; + let mut append_bytes = vm.new_zeroed_bytes(byte_len - origin_len)?; + if value.sign() == Sign::Minus { + append_bytes.fill(255); + } let bytes = match args.byteorder { ArgByteOrder::Big => { diff --git a/crates/vm/src/stdlib/_ctypes/array.rs b/crates/vm/src/stdlib/_ctypes/array.rs index d4674f33b07..965f7257e60 100644 --- a/crates/vm/src/stdlib/_ctypes/array.rs +++ b/crates/vm/src/stdlib/_ctypes/array.rs @@ -429,7 +429,7 @@ impl Constructor for PyCArray { } // Create array with zero-initialized buffer - let buffer = vec![0u8; total_size]; + let buffer = vm.new_zeroed_bytes(total_size)?; let instance = Self(PyCData::from_bytes_with_length(buffer, None, length)) .into_ref_with_type(vm, cls)?; diff --git a/crates/vm/src/stdlib/_io.rs b/crates/vm/src/stdlib/_io.rs index 5479c47abc4..4db5fb760c1 100644 --- a/crates/vm/src/stdlib/_io.rs +++ b/crates/vm/src/stdlib/_io.rs @@ -647,8 +647,7 @@ mod _io { #[pymethod] fn read(instance: PyObjectRef, size: OptionalSize, vm: &VirtualMachine) -> PyResult { if let Some(size) = size.to_usize() { - // FIXME: unnecessary zero-init - let b = PyByteArray::from(vec![0; size]).into_ref(&vm.ctx); + let b = PyByteArray::from(vm.new_zeroed_bytes(size)?).into_ref(&vm.ctx); let n = >::try_from_object( vm, vm.call_method(&instance, "readinto", (b.clone(),))?, diff --git a/crates/vm/src/stdlib/itertools.rs b/crates/vm/src/stdlib/itertools.rs index 6eb268d94c1..bc6ba3546a1 100644 --- a/crates/vm/src/stdlib/itertools.rs +++ b/crates/vm/src/stdlib/itertools.rs @@ -1123,7 +1123,7 @@ mod decl { #[derive(FromArgs)] struct ProductArgs { #[pyarg(named, optional)] - repeat: OptionalArg, + repeat: OptionalArg, } impl Constructor for PyItertoolsProduct { @@ -1135,19 +1135,38 @@ mod decl { vm: &VirtualMachine, ) -> PyResult { let repeat = args.repeat.unwrap_or(1); - let mut pools = Vec::new(); + if repeat < 0 { + return Err(vm.new_value_error("repeat argument cannot be negative")); + } + let repeat = repeat as usize; + + let mut single: Vec> = Vec::new(); for arg in iterables.iter() { - pools.push(arg.try_to_value(vm)?); + single.push(arg.try_to_value(vm)?); } - let pools = core::iter::repeat_n(pools, repeat) - .flatten() - .collect::>>(); + + let npools = single + .len() + .checked_mul(repeat) + .filter(|n| *n <= isize::MAX as usize / size_of::()) + .ok_or_else(|| vm.new_overflow_error("repeat argument too large"))?; + + let mut pools: Vec> = Vec::new(); + pools + .try_reserve_exact(npools) + .map_err(|_| vm.new_memory_error(""))?; + pools.extend(core::iter::repeat_n(single, repeat).flatten()); + + let mut idxs = Vec::new(); + idxs.try_reserve_exact(npools) + .map_err(|_| vm.new_memory_error(""))?; + idxs.resize(npools, 0); let l = pools.len(); Ok(Self { pools, - idxs: PyRwLock::new(vec![0; l]), + idxs: PyRwLock::new(idxs), cur: AtomicCell::new(l.wrapping_sub(1)), stop: AtomicCell::new(false), }) diff --git a/crates/vm/src/stdlib/os.rs b/crates/vm/src/stdlib/os.rs index 668545a3cec..ae39b9c31bc 100644 --- a/crates/vm/src/stdlib/os.rs +++ b/crates/vm/src/stdlib/os.rs @@ -328,7 +328,7 @@ pub(super) mod _os { #[pyfunction] fn read(fd: crt_fd::Borrowed<'_>, n: usize, vm: &VirtualMachine) -> PyResult { - let mut buffer = vec![0u8; n]; + let mut buffer = vm.new_zeroed_bytes(n)?; loop { match vm.allow_threads(|| crt_fd::read(fd, &mut buffer)) { Ok(n) => { diff --git a/extra_tests/snippets/builtin_int.py b/extra_tests/snippets/builtin_int.py index 2828b5ad26d..c111d253254 100644 --- a/extra_tests/snippets/builtin_int.py +++ b/extra_tests/snippets/builtin_int.py @@ -401,3 +401,9 @@ class SubInt(int): assert str(huge) finally: sys.set_int_max_str_digits(_orig_limit) + +# to_bytes is handed the length to allocate, so one that cannot be satisfied +# must raise. Zero and non-zero take different paths to the same buffer. +for value in (0, 1, -1): + with assert_raises(MemoryError): + value.to_bytes(2**60, "big", signed=True) diff --git a/extra_tests/snippets/stdlib_ctypes.py b/extra_tests/snippets/stdlib_ctypes.py index 109665b6a03..ea3538c348c 100644 --- a/extra_tests/snippets/stdlib_ctypes.py +++ b/extra_tests/snippets/stdlib_ctypes.py @@ -448,4 +448,14 @@ def get_win_folder_via_ctypes(csidl_name: str) -> str: array3[0:3] = [7, 8, 9] assert list(array3) == [7, 8, 9] + +# An array type carries the size of its buffer, so one too large to allocate +# must raise instead of aborting. +try: + (ctypes.c_char * (2**60))() +except MemoryError: + pass +else: + assert False, "an unallocatable array was created" + print("done") diff --git a/extra_tests/snippets/stdlib_itertools.py b/extra_tests/snippets/stdlib_itertools.py index 029d0d4229a..b5ee8645929 100644 --- a/extra_tests/snippets/stdlib_itertools.py +++ b/extra_tests/snippets/stdlib_itertools.py @@ -540,3 +540,10 @@ def __iter__(self): itertools.combinations(range(5), 2**44) with assert_raises(MemoryError): itertools.combinations_with_replacement(range(5), 2**44) + +# repeat is an arbitrary Python int: a negative one is refused, and one whose +# pool cannot be allocated must raise rather than take the process down. +with assert_raises(ValueError): + itertools.product([1], repeat=-1) +with assert_raises(OverflowError): + itertools.product([1, 2], repeat=2**60) diff --git a/extra_tests/snippets/stdlib_struct.py b/extra_tests/snippets/stdlib_struct.py index b95b6560d68..21305948269 100644 --- a/extra_tests/snippets/stdlib_struct.py +++ b/extra_tests/snippets/stdlib_struct.py @@ -156,3 +156,9 @@ def __init__(self): ): with assert_raises(RuntimeError): call() + + +# The buffer a format asks for is sized by the format: one too large to +# allocate must raise instead of aborting. +with assert_raises(MemoryError): + struct.pack("%dx" % (2**60)) From 6b9b80df36d78b81f0dd47508de843cf266c7dc8 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Tue, 18 Aug 2026 09:43:16 +0900 Subject: [PATCH 05/27] Collect with the count in the object header A collection kept the count it was working with in a table keyed by the object's address, and the objects it had proved reachable in a second one. Between them they were hashed once per candidate and twice per edge in the heap, which is where most of a collection over a live heap went. The count now lives in `PyInner::gc_refs`, with `GcBits::COLLECTING` saying it is meaningful, and reachability is `gc_refs == GC_REACHABLE` rather than membership in a set. Step 5 splits the candidates and clears the bit in one pass. gcbench, five interleaved pairs, median: a live heap of 423k objects goes from 0.101s to 0.055s and a dead one from 0.402s to 0.383s. The bits, generation, owner and count take eight bytes between them. A 64-bit header had those eight as the padding its alignment forces, so it is unchanged at 48 bytes; a 32-bit header grows from 24 to 28. Assisted-by: Claude --- crates/vm/src/gc_state.rs | 79 +++++++++++++++++++----------------- crates/vm/src/object/core.rs | 76 ++++++++++++++++++++++++++++++++-- crates/vm/src/object/mod.rs | 2 +- 3 files changed, 116 insertions(+), 41 deletions(-) diff --git a/crates/vm/src/gc_state.rs b/crates/vm/src/gc_state.rs index e5eb3758950..6cfeebe97df 100644 --- a/crates/vm/src/gc_state.rs +++ b/crates/vm/src/gc_state.rs @@ -4,7 +4,7 @@ use crate::common::linked_list::LinkedList; use crate::common::lock::{PyMutex, PyRwLock}; -use crate::object::{GC_NO_OWNER, GC_PERMANENT, GC_UNTRACKED, GcLink, GcOwner}; +use crate::object::{GC_NO_OWNER, GC_PERMANENT, GC_REACHABLE, GC_UNTRACKED, GcLink, GcOwner}; use crate::{AsObject, PyObject, PyObjectRef}; use core::ptr::NonNull; use core::sync::atomic::{AtomicBool, AtomicU16, AtomicU32, AtomicUsize, Ordering}; @@ -160,11 +160,11 @@ struct GcPtr(NonNull); /// choosing keys that collide. Nothing chooses these keys: they are addresses /// this process handed out, and the tables live and die inside one collection. /// What a collection needs from them is speed -- it hashes every tracked -/// object and every edge between them -- so this runs the address through a -/// handful of multiplies and shifts instead. The shifts are what earns the -/// speed: a table picks its bucket from the low bits, and an address arrives -/// with its low bits zeroed by alignment, so entropy has to be carried -/// downward or every object lands in the same few buckets. +/// object -- so this runs the address through a handful of multiplies and +/// shifts instead. The shifts are what earns the speed: a table picks its +/// bucket from the low bits, and an address arrives with its low bits zeroed +/// by alignment, so entropy has to be carried downward or every object lands +/// in the same few buckets. #[derive(Default)] struct GcPtrHasher(u64); @@ -594,12 +594,12 @@ impl GcState { retired.sort_unstable(); retired }; - // The candidates and their reference counts go in one table, not a set - // beside a map: every edge in the heap is looked up here, and the two - // held the same keys, so a second table only bought a second hash of - // the same address. `candidate_ptrs` keeps them in a walkable order, - // since the counts are written while the candidates are read. - let mut gc_refs: GcMap = GcMap::default(); + // Each candidate carries its own count, with `GcBits::COLLECTING` + // saying the count is there. Every edge in the heap is answered from + // that bit and that field; a table keyed by address turned each of + // those answers into a hash of the address instead. `candidate_ptrs` + // keeps the candidates in a walkable order, and the bit is what keeps + // an object that appears in two generation lists out of it twice. let mut candidate_ptrs: Vec = Vec::new(); for gen_list in &gen_locks { for obj in gen_list.iter() { @@ -607,12 +607,9 @@ impl GcState { obj.set_gc_owner(GC_NO_OWNER); } let strong_count = obj.strong_count(); - let ptr = GcPtr(NonNull::from(obj)); - if strong_count > 0 - && is_owned_by(obj, owner) - && gc_refs.insert(ptr, strong_count).is_none() - { - candidate_ptrs.push(ptr); + if strong_count > 0 && is_owned_by(obj, owner) && !obj.is_gc_collecting() { + obj.start_gc_refs(strong_count); + candidate_ptrs.push(GcPtr(NonNull::from(obj))); } } } @@ -679,24 +676,23 @@ impl GcState { unsafe { obj.gc_extend_referent_ptrs(&mut referent_ptrs) }; let end = referent_ptrs.len(); for &child_ptr in &referent_ptrs[start..end] { - if let Some(refs) = gc_refs.get_mut(&GcPtr(child_ptr)) { - *refs = refs.saturating_sub(1); + // SAFETY: the referents came from `traverse`, which handed out + // live references to them, and the world is stopped. + let child = unsafe { child_ptr.as_ref() }; + if child.is_gc_collecting() { + child.subtract_gc_ref(); } } referent_ranges.insert(ptr, (start, end)); } // Step 4: Find reachable objects (gc_refs > 0) and traverse from them - let mut reachable: GcSet = GcSet::default(); let mut worklist: Vec = Vec::new(); - #[expect( - clippy::iter_over_hash_type, - reason = "Iteration order doesn't matter here" - )] - for (&ptr, &refs) in &gc_refs { - if refs > 0 { - reachable.insert(ptr); + for &ptr in &candidate_ptrs { + let obj = unsafe { ptr.0.as_ref() }; + if obj.gc_refs() > 0 { + obj.mark_gc_reachable(); worklist.push(ptr); } } @@ -717,20 +713,29 @@ impl GcState { } }; for &child_ptr in children { - let gc_ptr = GcPtr(child_ptr); - if gc_refs.contains_key(&gc_ptr) && reachable.insert(gc_ptr) { - worklist.push(gc_ptr); + // SAFETY: as in step 3, the referents are live. + let child = unsafe { child_ptr.as_ref() }; + if child.is_gc_collecting() && child.mark_gc_reachable() { + worklist.push(GcPtr(child_ptr)); } } } } - // Step 5: Find unreachable objects - let unreachable: Vec = candidate_ptrs - .iter() - .filter(|ptr| !reachable.contains(ptr)) - .copied() - .collect(); + // Step 5: Split the candidates on what step 4 concluded, and hand the + // headers back: nothing past here reads `gc_refs`, and a candidate that + // kept the bit would be passed over by every later collection. + let mut reachable: Vec = Vec::new(); + let mut unreachable: Vec = Vec::new(); + for &ptr in &candidate_ptrs { + let obj = unsafe { ptr.0.as_ref() }; + if obj.gc_refs() == GC_REACHABLE { + reachable.push(ptr); + } else { + unreachable.push(ptr); + } + obj.end_gc_refs(); + } // With the world stopped, every frame on any thread's call stack is a // live root that is externally referenced and must have been diff --git a/crates/vm/src/object/core.rs b/crates/vm/src/object/core.rs index bdacb7c5b83..79d6b6ba945 100644 --- a/crates/vm/src/object/core.rs +++ b/crates/vm/src/object/core.rs @@ -297,6 +297,9 @@ bitflags::bitflags! { const SHARED_INLINE = 1 << 5; /// Use deferred reference counting const DEFERRED = 1 << 6; + /// In the candidate set of the collection that is running, so its + /// `gc_refs` is meaningful. `_PyGC_PREV_MASK_COLLECTING`. + const COLLECTING = 1 << 7; } } @@ -316,6 +319,11 @@ pub(crate) type GcOwner = u16; /// current. Every interpreter collects these. pub(crate) const GC_NO_OWNER: GcOwner = 0; +/// `gc_refs` of an object a running collection has proved reachable. One past +/// the largest count [`PyObject::start_gc_refs`] stores, so no real count can +/// be taken for it. +pub(crate) const GC_REACHABLE: u32 = u32::MAX; + /// Link implementation for GC intrusive linked list tracking pub(crate) struct GcLink; @@ -405,6 +413,11 @@ pub(super) struct PyInner { /// `track_object`; read to scope a collection to one interpreter. /// Sits in what would otherwise be padding, so it costs no space. pub(super) gc_owner: PyAtomic, + /// The count a running collection is working with: the strong count with + /// the references held from inside the candidate set taken off, or + /// [`GC_REACHABLE`] once the object has been proved reachable. Only + /// meaningful while `gc_bits` has [`GcBits::COLLECTING`]. + pub(super) gc_refs: PyAtomic, /// Intrusive linked list pointers for GC generational tracking pub(super) gc_pointers: Pointers, @@ -415,9 +428,11 @@ pub(super) struct PyInner { pub(crate) const SIZEOF_PYOBJECT_HEAD: usize = core::mem::size_of::>(); // ref_count, vtable, gc_pointers (two) and typ are one word each; the gc bits, -// generation and owner share the word of padding their alignment forces. Adding -// to that group is free only while this holds. -const _: () = assert!(SIZEOF_PYOBJECT_HEAD == 6 * core::mem::size_of::()); +// generation, owner and refs take eight bytes between them. A 64-bit header had +// those eight as the padding its alignment forces, so they cost it nothing; a +// 32-bit header spends a word on them. Adding to that group is free only while +// this holds. +const _: () = assert!(SIZEOF_PYOBJECT_HEAD == 5 * core::mem::size_of::() + 8); impl PyInner { /// Read type flags and member_count via raw pointers to avoid Stacked Borrows @@ -1248,6 +1263,7 @@ impl PyInner { gc_bits: Radium::new(0), gc_generation: Radium::new(GC_UNTRACKED), gc_owner: Radium::new(GC_NO_OWNER), + gc_refs: Radium::new(0), gc_pointers: Pointers::new(), typ: PyAtomicRef::from(typ), payload, @@ -1261,6 +1277,7 @@ impl PyInner { gc_bits: Radium::new(0), gc_generation: Radium::new(GC_UNTRACKED), gc_owner: Radium::new(GC_NO_OWNER), + gc_refs: Radium::new(0), gc_pointers: Pointers::new(), typ: PyAtomicRef::from(typ), payload, @@ -1803,6 +1820,57 @@ impl PyObject { self.0.gc_owner.store(owner, Ordering::Relaxed); } + /// Enter the running collection's candidate set, with `strong_count` as the + /// count to subtract internal references from. Counts that do not fit stop + /// one short of [`GC_REACHABLE`], which only ever keeps the object alive. + #[inline] + pub(crate) fn start_gc_refs(&self, strong_count: usize) { + let refs = strong_count.min(GC_REACHABLE as usize - 1) as u32; + self.0.gc_refs.store(refs, Ordering::Relaxed); + self.set_gc_bit(GcBits::COLLECTING); + } + + /// The count the running collection is working with. + #[inline] + pub(crate) fn gc_refs(&self) -> u32 { + self.0.gc_refs.load(Ordering::Relaxed) + } + + /// Whether this object is in the running collection's candidate set. + #[inline] + pub(crate) fn is_gc_collecting(&self) -> bool { + GcBits::from_bits_retain(self.0.gc_bits.load(Ordering::Relaxed)) + .contains(GcBits::COLLECTING) + } + + /// Take off one reference held from inside the candidate set. + #[inline] + pub(crate) fn subtract_gc_ref(&self) { + let refs = self.0.gc_refs.load(Ordering::Relaxed); + self.0 + .gc_refs + .store(refs.saturating_sub(1), Ordering::Relaxed); + } + + /// Mark the object reachable, answering whether this call was the one that + /// did it. + #[inline] + pub(crate) fn mark_gc_reachable(&self) -> bool { + if self.0.gc_refs.load(Ordering::Relaxed) == GC_REACHABLE { + return false; + } + self.0.gc_refs.store(GC_REACHABLE, Ordering::Relaxed); + true + } + + /// Leave the candidate set, whatever the collection concluded. + #[inline] + pub(crate) fn end_gc_refs(&self) { + self.0 + .gc_bits + .fetch_and(!GcBits::COLLECTING.bits(), Ordering::Relaxed); + } + /// _PyObject_GC_TRACK #[inline] pub(crate) fn set_gc_tracked(&self) { @@ -2723,6 +2791,7 @@ pub(crate) fn init_type_hierarchy() -> (PyTypeRef, PyTypeRef, PyTypeRef) { gc_bits: Radium::new(0), gc_generation: Radium::new(GC_UNTRACKED), gc_owner: Radium::new(GC_NO_OWNER), + gc_refs: Radium::new(0), gc_pointers: Pointers::new(), payload: type_payload, }, @@ -2739,6 +2808,7 @@ pub(crate) fn init_type_hierarchy() -> (PyTypeRef, PyTypeRef, PyTypeRef) { gc_bits: Radium::new(0), gc_generation: Radium::new(GC_UNTRACKED), gc_owner: Radium::new(GC_NO_OWNER), + gc_refs: Radium::new(0), gc_pointers: Pointers::new(), payload: object_payload, }, diff --git a/crates/vm/src/object/mod.rs b/crates/vm/src/object/mod.rs index becfcabb1d4..b6c7590a86d 100644 --- a/crates/vm/src/object/mod.rs +++ b/crates/vm/src/object/mod.rs @@ -9,5 +9,5 @@ pub use self::core::*; pub use self::ext::*; pub use self::payload::*; pub(crate) use core::SIZEOF_PYOBJECT_HEAD; -pub(crate) use core::{GC_NO_OWNER, GC_PERMANENT, GC_UNTRACKED, GcLink, GcOwner}; +pub(crate) use core::{GC_NO_OWNER, GC_PERMANENT, GC_REACHABLE, GC_UNTRACKED, GcLink, GcOwner}; pub use traverse::{MaybeTraverse, Traverse, TraverseFn}; From 457390973a67dde60ac4bf67be6ea1448144e600 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Tue, 18 Aug 2026 10:14:54 +0900 Subject: [PATCH 06/27] Take an explicit thread stack size as a floor in debug builds A debug build already started Python threads on 8 MiB rather than Rust's 2 MiB default, but an explicit threading.stack_size(N) went through verbatim. test_threading asks for 256 KiB, and starting a thread on that walked off the end of the stack: the guard page fault landed in the prologue of ExecutingFrame::run. Unoptimized, that prologue reserves 80,848 bytes where the optimized one reserves 656 -- execute_instruction is #[inline(always)] and LLVM only colors stack slots from opt-level 1, so the frame is the sum of all 200 instruction arms' temporaries rather than the largest. A Python call costs 88,672 bytes of native stack there, and threading's bootstrap is six frames deep, so 256 KiB holds less than half of what starting a thread takes. The floor reaches thread::Builder only; threading.stack_size() still answers with what was asked for, and release builds are unchanged. Assisted-by: Claude --- crates/vm/src/stdlib/_thread.rs | 50 +++++++++++++++++++++++++++------ 1 file changed, 41 insertions(+), 9 deletions(-) diff --git a/crates/vm/src/stdlib/_thread.rs b/crates/vm/src/stdlib/_thread.rs index 79dce3d21ce..a01711e1a36 100644 --- a/crates/vm/src/stdlib/_thread.rs +++ b/crates/vm/src/stdlib/_thread.rs @@ -616,25 +616,32 @@ pub(crate) mod _thread { const DEFAULT_THREAD_STACK_SIZE: usize = 8 * 1024 * 1024; /// Configure a `thread::Builder` with the stack size to use for a new - /// Python thread. Uses the value set via `threading.stack_size(N)` when - /// the user has provided one (non-zero). Otherwise, debug builds fall - /// back to [`DEFAULT_THREAD_STACK_SIZE`] and release builds leave the - /// builder unmodified (Rust's std default applies). + /// Python thread. Release builds use the value set via + /// `threading.stack_size(N)` when the user has provided one (non-zero) and + /// otherwise leave the builder unmodified (Rust's std default applies). + /// + /// Debug builds take [`DEFAULT_THREAD_STACK_SIZE`] as a floor rather than + /// only as a default: an unoptimized `ExecutingFrame::run` reserves around + /// eighty kilobytes of stack where an optimized one reserves under a + /// thousand, so a size that holds a Python call chain in release holds + /// three of its frames here — starting a thread at all needs six. The + /// value `threading.stack_size()` reports is untouched. fn apply_thread_stack_size( thread_builder: thread::Builder, vm: &VirtualMachine, ) -> thread::Builder { let configured = vm.state.stacksize.load(); - if configured != 0 { - return thread_builder.stack_size(configured); - } #[cfg(debug_assertions)] { - thread_builder.stack_size(DEFAULT_THREAD_STACK_SIZE) + thread_builder.stack_size(configured.max(DEFAULT_THREAD_STACK_SIZE)) } #[cfg(not(debug_assertions))] { - thread_builder + if configured == 0 { + thread_builder + } else { + thread_builder.stack_size(configured) + } } } @@ -2029,6 +2036,31 @@ pub(crate) mod _thread { }); } + /// A size small enough for CPython's frames is not small enough for an + /// unoptimized build's: `test_threading` asks for 256 KiB, which holds + /// three of them where starting a thread needs six. The size the + /// request set is still what `threading.stack_size()` answers with. + #[test] + #[cfg(all(debug_assertions, any(target_os = "linux", target_os = "macos")))] + fn explicit_python_thread_stack_size_is_a_floor_debug() { + const REQUESTED: usize = 256 * 1024; + + Interpreter::without_stdlib(Default::default()).enter(|vm| { + vm.state.stacksize.store(REQUESTED); + let builder = apply_thread_stack_size(thread::Builder::new(), vm); + let stack_size = builder + .spawn(current_thread_stack_size) + .expect("failed to spawn thread") + .join() + .expect("thread panicked"); + assert!( + stack_size >= DEFAULT_THREAD_STACK_SIZE, + "Python thread stack size is {stack_size} bytes, expected at least {DEFAULT_THREAD_STACK_SIZE}" + ); + assert_eq!(vm.state.stacksize.load(), REQUESTED); + }); + } + #[cfg(all(debug_assertions, target_os = "linux"))] fn current_thread_stack_size() -> usize { use libc::{ From b077f203115aa24acfac63c9302d38dcf21f08d5 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Tue, 18 Aug 2026 10:51:00 +0900 Subject: [PATCH 07/27] Check the native stack on every frame entry The C-stack guard ran on one frame entry in eight. That asks the margin to cover eight frames rather than one, and it does not: an unoptimized frame entered through native code takes 88,672 bytes against a debug margin of 262,144. A recursion whose steps re-enter that way -- `__add__` calling itself, a sort key that sorts -- ran off the end of the stack instead of raising RecursionError. On a debug build `class Add: __add__ = lambda s, o: s + o; Add() + 1` segfaulted on the main thread; it now raises, as it does under CPython and in release builds. enter_iframe checked and then called enter_iframe_unchecked, which checked again; it now leaves the check to the one call. Measured on a call-dominated benchmark, five interleaved pairs: instructions retired go up 0.17%, about four per call, which is the stack pointer read and the compare. Assisted-by: Claude --- crates/vm/src/vm/mod.rs | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/crates/vm/src/vm/mod.rs b/crates/vm/src/vm/mod.rs index 54d3e813eec..cea65c37b3a 100644 --- a/crates/vm/src/vm/mod.rs +++ b/crates/vm/src/vm/mod.rs @@ -2161,13 +2161,12 @@ impl VirtualMachine { ) -> PyResult { self.check_recursive_call("")?; - // Check the native C stack periodically. The sampling interval - // (every 8th call) balances overhead against the risk of missing - // an overflow between checks, especially when light and heavy - // frames alternate (each recursion step uses different native - // stack amounts). - let depth = self.recursion_depth.get(); - if depth & 7 == 0 && self.check_c_stack_overflow() { + // Every entry, not every eighth. The margin only has to cover what a + // single frame takes if the check runs each time; sampling asks it to + // cover eight, and a recursion whose steps re-enter through native + // code -- an `__add__` chain, a sort key that sorts -- takes more than + // the margin in that many. + if self.check_c_stack_overflow() { return Err(self.new_recursion_error(String::new())); } @@ -2261,11 +2260,7 @@ impl VirtualMachine { ) -> PyResult { self.check_recursive_call("")?; - let depth = self.recursion_depth.get(); - if depth & 7 == 0 && self.check_c_stack_overflow() { - return Err(self.new_recursion_error(String::new())); - } - + // The C stack is checked by `enter_iframe_unchecked` below. self.enter_iframe_unchecked(iframe) } @@ -2278,8 +2273,7 @@ impl VirtualMachine { &self, iframe: &mut crate::frame::InterpreterFrame, ) -> PyResult { - let depth = self.recursion_depth.get(); - if depth & 7 == 0 && self.check_c_stack_overflow() { + if self.check_c_stack_overflow() { return Err(self.new_recursion_error(String::new())); } From 45ce8f4820bf96709659411ecf783b319551ae22 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Tue, 18 Aug 2026 12:21:03 +0900 Subject: [PATCH 08/27] Take an iterable's length hint when a list is filled from it `map_py_iter` read `__length_hint__` only to pass it to `PyIterIter`, and returned an empty vector when the hint was `isize::MAX` or more. Collecting through `PyResult` dropped the iterator's lower bound, so nothing reserved the room the hint asked for. `list()`, `list.extend()` and `list.__iadd__()` now reserve it and report a hint they cannot honour as `MemoryError`; a hint that leaves no room for the elements the list already holds is passed over, as `list_extend()` does. `tuple()` and the other callers keep filling up without reserving. `length_hint_opt` errors other than the `TypeError` it already turns into `None` now reach the caller instead of being dropped. `__iadd__` and `inplace_concat` went through `extract_cloned`, which reads `__len__` and not `__length_hint__`; both call `PyList::extend` now, the way `list_inplace_concat()` calls `list_extend()`. The tuple, list and dict fast paths of `extract_elements_inner` reserve their known length, which the `collect()` they used dropped. Assisted-by: Claude --- crates/vm/src/builtins/list.rs | 13 ++-- crates/vm/src/vm/mod.rs | 109 ++++++++++++++++++++------- extra_tests/snippets/builtin_list.py | 32 ++++++++ 3 files changed, 120 insertions(+), 34 deletions(-) diff --git a/crates/vm/src/builtins/list.rs b/crates/vm/src/builtins/list.rs index fe674a45821..d6215731878 100644 --- a/crates/vm/src/builtins/list.rs +++ b/crates/vm/src/builtins/list.rs @@ -187,7 +187,10 @@ impl PyList { #[pymethod] pub(crate) fn extend(&self, x: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> { - let mut new_elements = x.try_to_value(vm)?; + // What is already here decides whether the iterable's length hint is + // believable, so it goes along with the request for the elements. + let held = self.borrow_vec().len(); + let mut new_elements = vm.extract_elements_sized(&x, held, Ok)?; self.borrow_vec_mut().append(&mut new_elements); Ok(()) } @@ -221,8 +224,7 @@ impl PyList { other: &PyObject, vm: &VirtualMachine, ) -> PyResult { - let mut seq = extract_cloned(other, Ok, vm)?; - zelf.borrow_vec_mut().append(&mut seq); + zelf.extend(other.to_owned(), vm)?; Ok(zelf.to_owned().into()) } @@ -231,8 +233,7 @@ impl PyList { other: PyObjectRef, vm: &VirtualMachine, ) -> PyResult> { - let mut seq = extract_cloned(&other, Ok, vm)?; - zelf.borrow_vec_mut().append(&mut seq); + zelf.extend(other, vm)?; Ok(zelf) } @@ -481,7 +482,7 @@ impl Initializer for PyList { fn init(zelf: PyRef, iterable: Self::Args, vm: &VirtualMachine) -> PyResult<()> { let mut elements = if let OptionalArg::Present(iterable) = iterable { - iterable.try_to_value(vm)? + vm.extract_elements_sized(&iterable, 0, Ok)? } else { vec![] }; diff --git a/crates/vm/src/vm/mod.rs b/crates/vm/src/vm/mod.rs index cea65c37b3a..ad29b26515c 100644 --- a/crates/vm/src/vm/mod.rs +++ b/crates/vm/src/vm/mod.rs @@ -2629,6 +2629,49 @@ impl VirtualMachine { where F: Fn(PyObjectRef) -> PyResult, { + self.extract_elements_inner(value, None, func) + } + + /// [`Self::extract_elements_with`] for a caller that allocates the + /// iterable's length hint up front, joining `held` elements it already + /// has. `list` does this, so a hint it cannot honour is a `MemoryError` + /// there; `tuple` does not, and finds out by filling up. + pub fn extract_elements_sized( + &self, + value: &PyObject, + held: usize, + func: F, + ) -> PyResult> + where + F: Fn(PyObjectRef) -> PyResult, + { + self.extract_elements_inner(value, Some(held), func) + } + + fn extract_elements_inner( + &self, + value: &PyObject, + held: Option, + func: F, + ) -> PyResult> + where + F: Fn(PyObjectRef) -> PyResult, + { + // A count known up front is taken in one go. Collecting into a + // `Result` instead would drop it: the adapter that carries the error + // may stop early, so it reports no lower bound and the vector grows a + // step at a time. + fn map_known_len( + items: impl ExactSizeIterator, + func: impl Fn(T) -> PyResult, + ) -> PyResult> { + let mut results = Vec::with_capacity(items.len()); + for item in items { + results.push(func(item)?); + } + Ok(results) + } + // Type-specific fast paths corresponding to _list_extend() in CPython // Objects/listobject.c. Each branch takes an atomic snapshot to avoid // race conditions from concurrent mutation (no GIL). @@ -2638,9 +2681,11 @@ impl VirtualMachine { } else if cls.is(self.ctx.types.list_type) { // The list is re-read on every step, the way map_iterable_object() // does it: func() runs Python, which can mutate or even clear the - // same list, and a borrow held across that call deadlocks it. + // same list, and a borrow held across that call deadlocks it. Its + // length at the start is only how much room to take, not how far + // the loop runs. let list = value.downcast_ref::().unwrap(); - let mut results = Vec::new(); + let mut results = Vec::with_capacity(list.borrow_vec().len()); let mut i = 0; loop { let elem = { @@ -2657,31 +2702,30 @@ impl VirtualMachine { return Ok(results); } else if cls.is(self.ctx.types.dict_type) { let keys = value.downcast_ref::().unwrap().keys_vec(); - return keys.into_iter().map(func).collect(); + return map_known_len(keys.into_iter(), func); } else if cls.is(self.ctx.types.dict_keys_type) { let keys = value.downcast_ref::().unwrap().dict.keys_vec(); - return keys.into_iter().map(func).collect(); + return map_known_len(keys.into_iter(), func); } else if cls.is(self.ctx.types.dict_values_type) { let values = value .downcast_ref::() .unwrap() .dict .values_vec(); - return values.into_iter().map(func).collect(); + return map_known_len(values.into_iter(), func); } else if cls.is(self.ctx.types.dict_items_type) { let items = value .downcast_ref::() .unwrap() .dict .items_vec(); - return items - .into_iter() - .map(|(k, v)| func(self.ctx.new_tuple(vec![k, v]).into())) - .collect(); + return map_known_len(items.into_iter(), |(k, v)| { + func(self.ctx.new_tuple(vec![k, v]).into()) + }); } else { - return self.map_py_iter(value, func); + return self.map_py_iter(value, held, func); }; - slice.iter().map(|obj| func(obj.clone())).collect() + map_known_len(slice.iter(), |obj| func(obj.clone())) } pub fn map_iterable_object(&self, obj: &PyObject, mut f: F) -> PyResult>> @@ -2713,33 +2757,42 @@ impl VirtualMachine { ref t @ PyTuple => Ok(t.iter().cloned().map(f).collect()), // TODO: put internal iterable type obj => { - Ok(self.map_py_iter(obj, f)) + Ok(self.map_py_iter(obj, None, f)) } }) } - fn map_py_iter(&self, value: &PyObject, mut f: F) -> PyResult> + fn map_py_iter(&self, value: &PyObject, held: Option, mut f: F) -> PyResult> where F: FnMut(PyObjectRef) -> PyResult, { let iter = value.to_owned().get_iter(self)?; - let cap = match self.length_hint_opt(value.to_owned()) { - Err(e) if e.class().is(self.ctx.exceptions.runtime_error) => return Err(e), - Ok(Some(value)) => Some(value), - // Use a power of 2 as a default capacity. - _ => None, - }; - // TODO: fix extend to do this check (?), see test_extend in Lib/test/list_tests.py, - // https://github.com/python/cpython/blob/v3.9.0/Objects/listobject.c#L922-L928 - if let Some(cap) = cap - && cap >= isize::MAX as usize + // `length_hint_opt` already answers `None` for the iterable that + // declines to guess; anything else it reports is the iterable's own + // error and belongs to the caller. + let cap = self.length_hint_opt(value.to_owned())?; + + // Take the room the iterable asks for up front, for the callers that + // do. Collecting into a `Result` drops the iterator's lower bound -- + // the adapter may stop early -- so without this the vector grows a step + // at a time and an iterable claiming more elements than can be held is + // found out by running out of memory rather than by saying so. + // + // A hint that does not leave room for what is already held is one the + // iterable cannot be telling the truth about, so it is passed over + // rather than refused: if it was honest the loop runs out of memory on + // its own, and if it lied there was nothing wrong to report. + let mut results: Vec = Vec::new(); + if let (Some(held), Some(cap)) = (held, cap) + && held <= (isize::MAX as usize) - cap { - return Ok(Vec::new()); + results + .try_reserve_exact(cap) + .map_err(|_| self.new_memory_error(""))?; + } + for element in PyIterIter::new(self, iter.as_ref(), cap) { + results.push(f(element?)?); } - - let mut results = PyIterIter::new(self, iter.as_ref(), cap) - .map(|element| f(element?)) - .collect::>>()?; results.shrink_to_fit(); Ok(results) } diff --git a/extra_tests/snippets/builtin_list.py b/extra_tests/snippets/builtin_list.py index 44492092bad..0157b0c80dd 100644 --- a/extra_tests/snippets/builtin_list.py +++ b/extra_tests/snippets/builtin_list.py @@ -930,3 +930,35 @@ def __eq__(self, other): # that product must raise instead of wrapping into a short allocation. with assert_raises(MemoryError): [1] * sys.maxsize + + +# A list takes the length an iterable reports before reading it, so one that +# reports more than can be held says so instead of filling memory. +class Reports: + def __init__(self, hint): + self.hint = hint + + def __iter__(self): + return iter([1, 2, 3]) + + def __length_hint__(self): + return self.hint + + +with assert_raises(MemoryError): + list(Reports(sys.maxsize)) +with assert_raises(MemoryError): + [].extend(Reports(sys.maxsize)) +with assert_raises(MemoryError): + empty = [] + empty += Reports(sys.maxsize) + +# A report that leaves no room for what the list already holds cannot be true, +# so it is passed over rather than refused. +held = [1, 2, 3, 4] +held.extend(Reports(sys.maxsize)) +assert held == [1, 2, 3, 4, 1, 2, 3] + +# A report the list can act on is acted on. +assert list(Reports(3)) == [1, 2, 3] +assert list(Reports(0)) == [1, 2, 3] From c7e2658a951e3c0bace8d2d1935c9234ddfdff70 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Tue, 18 Aug 2026 12:28:15 +0900 Subject: [PATCH 09/27] Drop map.__length_hint__ `map_methods` has no `__length_hint__`, so `operator.length_hint()` on a map answers 0, not the length of what it draws from. The method walked into the length hint of every iterator it holds, and a chain of maps 10000 long overflowed the native stack answering for the outermost one. It also took the longest of its iterators, where a map stops at the shortest. Assisted-by: Claude --- crates/vm/src/builtins/map.rs | 9 --------- extra_tests/snippets/builtin_map.py | 13 +++++++++++++ 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/crates/vm/src/builtins/map.rs b/crates/vm/src/builtins/map.rs index cb8db23e640..2606b61933a 100644 --- a/crates/vm/src/builtins/map.rs +++ b/crates/vm/src/builtins/map.rs @@ -54,15 +54,6 @@ impl Constructor for PyMap { #[pyclass(with(IterNext, Iterable, Constructor), flags(BASETYPE))] impl PyMap { - #[pymethod] - fn __length_hint__(&self, vm: &VirtualMachine) -> PyResult { - self.iterators.iter().try_fold(0, |prev, cur| { - let cur = cur.as_ref().to_owned().length_hint(0, vm)?; - let max = core::cmp::max(prev, cur); - Ok(max) - }) - } - #[pymethod] fn __reduce__(zelf: PyRef, vm: &VirtualMachine) -> PyTupleRef { let cls = zelf.class().to_owned(); diff --git a/extra_tests/snippets/builtin_map.py b/extra_tests/snippets/builtin_map.py index 559d108e38b..042a4947ff3 100644 --- a/extra_tests/snippets/builtin_map.py +++ b/extra_tests/snippets/builtin_map.py @@ -32,3 +32,16 @@ def mapping(x): assert list(map(mapping, [1, 2, 0, 4, 5])) == [1, 2] + + +# map does not report a length hint, so a chain of them is not walked to +# answer for one. +import operator + +assert not hasattr(map(lambda x: x, [1, 2, 3]), "__length_hint__") +assert operator.length_hint(map(lambda x: x, [1, 2, 3])) == 0 + +it = iter([1, 2, 3]) +for _ in range(10000): + it = map(lambda x: x, it) +assert operator.length_hint(it) == 0 From 94dc4dbfb6130b9d5003389c31ff08b60abcae0e Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Tue, 18 Aug 2026 17:47:49 +0900 Subject: [PATCH 10/27] Ask for a length hint where each caller asks for it `map_py_iter` asked the iterable it was handed, for every caller, and reported what asking raised. Only some callers ask it: `list_extend()` and `_PyBytes_FromIterator()` ask the iterable, `PySequence_Tuple()` asks the iterator, and the bytearray constructor asks nothing. `tuple()`, `min()`, `max()`, `collections.deque()` and `f(*x)` raised for an iterable whose `__len__` or `__length_hint__` does, where they answer. Which object is asked is now the caller's to say. `sorted()` asks the iterable, being `PySequence_List()`. `bytes_from_object()` stood in for `PyBytes_FromObject()`, for `bytearray_extend()` and for the bytearray constructor, which do not agree on this: the first two ask, the last does not. It is split, and assigning to a bytearray slice takes the constructor's side with `PyByteArray_FromObject()`. `list.extend()` counted what it held before the iterable had been asked, where `list_extend()` reads `Py_SIZE(self)` after. A `__length_hint__` that adds to the list made the overflow guard read a count too small and raise `MemoryError` where nothing is wrong; one that empties it made the guard skip a reservation that cannot be served. Assisted-by: Claude --- crates/vm/src/builtins/bytearray.rs | 6 +- crates/vm/src/builtins/bytes.rs | 4 +- crates/vm/src/builtins/list.rs | 9 +-- crates/vm/src/byte.rs | 27 +++++++-- crates/vm/src/bytes_inner.rs | 31 +++++++--- crates/vm/src/stdlib/builtins.rs | 4 +- crates/vm/src/vm/mod.rs | 85 +++++++++++++++++++++------ extra_tests/snippets/builtin_bytes.py | 22 +++++++ extra_tests/snippets/builtin_list.py | 57 ++++++++++++++++++ 9 files changed, 203 insertions(+), 42 deletions(-) diff --git a/crates/vm/src/builtins/bytearray.rs b/crates/vm/src/builtins/bytearray.rs index c8782127b3f..24cad2950c8 100644 --- a/crates/vm/src/builtins/bytearray.rs +++ b/crates/vm/src/builtins/bytearray.rs @@ -8,7 +8,7 @@ use crate::{ VirtualMachine, anystr::{self, AnyStr}, atomic_func, - byte::{bytes_from_object, value_from_object}, + byte::{bytearray_from_object, bytes_from_object, value_from_object}, bytes_inner::{ ByteInnerFindOptions, ByteInnerHexOptions, ByteInnerNewOptions, ByteInnerPaddingOptions, ByteInnerSplitOptions, ByteInnerSub, ByteInnerTranslateOptions, DecodeArgs, PyBytesInner, @@ -115,7 +115,7 @@ impl PyByteArray { let items = if zelf.is(&value) { zelf.borrow_buf().to_vec() } else { - bytes_from_object(vm, &value)? + bytearray_from_object(vm, &value)? }; if let Some(mut w) = zelf.try_resizable_opt() { w.elements.setitem_by_slice(vm, slice, &items) @@ -716,7 +716,7 @@ impl Initializer for PyByteArray { fn init(zelf: PyRef, options: Self::Args, vm: &VirtualMachine) -> PyResult<()> { // First unpack bytearray and *then* get a lock to set it. - let mut inner = options.get_bytearray_inner(vm)?; + let mut inner = options.get_inner(bytearray_from_object, vm)?; core::mem::swap(&mut *zelf.inner_mut(), &mut inner); Ok(()) } diff --git a/crates/vm/src/builtins/bytes.rs b/crates/vm/src/builtins/bytes.rs index bfa2fd3545b..79130a28c44 100644 --- a/crates/vm/src/builtins/bytes.rs +++ b/crates/vm/src/builtins/bytes.rs @@ -8,6 +8,7 @@ use crate::{ TryFromBorrowedObject, VirtualMachine, anystr::{self, AnyStr}, atomic_func, + byte::bytes_from_object, bytes_inner::{ ByteInnerFindOptions, ByteInnerHexOptions, ByteInnerNewOptions, ByteInnerPaddingOptions, ByteInnerSplitOptions, ByteInnerSub, ByteInnerTranslateOptions, DecodeArgs, PyBytesInner, @@ -138,8 +139,7 @@ impl Constructor for PyBytes { return payload.into_ref_with_type(vm, cls).map(Into::into); } - // Fallback to get_bytearray_inner - let elements = options.get_bytearray_inner(vm)?.elements; + let elements = options.get_inner(bytes_from_object, vm)?.elements; // Return empty bytes singleton for exact bytes types if elements.is_empty() && cls.is(vm.ctx.types.bytes_type) { diff --git a/crates/vm/src/builtins/list.rs b/crates/vm/src/builtins/list.rs index d6215731878..02a6d8327e2 100644 --- a/crates/vm/src/builtins/list.rs +++ b/crates/vm/src/builtins/list.rs @@ -188,9 +188,10 @@ impl PyList { #[pymethod] pub(crate) fn extend(&self, x: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> { // What is already here decides whether the iterable's length hint is - // believable, so it goes along with the request for the elements. - let held = self.borrow_vec().len(); - let mut new_elements = vm.extract_elements_sized(&x, held, Ok)?; + // believable, so it goes along with the request for the elements. It is + // counted where `list_extend()` reads `Py_SIZE(self)`, after the + // iterable has answered, because answering runs code that can change it. + let mut new_elements = vm.extract_elements_sized(&x, &|| self.borrow_vec().len(), Ok)?; self.borrow_vec_mut().append(&mut new_elements); Ok(()) } @@ -482,7 +483,7 @@ impl Initializer for PyList { fn init(zelf: PyRef, iterable: Self::Args, vm: &VirtualMachine) -> PyResult<()> { let mut elements = if let OptionalArg::Present(iterable) = iterable { - vm.extract_elements_sized(&iterable, 0, Ok)? + vm.extract_elements_sized(&iterable, &|| 0, Ok)? } else { vec![] }; diff --git a/crates/vm/src/byte.rs b/crates/vm/src/byte.rs index 0e90f296ac9..3b3fc132519 100644 --- a/crates/vm/src/byte.rs +++ b/crates/vm/src/byte.rs @@ -3,21 +3,38 @@ use num_traits::ToPrimitive; use crate::{ - AsObject, PyObject, PyResult, VirtualMachine, + AsObject, PyObject, PyObjectRef, PyResult, VirtualMachine, protocol::{BufferFlags, PyBuffer}, }; // PyBytes_FromObject pub fn bytes_from_object(vm: &VirtualMachine, obj: &PyObject) -> PyResult> { + collect_bytes(vm, obj, true) +} + +/// [`bytes_from_object`] for the bytearray constructor and for assigning to a +/// slice of one, which run the iterator without asking the object they were +/// handed how long it is. +pub fn bytearray_from_object(vm: &VirtualMachine, obj: &PyObject) -> PyResult> { + collect_bytes(vm, obj, false) +} + +fn collect_bytes(vm: &VirtualMachine, obj: &PyObject, measured: bool) -> PyResult> { if obj.check_buffer() { let buffer = PyBuffer::from_object(vm, obj, BufferFlags::FULL_RO)?; return Ok(buffer.contiguous_or_collect(|bytes| bytes.to_vec())); } - if !obj.fast_isinstance(vm.ctx.types.str_type) - && let Ok(elements) = vm.map_iterable_object(obj, |x| value_from_object(vm, &x)) - { - return elements; + if !obj.fast_isinstance(vm.ctx.types.str_type) { + let value = |x: PyObjectRef| value_from_object(vm, &x); + let elements = if measured { + vm.map_iterable_object_sized(obj, value) + } else { + vm.map_iterable_object(obj, value) + }; + if let Ok(elements) = elements { + return elements; + } } Err(vm.new_type_error("can assign only bytes, buffers, or iterables of ints in range(0, 256)")) diff --git a/crates/vm/src/bytes_inner.rs b/crates/vm/src/bytes_inner.rs index e16c636a964..11b3b1e1d96 100644 --- a/crates/vm/src/bytes_inner.rs +++ b/crates/vm/src/bytes_inner.rs @@ -7,7 +7,6 @@ use crate::{ PyBaseExceptionRef, PyByteArray, PyBytes, PyBytesRef, PyInt, PyIntRef, PyStr, PyStrRef, pystr, pystr::PyUtf8StrRef, }, - byte::bytes_from_object, cformat::cformat_bytes, common::hash, common::wtf8::is_py_ascii_whitespace, @@ -17,6 +16,11 @@ use crate::{ sequence::{SequenceExt, SequenceMutExt}, types::PyComparisonOp, }; +/// How a source object that is neither a size nor a string is turned into +/// bytes: [`crate::byte::bytes_from_object`] or +/// [`crate::byte::bytearray_from_object`]. +pub(crate) type FromObject = fn(&VirtualMachine, &PyObject) -> PyResult>; + use bstr::ByteSlice; use itertools::Itertools; use malachite_bigint::BigInt; @@ -64,8 +68,12 @@ impl ByteInnerNewOptions { Ok(bytes.as_bytes().to_vec().into()) } - fn get_value_from_source(source: PyObjectRef, vm: &VirtualMachine) -> PyResult { - bytes_from_object(vm, &source).map(|x| x.into()) + fn get_value_from_source( + source: PyObjectRef, + from_object: FromObject, + vm: &VirtualMachine, + ) -> PyResult { + from_object(vm, &source).map(|x| x.into()) } fn get_value_from_size(size: PyIntRef, vm: &VirtualMachine) -> PyResult { @@ -81,19 +89,26 @@ impl ByteInnerNewOptions { Ok(vm.new_zeroed_bytes(size)?.into()) } - fn handle_object_fallback(obj: PyObjectRef, vm: &VirtualMachine) -> PyResult { + fn handle_object_fallback( + obj: PyObjectRef, + from_object: FromObject, + vm: &VirtualMachine, + ) -> PyResult { match_class!(match obj { i @ PyInt => { Self::get_value_from_size(i, vm) } _s @ PyStr => Err(vm.new_type_error(STRING_WITHOUT_ENCODING.to_owned())), obj => { - Self::get_value_from_source(obj, vm) + Self::get_value_from_source(obj, from_object, vm) } }) } - pub fn get_bytearray_inner(self, vm: &VirtualMachine) -> PyResult { + /// `from_object` is how a source that is neither a size nor a string is + /// read: `bytes()` and `bytearray()` differ in whether they ask it how long + /// it is. + pub fn get_inner(self, from_object: FromObject, vm: &VirtualMachine) -> PyResult { match (self.source, self.encoding, self.errors) { (OptionalArg::Present(obj), OptionalArg::Missing, OptionalArg::Missing) => { // Try __index__ first to handle int-like objects that might raise custom exceptions @@ -105,7 +120,7 @@ impl ByteInnerNewOptions { // TypeError means the object doesn't support __index__, so fall back if e.fast_isinstance(vm.ctx.exceptions.type_error) { // Fall back to treating as buffer-like object - Self::handle_object_fallback(obj, vm) + Self::handle_object_fallback(obj, from_object, vm) } else { // Propagate other exceptions (e.g., ZeroDivisionError) Err(e) @@ -113,7 +128,7 @@ impl ByteInnerNewOptions { } } } else { - Self::handle_object_fallback(obj, vm) + Self::handle_object_fallback(obj, from_object, vm) } } (OptionalArg::Present(obj), OptionalArg::Present(encoding), errors) => { diff --git a/crates/vm/src/stdlib/builtins.rs b/crates/vm/src/stdlib/builtins.rs index 34be8c6178d..c3eb200af6d 100644 --- a/crates/vm/src/stdlib/builtins.rs +++ b/crates/vm/src/stdlib/builtins.rs @@ -1175,7 +1175,9 @@ mod builtins { #[pyfunction] fn sorted(iterable: PyObjectRef, opts: SortOptions, vm: &VirtualMachine) -> PyResult { - let items: Vec<_> = iterable.try_to_value(vm)?; + // `PySequence_List()`, so the room comes from what the iterable reports + // rather than from its iterator. + let items = vm.extract_elements_sized(&iterable, &|| 0, Ok)?; let lst = PyList::from(items); lst.sort(opts, vm)?; Ok(lst) diff --git a/crates/vm/src/vm/mod.rs b/crates/vm/src/vm/mod.rs index ad29b26515c..20e516d2aaf 100644 --- a/crates/vm/src/vm/mod.rs +++ b/crates/vm/src/vm/mod.rs @@ -879,6 +879,17 @@ struct SuspendedFrame { is_entry: bool, } +/// Which object a sequence being built asks how much room to take, and how the +/// answer is used. `PySequence_Tuple()` asks the iterator and takes nothing on +/// the answer; `list_extend()` asks the iterable it was handed and reserves. +#[derive(Clone, Copy)] +enum LengthHint<'a> { + Iterator, + /// Reserves what the iterable answers, unless it leaves no room for the + /// count this returns. + Iterable(&'a dyn Fn() -> usize), +} + impl VirtualMachine { fn init_callable_cache(&mut self) -> PyResult<()> { self.callable_cache.len = Some(self.builtins.get_attr("len", self)?); @@ -2629,29 +2640,29 @@ impl VirtualMachine { where F: Fn(PyObjectRef) -> PyResult, { - self.extract_elements_inner(value, None, func) + self.extract_elements_inner(value, LengthHint::Iterator, func) } - /// [`Self::extract_elements_with`] for a caller that allocates the - /// iterable's length hint up front, joining `held` elements it already - /// has. `list` does this, so a hint it cannot honour is a `MemoryError` - /// there; `tuple` does not, and finds out by filling up. + /// [`Self::extract_elements_with`] for a caller that asks the iterable + /// itself how much room to take, the way `list_extend()` does. `held` + /// answers how many elements the caller already has, and is read after the + /// iterable has been asked, since asking runs its code. pub fn extract_elements_sized( &self, value: &PyObject, - held: usize, + held: &dyn Fn() -> usize, func: F, ) -> PyResult> where F: Fn(PyObjectRef) -> PyResult, { - self.extract_elements_inner(value, Some(held), func) + self.extract_elements_inner(value, LengthHint::Iterable(held), func) } fn extract_elements_inner( &self, value: &PyObject, - held: Option, + hint: LengthHint<'_>, func: F, ) -> PyResult> where @@ -2723,12 +2734,37 @@ impl VirtualMachine { func(self.ctx.new_tuple(vec![k, v]).into()) }); } else { - return self.map_py_iter(value, held, func); + return self.map_py_iter(value, hint, func); }; map_known_len(slice.iter(), |obj| func(obj.clone())) } - pub fn map_iterable_object(&self, obj: &PyObject, mut f: F) -> PyResult>> + /// [`Self::map_iterable_object`] for a caller that asks the object it was + /// handed how long it is, rather than its iterator. + pub fn map_iterable_object_sized( + &self, + obj: &PyObject, + f: F, + ) -> PyResult>> + where + F: FnMut(PyObjectRef) -> PyResult, + { + self.map_iterable_object_inner(obj, LengthHint::Iterable(&|| 0), f) + } + + pub fn map_iterable_object(&self, obj: &PyObject, f: F) -> PyResult>> + where + F: FnMut(PyObjectRef) -> PyResult, + { + self.map_iterable_object_inner(obj, LengthHint::Iterator, f) + } + + fn map_iterable_object_inner( + &self, + obj: &PyObject, + hint: LengthHint<'_>, + mut f: F, + ) -> PyResult>> where F: FnMut(PyObjectRef) -> PyResult, { @@ -2757,20 +2793,29 @@ impl VirtualMachine { ref t @ PyTuple => Ok(t.iter().cloned().map(f).collect()), // TODO: put internal iterable type obj => { - Ok(self.map_py_iter(obj, None, f)) + Ok(self.map_py_iter(obj, hint, f)) } }) } - fn map_py_iter(&self, value: &PyObject, held: Option, mut f: F) -> PyResult> + fn map_py_iter( + &self, + value: &PyObject, + hint: LengthHint<'_>, + mut f: F, + ) -> PyResult> where F: FnMut(PyObjectRef) -> PyResult, { let iter = value.to_owned().get_iter(self)?; - // `length_hint_opt` already answers `None` for the iterable that - // declines to guess; anything else it reports is the iterable's own - // error and belongs to the caller. - let cap = self.length_hint_opt(value.to_owned())?; + // Whichever object the hint is asked of, an error it answers with is + // its own and belongs to the caller; `length_hint_opt` already answers + // `None` for the object that declines to guess. + let asked = match hint { + LengthHint::Iterator => iter.as_object(), + LengthHint::Iterable(_) => value, + }; + let cap = self.length_hint_opt(asked.to_owned())?; // Take the room the iterable asks for up front, for the callers that // do. Collecting into a `Result` drops the iterator's lower bound -- @@ -2781,10 +2826,12 @@ impl VirtualMachine { // A hint that does not leave room for what is already held is one the // iterable cannot be telling the truth about, so it is passed over // rather than refused: if it was honest the loop runs out of memory on - // its own, and if it lied there was nothing wrong to report. + // its own, and if it lied there was nothing wrong to report. What is + // held is counted now rather than before, since asking for the hint + // runs code that can add to it or take from it. let mut results: Vec = Vec::new(); - if let (Some(held), Some(cap)) = (held, cap) - && held <= (isize::MAX as usize) - cap + if let (LengthHint::Iterable(held), Some(cap)) = (hint, cap) + && held() <= (isize::MAX as usize) - cap { results .try_reserve_exact(cap) diff --git a/extra_tests/snippets/builtin_bytes.py b/extra_tests/snippets/builtin_bytes.py index 3cbed79c069..5b05936690d 100644 --- a/extra_tests/snippets/builtin_bytes.py +++ b/extra_tests/snippets/builtin_bytes.py @@ -766,3 +766,25 @@ def test_huge_size(): test_huge_size() + + +# bytes() asks the object it was handed how long it is, so what answering +# raises is the answer; the bytearray constructor asks nothing. +class BadLen: + def __iter__(self): + return iter([1, 2, 3]) + + def __len__(self): + raise RuntimeError("hello") + + +with assert_raises(RuntimeError): + bytes(BadLen()) +with assert_raises(RuntimeError): + int.from_bytes(BadLen(), "big") +assert bytearray(BadLen()) == bytearray(b"\x01\x02\x03") +with assert_raises(RuntimeError): + bytearray(b"ab").extend(BadLen()) +holder = bytearray(b"xyz") +holder[:] = BadLen() +assert holder == bytearray(b"\x01\x02\x03") diff --git a/extra_tests/snippets/builtin_list.py b/extra_tests/snippets/builtin_list.py index 0157b0c80dd..decf40ad4ae 100644 --- a/extra_tests/snippets/builtin_list.py +++ b/extra_tests/snippets/builtin_list.py @@ -959,6 +959,63 @@ def __length_hint__(self): held.extend(Reports(sys.maxsize)) assert held == [1, 2, 3, 4, 1, 2, 3] + +# Reporting runs the iterable's own code, so what the list holds is counted +# after the report rather than before it. +grew = [] + + +class Grows: + def __iter__(self): + return iter([7]) + + def __length_hint__(self): + grew.extend([0] * 100) + return sys.maxsize - 50 + + +grew.extend(Grows()) +assert len(grew) == 101 and grew[-1] == 7, grew[-3:] + +shrunk = [1] * 100 + + +class Shrinks: + def __iter__(self): + return iter([]) + + def __length_hint__(self): + shrunk.clear() + return sys.maxsize + + +with assert_raises(MemoryError): + shrunk.extend(Shrinks()) + + +# Only the callers that take the room ask the iterable itself; the rest ask its +# iterator, which is why an iterable whose __len__ raises reaches tuple() but +# not list(). +class Lazy: + def __len__(self): + raise NotImplementedError + + def __iter__(self): + return iter([1, 2, 3]) + + +assert tuple(Lazy()) == (1, 2, 3) +assert (lambda *a: a)(*Lazy()) == (1, 2, 3) +assert min(Lazy()) == 1 +with assert_raises(NotImplementedError): + list(Lazy()) +with assert_raises(NotImplementedError): + sorted(Lazy()) +with assert_raises(NotImplementedError): + [].extend(Lazy()) +with assert_raises(NotImplementedError): + [*Lazy()] + # A report the list can act on is acted on. assert list(Reports(3)) == [1, 2, 3] assert list(Reports(0)) == [1, 2, 3] From 5a9c726d6913359724ff169e22e22729d0df4462 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Tue, 18 Aug 2026 17:47:59 +0900 Subject: [PATCH 11/27] Settle product's pool count before it reads its arguments `product_new()` checks `repeat` and works out `npools` before it calls `PySequence_Tuple()` on any argument, and fills the pools `npools` times. The pools were filled by repeating the arguments `repeat` times instead, which walks that many steps even with no arguments to repeat: `product(repeat=2**62)` counted up to it rather than answering `[()]`. The count was also worked out after the arguments had been read, so a repeat too large to serve ran their code first. Assisted-by: Claude --- crates/vm/src/stdlib/itertools.rs | 23 ++++++++++++++++------- extra_tests/snippets/stdlib_itertools.py | 22 ++++++++++++++++++++++ 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/crates/vm/src/stdlib/itertools.rs b/crates/vm/src/stdlib/itertools.rs index bc6ba3546a1..5f85f1b7238 100644 --- a/crates/vm/src/stdlib/itertools.rs +++ b/crates/vm/src/stdlib/itertools.rs @@ -1140,22 +1140,31 @@ mod decl { } let repeat = repeat as usize; - let mut single: Vec> = Vec::new(); - for arg in iterables.iter() { - single.push(arg.try_to_value(vm)?); - } - - let npools = single + // The count is settled before the arguments are read, the way + // `product_new()` settles it before it calls `PySequence_Tuple()` + // on any of them, so a repeat too large to serve does not run their + // code first. + let npools = iterables + .iter() .len() .checked_mul(repeat) .filter(|n| *n <= isize::MAX as usize / size_of::()) .ok_or_else(|| vm.new_overflow_error("repeat argument too large"))?; + let mut single: Vec> = Vec::new(); + for arg in iterables.iter() { + single.push(arg.try_to_value(vm)?); + } + let mut pools: Vec> = Vec::new(); pools .try_reserve_exact(npools) .map_err(|_| vm.new_memory_error(""))?; - pools.extend(core::iter::repeat_n(single, repeat).flatten()); + // Filled by index, the way `product_new()` fills a tuple of + // `npools`. Repeating the arguments `repeat` times instead walks + // that many steps even when there are no arguments to repeat, so + // `product(repeat=2**62)` would spin rather than answer `[()]`. + pools.extend((0..npools).map(|i| single[i % single.len()].clone())); let mut idxs = Vec::new(); idxs.try_reserve_exact(npools) diff --git a/extra_tests/snippets/stdlib_itertools.py b/extra_tests/snippets/stdlib_itertools.py index b5ee8645929..ef06ce52985 100644 --- a/extra_tests/snippets/stdlib_itertools.py +++ b/extra_tests/snippets/stdlib_itertools.py @@ -547,3 +547,25 @@ def __iter__(self): itertools.product([1], repeat=-1) with assert_raises(OverflowError): itertools.product([1, 2], repeat=2**60) + + +# The pools are filled by their own count, so a repeat with nothing to repeat +# answers at once instead of counting up to it. +assert list(itertools.product(repeat=2**62)) == [()] +assert list(itertools.product(repeat=0)) == [()] + + +# The count is settled before the arguments are read, so a repeat too large to +# serve does not run their code first. +ran = [] + + +class Watched: + def __iter__(self): + ran.append(True) + return iter([1]) + + +with assert_raises(OverflowError): + itertools.product(Watched(), repeat=2**62) +assert ran == [] From 4df44e8ecb3e7c9b05d1a76b2430650915172130 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Tue, 18 Aug 2026 17:48:00 +0900 Subject: [PATCH 12/27] Let the bool format answer with the error its value raised `pack_single()` leaves `'?'` to `PyObject_IsTrue()` and returns what that raised. Packing classified the error instead, so a `ValueError` from a `__bool__` came back as "memoryview: invalid value for format '?'". Assisted-by: Claude --- crates/vm/src/buffer.rs | 11 ++++++++++- extra_tests/snippets/builtin_memoryview.py | 22 ++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/crates/vm/src/buffer.rs b/crates/vm/src/buffer.rs index b3b33aa24fe..b1de6eafbe2 100644 --- a/crates/vm/src/buffer.rs +++ b/crates/vm/src/buffer.rs @@ -61,6 +61,15 @@ impl PackError { }; Self { kind, exception } } + + /// An error that is the answer exactly as it was raised. `pack_single()` + /// leaves `'?'` to `PyObject_IsTrue()` this way, with no message of its own. + fn raised(exception: PyBaseExceptionRef) -> Self { + Self { + kind: PackErrorKind::Raised, + exception, + } + } } static OVERFLOW_MSG: &str = "total struct size too long"; // not a const to reduce code size @@ -803,7 +812,7 @@ impl Packable for bool { data: &mut [u8], ) -> Result<(), PackError> { let v = ArgIntoBool::try_from_object(vm, arg) - .map_err(|e| PackError::from_exception(e, vm))? + .map_err(PackError::raised)? .into_bool() as u8; v.pack_int::(data); Ok(()) diff --git a/extra_tests/snippets/builtin_memoryview.py b/extra_tests/snippets/builtin_memoryview.py index a57f9c38ae5..ef28ffc3d79 100644 --- a/extra_tests/snippets/builtin_memoryview.py +++ b/extra_tests/snippets/builtin_memoryview.py @@ -939,3 +939,25 @@ def test_delete_answers_readonly_first(): test_setitem_error_kinds() test_setitem_propagates_index_errors() test_delete_answers_readonly_first() + + +def test_bool_format_keeps_its_own_error(): + # Deciding truth is the value's own code, and what it raises is the answer. + class Raises: + def __init__(self, exc): + self.exc = exc + + def __bool__(self): + raise self.exc + + view = memoryview(bytearray(b"\x00")).cast("?") + for exc in (ZeroDivisionError("boom"), ValueError("nope"), TypeError("nah")): + try: + view[0] = Raises(exc) + except type(exc) as e: + assert str(e) == str(exc), e + else: + raise AssertionError(f"expected {type(exc).__name__}") + + +test_bool_format_keeps_its_own_error() From aff3fcacadce8df8cbcae372cfffc68d9ea579ca Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Tue, 18 Aug 2026 23:02:01 +0900 Subject: [PATCH 13/27] Release a cell's old value after the lock `PyCell::set` dropped what it replaced while still holding the mutex guarding the cell contents. A `__del__` running from that drop and reading the same cell waited on a lock its own caller held, so `del it` on a closure variable whose value has such a `__del__` deadlocked. The replaced value is now released once the guard is gone, as `Py_XSETREF` stores before it decrefs. Assisted-by: Claude --- .cspell.dict/cpython.txt | 1 + crates/vm/src/builtins/function.rs | 6 +++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/.cspell.dict/cpython.txt b/.cspell.dict/cpython.txt index c6a60a3a7f0..f9aa440edee 100644 --- a/.cspell.dict/cpython.txt +++ b/.cspell.dict/cpython.txt @@ -277,5 +277,6 @@ winconsoleio withitem withs worklist +XSETREF xstat XXPRIME diff --git a/crates/vm/src/builtins/function.rs b/crates/vm/src/builtins/function.rs index a5342d1df3a..94c231b0b83 100644 --- a/crates/vm/src/builtins/function.rs +++ b/crates/vm/src/builtins/function.rs @@ -1578,7 +1578,11 @@ impl PyCell { } pub(crate) fn set(&self, x: Option) { - *self.contents.lock() = x; + // What was here is released after the lock, the way `Py_XSETREF` stores + // before it decrefs. Releasing it under the lock would let a `__del__` + // that reads this cell wait on a lock this call still holds. + let replaced = core::mem::replace(&mut *self.contents.lock(), x); + drop(replaced); } #[pygetset] From 8f8382c2bc5f5ada82058d8ffe2f05b72d3165bd Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Tue, 18 Aug 2026 23:02:16 +0900 Subject: [PATCH 14/27] Have a set iterator hold the set it iterates The iterator kept only a reference to the inner hash table, so in `it = iter(A(*args))` the `A()` temporary was the last owner and died as the call returned, before `it` was bound. A `__del__` reading `it` there saw an unbound name and its error was printed and ignored. The iterator now holds the set object, as `si_set` does, and releases it once exhausted. That release happens after the lock is dropped, where `setiter_iternext()` puts its `Py_DECREF(so)` past `Py_END_CRITICAL_SECTION()`, so a `__del__` that iterates again does not wait on a lock the call holds. Assisted-by: Claude --- crates/vm/src/builtins/set.rs | 73 +++++++++++++++++++++-------------- 1 file changed, 43 insertions(+), 30 deletions(-) diff --git a/crates/vm/src/builtins/set.rs b/crates/vm/src/builtins/set.rs index d737612b158..31784b6f054 100644 --- a/crates/vm/src/builtins/set.rs +++ b/crates/vm/src/builtins/set.rs @@ -3,7 +3,7 @@ */ use super::{ IterStatus, PositionIterInternal, PyDict, PyDictRef, PyGenericAlias, PyTupleRef, PyType, - PyTypeRef, builtins_iter, + PyTypeRef, builtins_iter, locked_step, }; use crate::{ AsObject, Context, Py, PyObject, PyObjectRef, PyPayload, PyRef, PyResult, TryFromObject, @@ -378,13 +378,6 @@ impl PySetInner { Ok(true) } - fn iter(&self) -> PySetIterator { - PySetIterator { - size: self.content.size(), - internal: PyMutex::new(PositionIterInternal::new(self.content.clone(), 0)), - } - } - fn repr(&self, class_name: Option<&str>, vm: &VirtualMachine) -> PyResult { let empty = format!("{}()", class_name.unwrap_or("set")); collection_repr(class_name, "{", "}", &empty, self.elements().iter(), vm) @@ -933,7 +926,10 @@ impl Comparable for PySet { impl Iterable for PySet { fn iter(zelf: PyRef, vm: &VirtualMachine) -> PyResult { - Ok(zelf.inner.iter().into_pyobject(vm)) + Ok(PySetIterator::new(AnySet { + object: zelf.into(), + }) + .into_pyobject(vm)) } } @@ -1351,7 +1347,10 @@ impl Comparable for PyFrozenSet { impl Iterable for PyFrozenSet { fn iter(zelf: PyRef, vm: &VirtualMachine) -> PyResult { - Ok(zelf.inner.iter().into_pyobject(vm)) + Ok(PySetIterator::new(AnySet { + object: zelf.into(), + }) + .into_pyobject(vm)) } } @@ -1487,7 +1486,7 @@ impl TryFromObject for AnySet { #[pyclass(module = false, name = "set_iterator")] pub(crate) struct PySetIterator { size: DictSize, - internal: PyMutex>>, + internal: PyMutex>, } impl fmt::Debug for PySetIterator { @@ -1504,6 +1503,15 @@ impl PyPayload for PySetIterator { } } +impl PySetIterator { + fn new(set: AnySet) -> Self { + Self { + size: set.as_inner().content.size(), + internal: PyMutex::new(PositionIterInternal::new(set, 0)), + } + } +} + #[pyclass(flags(DISALLOW_INSTANTIATION), with(IterNext, Iterable))] impl PySetIterator { #[pymethod] @@ -1519,9 +1527,13 @@ impl PySetIterator { (vm.ctx .new_list(match &internal.status { IterStatus::Exhausted => vec![], - IterStatus::Active(dict) => { - dict.keys().into_iter().skip(internal.position).collect() - } + IterStatus::Active(set) => set + .as_inner() + .content + .keys() + .into_iter() + .skip(internal.position) + .collect(), }) .into(),), ) @@ -1531,26 +1543,27 @@ impl PySetIterator { impl SelfIter for PySetIterator {} impl IterNext for PySetIterator { fn next(zelf: &crate::Py, vm: &VirtualMachine) -> PyResult { - let mut internal = zelf.internal.lock(); - let next = if let IterStatus::Active(dict) = &internal.status { - match dict.next_entry_checked(internal.position, &zelf.size, |key, ()| key.clone()) { - Err(crate::dict_inner::DictChanged) => { - internal.status = IterStatus::Exhausted; - return Err(vm.new_runtime_error("set changed size during iteration")); - } + locked_step(&zelf.internal, |internal| { + let IterStatus::Active(set) = &internal.status else { + return (Ok(PyIterReturn::StopIteration(None)), None); + }; + let entry = set.as_inner().content.next_entry_checked( + internal.position, + &zelf.size, + |key, ()| key.clone(), + ); + match entry { + Err(crate::dict_inner::DictChanged) => ( + Err(vm.new_runtime_error("set changed size during iteration")), + internal.exhaust(), + ), Ok(Some((position, key))) => { internal.position = position; - PyIterReturn::Return(key) - } - Ok(None) => { - internal.status = IterStatus::Exhausted; - PyIterReturn::StopIteration(None) + (Ok(PyIterReturn::Return(key)), None) } + Ok(None) => (Ok(PyIterReturn::StopIteration(None)), internal.exhaust()), } - } else { - PyIterReturn::StopIteration(None) - }; - Ok(next) + }) } } From 1c7899db19af35a2977ba72fc6a9ba2ee9113f5c Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Tue, 18 Aug 2026 23:02:20 +0900 Subject: [PATCH 15/27] Release an exhausted iterator's container after the lock `PositionIterInternal::_next` overwrote its `IterStatus::Active` while the caller still held the mutex around it. Dropping the container there ran any `__del__` under that lock, and a `__del__` that iterated the same object again blocked on it. `exhaust()` now hands the container back instead of dropping it, and `locked_step()` releases it after the guard. list, list_reverseiterator, tuple, str, dict and its views and reverse views, bytes, bytearray, memoryview, array, deque, and the enumerate and sequence iterators all go through it. Assisted-by: Claude --- crates/stdlib/src/array.rs | 4 +- crates/vm/src/builtins/bytearray.rs | 4 +- crates/vm/src/builtins/bytes.rs | 4 +- crates/vm/src/builtins/dict.rs | 83 +++++++++++------------- crates/vm/src/builtins/enumerate.rs | 8 +-- crates/vm/src/builtins/iter.rs | 94 ++++++++++++++++++++++------ crates/vm/src/builtins/list.rs | 25 ++++---- crates/vm/src/builtins/memory.rs | 3 +- crates/vm/src/builtins/str.rs | 11 ++-- crates/vm/src/builtins/tuple.rs | 25 ++++---- crates/vm/src/stdlib/_collections.rs | 5 +- 11 files changed, 156 insertions(+), 110 deletions(-) diff --git a/crates/stdlib/src/array.rs b/crates/stdlib/src/array.rs index 7ecd8f4fd9f..22eb4837d48 100644 --- a/crates/stdlib/src/array.rs +++ b/crates/stdlib/src/array.rs @@ -19,7 +19,7 @@ pub mod array { builtins::{ PositionIterInternal, PyByteArray, PyBytes, PyBytesRef, PyDictRef, PyFloat, PyGenericAlias, PyInt, PyList, PyListRef, PyStr, PyStrRef, PyTupleRef, PyType, - PyTypeRef, PyUtf8StrRef, builtins_iter, + PyTypeRef, PyUtf8StrRef, builtins_iter, locked_next, }, class_or_notimplemented, convert::{ToPyObject, ToPyResult, TryFromBorrowedObject, TryFromObject}, @@ -1517,7 +1517,7 @@ pub mod array { impl IterNext for PyArrayIter { fn next(zelf: &Py, vm: &VirtualMachine) -> PyResult { - zelf.internal.lock().next(|array, pos| { + locked_next(&zelf.internal, |array, pos| { let value = array.read().get(pos, vm); Ok(if let Some(item) = value { PyIterReturn::Return(item?) diff --git a/crates/vm/src/builtins/bytearray.rs b/crates/vm/src/builtins/bytearray.rs index 24cad2950c8..b0222996072 100644 --- a/crates/vm/src/builtins/bytearray.rs +++ b/crates/vm/src/builtins/bytearray.rs @@ -1,7 +1,7 @@ //! Implementation of the python bytearray object. use super::{ PositionIterInternal, PyBytes, PyDictRef, PyGenericAlias, PyStrRef, PyTuple, PyTupleRef, - PyType, PyTypeRef, iter::builtins_iter, + PyType, PyTypeRef, iter::builtins_iter, locked_next, }; use crate::{ AsObject, Context, Py, PyObject, PyObjectRef, PyPayload, PyRef, PyResult, TryFromObject, @@ -936,7 +936,7 @@ impl PyByteArrayIterator { impl SelfIter for PyByteArrayIterator {} impl IterNext for PyByteArrayIterator { fn next(zelf: &Py, vm: &VirtualMachine) -> PyResult { - zelf.internal.lock().next(|bytearray, pos| { + locked_next(&zelf.internal, |bytearray, pos| { let buf = bytearray.borrow_buf(); Ok(PyIterReturn::from_result( buf.get(pos).map(|&x| vm.new_pyobj(x)).ok_or(None), diff --git a/crates/vm/src/builtins/bytes.rs b/crates/vm/src/builtins/bytes.rs index 79130a28c44..e611e1929f0 100644 --- a/crates/vm/src/builtins/bytes.rs +++ b/crates/vm/src/builtins/bytes.rs @@ -1,6 +1,6 @@ use super::{ PositionIterInternal, PyDictRef, PyGenericAlias, PyStrRef, PyTuple, PyTupleRef, PyType, - PyTypeRef, iter::builtins_iter, + PyTypeRef, iter::builtins_iter, locked_next, }; use crate::common::lock::LazyLock; use crate::{ @@ -797,7 +797,7 @@ impl PyBytesIterator { impl SelfIter for PyBytesIterator {} impl IterNext for PyBytesIterator { fn next(zelf: &Py, vm: &VirtualMachine) -> PyResult { - zelf.internal.lock().next(|bytes, pos| { + locked_next(&zelf.internal, |bytes, pos| { Ok(PyIterReturn::from_result( bytes .as_bytes() diff --git a/crates/vm/src/builtins/dict.rs b/crates/vm/src/builtins/dict.rs index d2b9dea31fa..193087a1461 100644 --- a/crates/vm/src/builtins/dict.rs +++ b/crates/vm/src/builtins/dict.rs @@ -1,6 +1,6 @@ use super::{ IterStatus, PositionIterInternal, PyBaseExceptionRef, PyGenericAlias, PyMappingProxy, PySet, - PyStr, PyStrRef, PyTupleRef, PyType, PyTypeRef, set, set::PySetInner, + PyStr, PyStrRef, PyTupleRef, PyType, PyTypeRef, locked_step, set, set::PySetInner, }; use crate::common::lock::LazyLock; use crate::object::{Traverse, TraverseFn}; @@ -1214,32 +1214,25 @@ macro_rules! dict_view { impl IterNext for $iter_name { fn next(zelf: &Py, vm: &VirtualMachine) -> PyResult { - let mut internal = zelf.internal.lock(); - let next = if let IterStatus::Active(dict) = &internal.status { - match dict.entries.next_entry_checked( - internal.position, - &zelf.size, - $project_fn, - ) { - Err(dict_inner::DictChanged) => { - internal.status = IterStatus::Exhausted; - return Err( - vm.new_runtime_error("dictionary changed size during iteration") - ); - } + locked_step(&zelf.internal, |internal| { + let IterStatus::Active(dict) = &internal.status else { + return (Ok(PyIterReturn::StopIteration(None)), None); + }; + let entry = + dict.entries + .next_entry_checked(internal.position, &zelf.size, $project_fn); + match entry { + Err(dict_inner::DictChanged) => ( + Err(vm.new_runtime_error("dictionary changed size during iteration")), + internal.exhaust(), + ), Ok(Some((position, item))) => { internal.position = position; - PyIterReturn::Return(($result_fn)(vm, item)) - } - Ok(None) => { - internal.status = IterStatus::Exhausted; - PyIterReturn::StopIteration(None) + (Ok(PyIterReturn::Return(($result_fn)(vm, item))), None) } + Ok(None) => (Ok(PyIterReturn::StopIteration(None)), internal.exhaust()), } - } else { - PyIterReturn::StopIteration(None) - }; - Ok(next) + }) } } @@ -1304,36 +1297,30 @@ macro_rules! dict_view { impl IterNext for $reverse_iter_name { fn next(zelf: &Py, vm: &VirtualMachine) -> PyResult { - let mut internal = zelf.internal.lock(); - let next = if let IterStatus::Active(dict) = &internal.status { - match dict.entries.prev_entry_checked( - internal.position, - &zelf.size, - $project_fn, - ) { - Err(dict_inner::DictChanged) => { - internal.status = IterStatus::Exhausted; - return Err( - vm.new_runtime_error("dictionary changed size during iteration") - ); - } + locked_step(&zelf.internal, |internal| { + let IterStatus::Active(dict) = &internal.status else { + return (Ok(PyIterReturn::StopIteration(None)), None); + }; + let entry = + dict.entries + .prev_entry_checked(internal.position, &zelf.size, $project_fn); + match entry { + Err(dict_inner::DictChanged) => ( + Err(vm.new_runtime_error("dictionary changed size during iteration")), + internal.exhaust(), + ), Ok(Some((found_index, item))) => { - if found_index == 0 { - internal.status = IterStatus::Exhausted; + let released = if found_index == 0 { + internal.exhaust() } else { internal.position = found_index - 1; - } - PyIterReturn::Return(($result_fn)(vm, item)) - } - Ok(None) => { - internal.status = IterStatus::Exhausted; - PyIterReturn::StopIteration(None) + None + }; + (Ok(PyIterReturn::Return(($result_fn)(vm, item))), released) } + Ok(None) => (Ok(PyIterReturn::StopIteration(None)), internal.exhaust()), } - } else { - PyIterReturn::StopIteration(None) - }; - Ok(next) + }) } } }; diff --git a/crates/vm/src/builtins/enumerate.rs b/crates/vm/src/builtins/enumerate.rs index 95e144dad21..dac19dd89cc 100644 --- a/crates/vm/src/builtins/enumerate.rs +++ b/crates/vm/src/builtins/enumerate.rs @@ -1,6 +1,6 @@ use super::{ IterStatus, PositionIterInternal, PyGenericAlias, PyIntRef, PyTupleRef, PyType, PyTypeRef, - iter::builtins_reversed, + iter::builtins_reversed, locked_rev_next, }; use crate::common::lock::{PyMutex, PyRwLock}; use crate::{ @@ -142,9 +142,9 @@ impl PyReverseSequenceIterator { impl SelfIter for PyReverseSequenceIterator {} impl IterNext for PyReverseSequenceIterator { fn next(zelf: &Py, vm: &VirtualMachine) -> PyResult { - zelf.internal - .lock() - .rev_next(|obj, pos| PyIterReturn::from_getitem_result(obj.get_item(&pos, vm), vm)) + locked_rev_next(&zelf.internal, |obj, pos| { + PyIterReturn::from_getitem_result(obj.get_item(&pos, vm), vm) + }) } } diff --git a/crates/vm/src/builtins/iter.rs b/crates/vm/src/builtins/iter.rs index 4e29df583a8..5fb6391fde7 100644 --- a/crates/vm/src/builtins/iter.rs +++ b/crates/vm/src/builtins/iter.rs @@ -91,41 +91,62 @@ impl PositionIterInternal { } } - fn _next(&mut self, f: F, op: OP) -> PyResult + /// `op` answers whether the step it took left this exhausted. + fn _next(&mut self, f: F, op: OP) -> (PyResult, Option) where F: FnOnce(&T, usize) -> PyResult, - OP: FnOnce(&mut Self), + OP: FnOnce(&mut Self) -> bool, { - if let IterStatus::Active(obj) = &self.status { - let ret = f(obj, self.position); - if let Ok(PyIterReturn::Return(_)) = ret { - op(self); - } else { - self.status = IterStatus::Exhausted; - } - ret + let IterStatus::Active(obj) = &self.status else { + return (Ok(PyIterReturn::StopIteration(None)), None); + }; + let ret = f(obj, self.position); + let done = if let Ok(PyIterReturn::Return(_)) = ret { + op(self) } else { - Ok(PyIterReturn::StopIteration(None)) + true + }; + let released = if done { self.exhaust() } else { None }; + (ret, released) + } + + /// Mark this exhausted and hand back what it was holding, for the caller to + /// release once it has dropped the lock guarding this. Releasing it under + /// that lock would let a `__del__` that iterates again deadlock. + #[must_use] + pub fn exhaust(&mut self) -> Option { + match core::mem::replace(&mut self.status, IterStatus::Exhausted) { + IterStatus::Active(obj) => Some(obj), + IterStatus::Exhausted => None, } } - pub fn next(&mut self, f: F) -> PyResult + /// Advance, along with what this was holding if the step exhausted it. See + /// [`Self::exhaust`] for why the caller is handed it rather than the drop + /// happening here; [`locked_next`] does the release for the common case. + #[must_use = "what this hands back is released after the lock, not here"] + pub fn next(&mut self, f: F) -> (PyResult, Option) where F: FnOnce(&T, usize) -> PyResult, { - self._next(f, |zelf| zelf.position += 1) + self._next(f, |zelf| { + zelf.position += 1; + false + }) } - pub fn rev_next(&mut self, f: F) -> PyResult + /// [`Self::next`] walking backwards, exhausted once it steps off the front. + #[must_use = "what this hands back is released after the lock, not here"] + pub fn rev_next(&mut self, f: F) -> (PyResult, Option) where F: FnOnce(&T, usize) -> PyResult, { self._next(f, |zelf| { if zelf.position == 0 { - zelf.status = IterStatus::Exhausted; - } else { - zelf.position -= 1; + return true; } + zelf.position -= 1; + false }) } @@ -153,6 +174,43 @@ impl PositionIterInternal { } } +/// Take `step` under the lock `internal` holds, releasing whatever the step +/// hands back only after that lock is gone. `setiter_iternext()` puts its +/// `Py_DECREF(so)` past `Py_END_CRITICAL_SECTION()` for the same reason: a +/// `__del__` that iterates again would otherwise wait on a lock still held here. +pub(crate) fn locked_step( + internal: &PyMutex>, + step: impl FnOnce(&mut PositionIterInternal) -> (PyResult, Option), +) -> PyResult { + let mut guard = internal.lock(); + let (ret, released) = step(&mut guard); + drop(guard); + drop(released); + ret +} + +/// [`PositionIterInternal::next`] with the release [`locked_step`] describes. +pub fn locked_next( + internal: &PyMutex>, + f: F, +) -> PyResult +where + F: FnOnce(&T, usize) -> PyResult, +{ + locked_step(internal, |internal| internal.next(f)) +} + +/// [`locked_next`] walking backwards. +pub fn locked_rev_next( + internal: &PyMutex>, + f: F, +) -> PyResult +where + F: FnOnce(&T, usize) -> PyResult, +{ + locked_step(internal, |internal| internal.rev_next(f)) +} + pub fn builtins_iter(vm: &VirtualMachine) -> PyObjectRef { vm.builtins.get_attr("iter", vm).unwrap() } @@ -227,7 +285,7 @@ impl PySequenceIterator { impl SelfIter for PySequenceIterator {} impl IterNext for PySequenceIterator { fn next(zelf: &Py, vm: &VirtualMachine) -> PyResult { - zelf.internal.lock().next(|obj, pos| { + locked_next(&zelf.internal, |obj, pos| { let seq = obj.sequence_unchecked(); PyIterReturn::from_getitem_result(seq.get_item(pos as isize, vm), vm) }) diff --git a/crates/vm/src/builtins/list.rs b/crates/vm/src/builtins/list.rs index 02a6d8327e2..3ba537a81e0 100644 --- a/crates/vm/src/builtins/list.rs +++ b/crates/vm/src/builtins/list.rs @@ -1,6 +1,7 @@ use super::{ PositionIterInternal, PyGenericAlias, PyTupleRef, PyType, PyTypeRef, iter::{builtins_iter, builtins_reversed}, + locked_next, locked_rev_next, }; use crate::atomic_func; use crate::common::lock::{ @@ -911,24 +912,22 @@ impl PyListIterator { impl PyListIterator { /// Fast path for FOR_ITER specialization. pub(crate) fn fast_next(&self) -> Option { - self.internal - .lock() - .next(|list, pos| { - let vec = list.borrow_vec(); - Ok(PyIterReturn::from_result(vec.get(pos).cloned().ok_or(None))) - }) - .ok() - .and_then(|r| match r { - PyIterReturn::Return(v) => Some(v), - PyIterReturn::StopIteration(_) => None, - }) + locked_next(&self.internal, |list, pos| { + let vec = list.borrow_vec(); + Ok(PyIterReturn::from_result(vec.get(pos).cloned().ok_or(None))) + }) + .ok() + .and_then(|r| match r { + PyIterReturn::Return(v) => Some(v), + PyIterReturn::StopIteration(_) => None, + }) } } impl SelfIter for PyListIterator {} impl IterNext for PyListIterator { fn next(zelf: &Py, _vm: &VirtualMachine) -> PyResult { - zelf.internal.lock().next(|list, pos| { + locked_next(&zelf.internal, |list, pos| { let vec = list.borrow_vec(); Ok(PyIterReturn::from_result(vec.get(pos).cloned().ok_or(None))) }) @@ -977,7 +976,7 @@ impl PyListReverseIterator { impl SelfIter for PyListReverseIterator {} impl IterNext for PyListReverseIterator { fn next(zelf: &Py, _vm: &VirtualMachine) -> PyResult { - zelf.internal.lock().rev_next(|list, pos| { + locked_rev_next(&zelf.internal, |list, pos| { let vec = list.borrow_vec(); Ok(PyIterReturn::from_result(vec.get(pos).cloned().ok_or(None))) }) diff --git a/crates/vm/src/builtins/memory.rs b/crates/vm/src/builtins/memory.rs index 30ddd28222b..86396825b0c 100644 --- a/crates/vm/src/builtins/memory.rs +++ b/crates/vm/src/builtins/memory.rs @@ -1,6 +1,7 @@ use super::{ PositionIterInternal, PyBytes, PyBytesRef, PyGenericAlias, PyInt, PyListRef, PySlice, PyStr, PyStrRef, PyTuple, PyTupleRef, PyType, PyTypeRef, PyUtf8StrRef, iter::builtins_iter, + locked_next, }; use crate::common::lock::LazyLock; use crate::{ @@ -1691,7 +1692,7 @@ impl PyMemoryViewIterator { impl SelfIter for PyMemoryViewIterator {} impl IterNext for PyMemoryViewIterator { fn next(zelf: &Py, vm: &VirtualMachine) -> PyResult { - zelf.internal.lock().next(|mv, pos| { + locked_next(&zelf.internal, |mv, pos| { let len = mv.__len__(vm)?; Ok(if pos >= len { PyIterReturn::StopIteration(None) diff --git a/crates/vm/src/builtins/str.rs b/crates/vm/src/builtins/str.rs index 6e774f7e652..9bf43e35132 100644 --- a/crates/vm/src/builtins/str.rs +++ b/crates/vm/src/builtins/str.rs @@ -1,10 +1,7 @@ use super::{ PositionIterInternal, PyBytesRef, PyDict, PyTupleRef, PyType, PyTypeRef, int::{PyInt, PyIntRef}, - iter::{ - IterStatus::{self, Exhausted}, - builtins_iter, - }, + iter::{IterStatus, builtins_iter}, }; use crate::{ AsObject, Context, Py, PyExact, PyObject, PyObjectRef, PyPayload, PyRef, PyRefExact, PyResult, @@ -379,7 +376,11 @@ impl IterNext for PyStrIterator { internal.1 += ch.len_wtf8(); return Ok(PyIterReturn::Return(ch.to_pyobject(vm))); } - internal.0.status = Exhausted; + let released = internal.0.exhaust(); + // The string is released after the lock. A `__del__` that iterates + // again would otherwise reach for a lock this call still holds. + drop(internal); + drop(released); } Ok(PyIterReturn::StopIteration(None)) } diff --git a/crates/vm/src/builtins/tuple.rs b/crates/vm/src/builtins/tuple.rs index d510e35326f..3bd53094e8c 100644 --- a/crates/vm/src/builtins/tuple.rs +++ b/crates/vm/src/builtins/tuple.rs @@ -1,5 +1,6 @@ use super::{ PositionIterInternal, PyGenericAlias, PyStrRef, PyType, PyTypeRef, iter::builtins_iter, + locked_next, }; use crate::common::lock::LazyLock; use crate::common::{hash, hash::PyHash, lock::PyMutex, wtf8::wtf8_concat}; @@ -701,25 +702,23 @@ impl PyTupleIterator { impl PyTupleIterator { /// Fast path for FOR_ITER specialization. pub(crate) fn fast_next(&self) -> Option { - self.internal - .lock() - .next(|tuple, pos| { - Ok(PyIterReturn::from_result( - tuple.get(pos).cloned().ok_or(None), - )) - }) - .ok() - .and_then(|r| match r { - PyIterReturn::Return(v) => Some(v), - PyIterReturn::StopIteration(_) => None, - }) + locked_next(&self.internal, |tuple, pos| { + Ok(PyIterReturn::from_result( + tuple.get(pos).cloned().ok_or(None), + )) + }) + .ok() + .and_then(|r| match r { + PyIterReturn::Return(v) => Some(v), + PyIterReturn::StopIteration(_) => None, + }) } } impl SelfIter for PyTupleIterator {} impl IterNext for PyTupleIterator { fn next(zelf: &Py, _vm: &VirtualMachine) -> PyResult { - zelf.internal.lock().next(|tuple, pos| { + locked_next(&zelf.internal, |tuple, pos| { Ok(PyIterReturn::from_result( tuple.get(pos).cloned().ok_or(None), )) diff --git a/crates/vm/src/stdlib/_collections.rs b/crates/vm/src/stdlib/_collections.rs index b48c0e670ac..4bec3a03a96 100644 --- a/crates/vm/src/stdlib/_collections.rs +++ b/crates/vm/src/stdlib/_collections.rs @@ -8,6 +8,7 @@ mod _collections { builtins::{ IterStatus::{Active, Exhausted}, PositionIterInternal, PyDict, PyGenericAlias, PyInt, PyStr, PyType, PyTypeRef, + locked_next, }, common::lock::{PyMutex, PyRwLock, PyRwLockReadGuard, PyRwLockWriteGuard}, convert::ToPyObject, @@ -695,7 +696,7 @@ mod _collections { impl SelfIter for PyDequeIterator {} impl IterNext for PyDequeIterator { fn next(zelf: &Py, vm: &VirtualMachine) -> PyResult { - zelf.internal.lock().next(|deque, pos| { + locked_next(&zelf.internal, |deque, pos| { if zelf.state != deque.state.load() { return Err(vm.new_runtime_error("Deque mutated during iteration")); } @@ -761,7 +762,7 @@ mod _collections { impl IterNext for PyReverseDequeIterator { fn next(zelf: &Py, vm: &VirtualMachine) -> PyResult { - zelf.internal.lock().next(|deque, pos| { + locked_next(&zelf.internal, |deque, pos| { if deque.state.load() != zelf.state { return Err(vm.new_runtime_error("Deque mutated during iteration")); } From 800c2d8c175f4f9c6a4c3a37a6bde12418b13263 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Tue, 18 Aug 2026 23:02:23 +0900 Subject: [PATCH 16/27] Unskip test_free_after_iterating Assisted-by: Claude --- Lib/test/seq_tests.py | 1 - Lib/test/test_array.py | 1 - Lib/test/test_bytes.py | 1 - Lib/test/test_dict.py | 1 - Lib/test/test_iter.py | 1 - Lib/test/test_set.py | 1 - Lib/test/test_str.py | 1 - 7 files changed, 7 deletions(-) diff --git a/Lib/test/seq_tests.py b/Lib/test/seq_tests.py index e8834c2bafc..b7875fe8f2f 100644 --- a/Lib/test/seq_tests.py +++ b/Lib/test/seq_tests.py @@ -439,7 +439,6 @@ def test_pickle(self): self.assertEqual(lst2, lst) self.assertNotEqual(id(lst2), id(lst)) - @unittest.skip("TODO: RUSTPYTHON; hangs") def test_free_after_iterating(self): support.check_free_after_iterating(self, iter, self.type2test) support.check_free_after_iterating(self, reversed, self.type2test) diff --git a/Lib/test/test_array.py b/Lib/test/test_array.py index 13df6134882..ae6fec1210e 100644 --- a/Lib/test/test_array.py +++ b/Lib/test/test_array.py @@ -1198,7 +1198,6 @@ def test_obsolete_write_lock(self): a = array.array('B', b"") self.assertRaises(BufferError, _testcapi.getbuffer_with_null_view, a) - @unittest.skip("TODO: RUSTPYTHON; hangs") def test_free_after_iterating(self): support.check_free_after_iterating(self, iter, array.array, (self.typecode,)) diff --git a/Lib/test/test_bytes.py b/Lib/test/test_bytes.py index 32a9ca7df87..16099ceb665 100644 --- a/Lib/test/test_bytes.py +++ b/Lib/test/test_bytes.py @@ -1041,7 +1041,6 @@ def test_find_etc_raise_correct_error_messages(self): self.assertRaisesRegex(TypeError, r'\bendswith\b', b.endswith, x, None, None, None) - @unittest.skip("TODO: RUSTPYTHON; hangs") def test_free_after_iterating(self): test.support.check_free_after_iterating(self, iter, self.type2test) test.support.check_free_after_iterating(self, reversed, self.type2test) diff --git a/Lib/test/test_dict.py b/Lib/test/test_dict.py index e2a73773cc2..046146dbfa6 100644 --- a/Lib/test/test_dict.py +++ b/Lib/test/test_dict.py @@ -1258,7 +1258,6 @@ def __eq__(self, o): d = {X(): 0, 1: 1} self.assertRaises(RuntimeError, d.update, other) - @unittest.skip("TODO: RUSTPYTHON; hangs") def test_free_after_iterating(self): support.check_free_after_iterating(self, iter, dict) support.check_free_after_iterating(self, lambda d: iter(d.keys()), dict) diff --git a/Lib/test/test_iter.py b/Lib/test/test_iter.py index 7ac48a50233..18e4b676c53 100644 --- a/Lib/test/test_iter.py +++ b/Lib/test/test_iter.py @@ -1137,7 +1137,6 @@ def test_iter_neg_setstate(self): self.assertEqual(next(it), 0) self.assertEqual(next(it), 1) - @unittest.skip("TODO: RUSTPYTHON; hangs") def test_free_after_iterating(self): check_free_after_iterating(self, iter, SequenceClass, (0,)) diff --git a/Lib/test/test_set.py b/Lib/test/test_set.py index 42f11c9eb28..40997a34e15 100644 --- a/Lib/test/test_set.py +++ b/Lib/test/test_set.py @@ -362,7 +362,6 @@ class C(object): gc.collect() self.assertTrue(ref() is None, "Cycle was not collected") - @unittest.skipIf("RUSTPYTHON_SKIP_ENV_POLLUTERS" in __import__("os").environ, "TODO: RUSTPYTHON") def test_free_after_iterating(self): support.check_free_after_iterating(self, iter, self.thetype) diff --git a/Lib/test/test_str.py b/Lib/test/test_str.py index 2a3c36f2e57..4869c5ca9b0 100644 --- a/Lib/test/test_str.py +++ b/Lib/test/test_str.py @@ -2606,7 +2606,6 @@ def test_compare(self): self.assertTrue(astral >= bmp2) self.assertFalse(astral >= astral2) - @unittest.skip("TODO: RUSTPYTHON; hangs") def test_free_after_iterating(self): support.check_free_after_iterating(self, iter, str) if not support.Py_GIL_DISABLED: From da37fb834e0e07b1c81b9950c94bd8dc96b2664a Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Wed, 19 Aug 2026 01:43:02 +0900 Subject: [PATCH 17/27] Stop listing test_set as an environment polluter `check_free_after_iterating` no longer leaves an ignored exception behind, and the job that reruns the listed tests ten times fails once one of them stops polluting. Assisted-by: Claude --- .github/workflows/ci.yaml | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 5a8daae06ef..3249aab33df 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -302,8 +302,7 @@ jobs: - '-u all' - '--timeout 600' - '--dont-add-python-opts' - env_polluting_tests: - - test_set + env_polluting_tests: [] skips: [] timeout: 50 - os: ubuntu-latest @@ -311,8 +310,7 @@ jobs: - '-u all' - '--timeout 600' - '--dont-add-python-opts' - env_polluting_tests: - - test_set + env_polluting_tests: [] skips: [] timeout: 60 - os: windows-2025 @@ -320,8 +318,7 @@ jobs: - '-u all' - '--timeout 600' - '--dont-add-python-opts' - env_polluting_tests: - - test_set + env_polluting_tests: [] skips: [] timeout: 50 fail-fast: false From c0faf2d8eaaacb0078cd961857dcc69cb09da69c Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Wed, 19 Aug 2026 01:43:02 +0900 Subject: [PATCH 18/27] Drop winsound imports left unused `TryFromBorrowedObject`, `exceptions`, and `ToWideString` have no reference in the module, which fails the Windows clippy line under `-Dwarnings`. Assisted-by: Claude --- crates/vm/src/stdlib/winsound.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/vm/src/stdlib/winsound.rs b/crates/vm/src/stdlib/winsound.rs index 75f576adf81..fbefa236c6a 100644 --- a/crates/vm/src/stdlib/winsound.rs +++ b/crates/vm/src/stdlib/winsound.rs @@ -6,9 +6,7 @@ pub(crate) use winsound::module_def; #[pymodule] mod winsound { use crate::builtins::{PyBaseExceptionRef, PyBytes, PyStr}; - use crate::convert::{IntoPyException, ToPyException, TryFromBorrowedObject}; - use crate::exceptions; - use crate::host_env::windows::ToWideString; + use crate::convert::{IntoPyException, ToPyException}; use crate::protocol::{BufferFlags, PyBuffer}; use crate::{AsObject, PyObjectRef, PyResult, VirtualMachine}; use rustpython_host_env::winsound::{PlaySoundError, PlaySoundSource, play_sound}; From b2ad9b17c481b41db5f9faf0ce4dd09adba9ab7f Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Wed, 19 Aug 2026 08:20:31 +0900 Subject: [PATCH 19/27] Ask nothing where the caller takes no room `map_py_iter` asked the iterator for a length hint on behalf of every caller that does not reserve, and reported what asking raised. Those callers ask nothing at all: `tuple()`, `f(*x)`, `bytearray(x)`, `min()`, `max()` and `collections.deque()` answer for an iterator whose `__length_hint__` raises, where they had been raising it. The answer was also never spent. It reached `PyIterIter` for a `size_hint()` the push loop does not read, so the lookup and any call it made were work thrown away: `tuple()` over a generator drops 22% of its instructions, and 17% over an iterator with a `__length_hint__` written in Python. Assisted-by: Claude --- crates/vm/src/vm/mod.rs | 48 +++++++++++++++------------- extra_tests/snippets/builtin_list.py | 38 ++++++++++++++++++++-- 2 files changed, 61 insertions(+), 25 deletions(-) diff --git a/crates/vm/src/vm/mod.rs b/crates/vm/src/vm/mod.rs index 20e516d2aaf..a26ab3761ce 100644 --- a/crates/vm/src/vm/mod.rs +++ b/crates/vm/src/vm/mod.rs @@ -879,12 +879,14 @@ struct SuspendedFrame { is_entry: bool, } -/// Which object a sequence being built asks how much room to take, and how the -/// answer is used. `PySequence_Tuple()` asks the iterator and takes nothing on -/// the answer; `list_extend()` asks the iterable it was handed and reserves. +/// Whether a sequence being built asks the iterable it was handed how much room +/// to take. `list_extend()` asks and reserves; `PySequence_Tuple()` and the +/// rest ask nothing at all. #[derive(Clone, Copy)] enum LengthHint<'a> { - Iterator, + /// Grows as the loop goes, the way `tuple()`, `set()`, `min()` and + /// `deque()` do, so an object slow to answer is never asked. + Unasked, /// Reserves what the iterable answers, unless it leaves no room for the /// count this returns. Iterable(&'a dyn Fn() -> usize), @@ -2640,7 +2642,7 @@ impl VirtualMachine { where F: Fn(PyObjectRef) -> PyResult, { - self.extract_elements_inner(value, LengthHint::Iterator, func) + self.extract_elements_inner(value, LengthHint::Unasked, func) } /// [`Self::extract_elements_with`] for a caller that asks the iterable @@ -2740,7 +2742,7 @@ impl VirtualMachine { } /// [`Self::map_iterable_object`] for a caller that asks the object it was - /// handed how long it is, rather than its iterator. + /// handed how long it is. pub fn map_iterable_object_sized( &self, obj: &PyObject, @@ -2756,7 +2758,7 @@ impl VirtualMachine { where F: FnMut(PyObjectRef) -> PyResult, { - self.map_iterable_object_inner(obj, LengthHint::Iterator, f) + self.map_iterable_object_inner(obj, LengthHint::Unasked, f) } fn map_iterable_object_inner( @@ -2808,20 +2810,18 @@ impl VirtualMachine { F: FnMut(PyObjectRef) -> PyResult, { let iter = value.to_owned().get_iter(self)?; - // Whichever object the hint is asked of, an error it answers with is - // its own and belongs to the caller; `length_hint_opt` already answers - // `None` for the object that declines to guess. - let asked = match hint { - LengthHint::Iterator => iter.as_object(), - LengthHint::Iterable(_) => value, - }; - let cap = self.length_hint_opt(asked.to_owned())?; // Take the room the iterable asks for up front, for the callers that // do. Collecting into a `Result` drops the iterator's lower bound -- // the adapter may stop early -- so without this the vector grows a step // at a time and an iterable claiming more elements than can be held is - // found out by running out of memory rather than by saying so. + // found out by running out of memory rather than by saying so. An error + // the ask answers with is the iterable's own and belongs to the caller + // that made it; `length_hint_opt` already answers `None` for the + // iterable that declines to guess. + // + // Nobody else asks, so what an object would have answered -- slowly, or + // by raising -- costs the rest nothing. // // A hint that does not leave room for what is already held is one the // iterable cannot be telling the truth about, so it is passed over @@ -2830,12 +2830,16 @@ impl VirtualMachine { // held is counted now rather than before, since asking for the hint // runs code that can add to it or take from it. let mut results: Vec = Vec::new(); - if let (LengthHint::Iterable(held), Some(cap)) = (hint, cap) - && held() <= (isize::MAX as usize) - cap - { - results - .try_reserve_exact(cap) - .map_err(|_| self.new_memory_error(""))?; + let mut cap = None; + if let LengthHint::Iterable(held) = hint { + cap = self.length_hint_opt(value.to_owned())?; + if let Some(cap) = cap + && held() <= (isize::MAX as usize) - cap + { + results + .try_reserve_exact(cap) + .map_err(|_| self.new_memory_error(""))?; + } } for element in PyIterIter::new(self, iter.as_ref(), cap) { results.push(f(element?)?); diff --git a/extra_tests/snippets/builtin_list.py b/extra_tests/snippets/builtin_list.py index decf40ad4ae..d2fb75b3576 100644 --- a/extra_tests/snippets/builtin_list.py +++ b/extra_tests/snippets/builtin_list.py @@ -993,9 +993,8 @@ def __length_hint__(self): shrunk.extend(Shrinks()) -# Only the callers that take the room ask the iterable itself; the rest ask its -# iterator, which is why an iterable whose __len__ raises reaches tuple() but -# not list(). +# Only the callers that take the room ask at all, which is why an iterable whose +# __len__ raises reaches tuple() but not list(). class Lazy: def __len__(self): raise NotImplementedError @@ -1016,6 +1015,39 @@ def __iter__(self): with assert_raises(NotImplementedError): [*Lazy()] + +# Nothing asks the iterator, so what it would have answered never runs. +class LoudIterator: + def __init__(self): + self.i = iter([1, 2, 3]) + + def __iter__(self): + return self + + def __next__(self): + return next(self.i) + + def __length_hint__(self): + raise NotImplementedError + + +class HandsOutLoud: + def __iter__(self): + return LoudIterator() + + +assert tuple(HandsOutLoud()) == (1, 2, 3) +assert (lambda *a: a)(*HandsOutLoud()) == (1, 2, 3) +assert min(HandsOutLoud()) == 1 +assert max(HandsOutLoud()) == 3 +assert list(HandsOutLoud()) == [1, 2, 3] +assert sorted(HandsOutLoud()) == [1, 2, 3] +assert bytearray(HandsOutLoud()) == bytearray(b"\x01\x02\x03") +held = [0] +held.extend(HandsOutLoud()) +assert held == [0, 1, 2, 3] + + # A report the list can act on is acted on. assert list(Reports(3)) == [1, 2, 3] assert list(Reports(0)) == [1, 2, 3] From 3613b897c058f5c40ce74525423dd7e537097f1e Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Wed, 19 Aug 2026 09:56:17 +0900 Subject: [PATCH 20/27] Stop asking an iterator how long it is to walk it `PyIter::iter` and `PyIter::into_iter` asked for a length hint and reported what asking raised. Nothing spent the answer: it reached `PyIterIter` for a `size_hint()` that every caller either loops past or drops, since collecting into a `Result` reports no lower bound. 23 operations answered for an iterator whose `__length_hint__` raises, where they had been raising it: `set`, `frozenset` and the nine `set` methods that take an iterable, `dict.fromkeys` and the dict view operators, `array` and `array.extend`, `all`, `any`, `sum`, `io.writelines`, `math.fsum`, `math.prod`, and `csv.writerow` and `writerows`. Over a generator, `set()` drops 16% of its instructions and `all()` 23%. `str.join` and `bytes.join` do ask, reaching their elements through `PySequence_Fast()`, which fills a list from the iterator. They take `iter_sized()`, which is now the only way to ask. `iter_without_hint` is gone, its callers being what `iter` already does. Assisted-by: Claude --- crates/vm/src/builtins/dict.rs | 4 +- crates/vm/src/builtins/range.rs | 2 +- crates/vm/src/builtins/str.rs | 4 +- crates/vm/src/bytes_inner.rs | 3 +- crates/vm/src/function/protocol.rs | 24 ++++++++--- crates/vm/src/protocol/iter.rs | 26 +++++++----- crates/vm/src/stdlib/_functools.rs | 2 +- crates/vm/src/stdlib/_operator.rs | 4 +- crates/vm/src/stdlib/os.rs | 2 +- extra_tests/snippets/builtin_iter.py | 62 ++++++++++++++++++++++++++++ 10 files changed, 109 insertions(+), 24 deletions(-) diff --git a/crates/vm/src/builtins/dict.rs b/crates/vm/src/builtins/dict.rs index 193087a1461..f56b2e7da23 100644 --- a/crates/vm/src/builtins/dict.rs +++ b/crates/vm/src/builtins/dict.rs @@ -240,7 +240,7 @@ impl PyDict { } })?; elem_iter - .into_iter::(vm)? + .into_iter::(vm) .collect::>>() })() .map_err(|exc| Self::add_update_sequence_note(exc, index, vm))?; @@ -257,7 +257,7 @@ impl PyDict { let iter = seq2.get_iter(vm)?; let dict = &self.entries; - for (index, element) in iter.iter_without_hint::(vm)?.enumerate() { + for (index, element) in iter.iter::(vm)?.enumerate() { let (key, value) = Self::update_sequence_pair(element?, index, vm)?; if !override_existing && dict.contains(vm, &*key)? { diff --git a/crates/vm/src/builtins/range.rs b/crates/vm/src/builtins/range.rs index 5962f90e521..928c67884a4 100644 --- a/crates/vm/src/builtins/range.rs +++ b/crates/vm/src/builtins/range.rs @@ -39,7 +39,7 @@ fn iter_search( ) -> PyResult { let mut count = 0; let iter = obj.get_iter(vm)?; - for element in iter.iter_without_hint::(vm)? { + for element in iter.iter::(vm)? { if vm.bool_eq(item, &*element?)? { match flag { SearchType::Index => return Ok(count), diff --git a/crates/vm/src/builtins/str.rs b/crates/vm/src/builtins/str.rs index 9bf43e35132..dc5a1daf4b9 100644 --- a/crates/vm/src/builtins/str.rs +++ b/crates/vm/src/builtins/str.rs @@ -1174,7 +1174,9 @@ impl PyStr { iterable: ArgIterable, vm: &VirtualMachine, ) -> PyResult { - let iter = iterable.iter(vm)?; + // `PyUnicode_Join()` reaches its elements through `PySequence_Fast()`, + // which fills a list from the iterator and so asks it how long it is. + let iter = iterable.iter_sized(vm)?; let joined = match iter.exactly_one() { Ok(first) => { let first = first?; diff --git a/crates/vm/src/bytes_inner.rs b/crates/vm/src/bytes_inner.rs index 11b3b1e1d96..d87fee4c2a2 100644 --- a/crates/vm/src/bytes_inner.rs +++ b/crates/vm/src/bytes_inner.rs @@ -629,7 +629,8 @@ impl PyBytesInner { } pub fn join(&self, iterable: ArgIterable, vm: &VirtualMachine) -> PyResult> { - let iter = iterable.iter(vm)?; + // `PySequence_Fast()`, as in `PyUnicode_Join()`. + let iter = iterable.iter_sized(vm)?; self.elements.py_join(iter) } diff --git a/crates/vm/src/function/protocol.rs b/crates/vm/src/function/protocol.rs index d503fabaca8..5d5e4527f57 100644 --- a/crates/vm/src/function/protocol.rs +++ b/crates/vm/src/function/protocol.rs @@ -91,16 +91,30 @@ impl ArgIterable { &self.iterable } - /// Returns an iterator over this sequence of objects. + /// This object's iterator. /// /// This operation may fail if an exception is raised while invoking the /// `__iter__` method of the iterable object. - pub fn iter<'a>(&self, vm: &'a VirtualMachine) -> PyResult> { - let iter = PyIter::new(match self.iter_fn { + fn get_iter(&self, vm: &VirtualMachine) -> PyResult { + Ok(PyIter::new(match self.iter_fn { Some(f) => f(self.iterable.clone(), vm)?, None => PySequenceIterator::new(self.iterable.clone(), vm)?.into_pyobject(vm), - }); - iter.into_iter(vm) + })) + } + + /// Returns an iterator over this sequence of objects. See [`PyIter::iter`] + /// for why it does not ask how long the iterator is. + /// + /// This operation may fail if an exception is raised while invoking the + /// `__iter__` method of the iterable object. + pub fn iter<'a>(&self, vm: &'a VirtualMachine) -> PyResult> { + Ok(self.get_iter(vm)?.into_iter(vm)) + } + + /// [`Self::iter`] for a caller that fills a sized container from the + /// iterator, the way `PySequence_Fast()` does. + pub fn iter_sized<'a>(&self, vm: &'a VirtualMachine) -> PyResult> { + self.get_iter(vm)?.into_iter_sized(vm) } } diff --git a/crates/vm/src/protocol/iter.rs b/crates/vm/src/protocol/iter.rs index 1aa0bcd5b13..3df72fd80eb 100644 --- a/crates/vm/src/protocol/iter.rs +++ b/crates/vm/src/protocol/iter.rs @@ -56,25 +56,31 @@ where iternext(self.0.borrow(), vm) } + /// Walks the iterator without asking it how long it is. Almost nothing + /// asks: a loop over an iterator takes no room up front, so what the + /// object would have answered -- slowly, or by raising -- never runs. pub fn iter<'a, 'b, U>( &'b self, vm: &'a VirtualMachine, - ) -> PyResult> { - let length_hint = vm.length_hint_opt(self.as_ref().to_owned())?; - Ok(PyIterIter::new(vm, self.0.borrow(), length_hint)) - } - - pub fn iter_without_hint<'a, 'b, U>( - &'b self, - vm: &'a VirtualMachine, ) -> PyResult> { Ok(PyIterIter::new(vm, self.0.borrow(), None)) } } impl PyIter { - /// Returns an iterator over this sequence of objects. - pub fn into_iter(self, vm: &VirtualMachine) -> PyResult> { + /// Returns an iterator over this sequence of objects. See [`Self::iter`] + /// for why it does not ask how long the iterator is. + pub fn into_iter(self, vm: &VirtualMachine) -> PyIterIter<'_, U, PyObjectRef> { + PyIterIter::new(vm, self.0, None) + } + + /// [`Self::into_iter`] for a caller that fills a sized container from the + /// iterator, the way `PySequence_Fast()` does. It asks how much room that + /// takes and answers with whatever asking raised. + pub fn into_iter_sized( + self, + vm: &VirtualMachine, + ) -> PyResult> { let length_hint = vm.length_hint_opt(self.as_object().to_owned())?; Ok(PyIterIter::new(vm, self.0, length_hint)) } diff --git a/crates/vm/src/stdlib/_functools.rs b/crates/vm/src/stdlib/_functools.rs index 944a2e8abdb..2c16632dc95 100644 --- a/crates/vm/src/stdlib/_functools.rs +++ b/crates/vm/src/stdlib/_functools.rs @@ -32,7 +32,7 @@ mod _functools { iterator, initial, } = args; - let mut iter = iterator.iter_without_hint(vm)?; + let mut iter = iterator.iter(vm)?; // OptionalOption distinguishes between: // - Missing: no argument provided → use first element from iterator // - Present(None): explicitly passed None → use None as initial value diff --git a/crates/vm/src/stdlib/_operator.rs b/crates/vm/src/stdlib/_operator.rs index 5e72ef03eb4..aac528c52aa 100644 --- a/crates/vm/src/stdlib/_operator.rs +++ b/crates/vm/src/stdlib/_operator.rs @@ -178,7 +178,7 @@ mod _operator { #[pyfunction(name = "countOf")] fn count_of(a: PyIter, b: PyObjectRef, vm: &VirtualMachine) -> PyResult { let mut count: usize = 0; - for element in a.iter_without_hint::(vm)? { + for element in a.iter::(vm)? { let element = element?; if element.is(&b) || vm.bool_eq(&b, &element)? { count += 1; @@ -199,7 +199,7 @@ mod _operator { #[pyfunction(name = "indexOf")] fn index_of(a: PyIter, b: PyObjectRef, vm: &VirtualMachine) -> PyResult { - for (index, element) in a.iter_without_hint::(vm)?.enumerate() { + for (index, element) in a.iter::(vm)?.enumerate() { let element = element?; if element.is(&b) || vm.bool_eq(&b, &element)? { return Ok(index); diff --git a/crates/vm/src/stdlib/os.rs b/crates/vm/src/stdlib/os.rs index ae39b9c31bc..3e5e2393ee3 100644 --- a/crates/vm/src/stdlib/os.rs +++ b/crates/vm/src/stdlib/os.rs @@ -2135,7 +2135,7 @@ pub(crate) fn envobj_to_dict( } let keys = vm.call_method(obj, "keys", ())?; let dict = vm.ctx.new_dict(); - for key in keys.get_iter(vm)?.into_iter::(vm)? { + for key in keys.get_iter(vm)?.into_iter::(vm) { let key = key?; let val = obj.get_item(&*key, vm)?; dict.set_item(&*key, val, vm)?; diff --git a/extra_tests/snippets/builtin_iter.py b/extra_tests/snippets/builtin_iter.py index 02d469a47ee..6ef0d7f8cde 100644 --- a/extra_tests/snippets/builtin_iter.py +++ b/extra_tests/snippets/builtin_iter.py @@ -69,3 +69,65 @@ def __len__(self): assert seq_it.__length_hint__() == 3 next(seq_it) assert seq_it.__length_hint__() == 2 + + +# Walking an iterator takes no room up front, so nothing on the way asks it how +# long it is. Only join does, reaching its elements through PySequence_Fast(), +# which fills a list from the iterator. +import array +import collections +import io +import math + + +class LoudIterator: + def __init__(self, seq): + self.i = iter(seq) + + def __iter__(self): + return self + + def __next__(self): + return next(self.i) + + def __length_hint__(self): + raise NotImplementedError("iterator hint") + + +def handing(seq=(1, 2, 3)): + class Handing: + def __iter__(self): + return LoudIterator(seq) + + return Handing() + + +assert set(handing()) == {1, 2, 3} +assert frozenset(handing()) == frozenset({1, 2, 3}) +assert {1}.difference(handing()) == set() +assert {1}.intersection(handing()) == {1} +assert {1}.symmetric_difference(handing()) == {2, 3} +assert {1}.issubset(handing()) +assert not {9}.issuperset(handing()) +assert dict.fromkeys(handing()) == {1: None, 2: None, 3: None} +assert array.array("b", handing()) == array.array("b", [1, 2, 3]) +assert all(handing()) and any(handing()) +assert sum(handing()) == 6 +assert math.fsum(handing()) == 6.0 +assert math.prod(handing()) == 6 +assert collections.deque(handing()) == collections.deque([1, 2, 3]) +assert tuple(handing()) == (1, 2, 3) +assert list(handing()) == [1, 2, 3] +assert min(handing()) == 1 +assert bytes(handing()) == b"\x01\x02\x03" +assert bytearray(handing()) == bytearray(b"\x01\x02\x03") +io.StringIO().writelines(handing(("a", "b"))) + +# join asks, and answers with what asking raised. +for empty in ("", b""): + try: + empty.join(handing((empty.__class__(),))) + except NotImplementedError: + pass + else: + raise AssertionError(f"{empty.__class__.__name__}.join did not ask") From 9f3580b0bf1f637c82ebeec6ffab56c4ab2d4f66 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Wed, 19 Aug 2026 12:34:27 +0900 Subject: [PATCH 21/27] Name the matrix jobs rather than let the matrix name them A generated job name lists every value in the matrix entry, so `cargo check` carried the booleans its dependencies and `skip_ssl` keys expand to, and the snippets job carried its test arguments and timeout. Adding or removing a key renames the check, which drops it from the required list until that list is edited to match. Emptying `env_polluting_tests` renamed three checks this way. `Run rust tests` and `clippy` keep the names they had. `cargo check` drops the booleans from six of its nine, and the snippets job drops its arguments and timeout from all three. Assisted-by: Claude --- .github/workflows/ci.yaml | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 3249aab33df..354902f9571 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -80,7 +80,10 @@ jobs: if: ${{ !contains(github.event.pull_request.labels.*.name, 'skip:ci') }} env: RUST_BACKTRACE: full - name: Run rust tests + # Named after the matrix entry rather than left to be named for it: a + # generated name lists every value in the entry, so adding or removing one + # renames the check and drops it from the required list. + name: Run rust tests (${{ matrix.os }}) runs-on: ${{ matrix.os }} timeout-minutes: 45 strategy: @@ -165,7 +168,10 @@ jobs: if: runner.os == 'Linux' cargo_check: - name: cargo check + # Named after the matrix entry rather than left to be named for it: a + # generated name lists every value in the entry, so adding or removing one + # renames the check and drops it from the required list. + name: cargo check (${{ matrix.os }}, ${{ matrix.target }}) runs-on: ${{ matrix.os }} needs: - determine_changes @@ -292,7 +298,10 @@ jobs: test_multiprocessing_fork test_multiprocessing_forkserver test_multiprocessing_spawn - name: Run snippets and cpython tests + # Named after the matrix entry rather than left to be named for it: a + # generated name lists every value in the entry, so adding or removing one + # renames the check and drops it from the required list. + name: Run snippets and cpython tests (${{ matrix.os }}) runs-on: ${{ matrix.os }} strategy: matrix: @@ -462,7 +471,10 @@ jobs: run: python -I scripts/whats_left.py ${{ env.CARGO_ARGS }} --features jit clippy: - name: clippy + # Named after the matrix entry rather than left to be named for it: a + # generated name lists every value in the entry, so adding or removing one + # renames the check and drops it from the required list. + name: clippy (${{ matrix.os }}) runs-on: ${{ matrix.os }} needs: - determine_changes From e07987dcb349d0dffebce86085a6da38ae59d873 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Wed, 19 Aug 2026 15:48:57 +0900 Subject: [PATCH 22/27] Keep a sequence iterator active when an element raises `PositionIterInternal::_next` exhausted the iterator for any non-`Return` result, so an error from `__getitem__` ended the walk. `iter_iternext()` lets go of its sequence for `IndexError` and `StopIteration` alone, and `PyIterReturn::from_getitem_result` has already turned the first of those into the second, so only `StopIteration` exhausts now. Both deque iterators keep exhausting on their own mutation guard, which `deque_iternext()` does by zeroing the counter before it raises; they share one step function for it, and the message it raises is lowercased to match the three other sites in the module. Assisted-by: Claude --- crates/vm/src/builtins/iter.rs | 13 ++++-- crates/vm/src/stdlib/_collections.rs | 68 ++++++++++++++++++++-------- extra_tests/snippets/builtin_iter.py | 22 +++++++++ 3 files changed, 79 insertions(+), 24 deletions(-) diff --git a/crates/vm/src/builtins/iter.rs b/crates/vm/src/builtins/iter.rs index 5fb6391fde7..2d231e9e6a8 100644 --- a/crates/vm/src/builtins/iter.rs +++ b/crates/vm/src/builtins/iter.rs @@ -101,10 +101,15 @@ impl PositionIterInternal { return (Ok(PyIterReturn::StopIteration(None)), None); }; let ret = f(obj, self.position); - let done = if let Ok(PyIterReturn::Return(_)) = ret { - op(self) - } else { - true + let done = match &ret { + Ok(PyIterReturn::Return(_)) => op(self), + Ok(PyIterReturn::StopIteration(_)) => true, + // An error belongs to the element, not to the walk, so the next + // call reaches for the same one again. `iter_iternext()` lets go of + // its sequence for `IndexError` and `StopIteration` alone, and + // `PyIterReturn::from_getitem_result` has already turned the first + // of those into the second. + Err(_) => false, }; let released = if done { self.exhaust() } else { None }; (ret, released) diff --git a/crates/vm/src/stdlib/_collections.rs b/crates/vm/src/stdlib/_collections.rs index 4bec3a03a96..5fc53c92c54 100644 --- a/crates/vm/src/stdlib/_collections.rs +++ b/crates/vm/src/stdlib/_collections.rs @@ -8,7 +8,7 @@ mod _collections { builtins::{ IterStatus::{Active, Exhausted}, PositionIterInternal, PyDict, PyGenericAlias, PyInt, PyStr, PyType, PyTypeRef, - locked_next, + locked_step, }, common::lock::{PyMutex, PyRwLock, PyRwLockReadGuard, PyRwLockWriteGuard}, convert::ToPyObject, @@ -694,16 +694,42 @@ mod _collections { } impl SelfIter for PyDequeIterator {} + + /// One step of either deque iterator, reaching for an element with `at`. + fn deque_step( + internal: &mut PositionIterInternal, + state: usize, + at: impl FnOnce(&VecDeque, usize) -> Option, + vm: &VirtualMachine, + ) -> (PyResult, Option) { + let Active(deque) = &internal.status else { + return (Ok(PyIterReturn::StopIteration(None)), None); + }; + if state != deque.state.load() { + // `deque_iternext()` empties the iterator before it raises, so what + // is left to walk reads as nothing. + return ( + Err(vm.new_runtime_error("deque mutated during iteration")), + internal.exhaust(), + ); + } + let item = at(&deque.borrow_deque(), internal.position); + let Some(item) = item else { + return (Ok(PyIterReturn::StopIteration(None)), internal.exhaust()); + }; + internal.position += 1; + (Ok(PyIterReturn::Return(item)), None) + } + impl IterNext for PyDequeIterator { fn next(zelf: &Py, vm: &VirtualMachine) -> PyResult { - locked_next(&zelf.internal, |deque, pos| { - if zelf.state != deque.state.load() { - return Err(vm.new_runtime_error("Deque mutated during iteration")); - } - let deque = deque.borrow_deque(); - Ok(PyIterReturn::from_result( - deque.get(pos).cloned().ok_or(None), - )) + locked_step(&zelf.internal, |internal| { + deque_step( + internal, + zelf.state, + |deque, pos| deque.get(pos).cloned(), + vm, + ) }) } } @@ -762,17 +788,19 @@ mod _collections { impl IterNext for PyReverseDequeIterator { fn next(zelf: &Py, vm: &VirtualMachine) -> PyResult { - locked_next(&zelf.internal, |deque, pos| { - if deque.state.load() != zelf.state { - return Err(vm.new_runtime_error("Deque mutated during iteration")); - } - let deque = deque.borrow_deque(); - let r = deque - .len() - .checked_sub(pos + 1) - .and_then(|pos| deque.get(pos)) - .cloned(); - Ok(PyIterReturn::from_result(r.ok_or(None))) + locked_step(&zelf.internal, |internal| { + deque_step( + internal, + zelf.state, + |deque, pos| { + deque + .len() + .checked_sub(pos + 1) + .and_then(|pos| deque.get(pos)) + .cloned() + }, + vm, + ) }) } } diff --git a/extra_tests/snippets/builtin_iter.py b/extra_tests/snippets/builtin_iter.py index 6ef0d7f8cde..bcbb8e26e12 100644 --- a/extra_tests/snippets/builtin_iter.py +++ b/extra_tests/snippets/builtin_iter.py @@ -131,3 +131,25 @@ def __iter__(self): pass else: raise AssertionError(f"{empty.__class__.__name__}.join did not ask") + + +# An error from an element is the element's, not the end of the walk, so the +# next step reaches for the same one again. +class Balky: + def __getitem__(self, i): + if i == 1: + raise ValueError("boom") + if i > 2: + raise IndexError + return i + + +it = iter(Balky()) +assert next(it) == 0 +for _ in range(2): + try: + next(it) + except ValueError as e: + assert str(e) == "boom", e + else: + raise AssertionError("the element's error did not reach the caller") From 357ed5e31f2ad1f6eedeb0b0f14e995caf220016 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Wed, 19 Aug 2026 15:48:59 +0900 Subject: [PATCH 23/27] Name the caller in the bytes conversion TypeError An object with no iteration protocol reached `PyObject_GetIter`'s "not iterable" message, and `bytearray.extend()` reported bytes. Each entry point now checks for the protocol first and names itself: bytes(object()) cannot convert 'object' object to bytes bytearray(object()) cannot convert 'object' object to bytearray bytearray().extend(object()) can't extend bytearray with object Assisted-by: Claude --- crates/vm/src/builtins/bytearray.rs | 4 ++-- crates/vm/src/byte.rs | 32 ++++++++++++++++++++++++--- extra_tests/snippets/builtin_bytes.py | 25 +++++++++++++++++++++ 3 files changed, 56 insertions(+), 5 deletions(-) diff --git a/crates/vm/src/builtins/bytearray.rs b/crates/vm/src/builtins/bytearray.rs index b0222996072..594ecc569d8 100644 --- a/crates/vm/src/builtins/bytearray.rs +++ b/crates/vm/src/builtins/bytearray.rs @@ -8,7 +8,7 @@ use crate::{ VirtualMachine, anystr::{self, AnyStr}, atomic_func, - byte::{bytearray_from_object, bytes_from_object, value_from_object}, + byte::{bytearray_extend_from_object, bytearray_from_object, value_from_object}, bytes_inner::{ ByteInnerFindOptions, ByteInnerHexOptions, ByteInnerNewOptions, ByteInnerPaddingOptions, ByteInnerSplitOptions, ByteInnerSub, ByteInnerTranslateOptions, DecodeArgs, PyBytesInner, @@ -643,7 +643,7 @@ impl Py { vm.new_buffer_error("non-contiguous buffer is not a bytes-like object") })? .to_vec(), - None => bytes_from_object(vm, &object)?, + None => bytearray_extend_from_object(vm, &object)?, }; self.try_resizable(vm)?.elements.extend(items); Ok(()) diff --git a/crates/vm/src/byte.rs b/crates/vm/src/byte.rs index 3b3fc132519..b22fb54fa08 100644 --- a/crates/vm/src/byte.rs +++ b/crates/vm/src/byte.rs @@ -9,23 +9,49 @@ use crate::{ // PyBytes_FromObject pub fn bytes_from_object(vm: &VirtualMachine, obj: &PyObject) -> PyResult> { - collect_bytes(vm, obj, true) + collect_bytes(vm, obj, true, |name| { + format!("cannot convert '{name}' object to bytes") + }) } /// [`bytes_from_object`] for the bytearray constructor and for assigning to a /// slice of one, which run the iterator without asking the object they were /// handed how long it is. pub fn bytearray_from_object(vm: &VirtualMachine, obj: &PyObject) -> PyResult> { - collect_bytes(vm, obj, false) + collect_bytes(vm, obj, false, |name| { + format!("cannot convert '{name}' object to bytearray") + }) } -fn collect_bytes(vm: &VirtualMachine, obj: &PyObject, measured: bool) -> PyResult> { +/// [`bytes_from_object`] for `bytearray_extend()`, which names what it was +/// doing rather than what it was converting to. +pub fn bytearray_extend_from_object(vm: &VirtualMachine, obj: &PyObject) -> PyResult> { + collect_bytes(vm, obj, true, |name| { + format!("can't extend bytearray with {name}") + }) +} + +/// `measured` is whether the object is asked how long it is; `unusable` names, +/// from the class name, what could not be done with one that is not iterable. +fn collect_bytes( + vm: &VirtualMachine, + obj: &PyObject, + measured: bool, + unusable: impl FnOnce(&str) -> String, +) -> PyResult> { if obj.check_buffer() { let buffer = PyBuffer::from_object(vm, obj, BufferFlags::FULL_RO)?; return Ok(buffer.contiguous_or_collect(|bytes| bytes.to_vec())); } if !obj.fast_isinstance(vm.ctx.types.str_type) { + // What `PyObject_GetIter()` cannot take is answered for by the caller, + // which knows what it was being asked to do, rather than by the + // iteration protocol saying the object is not iterable. + let cls = obj.class(); + if cls.slots.iter.load().is_none() && !cls.has_attr(identifier!(vm, __getitem__)) { + return Err(vm.new_type_error(unusable(&cls.name()))); + } let value = |x: PyObjectRef| value_from_object(vm, &x); let elements = if measured { vm.map_iterable_object_sized(obj, value) diff --git a/extra_tests/snippets/builtin_bytes.py b/extra_tests/snippets/builtin_bytes.py index 5b05936690d..0c45fc3571e 100644 --- a/extra_tests/snippets/builtin_bytes.py +++ b/extra_tests/snippets/builtin_bytes.py @@ -788,3 +788,28 @@ def __len__(self): holder = bytearray(b"xyz") holder[:] = BadLen() assert holder == bytearray(b"\x01\x02\x03") + + +# What could not be turned into bytes is answered for by whatever was asked, +# rather than by the iteration protocol. +def cannot(fn, message): + try: + fn() + except TypeError as e: + assert str(e) == message, e + else: + raise AssertionError(f"expected TypeError: {message}") + + +cannot(lambda: bytes(object()), "cannot convert 'object' object to bytes") +cannot(lambda: bytes(1.5), "cannot convert 'float' object to bytes") +cannot(lambda: bytearray(object()), "cannot convert 'object' object to bytearray") +cannot( + lambda: bytearray(b"ab").__setitem__(slice(0, 2), object()), + "cannot convert 'object' object to bytearray", +) +cannot(lambda: bytearray().extend(object()), "can't extend bytearray with object") +cannot( + lambda: bytearray(b"ab").__setitem__(slice(0, 2), "ab"), + "can assign only bytes, buffers, or iterables of ints in range(0, 256)", +) From 468f9dd871b86da670aa68503785ca6801e308a5 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Wed, 19 Aug 2026 15:49:01 +0900 Subject: [PATCH 24/27] Take a strong count too large to hold as reachable `start_gc_refs` clipped a count at `GC_REACHABLE - 1`, a number the per-reference subtraction could still walk down to zero and collect a live object. It now stores `GC_REACHABLE`, and `subtract_gc_ref` leaves that value alone. Assisted-by: Claude --- crates/vm/src/object/core.rs | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/crates/vm/src/object/core.rs b/crates/vm/src/object/core.rs index 79d6b6ba945..5534666da5f 100644 --- a/crates/vm/src/object/core.rs +++ b/crates/vm/src/object/core.rs @@ -1821,11 +1821,16 @@ impl PyObject { } /// Enter the running collection's candidate set, with `strong_count` as the - /// count to subtract internal references from. Counts that do not fit stop - /// one short of [`GC_REACHABLE`], which only ever keeps the object alive. + /// count to subtract internal references from. A count too large to hold is + /// taken as reachable outright, rather than clipped to a number the + /// subtraction could still walk down to zero. #[inline] pub(crate) fn start_gc_refs(&self, strong_count: usize) { - let refs = strong_count.min(GC_REACHABLE as usize - 1) as u32; + let refs = if strong_count >= GC_REACHABLE as usize { + GC_REACHABLE + } else { + strong_count as u32 + }; self.0.gc_refs.store(refs, Ordering::Relaxed); self.set_gc_bit(GcBits::COLLECTING); } @@ -1843,10 +1848,15 @@ impl PyObject { .contains(GcBits::COLLECTING) } - /// Take off one reference held from inside the candidate set. + /// Take off one reference held from inside the candidate set. A count that + /// did not fit stands for more references than every subtraction together + /// could take off, so it stays where [`Self::start_gc_refs`] put it. #[inline] pub(crate) fn subtract_gc_ref(&self) { let refs = self.0.gc_refs.load(Ordering::Relaxed); + if refs == GC_REACHABLE { + return; + } self.0 .gc_refs .store(refs.saturating_sub(1), Ordering::Relaxed); From 09dca1e62d941f3658b65724fbdf6110e518a519 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Wed, 19 Aug 2026 15:49:03 +0900 Subject: [PATCH 25/27] Raise AssertionError where a snippet asserts False `assert False` is removed under `-O`, which the snippet suite may run. Assisted-by: Claude --- extra_tests/snippets/stdlib_ctypes.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/extra_tests/snippets/stdlib_ctypes.py b/extra_tests/snippets/stdlib_ctypes.py index ea3538c348c..827fc598826 100644 --- a/extra_tests/snippets/stdlib_ctypes.py +++ b/extra_tests/snippets/stdlib_ctypes.py @@ -444,7 +444,7 @@ def get_win_folder_via_ctypes(csidl_name: str) -> str: except ValueError: pass else: - assert False, "slice assignment accepted an unbounded iterable" + raise AssertionError("slice assignment accepted an unbounded iterable") array3[0:3] = [7, 8, 9] assert list(array3) == [7, 8, 9] @@ -456,6 +456,6 @@ def get_win_folder_via_ctypes(csidl_name: str) -> str: except MemoryError: pass else: - assert False, "an unallocatable array was created" + raise AssertionError("an unallocatable array was created") print("done") From 0f38af2bbefb97ce503418cb585a090df9011548 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Wed, 19 Aug 2026 16:12:56 +0900 Subject: [PATCH 26/27] Keep raising for a collection that moved under its iterator A deque, set or dict iterator raised once and then read as spent. The guard is sticky: `deque_iternext()` looks at the deque's state before the count it keeps, and `dictiter_iternextkey()` and `setiter_iternext()` write a size no collection can have, so every later call finds the same thing and raises again. `dequereviter_next()` is the one exception, looking at its count first, so it runs out after the first raise. Both deque iterators now carry `dequeiterobject.counter` rather than reading a length back from the deque, and the dict and set iterators compare the size they captured against the collection's own every time they are asked how much is left, which is what makes that answer nothing from the moment the collection changes rather than only once the iterator has raised. The set's message is capitalized to match `setiter_iternext()`. Measured against 3.14.6, for each of deque, reversed deque, set, dict and a reversed dict view: the hint after the change, the error, the hint after the error, and what a later call answers. Assisted-by: Claude --- crates/vm/src/builtins/dict.rs | 70 ++++++++++++++---- crates/vm/src/builtins/set.rs | 33 +++++++-- crates/vm/src/stdlib/_collections.rs | 103 ++++++++++++++++++--------- extra_tests/snippets/builtin_iter.py | 60 ++++++++++++++++ 4 files changed, 214 insertions(+), 52 deletions(-) diff --git a/crates/vm/src/builtins/dict.rs b/crates/vm/src/builtins/dict.rs index f56b2e7da23..9c09833d321 100644 --- a/crates/vm/src/builtins/dict.rs +++ b/crates/vm/src/builtins/dict.rs @@ -24,6 +24,7 @@ use crate::{ use alloc::fmt; use core::cell::Cell; use core::ptr::NonNull; +use rustpython_common::atomic::{Ordering, PyAtomic, Radium}; use rustpython_common::lock::PyMutex; use rustpython_common::wtf8::Wtf8Buf; @@ -1164,6 +1165,11 @@ macro_rules! dict_view { #[derive(Debug)] pub(crate) struct $iter_name { pub(crate) size: dict_inner::DictSize, + /// Whether the dict was found to have changed, which + /// `dictiter_iternextkey()` records by writing a size no dict can + /// have. Sticky: what it makes the iterator answer, it answers + /// from then on. + changed: PyAtomic, pub(crate) internal: PyMutex>, } @@ -1179,13 +1185,26 @@ macro_rules! dict_view { fn new(dict: PyDictRef) -> Self { $iter_name { size: dict.size(), + changed: Radium::new(false), internal: PyMutex::new(PositionIterInternal::new(dict, 0)), } } #[pymethod] fn __length_hint__(&self) -> usize { - self.internal.lock().length_hint(|_| self.size.entries_size) + // `dictiter_len()` answers for a dict it can no longer walk + // with nothing, comparing the size it captured against the + // dict's own every time it is asked. + if self.changed.load(Ordering::Relaxed) { + return 0; + } + self.internal.lock().length_hint(|dict| { + if dict.size() == self.size { + self.size.entries_size + } else { + 0 + } + }) } #[pymethod] @@ -1218,14 +1237,22 @@ macro_rules! dict_view { let IterStatus::Active(dict) = &internal.status else { return (Ok(PyIterReturn::StopIteration(None)), None); }; + let mutated = + || vm.new_runtime_error("dictionary changed size during iteration"); + if zelf.changed.load(Ordering::Relaxed) { + // The dict is not looked at again once it has been + // found to change: an iterator that has raised keeps + // raising. + return (Err(mutated()), None); + } let entry = dict.entries .next_entry_checked(internal.position, &zelf.size, $project_fn); match entry { - Err(dict_inner::DictChanged) => ( - Err(vm.new_runtime_error("dictionary changed size during iteration")), - internal.exhaust(), - ), + Err(dict_inner::DictChanged) => { + zelf.changed.store(true, Ordering::Relaxed); + (Err(mutated()), None) + } Ok(Some((position, item))) => { internal.position = position; (Ok(PyIterReturn::Return(($result_fn)(vm, item))), None) @@ -1240,6 +1267,8 @@ macro_rules! dict_view { #[derive(Debug)] pub(crate) struct $reverse_iter_name { pub(crate) size: dict_inner::DictSize, + /// As in `$iter_name`. + changed: PyAtomic, internal: PyMutex>, } @@ -1257,6 +1286,7 @@ macro_rules! dict_view { let position = size.entries_size.saturating_sub(1); $reverse_iter_name { size, + changed: Radium::new(false), internal: PyMutex::new(PositionIterInternal::new(dict, position)), } } @@ -1287,9 +1317,17 @@ macro_rules! dict_view { #[pymethod] fn __length_hint__(&self) -> usize { - self.internal - .lock() - .rev_length_hint(|_| self.size.entries_size) + // As in `$iter_name`. + if self.changed.load(Ordering::Relaxed) { + return 0; + } + let internal = self.internal.lock(); + match &internal.status { + IterStatus::Active(dict) if dict.size() == self.size => { + internal.rev_length_hint(|_| self.size.entries_size) + } + _ => 0, + } } } @@ -1301,14 +1339,22 @@ macro_rules! dict_view { let IterStatus::Active(dict) = &internal.status else { return (Ok(PyIterReturn::StopIteration(None)), None); }; + let mutated = + || vm.new_runtime_error("dictionary changed size during iteration"); + if zelf.changed.load(Ordering::Relaxed) { + // The dict is not looked at again once it has been + // found to change: an iterator that has raised keeps + // raising. + return (Err(mutated()), None); + } let entry = dict.entries .prev_entry_checked(internal.position, &zelf.size, $project_fn); match entry { - Err(dict_inner::DictChanged) => ( - Err(vm.new_runtime_error("dictionary changed size during iteration")), - internal.exhaust(), - ), + Err(dict_inner::DictChanged) => { + zelf.changed.store(true, Ordering::Relaxed); + (Err(mutated()), None) + } Ok(Some((found_index, item))) => { let released = if found_index == 0 { internal.exhaust() diff --git a/crates/vm/src/builtins/set.rs b/crates/vm/src/builtins/set.rs index 31784b6f054..7e6f43bbb4c 100644 --- a/crates/vm/src/builtins/set.rs +++ b/crates/vm/src/builtins/set.rs @@ -1486,6 +1486,10 @@ impl TryFromObject for AnySet { #[pyclass(module = false, name = "set_iterator")] pub(crate) struct PySetIterator { size: DictSize, + /// Whether the set was found to have changed, which `setiter_iternext()` + /// records by writing a size no set can have. Sticky: what it makes the + /// iterator answer, it answers from then on. + changed: PyAtomic, internal: PyMutex>, } @@ -1507,6 +1511,7 @@ impl PySetIterator { fn new(set: AnySet) -> Self { Self { size: set.as_inner().content.size(), + changed: Radium::new(false), internal: PyMutex::new(PositionIterInternal::new(set, 0)), } } @@ -1516,7 +1521,19 @@ impl PySetIterator { impl PySetIterator { #[pymethod] fn __length_hint__(&self) -> usize { - self.internal.lock().length_hint(|_| self.size.entries_size) + // `setiter_len()` answers for a set it can no longer walk with nothing, + // comparing the size it captured against the set's own every time it is + // asked. + if self.changed.load(Ordering::Relaxed) { + return 0; + } + self.internal.lock().length_hint(|set| { + if set.as_inner().content.size() == self.size { + self.size.entries_size + } else { + 0 + } + }) } #[pymethod] @@ -1547,16 +1564,22 @@ impl IterNext for PySetIterator { let IterStatus::Active(set) = &internal.status else { return (Ok(PyIterReturn::StopIteration(None)), None); }; + let mutated = || vm.new_runtime_error("Set changed size during iteration"); + if zelf.changed.load(Ordering::Relaxed) { + // The set is not looked at again once it has been found to + // change: an iterator that has raised keeps raising. + return (Err(mutated()), None); + } let entry = set.as_inner().content.next_entry_checked( internal.position, &zelf.size, |key, ()| key.clone(), ); match entry { - Err(crate::dict_inner::DictChanged) => ( - Err(vm.new_runtime_error("set changed size during iteration")), - internal.exhaust(), - ), + Err(crate::dict_inner::DictChanged) => { + zelf.changed.store(true, Ordering::Relaxed); + (Err(mutated()), None) + } Ok(Some((position, key))) => { internal.position = position; (Ok(PyIterReturn::Return(key)), None) diff --git a/crates/vm/src/stdlib/_collections.rs b/crates/vm/src/stdlib/_collections.rs index 5fc53c92c54..4bd4aee25b3 100644 --- a/crates/vm/src/stdlib/_collections.rs +++ b/crates/vm/src/stdlib/_collections.rs @@ -278,6 +278,7 @@ mod _collections { fn __reversed__(zelf: PyRef) -> PyReverseDequeIterator { PyReverseDequeIterator { state: zelf.state.load(), + counter: AtomicCell::new(zelf.__len__()), internal: PyMutex::new(PositionIterInternal::new(zelf, 0)), } } @@ -633,6 +634,11 @@ mod _collections { #[derive(Debug, PyPayload)] struct PyDequeIterator { state: usize, + /// How many elements are left to walk, `dequeiterobject.counter`. Kept + /// beside the deque rather than read back from it, because a mutated + /// deque is walked no further and what is left of it then reads as + /// nothing. + counter: AtomicCell, internal: PyMutex>, } @@ -657,6 +663,8 @@ mod _collections { if let OptionalArg::Present(index) = index { let index = max(index, 0) as usize; iter.internal.lock().position = index; + iter.counter + .store(iter.counter.load().saturating_sub(index)); } Ok(iter) } @@ -667,13 +675,14 @@ mod _collections { pub(crate) fn new(deque: PyDequeRef) -> Self { Self { state: deque.state.load(), + counter: AtomicCell::new(deque.__len__()), internal: PyMutex::new(PositionIterInternal::new(deque, 0)), } } #[pymethod] fn __length_hint__(&self) -> usize { - self.internal.lock().length_hint(|obj| obj.__len__()) + self.counter.load() } #[pymethod] @@ -695,41 +704,61 @@ mod _collections { impl SelfIter for PyDequeIterator {} - /// One step of either deque iterator, reaching for an element with `at`. - fn deque_step( - internal: &mut PositionIterInternal, + /// Whether the deque moved under an iterator that captured `state`. What is + /// left to walk is emptied before the error goes out, the way + /// `deque_iternext()` zeroes its counter before it raises. + fn deque_moved( + internal: &PositionIterInternal, state: usize, + counter: &AtomicCell, + ) -> bool { + let Active(deque) = &internal.status else { + return false; + }; + if state == deque.state.load() { + return false; + } + counter.store(0); + true + } + + /// Hand back the element at the position the iterator keeps, `at` reaching + /// for it. Both deque iterators end here; they differ in whether they look + /// at the deque or at the count first. + fn deque_take( + internal: &mut PositionIterInternal, + counter: &AtomicCell, at: impl FnOnce(&VecDeque, usize) -> Option, - vm: &VirtualMachine, ) -> (PyResult, Option) { - let Active(deque) = &internal.status else { - return (Ok(PyIterReturn::StopIteration(None)), None); + let item = match &internal.status { + Active(deque) if counter.load() != 0 => at(&deque.borrow_deque(), internal.position), + _ => None, }; - if state != deque.state.load() { - // `deque_iternext()` empties the iterator before it raises, so what - // is left to walk reads as nothing. - return ( - Err(vm.new_runtime_error("deque mutated during iteration")), - internal.exhaust(), - ); - } - let item = at(&deque.borrow_deque(), internal.position); let Some(item) = item else { + counter.store(0); return (Ok(PyIterReturn::StopIteration(None)), internal.exhaust()); }; internal.position += 1; + counter.store(counter.load() - 1); (Ok(PyIterReturn::Return(item)), None) } + fn deque_mutated(vm: &VirtualMachine) -> PyResult { + Err(vm.new_runtime_error("deque mutated during iteration")) + } + impl IterNext for PyDequeIterator { fn next(zelf: &Py, vm: &VirtualMachine) -> PyResult { locked_step(&zelf.internal, |internal| { - deque_step( - internal, - zelf.state, - |deque, pos| deque.get(pos).cloned(), - vm, - ) + // The deque before the count, as in `deque_iternext()`, so an + // iterator still holding a deque that moved raises again on + // every call rather than running out after the first. + if deque_moved(internal, zelf.state, &zelf.counter) { + return (deque_mutated(vm), None); + } + deque_take(internal, &zelf.counter, |deque, pos| { + deque.get(pos).cloned() + }) }) } } @@ -739,6 +768,8 @@ mod _collections { #[derive(Debug, PyPayload)] struct PyReverseDequeIterator { state: usize, + /// As in [`PyDequeIterator`]. + counter: AtomicCell, // position is counting from the tail internal: PyMutex>, } @@ -755,6 +786,8 @@ mod _collections { if let OptionalArg::Present(index) = index { let index = max(index, 0) as usize; iter.internal.lock().position = index; + iter.counter + .store(iter.counter.load().saturating_sub(index)); } Ok(iter) } @@ -764,7 +797,7 @@ mod _collections { impl PyReverseDequeIterator { #[pymethod] fn __length_hint__(&self) -> usize { - self.internal.lock().length_hint(|obj| obj.__len__()) + self.counter.load() } #[pymethod] @@ -789,18 +822,18 @@ mod _collections { impl IterNext for PyReverseDequeIterator { fn next(zelf: &Py, vm: &VirtualMachine) -> PyResult { locked_step(&zelf.internal, |internal| { - deque_step( - internal, - zelf.state, - |deque, pos| { - deque - .len() - .checked_sub(pos + 1) - .and_then(|pos| deque.get(pos)) - .cloned() - }, - vm, - ) + // The count before the deque, as in `dequereviter_next()`, so + // an iterator that has raised once runs out instead. + if zelf.counter.load() != 0 && deque_moved(internal, zelf.state, &zelf.counter) { + return (deque_mutated(vm), None); + } + deque_take(internal, &zelf.counter, |deque, pos| { + deque + .len() + .checked_sub(pos + 1) + .and_then(|pos| deque.get(pos)) + .cloned() + }) }) } } diff --git a/extra_tests/snippets/builtin_iter.py b/extra_tests/snippets/builtin_iter.py index bcbb8e26e12..09fb94eeea2 100644 --- a/extra_tests/snippets/builtin_iter.py +++ b/extra_tests/snippets/builtin_iter.py @@ -153,3 +153,63 @@ def __getitem__(self, i): assert str(e) == "boom", e else: raise AssertionError("the element's error did not reach the caller") + + +# A collection that moved under its iterator raises every time it is asked +# again, rather than reading as spent after the first. What is left to walk +# reads as nothing from the moment the collection no longer matches. +from collections import deque +from operator import length_hint + + +def moved(make, mutate, restore, moved_hint, again): + it = make() + next(it) + assert length_hint(it) == 9, length_hint(it) + mutate() + assert length_hint(it) == moved_hint, length_hint(it) + try: + next(it) + except RuntimeError: + pass + else: + raise AssertionError("a collection that moved was walked further") + assert length_hint(it) == 0, length_hint(it) + restore() + try: + next(it) + except RuntimeError: + got = RuntimeError + except StopIteration: + got = StopIteration + else: + raise AssertionError("a collection that moved was walked further") + assert got is again, got + assert length_hint(it) == 0, length_hint(it) + + +# A deque iterator carries its own count, so what the deque does to its own +# length before the iterator is asked again is not what the count answers. +d = deque(range(10)) +moved(lambda: iter(d), d.pop, lambda: d.append(99), 9, RuntimeError) +d2 = deque(range(10)) +# `dequereviter_next()` looks at the count before the deque, so once the count +# is spent the deque is never looked at again. +moved(lambda: reversed(d2), d2.pop, lambda: d2.append(99), 9, StopIteration) + +# A dict or set iterator answers from the size it captured, which the +# collection stops matching the moment it changes. +s = set(range(10)) +moved(lambda: iter(s), lambda: s.add(99), lambda: s.discard(99), 0, RuntimeError) +dd = {i: i for i in range(10)} +moved( + lambda: iter(dd), lambda: dd.update({99: 99}), lambda: dd.pop(99), 0, RuntimeError +) +dv = {i: i for i in range(10)} +moved( + lambda: reversed(dv.items()), + lambda: dv.update({99: 99}), + lambda: dv.pop(99), + 0, + RuntimeError, +) From 7392a20f4657d60caf3e0f5e0b147401b35faf34 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Wed, 19 Aug 2026 18:30:36 +0900 Subject: [PATCH 27/27] Give apt-get update a deadline to fail on The step waits on `apt-get update`, which has no deadline of its own, so a source that takes the connection and then stops answering holds the job until the workflow's own timeout. The retry that disables the Microsoft and azure-cli sources runs only when the update exits non-zero, which a held connection never does; three jobs on this branch sat on this step for 20 minutes to five and a half hours. Each attempt is now bounded, and the transports are given a timeout, so a source that stops answering reaches the retry. Assisted-by: Claude --- .github/actions/install-linux-deps/action.yml | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/.github/actions/install-linux-deps/action.yml b/.github/actions/install-linux-deps/action.yml index c2f1b20f2d9..6ce8393ce41 100644 --- a/.github/actions/install-linux-deps/action.yml +++ b/.github/actions/install-linux-deps/action.yml @@ -50,14 +50,26 @@ runs: GCC_AARCH64_LINUX_GNU: ${{ inputs.gcc-aarch64-linux-gnu }} GCC_MINGW_W64_X86_64: ${{ inputs.gcc-mingw-w64-x86-64 }} run: | - if ! sudo apt-get update; then - echo "::warning::apt-get update failed; disabling nonessential Microsoft apt sources and retrying" + # `apt-get update` has no deadline of its own, so a source that takes + # the connection and then stops answering holds the job rather than + # failing it, and the retry below never runs. Bound each attempt and + # give the transports a timeout to fail on. + apt_update() { + sudo timeout 300 apt-get \ + -o Acquire::Retries=3 \ + -o Acquire::http::Timeout=20 \ + -o Acquire::https::Timeout=20 \ + update + } + + if ! apt_update; then + echo "::warning::apt-get update did not finish; disabling nonessential Microsoft apt sources and retrying" for source in /etc/apt/sources.list.d/*microsoft* /etc/apt/sources.list.d/*azure-cli*; do if [ -e "$source" ]; then sudo mv "$source" "$source.disabled" fi done - sudo apt-get update + apt_update fi packages=()