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

[NEED DISCUSSION] Remove #[init] attribute #658

Merged
merged 3 commits into from
Nov 9, 2019
Merged

Conversation

kngwyu
Copy link
Member

@kngwyu kngwyu commented Oct 31, 2019

See #651 for detail.

Now we can override __init__ function of pyclasses, by

#[pymethods]
impl Foo {
    #[init]
    fn init(&mut self) -> () {
        // some initialization codes
    }

.
But this feature is not very popular and confusing since we mainly use #[new].

So I think this feature should be removed.
Our proc-macro code tends to be complicated, so make it as simple as possible is important.
Any idea?

res = Some(FnType::FnInit)
return Err(syn::Error::new_spanned(
name,
"#[init] is duplicated from PyO3 0.9.0!",
Copy link
Member

Choose a reason for hiding this comment

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

Should this be #[init] is removed from PyO3 0.9.0!? Since #[init] was never working I think we don't have to return an error.

@pganssle
Copy link
Member

It is very unusual from a Python perspective to always implement __new__ (which is very rarely implemented in pure Python) and never implement __init__.

Per this comment which @Alexander-N mentioned in #651, I guess I understand why it would be more common in Rust to implement __new__, but it does feel weird not to provide any support. Is there any way we can make #[init] a feature that needs to be enabled? Possibly like: #[pyclass(init)], or something of that nature? That feels like it would prevent unwary people from running into soundness issues when implementing __init__, while also enabling people to put actual initialization into the __init__ function.

I definitely think additional discussion would be useful, because I'm just not familiar enough with the use cases for Rust-backed Python classes to know when you'd actually want an __init__. My hunch is that it would be useful to separate initialization and construction of the object when you are designing a class intended to be subclassed in Python - you would ensure that the Rust struct is properly initialized in #[new] and then put any overridable initialization behavior in #[init].

It may be worth looking at C extensions and standard library modules that define both tp_new and tp_init.

Copy link
Member

@konstin konstin left a comment

Choose a reason for hiding this comment

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

I don't know much about #[init] so I can't help with that decision.

@kngwyu
Copy link
Member Author

kngwyu commented Nov 1, 2019

@pganssle
Thank you for your comment.
I'm going to investigate how other similar libraries(e.g., pybind11) use tp_init.

@kngwyu
Copy link
Member Author

kngwyu commented Nov 1, 2019

Looks C/C++ libraries adopt an opposite approach to us.
Here's pybind11's tp_new function.
The comment says

It only allocates space for the C++ object, but doesn't call the constructor -- an __init__ function must do that.

However, it is really not Rustic way to do so, as dgrunwald mentioned in this comment.

@althonos
Copy link
Member

althonos commented Nov 1, 2019

However, it is really not Rustic way to do so, as dgrunwald mentioned in this comment.

Yeah, Rust has RAII and allocating an object without initializing it would be highly unsafe, especially for dropable values.

@pganssle
Copy link
Member

pganssle commented Nov 1, 2019

Yeah, Rust has RAII and allocating an object without initializing it would be highly unsafe, especially for dropable values.

I'm fully on board with the idea that Rust objects should be initialized with __new__, it makes sense to do it that way. I think we can take that as a given and then ask the question of when does it make sense for someone to implement both tp_new and tp_init. I did a bit of searching and couldn't find any good examples of classes that implement both __new__ and __init__ in pure Python, but it's considerably harder to search for the equivalent thing in C extensions (since tp_init and tp_new are often initialized as part of the class struct initialization, so they are positional arguments 18 and 21 or some nonsense like that), and C extensions are the relevant comparison.

@kngwyu
Copy link
Member Author

kngwyu commented Nov 9, 2019

Thanks for all comments.
I decided to remove this feature for now.
If we realized it's useful, we can retriveit any time.

@kngwyu kngwyu merged commit 99f9b36 into PyO3:master Nov 9, 2019
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.

None yet

5 participants