#[builder(default)] on struct-level does not use Default impl of the struct

[TedDriggs]

rustc version: 1.17.0-nightly (6eb9960d3 2017-03-19)
derive_builder version: 0.4.0

When a struct has a non-derived impl Default, those values do not get used in the output of the builder.

Repro

#[macro_use]
extern crate derive_builder;

#![cfg(test)]

#[derive(Debug, Clone, PartialEq, Eq, Builder)]
#[builder(default, setter(into))]
pub struct Example {
    pub a_flag: bool,
    pub limit: Option<String>,
    pub third: String,
}

impl Default for Example {
    fn default() -> Self {
        Example {
            a_flag: false,
            limit: Some("20".into()),
            third: "Hey".to_string(),
        }
    }
}

fn main() {
    let struct_update = Example {
        a_flag: true,
        ..Example::default()
    };
    
    let builder = ExampleBuilder::default().a_flag(true).build().unwrap();
    
    assert_eq!(builder, struct_update);
}

This panics with the following error:

panicked at 'assertion failed: `(left == right)` (left: `Example { a_flag: true, limit: None, third: "" }`, right: `Example { a_flag: true, limit: Some("20"), third: "Hey" }`)'

Expected: The resulting structs are equal.

[colin-kiegel]

This is an interesting proposal, but it would be a breaking change.

Right now #[builder(default)] on the struct level has a very different meaning - it is just sugar for having #[builder(default)] on each field.

#[derive(Builder)]
pub struct Example {
    #[builder(default)]
    pub a_flag: bool,
    #[builder(default)]
    pub limit: Option<String>,
    #[builder(default)]
    pub third: String,
}

I think the current meaning can also be useful in some cases. I'm not sure which one is more important and I would like to hear more opinions about it.

cc @killercup do you have an opinion about this?

[TedDriggs]

I'd interpreted this comment on #3 to mean emulating serde's behavior, which does use the Default implementation. IMO the by-field approach is the far less common one, and that should be explicitly opted into either by adding that annotation to each field or using something like #[builder(default(by_field))].

[TedDriggs]

After looking at this some more, I don't think the current implementation should be considered valid behavior.

The following assertion seems expected for any type:

assert_eq!(Ok(Example::default()), ExampleBuilder::default().build());

Worse still, #[builder(default)] now means different things at the struct level vs the field level for the same type. If I make a new struct:

#[derive(Debug, Clone, Builder)]
#[builder(default)]
struct Outer {
    inner: Example
}

then the invariant I wrote above holds, because derive_builder's current default semantics would have invoked Example::default().

If this change gets made quickly enough, then there won't have been much time for people to take a dependency on the wrong behavior. The current behavior could be opted into by adding a second method to the builder blank(). That also gives control over which one to start from to the caller, rather than leaving it to the struct creator.

Update: I'm trying to work on a pull request for this; haven't worked with proc macros before, so it may be a little rough. Hoping to send a PR out soon.

[colin-kiegel]

Ok, you've convinced me that there are strong reasons to change the behaviour. Especially the precedence case of serde was already very convincing. :-)

I thought about how to introduce this change and came up with this:

• release a maintenance version 0.4.1
  • introduce a new feature, which is opt-in (=non-breaking change), which changes the meaning of #[builder(default)] on the struct level as suggested
  • at the same time emit a deprecation note if #[builder(default)] is used on the struct level without the new opt-in feature
• at some point in the future bump to 0.5
  • remove the opt-in feature + deprecation
  • have the new meaning for everyone

This compatibility-dance adds a little complexity to the actual implementation, but I'd be willing to do it, if only for practising it. :-)

Btw. thank you very much for starting the implementation! 👍
Would you rather get some guiding feedback on how to proceed with the PR, or would you prefer me to complete it (according to what I called the 'compatibility-dance')?

[TedDriggs]

I'll give it a shot this afternoon; I think I know how to do everything except for the deprecation warning on usage-without-cfg flag.

[colin-kiegel]

released v0.4.1, which deprecates the old behaviour and offers an opt-in to the new behaviour via --feature "struct_default". Thanks for the discussion and the PR. 👍