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

bon 3.0 wishlist (far, maybe never accepted breaking changes) #85

Closed
Veetaha opened this issue Aug 31, 2024 · 19 comments
Closed

bon 3.0 wishlist (far, maybe never accepted breaking changes) #85

Veetaha opened this issue Aug 31, 2024 · 19 comments
Labels
design needed The feature requires more design effort

Comments

@Veetaha
Copy link
Contributor

Veetaha commented Aug 31, 2024

⚠️ There is no particular plan for a new major release right now! This issue exists just to record some thoughts on the things that we may potentially break in the far future, or we maybe we won't. They aren't as critical, or maybe even arguable, thus we don't have any immediate plans for a new major release yet.

Here are some items that we may consider doing as part of the bon 3.0 major release. These are specifically breaking changes that we can't afford to do as part of a minor/patch release.

  1. Reconsider making the generated Builder type private (unexposed) by default. E.g. the builder type should contain __ as a prefix and be #[doc(hidden)] to effectively make it an "anonymous type". The reason is to hide it from the rustdoc output by default because its type signature is rather complex and noisy, such that it's unlikely people may read its docs.
  2. Restrict the relative ordering of positional members, regular named members, and skipped members

Verdict

  1. Rejected. The rustdoc output is now much cleaner with the Flexible builder extension and type signature API #145. There are no noisy generics - only a single builder type state generic parameter
  2. Rejected. I don't think restricting more than what's already limited today makes sense (the order of start_fn -> finish_fn -> all other members). The order of members should only represent the order in which variables appear in scope of the skip/default expressions, and today's order is completely consistent with that.
@Veetaha Veetaha added the design needed The feature requires more design effort label Aug 31, 2024
@Veetaha Veetaha changed the title bon 3.0 wishlist (far future) bon 3.0 wishlist (far maybe unreachable future) Aug 31, 2024
@Veetaha Veetaha changed the title bon 3.0 wishlist (far maybe unreachable future) bon 3.0 wishlist (far, maybe unreachable future) Aug 31, 2024
@Veetaha Veetaha changed the title bon 3.0 wishlist (far, maybe unreachable future) bon 3.0 wishlist (far, maybe never accepted major breaking changes) Aug 31, 2024
@Veetaha Veetaha changed the title bon 3.0 wishlist (far, maybe never accepted major breaking changes) bon 3.0 wishlist (far, maybe never accepted breaking changes) Sep 1, 2024
@dzmitry-lahoda
Copy link
Contributor

Allow for groups and conflicts like in clap. Also saw something like that in compile time builder, cannot find it now, as I recall it needed some solver to be run at build time.

These features allow to state:

  • if this field was set, than some other field(s) must be set
  • if this field was set, than some other field(s) cannot be set

