Make more constructors return &PyX instead of Py<PyX>

[birkenfeld]

fixes #405

Remaining one is PyTuple, where I'm running into a test failure. Will provide as a separate PR.

[birkenfeld]

Missing changelog entry; I'd like to combine this with the one for PySet when both are merged.

[konstin]

CC @pganssle for the datetime constructors

[pganssle]

The datetime constructors broke a bunch of tests. I have a half-completed branch somewhere that fixes the tests, but IIRC it was semi-blocked on #392.

@birkenfeld If you don't want to be bothered with fixing the datetime changes, I can handle them later.

[birkenfeld]

@pganssle I've found them now 😄

Do you know how to change the utcoffset method? Trying to make it return PyResult<&'p PyDelta> makes the proc-macro complain:

190 | #[pymethods]
    | ^^^^^^^^^^^^
    |
    = help: message: python method can not be generic: Ident { ident: "utcoffset", span: #0 bytes(4224..4233) }

The pyfunctions seem to be fine with it. Although, trying to import that module makes python3 segfault...

[kngwyu]

Do you know how to change the utcoffset method? Trying to make it return PyResult<&'p PyDelta> makes the proc-macro complain

Changing it to return Py<PyDelta> works but I think it's better to separate datetime fixes into an other PR.

[birkenfeld]

Sorry, can you be more specific what you want to separate? Without the datetime changes there isn't much left here...

[birkenfeld]

Also it's unclear to me how to get the Py from the reference 😄

[kngwyu]

Without the datetime changes there isn't much left here...

Yeah but there's many examples for datetime module and I thought it's tough stuff to fix all of them.
But spending after some time fixing this, I found it not too much difficult.

Also it's unclear to me how to get the Py from the reference

use pyo3::AsPyPointer;
#[pyfunction]
fn make_date(py: Python<'_>, year: i32, month: u8, day: u8) -> PyResult<Py<PyDate>> {
    PyDate::new(py, year, month, day).map(|d| unsafe { Py::from_borrowed_ptr(d.as_ptr()) })
}

This works but maybe we need an easier way.

[birkenfeld]

use pyo3::AsPyPointer;
#[pyfunction]
fn make_date(py: Python<'_>, year: i32, month: u8, day: u8) -> PyResult<Py<PyDate>> {
    PyDate::new(py, year, month, day).map(|d| unsafe { Py::from_borrowed_ptr(d.as_ptr()) })
}

This works but maybe we need an easier way.

Uh oh. Should unsafe really be required here?

[konstin]

I think we need a generic impl of that snippet:

pyo3/src/types/tuple.rs

Lines 137 to 141 in 874d8a0

	impl<'a> FromPy<&'a PyTuple> for Py<PyTuple> {
	fn from_py(tuple: &'a PyTuple, _py: Python) -> Py<PyTuple> {
	unsafe { Py::from_borrowed_ptr(tuple.as_ptr()) }
	}
	}

This would also avoid unsafe in downstream code

[konstin]

This seems to work:

impl<'a, T: AsPyPointer> FromPy<&'a T> for Py<T> {
    fn from_py(tuple: &'a T, _py: Python) -> Py<T> {
        unsafe { Py::from_borrowed_ptr(tuple.as_ptr()) }
    }
}

Might be good idea to investigate this for soundness and make another pull request of that.

[kngwyu]

@konstin
Yeah I agree.

I noticed we can return PyResult<&T> for some pyfunctions in the datetime example.
E.g.,

#[pyfunction]
fn make_date(py: Python, year: i32, month: u8, day: u8) -> PyResult<&PyDate> {
    PyDate::new(py, year, month, day)
}

But when we have &PyTzInfo as an argument, it doesn't work for the lifetime reason. We have to return Py<T> in datetime_from_timestamp and make_datetime.

#[pyfunction]
fn datetime_from_timestamp(py: Python, ts: f64, tz: Option<&PyTzInfo>) -> PyResult<&PyDateTime> {
    PyDateTime::from_timestamp(py, ts, tz)
}

I think this can be resolved by hacking proc-macro codes, but not sure.

[pganssle]

@kngwyu Why can't we use:

fn datetime_from_timestamp(py: Python<'p>,
                           ts: f64, tz: Option<&'p PyTzInfo>)
                        -> PyResult<&'p PyDateTime> {

The PyTzInfo just has to live as long as the PyDateTime or longer, so the annotation seems correct.

[kngwyu]

@pganssle
We can't use the lifetime specifier py there. It's the limitation of our proc macro codes.
So the compiler can't know python and tz has the same lifetime.

[kngwyu]

@pganssle
Hoops, sorry I noticed this works.

#[pyfunction]
fn datetime_from_timestamp<'py>(
    py: Python<'py>,
    ts: f64,
    tz: Option<&'py PyTzInfo>,
) -> PyResult<&'py PyDateTime> {
    PyDateTime::from_timestamp(py, ts, tz)
}

But now methods can't take lifetime specifiers so we can't compile

    fn utcoffset<'py>(&self, py: Python<'py>, _dt: &'py PyDateTime) -> PyResult<&'py PyDelta> {
        PyDelta::new(py, 0, 3600, 0, true)
    }

Edited
#461 make it possible to compile pymethods with lifetime specifiers

[codecov]

Codecov Report

Merging #446 into master will decrease coverage by 0.1%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #446      +/-   ##
==========================================
- Coverage   87.87%   87.76%   -0.11%     
==========================================
  Files          65       65              
  Lines        3398     3393       -5     
==========================================
- Hits         2986     2978       -8     
- Misses        412      415       +3

Impacted Files	Coverage Δ
src/types/string.rs	89.83% <100%> (ø)	⬆️
src/types/floatob.rs	95.23% <100%> (ø)	⬆️
src/types/datetime.rs	100% <100%> (ø)	⬆️
src/err.rs	61.21% <0%> (-1.22%)	⬇️
src/buffer.rs	69.9% <0%> (-0.49%)	⬇️
src/instance.rs	96.73% <0%> (-0.11%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74600f5...cdabf9e. Read the comment docs.

[codecov]

Codecov Report

Merging #446 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #446      +/-   ##
==========================================
- Coverage   87.87%   87.85%   -0.02%     
==========================================
  Files          65       65              
  Lines        3398     3393       -5     
==========================================
- Hits         2986     2981       -5     
  Misses        412      412

Impacted Files	Coverage Δ
src/types/string.rs	89.83% <100%> (ø)	⬆️
src/types/floatob.rs	95.23% <100%> (ø)	⬆️
src/types/datetime.rs	100% <100%> (ø)	⬆️
src/instance.rs	96.73% <0%> (-0.11%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74600f5...cdabf9e. Read the comment docs.

[birkenfeld]

Whoops, rustfmt fixed.

[konstin]

Thank you!