add gil-refs-migration feature

[davidhewitt]

Closes #3958

This PR implements the gil-refs-migration feature as proposed in that issue, and moves the FromPyObjectBound implementations to be gated on that feature.

This way users can migrate the rest of the Bound API deprecations without immediately being broken by the extract changes.

I also added open-by-default details tags to the 0.21 migration guide as per #3961.

[Icxolu]

Looks good! After #3959, this would only affect trait qualified syntax (FromPyObject::extract) which extract one of the borrowed types, right? I'm don't know how common that is, and if it warrants a separate feature, but I'm completely fine having it.

One disadvantage I could see is that something like this

let obj: Bound<'py, PyType> = py.get_type_bound::<PyList>();
let name: &'py str = obj.getattr("__name__")?.extract()?;

will still compile with gil-refs-migration enabled and then start failing once disabled. I guess that's the trade-off if we want to keep compile errors minimal while still enabling deprecation warnings.

[davidhewitt]

After #3959, this would only affect trait qualified syntax (FromPyObject::extract) which extract one of the borrowed types, right?

I think it's a little more than just the syntax, it's about delaying the breakages caused by the lifetime change for the FromPyObject -> FromPyObjectBound for &str and such to be the last thing that needs fixing rather than the first. I think that's easier for users to deal with: fix all the deprecations first and then adjust these snippets as a final step.

One disadvantage I could see is that something like this [...] I guess that's the trade-off if we want to keep compile errors minimal while still enabling deprecation warnings.

Yes, though I was hoping it's an advantage of this feature to allow the rest of the code to migrate first, rather than a disadvantage :)

[Icxolu]

breakages caused by the lifetime change for the FromPyObject -> FromPyObjectBound for &str and such

But these breakages do only occur once users actively migrating the code, right? Just turning off gil-refs does not have that effect immediately I think. By allowing this intermediate state, users have to touch each place twice, once for the initial migration and then once again to fix the lifetime issues after turning off gil-refs-migration, whereas they could probably do it in one go most of the time. But maybe that's not too bad and if this helps for migrating edge cases I'm fine either way.

[davidhewitt]

Just turning off gil-refs does not have that effect immediately I think.

Right I see, because now that #3959 is merged, not only does it allow &str extraction but because of the nature of the gil ref lifetime the extracted &str from &'py PyAny continues to have &'py str lifetime which avoids the temporaries 👍. I had missed that point previously, sorry 🤦

So the only break which this feature would now help avoid is users working with abi3 before 3.10, where the FromPyObjectBound for &str implementation cannot exist. But you're right it adds two touch points to the code.

So maybe this feature is indeed an overcomplication.

I had few other possible things I was wondering for this feature:

• At the moment we have not deprecated any methods like PyAny::get_item, which can also return GIL Refs. I was wondering if we should deprecate some (all?) of those methods, and I was wondering if we should mark those deprecations with not(feature = "gil-refs") or not(feature = "gil-refs-migration").
• We made breaking changes to intern! to return &Bound<'py, PyStr>. I was wondering if we should partially revert that and only break on not(feature = "gil-refs-migration").

I would say while those ideas might help users, they are not necessarily obvious or intuitive, so perhaps for the sake of simplicity we should back out from this.

[Icxolu]

I had missed that point previously, sorry

No worries 😄, I could have also just overlooked something as well.

• At the moment we have not deprecated any methods like PyAny::get_item, which can also return GIL Refs. I was wondering if we should deprecate some (all?) of those methods,

I while back I also though about if we should deprecate the inherent methods. Deprecating only the ones that return gil-refs feels a bit arbitrary to me, because they all operate on gil-refs and fundamentally are all deprecated. But actually marking all of them as deprecated will cause a lot of warning without immediate action, since resolving them requires the migration of the receiver and not the method itself. We have deprecated all the constructors and most of the macros (I think #[derive(FromPyObject)]s from_py_with is the only one that we currently don't have an idea for). If all of the ways to receive a gil-ref are deprecated that's probably enough.

Another option could maybe be to deprecate them under a rustc #cfg(...) flag, such that after fully migrating users can double check that there are really no gil-ref APIs lurking around somewhere. That way we don't have another feature, that could potentially left turned on by users (accidentally) and then break once we remove those features, and also don't flood everyones console with even more warnings by default.

• We made breaking changes to intern! to return &Bound<'py, PyStr>. I was wondering if we should partially revert that and only break on not(feature = "gil-refs-migration")

Is there a case that we reasonably could expect that to break anything? It looks like every method that I think makes sense to use it in conjunction with (...attr, ...item, call_method..., ...) is already generic in a way to be compatible either way. And for the (rare) case where it does break something, we could document in the migration guide to use .as_gil_ref() temporarily.

[davidhewitt]

Yes, I agree with all of the above. I think the case for this feature is not strong enough.

A #[cfg] just to check the gil-refs is an interesting idea. We don't need to block 0.21 final on that either as it'd be just an additive command line flag.