I have my manually crafted builders and do:

  • start from most stable input (most stable inputs are to the left in function parameters, allows for partial application to be effective), doubt it can be automated - hence forever manual builder. interestingly, bon helps partially with this basically adding partial application and named parameters somewhat into Rust (like it is in F#)
  • do most limiting input first. so if one group field sets most limits to reduce source code of generated types, they must be set first (not all methods available in consumer of builder at once). this can be optimization problem of groups/conflicts - find group constraints which limit type states most.
  • not sure if that possible, if field is elementary enum, allow named method to set field value for that. enum has variants Bar and Foo, and stored in field type so add method to set type_foo and type_bar. Useful with group feature.

@Veetaha
Copy link
Contributor Author

Veetaha commented Sep 8, 2024

Hi @dzmitry-lahoda, this issue is specifically about breaking changes. What you proposed is not a breaking change, therefore I've extracted it into a separate issue #110.

@cksac
Copy link

cksac commented Sep 11, 2024

The function builder required finish_fn, which may not look good in method chaining use case.
Not sure can we have option to generate only two variant, <fn>, <fn>_with without need to call finish_fn?

use bon::builder;

#[builder]
fn greet(name: &str, level: Option<u32>) -> String {
    let level = level.unwrap_or(0);

    format!("Hello {name}! Your level is {level}")
}

// required fields into positional arguments
let greeting = greet("Bon");
assert_eq!(greeting, "Hello Bon! Your level is 0");

// use <fn>_with for positional arguments and closure to set provide optional fields
let greeting = greet_with("Bon", |b| { b.level(24); });
assert_eq!(greeting, "Hello Bon! Your level is 24");


struct Tensor {}

#[bon]
impl Tensor {
    pub fn new() -> Self {
        Tensor{}
    }

    #[builder]
    fn op1(&self, x: &str, level: Option<&str>) -> Tensor {
        Tensor{}
    }

    #[builder]
    fn op2(&self, y: &str, level: Option<&str>) -> Tensor {
        Tensor{}
    }
}


let tensor = Tensor::new();
tensor.op1("x").op2("y");
tensor.op1_with("x", |b| b.level(2)).op2("y");

verse existing

let tensor = Tensor::new();
tensor.op1("x").call().op2("y").call();
tensor.op1("x").level(2).call().op2("y").call();

@Veetaha
Copy link
Contributor Author

Veetaha commented Sep 11, 2024

Hey @cksac the requirement of the finish_fn call can not be easily alleviated because this method is the one that converts TBuilder into T. Bon generates the builder that is designed with extensibility in mind. For example, if you add more optional fields to the T in the future, then how will the builder know if you intend to set the optional fields or it's already time to convert into the T object?

See an alternative feature that may solve/alleviate this problem. I have a draft PR for it #125 which you can test for yourself (the release target is somewhere by the end of this week). It allows you to pass arguments into the start_fn and finish_fn. But note that it's not fully finished yet, and the syntax may change a bit before merge.

I can't see a practical application in the example with the Tensor that you shown, so I'll show my example with the Point struct. I hope it'll help you to apply this idea to your use case with a Tensor or whatever else use case you have.

use bon::builder;

#[builder]
fn greet(#[builder(pos = start_fn)] name: &str, level: Option<u32>) -> String {
    let level = level.unwrap_or(0);

    format!("Hello {name}! Your level is {level}")
}

let greeting = greet("Bon").call();
let greeting = greet("Bon").level(24).call();


#[derive(bon::Builder)]
#[builder(start_fn = x, finish_fn = y)]
struct Point {
    #[builder(pos = start_fn)]
    x: u32,

    #[builder(pos = finish_fn)]
    y: u32,
}

let _ = Point::x(1).y(2);

@cksac let me know if this solves (or at least helps with) your problem

@Veetaha
Copy link
Contributor Author

Veetaha commented Sep 11, 2024

One more trick. If you want to avoid calling the finishing method, just make the finishing method accept one of the required fields (which, additionally forces the caller to pass the value for this member as the last one in the chain). Annotate all optional fields as builder(skip) (although it's not required if you'd also like to set them in the builder), and then add setters for optional fields on your resulting struct.

In this example, after the .arg2(2) call the Example object was already built with all optional fields set to None:

use bon::Builder;

#[derive(Builder)]
#[builder(finish_fn = arg2)]
struct Example {
    arg1: i32,

    #[builder(pos = finish_fn)]
    arg2: i32,

    #[builder(skip)]
    optional_1: Option<i32>,

    #[builder(skip)]
    optional_2: Option<i32>,
}

impl Example {
    fn optional_1(mut self, value: i32) -> Self {
        self.optional_1 = Some(value);
        self
    }

    fn optional_2(mut self, value: i32) -> Self {
        self.optional_2 = Some(value);
        self
    }
}

let _ = Example::builder().arg1(1).arg2(2).optional_1(99);

I'll probably need to document all these patterns in the "Patterns" section of the docs.

Note that al of the above requires #125 to be released.

@cksac
Copy link

cksac commented Sep 11, 2024

I think we can generate two function variants from original function which use the generate builder? like

use bon::builder;

#[builder]
fn greet(name: &str, level: Option<u32>) -> String {
    let level = level.unwrap_or(0);
    format!("Hello {name}! Your level is {level}")
}

// generated helper functions
fn _greet(name: &str) -> String {    
    greet().name(name).call()
}

fn _greet_with<F>(name: &str, set: F) -> String 
where F: FnOnce(GreetBuilder) -> GreetBuilder
{
    let builder = greet().name(name);
    set(builder).call()
}

@cksac
Copy link

cksac commented Sep 11, 2024

with custom finish_fn, it seems even harder to read?

tensor
    .op1("x") // TensorOp1Builder
    .level(24) // Tensor, finish_fn of op1
    .op2("y") // TensorOp2Builder
    .call() // Tensor, finish_fn of op2

compare to

    tensor
    .op1("x") // Tensor    
    .op2_with("y",  |b| b.level(2)) // Tensor

@Veetaha
Copy link
Contributor Author

Veetaha commented Sep 11, 2024

As for me, the closure syntax is harder to read and compose because now your code is nested inside of anther function. You can no longer easily use the partial builder pattern and you can't do an early return from the function.

See the comment idanarye/rust-typed-builder#66 (comment) from the author of typed-builder which explains why closure syntax is bad.

Could elaborate on your real use case where you'd like the syntax proposed here? I'd like to understand the logic behind the builder in your case to better understand the problem domain.

@cksac
Copy link

cksac commented Sep 11, 2024

image
I use a lot of method chaining in a ML lib, the purpose enhancement is for readability.
you can see below code, the builder type returned in between the method chain, I can't tell which call create a Tensor without IDE's help, right?

tensor
    .op1("x") // TensorOp1Builder
    .level(24) // Tensor, finish_fn of op1
    .op2("y") // TensorOp2Builder
    .call() // Tensor, finish_fn of op2

@Veetaha
Copy link
Contributor Author

Veetaha commented Sep 11, 2024

In the example shown here the existing API already looks nice. Every method returns a Tensor on every step of an operation. Every operation takes only a single parameter. So I don't see a need for using bon in this case. bon is useful if your method takes 3 or more parameters (or at least it can grow in the future to accept more parameters).

Basically, as I understand your Tensor API it's more of a "combinators" combination where you can also apply the same combinators several times (like there is max_pool2d applied several times. While bon constraints the caller to never call the same setter twice.

@cksac
Copy link

cksac commented Sep 11, 2024

The example I show did't apply bon yet, and some ops will take more than 1 args.
btw, I am propose the enhance for function builder only not struct.

@Veetaha
Copy link
Contributor Author

Veetaha commented Sep 11, 2024

If you'd mix a builder in the middle of these combinators layering (especially, if you remove the explicit finish_fn call), then it indeed becomes hard to distinguish where one combinator statrs and ends:

tensor
    .operation_1(some_arg, some_arg2) // -> Builder
    .optional_param_to_operation_1("foo") // -> Builder
    .call() // -> Tensor (now that we've set all optional params, call the next combinator) ...
    .operation_2(some_arg, some_other_arg) // -> Builder

The problem that the caller needs to set optional parameters and identify the "end" when no more optional parameters should be expected requires an explicit finish_fn call. I don't see a way around it.

The closure syntax looks unpretty, and I would like to avoid generating multiple variants of the same API and setters if possible to keep compile times lower and cognitive overhead lower as well (when there is just one way to do something, it makes your code structure more consistent)

@Veetaha
Copy link
Contributor Author

Veetaha commented Sep 11, 2024

Btw, in the example initial here (I modified it a bit):

impl Tensor {
    #[builder]
    fn op1(&self, x: &str, level: Option<&str>, op2: Option<&'static str>) -> Tensor {
        Tensor{}                             // ^^^ added a new parameter
    }

    #[builder]
    fn op2(&self, y: &str, level: Option<&str>) -> Tensor {
        Tensor{}
    }
}

let tensor = Tensor::new();
tensor.op1("x").op2("y");
tensor.op1_with("x", |b| b.level(2)).op2("y");

The tensor.op1("x").op2("y") can't possibly work because there is ambiguity: "are you trying to set the parameter named op2 in the op1 method, or are you already finished and want to call the op2 method on Tensor?"

Same thing with

tensor.op1_with("x", |b| b.level(2)).op2("y");

Is that op2 a parameter for op1 or a method on Tensor? The compiler has no way to know at which point it should convert the builder into a Tensor.

@cksac
Copy link

cksac commented Sep 11, 2024

here is a working example showing what I want to achieve. And comparison of two style.
for implementation, I think we can allow a config to generate one of fn builder to avoid breaking change.

use bon::bon;

struct Tensor {}

#[bon]
impl Tensor {
    pub fn new() -> Self {
        Tensor {}
    }

    #[builder]
    fn _sum(
        &self,
        dims: &[i64],
        #[builder(default)] keep_dim: bool,
        #[builder(default)] mean: bool,
    ) -> Tensor {
        Tensor {}
    }

    // GENERATED, all required parameters are positional
    fn sum(&self, dims: &[i64]) -> Tensor {
        self._sum().dims(dims).call()
    }

    // GENERATED, all required parameters are positional, with clouser to set optional parameters
    fn sum_with<F, __KeepDim, __Mean>(&self, dims: &[i64], set: F) -> Tensor
    where
        for<'__f1, '__f2> F: FnOnce(
            TensorSumBuilder<
                '__f1,
                '__f2,
                (
                    ::bon::private::Set<&'__f1 [i64]>,
                    ::bon::private::Unset<::bon::private::Optional>,
                    ::bon::private::Unset<::bon::private::Optional>,
                ),
            >,
        ) -> TensorSumBuilder<
            '__f1,
            '__f2,
            (::bon::private::Set<&'__f1 [i64]>, __KeepDim, __Mean),
        >,
        __KeepDim: bon::private::IntoSet<Option<bool>, TensorSumBuilder__keep_dim>,
        __Mean: ::bon::private::IntoSet<Option<bool>, TensorSumBuilder__mean>,
    {
        let b = self._sum().dims(dims);
        set(b).call()
    }

    #[builder]
    fn _mean(&self, dims: &[i64], #[builder(default)] keep_dim: bool) -> Tensor {
        Tensor {}
    }

    fn mean(&self, dims: &[i64]) -> Tensor {
        self._mean().dims(dims).call()
    }

    fn mean_with<F, __KeepDim>(&self, dims: &[i64], set: F) -> Tensor
    where
        for<'__f1, '__f2> F: FnOnce(
            TensorMeanBuilder<
                '__f1,
                '__f2,
                (
                    ::bon::private::Set<&'__f1 [i64]>,
                    ::bon::private::Unset<::bon::private::Optional>,
                ),
            >,
        ) -> TensorMeanBuilder<
            '__f1,
            '__f2,
            (::bon::private::Set<&'__f1 [i64]>, __KeepDim),
        >,
        __KeepDim: ::bon::private::IntoSet<Option<bool>, TensorMeanBuilder__keep_dim>,
    {
        let b = self._mean().dims(dims);
        set(b).call()
    }
}

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

    // exiting
    let output = t
        ._sum()
        .dims(&[1, 2])
        .keep_dim(true)
        .mean(false)
        .call()
        ._mean()
        .dims(&[1])
        .keep_dim(true)
        .call()
        ._sum()
        .dims(&[1])
        .call()
        ._mean()
        .dims(&[1])
        .call();

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

@Veetaha
Copy link
Contributor Author

Veetaha commented Sep 11, 2024

@cksac how about this? What if bon generates a From<TBuilder> for T for you, so you can then implement such API with the closure manually if you want. Right now such impl isn't generated, but it's possible to add, here is how it looks:

struct Tensor {}

// Define optional parameters in a struct
#[derive(bon::Builder)]
struct SumOptionalParams {
    #[builder(default)]
    keep_dim: bool,
    
    #[builder(default)]
    mean: bool,
}

// This impl will be generated by `bon::Builder` (maybe by default, or maybe opt-in via an attribute config)
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()
    }
}

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

    fn sum<T: Into<SumOptionalParams>>(
        &self,
        dims: &[i64],
        optional: impl FnOnce(SumOptionalParamsBuilder) -> T,
    ) -> Tensor {
        let params: SumOptionalParams = optional(SumOptionalParams::builder()).into();
        Tensor {}
    }
}

