From 5499d1087941c8030d5836471ff32f23fbb1e797 Mon Sep 17 00:00:00 2001 From: Matthew Broadway Date: Sun, 3 Nov 2024 15:50:59 +0000 Subject: [PATCH] fix some tests and linting errors --- Cargo.toml | 2 +- guide/src/class/metaclass.md | 3 +- src/impl_/pyclass.rs | 38 +++++++++---------- src/pycell/impl_.rs | 9 +++++ src/pyclass/create_type_object.rs | 38 ++++++++++++------- src/type_object.rs | 6 +-- tests/test_class_basics.rs | 5 +-- tests/test_compile_error.rs | 3 ++ tests/test_inheritance.rs | 10 ++--- tests/ui/invalid_extend_variable_sized.rs | 14 +++++++ tests/ui/invalid_extend_variable_sized.stderr | 15 ++++++++ 11 files changed, 94 insertions(+), 49 deletions(-) create mode 100644 tests/ui/invalid_extend_variable_sized.rs create mode 100644 tests/ui/invalid_extend_variable_sized.stderr diff --git a/Cargo.toml b/Cargo.toml index 9e931ed00b6..f2c7b42c187 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -12,7 +12,7 @@ categories = ["api-bindings", "development-tools::ffi"] license = "MIT OR Apache-2.0" exclude = ["/.gitignore", ".cargo/config", "/codecov.yml", "/Makefile", "/pyproject.toml", "/noxfile.py", "/.github", "/tests/test_compile_error.rs", "/tests/ui"] edition = "2021" -rust-version = "1.63" +rust-version = "1.65" [dependencies] cfg-if = "1.0" diff --git a/guide/src/class/metaclass.md b/guide/src/class/metaclass.md index fb62bb2d479..08ab29ce29f 100644 --- a/guide/src/class/metaclass.md +++ b/guide/src/class/metaclass.md @@ -29,9 +29,8 @@ impl MyMetaclass { slf: Bound<'_, Metaclass>, _args: Bound<'_, PyTuple>, _kwargs: Option>, - ) -> PyResult<()> { + ) { slf.borrow_mut().counter = 5; - Ok(()) } fn __getitem__(&self, item: u64) -> u64 { diff --git a/src/impl_/pyclass.rs b/src/impl_/pyclass.rs index 0fbde77872c..6fa027047c8 100644 --- a/src/impl_/pyclass.rs +++ b/src/impl_/pyclass.rs @@ -1,3 +1,5 @@ +#[cfg(Py_3_12)] +use crate::pycell::impl_::PyClassObjectContents; use crate::{ conversion::IntoPyObject, exceptions::{PyAttributeError, PyNotImplementedError, PyRuntimeError, PyValueError}, @@ -8,10 +10,7 @@ use crate::{ pyclass_init::PyObjectInit, pymethods::{PyGetterDef, PyMethodDefType}, }, - pycell::{ - impl_::{InternalPyClassObjectLayout, PyClassObjectContents}, - PyBorrowError, - }, + pycell::{impl_::InternalPyClassObjectLayout, PyBorrowError}, types::{any::PyAnyMethods, PyBool}, Borrowed, BoundObject, Py, PyAny, PyClass, PyErr, PyRef, PyResult, PyTypeInfo, Python, }; @@ -1197,20 +1196,9 @@ pub enum PyObjectOffset { /// An offset relative to the start of the subclass-specific data. /// Only allowed when basicsize is negative (which is only allowed for python >=3.12). /// - #[cfg(Py_3_12)] Relative(ffi::Py_ssize_t), } -impl PyObjectOffset { - pub fn to_value_and_is_relative(&self) -> (ffi::Py_ssize_t, bool) { - match self { - PyObjectOffset::Absolute(offset) => (*offset, false), - #[cfg(Py_3_12)] - PyObjectOffset::Relative(offset) => (*offset, true), - } - } -} - impl std::ops::Add for PyObjectOffset { type Output = PyObjectOffset; @@ -1221,7 +1209,6 @@ impl std::ops::Add for PyObjectOffset { match self { PyObjectOffset::Absolute(offset) => PyObjectOffset::Absolute(offset + rhs), - #[cfg(Py_3_12)] PyObjectOffset::Relative(offset) => PyObjectOffset::Relative(offset + rhs), } } @@ -1321,11 +1308,16 @@ impl< pub fn generate(&self, name: &'static CStr, doc: &'static CStr) -> PyMethodDefType { use crate::pyclass::boolean_struct::private::Boolean; if ClassT::Frozen::VALUE { - let (offset, is_relative) = Offset::offset().to_value_and_is_relative(); - let flags = if is_relative { - ffi::Py_READONLY | ffi::Py_RELATIVE_OFFSET - } else { - ffi::Py_READONLY + let (offset, flags) = match Offset::offset() { + PyObjectOffset::Absolute(offset) => (offset, ffi::Py_READONLY), + #[cfg(Py_3_12)] + PyObjectOffset::Relative(offset) => { + (offset, ffi::Py_READONLY | ffi::Py_RELATIVE_OFFSET) + } + #[cfg(not(Py_3_12))] + PyObjectOffset::Relative(_) => { + panic!("relative offsets not valid before python 3.12"); + } }; PyMethodDefType::StructMember(ffi::PyMemberDef { name: name.as_ptr(), @@ -1560,6 +1552,10 @@ where let contents = class_obj.contents_mut() as *mut PyClassObjectContents; (contents.cast::(), offset) } + #[cfg(not(Py_3_12))] + PyObjectOffset::Relative(_) => { + panic!("relative offsets not valid before python 3.12"); + } }; // Safety: conditions for pointer addition must be met unsafe { base.add(offset as usize) }.cast::() diff --git a/src/pycell/impl_.rs b/src/pycell/impl_.rs index c60c63ed52c..96eafc2348c 100644 --- a/src/pycell/impl_.rs +++ b/src/pycell/impl_.rs @@ -248,6 +248,14 @@ pub trait PyClassObjectLayout: PyLayout { } #[doc(hidden)] +#[cfg_attr( + all(diagnostic_namespace), + diagnostic::on_unimplemented( + message = "the class layout is not valid", + label = "required for `#[pyclass(extends=...)]`", + note = "the python version being built against influences which layouts are valid", + ) +)] pub trait InternalPyClassObjectLayout: PyClassObjectLayout { /// Obtain a pointer to the contents of an uninitialized PyObject of this type /// Safety: the provided object must have the layout that the implementation is expecting @@ -551,6 +559,7 @@ impl InternalPyClassObjectLayout for PyVariableClassObject unsafe impl PyLayout for PyVariableClassObject {} +#[cfg(Py_3_12)] impl PyClassObjectLayout for PyVariableClassObject where ::LayoutAsBase: PyClassObjectLayout, diff --git a/src/pyclass/create_type_object.rs b/src/pyclass/create_type_object.rs index bcf4dffe2f4..f902f8fffa6 100644 --- a/src/pyclass/create_type_object.rs +++ b/src/pyclass/create_type_object.rs @@ -19,7 +19,7 @@ use std::{ collections::HashMap, ffi::{CStr, CString}, os::raw::{c_char, c_int, c_ulong, c_void}, - ptr, + ptr::{self, addr_of_mut}, }; pub(crate) struct PyClassTypeObject { @@ -260,7 +260,11 @@ impl PyTypeBuilder { } get_dict = get_dict_impl; - closure = dict_offset as _; + if let PyObjectOffset::Absolute(offset) = dict_offset { + closure = offset as _; + } else { + unreachable!("PyObjectOffset::Relative requires >=3.12"); + } } property_defs.push(ffi::PyGetSetDef { @@ -370,11 +374,16 @@ impl PyTypeBuilder { { #[inline(always)] fn offset_def(name: &'static CStr, offset: PyObjectOffset) -> ffi::PyMemberDef { - let (offset, is_relative) = offset.to_value_and_is_relative(); - let flags = if is_relative { - ffi::Py_READONLY | ffi::Py_RELATIVE_OFFSET - } else { - ffi::Py_READONLY + let (offset, flags) = match offset { + PyObjectOffset::Absolute(offset) => (offset, ffi::Py_READONLY), + #[cfg(Py_3_12)] + PyObjectOffset::Relative(offset) => { + (offset, ffi::Py_READONLY | ffi::Py_RELATIVE_OFFSET) + } + #[cfg(not(Py_3_12))] + PyObjectOffset::Relative(_) => { + panic!("relative offsets not valid before python 3.12"); + } }; ffi::PyMemberDef { name: name.as_ptr().cast(), @@ -414,16 +423,19 @@ impl PyTypeBuilder { Some(PyObjectOffset::Absolute(offset)) => { (*type_object).tp_dictoffset = offset; } - // PyObjectOffset::Relative requires >=3.12 - _ => {} + Some(PyObjectOffset::Relative(_)) => { + panic!("PyObjectOffset::Relative requires >=3.12") + } + None => {} } - match weaklist_offset { Some(PyObjectOffset::Absolute(offset)) => { (*type_object).tp_weaklistoffset = offset; } - // PyObjectOffset::Relative requires >=3.12 - _ => {} + Some(PyObjectOffset::Relative(_)) => { + panic!("PyObjectOffset::Relative requires >=3.12") + } + None => {} } })); } @@ -447,7 +459,7 @@ impl PyTypeBuilder { // Safety: self.tp_base must be a valid PyTypeObject let is_metaclass = - unsafe { ffi::PyType_IsSubtype(self.tp_base, &raw mut ffi::PyType_Type) } != 0; + unsafe { ffi::PyType_IsSubtype(self.tp_base, addr_of_mut!(ffi::PyType_Type)) } != 0; if is_metaclass { // if the pyclass derives from `type` (is a metaclass) then `tp_new` must not be set. // Metaclasses that override tp_new are not supported. diff --git a/src/type_object.rs b/src/type_object.rs index 87f33debcfa..aaa6431db07 100644 --- a/src/type_object.rs +++ b/src/type_object.rs @@ -2,7 +2,6 @@ use crate::ffi_ptr_ext::FfiPtrExt; use crate::impl_::pyclass::PyClassImpl; -use crate::pycell::impl_::InternalPyClassObjectLayout; use crate::types::any::PyAnyMethods; use crate::types::{PyAny, PyType}; use crate::{ffi, Bound, Python}; @@ -44,8 +43,9 @@ pub unsafe trait PyTypeInfo: Sized { /// Module name, if any. const MODULE: Option<&'static str>; - /// The type of object layout to use for ancestors or descendents of this type - type Layout: InternalPyClassObjectLayout; + /// The type of object layout to use for ancestors or descendants of this type. + /// should implement `InternalPyClassObjectLayout` in order to actually use it as a layout. + type Layout; /// Returns the PyTypeObject instance for this type. fn type_object_raw(py: Python<'_>) -> *mut ffi::PyTypeObject; diff --git a/tests/test_class_basics.rs b/tests/test_class_basics.rs index 41993f1e941..92afc2cfd1c 100644 --- a/tests/test_class_basics.rs +++ b/tests/test_class_basics.rs @@ -466,10 +466,9 @@ fn no_dunder_dict_support_setattr() { .setattr("a", 1) .unwrap_err() .to_string(); - assert_eq!( - &err, + assert!(err.contains( "AttributeError: 'builtins.NoDunderDictSupport' object has no attribute 'a'" - ); + )); }); } diff --git a/tests/test_compile_error.rs b/tests/test_compile_error.rs index 9b2b761d817..ff538788427 100644 --- a/tests/test_compile_error.rs +++ b/tests/test_compile_error.rs @@ -23,6 +23,9 @@ fn test_compile_errors() { t.compile_fail("tests/ui/reject_generics.rs"); t.compile_fail("tests/ui/deprecations.rs"); t.compile_fail("tests/ui/invalid_closure.rs"); + // only possible to extend variable sized types after 3.12 + #[cfg(not(Py_3_12))] + t.compile_fail("tests/ui/invalid_extend_variable_sized.rs"); t.compile_fail("tests/ui/pyclass_send.rs"); t.compile_fail("tests/ui/invalid_argument_attributes.rs"); t.compile_fail("tests/ui/invalid_intopy_derive.rs"); diff --git a/tests/test_inheritance.rs b/tests/test_inheritance.rs index 8483ac69812..d308cf1be06 100644 --- a/tests/test_inheritance.rs +++ b/tests/test_inheritance.rs @@ -200,11 +200,10 @@ mod inheriting_type { slf: Bound<'_, Metaclass>, _args: Bound<'_, PyTuple>, _kwargs: Option>, - ) -> PyResult<()> { + ) { let mut slf = slf.borrow_mut(); assert_eq!(slf.counter, 999); slf.counter = 5; - Ok(()) } fn __getitem__(&self, item: u64) -> u64 { @@ -283,7 +282,7 @@ mod inheriting_type { } #[test] - #[should_panic = "Metaclasses must specify __init__"] + #[should_panic(expected = "Metaclasses must specify __init__")] fn inherit_type_missing_init() { use pyo3::types::PyType; @@ -311,7 +310,7 @@ mod inheriting_type { } #[test] - #[should_panic = "Metaclasses must not specify __new__ (use __init__ instead)"] + #[should_panic(expected = "Metaclasses must not specify __new__ (use __init__ instead)")] fn inherit_type_with_new() { use pyo3::types::PyType; @@ -332,8 +331,7 @@ mod inheriting_type { _slf: Bound<'_, MetaclassWithNew>, _args: Bound<'_, PyTuple>, _kwargs: Option>, - ) -> PyResult<()> { - Ok(()) + ) { } } diff --git a/tests/ui/invalid_extend_variable_sized.rs b/tests/ui/invalid_extend_variable_sized.rs new file mode 100644 index 00000000000..1ebd04a1721 --- /dev/null +++ b/tests/ui/invalid_extend_variable_sized.rs @@ -0,0 +1,14 @@ +use pyo3::prelude::*; +use pyo3::types::{PyDict, PyTuple, PyType}; + +#[pyclass(extends=PyType)] +#[derive(Default)] +struct MyClass {} + +#[pymethods] +impl MyClass { + #[pyo3(signature = (*_args, **_kwargs))] + fn __init__(&mut self, _args: &Bound<'_, PyTuple>, _kwargs: Option<&Bound<'_, PyDict>>) {} +} + +fn main() {} diff --git a/tests/ui/invalid_extend_variable_sized.stderr b/tests/ui/invalid_extend_variable_sized.stderr new file mode 100644 index 00000000000..5acd7e732bf --- /dev/null +++ b/tests/ui/invalid_extend_variable_sized.stderr @@ -0,0 +1,15 @@ +error[E0277]: the class layout is not valid + --> tests/ui/invalid_extend_variable_sized.rs:4:1 + | +4 | #[pyclass(extends=PyType)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ required for `#[pyclass(extends=...)]` + | + = help: the trait `pyo3::pycell::impl_::InternalPyClassObjectLayout` is not implemented for `PyVariableClassObject` + = note: the python version being built against influences which layouts are valid + = help: the trait `pyo3::pycell::impl_::InternalPyClassObjectLayout` is implemented for `PyStaticClassObject` +note: required by a bound in `PyClassImpl::Layout` + --> src/impl_/pyclass.rs + | + | type Layout: InternalPyClassObjectLayout; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `PyClassImpl::Layout` + = note: this error originates in the attribute macro `pyclass` (in Nightly builds, run with -Z macro-backtrace for more info)