diff --git a/.cspell.dict/cpython.txt b/.cspell.dict/cpython.txt index da85c312898..f9aa440edee 100644 --- a/.cspell.dict/cpython.txt +++ b/.cspell.dict/cpython.txt @@ -165,6 +165,7 @@ Nondescriptor noninteger nops noraise +npools nseen NSIGNALS numer @@ -276,5 +277,6 @@ winconsoleio withitem withs worklist +XSETREF xstat XXPRIME 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=() diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 5a8daae06ef..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: @@ -302,8 +311,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 +319,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 +327,7 @@ jobs: - '-u all' - '--timeout 600' - '--dont-add-python-opts' - env_polluting_tests: - - test_set + env_polluting_tests: [] skips: [] timeout: 50 fail-fast: false @@ -465,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 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: 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"))] 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/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 038e7cae9f3..b1de6eafbe2 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,62 @@ 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 } + } + + /// 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 #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -438,22 +491,45 @@ 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]; + let mut data = vm + .new_zeroed_bytes(self.size) + .map_err(|e| PackError::from_exception(e, vm))?; - 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 +547,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 +632,7 @@ trait Packable { code: FormatType, arg: PyObjectRef, data: &mut [u8], - ) -> PyResult<()>; + ) -> Result<(), PackError>; fn unpack(vm: &VirtualMachine, data: &[u8]) -> PyObjectRef; } @@ -584,7 +662,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 +676,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 +708,7 @@ where T::max_value() ) }; - new_struct_error(vm, msg) + PackError::new(PackErrorKind::Value, vm, msg) }) } @@ -641,15 +731,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 +767,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 +795,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 +810,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(PackError::raised)? + .into_bool() as u8; v.pack_int::(data); Ok(()) } @@ -727,13 +829,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/bytearray.rs b/crates/vm/src/builtins/bytearray.rs index c8782127b3f..594ecc569d8 100644 --- a/crates/vm/src/builtins/bytearray.rs +++ b/crates/vm/src/builtins/bytearray.rs @@ -1,14 +1,14 @@ //! 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, VirtualMachine, anystr::{self, AnyStr}, atomic_func, - byte::{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, @@ -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) @@ -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(()) @@ -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(()) } @@ -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 bfa2fd3545b..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::{ @@ -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) { @@ -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..9c09833d321 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}; @@ -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; @@ -240,7 +241,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 +258,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)? { @@ -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] @@ -1214,32 +1233,33 @@ 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, - ) { + locked_step(&zelf.internal, |internal| { + 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) => { - internal.status = IterStatus::Exhausted; - return Err( - vm.new_runtime_error("dictionary changed size during iteration") - ); + zelf.changed.store(true, Ordering::Relaxed); + (Err(mutated()), None) } 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) + }) } } @@ -1247,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>, } @@ -1264,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)), } } @@ -1294,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, + } } } @@ -1304,36 +1335,38 @@ 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, - ) { + locked_step(&zelf.internal, |internal| { + 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) => { - internal.status = IterStatus::Exhausted; - return Err( - vm.new_runtime_error("dictionary changed size during iteration") - ); + zelf.changed.store(true, Ordering::Relaxed); + (Err(mutated()), None) } 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/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] 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/builtins/iter.rs b/crates/vm/src/builtins/iter.rs index 4e29df583a8..2d231e9e6a8 100644 --- a/crates/vm/src/builtins/iter.rs +++ b/crates/vm/src/builtins/iter.rs @@ -91,41 +91,67 @@ 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 - } else { - Ok(PyIterReturn::StopIteration(None)) + let IterStatus::Active(obj) = &self.status else { + return (Ok(PyIterReturn::StopIteration(None)), None); + }; + let ret = f(obj, self.position); + 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) + } + + /// 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 +179,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 +290,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 fe674a45821..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::{ @@ -187,7 +188,11 @@ 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. 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(()) } @@ -221,8 +226,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 +235,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 +484,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![] }; @@ -909,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))) }) @@ -975,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/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/crates/vm/src/builtins/memory.rs b/crates/vm/src/builtins/memory.rs index 0b04de133e7..86396825b0c 100644 --- a/crates/vm/src/builtins/memory.rs +++ b/crates/vm/src/builtins/memory.rs @@ -1,12 +1,13 @@ 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::{ 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 +330,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 +854,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 +1148,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 +1302,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) } }), }; @@ -1681,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/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/set.rs b/crates/vm/src/builtins/set.rs index d737612b158..7e6f43bbb4c 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,11 @@ impl TryFromObject for AnySet { #[pyclass(module = false, name = "set_iterator")] pub(crate) struct PySetIterator { size: DictSize, - internal: PyMutex>>, + /// 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>, } impl fmt::Debug for PySetIterator { @@ -1504,11 +1507,33 @@ impl PyPayload for PySetIterator { } } +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)), + } + } +} + #[pyclass(flags(DISALLOW_INSTANTIATION), with(IterNext, Iterable))] 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] @@ -1519,9 +1544,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 +1560,33 @@ 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()) { + locked_step(&zelf.internal, |internal| { + 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) => { - internal.status = IterStatus::Exhausted; - return Err(vm.new_runtime_error("set changed size during iteration")); + zelf.changed.store(true, Ordering::Relaxed); + (Err(mutated()), None) } 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) + }) } } diff --git a/crates/vm/src/builtins/str.rs b/crates/vm/src/builtins/str.rs index 6e774f7e652..dc5a1daf4b9 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)) } @@ -1173,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/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/byte.rs b/crates/vm/src/byte.rs index 0e90f296ac9..b22fb54fa08 100644 --- a/crates/vm/src/byte.rs +++ b/crates/vm/src/byte.rs @@ -3,21 +3,64 @@ 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, |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, |name| { + format!("cannot convert '{name}' object to bytearray") + }) +} + +/// [`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) - && 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) { + // 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) + } 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..d87fee4c2a2 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) => { @@ -614,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/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..5534666da5f 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,67 @@ 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. 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 = 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); + } + + /// 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. 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); + } + + /// 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 +2801,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 +2818,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}; 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/_collections.rs b/crates/vm/src/stdlib/_collections.rs index b48c0e670ac..4bd4aee25b3 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_step, }, common::lock::{PyMutex, PyRwLock, PyRwLockReadGuard, PyRwLockWriteGuard}, convert::ToPyObject, @@ -277,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)), } } @@ -632,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>, } @@ -656,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) } @@ -666,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] @@ -693,16 +703,62 @@ mod _collections { } impl SelfIter for PyDequeIterator {} + + /// 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, + ) -> (PyResult, Option) { + let item = match &internal.status { + Active(deque) if counter.load() != 0 => at(&deque.borrow_deque(), internal.position), + _ => None, + }; + 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 { - zelf.internal.lock().next(|deque, pos| { - if zelf.state != deque.state.load() { - return Err(vm.new_runtime_error("Deque mutated during iteration")); + locked_step(&zelf.internal, |internal| { + // 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); } - let deque = deque.borrow_deque(); - Ok(PyIterReturn::from_result( - deque.get(pos).cloned().ok_or(None), - )) + deque_take(internal, &zelf.counter, |deque, pos| { + deque.get(pos).cloned() + }) }) } } @@ -712,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>, } @@ -728,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) } @@ -737,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] @@ -761,17 +821,19 @@ mod _collections { impl IterNext for PyReverseDequeIterator { fn next(zelf: &Py, vm: &VirtualMachine) -> PyResult { - zelf.internal.lock().next(|deque, pos| { - if deque.state.load() != zelf.state { - return Err(vm.new_runtime_error("Deque mutated during iteration")); + locked_step(&zelf.internal, |internal| { + // 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); } - 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))) + deque_take(internal, &zelf.counter, |deque, pos| { + deque + .len() + .checked_sub(pos + 1) + .and_then(|pos| deque.get(pos)) + .cloned() + }) }) } } 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/_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/_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/_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/_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::{ 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/stdlib/itertools.rs b/crates/vm/src/stdlib/itertools.rs index 6eb268d94c1..5f85f1b7238 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,47 @@ 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; + + // 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() { - 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 mut pools: Vec> = Vec::new(); + pools + .try_reserve_exact(npools) + .map_err(|_| vm.new_memory_error(""))?; + // 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) + .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..3e5e2393ee3 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) => { @@ -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/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}; diff --git a/crates/vm/src/vm/mod.rs b/crates/vm/src/vm/mod.rs index 54d3e813eec..a26ab3761ce 100644 --- a/crates/vm/src/vm/mod.rs +++ b/crates/vm/src/vm/mod.rs @@ -879,6 +879,19 @@ struct SuspendedFrame { is_entry: bool, } +/// 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> { + /// 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), +} + impl VirtualMachine { fn init_callable_cache(&mut self) -> PyResult<()> { self.callable_cache.len = Some(self.builtins.get_attr("len", self)?); @@ -2161,13 +2174,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 +2273,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 +2286,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())); } @@ -2635,6 +2642,49 @@ impl VirtualMachine { where F: Fn(PyObjectRef) -> PyResult, { + self.extract_elements_inner(value, LengthHint::Unasked, func) + } + + /// [`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: &dyn Fn() -> usize, + func: F, + ) -> PyResult> + where + F: Fn(PyObjectRef) -> PyResult, + { + self.extract_elements_inner(value, LengthHint::Iterable(held), func) + } + + fn extract_elements_inner( + &self, + value: &PyObject, + hint: LengthHint<'_>, + 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). @@ -2644,9 +2694,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 = { @@ -2663,34 +2715,58 @@ 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, hint, func); }; - slice.iter().map(|obj| func(obj.clone())).collect() + map_known_len(slice.iter(), |obj| func(obj.clone())) + } + + /// [`Self::map_iterable_object`] for a caller that asks the object it was + /// handed how long it is. + 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::Unasked, f) } - pub fn map_iterable_object(&self, obj: &PyObject, mut f: F) -> PyResult>> + fn map_iterable_object_inner( + &self, + obj: &PyObject, + hint: LengthHint<'_>, + mut f: F, + ) -> PyResult>> where F: FnMut(PyObjectRef) -> PyResult, { @@ -2719,33 +2795,55 @@ 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, hint, f)) } }) } - fn map_py_iter(&self, value: &PyObject, 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)?; - 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 - { - return Ok(Vec::new()); - } - let mut results = PyIterIter::new(self, iter.as_ref(), cap) - .map(|element| f(element?)) - .collect::>>()?; + // 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. 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 + // 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. 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(); + 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?)?); + } results.shrink_to_fit(); Ok(results) } 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>, diff --git a/extra_tests/snippets/builtin_bytes.py b/extra_tests/snippets/builtin_bytes.py index 3cbed79c069..0c45fc3571e 100644 --- a/extra_tests/snippets/builtin_bytes.py +++ b/extra_tests/snippets/builtin_bytes.py @@ -766,3 +766,50 @@ 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") + + +# 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)", +) 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/builtin_iter.py b/extra_tests/snippets/builtin_iter.py index 02d469a47ee..09fb94eeea2 100644 --- a/extra_tests/snippets/builtin_iter.py +++ b/extra_tests/snippets/builtin_iter.py @@ -69,3 +69,147 @@ 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") + + +# 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") + + +# 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, +) diff --git a/extra_tests/snippets/builtin_list.py b/extra_tests/snippets/builtin_list.py index 44492092bad..d2fb75b3576 100644 --- a/extra_tests/snippets/builtin_list.py +++ b/extra_tests/snippets/builtin_list.py @@ -930,3 +930,124 @@ 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] + + +# 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 at all, 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()] + + +# 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] 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 diff --git a/extra_tests/snippets/builtin_memoryview.py b/extra_tests/snippets/builtin_memoryview.py index 979f584a2b1..ef28ffc3d79 100644 --- a/extra_tests/snippets/builtin_memoryview.py +++ b/extra_tests/snippets/builtin_memoryview.py @@ -850,3 +850,114 @@ 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() + + +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() diff --git a/extra_tests/snippets/stdlib_ctypes.py b/extra_tests/snippets/stdlib_ctypes.py index 109665b6a03..827fc598826 100644 --- a/extra_tests/snippets/stdlib_ctypes.py +++ b/extra_tests/snippets/stdlib_ctypes.py @@ -444,8 +444,18 @@ 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] + +# 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: + raise AssertionError("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..ef06ce52985 100644 --- a/extra_tests/snippets/stdlib_itertools.py +++ b/extra_tests/snippets/stdlib_itertools.py @@ -540,3 +540,32 @@ 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) + + +# 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 == [] 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))