generate a (customizable) constructor

[colin-kiegel]

Currently all builder structs implement the Default trait. Using the Default trait is the proposed way to create a new builder instance right now.

As mentioned in this comment by @faern it would be nice to have a dedicated constructor. We could make the identifier customizable (defaulting to new) and we could offer opting out of generating a constructor.

Possible API

• #[builder(constructor(name="brand_new", ...))]
• #[builder(new(name="foo", private, ...))]
• ...

Questions:

• should we offer renaming the constructor?
• should we offer to make the constructor private? (such that it can be wrapped - for easier re-ordering, or
  something else)?
• how should we namespace these settings?

Outlook

This could allow better support for infallible build methods #56, if the build method would already take all required values (new(a, b) instead of new().a(a).b(b)).

Limitations:

• the order of required fields in the constructor signature would most likely be identical with the order of fields. I would not like to offer an API for reordering fields, or something similar. In certain edge cases you might need implement the constructor manually - which will always be possible of course.

[faern]

I like the #[builder(constructor(...))] approach better than #[builder(new(...))], since constructor is a more correct name for the type of function, whereas "new" usually is the name of the function.

I think starting out simple can be desirable, not possible to rename the constructor and not possible to reorder fields.

Are you thinking that if one ask to get a constructor generated for them, it automatically takes all required fields as arguments, or would the default be to just work like how default works today?

[colin-kiegel]

Ok, I also like #[builder(constructor(...))], but just wanted to ask the question, especially since it's a bit longish.

Yes, starting simple is a valid option. But I don't think I would ever support reordering fields - just wanted to make that clear upfront.

Yes IMO the constructor should always take all required fields (those that don't have a default). This behaviour would be completely independent of fallible/infallible choice. This has the advantage that method signatures are somewhat more consistent across different derived builders. The new function would always tell you the required fields. If you then opt into infallible build methods and you do have required fields, then the Default trait would not be implemented anymore and the signature of the build method would change.

Does that make sense? :-)

[faern]

I agree with never supporting field reorders. Then one can just reorder the fields of their struct instead to get that result.

I completely agree with the constructor always taking arguments, no matter infallible or not. Better not create too many possible variants of builders. I like that the constructor always takes the values of the required fields.

I have to say I think it's kind of odd that builders with required fields do implement Default as it is actually. Doing FooBuilder::default().build() translates to "give me the default Foo" in my head, but it will error if there are required fields since there is no such thing as a "default Foo". To me T::default() should give a sane default of T, but in the case of the builder it's not really a sane default since it's in an unusable state until one calls a couple of setters. I think the Default trait should only be implemented on builders where the object we are building can be created with only defaults.

[colin-kiegel]

Hm, I don't find it odd. :-) Because the Default trait would only ever be implemented if the build method is fallible. That basically targets the scenario, where you don't statically know the input. In that case we need some constructor that initializes the struct with None values for all fields, including the required ones. And that can't be the new contructor - since that will take all required fields. I think Default is just a perfect fit here, since it never takes any arguments. Again this only targets the use case, where you may not always know all the input up front.

So maybe it just seems odd, because we only have the fallible behaviour currently? As soon as you opt into the infallible behaviour, you would basically tell derive builder that you'll always know everything up front and the Default implementation would disappear.

The updated documentation would definitely use new instead of default all over the place.

Another reason to keep the Default impl is backwards compatibility.

[TedDriggs]

I have to say I think it's kind of odd that builders with required fields do implement Default as it is actually. Doing FooBuilder::default().build() translates to "give me the default Foo" in my head, but it will error if there are required fields since there is no such thing as a "default Foo". To me T::default() should give a sane default of T, but in the case of the builder it's not really a sane default since it's in an unusable state until one calls a couple of setters. I think the Default trait should only be implemented on builders where the object we are building can be created with only defaults.

@faern have a look at #61 and #62 - the default case is getting fixed.

[colin-kiegel]

To avoid any confusion, I think there are two separate discussions about Default going on. This discussion is "when does it make sense for FooBuilder to implement Default" and the other one is "what does foo_builder.build() do, when Foo implements Default and you specified #[builder(default)] on the struct level". :-)

[TedDriggs]

How much code does this save the crate user, vs. adding their own impl block to the builder struct? I'm worried about documentation for the generated constructor method, and I'm not sure people would expect a field reordering in their source code to change method signatures in ways that might not even break their build. Consider the case of Credentials { username: String, password: String }

