feature gate PyCell
#4177
feature gate PyCell
#4177
Conversation
src/type_object.rs
Outdated
| unsafe impl<T> HasPyGilRef for T | ||
| where | ||
| T: PyNativeType, | ||
| { | ||
| type AsRefTarget = Self; | ||
| } | ||
|
|
||
| #[cfg(not(feature = "gil-refs"))] | ||
| unsafe impl<T> HasPyGilRef for T {} | ||
|
|
There was a problem hiding this comment.
Hmm, is this right? Does it cause any soundness issues if its implemented for everything?
There was a problem hiding this comment.
The idea was, that implementing it for everything should be functionally equivalent to removing the requirement. There are no APIs left (at least I could find any) that have HasPyGilRef as a direct bound without gil-refs. So this blanket is only there to satisfy the requirement for PyTypeCheck and PyTypeInfo, which currently use it as a supertrait.
But someone please double check that this has no other implication 🙃
There was a problem hiding this comment.
So is this temporary, i.e. will we feature-gate the whole trait and its usage as a super trait later on?
There was a problem hiding this comment.
I was thinking temporary until we fully remove the gil-refs in 0.23, at which point we can also remove the trait and the supertrait requirement.
There was a problem hiding this comment.
Couldn't we already feature gate the trait (and bounds) in 0.22 based on the feature?
There was a problem hiding this comment.
I think all the bounds are already gated. We could also gate the whole trait, but then we have to duplicate the definitions of PyTypeCheck and PyTypeInfo to conditionally include the supertrait. (Or we could could introduce a helper trait in the middle to bridge, but I think that is more or less I've done here, just with a different name)
There was a problem hiding this comment.
We could also gate the whole trait, but then we have to duplicate the definitions of PyTypeCheck and PyTypeInfo to conditionally include the supertrait.
I think I would prefer that (we could use a small local helper macro like we had for the const thread_local! syntax. Personally, I would find that less weird than the universal trait impl. I do see any unintended side effects though, so this appears to be purely a matter of preference to me.
There was a problem hiding this comment.
Sure thing, PyTypeCheck and PyTypeInfo are actually not that big, so introducing a macro for them is maybe a bit overkill and maybe having them duplicated until 0.22 is out is not such big of a deal.
src/instance.rs
Outdated
| Python::with_gil(|py| { | ||
| use crate::types::PyAnyMethods; | ||
| let obj = self.bind(py).as_any(); | ||
| match obj.str() { | ||
| Ok(s) => return f.write_str(&s.to_string_lossy()), | ||
| Err(err) => err.write_unraisable_bound(py, Some(obj)), | ||
| } | ||
|
|
||
| match obj.get_type().name() { | ||
| Ok(name) => write!(f, "<unprintable {} object>", name), | ||
| Err(_err) => f.write_str("<unprintable object>"), | ||
| } | ||
| }) |
There was a problem hiding this comment.
Did this implementation need expanding? It looks like it already went through Bound...
There was a problem hiding this comment.
Ah, good catch, we can just drop the AsRefTarget bound 👍
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
Part of #3960
This moves the deprecated
PyCelland its accompanying APIs behind thegil-refsfeature gate.pyo3-macros-backendPyNativeTypefeature gateHasPyGilRef::AsRefTarget(the trait can be fully removed once thegil-refsfeature is gone)HasPyGilRefPyCell