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

add IntoPyObjectRef derive macro #4672

Merged
merged 1 commit into from
Nov 4, 2024

Conversation

Icxolu
Copy link
Contributor

@Icxolu Icxolu commented Oct 30, 2024

From #4041 (comment)

Introduces the IntoPyObjectRef derive macro to implement IntoPyObject for a reference to the derived type. It pretty much uses the same code gen as the ordinary IntoPyObject derive macro. I have moved the relevant branches up to the root function, to avoid threading the reference condition through the whole machinery. We really only need to adjust the ident itself, generic bounds, transparent types and introduce the named lifetime, the body works by inference and does not need changes between the two.

I added this to the hygiene, roundtrip and ui tests. I skipped hygiene IntoPyObject4 because it would need IntoPyObject for &&Bound which we currently don't have, not sure where we want to draw the line.

Closes #4041 🎉

@Icxolu Icxolu added this to the 0.23 milestone Oct 30, 2024
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.

Great, awesome to see that this just works out! 🎉

Just a final thought - is RefIntoPyObject or IntoPyObjectRef better? I quite like it starting with Into, which would suggest we stick with IntoPyObjectRef, but the order RefIntoPyObject feels more accurate to me as a description. 🤔

@davidhewitt
Copy link
Member

Also, thank you for just seeing my brief musing and running with it to build something which is hopefully quite convenient for users!

@davidhewitt
Copy link
Member

davidhewitt commented Nov 2, 2024

I added this to the hygiene, roundtrip and ui tests. I skipped hygiene IntoPyObject4 because it would need IntoPyObject for &&Bound which we currently don't have, not sure where we want to draw the line.

I feel like there might be a way where we could use specialization inside the macro to achieve that, but I'm also happy if we defer that for a follow-up for simplicity's sake.

(i.e. because &Bound is Copy it feels to me like we might not need to support &&Bound, but it might be that there's thorny edge cases...)

@Icxolu
Copy link
Contributor Author

Icxolu commented Nov 2, 2024

Just a final thought - is RefIntoPyObject or IntoPyObjectRef better? I quite like it starting with Into, which would suggest we stick with IntoPyObjectRef, but the order RefIntoPyObject feels more accurate to me as a description. 🤔

Personally I prefer IntoPyObjectRef. (It reads nicer to me) I think the name is still descriptive enough. If there is need we can always extend the documentation a bit to make.

I feel like there might be a way where we could use specialization inside the macro to achieve that, but I'm also happy if we defer that for a follow-up for simplicity's sake.

I guess we probably could special case here, but this applies to pretty much to all types. So if this would store &String (for whatever reason), IntoPyObjectRef would have &&String which we don't supply an impl for. Special casing a few (of our) types is not really a solution to the problem.

Maybe we can provide this blanket impl?

impl<'a, 'py, T> IntoPyObject<'py> for &&'a T
where
    &'a T: IntoPyObject<'py>,
{
    type Target = <&'a T as IntoPyObject<'py>>::Target;
    type Output = <&'a T as IntoPyObject<'py>>::Output;
    type Error = <&'a T as IntoPyObject<'py>>::Error;

    #[inline]
    fn into_pyobject(self, py: Python<'py>) -> Result<Self::Output, Self::Error> {
        (*self).into_pyobject(py)
    }
}

@davidhewitt
Copy link
Member

That blanket seems like a very good idea, I can't think of a reason why a user would ever want a different implementation for &&T if they've implemented for &T.

Let's add that blanket before 0.23 as a follow up, maybe I can PR that quickly this morning in an hour or so...

@davidhewitt davidhewitt added this pull request to the merge queue Nov 4, 2024
Merged via the queue into PyO3:main with commit 00d84d8 Nov 4, 2024
43 of 45 checks passed
@Icxolu Icxolu deleted the derive-intopyobject-ref branch November 4, 2024 17:43
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.

Contribution opportunity: improved conversion traits
2 participants