feat: allow to access already set fields within pattern of partial shared builder

[dzmitry-lahoda]

use bon::builder;

#[builder(on(String, into))]
struct User {
    name: String,
    tags: Vec<String>,
    alias: Option<String>,
    description: Option<String>,
}

// Common part is here
let user = User::builder()
    .name("Bon")
    .tags(vec!["confectioner".to_owned()]);

// Differences are in each branch
let user = if user.GET_name().as_str() == "Bonny" {
    user
        .alias("Good girl")
        .description("Knows mathematics 🐱")
        .build()
} else {
    user
        .description("Skipped math classes 😿")
        .build()
};

real world usage

proptest allows to chain into hierarchy generated properties via prop_flat_map (with optimized shrinking retained).

so each prop_flat_map in chain depends on data to be generated from upper layer, uses that data to constraint its out put.

prop_flat_map(|prev| prev, prev+1 ).prop_flat_map(|(prev_0, prev_1|, ..., 0..prev_1).

so final test function gets do_test(prev_0, prev_1, prev_2).

so I thin of using builder on each stage and path only partial builder, instead of many parameters. but need access to already set fields. in the end I do call build and produce final data struct.

example without builder:

    runner
        .run(
            &(
                1..1_000_000u64,
                0..(7u8 + 1),
                0..(6u8 + 1),
                0..(6u8 + 1),
                Margins::valid_margins_strategy(),
            )
                .prop_flat_map(
                    |(
                        market_max_balance_in_units,
                        base_token_decimals,
                        quote_token_decimals,
                        market_price_decimals,
                        market_margins,
                    )| {
                        (
                            Just(market_max_balance_in_units),
                            Just(base_token_decimals),
                            Just(quote_token_decimals),
                            Just(market_price_decimals),
                            1u64..(market_max_balance_in_units + 1),
                            0..(base_token_decimals + 1),
                            1..(Decimals::from_repr(market_price_decimals)
                                .add_u8(2)
                                .scale_u64()
                                + 1), // price_mantissa
                            Just(market_margins),
                            // TODO: variate markets and token counts
                            // TODO: variate Weigth and Margins - make sure that balance of user is inverse to this
                            // TODO: variate many makers vs one taker (ensure full fill always)
                            // TODO: pepr market
                            // TODO: variate limit via size/price/quote (use inverse formuals to set limit correct)
                            // TODO: variate fill types as long as they lead to same results (full fills)
                            // TODO: extend to partial fill results (will lead to test running very long time, need to configure test runner) - so some amounts stay on balance
                        )
                    },
                ),
            |(
                market_max_balance_in_units,
                base_token_decimals,
                quote_token_decimals,
                market_price_decimals,
                base_traded_in_size,
                market_size_decimals,
                price_mantissa,
                market_margins,
            )| {
                //TODO: failed trades are fine, so somehow needed to count this
                let _ = assert_different_market_setups_and_order_sizes_filled_well(
                    base_token_decimals,
                    quote_token_decimals,
                    market_size_decimals,
                    market_price_decimals,
                    market_max_balance_in_units,
                    base_traded_in_size,
                    price_mantissa,
                    market_margins,
                );
                Ok(())
            },
        )
        .unwrap();

https://altsysrq.github.io/proptest-book/proptest/tutorial/higher-order.html

A note for the community from the maintainers

Please vote on this issue by adding a 👍 reaction to help the maintainers with prioritizing it. You may add a comment describing your real use case related to this issue for us to better understand the problem domain.

[dzmitry-lahoda]

I could have refactor to make bigger objects to consist of smaller, or write my own builders, or custom test inputs data objects.
Idea is that autogenerated builder may be useful for a while and prototype further actions, before doing all things manually(if at all ever).

[dzmitry-lahoda]

Tried to quickly get things done, so as I see now builder_impl generates single builder type.

So need some refactor to allow builder to generate many impls so each getter added iff value was set as witnessed by non generic parameter constrained specialezed.

Seems required changes are more broad than just for this single feature?

[Veetaha]

Let's discuss the design for this feature first.

First of all, I expect adding getters is not something that everyone needs. Having them generated additionally will increase the compilation time (we have crates where there are ~320 structs with builders, where compile time is important).

So getters should definitely be an opt-in feature. Then... How do we opt-in? There should be some attribute that enables getters for specific members. There are various different ways to generate getters:

• By readonly reference e.g. get_{member}(&self)
• By mutable reference e.g. get_{member}_mut(&mut self)
• By value e.g. into_{member}(self)

Then what about Copy types? There should probably be a way to return the value by Copy, there should be a config for this as well.

For Option types there should also be a way to return an Option<&T> instead of &Option<T>.

For Deref types there should be an attribute that configures returning &<T as Deref>::Target

Then there is probably a use case where one would like to deconstruct the builder into raw parts entirely. In this case, it would be cool if Rust had anonymous structs (rust-lang/rfcs#2584), but alas there aren't, so this would need to be a workaround. The macro should generate some projection struct that can hold any state of the members (set or unset).

In general, this looks like a big feature that will evolve substantially. If you'd like to have some subset of this feature quickly, I'm fine to have it but granted that it's added under an unstable cargo feature flag, and it's opt-in via attributes.

My first naive take at this would be replicating some of the API of getset and improving it over time under an unstable feature gate. The config should probalby be by-member. If one needs to generate setters for all members at once they can use #[builder(on(_, ...))] to configure it at the top-level

[dzmitry-lahoda]

Subset like this

Enabling only for specific fields sounds great, because people can pick exact places where common splits in shared partial builder happen (likely will compose nice with groups - so splits can be made around these points - as i do in manual builder anyway) with proper Option handling.

Readonly pub reference only. For mutation people use builder. Feature only for comparison and decision making, not for getting clone of value out. #[builder(get)].

Questions

Currently as I see I can set once, rls shows me I can set value second time, but compilation fails.
So is it possible to avoid showing me setter if I have already set?
Asking because for getter, I need to show getter iff I have already set.
What are options here?

What about life time? My assumption is that holding reference to property will not prevent setting other fields. Any changes envisioned here?

[Veetaha]

So is it possible to avoid showing me setter if I have already set?

I don't think so. I think it's more of an issue with rust-analyzer because it doesn't take into account trait bounds when showing completions. Maybe there is a way to work around that but I don't think it's worth the effort at least now. We have a readable compile error in case the field is set the second time. This is good enough.

Especially, I want to keep the current design where there is a single impl block for the builder. This is because it makes the rustdoc output much more cleaner and removes a lot of noise from the generated docs.

My assumption is that holding reference to property will not prevent setting other fields.

Unfortunately, Rust doesn't work that way. The getter will take &self as an input, and return &MemberType. It means while the &MemberType reference returned by the getter is alive no mutations to self can happen (especially consuming self).

[dzmitry-lahoda]

current design where there is a single impl block for the builder.

i need to dig into this design. never new it is possible to do without several impls.

It means while the &MemberType reference returned by the getter is alive no mutations to self can happen (especially consuming self).

so here unsafe needed.

for sure no references could be hold if we when build is called, that is fine.

also double set same field prevented.

so after field is set, it is set. fine to continue mutate rest of builder state and hold reference to old state, so just need to use unsafe internally within well known safe patter, same as array split into 2 reference (one is mut and other not, as long as slices do not overlap).

so need to check if this #97 desing works well with getters.

[dzmitry-lahoda]

So I guess next step could be send proof of concept of manually modified expand of bon builder doing Getter and #97 ?

[Veetaha]

so here unsafe needed.

That is currently a deal breaker for me. There is no unsafe code in bon at this point and I don't want to start adding it right now. Especially in the context of getters. I don't think there is actually any reason to use unsafe for getters.

Better if you could provide an example case that solves some particular problem where you'd think unsafe is fine.

[dzmitry-lahoda]

what about defaults?

simplest is not to have getter for default which was not set explicitly.

any other options?

[Veetaha]

Yes, uny unset member should have no getter generated for it. We have a guarantee that default values are evaluated lazily only when the final build() or call() method is invoked. This way users don't need to pay the cost of evaluating the default value when they just create the builder and especially if they intend set a non-default value for the field with a setter

[dzmitry-lahoda]

@Veetaha i am trying to thing up getter code to look like.

For example this simple builder expanded https://github.com/dzmitry-lahoda-forks/bon/blob/dz/2/bon-get/src/main.rs

As I see specialization of setter is done via return value.

Unfortunately I do not have return value in just field value to return.

Any ideas how to make it nice?

I tinkering next:

• custom trait per field, so it may be slow to compile?
• return not value, but wrapper value, from which can get underlying value, so in this case I will have return something to specialize
• implemented some existing trait, like AsRef for each variant of builder, so it is many impls

So is it possible at all to keep single impl or go right away with many?

[dzmitry-lahoda]

Mat be pub trait IsSet {} trait? Is goal to avoid many impls in expanded code?

[dzmitry-lahoda]

   --> bon-get/src/main.rs:187:16
    |
187 |     let c= aaa.get_c();
    |                ^^^^^ the trait `IsSet<Option<Vec<u8>>>` is not implemented for `Unset<Optional>`
    |

with

    #[allow(clippy::inline_always, clippy::impl_trait_in_params)]
    #[inline(always)]
    pub fn get_c(&self) -> Option<&Vec<u8>>
    where
        __C: ::bon::private::IsSet<Option<Vec<u8>>>,
    {
        self.__private_members.2.is_set().as_ref()
    }