Make it possible to add #[builder] to existing code without changing the name of the function under the macro

[drmason13]

First of all, thank you for this fantastic crate, I love the idea of functions and structs sharing a builder derive, and the documentation is brilliant, the big effort that went into it all is clear to see.

I read through
https://elastio.github.io/bon/docs/guide/compatibility#adding-builder-to-existing-code

and followed the link to

https://elastio.github.io/bon/docs/reference/builder#expose-positional-fn

but I couldn't see how to make the preserved fn backwards compatible after adding the builder.

use bon::builder;

#[builder(expose_positional_fn = example_positional)] 
fn example(x: u32, y: u32) {}

// Positional function is now available under the given name
example_positional(1, 2);                                     

// Builder syntax is also available (unchanged)
example()
    .x(1)
    .y(2)
    .call();

Old code would do example(1, 2) and expect the function result, not a builder type. It is a small breaking change, as code could be changed to use example_positional(1, 2) instead easily with find/replace.

Would it be possible to change the name of the builder and keep the original function unchanged? E.g. for this to work:

use bon::builder;

#[builder(builder_fn = example_builder, expose_positional_fn)] 
fn example(x: u32, y: u32) {}

// Positional function is now available under the *original* name
example(1, 2);                                     

// Builder syntax is also available with the *explicit* name
example_builder()
    .x(1)
    .y(2)
    .call();

I'm not sure I want this as a feature request, but I think it deserves calling out specifically that expose_positional_fn is useful for "easing the breakage" but not able to provide backwards compatibility - if indeed that is the case.

I would add that statement to the compatibility section.

[Veetaha]

Would it be possible to change the name of the builder and keep the original function unchanged? E.g. for this to work:

The main idea is that the function's name under the #[builder] macro becomes the name of the function that returns the builder. In this case the code that you'd like to have should be this:

use bon::builder;

#[builder(expose_positional_fn = example)] 
fn example_builder(x: u32, y: u32) {}

example(1, 2);                                     

example_builder()
    .x(1)
    .y(2)
    .call();

The positional function becomes hidden by default and the current name of the function is the name of the function that returns a builder. I can add this as an example to that section to avoid confusion, thanks!

[Veetaha]

On the other hand.. It's understandable that you'd expect expose_positional_fn to flip this behavior where the positional function name is preserved and you need to specify a function for the builder.

I can also imagine it as a feature where providing a builder_fn (or in bon's terminology it's start_fn) flips the naming behavior of function's name such that it stays as positional function's name.

[Veetaha]

I updated the docs with the example above. I'll keep this issue open as a feature request to implement the syntax in your proposal where the positional function's name stays the same and the start_fn (that returns the builder) is explicitly specified in the attribute.

[drmason13]

Oh, I see, thank you for the example, that makes it clear to me how it is intended to be used.

It seems obvious in retrospect, but thank you for clarifying! I guess the new syntax will be more intuitive, but we can make do without it until (if) there's a new major version.

[Veetaha]

I don't think a major version will be required for this feature. The default behavior should still be such that the function's name is used for the builder function. However when setting an explicit start_fn the flipped behavior is used. This extends the existing API and doesn't break it

[drmason13]

use bon::builder;

// The previous name of the positional function needs to be specified
// as the value for `expose_positional_fn`.
#[builder(start_fn = example_builder, expose_positional_fn = example)] 
fn example(x: u32, y: u32) {}      

// The positional function is available
example(1, 2);

// The builder syntax is available with the given name
example_builder()
    .x(1)
    .y(2)
    .call();

Does the above compile today?

That's the most awkward example I can think of that might be broken by the proposed change.

I think that should still compile, but the = example becomes unnecessary with the change:

// The previous name of the positional function needs *is inferred*
// as the value for `expose_positional_fn`.
#[builder(start_fn = example_builder, expose_positional_fn)]
fn example(x: u32, y: u32) {}

[Veetaha]

#[builder(start_fn = example_builder, expose_positional_fn = example)] 

Such syntax should be a compile error, because now there are 3 names involved, when only 2 are needed.

Does the above compile today?

It doesn't because start_fn isn't available on functions today. It's exposed only for #[builder] on structs. The reference for this attribute specifies that it "applies to structs" only.

[Veetaha]

Hey @drmason13 I've just found a probably unrelated bug, but I want to make sure the fix won't affect your code.

