Design: Builder methods and argument validation

I'm writing a builder struct (the self-mutating kind, not the self-consuming kind), and some of its methods take strings that must follow certain formats (a different format for each method). The obvious implementation is to make these methods return Result<&mut Self, Error>, with Err(Error) returned if the string is invalid. However, I've never seen builder methods that return Result before, and I suspect that such a method may violate best practices in some way. The alternative is to create dedicated types for each argument string format so that the type system ensures validity, but this seems like overkill; in particular, the only difference from the user's perspective would be that builder.some_param("foo")? becomes builder.some_param("foo".parse()?) — more typing (with a keyboard) for more typing (with a type system).[1]

What say you, Rustaceans? Are builder methods that return Result<&mut Self, Error> acceptable, or should the user be required to parse() their string arguments instead? Something else?


  1. Maybe that should be the Rust motto? ↩ī¸Ž

Using separate types would let you reuse the validation in other places, but if you never need that I agree it's probably overkill.

I think traditionally all of the validation logic that might fail is moved to the build method that actually constructs the new type from the builder. I wouldn't say there's necessarily anything wrong with returning a Result from a builder method though.

3 Likes

Having build() return a Result is not an option that occurred to me. While I can see the appeal, I don't really like the idea of the error occurring at a different point than where the invalid argument was given, as either the user wouldn't know which argument was bad or else I'd have to add a parameter identifier to the error struct.

Adding ? to the end is a 1-character change so I would say it's perfectly acceptable for a builder method to return a Result<&mut Self, Error>. That also allows you to tack on extra context (e.g. builder.some_param("foo").context("Unable to parse some param")? if you use anyhow).

Builders are meant to be convenient after all, so you'll often accept things like impl Into<String> or impl SomeTrait + 'static to allow users to pass in more types. I'd be perfectly fine if you had something like fn with_url<U>(&mut self, url: U) -> Result<&mut self, Error> where U: TryInto<Url>. That would open the door for passing in string literals (e.g. during tests or when the value is hard-coded) or a pre-parsed Url object you already have.

I think that's overselling it a bit. It's only a one character change in a context where ? can convert to your return type. A library that goes from infallible to Result isn't typically a one character change for the consumers, it's at least a .map_err or impl From or such.

That said, if your system is built around parsing, there's no avoiding being fallible, in which case I'd agree it's perfectly acceptable to return a Result (and preferrable to panicking, say).

But if you go the more typing for the sake of more types route, it's often possible to provide an infallible alternative, such as an enum to represent the parsed results. If I can code the infallible

builder.some_param(Foo::Bar(17))

then I don't need to code the fallible

builder.some_param("foo".parse()?)

I think both the individual steps and the final build method returning Results is reasonable, depending on the kinds of errors.

Like .name(the_name).unwrap() might make sense if it can validate the name in isolation somehow. Then .build() being fallible would be for things that can only be checked by looking at a combination of fields.

3 Likes