Idea: Let the builder wrap a Result<T,Error>

[nielsle]

Here is a loose idea. It is mostly meant as an inspiration..

It could be to have a builder that wraps a Result. This would allow the builder to validate fields during the build process, and keep track of errors. Rust already allows you to do this with the try!(), macro, and the ?-operator has been accepted as RFC 243, but I still think that it could be nice to have a builder that keeps track of errors.

Here is what the code could look like

// The following only works on unstable
![feature(try_from)]
use std::convert::{TryFrom, TryInto};

#[derive(Debug)]
enum  MyError {
    TooOld
}

// I am not fully sure if it makes sense to annotate the fields
// #[builder(PersonBuilder, MyError)]
#[derive(Default)]
struct Person {
    name: String,
    // #[try_from]
    age: Age,
    // #[try_from]
    mail: Mail
}

#[derive(Default)]
struct Age(u32);

impl TryFrom<u32> for Age {
    type Err = MyError;
    fn try_from(age: u32) -> Result<Age,MyError> {
        if age > 200 {
            Err(MyError::TooOld)
        } else {
            Ok(Age(age))
        }
    }
}

#[derive(Default)]
struct Mail(String);

// We should probably have TryFrom<&str> here
impl TryFrom<String> for Mail {
    type Err = MyError;
    fn try_from(mail: String) -> Result<Mail,MyError>{
        // Parsing goes here
        Ok(Mail(mail.into()))
    }
}

fn main() {

    let _person: Result<Person, MyError> = PersonBuilder::default()
        .name("bob".to_string())
        .age(25)
        .mail("a@b.c".to_string())
        .build();
}

Here is the code that could be derived behind the scenes

struct PersonBuilder {
    inner: Result<Person, MyError>
}

impl PersonBuilder {
    fn name(mut self, name: String) -> PersonBuilder {
        if let &mut  Ok(ref mut inner) = &mut self.inner {
            inner.name = name
        };
        self
    }

    // This code can probably be prettyfied
    fn age<T: TryInto<Age, Err=MyError>>(mut self, age: T)  -> PersonBuilder {
        match age.try_into() {
            Err(e) => if self.inner.is_ok() {
                self.inner = Err(e)
            },
            Ok(v) => if let &mut Ok(ref mut inner) = &mut self.inner {
                inner.age = v
            }
        };
        self
    }


    fn mail<T: TryInto<Mail, Err=MyError>>(mut self, mail: T) -> PersonBuilder {
        match mail.try_into() {
            Err(e) => if self.inner.is_ok() {
                self.inner = Err(e)
            },
            Ok(v) => if let &mut Ok(ref mut inner) = &mut self.inner {
                inner.mail = v
            }
        };
        self
    }

    fn build(self) -> Result<Person,MyError> {
        self.inner
    }
}

impl Default for PersonBuilder {
    fn default() -> PersonBuilder {
        PersonBuilder{
            inner: Ok(Person::default())
        }
    }
}

[colin-kiegel]

That's a very nice proposal. Thank you. :-)

Some minor adjustment:

• the first error should be permanent and not be overwritten by following setter-calls
• alternatively, errors could be stored in a separate vector

What could be the syntax?

• #[derive(Builder(fallible))]
• #[derive(Builder(tryInto))]
• ...

[nielsle]

Perhaps the user should be required to name of the builder, so that the names don't clash when two builders are defined in the same module.

#[derive(Builder(PersonBuilder, tryInto))]
struct Person { ...}

[nielsle]

Regarding errors, I would probably prefer to fail fast and get the first error. That behavior is similar to and_then, but I also see the merit in gathering errors in a vector.

On a slightly related note here isa much more general idea, but I am not sure if it can even be implemented in a beautiful way, and I am not fully sure if it could work with the existing builder, but nevertheless:

Given the following code

#[derive(ResultWrapper(FooWrapper))]
struct Foo;

impl Foo {
    fn new() -> Foo{..........}
    fn bar(self, .........) -> Result<Foo,MyError>{..........}
    fn baz(self, .........) -> Result<Foo,MyError>{..........}
}

let foo: Result<Foo,MyError> = FooWrapper::new()..bar(.....).baz(.....).inner();

It could be great to derive the following code could be derived behind the scenes

#[derive(Default)]
struct FooWrapper{
    inner: Result<Foo,Error>
}

impl FooWrapper {

    fn new() -> FooWrapper {
        FooWrapper {
            inner:Foo::new()
        }
    }

    fn bar(self, .........) -> FooWrapper {
        match self.inner {       
            Err(e) => Result { inner: Err(e) }
            Ok(v) => {
                match v.bar() {
                    Err(e) => FooWrapper { inner: Err(e) },
                    Ok(v) => FooWrapper { inner: Ok(v) }
                }
            }
        }
    }

    fn baz(self, .........) -> FooWrapper {
        match self.inner {       
            Err(e) => Result { inner: Err(e) }
            Ok(v) => {
                match v.baz() {
                    Err(e) => FooWrapper { inner: Err(e) },
                    Ok(v) => FooWrapper { inner: Ok(v) }
                }
            }
        }
    }

    fn inner(self) -> Result<Person,MyError> {
        self.inner
    }
}

The idea is to wrap each method with a method that updates the Result.

[colin-kiegel]

I like the general idea, but I would like from more people in need / favour of this before implementing something like that. :-)

[TedDriggs]

Would this approach satisfy the use-case?

