-
Notifications
You must be signed in to change notification settings - Fork 905
Implement auto_new attribute for #[pyclass]
#5421
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
base: main
Are you sure you want to change the base?
Conversation
|
I am not quite sure why the CI failing, I dont see any failure in the logs |
davidhewitt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, and sorry for the very slow review.
I think this feature makes a ton of sense, I've wanted something like this for ages. auto_new is maybe the right name, the only alternative I know is something like dataclass where this is roughly equivalent to the dataclass(init=True) option. I don't think init is correct here though, since this generates __new__.
To resolve before merge: #5284 proposes #[pyclass(default)] which would allow a constructor which takes no arguments and uses Default::default(). It might be nice to not have separate default and auto_new attributes. What if we had #[pyclass(auto_new = "from_fields")] (this PR) and #[pyclass(auto_new = "default")] ? Name and design ideas welcome.
Other open questions, potentially fine to leave for follow ups:
- We could allow
#[pyo3(default = X)]annotations on the fields to construct them. - We could consider
#[pyo3(kw_only)]at both the class and field level.
The CI failure is UI tests, you can fix with nox -s update-ui-tests:
https://github.com/PyO3/pyo3/actions/runs/17557400301/job/49865008001?pr=5421#step:19:3264
pyo3-macros-backend/src/pyclass.rs
Outdated
| if matches!(methods_type, PyClassMethodsType::Specialization) { | ||
| bail_spanned!(opt.span() => "`auto_new` requires the `multiple-pymethods` feature."); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need this restriction (at the cost of a slightly more complicated implementation) - see generate_protocol_slot and how it's used for __str__.
6b530ee to
bb9959b
Compare
|
Hi, thanks for the review! In regards to the defaults-related PR, As for the followups, I would leave those to be in a separate PR and get this and the default one out first. I don't have the capacity to work on them at the moment but maybe if we open an issue someone will pick it up. I am also gonna look at the |
Perhaps we could just call it |
|
@davidhewitt in regards to @Icxolu i think it might be a little confusing for newbies reading docs to have both |
|
Good question, #5551 produces a I think |
|
With #5551 merged, this should be able to rebase on top now. |
92373ef to
036004f
Compare
|
I am running into an issue, I think #[pyclass(get_all, set_all, new = "from_fields")]
struct AutoNewCls {
a: i32,
b: String,
c: Option<f64>,
}
I am not sure how to go about fixing this properly @davidhewitt maybe you could take a look? I pushed the latest broken commit with it, other than that it should work but hard to tell with this being broken. |
davidhewitt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for following up on this, and sorry for the delay from my end. I pushed a fix which has CI passing now.
Before merging, I have a few final points of discussion and test suggestions. See comments below.
| ); | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to add a test with a tuple struct, e.g.
struct Point2d(f64, f64);... I would think the generated constructor would allow only positional inputs, as there are no names for the fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(It might be good enough for this to gracefully fail with a "not yet supported" message as far as this PR is concerned.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unless i am missing something i added support for tuple structs here 4881636
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub enum NewImplTypeAttributeValue { | ||
| FromFields, | ||
| // Future variant for 'default' should go here | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have a quick think about the interaction of #[pyo3(new = "default")] and the other future extensions I suggested in #5421 (review)
- Should
new = "default"accept any arguments? The easy answer is no. But what if users want to have a constructor which is the equivalent ofMyStruct { x, y, ...Default::default() }, i.e. use the struct-level default except for some specific fields? - Should fields accept
#[pyo3(new = <value>)]to remove them as arguments from the generated constructor and always set them to<value>(similar todataclasses.field(init = False))? This would appear to have very strong overlap with #[pyo3(new = "default")]on the struct, presumablyfor the field would come from theDefault` implementation. - Once we have both of these, what is the difference between
"from_fields"with a bunch of#[pyo3(default = <value>)]annotations on fields?
... it feels to me like the general design would be that new = "from_fields" would not require a Default implementation, and would allow users to take fields out of the constructor and instead give them default values via #[pyo3(default = <value>)].
new = "default" would be the opposite; it would require a Default implementation and would require users to opt-in to add fields to the constructor to allow callers to override the value set by the Default implementation. It feels like #[pyo3(new = true)] might be the right annotation for this, but I can't decide. That can in theory be a future extension for new = "default" so maybe it's a long way off.
We don't need to solve this now, but I'd at least like to make sure that having "from_fields" as implemented here doesn't accidentally close off the design space. We can write this all into a follow-up issue after merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If using new = "from_fields", i think field-level defaults should make that field a keyword argument in __new__ and give it the configured default. Example:
#[pyclass(new = "from_fields")]
struct Foo {
a: u64
#[pyo3(default=5)]
b: u64
}would be the equivalent of
class Foo:
def __new__(a: int, b: int = 5) -> Self:
...Using new = "default" could then simply always produce an empty __new__.

PR for issue #5416
This introduces a new
#[pyclass]attribute calledauto_new, whereset_allis required to be used alongside it. It automatically generates a#[new]function for this class, similar to that of the python dataclass. It requires themultiple-pymethodsfeautre, as it generates its own#[pymethods]block