derive(FromPyObject): adds default option

[Tpt]

Takes an optional expression to set a custom value that is not the one from the Default trait

Note that the default value is only used if the key fetching fails (item/attr not present...) but errors are still raised if the value extraction fails

Issue #4643

[Icxolu]

Thanks! This looks pretty good already.

Note that the default value is only used if the key fetching fails (item/attr not present...) but errors are still raised if the value extraction fails

I think this makes sense 👍.

Some points that come to my mind:

• We should document this option in the FromPyObject section of the guide, probably with some example showcasing the error decision from above.
• It looks like this should work for tuple structs (we should add a test), how about complex enums?
• How does this interact with from_py_with? I would say only one or the other makes sense, and we should error if both are supplied.

[Icxolu]

Default and Ok are unhygienic, we should add this option to the hygiene tests.

[Tpt]

Fixed. Thank you for spotting!

[davidhewitt]

This idea makes sense to me, agreed with all the above 👍

[davidhewitt]

Possible design question: serde uses default = "path" where "path" should be a function. https://serde.rs/field-attrs.html#default--path

Should we be doing similar? I wonder what the advantage of the function is, I guess it's more general? Is it for hygiene reasons? 🤔

[Icxolu]

I wonder what the advantage of the function is, I guess it's more general? Is it for hygiene reasons?

I think this is mostly for historical reasons from before Rust allowed expressions (or identifiers) in attributes. Nowadays making use of proper tokens instead of stringly typing it is prefered I would say. IMO it is also way more convenient to just write the expression inline instead of having to write a function for every default.

[davidhewitt]

True, and if we allow arbitrary expressions we can always allow something like default = f() to call a function 👍

[Tpt]

Thank you for the reviews. I will apply the changes you asked for.

How does this interact with from_py_with? I would say only one or the other makes sense, and we should error if both are supplied.

The default parameter value must return the Rust field type. The current code does:

• If getitem/getattr returns a value, then call from_py_with with it. If if succeeds, it's the Rust field value, if not raise the error
• If getitem/getattr fails, call the default expression and set the Rust field value with it.

It seems fairly well defined to me and is consistent with the behavior of #[pyo3(signature)] on functions but I can tweak this behavior if you want.

I am going to add an extra test covering the usage of the two options together

[Icxolu]

The default parameter value must return the Rust field type. The current code does:

• If getitem/getattr returns a value, then call from_py_with with it. If if succeeds, it's the Rust field value, if not raise the error
• If getitem/getattr fails, call the default expression and set the Rust field value with it.

It seems fairly well defined to me and is consistent with the behavior of #[pyo3(signature)] on functions but I can tweak this behavior if you want.

Ah, having it written out like that makes sense. (In my head it felt more convoluted around error cases 😁) I'm good with keeping it as is then. Maybe this is also a good addition for the guide section.

I am going to add an extra test covering the usage of the two options together

Sounds good, thank you!

[Tpt]

@Icxolu Thank you again for your review! I have improved test coverage and fixed hygiene.

We should document this option in the FromPyObject section of the guide, probably with some example showcasing the error decision from above.

Done!

It looks like this should work for tuple structs (we should add a test), how about complex enums?

Yes, but it would need some extra code. I would prefer to do it in a follow-up MR. On complex enums, I guess it would work too with the surprising behavior that a variant Foo { #[pyo3(default)] value: usize } becomes a catch all just like the Foo {} variant.

[Icxolu]

Yes, but it would need some extra code. I would prefer to do it in a follow-up MR.

Sure thing. In that case we should probably make sure here, that we properly error out if applied to tuple structs or enums with some ui tests until we add additional support in the future.

[Icxolu]

I think we should mention here, that the default is used only if the lookup failed (not when the extraction failed).

[Tpt]

Done!

[Tpt]

Sure thing. In that case we should probably make sure here, that we properly error out if applied to tuple structs or enums with some ui tests until we add additional support in the future.

Thank you! After doing some testing, the current code already works with enum variant named fields so I added support to them. I also included an error when all fields are set as default to be consistent with the current error that prevents deriving FromPyObject on unit structs. I have also

[Icxolu]

Thanks you! I think this looks perfect now 🚀