bon

bon copied to clipboard

Let me give an example and I will describe the ask as an inline comment.

use bon::builder;

#[builder]
#[derive(Debug)]
struct TestStruct {
    // The following expression will generate a compiler error today. But can the builder:
   // - skip the generation of x.
   // - instead generate it based on the provided value of y. This should be possible since
   // the y-value should already the specified before one can call build()
    #[builder(skip = TestStructBuilder::y() + 23)]
    x: i32,
    y: i32,
}

Then call it as following:

assert_eq!(25, TestStruct::builder().y(2).build());

Hi! Thank you for creating the issue. It's definitely possible to update bon's logic to expose the values of other members to the expression used in skip, and I'd be fine with having this feature. Note that a similar thing may also apply to #[builder(default = expression)].

For example, the generated code can define local variables for every member, and may look like this:

fn build(self) -> TestStruct {
    // This is the value of `y` set by the setters
    let y = self.__private_impl.y.0;
    
    // Now the `skip_expression` has access to `y`. It's wrapped in an 
    // immediately-invoked closure to prevent the `skip_expression` from
    // using the `return` keyword and returning from `build` instead.
    let x = (|| skip_expression)();
    
    TestStruct { y, x }
}

However, I think the order of struct fields will have to matter then. For example, if the skip expression for x needs to use the field y, the field needs to be declared after the y field. The order of fields will define the order of initialization and thus the variables accessible by the skip expression. The same concept can be used for default expressions which may want to access the values of other members as well.

What's available in the meantime

It's possible to generate a builder that has this behaviour using a bit more verbose syntax today using the stable version of bon released at the time of this writing (1.2.1). Just like with any advanced use case that bon doesn't support directly for #[builder] placed on a struct I usually recommend moving the #[builder] from the struct to a method named new on a struct where you may implement this behavior like this:

use bon::bon;

#[derive(Debug)]
struct TestStruct {
    x: i32,
    y: i32,
}

#[bon]
impl TestStruct {
    #[builder]
    fn new(y: i32) -> Self {
        Self { x: y + 23, y }
    }
}

// Works as expected
assert_eq!(25, TestStruct::builder().y(2).build().x);

Did you consider using this approach? Or maybe you are already using this approach in the meantime?

Thanks a lot @Veetaha for considering this feature.

However, I think the order of struct fields will have to matter the ..

The order of fields only matters if there is a transitive dependency. If x depends on y and y depends on z. Then y must be created before x. But if we constrain (compile time) that y must be set by the user, then it should be fine.

->->->->->->->->->->->->->->->->->->->->->->->->->->->-

What's available in the meantime

...
#[builder]
    fn new(y: i32) -> Self {
        Self { x: y + 23, y }
    }
...

I am assuming that new() must be defined such that it takes all the arguments that the builder should provide setter for. In my case, the struct I want to build has awfully long list of fields that the user needs to set. The new will be really non-idiomatic in that case.

I you are okay with me sending a PR, I can do that. You can just guide me through. But I understand if you are not accepting PRs at this point.

But if we constrain (compile time) that y must be set by the user, then it should be fine.

Yeah, makes sense. I was just thinking to keep things simple, but we can make it more flexible. We can have such a rule that all variables without default = ... or skip = ... are initialized first, and only then go all variables with default = ... and skip = ..., whose relative order must be preserved.

I am assuming that new() must be defined such that it takes all the arguments that the builder should provide setter for.

Yeah, that's true. I see why you'd like this feature then.

I you are okay with me sending a PR, I can do that.

I'm generally open to any PRs. However, at this specific time, I'm preparing a breaking change around disabling the automatic into conversions (as described in https://github.com/elastio/bon/issues/15#issuecomment-2297202745) and a new 2.0 release. It's close to being done. Most of the docs were updated, but some more pages are left (https://github.com/elastio/bon/pull/54), I think it'll be in master by the end of the week.

That PR changes the code for finish_fn which is part of the focus of this issue. So I'd like to avoid merge conflicts. I'll finish #54 and I'll probably just work on this issue from my side to deliver it quicker as part of the same 2.0 release.

---

Some more thoughts out-loud on this feature:

• How should this interact with #[builder(name = renamed)] attribute? I think it should be ignored, and variables should be of the same name as struct field names or fn arg names.
• In functions, it's possible to use destructuring with complex patterns. In this case there is no obvious identifier to use for the variable in scope. I think that in this case we just don't create any variable and pass the member's value directly to the function or assign it to the struct's field. In general, using skip with function syntax shouldn't be required, it's simpler to just create a local variable in the function directly.
• What about the self member in a builder for associated methods that take self? I suppose it should also be in scope for the skip attribute. But.. it's rather complicated to implement this because self will be the builder struct itself in the context of the skip expression. I suppose we can just accept a limitation that self isn't available to any skip/default expressions. With function's syntax there is frankly a better way to express complex dependency on self by not using the skip/default attribute, but using Option<T> or creating the skipped state inside of the function directly.

Another thought.. Since I'm already doing a 2.0 I can just make another breaking change of prohibiting the usage of skip attribute in function arguments (I don't think anyone has actually used it already, since I don't see why anyone would prefer this attribute over just creating a local variable inside of a function). It is supported there today just because structs and functions use the same framework of attributes parsing and builder generation, but it's easy to make an exception by adding some preliminary validation for attributes on functions, where skip shouldn't be supported.

Thanks a lot for helping out with this feature request.

That PR changes the code for finish_fn which is part of the focus of this issue. So I'd like to avoid merge conflicts. I'll finish https://github.com/elastio/bon/pull/54 and I'll probably just work on this issue from my side to deliver it quicker as part of the same 2.0 release.

That 's totally understandable. I will wait for you to merge this feature.

How should this interact with #[builder(name = renamed)] attribute? I think it should be ignored, and variables should be of the same name as struct field names or fn arg names.

I agree with ignoring this. I think of re-naming a field to be a feature of a serialization library like serde or serde-dynamo where how one deals with the variables in code is different from how they are transferred or persisted. It is mainly because one cares about readability when writing code but cares about byte-size efficiency at other times. We should also care about this macro playing nice with other widely used macros like serde.

In general, using skip with function syntax shouldn't be required.

Makes sense to me.

What about the self member in a builder for associated methods that take self?

When writing a builder, most of the action happens in the fn build(). By that time, the Builder struct is already constructed with the right values and I think it will be good for the skip() to have access to the self of builder and be able to write the skip/default expressions on top of it.

2.0 I can just make another breaking change of prohibiting the usage of skip attribute in function arguments ..

Although we should try to minimize breaking changes, it should be okay to include some necessary ones as part of major version upgrade. We can just document it in the release notes of the version and provide a few examples of how to resolve the errors if one does run into them.

I implemented (https://github.com/elastio/bon/pull/58) a simpler version of this feature where every member is initialized in the order of declaration, and skip is now unsupported in function/method syntax.

This makes the API simpler and easier to understand. It will also make it easier for people to reason about the order of initialization, especially if something in their code depends on this order, like if the Default trait impls mutate some global state, which is a bad practice, but anyway should be accounted for.

I also, think it's not a big deal for people to reorder their members to achieve the desired order of initialization. I'd rather wait for a compelling use case where reordering of members is unacceptable. If someone creates an issue about it, we'll discuss it later. Anyway, it's possible to extend the behavior implemented in #58 without breaking changes if needed.

I'll release it as part of the 2.0 release.

Thanks a lot @Veetaha. I left a comment on your PR. You can give it a shot if you think it makes sense. I appreciate your attention to feature request with such a quick turn-around time. Now I will wait for the v2.0. Thanks again. It was a pleasure.