Typesafe builders

[cramertj]

With macros 1.1, it should be easy to implement builders like this one. This would allow the type-checker to confirm that all required (non-defaulting) fields are set exactly once. Would you consider an addition to the library implementing this feature? I'd be happy to put together a PR if there is interest.

[killercup]

Ohh, that looks interesting! IIUC, you define a new struct with the same field names, but each field has a type parameter that is of type Unset by default (which is a unit struct). Calling methods will override the fields values as well as their types.

This is a far nicer version of the Optional Builder described in #32 (comment). Also, I love session types!

I'm hoping we can land #32 soon-ish. A PR building on that would be great. What do you think, @colin-kiegel?

@cramertj, what API are you envisioning? I assume you want to expand

#[derive(Builder)]
#[setters(fancy_name_here)]
struct Foo {
    bar: i32,
    baz: i32,
}

to something like

struct Foo {
    bar: i32,
    baz: i32,
}

mod foo_builder {
    struct FooBuilderFieldUnset;

    struct FooBuilder<T0 = FooBuilderFieldUnset, T1 = FooBuilderFieldUnset> {
        bar: T0,
        baz: T1,
    }

    impl<T0, T1> FooBuilder<Unset, T1> {
        fn bar(self, bar: i32) -> FooBuilder<i32, T1> {
            FooBuilder {
                bar: bar,
                baz: self.baz,
            }
        }
    }

    impl<T0, T1> FooBuilder<T0, Unset> {
        fn baz(self, baz: i32) -> FooBuilder<T0, i32> {
            FooBuilder {
                bar: self.bar,
                baz: self.baz,
            }
        }
    }

    impl FooBuilder<i32, i32> {
        fn build(self) -> Foo<i32, i32> {
            Foo {
                bar: self.bar,
                baz: self.baz,
            }
        }
    }
}

use foo_builder::FooBuilder;

[colin-kiegel]

This is definitely an intersting idea and generating the code should be no problem.

IMO the most critical part is the API - since there are ideas/requests to extend this crate into very different directions. I think we should be careful here not to end up in a messy situation.

Let's say we have these different 'modes'

• simplified builder: One struct Foo only, which is the builder and we don't care what will be built with that.
• traditional builder: Two structs Foo and FooBuilder where we generate the latter.
• typesafe builder: As above - similar to the traditional builder.
• .. or even forward the method to an internal struct? (#31)

I assume those modes would be mutually exclusive.

On top of that we have additional options and features and I wonder if all of these have the same meaning or any meaning for all of these modes. I would like to go through as many combinations as we can think of beforehand:

• setters: Do we want to be able to generate setters on Foo too in the TB-case? Or should the user be able to choose?
• setter settings probably do the same in both SB- and the TB-case - unless we need more of them in the TB-case.
  ** pattern: mutable/immutable/owned
  ** setter type conversion: as soon as we make this optional
  ** setter visibility
  ** skipping individual setters
• getters: Should these getters end up on Foo or FooBuilder for the traditional/typesafe builder (TB), or both?
• custom defaults: Should these only be relevant for FooBuilder in the TB-case, or independently just a Default impl for Foo, or both?
• Let the builder wrap a Result<T,Error> #25 probably only makes sense in the TB-case.

So basically what I would like to do is: Take all settings from the simplified builder (SB) case and define their meaning in the TB-cases and vice-versa.

Does that make sense to you?

If anyone wants to start a suggestion please go ahead. You can find the intermediate settings documentation of the macros 1.1 re-implementation here. :-)

[killercup]

Great points, @colin-kiegel. The API surface might indeed get quite large if we keep on adding new shiny things. Let me think about that for a minute.

Regarding getters: Are these part of the builder pattern? It makes sense to have them to avoid leaking the internal names of fields (or even to conversions), but it might be an option to not offer them in this crate. The accessors crate might be a good alternative. It's name suggests generating 'simple' methods (getters, setters).

