Add support for positional params in start_fn and finish_fn

[Veetaha]

Partially addresses #24.

It's now possible to add #[builder(start_fn/finish_fn)] to the members to mark them as positional arguments to the start_fn/finish_fn respectively.

The relative order of members with #[builder(start_fn/finish_fn)] matters. Such that all members with start_fn will go in the same order as they are declared relative to each other in the start function, and all members with finish_fn will go in the same order as they are declared relative to each other in the finishing method.

For anyone willing to test this feature right from the oven, use this in Cargo.toml:

bon = { git = "https://github.com/elastio/bon", branch = "feat/positional-only-members" }

Here are some tests that showcase the API:

bon/bon/tests/integration/builder/positional_members.rs

Lines 19 to 145 in be3c982

	#[test]
	fn smoke_struct() {
	#[derive(Debug, Builder)]
	#[allow(dead_code)]
	struct Sut {
	#[builder(start_fn)]
	starter_1: bool,
	#[builder(start_fn, into)]
	starter_2: char,
	#[builder(start_fn, into)]
	starter_3: Option<&'static str>,
	#[builder(finish_fn)]
	finisher_1: &'static str,
	#[builder(finish_fn, into)]
	finisher_2: &'static str,
	named: u32,
	}
	assert_debug_eq(
	Sut::builder(true, IntoChar('c'), None)
	.named(99)
	.build("1", IntoStrRef("2")),
	expect![[r#"
	Sut {
	starter_1: true,
	starter_2: 'c',
	starter_3: None,
	finisher_1: "1",
	finisher_2: "2",
	named: 99,
	}"#]],
	);
	let _ = Sut::builder(true, 'c', "str");
	}
	#[test]
	fn smoke_fn() {
	#[builder]
	fn sut(
	#[builder(start_fn)] starter_1: bool,
	#[builder(start_fn, into)] starter_2: char,
	#[builder(start_fn, into)] starter_3: Option<&'static str>,
	#[builder(finish_fn)] finisher_1: &'static str,
	#[builder(finish_fn, into)] finisher_2: &'static str,
	named: u32,
	) -> (
	bool,
	char,
	Option<&'static str>,
	u32,
	&'static str,
	&'static str,
	) {
	(
	starter_1, starter_2, starter_3, named, finisher_1, finisher_2,
	)
	}
	assert_debug_eq(
	sut(true, IntoChar('c'), None)
	.named(99)
	.call("1", IntoStrRef("2")),
	expect![[r#"(true, 'c', None, 99, "1", "2")"#]],
	);
	let _ = sut(true, 'c', "str");
	}
	#[test]
	fn smoke_impl_block() {
	struct Sut;
	#[bon]
	impl Sut {
	#[builder]
	fn sut(
	#[builder(start_fn)] starter_1: bool,
	#[builder(start_fn, into)] starter_2: char,
	#[builder(start_fn, into)] starter_3: Option<&'static str>,
	#[builder(finish_fn)] finisher_1: &'static str,
	#[builder(finish_fn, into)] finisher_2: &'static str,
	named: u32,
	) -> (
	bool,
	char,
	Option<&'static str>,
	u32,
	&'static str,
	&'static str,
	) {
	(
	starter_1, starter_2, starter_3, named, finisher_1, finisher_2,
	)
	}
	#[builder]
	fn with_self(
	&self,
	#[builder(start_fn)] starter_1: bool,
	#[builder(finish_fn)] finisher_1: &'static str,
	named: u32,
	) -> (bool, u32, &'static str) {
	let _ = self;
	(starter_1, named, finisher_1)
	}
	}
	assert_debug_eq(
	Sut::sut(true, IntoChar('c'), None)
	.named(99)
	.call("1", IntoStrRef("2")),
	expect![[r#"(true, 'c', None, 99, "1", "2")"#]],
	);
	let _ = Sut::sut(true, 'c', "str");
	assert_debug_eq(
	Sut.with_self(true).named(99).call("1"),
	expect![[r#"(true, 99, "1")"#]],
	);
	}

This is still a draft that I'm planning to finish by the end of the week. Remaining work:

• Add start fn args into builder's debug output and Clone derive
• Enforce the order: start_fn params, finish_fn params, and then named/skipped params in any order (this way the order of evaluation of skip/default expressions and the variables available in their scope is consistent with the order of declaration)
• Add more exhaustive tests, update existing ones, validate edge cases
• Update the API reference documentation on the website
• Add a new page to the Guide documentation on the website
• (Maybe post-merge) start a blog post for the next minor release
• Self-review and post-refactoring

[Veetaha]

cc @EdJoPaTo, @dzmitry-lahoda, and @ThomasAlban. I'd be glad to hear your feedback on this design, maybe you'd have any ideas for some better syntax to express this.

For example, I'm thinking that maybe there should be some short syntax like #[builder(pos)], that implies that the member should be a positional parameter of the start_fn. Ot maybe just do the syntax like this but without pos =:

#[builder(start_fn)]
#[builder(finish_fn)]

[ThomasAlban]

First of all, thank you very much for all your work on this crate!
I definitely prefer the

#[builder(start_fn)]
#[builder(finish_fn)]

over what is currently available. However, supplying fields as args to finish_fn is something I can't see myself ever using, I was more hoping for a single toggle at the top of the struct or fn which toggles every required field to be an arg to the start function. I was also hoping this would mean there would be no need for the finish_fn, further reducing the verbosity of the API.
I realise sacrificing the finish_fn also removes the ability to check you haven't called any optional functions multiple times, but I don't think this matters as logically it makes sense that the final call is the value that is used anyway.
Let me know if there are any reasons why supplying args to finish_fn would be useful as I can't see any!

[Veetaha]

I was more hoping for a single toggle at the top of the struct or fn which toggles every required field to be an arg to the start function.

Yeah, that's why this PR "partially addresses #24" (as written at the top of the PR description)

Let me know if there are any reasons why supplying args to finish_fn would be useful as I can't see any!

I do think it's useful. For example an API for sending messages from one person to another

#[derive(bon::Builder)]
#[builder(start_fn = from, finish_fn = to, on(String, into))]
struct Message {
    #[builder(pos = start_fn)]
    sender: String,

    body: String,
    is_urgent: bool,

    #[builder(pos = finish_fn)]
    recipient: String,
}

let message = Message::from("Blackjack")
    .body("Hello, Bon!")
    .is_urgent(true)
    .to("Bon");

This basically forces the developer into a specific order of method calls. In this case it requires you to specify the sender first first, and the recipient as the last field.

Another use case is the API similar to sqlx:

let account = sqlx::query!("select (1) as id, 'Herp Derpinson' as name")
    .map(|record| (record.id, record.name))
    .fetch_one(&mut conn)
    .await?;

Here you need to pass the DB connection object as an argument of the finishing method (i.e. where you'd like to fetch from).

[ThomasAlban]

Ah yes I hadn't considered how it allows you to force an order in your API. Yeah, makes perfect sense and I retract my previous statement!
Do you plan on adding something more like a single toggle to reduce API verbosity?

[Veetaha]

Do you plan on adding something more like a single toggle to reduce API verbosity?

Not yet, I don't know what that syntax would look like yet, and if it will be used that often to justify such additional syntax. I'll keep it simple initially

[Veetaha]

I definitely prefer the

#[builder(start_fn)]
#[builder(finish_fn)]

Now I think this notation isn't obvious enough. Maybe some of these

#[builder(via = start_fn)]
#[builder(via = finish_fn)]

#[builder(syntax = start_fn)]
#[builder(syntax = finish_fn)]

#[builder(source = start_fn)]
#[builder(source = finish_fn)]

#[builder(start_fn_arg)]
#[builder(finish_fn_arg)]

#[builder(positional)]
#[builder(positional(finish_fn))]

#[builder(starter)]
#[builder(finisher)]

#[builder(start_fn(arg))]
#[builder(finish_fn(arg))]

[Veetaha]

I don't really see a nice syntax for this. The one that sucks less I think is

#[builder(source = start_fn)]
#[builder(source = finish_fn)]

Which delivers the semantic meaning of "where the value for this member comes from".
ChatGPT doesn't really help

[ThomasAlban]

Could do 'src' instead of 'source' to make it more concise?
To be honest though, I think #[builder(start_fn)] is actually more clear than #[builder(src = start_fn)], because the first says to me 'this field is built in the start function', whereas the second introduces more stuff to read, also I prefer 'start_fn' being a sort of 'tag' that can be given to a field, rather than one of 2 possible things that source/src can be set to - this might lead the users of the api to believe 'source' can be set to more things other than start_fn/finish_fn.

[Veetaha]

Yeah, I just can't decide. Now that you put it that way, it seems logical. Glad I can put the blame of naming on someone else 🐱 . Be it #[builder(start_fn/finish_fn)]

[EdJoPaTo]

I think I prefer #[builder(on_start_fn)] to be a bit more clear what is meant from just reading some code of someone else without knowing bon details.

The key=value doesn’t seem to add much helpful context, might suggest more values and is longer so I think having only the keyword is a good choice.

I like thinking about it with multiple people to get different views and thoughts on it!

[Veetaha]

I think I prefer #[builder(on_start_fn)] to be a bit more clear what is meant from just reading some code of someone else without knowing bon details.

There is already builder(on(...)) attribute where "on" has a bit different meaning (similar to "if" or "when")

So if adding more clarity, I guess start_fn_arg works better.

[ThomasAlban]

start_fn_arg is also fine, if you think the extra characters are worth it for readability. For me, start_fn is just as readable, and I think this feature is going to get used quite a lot so any saving on characters is a benefit!

[EdJoPaTo]

Why is saving characters a benefit? Autocompletion exists, looking these up and copying will also be done quite often. The output binary size or performance will not be different in any way. We do not need to save characters. The focus should be on readable code, especially when people read code without being familiar with bon.

Thinking about it again, #[builder(start_fn)] might actually be enough as the logic here is "related to builder" → "something with the start function on the builder" So I think its probably fine the way it is then.

[Veetaha]

Why is saving characters a benefit

Just pure aesthetics.

Autocompletion exists,

Unfortunately, not for proc macros. I experimented with some hacks for autocompletion, and they do work, but they don't work at member level (yet), unfortunately. I'll try to fight with that once there is nothing more in priority for bon (#71)

[Veetaha]

Anyway, all of us are biased because we already know how this feature works. In general, I don't think _arg suffix makes that big of a difference. I guess people will either way often need to look up "what the hell is a start_fn and finish_fn?". So I'd go with the shorter notation

[Veetaha]

Done this change was released in a 2.3 version of bon:

• Reddit post
• Blog post