From 79591f2161ae58c00ecc5eff5bfaf9d924647d7d Mon Sep 17 00:00:00 2001 From: Code Apprentice Date: Sun, 16 Jun 2024 04:23:03 -0600 Subject: [PATCH] Add error messages for unsupported macro features on compilation (#4194) * First implementation * tweak error message wording * Fix boolean logic * Remove redundant parens * Add test for weakref error * Fix test * Reword error message * Add expected error output * Rinse and repeat for `dict` * Add test output file * Ignore Rust Rover config files * cargo fmt * Add newsfragment * Update newsfragments/4194.added.md Co-authored-by: David Hewitt * Use ensure_spanned! macro Co-authored-by: David Hewitt * Use ensure_spanned! macro for weakref error, too Co-authored-by: David Hewitt * Revert "Ignore Rust Rover config files" This reverts commit 6c8a2eec581ed250ec792d8465772d649b0a3199. * Update wording for error message Co-authored-by: Icxolu <10486322+Icxolu@users.noreply.github.com> * Update weakref error message, too * Refactor constant to a pyversions module * Fix compiler errors * Another wording update Co-authored-by: David Hewitt * And make weakref wording the same * Fix compiler error due to using weakref in our own code * Fix after merge * apply conditional pyclass * update conditional compilation in tests --------- Co-authored-by: cojmeister Co-authored-by: David Hewitt Co-authored-by: Icxolu <10486322+Icxolu@users.noreply.github.com> --- newsfragments/4194.added.md | 1 + pyo3-macros-backend/src/lib.rs | 1 + pyo3-macros-backend/src/pyclass.rs | 36 +++++++++++----- pyo3-macros-backend/src/pyversions.rs | 3 ++ src/tests/hygiene/pyclass.rs | 13 +++++- tests/test_class_basics.rs | 50 +++++++++++++++++++++ tests/test_compile_error.rs | 4 ++ tests/test_unsendable_dict.rs | 49 --------------------- tests/test_various.rs | 62 ++++++++++++++------------- tests/ui/abi3_dict.rs | 7 +++ tests/ui/abi3_dict.stderr | 5 +++ tests/ui/abi3_weakref.rs | 7 +++ tests/ui/abi3_weakref.stderr | 5 +++ 13 files changed, 151 insertions(+), 92 deletions(-) create mode 100644 newsfragments/4194.added.md create mode 100644 pyo3-macros-backend/src/pyversions.rs delete mode 100644 tests/test_unsendable_dict.rs create mode 100644 tests/ui/abi3_dict.rs create mode 100644 tests/ui/abi3_dict.stderr create mode 100644 tests/ui/abi3_weakref.rs create mode 100644 tests/ui/abi3_weakref.stderr diff --git a/newsfragments/4194.added.md b/newsfragments/4194.added.md new file mode 100644 index 00000000000..6f032138d25 --- /dev/null +++ b/newsfragments/4194.added.md @@ -0,0 +1 @@ +Added error messages when using `weakref` or `dict` when compiling for `abi3` for Python older than 3.9 diff --git a/pyo3-macros-backend/src/lib.rs b/pyo3-macros-backend/src/lib.rs index a9d75a2a6fe..5d7437a4295 100644 --- a/pyo3-macros-backend/src/lib.rs +++ b/pyo3-macros-backend/src/lib.rs @@ -19,6 +19,7 @@ mod pyclass; mod pyfunction; mod pyimpl; mod pymethod; +mod pyversions; mod quotes; pub use frompyobject::build_derive_from_pyobject; diff --git a/pyo3-macros-backend/src/pyclass.rs b/pyo3-macros-backend/src/pyclass.rs index 403e5e8e9dc..9484b3dbf26 100644 --- a/pyo3-macros-backend/src/pyclass.rs +++ b/pyo3-macros-backend/src/pyclass.rs @@ -1,5 +1,12 @@ use std::borrow::Cow; +use proc_macro2::{Ident, Span, TokenStream}; +use quote::{format_ident, quote, quote_spanned}; +use syn::ext::IdentExt; +use syn::parse::{Parse, ParseStream}; +use syn::punctuated::Punctuated; +use syn::{parse_quote, parse_quote_spanned, spanned::Spanned, Result, Token}; + use crate::attributes::kw::frozen; use crate::attributes::{ self, kw, take_pyo3_options, CrateAttribute, ExtendsAttribute, FreelistAttribute, @@ -14,16 +21,9 @@ use crate::pymethod::{ impl_py_getter_def, impl_py_setter_def, MethodAndMethodDef, MethodAndSlotDef, PropertyType, SlotDef, __GETITEM__, __HASH__, __INT__, __LEN__, __REPR__, __RICHCMP__, }; -use crate::utils::Ctx; use crate::utils::{self, apply_renaming_rule, PythonDoc}; -use crate::PyFunctionOptions; -use proc_macro2::{Ident, Span, TokenStream}; -use quote::{format_ident, quote, quote_spanned}; -use syn::ext::IdentExt; -use syn::parse::{Parse, ParseStream}; -use syn::parse_quote_spanned; -use syn::punctuated::Punctuated; -use syn::{parse_quote, spanned::Spanned, Result, Token}; +use crate::utils::{is_abi3, Ctx}; +use crate::{pyversions, PyFunctionOptions}; /// If the class is derived from a Rust `struct` or `enum`. #[derive(Copy, Clone, Debug, PartialEq, Eq)] @@ -180,9 +180,17 @@ impl PyClassPyO3Options { }; } + let python_version = pyo3_build_config::get().version; + match option { PyClassPyO3Option::Crate(krate) => set_option!(krate), - PyClassPyO3Option::Dict(dict) => set_option!(dict), + PyClassPyO3Option::Dict(dict) => { + ensure_spanned!( + python_version >= pyversions::PY_3_9 || !is_abi3(), + dict.span() => "`dict` requires Python >= 3.9 when using the `abi3` feature" + ); + set_option!(dict); + } PyClassPyO3Option::Eq(eq) => set_option!(eq), PyClassPyO3Option::EqInt(eq_int) => set_option!(eq_int), PyClassPyO3Option::Extends(extends) => set_option!(extends), @@ -199,7 +207,13 @@ impl PyClassPyO3Options { PyClassPyO3Option::SetAll(set_all) => set_option!(set_all), PyClassPyO3Option::Subclass(subclass) => set_option!(subclass), PyClassPyO3Option::Unsendable(unsendable) => set_option!(unsendable), - PyClassPyO3Option::Weakref(weakref) => set_option!(weakref), + PyClassPyO3Option::Weakref(weakref) => { + ensure_spanned!( + python_version >= pyversions::PY_3_9 || !is_abi3(), + weakref.span() => "`weakref` requires Python >= 3.9 when using the `abi3` feature" + ); + set_option!(weakref); + } } Ok(()) } diff --git a/pyo3-macros-backend/src/pyversions.rs b/pyo3-macros-backend/src/pyversions.rs new file mode 100644 index 00000000000..23d25bf8cce --- /dev/null +++ b/pyo3-macros-backend/src/pyversions.rs @@ -0,0 +1,3 @@ +use pyo3_build_config::PythonVersion; + +pub const PY_3_9: PythonVersion = PythonVersion { major: 3, minor: 9 }; diff --git a/src/tests/hygiene/pyclass.rs b/src/tests/hygiene/pyclass.rs index 27a6b388769..8654e538728 100644 --- a/src/tests/hygiene/pyclass.rs +++ b/src/tests/hygiene/pyclass.rs @@ -10,15 +10,24 @@ pub struct Foo; #[pyo3(crate = "crate")] pub struct Foo2; -#[crate::pyclass( +#[cfg_attr(any(Py_3_9, not(Py_LIMITED_API)), crate::pyclass( name = "ActuallyBar", freelist = 8, + unsendable, + subclass, + extends = crate::types::PyAny, + module = "Spam", weakref, + dict +))] +#[cfg_attr(not(any(Py_3_9, not(Py_LIMITED_API))), crate::pyclass( + name = "ActuallyBar", + freelist = 8, unsendable, subclass, extends = crate::types::PyAny, module = "Spam" -)] +))] #[pyo3(crate = "crate")] pub struct Bar { #[pyo3(get, set)] diff --git a/tests/test_class_basics.rs b/tests/test_class_basics.rs index bc8d2dab275..19547cffba9 100644 --- a/tests/test_class_basics.rs +++ b/tests/test_class_basics.rs @@ -428,6 +428,7 @@ fn test_tuple_struct_class() { }); } +#[cfg(any(Py_3_9, not(Py_LIMITED_API)))] #[pyclass(dict, subclass)] struct DunderDictSupport { // Make sure that dict_offset runs with non-zero sized Self @@ -479,6 +480,7 @@ fn access_dunder_dict() { } // If the base class has dict support, child class also has dict +#[cfg(any(Py_3_9, not(Py_LIMITED_API)))] #[pyclass(extends=DunderDictSupport)] struct InheritDict { _value: usize, @@ -509,6 +511,7 @@ fn inherited_dict() { }); } +#[cfg(any(Py_3_9, not(Py_LIMITED_API)))] #[pyclass(weakref, dict)] struct WeakRefDunderDictSupport { // Make sure that weaklist_offset runs with non-zero sized Self @@ -534,6 +537,7 @@ fn weakref_dunder_dict_support() { }); } +#[cfg(any(Py_3_9, not(Py_LIMITED_API)))] #[pyclass(weakref, subclass)] struct WeakRefSupport { _pad: [u8; 32], @@ -559,6 +563,7 @@ fn weakref_support() { } // If the base class has weakref support, child class also has weakref. +#[cfg(any(Py_3_9, not(Py_LIMITED_API)))] #[pyclass(extends=WeakRefSupport)] struct InheritWeakRef { _value: usize, @@ -666,3 +671,48 @@ fn drop_unsendable_elsewhere() { capture.borrow_mut(py).uninstall(py); }); } + +#[test] +#[cfg(any(Py_3_9, not(Py_LIMITED_API)))] +fn test_unsendable_dict() { + #[pyclass(dict, unsendable)] + struct UnsendableDictClass {} + + #[pymethods] + impl UnsendableDictClass { + #[new] + fn new() -> Self { + UnsendableDictClass {} + } + } + + Python::with_gil(|py| { + let inst = Py::new(py, UnsendableDictClass {}).unwrap(); + py_run!(py, inst, "assert inst.__dict__ == {}"); + }); +} + +#[test] +#[cfg(any(Py_3_9, not(Py_LIMITED_API)))] +fn test_unsendable_dict_with_weakref() { + #[pyclass(dict, unsendable, weakref)] + struct UnsendableDictClassWithWeakRef {} + + #[pymethods] + impl UnsendableDictClassWithWeakRef { + #[new] + fn new() -> Self { + Self {} + } + } + + Python::with_gil(|py| { + let inst = Py::new(py, UnsendableDictClassWithWeakRef {}).unwrap(); + py_run!(py, inst, "assert inst.__dict__ == {}"); + py_run!( + py, + inst, + "import weakref; assert weakref.ref(inst)() is inst; inst.a = 1; assert inst.a == 1" + ); + }); +} diff --git a/tests/test_compile_error.rs b/tests/test_compile_error.rs index 2b32de2fcfa..c978e413d84 100644 --- a/tests/test_compile_error.rs +++ b/tests/test_compile_error.rs @@ -67,4 +67,8 @@ fn test_compile_errors() { #[cfg(all(Py_LIMITED_API, not(feature = "experimental-async")))] // output changes with async feature t.compile_fail("tests/ui/abi3_inheritance.rs"); + #[cfg(all(Py_LIMITED_API, not(Py_3_9)))] + t.compile_fail("tests/ui/abi3_weakref.rs"); + #[cfg(all(Py_LIMITED_API, not(Py_3_9)))] + t.compile_fail("tests/ui/abi3_dict.rs"); } diff --git a/tests/test_unsendable_dict.rs b/tests/test_unsendable_dict.rs deleted file mode 100644 index a39aa1ab714..00000000000 --- a/tests/test_unsendable_dict.rs +++ /dev/null @@ -1,49 +0,0 @@ -#![cfg(feature = "macros")] - -use pyo3::prelude::*; -use pyo3::py_run; - -#[pyclass(dict, unsendable)] -struct UnsendableDictClass {} - -#[pymethods] -impl UnsendableDictClass { - #[new] - fn new() -> Self { - UnsendableDictClass {} - } -} - -#[test] -#[cfg_attr(all(Py_LIMITED_API, not(Py_3_10)), ignore)] -fn test_unsendable_dict() { - Python::with_gil(|py| { - let inst = Py::new(py, UnsendableDictClass {}).unwrap(); - py_run!(py, inst, "assert inst.__dict__ == {}"); - }); -} - -#[pyclass(dict, unsendable, weakref)] -struct UnsendableDictClassWithWeakRef {} - -#[pymethods] -impl UnsendableDictClassWithWeakRef { - #[new] - fn new() -> Self { - Self {} - } -} - -#[test] -#[cfg_attr(all(Py_LIMITED_API, not(Py_3_10)), ignore)] -fn test_unsendable_dict_with_weakref() { - Python::with_gil(|py| { - let inst = Py::new(py, UnsendableDictClassWithWeakRef {}).unwrap(); - py_run!(py, inst, "assert inst.__dict__ == {}"); - py_run!( - py, - inst, - "import weakref; assert weakref.ref(inst)() is inst; inst.a = 1; assert inst.a == 1" - ); - }); -} diff --git a/tests/test_various.rs b/tests/test_various.rs index 0e619f49a28..dfc6498159c 100644 --- a/tests/test_various.rs +++ b/tests/test_various.rs @@ -2,7 +2,7 @@ use pyo3::prelude::*; use pyo3::py_run; -use pyo3::types::{PyDict, PyTuple}; +use pyo3::types::PyTuple; use std::fmt; @@ -113,39 +113,41 @@ fn pytuple_pyclass_iter() { }); } -#[pyclass(dict, module = "test_module")] -struct PickleSupport {} - -#[pymethods] -impl PickleSupport { - #[new] - fn new() -> PickleSupport { - PickleSupport {} +#[test] +#[cfg(any(Py_3_9, not(Py_LIMITED_API)))] +fn test_pickle() { + use pyo3::types::PyDict; + + #[pyclass(dict, module = "test_module")] + struct PickleSupport {} + + #[pymethods] + impl PickleSupport { + #[new] + fn new() -> PickleSupport { + PickleSupport {} + } + + pub fn __reduce__<'py>( + slf: &Bound<'py, Self>, + py: Python<'py>, + ) -> PyResult<(PyObject, Bound<'py, PyTuple>, PyObject)> { + let cls = slf.to_object(py).getattr(py, "__class__")?; + let dict = slf.to_object(py).getattr(py, "__dict__")?; + Ok((cls, PyTuple::empty_bound(py), dict)) + } } - pub fn __reduce__<'py>( - slf: &Bound<'py, Self>, - py: Python<'py>, - ) -> PyResult<(PyObject, Bound<'py, PyTuple>, PyObject)> { - let cls = slf.to_object(py).getattr(py, "__class__")?; - let dict = slf.to_object(py).getattr(py, "__dict__")?; - Ok((cls, PyTuple::empty_bound(py), dict)) + fn add_module(module: Bound<'_, PyModule>) -> PyResult<()> { + PyModule::import_bound(module.py(), "sys")? + .dict() + .get_item("modules") + .unwrap() + .unwrap() + .downcast::()? + .set_item(module.name()?, module) } -} -fn add_module(module: Bound<'_, PyModule>) -> PyResult<()> { - PyModule::import_bound(module.py(), "sys")? - .dict() - .get_item("modules") - .unwrap() - .unwrap() - .downcast::()? - .set_item(module.name()?, module) -} - -#[test] -#[cfg_attr(all(Py_LIMITED_API, not(Py_3_10)), ignore)] -fn test_pickle() { Python::with_gil(|py| { let module = PyModule::new_bound(py, "test_module").unwrap(); module.add_class::().unwrap(); diff --git a/tests/ui/abi3_dict.rs b/tests/ui/abi3_dict.rs new file mode 100644 index 00000000000..764a4d415a7 --- /dev/null +++ b/tests/ui/abi3_dict.rs @@ -0,0 +1,7 @@ +//! With abi3, dict not supported until python 3.9 or greater +use pyo3::prelude::*; + +#[pyclass(dict)] +struct TestClass {} + +fn main() {} diff --git a/tests/ui/abi3_dict.stderr b/tests/ui/abi3_dict.stderr new file mode 100644 index 00000000000..5887d5aa84a --- /dev/null +++ b/tests/ui/abi3_dict.stderr @@ -0,0 +1,5 @@ +error: `dict` requires Python >= 3.9 when using the `abi3` feature + --> tests/ui/abi3_dict.rs:4:11 + | +4 | #[pyclass(dict)] + | ^^^^ diff --git a/tests/ui/abi3_weakref.rs b/tests/ui/abi3_weakref.rs new file mode 100644 index 00000000000..f45b2258c96 --- /dev/null +++ b/tests/ui/abi3_weakref.rs @@ -0,0 +1,7 @@ +//! With abi3, weakref not supported until python 3.9 or greater +use pyo3::prelude::*; + +#[pyclass(weakref)] +struct TestClass {} + +fn main() {} diff --git a/tests/ui/abi3_weakref.stderr b/tests/ui/abi3_weakref.stderr new file mode 100644 index 00000000000..b8ef3936cdb --- /dev/null +++ b/tests/ui/abi3_weakref.stderr @@ -0,0 +1,5 @@ +error: `weakref` requires Python >= 3.9 when using the `abi3` feature + --> tests/ui/abi3_weakref.rs:4:11 + | +4 | #[pyclass(weakref)] + | ^^^^^^^