[colin-kiegel]

Documentation: As long as the fields of the target struct are documented, we could collect that documentation and stitch it together to get a generic documentation for the constructor. That feels ok to me. :-)

Reordering: I'm worried about that, too. Here are some random ideas:

• We could try to force users to write unit tests somehow ...

#[cfg(not(test))]
// For `cargo build` we emit the method as normal.
pub fn new() {}

#[cfg(test)]
#[cfg_attr(test, forbid(unused))]
// If `cargo test` is run, we make add `#[forbid(unused)]`.
// This forces the user to write something like a unit test,
// thereby locking down the signature. Note that 
// `#[forbid(unused)]` does not work with exported items.
// To prevent that, we can make the method private.
fn new() {}

This approach however is not perfect, because we need to make the function private for cargo test, which would be very surprising. Or we could conditionally emit a dummy method new_check_my_signature(). But I also doubt that the usability would be good, for someone not familiar with this hack.

• Always emit a private constructor and force the user to wrap it, if it is supposed to be public.

• Or .. let the user specify the order upfront .. #[builder(constructor(args(foo, bar, baz)))]. That would render the constructor opt-in.

Note: The important value of the constructor with required fields is, that it would enable us to have a non-panicking infallible build method.

[faern]

@TedDriggs For me this adds much more value than just adding a constructor (that a user can do manually with the existing version). I'm looking forward to infallible builds so I can get rid of the Default impl and thus statically get rid of one kind of error.

I don't see reordering as that much of a problem actually. In the same way that I have to be careful to not reorder arguments in my public API I should be careful to not rearrange structs if I know they will have code generated for them. But I agree it adds one level of indirection that is easy to forget. I just really don't like the thought of having to maintain two lists of my required fields, first the fields themselves and then the order in which they come again.

[TedDriggs]

If the author has to specify all the arguments and their positions anyway, then all we seem to have saved them is some generic type declarations (if they want to use Into) and the method body.

Note: The important value of the constructor with required fields is, that it would enable us to have a non-panicking infallible build method.

I'm looking forward to infallible builds so I can get rid of the Default impl and thus statically get rid of one kind of error.

I still don't think it's good for the exporting crate to make the decision that the builder must be infallible; the author can't know how the consuming crate will populate the object - it could have all the values hardcoded and is using the builder for convenient conversions, or it could be parsing user input into the builder and have no idea whether or not it's going to succeed.

Alternative using Session Types

If derive_builder was able to export types, then an alternative might be using a session type like Hyper does for Fallible vs. Infallible builders. The constructor method would return FooBuilder<Infallible>, and there'd be an impl declared, impl From<FooBuilder<Infallible>> for Foo, which enabled the conversion. In the case where the builder declared a default, there would be a similar infallible conversion for the default case.

Session types could also be useful for converting a fallible builder to an infallible one, verifying that all required fields were present and performing a "pre-build".

[colin-kiegel]

I'm not sure if I understood everything correctly, but it sounds at least comparable to this idea of typesafe builders #33. I.e. encode the state of the builder with the typesystem and use generic bounds to tell the compiler, whether the build method can be called safely.

That's actually a feature I am very much looking forward to. It might very well blow up compile time, but it would be a really cool showcase for derive_builder. :-)

If we want to dig deeper into that direction, let's discuss it in #33 instead to keep the discussions organized (..it's already all so intertwined ^^).

[xNxExOx]

I would like to ask and maybe extent the original idea with my use case.

I tried to find a way to disable FooBuilder::default() somehow, but the generation code is too complicated for me to find out if it is possible, or not. (Some parts of documentation looks like it can be generated without the Default)

I need a constructor for the builder (and I have no issue writing it myself), to force user to enter the required fields, and all other fields are optional, but the builder can be constructed without those fields, which is kind of confusing, and even more, if I disallow setting them.

[TedDriggs]

@xNxExOx having a way to disable emitting default sounds more reasonable scope-wise than custom constructor generation. Unfortunately, I don't have time at the moment to implement it, but I'm happy to review a PR that adds it as an option. We'd need to think about the option naming to avoid confusion with field defaults.

[TedDriggs]

See discussion on #56 for this; custom constructors (and the resulting infallible build methods) are going to stay out of scope for this crate, but they can be incrementally wrapped around a generated builder to present the desired API to users of that builder.