Pyo3: Python::assume_gil_acquired() can infer static lifetime.

[davidhewitt]

As per the comment on #797 we should probably remove Python::assume_gil_acquired(), because it can give you a Python<'static> thanks to the lifetime inference. This is almost certainly unsound.

I think it probably explains this crash #653

See this playground for a minimal example: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=213173005786e15199cf0312079c3d72

A function assume_gil_acquired() would still be useful, but I think it needs to return a type without a lifetime, so that there is no risk of incorrect inference. For example, something like this:

struct AssumeGILAcquired(Unsendable);

fn assume_gil_acquired() -> AssumeGilAcquired { ... }

impl AssumeGILAcquired {
    fn py(&self) -> Python { ... }
}

/// later on:
let gil = assume_gil_acquired();
let py = gil.py();

In the above example, the Python lifetime is tied exactly to the lifetime of the AssumeGILAcquired token, so we can avoid 'static lifetime problems.

[kngwyu]

Internally, we have lots of codes like

        let py = Python::assume_gil_acquired();
        let _pool = gil::GILPool::new_no_pointers(py);

In such cases, py has to have the same lifetime as _pool, so a possible fix is

struct GILPool { ... }
let _pool = GILPool::new();
let py = _pool.py(); // the existance of GILPool already assumes GIL is hold

[kngwyu]

For now, could I open a PR that adds GILPool::py?
I'm not sure we need AssumeGILAcquired or not, but they can coexist.
But I'm afraid the change would conflict with #797.

[davidhewitt]

For now, could I open a PR that adds GILPool::py?

Sure thing, I'd be happy to see the unsoundness fixed sooner rather than later.

Don't worry about #797. I'll fix it up when I get a chance; I've been quite busy recently. When I have some time again I was planning to first help with bugfixes for 0.9 before we begin making breaking changes like #797 for an 0.10 release

[kngwyu]

Ooops, because of the lifetime problem, it looks like we need the same trick as #797 to use GILPool::py. So maybe it would be better to merge it first.

[davidhewitt]

Which trick from #797 do you mean? struct AssumeGILAcquired ?

[kngwyu]

I mean the changes of CallbackConverter.

[davidhewitt]

Ok. I'll try to start figuring #797 again soon. Maybe next week. I think there are still more bugfixes we could do for 0.9.x before we start merging breaking changes for 0.10.

[kngwyu]

Thank you. We don't have to be so upset, but please let me complete the PR if you were still busy next week.

I think there are still more bugfixes we could do for 0.9.x

Since #839 has some breaking changes the next version should be 0.10 if we apply semver explicitly.

[davidhewitt]

Since #839 has some breaking changes the next version should be 0.10 if we apply semver explicitly.

That's very true. Ok I will do my best to find time soon to add #797 ready for 0.10 😄

[davidhewitt]

Now that #858 is merged I guess that GILPool::py should be possible again 🚀

[davidhewitt]

Now that we have GILPool::py() and don't make incorrect 'static lifetimes internally, and also have the safety note on assume_gil_acquired, I think there is no further work to do here. 🎉