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.

Sign up for GitHub

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

Jump to bottom

Allow #[getter] and #[setter] functions to take PyRef #999

Merged
merged 2 commits into from Jun 27, 2020
Merged

Allow #[getter] and #[setter] functions to take PyRef #999

merged 2 commits into from Jun 27, 2020

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Jun 23, 2020 •
edited

Closes #837

I rewrote the proc_macro code to allow #[getter] and #[setter] functions to take &self as well as PyRef<Self> etc.

It might also be possible to make a similar change so that all #[pyproto] methods can also choose beteween &self, &mut self, PyRef<Self>, etc. but this change was necessary first so I began with this PR.

@kngwyu
Copy link
Member

kngwyu commented Jun 24, 2020

Thank you! The approach looks good, but it seems like there are some rooms for refactoring. I'll give a detailed review later.
In addition,

It might also be possible to make a similar change so that all #[pyproto] methods can also choose beteween &self, &mut self, PyRef, etc. but this change was necessary first so I began with this PR.

Is it possible?
Each protocol method has trait defintion(e.g., fn __len__(&'p self) -> Self::Result) and it is really restrictive.

@davidhewitt
Copy link
Member Author

I'll give a detailed review later.

👍 no rush. I'm sure there are things we can improve in here further. Also I don't mind if you want to wait for this until after 0.11 is released as it's quite a hefty rewrite of the proc macro code!

Is it possible?

That's why I say might 😄 . The pyproto code uses the same function parser so this PR would at least enable us to parse such function definitions. I need to experiment another time to understand if the rest of the implementation is possible!

@davidhewitt
Copy link
Member Author

davidhewitt commented Jun 24, 2020 •
edited

I spotted a refactoring around SelfType to simplify the code there.

Also I was able to use quote_spanned! instead of quote! so that invalid self types get highlighted appropriately in error messages (see the compile_error test). With just quote! the error message just highlights the #[pymethods] attribute. This will be worth remembering in the future when we edit proc macro code!

kngwyu
kngwyu requested changes Jun 25, 2020
View reviewed changes
pyo3-derive-backend/src/method.rs Outdated Show resolved Hide resolved
pyo3-derive-backend/src/method.rs Outdated Show resolved Hide resolved
pyo3-derive-backend/src/method.rs Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Member Author

Thanks for the review - I merged all of the suggestions!

@davidhewitt @kngwyu
Apply suggestions from code review
c3e993e 
Co-authored-by: Yuji Kanagawa <yuji.kngw.80s.revive@gmail.com>
davidhewitt
davidhewitt commented Jun 27, 2020
View reviewed changes
tests/test_compile_error.rs
@@ -5,6 +5,8 @@ fn test_compile_errors() {
t.compile_fail("tests/ui/invalid_macro_args.rs");
t.compile_fail("tests/ui/invalid_property_args.rs");
t.compile_fail("tests/ui/invalid_pyclass_args.rs");
t.compile_fail("tests/ui/invalid_pymethod_names.rs");
Copy link
Member Author

Choose a reason for hiding this comment

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

This test already existed but seemed to accidentally be lost from the compile error test list.

Copy link
Member

Choose a reason for hiding this comment

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

Ooops, I accidently removed it when tackling the CI problem. Thanks!

@kngwyu
Copy link
Member

kngwyu commented Jun 27, 2020

Thanks!

@kngwyu kngwyu merged commit 15d919a into PyO3:master Jun 27, 2020
@davidhewitt davidhewitt deleted the simplify-method-receivers branch August 10, 2021 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kngwyu kngwyu kngwyu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Property methods cannot take self as a PyRef
2 participants
@davidhewitt @kngwyu