rust-typed-builder icon indicating copy to clipboard operation
rust-typed-builder copied to clipboard

Ideas about using closures

Open QnnOkabayashi opened this issue 3 years ago • 2 comments

I'll preface this by saying that my idea is a huge breaking change, and I'm more interested to hear feedback on whether this would even work.

Currently, typed-builder requires the programmer calls .build() at the end, and also exposes *Builder types that can be created and thrown around anywhere. This isn't necessarily a bad thing, but semantically it doesn't make sense to ever allow a builder type to not be built to completion, which is possible by just not calling .build().

I propose that we use a closure instead, which I would argue isn't any worse syntactically:

let conf = Config::build(|builder| builder
    .foo(42)
    .bar("Rust <3".into())
);

This way, we no longer have to call .build() manually; whatever is returned by the closure is what gets built. We can also enforce that the only way (in the public API) to get a *Builder is getting passed one as the closure argument, which can never be taken out of context and must get used. This would also require not implementing Clone.

Thoughts? Is there a reason it's not designed like this?

QnnOkabayashi avatar May 01 '22 21:05 QnnOkabayashi

Why go an extra mile just to decrease usability? The builder is already #[must_use], which is more than enough protection for something that has no side-effect. The ability to move partially built builders around and to clone them was requested by users of this crate, and I see no reason to block it. I'm not protecting them from anything - there are no dangerous things that can be triggered by people explicitly using builders as first class rust values.

Also, I disagree with your claim that:

semantically it doesn't make sense to ever allow a builder type to not be built to completion

Consider this:

let conf = Config::builder()
    .foo(some_string.parse()?)
    .bar("Rust <3".into())
    .build()

Converting this to your closure style won't work:

let conf = Config::build(|builder| builder
    .foo("some_user_string".parse()?) // can't just ? from a any old closure...
    .bar("Rust <3".into())
);

We'd have to add another method:

let conf = Config::try_build(|builder| Ok(builder
    .foo("some_user_string".parse()?)
    .bar("Rust <3".into())
))?;

This is already starting to get a little weird, but it gets worse. That code won't type-check. The reason is that for obvious reasons we want to make try_build's error type generic - let's call it E. People imagine Rust's result? to be like:

match result {
    Ok(ok) => ok,
    Err(err) => {
        return Err(err);
    }
}

But it's actually more like

match result {
    Ok(ok) => ok,
    Err(err) => {
        return Err(err.into());
    }
}

Noticed the into()? Rust will try to convert the result to the result of encasing function. Which is usually a good thing - saves you a .map_err(|err| err.into()) - but in cases like this it can be quite a problem. The reason is that we have two ?s:

  • The first one, from .parse()?, converts the ParseIntError to E.
  • The second one, from ::try_build(...)?, converts the E to whatever error type the encasing function has in its Result.

But... Rust doesn't know what E is! It can be any type that supports these two conversions.

So, we'd have to do something like this:

let conf = Config::try_build(|builder| Ok::<_, ParseIntError>(builder
    .foo("some_user_string".parse()?)
    .bar("Rust <3".into())
))?;

At this point, would you not agree that it's at least a little bit "worse syntactically"?

idanarye avatar May 01 '22 22:05 idanarye

I didn't really consider the case where we might want to ? within the closure, and you're right that my idea totally breaks down here. Thank you for writing all this up!

QnnOkabayashi avatar May 01 '22 23:05 QnnOkabayashi