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

Introduce the syntax where builder is passed via a closure #127

Open
Veetaha opened this issue Sep 11, 2024 · 7 comments
Open

Introduce the syntax where builder is passed via a closure #127

Veetaha opened this issue Sep 11, 2024 · 7 comments
Labels
design needed The feature requires more design effort feature request A new feature is requested

Comments

@Veetaha
Copy link
Contributor

Veetaha commented Sep 11, 2024

This is based on a comment from @cksac in #85 (comment)
It should look something like this. An additional config should enable the generation of a separate syntax for the builder where all required parameters are passed as positional parameters at the start of the function and optional parameters configured with the builder passed as an input to the trailing closure argument.

    use bon::bon;

    struct Tensor {}

    #[bon]
    impl Tensor {
        #[builder(closure_fn = sum_with)]
        fn sum(
            &self,
            dims: &[i64],
            #[builder(default)] keep_dim: bool,
            #[builder(default)] mean: bool,
        ) -> Tensor {
            Tensor {}
        }

    }

    let t = Tensor{};

    let output = t
        .sum_with(&[1, 2], |b| b.keep_dim(true).mean(false))
        // Automatically sets all optional parameters to default values
        .sum(&[1]);

I think this API looks too exotic, and it's probably fine if it would require a bit more code to write for the user. For example, if we just generate a From<TBuilder> for T impl, this API can be written manually using existing bon primitives (see the comment #85 (comment)).

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.

@Veetaha Veetaha added design needed The feature requires more design effort feature request A new feature is requested labels Sep 11, 2024
@cksac
Copy link

cksac commented Sep 11, 2024

for compare with bon-generate all, if bon only generate From<TBuilder> for T

use bon::bon;

// TODO: GENERATED By bon
impl<__KeepDim, __Mean> From<SumOptionalParamsBuilder<(__KeepDim, __Mean)>> for SumOptionalParams
where
    __KeepDim: bon::private::IntoSet<Option<bool>, SumOptionalParamsBuilder__keep_dim>,
    __Mean: bon::private::IntoSet<Option<bool>, SumOptionalParamsBuilder__mean>,
{
    fn from(builder: SumOptionalParamsBuilder<(__KeepDim, __Mean)>) -> Self {
        builder.build()
    }
}

// User code
#[derive(bon::Builder)]
struct SumOptionalParams {
    #[builder(default)]
    keep_dim: bool,
    #[builder(default)]
    mean: bool,
}

struct Tensor {}

impl Tensor {
    pub fn new() -> Self {
        Tensor {}
    }

    // NOTE: orignal function need to be rewrite to accept options struct
    fn _sum(&self, dims: &[i64], options: SumOptionalParams) -> Tensor {
        Tensor {}
    }
 
    pub fn sum(&self, dims: &[i64]) -> Tensor {
        let options = SumOptionalParams::builder().build();
        self._sum(dims, options)
    }

    pub fn sum_with<T>(
        &self,
        dims: &[i64],
        options: impl FnOnce(SumOptionalParamsBuilder) -> T,
    ) -> Tensor
    where
        T: Into<SumOptionalParams>,
    {
        let options = options(SumOptionalParams::builder()).into();
        self._sum(dims, options)
    }
}

fn main() {
    let t = Tensor::new();
    t.sum(&[1]).sum_with(&[1, 2], |b| b.keep_dim(true));
    println!("test");
}

@cksac
Copy link

cksac commented Sep 11, 2024

another version without closure, which allow early return and accept partial builder

use bon::bon;

// TODO: GENERATED By bon
impl<__KeepDim, __Mean> From<SumOptionalParamsBuilder<(__KeepDim, __Mean)>> for SumOptionalParams
where
    __KeepDim: bon::private::IntoSet<Option<bool>, SumOptionalParamsBuilder__keep_dim>,
    __Mean: bon::private::IntoSet<Option<bool>, SumOptionalParamsBuilder__mean>,
{
    fn from(builder: SumOptionalParamsBuilder<(__KeepDim, __Mean)>) -> Self {
        builder.build()
    }
}

// User code
#[derive(bon::Builder)]
struct SumOptionalParams {
    #[builder(default)]
    keep_dim: bool,
    #[builder(default)]
    mean: bool,
}

struct Tensor {}

impl Tensor {
    pub fn new() -> Self {
        Tensor {}
    }

    // NOTE: orignal function need to be rewrite to accept options struct
    fn _sum(&self, dims: &[i64], options: SumOptionalParams) -> Tensor {
        Tensor {}
    }

    pub fn sum(&self, dims: &[i64]) -> Tensor {
        let options = SumOptionalParams::builder().build();
        self._sum(dims, options)
    }

    pub fn sum_with(&self, dims: &[i64], options: impl Into<SumOptionalParams>) -> Tensor {
        let options = options.into();
        self._sum(dims, options)
    }
}

fn main() {
    let t = Tensor::new();
    let default_options = SumOptionalParams::builder().mean(true);

    let output = t
        .sum(&[1])
        .sum_with(&[1, 2], SumOptionalParams::builder().keep_dim(true))
        .sum_with(&[1, 2], default_options);
}

@Veetaha
Copy link
Contributor Author