The bug is that when you place #[builder] on a method called new(), then that method is exposed under the name new() by default. Also, the type name of the builder is slightly different than expected {Type}NewBuilder instead of {Type}Builder, which breaks the promise of compatibility with #[builder] placed on a struct.

Instead the positional method new() is supposed to be renamed to __orig_new() and #[doc(hidden)] should be added to it by default.

I prepared a fix for that #19, that will be released as a new patch

[drmason13]

I haven't used this crate properly yet so there's no risk of breakage for me, but I appreciate the concern :)

[Veetaha]

I think the syntax here should be the following.

The expose_positional_fn attribute should be removed (I never liked it to be honest 😳).
Instead, the following should be possible:

• By default the positional function should be hidden, and start_fn takes its place.
• If you set a start_fn, then it becomes the name of the function for builder syntax, while original positional function is left as is untouched
• The case of the method new is special. It works similar - by default the method builder takes its place and new is hidden. If you set start_fn = builder explicitly, only then the method new will be left untouched. It'll also be specially handled to make the builder named TBuilder instead of TBuilderBuilder (as it would be called for other methods (T{Method}Builder). This is to make this syntax compatible with deriving the builder from a struct.

I can include this change in the 3.0 release of bon, which is going to have some other minor breaking changes as well (e.g. #145)

[Veetaha]

Hi, sorry, it takes a lot of time to get this resolved just because I'm thinking too much about it and questioning the design again and again. Howerver, this issue is my current priority for bon before 3.0. I'd be glad to hear your opinion on the following.

The case of the method new is a bit special in that it's already the case that the start_fn is technically generated under a separate name (builder). Unfortunately, this special case of the method new doesn't fit into the overall behavior that would otherwise be intuitive for any other method named differently. The behavior is that "if you specify a start_fn with a given name, then the starting function will be generated separately under that given name, and the original function will stay available as is". I love this behavior because it makes the start_fn attribute purely additive for functions.

For example, with the current expose_positional_fn attribute it's not easy to put bon under a cargo feature. For example, if a crate depends on bon conditionally, and if "bon" cargo feature is enabled, then it would like to generate builders for existing functions additionally to them (without hiding them). It's not easy to do that because you can't just write smth like #[cfg_attr(feature = "bon", builder(expose_positional_fn = pos_fn_name)] because the name of the function under the attribute also needs to change (since that name will be used as the name of the starting function).

With the #[builder(start_fn = start_fn_name)] approach, this attribute can be easily placed under a cfg_attr and work additively. This is why I love this design.

However, this design doesn't look intuitive with the method named new because "there is already a start_fn under a different name builder", and I don't see a better design that does something about it without introducing any additional attributes and special behavior.

At this point, I think I'm just prepared to the fact that #[builder(start_fn = builder)] will not work intuitively when placed on the method named new. I'm also thinking of providing a shortcut #[builder(start_fn)] for the method named new just to shorten this notation. And you can see how confusing this looks either way... I believe we can live with that because this feature is used very rarely.

Currently, there are around ~100 open source repos that use bon directly and there is zero usages of expose_positional_fn in them. I suppose some private repos may use this feature for legacy reasons, but still rarely. So I can accept this degree of confusion when reading #[builder(start_fn)] annotation in this case.

The documentation of bon will describe this behavior as "if you specify #[builder(start_fn)] on a method or function, it means the starting function no longer replaces the original function keeping it available as is and instead generates the starting function in addition to it under the given name, the name of the starting function can be omitted in case of the method named new, in which case the default name of builder will be chosen".

Will I regret this in the future? I don't know. But if I or someone else discovers a better design for this at some point in the future I'll for sure implement that and deprecate the existing one.

If you have any feedback I'd be glad to hear it!

[drmason13]

I'm struggling to grasp all of this, I will need some time to tinker and get a feeling for how it works.

I'll try and make some before and after comparisons when various attributes are added and then let you know my thoughts.

[Veetaha]

Sure, I'll ping you once I have a 3.0.0-rc version

[Veetaha]

In the meantime, @drmason13 , maybe you could share what specific use case you have for exposing the positional function? Is this required for only backwards compatibility, or do you have other motivations?

[drmason13]

I might not have made this clear, but my question is mostly hypothetical. If someone needed backwards compatibility, how would they go about it.

[Veetaha]

I see, thanks for clarifying