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

Porting to 0.9 after #770 #785

Closed
birkenfeld opened this issue Mar 3, 2020 · 7 comments · Fixed by #795
Closed

Porting to 0.9 after #770 #785

birkenfeld opened this issue Mar 3, 2020 · 7 comments · Fixed by #795
Milestone

Comments

@birkenfeld
Copy link
Member

Two points (Using pyo3 master branch from today, Rust 1.43.0 nightly from 2020-02-22), both should be noted in the release notes.

first

After #770, I've noticed that I can't use &Class and &mut Class as method arguments anymore, where Class is a #[pyclass] annotated struct. Instead, it has to be &PyCell<Class> (I assume, at least that works).

second

Furthermore, now I can't cast PyAny and PyObject objects referring to a Class to Rust references anymore. Previously I used PyAny::downcast_ref (now downcast) and PyObject::cast_as. The error is in both cases the same, i.e.

error[E0277]: the trait bound `ast::segments::OtherSegment: pyo3::instance::PyNativeType` is not satisfied
   --> src/ast/mod.rs:405:37
    |
405 |         } else if let Ok(seg) = seg.cast_as::<OtherSegment>(py) {
    |                                     ^^^^^^^ the trait `pyo3::instance::PyNativeType` is not implemented for `ast::segments::OtherSegment`
    |
    = note: required because of the requirements on the impl of `pyo3::conversion::PyTryFrom<'_>` for `ast::segments::OtherSegment`

One way I found to fix this was to use seg.extract::<&PyCell<OtherSegment>>.borrow().
This is quite cumbersome -- is there/could there be a shortcut?

Of course this should also be noted in the release notes.

Thanks!

@kngwyu
Copy link
Member

kngwyu commented Mar 3, 2020

Thank you.
I'll write a migration note before releasing 0.9.0.

&Class and &mut Class as method arguments anymore,

We can't implement FromPyObject for them. You can use PyRef<T> and PyRefMut<T> as arguments.

One way I found to fix this was to use seg.extract::<&PyCell>.borrow().
This is quite cumbersome -- is there/could there be a shortcut?

Please use seg.extract::<PyRef<OtherSegment>>. seg.extract::<&OtherSegment> is difficult due to we have to track reference counts of PyCells using PyRef/PyRefMut.

@kngwyu kngwyu added this to the 0.9.0 milestone Mar 3, 2020
@davidhewitt
Copy link
Member

We can't implement FromPyObject for them. You can use PyRef and PyRefMut as arguments.

Agreed we can't implement FromPyObject for them. It would be nice if we can find another way for the generated wrappers to support &Class and &mut Class arguments - by calling .try_borrow() in the wrapper, perhaps, like we can do for &self.

As for supporting them in downcast_ref - I guess that's just not possible, because the PyRef needs to exist.

@kngwyu
Copy link
Member

kngwyu commented Mar 4, 2020

#791 enables &PyClass and &mut PyClass at arguments position again.

@birkenfeld
Copy link
Member Author

birkenfeld commented Mar 4, 2020

Awesome! For my #2, I found that extract::<PyRef<Class>> works as well, as you said, so no need for borrow(). I think that's quite ergonomic now.

Am I right that cast_as and downcast can now only be used for native Python types anymore? (Since it seems to require PyNativeType.)

@davidhewitt
Copy link
Member

Awesome! For my #2, I found that extract::<PyRef> works as well, as you said, so no need for borrow(). I think that's quite ergonomic now.

We should probably add an example near the top of class.md which makes use of PyRef<Class> - as this is one of the first things users will see.

Am I right that cast_as and downcast can now only be used for native Python types anymore? (Since it seems to require PyNativeType.)

If PyRef<T> isn't PyNativeType, it probably should be?

@kngwyu
Copy link
Member

kngwyu commented Mar 5, 2020

cast_as requires PyTryFrom and we cannot implement it for PyRef and PyRefMut because its errot type is PyDowncastError.
So we can

  • Use cast_as::<&PyCell> and encourage users to do so

or

  • Implement PyTryFrom for PyRef and PyRefMut by changing its return type to more general PyErr

To keep our API minimal I'm for the former idea, but it's not very problematic to adopt the later way.

@davidhewitt
Copy link
Member

Implement PyTryFrom for PyRef and PyRefMut by changing its return type to more general PyErr

I actually prefer going in this direction (eventually) - but I think we might want to wait until we've improved the ergonomics of error handling.

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

Successfully merging a pull request may close this issue.

3 participants