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

Custom error example #194

Merged
merged 9 commits into from Jan 19, 2021
Merged

Conversation

TedDriggs
Copy link
Collaborator

@andy128k this is an interesting example of using custom validation errors today. For #181, we'd need a way for this to work without the heap allocation of String, which is what's driving #191.

If the build() method cannot convert the validation error to the
generated builder error, this is a problem with the validation function.
The error span should point there, not at the macro call-site.

Due to some issue in `darling` or `syn`, this change is pointing at the
closing quotation instead of the whole path; that is being tracked as
TedDriggs/darling#120.
Structured errors need a safe way to act on uninitialized fields
that doesn't conflate with validation errors. This commit adds that by
providing a struct to represent an uninitialized field and generating
conversions from that into the auto-generated error type.
@andy128k
Copy link
Contributor

@TedDriggs I am afraid of the situation when user will provide different non-compatible parameters and will receive a cryptic error message.

#[derive(Builder)]
#[builder(error = CError, unknown_field = "CError::WTF")]
struct TheStruct {
   #[builder("self.a_default()?")]
   a: i32,
   #[builder("self.b_default()?")]
   b: i32,
}

impl TheStructBuilder {
    fn a_default() -> Result<i32, AError> { ... }
    fn b_default() -> Result<i32, BError> { ... }
}

enum CError {
    WTF(&'static str),
    A(AError),
    B(BError),
}

impl From<AError> for CError { ... } // missing this will cause a cryptic error "From<AError> is not implemented in CError. Error is inside a macro"
impl From<BError> for CError { ... }

This looks like an attempt to create a new language inside of attributes.
"self.a_default()?" are already "unstructured" AF (sorry for my language).

@TedDriggs
Copy link
Collaborator Author

For the record, I feel passing a block in default is an anti-pattern, and were it not for backwards compatibility I would have gone with the serde pattern of accepting a path in that position. We can improve the confusing errors situation there by making the emitted spans better, but in general it's not something I'd encourage people to use.

@andy128k
Copy link
Contributor

@TedDriggs This crate has version 0.9. Semver allows to break compatibility. Let's change default attribute. Users who use ? may be advised to move validation into a custom validator.

@andy128k
Copy link
Contributor

Side note: Requiring From<derive_builder::UninitializedFieldError> for custom error may contradict (or clash) with "infallible" builders #56 (which I would like this crate to implement in a future 😏 ).

This test checks that we don't require unnecessary impls
for custom errors when uninitialized fields are impossible.
@TedDriggs
Copy link
Collaborator Author

TedDriggs commented Jan 19, 2021

@andy128k the macro itself doesn't - indeed, it can't - require that the specified error type have the impl; it can only attempt to use the impl and fail if it's not present. Builders with default values technically don't require this impl as it stands today; I've added a run-pass test to make sure that stays the case.


Regarding complex defaults, these are previously discussed in #53 and #65. A cursory inspection of dependent crates shows usage (example) that would be broken by attempting to enforce a serde-style more limited syntax for default. As noted in the linked issue and PR, complexity in the default expressions will make errors harder to understand. I've filed #195 to make the spans better there, but as @colin-kiegel notes, the documentation does already discourage that usage.

@andy128k
Copy link
Contributor

Possible option is to add additional parameter.

#[builder(default)] // uses Default::default()
#[builder(default = "42")] // uses "42"
#[builder(default_method = method_ident)] // uses self.method_ident

Downside: more changes for those who migrate from 0.9.

@TedDriggs
Copy link
Collaborator Author

Feel free to create an issue for the default question; I don't think it blocks this PR, since the advanced scenario for that was pretty bad before this PR too.

At this point, I believe the following are true:

  1. This PR allows the validate function to return &'static str without it being mistaken for an uninitialized field.
  2. This PR allows the creation of errors that don't allocate, even with validate being used.
  3. When a conversion is missing for uninitialized field or for validate, the compile error produced is readable.
  4. This PR does not require any new crates to directly depend on derive_builder.

As long as these hold, I'm inclined to move forward with this approach instead of the approach in #192.

@andy128k
Copy link
Contributor

As soon as this PR removes From<&str> for #builder_error_ident, I am happy to close my PR in favor of this one.

@TedDriggs TedDriggs merged commit c72cd24 into colin-kiegel:master Jan 19, 2021
@TedDriggs TedDriggs deleted the custom-error-example branch January 19, 2021 18:07
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

Successfully merging this pull request may close these issues.

None yet

2 participants