deprecate gil-refs in from_py_with

[Icxolu]

This deprecates the use of gil-refs in from_py_with. This PR implements it only for #[pyfunction] arguments. While it would be easily possible to generate the same code for #[derive(FromPyObject)] the warning gets eaten by #[automatically_derived]. I don't see a good way around that, but I'm happy to adapt if someone has an idea.

[davidhewitt]

Ah, brilliant! I hadn't even gotten around to thinking about this one yet. I have a few thoughts...

Regarding the #[derive(FromPyObject)] case, I suppose it'd be possible to spit out some code in an anonymous constant outside of the #[automatically_derived]?

Something like

const _: () = {
    let (_, e) = inspect_fn(function_to_inspect);
    e.extract_from_py_with();
}

If those can be const fn, that's probably sufficient to trigger the warnings? Probably worth deferring that to a separate PR as threading that code through to the top level might be painful.

[davidhewitt]

Suggested change

	note = "use `&Bound<'_, PyAny>` as argument for this `from_py_with` extractor"
	note = "use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor"

[davidhewitt]

Smart! In #3847 (which I'm slowing making progress on) I recently found that it's also possible to take the Extractor as an argument by reference:

Suggested change

	#[allow(clippy::type_complexity)]
	pub fn inspect_fn<A, T>(f: fn(A) -> PyResult<T>) -> (fn(A) -> PyResult<T>, Extractor<A>) {
	(f, Extractor::new())
	}
	#[allow(clippy::type_complexity)]
	pub fn inspect_fn<A, T>(f: fn(A) -> PyResult<T>, _: &Extractor<A>) -> fn(A) -> PyResult<T> {
	f
	}

The nice thing about this is that the dance in the macro code gets simpler. Instead of:

let extractor;
let (thing, e) = inspect_fn(thing);
extractor = e;

can just write:

let extractor = Extractor::new();
let thing = inspect_fn(thing, &e);

So far this seems to be working for me for type inference just as well as the original form and seems to be easier to fit into macro code paths, I wonder if it will also simplify the return type and make clippy happier.

[Icxolu]

Interesting! That does look nicer and easier to work with. In this particular case it does not make too much of a difference, because I did not need that empty binding, but it does not hurt either (and it makes clippy indeed happier)

[Icxolu]

Regarding the #[derive(FromPyObject)] case, I suppose it'd be possible to spit out some code in an anonymous constant outside of the #[automatically_derived]?

Deref coercion is not allowed in consts 😢

[davidhewitt]

Looks good to me, thanks! Let's merge to resolve possible conflicts with #3968

[davidhewitt]

Deref coercion is not allowed in consts 😢

Ah 😢. I wonder if there's some dirty hack like packaging into a non-const function, given this code doesn't need to run:

const _: () = {
    fn check_from_py_with() {
        let e = pyo3::impl_::pymethods::Extractor::new();
        inspect_fn(function_to_inspect, &e);
        e.extract_from_py_with();
    }
}

... from a quick test in the playground, this sort of structure can still trigger deprecation warnings: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=fac5238c3e4fdd1ce0da0b2209a64b6c

(and because each anonymous constant block is its own scope, all these functions can share a common name, I think they won't conflict.)

[Icxolu]

Ah 😢. I wonder if there's some dirty hack like packaging into a non-const function, given this code doesn't need to run:

Hmm, it seems like this only works if the constant (anonymous or not) lives in the source itself, if it is generated by the macro it does not trigger. Are these const generated by proc macros handled specially by the compiler 🤔

[davidhewitt]

Eurgh, I have no idea 😂

I guess unless we find new tricks, we have to accept that we can't emit a deprecation warning for that case.

[Icxolu]

Hmm, it seems like this only works if the constant (anonymous or not) lives in the source itself, if it is generated by the macro it does not trigger.

I actually got it to work 🎊 I forgot to set a span for the const _: () = {} and it seems like the compiler just throws the warning in the void in that case. I kind of expected it to show on the derive but I guess that's not how it works. I'll prepare the PR with the followup.

[davidhewitt]

Great!