Personally, I want to make sure we generate code that is useful to a lot of people in practice and that tries to be as good or better as the code you'd write yourself. The simple setters we currently generate are nice, and try to be a bit more clever than what you'd write yourself (using Into). Typesafe builders are something I'd probably never write myself, as they are far too much boilerplate code that I could get wrong. So, on that scale, implementing typesafe builders might offer something people didn't have before.

Sadly, only implementing typesafe builders is not really an option, as overwriting set values can be a valid use case (e.g. when passing partially built builders around). Also, debugging them will require some deeper understanding (missing method errors with a lot of constraints on a lot of type parameters).

Hybrid approaches might also be interesting. Not restricting fields to only be set once is just a matter of loosening some constraints. Keeping the final build method on fully defined builder is a great property to have. I would also argue that having access to default values for fields is a relevant use case.

tl;dr Let's get rid of 'simple' builders in favor of an accessor crate, and implement traditional or typesafe builders or a mix of both.

[cramertj]

Personally, I think that #[derive(Builder)] looks like it should implement a trait called Builder for a given type. It makes sense to me to have each functionality as a separate derived "thing", and perhaps even move the accessors into a different crate. Something like this, perhaps: [derive(Builder, TypesafeBuilder, Setters, Getters,...)]. Builder and TypesafeBuilder traits could include builder and safe_builder methods, respectively.

[colin-kiegel]

This is a fun discussion.

@cramertj Hm, I think the main traits we can implement are From<FooBuilder> for Foo and/or TryFrom. :-)

On top of that we could derive a FooBuilderTrait where all type parameters are associated types. But associated types can't be erased via boxing. Therefore I don't see a use case for such a trait. Plus you'd need to import this trait into any scope to be able to use setter methods on FooBuilder - ugh. 😟 .. Do you think there is a use case to have such a FooBuilderTrait (maybe optionally)?

I guess that's as far as we will get with abstracting the builder pattern or anything else into traits here.

@killercup ok, let's go orthogonal to the accessors crate and not try to do to much at once. We could even get in touch with the maintainers of the accessors crate and try to sync our APIs to some degree (prefixes, skipping fields, etc.). I think it would be cool if these could feel like siblings. But it's no deal breaker if not.

In that perfect synced world, we would advocate to use the accessors crate for deriving getters on Foo. While our crate could derive getters on the derived FooBuilder with basically the same syntax.. But who knows - maybe we don't need getters on FooBuilder.

We should then use a different namespacing for our attributes, e.g. #[builder(setters)] instead of #[setters(...)] etc. If we are sure that we won't have getters, we could omit the attribute namespace for setter attributes. Or we could only namespace getter attributes, but not setter attributes - to keep frequently used attributes as compact as possible. This way we could also reintroduce getters on FooBuilder later.

Can we argue similarly with CustomDefaults? We already said this should be something separate. Suppose there is a crate for that, which I am sure there will be soon enough, if not already. We don't need to implement CustomDefaults for Foo, but maybe we need a similar concept for FooBuilder and we might want to share our syntax/api with some other crate to create a good user experience if used in combination. Also not a deal breaker if it's not synced with anything else.

If typesafety is a global property, we could indeed promote it to the level of the derive entry point #[derive(TypesafeBuilder)] as @cramertj suggested. That's definitively a plus in compact syntax for our options. However, if we want to set/override this on a per field-level, we might want to stick with something more like #[derive(Builder)] #[builder(typesafe)], since #[builder(typesafe)] could also appear on a field. Or we allow both and treat the first variant just as shorthand notation if that's not too confusing.

I see some trouble in deriving a TypesafeBuilder for a generic Foo<T>. If all fields are unset, then FooBuiler does not need the type parameter T. But if only some fields are set, we'll have to figure out if one of them needs the parameter T. So maybe we need to start TypesafeBuilder with the restriction to non-generic Foo and figure that out later.

That's it for now.

[TedDriggs]

This would be much easier to implement I suspect if the builder had two states, expressed as session types:

