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

Enum-based Error for Uninitialized Fields? #180

Closed
EndilWayfare opened this issue Nov 30, 2020 · 8 comments
Closed

Enum-based Error for Uninitialized Fields? #180

EndilWayfare opened this issue Nov 30, 2020 · 8 comments

Comments

@EndilWayfare
Copy link

First off, love the project. It's awesome to have standard derives for common Rust patterns. I was definitely going to roll my own ad-hoc, informally specified macro if this didn't exist. Good thing, because I'm new-ish to Rust macros, only having used macro_rules (which would be an actual disaster for this use case, macro_attr or no).

Emitting string errors is fine for logging purposes or general "hey, user, there's a problem" notifications. But, I often find myself wanting to programmatically act on errors (e.g. highlighting missing required fields in a yew app, generating custom verbiage for user-facing messages, etc.). #169 is a fantastic step in that direction (yay std::error::Error), and I can't wait until that's released.

But, what if there were an option for an error with generated ErrorKind enum? The macro already knows about the field names to create the Builder, so it should be possible to generate an enum of fields. This seems simultaneously easy and non-trivial to me, with limited knowledge of proc_macro performance and capability. If this would be an acceptable feature, I'd love to try and contribute.

For now, I'll keep writing custom build functions. It's a small drawback given everything else being automagically generated.

@TedDriggs
Copy link
Collaborator

#169 includes a structured error for uninitialized fields with the FooBuilderError::UninitializedField(&'static str) variant. That will contain the field name, which can then be matched against in your code.

Can you share some example code which shows 1) how this builder is being assembled such that different runtime paths lead to different fields not being initialized, and 2) how you'd be using the structured error to better present errors back to the user?

@andy128k
Copy link
Contributor

andy128k commented Jan 14, 2021

As soon as there is a standard way to convert a String to Box<dyn std::error::Error>, we can change ValidationError's definition from ValdationError(String) to ValdationError(Box<dyn Error>). This will allow users of the crate to have structured custom errors and possibility to downcast them.

@EndilWayfare
Copy link
Author

let foo = foo_builder.build();
if let Err(err) = foo {
   match err {
        FooBuilderError::UninitializedField(field) => match field {
            FooField::Bar => { /* do something `bar` related */ }
            FooField::Baz => { /* do something `baz` related */ }
        }
        // ...
    } 
}

As opposed to

let foo = foo_builder.build();
if let Err(err) = foo {
   match err {
        FooBuilderError::UninitializedField(field) => {
            // Custom string processing function? Tied to `rust-derive-builder` internal impl of `Display` for `FooBuilderError`?
            match get_field_name_from_error_string(field) {
                "bar" => { /* do something `bar` related */ }
                "baz" => { /* do something `baz` related */ }
                // No compiler help to ensure exhaustive matching
                _ => { /* Have to cover the default case that should never really happen */ }
            }
        }
        // ...
    } 
}

In my understanding, "structured error" is more about "error is data that lends itself to programmatic operation" rather than "error impls std::error::Error". No matter how it is wrapped, if ultimately the error handling API is stringly typed, that's suboptimal (slower, less precise, more dev mental overhead) in my opinion.

@TedDriggs
Copy link
Collaborator

TedDriggs commented Jan 15, 2021

As a general principle, stringly-typed APIs are less idiomatic in Rust than stronger types. However, structure also increases the risk of API breakage requiring version bumps.

derive_builder is not a good substitute for a user-input validation library, since it will return an error after the first uninitialized field, meaning exhaustive errors aren't possible. The benefits of the structured error in 0.10.0 are:

  • Ability to separate blank field errors from custom validation errors
  • Removing the need to allocate for uninitialized-field cases

We'd be very interested in seeing examples of code in published crates where builders are being used in a way that meaningful recovery from the uninitialized-field case is possible, so that we can incorporate that into the error discussion.

An example question that we'd need to answer for this approach: Which fields are in the enum? The ones that are required, or all the fields the builder wasn't told to skip?

@EndilWayfare
Copy link
Author

Yeah, I definitely appreciate that library design is complicated and full of tradeoffs. Maybe this is the kind of thing that would be best handled by a separate project that can version independently? Maybe multiple 3rd-party custom-build-function-derives are the best we can hope for, since it's impossible to be all things to all people.

Unfortunately, all my work in Rust at the moment is proprietary, so I don't have a lot in the way of examples. I'd like to start spinning off reusable chunks into public crates at some point, sooner rather than later, once

  • I have time
  • I can get buy-in
  • Patterns emerge clearly and stabilize

The "builder pattern" is so flexible and "rusty", it applies to a nearly unlimited variety of potential uses and variations. Perhaps the best future direction is figuring out how to make derives "pluggable" by design, so they work cleanly together and reduce the likelihood of interfering with each other?

@TedDriggs
Copy link
Collaborator

There may be a path forward here without reimplementing derive_builder, based on how the discussion in the #192 comments goes. That path would be:

  1. Create a new proc-macro which generates an enum of field names and emits an impl FromStr for that enum
  2. Leverage the proposed build_fn attribute to use a custom error type; MySaferError
  3. Have impl From<derive_builder::UnitializedFieldError> for MySaferError use that FromStr on the field name and panic if it doesn't match. Since the macro from step 1 keeps the enum in sync with the struct, this would be a programmer error so a panic is appropriate.

That makes the field-name-to-typesafe-error completely pluggable, sidestepping questions about naming convention of enum variants, which fields to include as variants, etc.

Using darling and ident_case, the proc-macro crate described in step 1 is very easy to implement; you can check out the field_names crate to see how that'd work.

@andy128k
Copy link
Contributor

I tried to wrap this around my head, but still struggle to come up with a scenario how an enumeration of fields can be useful.

  1. A builder is used to gather user's input. This crate doesn't fit well to such task because a builder fails on a first missing field (better explanation).
  2. A builder represents a state of automata (finite state machine). This problem has a better solution -- match builder struct itself before invocation of build method.

@TedDriggs
Copy link
Collaborator

Given that there is an outside-this-crate solution that still allows leveraging these builders and the misgivings of the maintainers, I'm closing this as "Won't Do."

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

3 participants