From 9a641f7380a92c1a94190a319a903c8f40045ded Mon Sep 17 00:00:00 2001 From: Icxolu <10486322+Icxolu@users.noreply.github.com> Date: Wed, 25 Sep 2024 23:12:58 +0200 Subject: [PATCH] fix unintentional `unsafe_code` trigger (#4574) * Revert "Add missing #[allow(unsafe_code)] attributes (#4396)" This reverts commit 0e03b39caa18d016a6951307b297ca7e6f999e24. * fix unintentional `unsafe_code` trigger * add newsfragment --- newsfragments/4574.fixed.md | 2 ++ pyo3-macros-backend/src/pyclass.rs | 3 +-- pyo3-macros-backend/src/pymethod.rs | 15 +++++++++------ src/exceptions.rs | 1 - src/impl_/pyclass.rs | 6 +----- src/macros.rs | 1 - src/tests/hygiene/mod.rs | 1 - src/types/mod.rs | 7 ------- tests/test_compile_error.rs | 2 ++ tests/ui/forbid_unsafe.rs | 30 +++++++++++++++++++++++++++++ 10 files changed, 45 insertions(+), 23 deletions(-) create mode 100644 newsfragments/4574.fixed.md create mode 100644 tests/ui/forbid_unsafe.rs diff --git a/newsfragments/4574.fixed.md b/newsfragments/4574.fixed.md new file mode 100644 index 00000000000..c996e927289 --- /dev/null +++ b/newsfragments/4574.fixed.md @@ -0,0 +1,2 @@ +Fixes `#[forbid(unsafe_code)]` regression by reverting #4396. +Fixes unintentional `unsafe_code` trigger by adjusting macro hygiene. \ No newline at end of file diff --git a/pyo3-macros-backend/src/pyclass.rs b/pyo3-macros-backend/src/pyclass.rs index 2dd92dbe460..cbb731f089f 100644 --- a/pyo3-macros-backend/src/pyclass.rs +++ b/pyo3-macros-backend/src/pyclass.rs @@ -1659,7 +1659,7 @@ fn impl_pytypeinfo( #[cfg(feature = "gil-refs")] let has_py_gil_ref = quote! { - #[allow(deprecated, unsafe_code)] + #[allow(deprecated)] unsafe impl #pyo3_path::type_object::HasPyGilRef for #cls { type AsRefTarget = #pyo3_path::PyCell; } @@ -1671,7 +1671,6 @@ fn impl_pytypeinfo( quote! { #has_py_gil_ref - #[allow(unsafe_code)] unsafe impl #pyo3_path::type_object::PyTypeInfo for #cls { const NAME: &'static str = #cls_name; const MODULE: ::std::option::Option<&'static str> = #module; diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index 861b24c07c0..2672c6a07ac 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -781,14 +781,20 @@ pub fn impl_py_getter_def( // TODO: on MSRV 1.77+, we can use `::std::mem::offset_of!` here, and it should // make it possible for the `MaybeRuntimePyMethodDef` to be a `Static` variant. - let method_def = quote_spanned! {ty.span()=> + let generator = quote_spanned! { ty.span() => + #pyo3_path::impl_::pyclass::MaybeRuntimePyMethodDef::Runtime( + || GENERATOR.generate(#python_name, #doc) + ) + }; + // This is separate so that the unsafe below does not inherit the span and thus does not + // trigger the `unsafe_code` lint + let method_def = quote! { #cfg_attrs { #[allow(unused_imports)] // might not be used if all probes are positve use #pyo3_path::impl_::pyclass::Probe; struct Offset; - #[allow(unsafe_code)] unsafe impl #pyo3_path::impl_::pyclass::OffsetCalculator<#cls, #ty> for Offset { fn offset() -> usize { #pyo3_path::impl_::pyclass::class_offset::<#cls>() + @@ -796,7 +802,6 @@ pub fn impl_py_getter_def( } } - #[allow(unsafe_code)] const GENERATOR: #pyo3_path::impl_::pyclass::PyClassGetterGenerator::< #cls, #ty, @@ -804,9 +809,7 @@ pub fn impl_py_getter_def( { #pyo3_path::impl_::pyclass::IsPyT::<#ty>::VALUE }, { #pyo3_path::impl_::pyclass::IsToPyObject::<#ty>::VALUE }, > = unsafe { #pyo3_path::impl_::pyclass::PyClassGetterGenerator::new() }; - #pyo3_path::impl_::pyclass::MaybeRuntimePyMethodDef::Runtime( - || GENERATOR.generate(#python_name, #doc) - ) + #generator } }; diff --git a/src/exceptions.rs b/src/exceptions.rs index ecf6e54b165..496d614fc20 100644 --- a/src/exceptions.rs +++ b/src/exceptions.rs @@ -34,7 +34,6 @@ macro_rules! impl_exception_boilerplate { #[cfg(feature = "gil-refs")] impl ::std::error::Error for $name { fn source(&self) -> ::std::option::Option<&(dyn ::std::error::Error + 'static)> { - #[allow(unsafe_code)] unsafe { #[allow(deprecated)] let cause: &$crate::exceptions::PyBaseException = self diff --git a/src/impl_/pyclass.rs b/src/impl_/pyclass.rs index 8e978aff8fb..8fc0ec2341e 100644 --- a/src/impl_/pyclass.rs +++ b/src/impl_/pyclass.rs @@ -353,7 +353,6 @@ slot_fragment_trait! { #[macro_export] macro_rules! generate_pyclass_getattro_slot { ($cls:ty) => {{ - #[allow(unsafe_code)] unsafe extern "C" fn __wrap( _slf: *mut $crate::ffi::PyObject, attr: *mut $crate::ffi::PyObject, @@ -437,7 +436,6 @@ macro_rules! define_pyclass_setattr_slot { #[macro_export] macro_rules! $generate_macro { ($cls:ty) => {{ - #[allow(unsafe_code)] unsafe extern "C" fn __wrap( _slf: *mut $crate::ffi::PyObject, attr: *mut $crate::ffi::PyObject, @@ -554,7 +552,6 @@ macro_rules! define_pyclass_binary_operator_slot { #[macro_export] macro_rules! $generate_macro { ($cls:ty) => {{ - #[allow(unsafe_code)] unsafe extern "C" fn __wrap( _slf: *mut $crate::ffi::PyObject, _other: *mut $crate::ffi::PyObject, @@ -747,7 +744,6 @@ slot_fragment_trait! { #[macro_export] macro_rules! generate_pyclass_pow_slot { ($cls:ty) => {{ - #[allow(unsafe_code)] unsafe extern "C" fn __wrap( _slf: *mut $crate::ffi::PyObject, _other: *mut $crate::ffi::PyObject, @@ -872,7 +868,7 @@ macro_rules! generate_pyclass_richcompare_slot { ($cls:ty) => {{ #[allow(unknown_lints, non_local_definitions)] impl $cls { - #[allow(non_snake_case, unsafe_code)] + #[allow(non_snake_case)] unsafe extern "C" fn __pymethod___richcmp____( slf: *mut $crate::ffi::PyObject, other: *mut $crate::ffi::PyObject, diff --git a/src/macros.rs b/src/macros.rs index 0c2846de8a3..ab91d577ac5 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -207,7 +207,6 @@ macro_rules! wrap_pymodule { #[macro_export] macro_rules! append_to_inittab { ($module:ident) => { - #[allow(unsafe_code)] unsafe { if $crate::ffi::Py_IsInitialized() != 0 { ::std::panic!( diff --git a/src/tests/hygiene/mod.rs b/src/tests/hygiene/mod.rs index 9bf89161b24..c950e18da94 100644 --- a/src/tests/hygiene/mod.rs +++ b/src/tests/hygiene/mod.rs @@ -1,6 +1,5 @@ #![no_implicit_prelude] #![allow(dead_code, unused_variables, clippy::unnecessary_wraps)] -#![deny(unsafe_code)] // The modules in this test are used to check PyO3 macro expansion is hygienic. By locating the test // inside the crate the global `::pyo3` namespace is not available, so in combination with diff --git a/src/types/mod.rs b/src/types/mod.rs index 8fa3807f3f1..3cf257c80f2 100644 --- a/src/types/mod.rs +++ b/src/types/mod.rs @@ -124,7 +124,6 @@ pub trait DerefToPyAny { macro_rules! pyobject_native_type_base( ($name:ty $(;$generics:ident)* ) => { #[cfg(feature = "gil-refs")] - #[allow(unsafe_code)] unsafe impl<$($generics,)*> $crate::PyNativeType for $name { type AsRefSource = Self; } @@ -163,7 +162,6 @@ macro_rules! pyobject_native_type_base( { #[inline] fn to_object(&self, py: $crate::Python<'_>) -> $crate::PyObject { - #[allow(unsafe_code)] unsafe { $crate::PyObject::from_borrowed_ptr(py, self.as_ptr()) } } } @@ -194,7 +192,6 @@ macro_rules! pyobject_native_type_named ( } } - #[allow(unsafe_code)] unsafe impl<$($generics,)*> $crate::AsPyPointer for $name { /// Gets the underlying FFI pointer, returns a borrowed pointer. #[inline] @@ -209,7 +206,6 @@ macro_rules! pyobject_native_type_named ( impl<$($generics,)*> $crate::IntoPy<$crate::Py<$name>> for &'_ $name { #[inline] fn into_py(self, py: $crate::Python<'_>) -> $crate::Py<$name> { - #[allow(unsafe_code)] unsafe { $crate::Py::from_borrowed_ptr(py, self.as_ptr()) } } } @@ -221,7 +217,6 @@ macro_rules! pyobject_native_type_named ( #[inline] fn from(other: &$name) -> Self { use $crate::PyNativeType; - #[allow(unsafe_code)] unsafe { $crate::Py::from_borrowed_ptr(other.py(), other.as_ptr()) } } } @@ -231,7 +226,6 @@ macro_rules! pyobject_native_type_named ( #[cfg(feature = "gil-refs")] impl<'a, $($generics,)*> ::std::convert::From<&'a $name> for &'a $crate::PyAny { fn from(ob: &'a $name) -> Self { - #[allow(unsafe_code)] unsafe{&*(ob as *const $name as *const $crate::PyAny)} } } @@ -255,7 +249,6 @@ macro_rules! pyobject_native_static_type_object( #[macro_export] macro_rules! pyobject_native_type_info( ($name:ty, $typeobject:expr, $module:expr $(, #checkfunction=$checkfunction:path)? $(;$generics:ident)*) => { - #[allow(unsafe_code)] unsafe impl<$($generics,)*> $crate::type_object::PyTypeInfo for $name { const NAME: &'static str = stringify!($name); const MODULE: ::std::option::Option<&'static str> = $module; diff --git a/tests/test_compile_error.rs b/tests/test_compile_error.rs index 0312fa6dd3a..e51ca64912f 100644 --- a/tests/test_compile_error.rs +++ b/tests/test_compile_error.rs @@ -63,6 +63,8 @@ fn test_compile_errors() { #[cfg(any(not(Py_LIMITED_API), Py_3_10))] // to avoid PyFunctionArgument for &str t.compile_fail("tests/ui/invalid_cancel_handle.rs"); t.pass("tests/ui/pymodule_missing_docs.rs"); + #[cfg(not(Py_LIMITED_API))] + t.pass("tests/ui/forbid_unsafe.rs"); #[cfg(all(Py_LIMITED_API, not(feature = "experimental-async")))] // output changes with async feature t.compile_fail("tests/ui/abi3_inheritance.rs"); diff --git a/tests/ui/forbid_unsafe.rs b/tests/ui/forbid_unsafe.rs new file mode 100644 index 00000000000..9b62886b650 --- /dev/null +++ b/tests/ui/forbid_unsafe.rs @@ -0,0 +1,30 @@ +#![forbid(unsafe_code)] + +use pyo3::*; + +#[allow(unexpected_cfgs)] +#[path = "../../src/tests/hygiene/mod.rs"] +mod hygiene; + +mod gh_4394 { + use pyo3::prelude::*; + + #[derive(Eq, Ord, PartialEq, PartialOrd, Clone)] + #[pyclass(get_all)] + pub struct VersionSpecifier { + pub(crate) operator: Operator, + pub(crate) version: Version, + } + + #[derive(Eq, Ord, PartialEq, PartialOrd, Debug, Hash, Clone, Copy)] + #[pyo3::pyclass(eq, eq_int)] + pub enum Operator { + Equal, + } + + #[derive(Clone, Eq, PartialEq, PartialOrd, Ord)] + #[pyclass] + pub struct Version; +} + +fn main() {}