let t = Tensor::new()
    .sum(&[1, 2], |b| b.keep_dim(true).mean(false));

This code compiles with the current version of bon. The thing that's lacking is that From<TBuilder> for T impl.

Or maybe it's better to have a trait that could be used as a geneiric bound for the return type of the closure smth like

// or TotalBuilder, IDK
trait CompleteBuilder {
    type Output;
    
    fn build() -> Self::Output;
}

but I'm not sure about it yet. This will conflict with builders that specify additional parmeters in finish_fn

@cksac
Copy link

cksac commented Sep 11, 2024

@Veetaha , with From<TBuilder> for T, still a lot of work at user end, also the original function need to rewrite..
I am appreciate if bon can generate all from original function definition. That's why I post in this issue bon 3.0 wishlist :)

when generated this style, finish_fn with additional parameters should not allowed.

@Veetaha
Copy link
Contributor Author

Veetaha commented Sep 11, 2024

Yeah, it indeed requires more code to write, but I doubt there is a popular demand for such builder syntax, although I'm ready to be proven wrong. The gist is that if there is a From<TBuilder> for T generated, then the users can implement this desired syntax using stable bon features. Maybe another custom macro could be implemented on user side, which sucks, but also gives you a ton of flexibility to shape you builder API as you want using the primitives from bon.

@Veetaha
Copy link
Contributor Author

Veetaha commented Sep 11, 2024

I don't think there is anything 3.0 to this feature, because it doesn't require a breaking change, I created a separate issue #127 for this. I'm fine with having a From<TBuilder> for T syntax, but I'm currently not convinced that bon should generate the whole closure syntax because I don't have evidence of broad demand for this feature.

@Veetaha
Copy link
Contributor Author

Veetaha commented Oct 25, 2024

Closing this in favour of #156

@Veetaha Veetaha closed this as completed Oct 25, 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
Projects
None yet
Development

No branches or pull requests

3 participants