Veetaha commented Sep 11, 2024

Yeah, I also was thinking of suggesting this, but this requires importing optional parameters struct type name.

@cksac
Copy link

cksac commented Sep 11, 2024

another design is extract all params in function into param struct, like

use bon::bon;

// TODO: GENERATED By bon
impl<'a, __Dims, __KeepDim, __Mean> From<SumParamsBuilder<'a, (__Dims, __KeepDim, __Mean)>>
    for SumParams<'a>
where
    __Dims: bon::private::IntoSet<&'a [i64], SumParamsBuilder__dims>,
    __KeepDim: bon::private::IntoSet<Option<bool>, SumParamsBuilder__keep_dim>,
    __Mean: bon::private::IntoSet<Option<bool>, SumParamsBuilder__mean>,
{
    fn from(builder: SumParamsBuilder<'a, (__Dims, __KeepDim, __Mean)>) -> Self {
        builder.build()
    }
}

#[derive(bon::Builder)]
#[builder(start_fn = new)]
struct SumParams<'a> {
    dims: &'a [i64],
    #[builder(default)]
    keep_dim: bool,
    #[builder(default)]
    mean: bool,
}

struct Tensor {}

impl Tensor {
    pub fn new() -> Self {
        Tensor {}
    }

    pub fn sum<'b>(&self, params: impl Into<SumParams<'b>>) -> Tensor {
        let params = params.into();
        // todo logic
        Tensor {}
    }
}

// User can also implment into for SumParams
impl<'a, const N: usize> From<&'a [i64; N]> for SumParams<'a> {
    fn from(dims: &'a [i64; N]) -> Self {
        SumParams::new().dims(dims).build()
    }
}

fn main() {
    let t = Tensor::new();

    let output = t
        .sum(SumParams::new().dims(&[1, 2]))
        .sum(SumParams::new().dims(&[1, 2]).keep_dim(false))
        .sum(SumParams::new().dims(&[1, 2]).keep_dim(false).mean(false))
        .sum(&[1, 2]);
    //with https://github.com/elastio/bon/pull/125
    //.sum(SumParams::new(&[1, 2]).keep_dim(false).mean(false));
}

good: only 1 variant of original function
bad: more verbose to call, can use custom implement into<FParams> if only single required params.

not sure which style is better.

@dzmitry-lahoda
Copy link
Contributor

dzmitry-lahoda commented Sep 11, 2024

not sure if that was discussed, would be nice macro force all option parameters to be in the end in original function too(fail to compile if not). so macro will not reorder parameters as they written in text.

because if parameters will be reordered in macro, but not in original text, it may lead to subtle usage bugs.

@cksac
Copy link

cksac commented Sep 12, 2024

Recap of above pattern. Personally, I prefer the closure style 1) > 2)

// 1)
let output = t
    .sum(&[1]);
    .sum_with(&[1, 2], |b| b.keep_dim(true).mean(false));

// 2) 
let output = t
    .sum(|b| b.dims(&[1, 2]))
    .sum(|b| b.dims(&[1, 2]).keep_dim(false).mean(false));

// 3)
let output = t
    .sum(&[1])
    .sum_with(&[1, 2], TensorSum__OptParams::builder().keep_dim(true).mean(false));

// 4)
let output = t
    .sum(TensorSum__Params::new().dims(&[1, 2]))
    .sum(TensorSum__Params::new().dims(&[1, 2]).keep_dim(false).mean(false));
  • using closure

    • pros
      • hide the generated func param struct, don't need to be import
      • will not have name conflict with existing associated methods
    • cons
      • can't early return when set the params in clouser
      • can't use partial builder
  • capture only optional function params to param struct

    • pros
      • easy to use when use default for optional params
    • cons
      • two variants for each method
  • capture all function params to param struct

    • pros
      • only 1 variant for each method
      • consistent
    • cons
      • harder to know what params are required, need compile error to show

@Veetaha Veetaha changed the title Builder with closure syntax Introduce builder with closure syntax Nov 10, 2024
@Veetaha
Copy link
Contributor Author

Veetaha commented Nov 10, 2024

I've published a 3.0.0 release of bon that stabilizes and documents the builder's typestate.

I've added a trait IsComplete, that could be used instead of From<Builder> like this:

use bon::Builder;

#[derive(Builder)]
struct ExampleParams {
    x1: u32,
    x2: u32,
    x3: Option<u32>,
}

// Our goal is to have this API
example(|builder| builder.x1(1).x2(2));

// Below is how we could achieve this

// Import traits from the generated module
use example_params_builder::{IsComplete};

fn example<S: IsComplete>(f: impl FnOnce(ExampleParamsBuilder) -> ExampleParamsBuilder<S>) {
    let builder = f(ExampleParams::builder());
    let params = builder.build();
}

I think this is superior to From trait because it works with builders, that are async, unsafe, if their finishing function accepts additional positional parameters, or if it returns a generic type (in which case From would be impossible due to trait coherence rules).

cc @cksac

@Veetaha Veetaha changed the title Introduce builder with closure syntax Introduce the syntax where builder is passed via a closure Nov 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design needed The feature requires more design effort feature request A new feature is requested
Projects
None yet
Development

No branches or pull requests

3 participants