Add missing #[allow(unsafe_code)] attributes

[LilyFoote]

Fixes #4394.

[LilyFoote]

@davidhewitt Do we have a standard way to test this sort of thing? I also noticed we have some other unsafe usage without #[allow(unsafe_code)]. I'm happy to add those too if that's appropriate.

[davidhewitt]

Great question. From a quick try, it looks like adding #![forbid(unsafe_code)] to src/tests/hygiene/mod.rs will probably do the trick? We already try to cover all macros for hygiene there, so it's maybe a good way to cover them for unsafety at the same time :)

[Icxolu]

adding ui tests that should pass (for example i added pymodule_missing_docs a while ago) is also an option

[davidhewitt]

Looks good to me, thanks!

[EwoutH]

Thanks! When can we expect a new release which includes this bugfix (either 0.23 or a backport to 0.22.x)?

[davidhewitt]

I'm prepping 0.22.3 bugfix which includes this fix, to be released in the coming days.

[woodruffw]

Thank you for adding these attributes! FYI, they cause some changes to buildability for crates that disable unsafe at the top-level via forbid(unsafe_code), since Rust apparently doesn't distinguish between macro expansions and first-party code for lint attributes.

The fix (in my case) was to weaken forbid(...) to deny(unsafe_code), to allow the macro-expanded attributes to correctly override the top-level setting.

See woodruffw/pyrage@68d6617 for an example of the above.

[LilyFoote]

Yeah, I had to do that in this PR too (https://github.com/PyO3/pyo3/pull/4396/files#diff-2f6330ea73ca5013627fefc12704a0068483a52a99ccf39c781e89dc4ed7a4e1R3).

[mbuesch]

I'm prepping 0.22.3 bugfix which includes this fix,

Note that this seems to have broken the build for users that declare #[forbid(unsafe_code)] in their main.rs.
IMO this should have been a semantic versioning major bump.

error[E0453]: allow(unsafe_code) incompatible with previous forbid
  --> my.rs:33:1
   |
33 | / create_exception!(
**snip**
   |
  ::: main.rs:11:11
   |
11 |   #![forbid(unsafe_code)]
   |             ----------- `forbid` level set here
   |
   = note: this error originates in the macro `$crate::pyobject_native_type_named` which comes from the expansion of the macro `create_exception` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0453]: allow(unsafe_code) incompatible with previous forbid

[LilyFoote]

Does #[forbid(unsafe_code)] trigger with 0.22.2? If not, it feels really weird to me that us explicitly marking these uses of unsafe as allowed causes problems.

[mbuesch]

forbid means that the attribute cannot be allowed subsequently.
deny means that the attribute can be allowed subsequently.

Therefore, I changed my code to #[deny(unsafe_code)] and everything is fine.

It is Ok to have this kind of change in macros.
However, IMO it was an API change that required a major bump. I did not expect such a build breakage from a cargo update or a not---locked build.

Edit:
So the forbid-check doesn't seem to take crate-origin of the code into account whereas the unsafe-keyword check itself does.
That is a bit weird and inconsistent, indeed.

[davidhewitt]

Sorry that we broke builds here; that wasn't the intention, and I believed this to be a bugfix based on user reports that this lint was firing in their code.

Given that the lint wasn't firing for you despite the forbid, I can only assume we added this allow to places it's not needed, i.e. we added it to too many places.

Perhaps there is something we can do to adjust our testing methodology and get this partially reverted for 0.22.4.

[mbuesch]

Thanks for your helpful and quick answer.

Yes, it's a bit puzzling what's really going on. Maybe it depends on the Rust compiler version whether a lint triggers without this allow in the macros?
I use Rust 1.81.0.

Just to be sure I reverted my project back to pyo3 0.22.2 with the original #![forbid(unsafe_code)] in place.
It shows no warning in the compiler and clippy runs.

[Icxolu]

I did a bit of investigation with this reverted and the reproduction from #4394. The lint seems to trigger only on the
#[pyclass(get_all)] option and goes away if that is removed. The additional code for get_all is here

pyo3/pyo3-macros-backend/src/pymethod.rs

Lines 769 to 799 in 0079003

	let method_def = quote_spanned! {ty.span()=>
	#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>() +
	#pyo3_path::impl_::pyclass::offset_of!(#cls, #field)
	}
	}
	#[allow(unsafe_code)]
	const GENERATOR: #pyo3_path::impl_::pyclass::PyClassGetterGenerator::<
	#cls,
	#ty,
	Offset,
	{ #pyo3_path::impl_::pyclass::IsPyT::<#ty>::VALUE },
	{ #pyo3_path::impl_::pyclass::IsToPyObject::<#ty>::VALUE },
	{ #pyo3_path::impl_::pyclass::IsIntoPy::<#ty>::VALUE },
	{ #pyo3_path::impl_::pyclass::IsIntoPyObjectRef::<#ty>::VALUE },
	{ #pyo3_path::impl_::pyclass::IsIntoPyObject::<#ty>::VALUE },
	> = unsafe { #pyo3_path::impl_::pyclass::PyClassGetterGenerator::new() };
	#pyo3_path::impl_::pyclass::MaybeRuntimePyMethodDef::Runtime(
	|| GENERATOR.generate(#python_name, #doc)
	)
	}
	};

and in contrast to other emitted code with unsafe in it, this one has a Span. And if I remove that span, then the warning goes away with it. So maybe the correct solution is to make sure to only emit unsafe code without a Span?

But I'm not really sure why that works. Without an explicitly set Span, quote! uses the call_site span, which in my understanding should behave the same as if the generated code was written out manually, which is clearly not the case here...

[davidhewitt]

I've staged the revert onto #4575, so we will ship that when we get the other fixes done.