fix unintentional unsafe_code trigger

[Icxolu]

Another attempt at fixing #4394.

This reverts #4396 (can also be done separately first) as it does not play nicely #[forbid(unsafe_code)].

The root cause seemed to be that the unsafe code generated for the get_all option inherited a Span. This changes that, so that only the part needed for diagnostics inherits the Span an the rest gets the default call_site span.

I added a pass ui tests, with the repro from #4394, to tests against this. It probably does not cover every emitted unsafe, but we can probably extend it in the future is other cases pop up. Reusing the hygiene tests for this did not work, probably because they are part of the PyO3 crate and thus the spans/macro hygiene is different than for external crates...

[Icxolu]

Thanks for the review! I'm going to leave this open for now to give the others a chance on some feedback on whether we all agree that this is the correct fix.

[davidhewitt]

Ah, that makes a lot of sense. Thank you for diving in. I think I will cherry-pick this fix onto a 0.22.4 patch (even if the build break has now already happened).

Making a UI test is a smart way to match downstream user experience. 👍

Just one thought, which might help us re-use stuff... 🤔

[davidhewitt]

I wonder, does it make sense to include all the hygiene tests in here?

Something like

use pyo3::*;  // to make this `crate` match PyO3's `crate` used in the hygiene tests

#[path = "../../src/tests/hygiene/mod.rs"]
mod hygiene;

[Icxolu]

Very smart! This can't test the cfg variants in there, but I think that's fine, so I simply allowed unexpected_cfgs for that.

[davidhewitt]

Great!

[davidhewitt]

Ah, but the abi3 tests have some issues 😭

[davidhewitt]

Ah, but the abi3 tests have some issues 😭

[Icxolu]

Oops, I guess we just gate the test?

[davidhewitt]

Maybe split the test into two; run the bit I suggested in a separate (gated) test? Seems a shame to have no coverage under abi3... 🤔

(I had a look at trybuild docs for build.rs / cfg support inside tests themselves, but I couldn't see anything...)

[davidhewitt]

Ah nvm you already just gated, I think that's good enough 👍

[Icxolu]

(I had a look at trybuild docs for build.rs / cfg support inside tests themselves, but I couldn't see anything...)

Yeah, it's declared out of scope for trybuild dtolnay/trybuild#206