Skip to content

Conversation

davidhewitt
Copy link
Member

The constructors such as Py::from_owned_ptr are implemented on Py<T>. This is different to Bound and Borrowed where the implementation is only on Bound<'py, PyAny> and Borrowed<'a, 'py, PyAny>.

I would argue that implementing only on the PyAny type is better because it forces the caller to decide what casting logic to do - e.g. .cast(), .cast_exact(), or .cast_unchecked(). The Py<T> constructors basically all do .cast_unchecked().

Implementing this diff I found places in the codebase where the unchecked cast was probably not good (return value of PyNumber_Index), and I'd bet user code more likely makes the same mistake.

If we like moving forward with this, I think a better path than actually changing the constructors immediately would be to introduce new temporary names (Py::any_from_owned_ptr etc. maybe?) and deprecate the existing, and rename the new forms once the deprecated ones are removed.

We might want to also introduce proper .cast() methods to Py<T> similar to Bound::cast etc, to make the handling of the Py<PyAny> return values easier.

@Icxolu
Copy link
Contributor

Icxolu commented Oct 5, 2025

In general I would be in favor of this. Maybe we can even go a step further and just deprecate them, and tell people to use the Bound or Borrowed constructors instead and unbind in the end to get the Py, which I would say ist best practice anyway. It's not really longer to write and all the functionality is already there.

@davidhewitt
Copy link
Member Author

davidhewitt commented Oct 7, 2025

Hmm interesting, I hadn't really considered that possibility.

To be honest, temporary introduction of Py::any_from_owned_ptr etc. does seem pretty pointless if we'd know we'd want to rename it back to Py::from_owned_ptr eventually anyway, given we've got Bound and Borrowed constructors which are preferred as you say.

I'll come back to this for 0.28 I think 👍

@davidhewitt davidhewitt added this to the 0.28 milestone Oct 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants