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

[suggestion] include build method when all required fields are not available. #2

Closed
sahandevs opened this issue Aug 3, 2022 · 8 comments · Fixed by #7
Closed

[suggestion] include build method when all required fields are not available. #2

sahandevs opened this issue Aug 3, 2022 · 8 comments · Fixed by #7

Comments

@sahandevs
Copy link
Contributor

sahandevs commented Aug 3, 2022

In order for users to know which fields are missing we can do something like this:

struct Builder {
   req1: Option<usize>,
   req2: Option<usize>,
   opt1: usize
}

impl Builder<false, false> {
    fn build(&self, req1: usize, req2: usize) -> ...
}

impl Builder<false, true> {
    fn build(&self, req1: usize) -> ...
}

impl Builder<true, false> {
    fn build(&self, req2: usize) -> ...
}

impl Builder<true, true> {
   fn build(&self) -> ...
}

The only problem is that it generates a lot of code and it may not be ideal. But if you are OK with the design, I can send a PR if you want to.

@maminrayej
Copy link
Owner

Yes if I'm not mistaken, the number of generated impl blocks will be exponential in terms of required parameters.
This is actually the original problem that I discussed with you which resulted in the implementation you provided in your gist.

Suppose we have n required parameters. Each required parameter could be given a value or not. So each parameter has two states. We have n parameters. So all of the parameters together will have 2^n states. For each state we have to generate an impl block.

Another problem is the functions inside an impl block. In the snippet you provided, there is no way to generate the <false, true> state from the <false, false> state which is the initial state of the builder. So the user has to set the parameters all at once which isn't ideal since it's not a builder pattern anymore. The solution would be to add two other function to that impl block:

impl Builder<false, false> {
    fn build(&self, req1: usize, req2: usize) -> Builder<true, true> { ... }
    fn req1(&self, req1: usize) -> Builder<true, false> { ... }
    fn req2(&self, req2: usize) -> Builder<false, true> { ... }
}

You can imagine if we had more required parameters, we had to connect all the 2^n states by hand which will complicate the code needed to generate the builder. The current design only generates three impl blocks and number of generated functions is linear in terms of the required parameters.

I agree that if the user forgets to initialize a required parameter, the generated error by the compiler is cryptic and unhelpful. I think the problem you're trying to solve here is really a UI problem that I don't have a solution for unfortunately.

@sahandevs
Copy link
Contributor Author

Yes, that's its biggest issue. I've tried a few other tricks, this one worked pretty well but has its own issues:

trait IncompleteBuilder {
    /// SAFETY:
    /// missing fields
    unsafe fn build() -> ! { panic!("build() was called when the builder is incomplete") }
}

impl<const A: bool, const B: bool, const F: bool> IncompleteBuilder for ItemBuilder<A, B, F> {
    
}

which works fine (if we define it after impl ItemBuilder for ItemBuilderStruct<true, true, true) and make the build function unsafe to call. The reason I'm using a trait here is that we can't have two functions with the same names but with can work around that with the use of traits. Though the problem with this method is in order to use this build method, the trait must be within the scope. it works fine if we use the builder in the same module as it is defined but everywhere we have to do something like use mod::{ItemBuilder, IncompleteBuilder} (we can still make the interface better by defining a mod ItemBuilder and putting the ItemBuilder and IncompleteBuilder inside it and use it like use mod::ItemBuilder::* but this way is still awkward)

@maminrayej
Copy link
Owner

Doesn't this design kind of defeats the purpose of a compile-time correct builder?
We generate a state machine to detect when it's safe to call the builder at compile time. Having to rely on panicking on runtime decreases the usability of the builder since using it is not different than using a regular builder.

I may be completely missing the point of your proposed design. So please bear with me until we figure it out :)

@sahandevs
Copy link
Contributor Author

I think you've missed the unsafe keyword there :D

@maminrayej
Copy link
Owner

After testing it in the playground, now I get what you meant. It works but if I'm not mistaken, as I tested it in the playground, calling the unsafe build function still does not say to the caller which fields are missing.
I believe the problem is that the compile error happens after the macro expansion hence we don't have any control over the message shown to the user.

@sahandevs
Copy link
Contributor Author

Unfortunately, the compiler doesn't give us many tools to show custom error messages to the user. the closest thing I've found is the combination of the unsafe and #[deprecated(reason = "missing fields")]. maybe opening an issue/RFC for something like #[#uncallable(reason="")] in rust-RFC repo wouldn't be a bad idea.

@sahandevs
Copy link
Contributor Author

https://gist.github.com/sahandevs/00ac326829c83926e78fd91e3a37e883

I'll send a PR and put the above implementation behind a nightly only feature gate if you are ok with it.

@maminrayej
Copy link
Owner

Neat solution :)
I'll be more than happy with that.

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 a pull request may close this issue.

2 participants