structured errors instead of String

[colin-kiegel]

The build method currently returns Result<Foo, String>. But representing errors as String is not very rustic.

It's also a show-stopper for using derive_builder with #![no_std] on the stable toolchain. The reason is that String on no_std requires the collections crate, which is marked as unstable.

These are already two reasons that representing errors as String is not optimal.

Proposal

Find a way to return structured errors instead of Strings. On std they should implement the usual fmt traits. On no_std they should be compliant with the stable toolchain.

Questions

The main question is, how do we get the structured errors into scope? This is unfortunately not straightforward, since derive_builder is a proc_macro crate.

proc-macro crate types cannot export any items other than functions tagged with #[proc_macro_derive] currently.

So we need to work around that limitation somehow. These ideas come to my mind right now

1. We could add a derive_builder_common crate, which just exports traits / types / impls and must then be imported by any user of derive_builder.
2. We could add a flag to derive_builder to emit the traits / types / impls directly into the target crate.
3. Each derivation with derive_builder could generate unique error definitions for that struct.
4. Wait until proc_macros can export items.

All these approaches have their own drawbacks.

• 1+2 add boilerplate and complexity for users of derive_builder. The out-of-the-box experience is a bit impaired.
• 3 introduces unnecessary redundancy (and complexity). This might get ugly if a user has multiple derived builders and does pattern matching on the errors.
• 4 sounds like macros 2.0 which will most certainly take quite a while from now to land on stable.

cc @killercup

[killercup]

I'd go with 3. I'm not sure how much code for the error type we are talking about, but I'd be surprised if it ended up being more than 50 lines or so, with most of them trait impls.

I wouldn't worry about the "matching on errors" case, as it's not really possible right now anyway. (Aside from comparing the strings, which, given a Display impl, users can still do.)

[colin-kiegel]

Ok. Hm, if someone wants to wrap that error they would either need one variant for each builder FooBuilderError, BarBuilderError or box it as some trait object. But then again std::error::Error is not in core, for some reason not obvious to me. Hmm... I guess on core, we would just implement Display and/or Debug then, and people could wrap that?

Here is a supposedly core + stable-compliant draft:

#[derive(Display)]
pub enum FooBuilderError<'a> {
    // pre-defined errors:
    UnitializedField(&'static str),
    // ...
    //////////////////////////////
    // user-defined errors:
    CustomDefault(&'static str, &'a Display),
               // ^~~ by convention the first entry could be the field ident?
    // ...
}

Pre-defined errors could refer to field idents via &'static str. I don't see that pre-defined errors need to reference dynamic content, because I'm currently not planning to have pre-defined validators.

But user-defined errors should be able to do dynamic stuff. Without collections on core + stable we are then left with a reference to some trait like &Display with finite lifetime 'a.

So here are two problems I see with that draft:

• Since this lifetime ends up in return position of build), we need to bind it to a input reference afaik. But we can only do that in the mutable/immutable pattern. How could we tell the compiler how long 'a is supposed to life? Afaik we can't have a function that's generic over some 'a in return position only with safe Rust, right?
• On std (or core with collections) it would be uncomfortable to pass this error around, because the lifetime of the error would be limited to the lifetime of the builder. Users would not be able to propagate beyond that. And all wrapping error types would inherit the lifetime.

I currently think we won't get any farther than that - at least on stable + core. Do I maybe miss something?

So maybe we could have one definition which targets core specifically and another one which targets std and is more ergonomic?? I believe that performance should generally only be a concern on the happy path - so I wouldn't worry about allocations on the unhappy-path, but instead focus on ergonomics.

As a side note: Speaking of core, as a workaround we could simply replace String with &'static str, since we know at least all the pre-defined/generated errors at compile time. But users of derive_builder would be restricted to only inject statically known errors through hooks like custom default (or other hooks yet to come). One compromise could be, that we could switch the error type on no_std only - or make it a no_std-independent switch. That would not be a structured error, but at least no_std could be used on stable.

[colin-kiegel]

Hm, if we are generating builder-specific error definitions anyway, we could allow users some flexibility - at least in theory. I'm quite skeptical that this would blow up the public API and learning curve of derive_builder significantly.

Anyway some annotation could tell derive_builder that the custom default error will always be MyCustomDefaultError or MyCustomDefaultErrorRef<'a>, which you could then implement yourself. We could then collect the lifetimes of the custom error types and add it to the definition of FooBuilderError<'a, 'b, ...>.

Currently I don't believe that this would lead to a nice API, but I may be wrong.

PS: These builder-specific errors would also be harder to migrate to a more centralized definition in the future - in case proc_macros would be allowed to export items at some point.

[killercup]

