Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fallible alternative to IntoPy? #1813

Closed
kszucs opened this issue Aug 18, 2021 · 6 comments
Closed

Fallible alternative to IntoPy? #1813

kszucs opened this issue Aug 18, 2021 · 6 comments

Comments

@kszucs
Copy link

kszucs commented Aug 18, 2021

While FromPyObject::extract() returns with a PyResult the IntoPy conversion trait returns with a PyObject making it hard to implement conversions which may return with an error.

Tried to manually raise an exception, but unable to propagate the correct error:

impl<'a> IntoPy<PyObject> for CustomType {
    fn into_py(self, py: Python) -> PyObject {
        match self.to_python(py) {
            Ok(obj) => obj,
            Err(err) => {
                err.restore(py);
                py.None() // SystemError: <built-in function to_python_raises> returned a result with an error set
                //panic!("error occured") => pyo3_runtime.PanicException: error occured
            }
        }
    }
}

Is there a workaround or plans to add support for a fallible IntoPy alternative?

A bit unrelated, but I'd like to mention that the current naming is a bit confusing, something like the following would better align with the rust conversion traits.

FromPy[Object]
IntoPy[Object]
TryFromPy[Object]
TryIntoPy[Object]
@davidhewitt
Copy link
Member

Yes, I absolutely agree that something should be done here.

I think the overall question is just about defining a better iteration of PyO3's conversion traits. We already have ToPyObject and IntoPy for the to-Python direction, and I think we should aim to start with a plan of what these conversions should look like in the future.

@elliottgorrell
Copy link

Hey @davidhewitt is there any update on this? It feels like a pretty large chink in the armour. Because if IntoPy panics python ends up with a PanicException which is more than likely going to bubble up uncaught and crash the python process as it is based off BaseException.

I could potentially help but curious if there is a fundamental reason this isn't fallible or hasn't been prioritised?

@davidhewitt
Copy link
Member

davidhewitt commented Feb 11, 2023

No update, simple answer is that I haven't had time to invest into this particular issue.

The challenges I see:

  • How many new traits do we need? Fallible IntoPy or ToPyObject could both be useful.
  • What is the migration path? E.g. IntoPy for HashMap probably needs to be fallible, but it might break a lot of code to replace it.
  • Do we make this easier by having blanket implementation of fallible conversion based on non-fallible conversion? Probably, but then it's not possible to retain the panicking fallible implementation for backwards compatibility.

I'm hoping the best way forward it to start an initiative to discuss pyo3's conversion traits in general and how to simplify them, either iteratively or in a big bang. Proposals are welcome, although I think to be worth the adoption pain for our users we need to be reasonably sure that a new trait system would be be robust enough to be stable for a long time and have an update pathway which is easy to document.

@SuperJappie08
Copy link
Contributor

My project could use this feature (TryIntoPy trait).

I have a suggestion on how to implement this:

  • Create TryIntoPy trait, IntoPy<T> but returning a PyResult<T>
  • Replace most IntoPy trait bounds with a TryIntoPy
  • Probably need to Keep the IntoPy<T> bounds for IntoPy<T> implementers, which have fields.
  • Create a blanket implementation for TryIntoPy for all IntoPy implementors.
    Which could look something like this
impl<Target, Value> TryIntoPy<Target> for Value where Value: IntoPy<Target> {
    fn try_into_py(self, py: Python<'_>) -> PyResult<Target> {
        Ok(self.into_py(py))
    }

    #[cfg(feature = "experimental-inspect")]
    fn type_output() -> TypeInfo {
        IntoPy<Target>::<Value>::type_output()
    }
}

I think this could work for IntoPy, I think it would also be mostly backwards compatible.
I have not looked into ToPyObject much yet, however I already found some implementations with expect which could panic in the sets.

impl<T, S> ToPyObject for collections::HashSet<T, S>
where
T: hash::Hash + Eq + ToPyObject,
S: hash::BuildHasher + Default,
{
fn to_object(&self, py: Python<'_>) -> PyObject {
new_from_iter(py, self)
.expect("Failed to create Python set from HashSet")
.into()
}
}

Those might need to be moved to the TryToPyObject, it depends if then panic only happens in certain circumstances, it might be fine if the panic is not resolvable by the end-user. Otherwise, it could also just return a PyResult.

@SuperJappie08
Copy link
Contributor

I have tried this, but the blanket implementation does not work, since it will not allow for infallible versions of things with fields like, PyDict, which should have a TryIntoPy and also a IntoPy when possible.

@davidhewitt
Copy link
Member

Solved by #4060

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants