pyo3 icon indicating copy to clipboard operation
pyo3 copied to clipboard

Implement `auto_new` attribute for `#[pyclass]`

Open RedKinda opened this issue 4 months ago • 12 comments

PR for issue #5416

This introduces a new #[pyclass] attribute called auto_new, where set_all is required to be used alongside it. It automatically generates a #[new] function for this class, similar to that of the python dataclass. It requires the multiple-pymethods feautre, as it generates its own #[pymethods] block

RedKinda avatar Sep 08 '25 13:09 RedKinda

I am not quite sure why the CI failing, I dont see any failure in the logs

RedKinda avatar Sep 08 '25 17:09 RedKinda

Hi, thanks for the review!

In regards to the defaults-related PR, #[pyclass(auto_new = "from_fields")] and #[pyclass(auto_new = "default")] make sense to me, names sounds fine.

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 generate_protocol_slot comment before merging, but might take a bit since I am not familiar with it yet.

RedKinda avatar Oct 21 '25 16:10 RedKinda

auto_new is maybe the right name, the only alternative I know is something like dataclass

Perhaps we could just call it new. The "auto" feels a bit redundant to me. Just setting the option already implies that something is happening. We don't call it auto_str for example, just str.

Icxolu avatar Oct 21 '25 17:10 Icxolu

@davidhewitt in regards to generate_protocol_slot for auto_new should that be a new pub const __NEW__: SlotDef? not sure what else but just confirming

@Icxolu i think it might be a little confusing for newbies reading docs to have both #[pyclass(new)] and #[new] on functions, but i don't feel strongly about this

RedKinda avatar Oct 22 '25 00:10 RedKinda

Good question, #5551 produces a __NEW__ slotdef which you should be able to use, I'm going to split that diff up. Feel free to either base off that PR or give me some time to merge it in stages (might take a few days).

I think #[pyclass(new)] seems correct to me, this matches #[pyclass(str)] and __str__. I would quite like to have #[new] optionally just be __new__.

davidhewitt avatar Oct 22 '25 17:10 davidhewitt

With #5551 merged, this should be able to rebase on top now.

davidhewitt avatar Nov 18 '25 13:11 davidhewitt

I am running into an issue, I think generate_protocol_slot assumes first argument is self, and generates broken code when it isn't. Image below shows cargo expanded version of this simple test struct.

#[pyclass(get_all, set_all, new = "from_fields")]
struct AutoNewCls {
    a: i32,
    b: String,
    c: Option<f64>,
}
image

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.

RedKinda avatar Nov 18 '25 23:11 RedKinda

looks like nox -s update-ui-tests isnt working for me :/ I think I am running on a different rust version producing slightly different errors, as it slightly modifies unrelated tests as well

RedKinda avatar Dec 04 '25 19:12 RedKinda

I pushed a commit which should fix. The &str extraction only works like that on Python 3.9 and older, perhaps caused by your system Python? I am unsure why we don't notice that as a problem in our CI 🤔

We use latest stable Rust, also latest trybuild version (rustup update stable && cargo update should align you with CI).

davidhewitt avatar Dec 05 '25 20:12 davidhewitt

I will give another pass at review probably on Sunday.

davidhewitt avatar Dec 05 '25 20:12 davidhewitt

Looks like the &str case in the UI test is indeed still a problem. I'm currently investigating that code to see what we can do to improve the error message.

davidhewitt avatar Dec 09 '25 10:12 davidhewitt

Circling back here, I have some ideas how to solve that problem, but the async code being heavily special cased (xref #5681) is causing issues for me. If I get that code adjusted to not have the special case I think I can land cleanups to unblock here.

davidhewitt avatar Dec 09 '25 16:12 davidhewitt