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

Py::new might not need to return PyResult #1081

Closed
davidhewitt opened this issue Aug 5, 2020 · 5 comments
Closed

Py::new might not need to return PyResult #1081

davidhewitt opened this issue Aug 5, 2020 · 5 comments

Comments

@davidhewitt
Copy link
Member

At the moment Py::new returns PyResult<Py<T>>, though in discussion in #1074 (comment) we came to concensus that it can probably just return Py<T>. The only error situation which can occur is out-of-memory. Most other pyo3 constructors where this is the only error situation just panic on OOM. (This is the same in the Rust stdlib.)

At the moment this fallibility comes down to PyClassAlloc::new returning nullptr on any error. And also PyClassAlloc::new can allocate a subtype, which could run arbitrary code through the subtype's tp_alloc.

If PyClassAlloc::new was split into e.g. ::new and ::new_subtype, where ::new always allocates Self, then we should be able to refactor code enough to change semantics of Py::new so that it panics on OOM.

pyo3/src/pycell.rs

Lines 374 to 377 in f2ba3e6

let base = T::new(py, subtype);
if base.is_null() {
return Err(PyErr::fetch(py));
}

@davidhewitt
Copy link
Member Author

It was interesting to note for pyclasses that x.into_py(py) is infallible, whereas Py::new(py, x) is fallible - which adds weight to the argument that Py::new doesn't need to return result.

@birkenfeld
Copy link
Member

Candidate for 0.12?

@davidhewitt
Copy link
Member Author

Would be cool but I'm slightly concerned the limited API may affect the code paths here, so I think we should coordinate this with the abi3 work in 0.13.

@davidhewitt davidhewitt added this to the 0.13 milestone Sep 8, 2020
@davidhewitt
Copy link
Member Author

Further thinking on this subject is that perhaps Rust types which subclass Python types could be fallible to construct, so this issue probably shouldn't be implemented before #991.

@davidhewitt davidhewitt removed this from the 0.13 milestone Dec 19, 2020
@davidhewitt
Copy link
Member Author

I think with #4041 we are now thinking that into-python conversions will likely be fallible in the future, and given the subclassing issue above I think the general conclusion is that this should stay fallible (as it currently is).

@davidhewitt davidhewitt closed this as not planned Won't fix, can't repro, duplicate, stale May 24, 2024
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

2 participants