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

Allow 0..n pymethod blocks without specialization #332

Merged
merged 8 commits into from
Feb 1, 2019
Merged

Conversation

konstin
Copy link
Member

@konstin konstin commented Jan 30, 2019

Surprisingly it's possible and even easy to support multiple impl blocks and even get rid of specialization! This all works thanks to inventory, which uses the ctor binary section to have life-before-main code collect a bunch of instances.

This pull requests uses the doc(hidden) trait inventory::Collect, for which I've opened dtolnay/inventory#5

As a bonus, we also get rid of PyPropMethodsProtocolImpl.

Fixes #302
Relevant for #210

@discosultan
Copy link

Does this mean pyo3 will be supported on stable Rust?

@konstin
Copy link
Member Author

konstin commented Jan 30, 2019

Not yet, but that was a major blocker. You can see the remaining blocker by commenting out #![feature(specialization)] in src/lib.rs and running cargo check. And we're of course happy about pull requests fixing some of those errors ;)

Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

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

LGTM

@kngwyu
Copy link
Member

kngwyu commented Feb 1, 2019

Really great PR, thanks.
Looks the outputs of repr changed in examples, e.g. rustapi_module.datetime.TzClass changed to TzClass, which makes travis fail.
Is this intended?

@konstin konstin force-pushed the inventory branch 2 times, most recently from 8b2297d to 55109c7 Compare February 1, 2019 14:01
@konstin
Copy link
Member Author

konstin commented Feb 1, 2019

I've removed the module name because currently the module name is whatever module initializes the pyclass, which imho bad because it's not really deterministic. E.g. Adding a module to your module that also adds the class make the class in both modules end up with a different module name.

@konstin
Copy link
Member Author

konstin commented Feb 1, 2019

Eventually, I'd like to have a module name again, but it should be something less volatile.

@konstin konstin merged commit 853fc7c into master Feb 1, 2019
@konstin konstin deleted the inventory branch February 1, 2019 16:40
@athre0z
Copy link
Contributor

athre0z commented Feb 1, 2019

Very nice, thanks!

@kngwyu
Copy link
Member

kngwyu commented Feb 2, 2019

@konstin

I've removed the module name because currently the module name is whatever module initializes the pyclass, which imho bad because it's not really deterministic.

I see, 👍 for this.

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.

4 participants