Allow opt-in generation of try_{field} methods, at either the struct or field level. That would produce something like the following:

#[derive(Builder)]
#[builder(setter(into))]
pub struct Person {
    name: String,
    
    #[builder(try_setter)]
    age: Age,
    
    #[builder(try_setter)]
    mail: Mail,
}

// This would be a struct but I'm lazy and didn't want to write the method bodies.
trait PersonBuilder {
    fn name<VALUE: Into<String>>(&mut self, value: VALUE) -> &mut Self;
    
    fn age<VALUE: Into<Age>>(&mut self, value: VALUE) -> &mut Self;
    
    fn try_age<VALUE: TryInto<Age>>(&mut self, value: VALUE) -> Result<&mut Self, VALUE::Err>;
    
    fn mail<VALUE: Into<Mail>>(&mut self, value: VALUE) -> &mut Self;
    
    fn try_mail<VALUE: TryInto<Mail>>(&mut self, value: VALUE) -> Result<&mut Self, VALUE::Err>;
    
    fn build(self) -> Result<Person, String>;
}

fn make_person_with(name: &str, age: usize, address: String) -> Result<Person, Error> {
    PersonBuilder::default()
        .try_age(age)?
        .try_mail(address)?
        .name(name)
        .build()
        .map_err(Into::into)
}

Advantages:

1. Mix fallible and infallible setters
2. Doesn't require all converters to have the same error type
3. Mandatory immediate error handling on setter failure (very Rust-y), but caller has choices (very dev-friendly):
4. Concisely "abort"
5. Call the try_{field} method again with different data
6. Ignore the field and see if the struct will still build - this might be useful if the field was optional or best-effort anyway.

If there's a blanket TryFrom impl for all existing FromStr impls once the trait stabilizes, then this becomes a powerful tool for input validation.

[colin-kiegel]

Interesting proposal. I like the comparably low implementation footprint and self-documenting generated api. :-)

I'm trying to organize the builder attributes in namespaces like #[builder(setter(...))]. According to this design principle, we could put the new attribute into the setter(...) namespace, perhaps #[builder(setter(try))] (or try_into)? Note: we already have #[builder(setter(into))], which just changes the signature of the existing setter, without introducing another variant.

On the other hand, if we do introduce it as something like #[builder(try_setter)] then this could be its own namespace, which could host similar nested attributes like prefix, name, skip (if try_setter was set on the struct, then skipping on a field level might make sense). This would also make it more clear what something like #[builder(setter(skip), try_setter))] should mean as opposed to #[builder(setter(skip, try_into))].

[TedDriggs]

Note: we already have #[builder(setter(into))], which just changes the signature of the existing setter, without introducing another variant.

This is a good point, and probably a case for a second attribute. I like try_setter quite a bit. I'm happy to take a stab at a PR for this one, though I have one question: How do I conditionally use a crate-level feature - try_from - in such a way that the project builds on both nightly and stable? The feature would require nightly for now, but I don't know how to declare a conditional crate-level attribute so that it still builds on stable with the feature disabled.

[colin-kiegel]

Cool. Importantly only the generated code would need nightly, which means, that only the integration test should have this problem.

The crate derive_builder_test already has a feature flag compiletests which requires nightly. You can rename that feature flag to nightlytests and use #[cfg(feature = "nightlytests")] to conditionally enable the integration test. Please also update all references to that feature flag. :-)

[TedDriggs]

Working on this now. I was initially thinking this should be a separate attribute, but I can't think of a good reason why try_setter should be present without setter also being enabled, and the try variant should probably always share visibility with the base setter.

[colin-kiegel]

We need to decide how the API should look like

(a) #[builder(setter(try))] <- piggy-back style
(b) #[builder(try_setter)] <- stand-alone style

Piggy-back style allows us to share settings like visibility and basename. Shared settings are easier to implement and probably sufficient in most cases. The current implementation in PR #72 already uses shared settings, which would match with the piggy-back style API. One thing that is especially nice, is the synchronization of name changes. If you change the setter name to baz the try-setter will becomme try_baz automatically. Very nice.

The stand-alone style makes it much more clear what's happening as opposed to #[builder(setter(try, into, name="baz", skip))] (where try adds another variant and into changes the existing variant and name even changes both variants .. and what would even skip do? This is just confusing).

Can we get the best of both worlds?

Let's call it loosely coupled style:
(c) #[builder(try_setter)] <- will inherit all settings from #[builder(setter)] but can overwrite them.

We can start c with having no sub-options for #[builder(try_setter)]. As in the current PR all settings will be inherited by #[builder(setter)] - the try-setter will only overwrite the enabledness. We can later add other overrides (for name/prefix, visibility) as needed.

Does that make sense?

[TedDriggs]

Works for me, I'm having a hard time coming up with a valid case for any of those properties being different for the try_setter, but I do like the fact that it's clear this creates a second method.

[colin-kiegel]

Ok, cool. We can start without any additional options on try_setter.

However in the future a use case could be this combination #[builder(setter(skip), try_setter(prefix=""))]. That should then disable the standard setter and replace it with the try-setter without the implicit try_ prefix. Another use case for options on try_setter would be if e.g. some french people would like to name it something like foo_try in their language (they often have reverse word ordering) - they could then use the name attribute. But we can leave all of that for a future extension, as long as we know how we could do that.

[colin-kiegel]

Just released v0.4.4 with fallible setters implemented by @TedDriggs 🎉 Thank you! :-)