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

Support default method implementation #2014

Merged
merged 1 commit into from
Nov 29, 2021
Merged

Support default method implementation #2014

merged 1 commit into from
Nov 29, 2021

Conversation

jovenlin0527
Copy link
Contributor

@jovenlin0527 jovenlin0527 commented Nov 21, 2021 •
edited
Loading

Precursor to #2013. This allows us to inject default implementation of slot methods.

Add a trait (PyClassDefaultSlots) to support default methods.
Add pyimpl::gen_default_slot_impls to help generate definition of default methods in the macro backend.
Add default implementation of __repr__ for #[pyclass] enums.

davidhewitt
davidhewitt reviewed Nov 22, 2021
View reviewed changes
pyo3-macros-backend/src/pyclass.rs Outdated
Comment on lines 770 to 771
// It's collected last so Python ignore them if it found methods with same names.
visitor(collector.py_class_default_impls());
Copy link
Member

Choose a reason for hiding this comment

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

It seems a bit more thorny than that:

https://github.com/python/cpython/blob/d2b55b07d2b503dcd3b5c0e2753efa835cff8e8f/Objects/typeobject.c#L5655-L5660

Looks like if the method definition has METH_COEXIST then it overwrites any previous method. So we just need to be careful to not use METH_COEXIST with these default methods. Which I guess is fine.

tests/test_default_impls.rs Outdated Show resolved Hide resolved
@jovenlin0527
Copy link
Contributor Author

jovenlin0527 commented Nov 22, 2021 •
edited
Loading

I need to touch a lot of implementation details in order to rename the generated method, and there's a lot of code duplication between gen_default_slot_impls and gen_py_method. I can't reuse gen_py_method because then it will be very hard to rename methods.

@jovenlin0527 jovenlin0527 marked this pull request as draft November 22, 2021 20:48
@jovenlin0527
Copy link
Contributor Author

jovenlin0527 commented Nov 22, 2021 •
edited
Loading

So I forgot there is #[pyo3(name= ...)] and it supports magic methods....
Anyway, using that attribute makes the code cleaner. The new commit respects privacy better, though there's still code duplication between pyimpl::impl_methods and pyimpl::gen_default_slot_impls.

@jovenlin0527 jovenlin0527 force-pushed the default_impl branch 2 times, most recently from 94c23e4 to 58f1c94 Compare November 24, 2021 17:33
@jovenlin0527 jovenlin0527 marked this pull request as ready for review November 24, 2021 17:34
@jovenlin0527
Copy link
Contributor Author

jovenlin0527 commented Nov 24, 2021 •
edited
Loading

I was very sick from my vaccine shot yesterday, so couldn't do anything. Sorry.

You mentioned there would be trait conflicts from default SlotFragment implementations, so I reverted my last commit.

@davidhewitt
Copy link
Member

No worries, I have also been very busy. I'll do my best to review this over the weekend, or end of Monday at the latest.

davidhewitt
davidhewitt approved these changes Nov 29, 2021
View reviewed changes
Copy link
Member

@davidhewitt davidhewitt left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks, this looks fine to me! Sorry for the delay.

Just needs the CHANGELOG entry removed please (and maybe squash this branch at the same time) and then I think it's good to merge.

EDIT: oh and also the #[doc(hidden)] addition to the generated methods.

pyo3-macros-backend/src/pyimpl.rs Show resolved Hide resolved
pyo3-macros-backend/src/pyimpl.rs
Comment on lines +142 to +144
pub fn gen_default_slot_impls(cls: &syn::Ident, method_defs: Vec<TokenStream>) -> TokenStream {
// This function uses a lot of `unwrap()`; since method_defs are provided by us, they should
// all succeed.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a huge fan of taking Vec<TokenStream> as an argument and all the corresponding unwrap it subsequently leads to.

I think an alternative might be to take Vec<(syn::ItemFn, PyMethod, &SlotDef)> and have the machinery to just produce the slots in here. But TBH that would probably lead to quite a large refactoring so I think it's better here to just focus on getting the end-user output as desired and then we can refactor internals of PyO3 for compile speed to our hearts' content later! 😂

CHANGELOG.md Outdated Show resolved Hide resolved
@jovenlin0527 jovenlin0527 mentioned this pull request Nov 29, 2021
Closed
davidhewitt
davidhewitt approved these changes Nov 29, 2021
View reviewed changes
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.

Perfect, thanks again!

@davidhewitt davidhewitt merged commit 8a03778 into PyO3:main Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidhewitt davidhewitt davidhewitt 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.
2 participants
@jovenlin0527 @davidhewitt