Improve detecting whether default can be derived

[AaronDewes]

This checks if a struct contains a required member for which Default is not implemented (Enums or other structs which don't have Default). If it does, Default will not be derived for that struct.

[clux]

Hey thanks for this.

I think the idea here is interesting, but I am a little confused about how practically useful it is. Maybe I am missing something, but if you have an enum embedded deeply within your spec struct, and want to derive Default, then this would basically recursively search all containing structs recursively and disable it on them, right?

If that's the case, isn't it better to not ask to derive Default? Because you'll only be able to practically derive it for a small subset of the structs anyway?

The thing I do for enums is to --derive=Default anyway (even if it leads to code that doesn't compile out of the box), and then add a couple of manual impl Default for ThisEnum to polyfill in the handful of enums (while #67 is unimplemented) to allow Default to work on the whole struct, and I suspect this new behaviour here might make this harder.

[AaronDewes]

The thing I do for enums is to derive default anyway, and then add a couple of manual impl Default for ThisEnum to polyfill in the handful of enums (while #67 is unimplemented) to allow Default to work on the whole struct, and I suspect this new behaviour here might make this harder.

Yes, but this change was useful for me when I did not want to go through the code and manually implement default for enums.

My use case was specifically generating types for cert-manager.

Here are the CRDs. I am specifically using Certificate.

CertificateSpec has a lot of options, most of which I don't care about in most cases. To make it easier to use, I often just write

CertificateSpec {
  some_prop: value,
  ..Default::default(),
}

For that to work, I need default to be derived on CertificateSpec. So I pass --derive=Default to kopium.

Now, the output won't compile, because Default is also derived on CertificateAdditionalOutputFormats.

/// CertificateAdditionalOutputFormat defines an additional output format of a Certificate resource. These contain supplementary data formats of the signed certificate chain and paired private key.
#[derive(Serialize, Deserialize, Clone, TypedBuilder, Debug, PartialEq, JsonSchema)]
pub struct CertificateAdditionalOutputFormats {
    /// Type is the name of the format type that should be written to the Certificate's target Secret.
    #[serde(rename = "type")]
    pub r#type: CertificateAdditionalOutputFormatsType,
}

/// CertificateAdditionalOutputFormat defines an additional output format of a Certificate resource. These contain supplementary data formats of the signed certificate chain and paired private key.
#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)]
pub enum CertificateAdditionalOutputFormatsType {
    #[serde(rename = "DER")]
    Der,
    #[serde(rename = "CombinedPEM")]
    CombinedPem,
}

CertificateAdditionalOutputFormat is only used inside an Option<Vec<_>>, so I don't need Default on it. This PR prevents default from being derived on it, while the main CertificateSpec can stay Default.

I could of course manually change it, but I want to avoid manual changes in generated code.

Also, this PR contains another small change:

TypedBuilder is no longer derived on enums, which always gave an error for me. I can move that into a separate PR, because I assume that being derived for enums is a bug.

[clux]

CertificateAdditionalOutputFormat is only used inside an Option<Vec<_>>, so I don't need Default on it. This PR prevents default from being derived on it, while the main CertificateSpec can stay Default.

Ah, thanks for clarifying. You're searching the other way from what I was expecting. I thought you wouldn't get a Default derive on CertificateSpec with this change. The PR makes more sense now.

(For clarity I typically would have generated this to crds/certificate.rs and in crds/mod.rs add my polyfills to avoid messing with generation, but I do think your argument here is strong in that you shouldn't need to have a default derive on something that's option wrapped like this).

I like the TypedBuilder fix also. Lemme look in more detail.

[AaronDewes]

I now added a flag --force-derive-default that re-enables the previous behaviour for derive, because some users may still want to implement Default for a few enums externally instead, as you suggested.

Please let me know if that sounds okay to you, or if I should instead extend the syntax for --derive or do something else. In my opinion, this new flag is the simplest and probably most understandable approach to iimplement this.

[clux]

i like the optimisation. added some minor import/default cleanups and a suggestion to invert the flag to avoid breaking things. possibly an inverted flag could be generalised to take the type we are doing detection on in the future.

[AaronDewes]

I've implemented your suggestions. The can_derive_default() does not yet take into account whether for nullable fields, a default: value is provided in the CRD. However, such defaults are currently ignored by kopium anyway, so I don't think it's an issue.

I'll check if I can add a way to also implement support for that in another PR, but I'm not sure if that is going to work out.

[clux]

I've implemented your suggestions. The can_derive_default() does not yet take into account whether for nullable fields, a default: value is provided in the CRD. However, such defaults are currently ignored by kopium anyway, so I don't think it's an issue.

I'll check if I can add a way to also implement support for that in another PR, but I'm not sure if that is going to work out.

Yeah, agreed, that is an orthogonal concern atm. But proper handling of it would also be very welcome!

[clux]

I'll merge in and probably do a release early next week. If you have other things you want to try like the default handling I can wait for that.

Thanks a lot for this!

[david-rusnak]

FYI, I think this behavior is currently always on (not opt-in like expected) - even without the --smart-derive-elision flag, none of my structs now derive Default after this new release. I'll keep taking a look to see why that is happening, but just thought I'd flag that here.

[AaronDewes]

There's a typo I didn't notice. i'll fix it

[clux]

Thank you both. I'll ship a patch release for this.