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

Explicitely create UninitializedField errors #192

Conversation

andy128k
Copy link
Contributor

No description provided.

@andy128k andy128k force-pushed the explicit-uninitialized-field-error branch from d686cda to 758b879 Compare January 14, 2021 00:46
@TedDriggs
Copy link
Collaborator

I think I'm understanding the mechanics of what's happening; can you provide a bit more context on the rationale for the shift here?

@andy128k
Copy link
Contributor Author

My initial implementation of error type had an assumption that UninitializedField error is created from a &str and ValidationError is created from a String (using std::convert::Into). I think it is not sound enough and it is possible to mix such errors (e. g. custom default function returning Result<T, &'static str> may trigger wrong type of error).

Additionally this PR relaxes requirements for ValidationError. No need to provide a String but anything implementing AsRef<str> (String, &str, Cow<'_, str>).

@TedDriggs
Copy link
Collaborator

I'm trying a different approach to this.

In derive_builder_core, export an UninitializedFieldError struct, then re-export that from derive_builder with #[doc(inline)].

The generated FooBuilderError will include impl From<UninitializedFieldError> for FooBuilderError.

We'd then accept #[builder(build_fn(error = "MyCustomError"))], which would signal the macro to use that type instead of generating a new error enum. Macro callers could use any type that met two simple impl requirements:

  1. impl From<UninitializedFieldError> for MyCustomError
  2. impl From<WHATEVER_MY_VALIDATE_FN_ERRORS_WITH> for MyCustomError

That makes it easier to avoid allocating in #181, and makes it easier for the macro and its caller to provide a custom error type: The macro gets to stick with using ? and simple conversions, while the caller doesn't need to match up names for their custom errors.

@andy128k
Copy link
Contributor Author

@TedDriggs Somehow I had an impression, runtime dependency is unwanted. Right now, derive_builder (including core and macro crates) is a compile-time only dependency for its users. Introducing any types will make it a runtime and link dependency.

So, some (let's say derive_builder_runtime) crate need to be created. And this crate can contain a generic BuilderError too (Right now all error types are similar).

@TedDriggs
Copy link
Collaborator

When thinking about derive_builder, I think about three crates:

  1. derive_builder itself
  2. Crate A: The crate that directly imports derive_builder
  3. Crate B: The crate that directly imports crate A

The goal is that crate B does not need to import derive_builder. Having the derived build function return a derive_builder::BuilderError would violate this goal because crate B would need to import that in order to match against the error or pass it from function to function. Having derive_builder export an UninitializedFieldError for crate A to consume doesn't violate this requirement because crate B still remains ignorant of the use of derive_builder.

We technically already have a runtime dependency, insofar as we use ::derive_builder::export::_ for all our references to the core libraries. This is why we have derive_builder_macro separate from derive_builder.

@andy128k
Copy link
Contributor Author

andy128k commented Jan 19, 2021

@TedDriggs And what about custom validator? Do you want to force user to provide a custom error type when validator is specified? If not, should the crate export error enum (like it is defined in master branch right now)?

But looks like a discussion shifted from original problem. This PR fixes the potential issue when custom error type implements From<&str> and may mistakenly misinterpret UninitializedFieldError as something else.

@TedDriggs
Copy link
Collaborator

You're right that the conversation is wandering a bit here; your initial From-based implementation was closer to what I landed on as a more robust solution to this problem. Let's move the discussion to #194, since that is demonstrating the alternate proposal for how to make this safe and extensible.

@andy128k andy128k mentioned this pull request Jan 19, 2021
@andy128k
Copy link
Contributor Author

Superseded by #194

@andy128k andy128k closed this Jan 19, 2021
@andy128k andy128k deleted the explicit-uninitialized-field-error branch January 19, 2021 17:49
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.

2 participants