1. AllRequiredFieldsSet
2. MissingRequiredFields

A custom method to initialize the builder could demand all the required fields and then return FooBuilder<AllRequiredFieldsSet>. That particular implementation would have a safe build method (possibly with a different name than the fallible build method).

For cases where the caller can't invoke that method, a second method check_required_fields() can be generated with the return type: Result<FooBuilder<AllRequiredFieldsSet>, String>. This would enable upgrading a builder once all fields are present. As long as we don't allow unsetting of fields, this would work well.

To avoid an absolute explosion of types, I think we'd need to figure out a way to export this state trait and the two empty enums once, directly from the crate.

[rubdos]

Hi! I just bounced on this issue Googling for the exact thing you guys are trying here. I am not using the crate, but I may want to file some Rust RFC. I think you guys may be interested in this little syntactic thing.

Consider my current code:

pub struct EdgeBuilder<F, T> {
    from: F,
    to: T,
}

impl Default for EdgeBuilder<(), ()> {
    fn default() -> EdgeBuilder<(), ()> {
        EdgeBuilder {
            from: (),
            to: (),
        }
    }
}

impl<F, T> EdgeBuilder<F, T> {
    pub fn from(self, vertex: &Vertex) -> EdgeBuilder<[u8; 32], T> {
        let EdgeBuilder {
            from, to
        } = self;
        EdgeBuilder {
            from: *vertex.owner.bytes(),
            to,
        }
    }

    pub fn to(self, vertex: &Vertex) -> EdgeBuilder<F, [u8; 32]> {
        let EdgeBuilder {
            from, to
        } = self;
        EdgeBuilder {
            to: *vertex.owner.bytes(),
            from,
        }
    }
}

impl EdgeBuilder<[u8; 32], [u8; 32]> {
    pub fn build(self) -> ::crypto::Result<Edge> {
        Err(error::Unspecified)
    }
}

(written by hand), versus the code I would want to write:

pub struct EdgeBuilder<F, T> {
    from: F,
    to: T,
}

impl Default for EdgeBuilder<(), ()> {
    fn default() -> EdgeBuilder<(), ()> {
        EdgeBuilder {
            from: (),
            to: (),
        }
    }
}

impl<F, T> EdgeBuilder<F, T> {
    pub fn from(self, vertex: &Vertex) -> EdgeBuilder<[u8; 32], T> {
        EdgeBuilder {
            from: *vertex.owner.bytes(),
            .. self
        }
    }

    pub fn to(self, vertex: &Vertex) -> EdgeBuilder<F, [u8; 32]> {
        EdgeBuilder {
            to: *vertex.owner.bytes(),
            .. self
        }
    }
}

impl EdgeBuilder<[u8; 32], [u8; 32]> {
    pub fn build(self) -> ::crypto::Result<Edge> {
        Err(error::Unspecified)
    }
}

The difference lies in the elision:

    pub fn to(self, vertex: &Vertex) -> EdgeBuilder<F, [u8; 32]> {
        EdgeBuilder {
            to: *vertex.owner.bytes(),
            .. self
        }
    }

which is currently not possible because EdgeBuilder<F, [u8; 32]> is not EdgeBuilder<F, T>, so this automatic destructuring is not possible.

---

So, let me know whether you guys are interested in this, I may need some help doing the first RFC stuff :-) This may or may not reduce code generation for you guys. Not sure.

[TedDriggs]

Revisiting this thread. I agree with @cramertj that a Builder trait should exist; I'd imagine it looking like this:

pub trait Builder {
    type Builder;
    fn builder() -> Self::Builder;
}

The set-once builders feel like they might belong in a separate crate, maybe? While the consuming code might not change much between the different modes, the generated docs would change radically, and any code that's handing these across a function boundary would have to change as well.

[TedDriggs]

The exactly-once semantics, session types, etc. feel out of scope for this crate, and it'd be a substantial breaking change for all existing users. I'm disinclined to ship this in derive_builder, but anyone who wants to make a new crate is welcome to.