I was thinking of a pretty abstract struct FooBuilderError(&'static str) (not even an enum) that just contains a static error messages we can generate (e.g., "Field lorem missing") or a hard-coded generic message (e.g., "Shit's on fire, yo").

I would not make this a complicated API at all – the struct should probably only have one impl: AsRef<str> to get the error message (the inner field is private). Custom errors sound way to complex for now.

[colin-kiegel]

Ok, so basically just a newtype then. That would be more forward-compatible, if we keep adding more failure modes. With the enum based approach, each new variant would be a breaking change. So this is a strong argument for a plain newtype (or a Other variant for all future failure modes). Ok, let's assume we do the plain newtype.

I still think the newtype could just wrap String on std then, no? As mentioned before, I don't consider performance the primary concern on the Err-path if Result is used idiomatically.

I do plan to add user defined filters for fields which could then emit custom errors with dynamic information. `1277.0.0.1` is not a valid IP address, the first byte `1277` is not within the range `0...255` instead of not a valid IP address, the first byte is not within the range `0...255` .

We could support this with

#[cfg(no_std)]
struct FooBuilderError(String)
#[cfg(not(no_std))]
struct FooBuilderError(&'static str)

Going all &'static str is less flexible (and a breaking change), but also less complex to document/learn/implement. :-)

I haven't made up my mind and would like to collect some possible use cases for custom error messages and see how important it is to have this flexibility. Of course the longer we wait the more painful the breaking change might be for other people..

I feel like the current custom defaults are unlikely to error. We should revisit this question at latest when we come to those user defined filters or something similar.

[killercup]

Yeah, wrapping a string but no exposing it on std is a nice way to add nicer error messages. Colin Kiegel <notifications@github.com> schrieb am So. 26. März 2017 um 16:37:

…

Ok, so basically just a newtype then. That would be more forward-compatible, if we keep adding more failure modes. With the enum based approach, each new variant would be a breaking change. So this is a strong argument for a plain newtype (or a Other variant for all future failure modes). Ok, let's assume we do the plain newtype. I still think the newtype could just wrap String *on std* then, no? As mentioned before, I don't consider performance the primary concern on the Err-path if Result is used idiomatically. I *do* plan to add user defined filters for fields which could then emit custom errors with dynamic information. `1277.0.0.1` is not a valid IP address, the first byte `1277` is not within the range `0...255` instead of not a valid IP address, the first byte is not within the range `0...255`. We could support this with #[cfg(no_std)]struct FooBuilderError(String) #[cfg(not(no_std))]struct FooBuilderError(&'static str) Going all &'static str is less flexible (and a breaking change), but also less complex to document/learn/implement. :-) I haven't made up my mind and would like to collect some possible use cases for custom error messages and see how important it is to have this flexibility. Of course the longer we wait the more painful the breaking change might be for other people.. I feel like the current custom defaults are unlikely to error. We should revisit this question at latest when we come to those user defined filters or something similar. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#60 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABOX-0woDiRbBd5qCohNYM_UJD6_Ursks5rpngugaJpZM4MpMi1> .

[colin-kiegel]

Ok, cool. :-)

We should also think about which traits we want to implement. Since we do code generation, each trait we add later would be a breaking change - the user could have already just impl'd this trait.

• Error on std and Display/Debug on core would be obvious.
• I guess Clone wouldn't harm.
• I don't know, if we want to have AsRef<str> and/or PartialEq / Eq?
• I haven't done any concurrent programming with Rust, so I'm unsure about Sync / Send?

[colin-kiegel]

Hm, that question applies to the builder, too. I already had the idea to introduce a #[builder(derive(Bar))] annotation, which would add #[derive(Bar)] to FooBuilder - since the ability to implement one of these traits might very well depend on the fields of Foo. However with the errors I would like to have a fixed set, to keep it simple. :-)

[colin-kiegel]

Hm, there may be a trick to get around the limitation of proc_macro crates not being able to export items. It would involve another crate to re-export the custom derive (+additional items). I haven't tried it yet.

[TedDriggs]

The error-chain crate has looked at a lot of these issues before; it might be worth considering taking a page from that work.

The UninitializedField variant feels weak to me, as there might be more than one field missing, and the build method consumes the builder, which prevents the caller from inspecting it to find additional errors. Maybe the error should contain the actual builder object so that the caller can inspect it for errors?

[colin-kiegel]

Hm, returning the builder for inspection is an interesting idea!

However, I don't think we can easily report multiple errors at once. We would need to return a vector of errors, because it could be 1...n. I believe it's better to pick and report only one error.

The error_chain crate defines a macro by example. It is therefore able to export other items too (in contrast to proc-macro crates like derive_builder). I haven't looked at error_chain in detail, but at least I can say that less restrictions apply, so we may not be able to copy everything. But I'll take a closer look, thx. :-)

[colin-kiegel]

@TedDriggs points out in #67 that there is a need to

... surface structured errors in a manner that fits with "my" crate's error-handling strategy.

I think that's an important observation.

If we introduce structured errors, what will be our story for custom structured errors? Do we require users to wrap a (private) build function or implement their own? Or is there some clever way to let users manipulate the errors we emit, including their type?

[TedDriggs]

FYI, I think I now know how to make a proc-macro crate that exports things: Check out from_variants for an example of it in action, courtesy of the serde team.

[colin-kiegel]

Ok, cool. I'd be interested in your experience, because my current attempt #94 got stuck due to some no_std trouble. I'll give you write permissions to this branch in case you want to contribute to this refactoring. :-)

[fluffysquirrels]

I'm working on an embedded crate at the moment (no_std and no allocator, so no String). I may find some time to work on this enhancement. I'll post an update if I start work.

[TedDriggs]

I had an idea for this one: We could accept the path to an error type as an attribute and use that in the build function return type.

#[derive(Builder)]
#[builder(build_fn(error = "crate::Error"))]
pub struct MyStruct { my_field: String }

As long as there's an Into conversion from String to crate::Error, that'd work (or alternatively we could let the user specify both a type and a conversion function).

This way, derive_builder doesn't have to export anything, but the deriving type can have its build fn can use its own crate's error type and still have an autogenerated build method.