Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ideas about using closures #66

Open
QnnOkabayashi opened this issue May 1, 2022 · 2 comments
Open

Ideas about using closures #66

QnnOkabayashi opened this issue May 1, 2022 · 2 comments

Comments

@QnnOkabayashi
Copy link

QnnOkabayashi commented May 1, 2022

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?

@idanarye
Copy link
Owner

idanarye commented May 1, 2022

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"?

@QnnOkabayashi
Copy link
Author

QnnOkabayashi commented May 1, 2022

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants