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

Improve lifetime insertions for #[pyproto] #1093

Merged
merged 2 commits into from
Aug 11, 2020
Merged

Conversation

kngwyu
Copy link
Member

@kngwyu kngwyu commented Aug 8, 2020

This PR allows:

#[pyproto]
impl PyIterProtocol for Iter {
    fn __iter__(slf: PyRef<Self>) -> PyRef<Self> {
        slf
    }
}

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This actually helps fix a lot of #1031!

I think this change needs to go a bit further before merging, because at the moment it fixes some protocol methods but not all of them. For example, this code is still not supported:

#[pyproto]
impl PyDescrProtocol for MyClass {
    fn __get__(
        mut slf: PyRefMut<Self>,     // <--- This is very similar to your example, but is TernaryS receiver
        _instance: PyRef<Self>,      // <--- This one still has a problem
        _owner: Option<&PyType>,     // <--- This reference is nested in an Option, so get_arg_ty misses
    ) -> PyRefMut<Self> {            // OK after this PR =)
        slf
    }

I was actually thinking about a refactoring the other day which refactors the MethodProto enum to a struct instead. I can't find my branch right now, but this was the essence of the change:

use crate::method::SelfType;

pub struct MethodProto {
    name: &'static str,
    proto: &'static str,
    receiver: SelfType,
    args: &[&'static str],         // names of all the arguments
    result: Option<&'static str>   // name of the return type, if any (I think only `Free` does not have)
}

Maybe taking that refactoring now would help this PR? I can submit as a different PR, or you can use this idea directly in this one?

}
}
// Insert lifetime to PyRef (i.e., PyRef<Self> -> PyRef<'p, Self>)
insert_lifetime_to_pyref(&mut slf_ty);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should add this inside get_arg_ty instead of calling here.

syn::ReturnType::Type(_, ty) => {
let mut ty = ty.clone();
// Insert lifetime to PyRef (i.e., PyRef<Self> -> PyRef<'p, Self>)
insert_lifetime_to_pyref(&mut ty);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if we could support adding lifetimes to references too. I notice that get_arg_ty does this.

@kngwyu kngwyu changed the title Allow PyRef<Self> as return type by inserting 'p Improve lifetime insertions for #[pyproto] Aug 9, 2020
@kngwyu
Copy link
Member Author

kngwyu commented Aug 9, 2020

Thanks, I made it more general. Now your descr example works too.

I was actually thinking about a refactoring the other day which refactors the MethodProto enum to a struct instead. I can't find my branch right now, but this was the essence of the change:

It sounds nice since the current implementation is crafty. However, I want to leave it for another chance, since I have to complete another PR first (for #844).

@kngwyu kngwyu force-pushed the iterator-example branch 2 times, most recently from d43ddf6 to c01e2d8 Compare August 9, 2020 15:07
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done! I'm happy for that refactoring to wait for another time 👍

CHANGELOG.md Outdated Show resolved Hide resolved
@kngwyu kngwyu merged commit b17d4ff into PyO3:master Aug 11, 2020
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