Skip to content

Adding support for build_from and build_from_clone builder methods. - #313

Closed
BernardIgiri wants to merge 0 commit into
elastio:masterfrom
BernardIgiri:master
Closed

Adding support for build_from and build_from_clone builder methods.#313
BernardIgiri wants to merge 0 commit into
elastio:masterfrom
BernardIgiri:master

Conversation

@BernardIgiri

Copy link
Copy Markdown

Add build_from and build_from_clone builder methods

Closes #310

This PR adds support for two new optional top-level builder attributes:

  • #[builder(build_from)] – Adds a .build_from(&T) method
  • #[builder(build_from_clone)] – Adds a .build_from_clone(T) method

These methods allow partially configured builders to fill in missing fields from an existing instance of the target type (T) before finalizing the build. This is useful when:

  • You want to override only a few fields but reuse most of the existing data.
  • You're working with types that aren't Default, making field reuse non-trivial.

Example

#[derive(Builder, Clone)]
#[builder(build_from, build_from_clone)]
struct User {
    name: String,
    age: u8,
}

let jon = User::builder().name("Jon".into()).age(25).build();
let alice = User::builder().name("Alice".into()).build_from(&jon);
assert_eq!(alice.name, "Alice");
assert_eq!(alice.age, 25);

Implementation notes

  • The new code lives in builder_gen/build_from.rs and is only emitted if the attributes are enabled.

  • Each field is handled according to its member kind:

    • Named fields fallback to from.field.clone() if not explicitly set.
    • FinishFn fields always defer to from.field.clone().
    • Skip fields use Default::default().
  • Integration tests verify both methods.

Next steps

  • Reference docs have been stubbed in src/__/ide.rs; we may want to flesh out the corresponding links on the site.

@Veetaha This implements the optional build_from and build_from_clone methods discussed in #310 — feedback welcome!

@Veetaha Veetaha left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a bunch of (quite a lot actually) edge cases that this needs to handle. Not saying we should handle them all right now, we can start with a good enough MVP and put it under the experimental-build-from flag for now and iterate further.

I'm still not feeling in love with the attribute names and syntax. I also considered nesting them under finish_fn like #[builder(finish_fn(build_from))] to reduce noise in the site docs, but this syntax doesn't look intuitive (seems like we are overriding the finish_fn instead of adding one more method. Btw. we should also update them (they are in the website directory in this repo). But IDK, I may change the naming if some great idea comes to mind a bit later

Could also be done via #[builder(derive(BuildFrom))] like it's a "derive" for a builder that adds one more method. Still not sure

pub(crate) derive: DerivesConfig,

#[darling(default)]
pub(crate) build_from: bool,

@Veetaha Veetaha Jul 3, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should use Option<Option<ItemSigConfig>> to make their names, visibility and docs configurable, plus inherit the visibility of the builder struct similar to how the regular finish_fn does that here:

vis: finish_fn.vis.unwrap_or_else(|| builder_type.vis.clone()),

(^ this is the place where a bunch of complex inter-field-dependent defaults are resolved)

Otherwise the docs would be incorrectly claiming that these attributes support vis, name, doc attributes.

I think this may require writing a custom with parser function that handles a syn::Meta::Path (in which case it returns None (i.e. Some(None))), and otherwise delegates to ItemSigConfig.

quote! {
let #ident: #ty = match self.__unsafe_private_named.#index {
Some(value) => value,
None => from.#ident.clone(),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is that build_from accepts T by value and does not call clone() at all (that's why this method's name is a bit shorter since it is less expensive and should be the preferred thing), but build_from_clone accepts &T and calls .clone() if needed (that's why it has "clone" in its name).

.ident
.clone();

Ok(quote_spanned! { span => #ident })

@Veetaha Veetaha Jul 3, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a known footgun of quote_spanned!. It doesn't overwrite the span of interpolated tokens such as #ident - only the bare tokens created as part of that quasi-quotation syntax. But (1), otherwise, I don't see a need to override the span here. The span is preserved from the input by default, and that's desired. But (2) see my comment below about the body

Comment on lines +108 to +110
#ctor_path {
#( #ctor_args, )*
}

@Veetaha Veetaha Jul 3, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunatelly, this approach won't work in case if the builder is generated with the #[builder] macro on top of a function. We have the body.generate() in finish_fn.rs here that generates the proper body:

let body = &self.finish_fn.body.generate(self);

It's only requirement is that members are assigned to variables of the same name in scope. Plus we need to inherit the potential async / unsafe / const modifiers and maybe handle the case of the Result from the original function

use crate::prelude::*;

#[derive(Builder, Clone)]
#[builder(build_from, build_from_clone)]

@Veetaha Veetaha Jul 3, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should support method/function syntax too, unless you'd like to keep it supported only on struct derives initially.

For that, we should have tests for methods/functions too. In fact, bon's tests are quite repetitive (unfortunately) - we test all 3 cases of the syntax - structs, methods and free functions. I recommend following the naming conventions and patterns like here. E.g. test functions are called test_{struct,method,function}. Different test suites are separated into different test modules (inline modules can be used if tests are small). You may notice that sometimes, not all three cases are covered (if as a white-box we know the code under test is agnostic to the kind of syntax - struct or function - doesn't matter to it).

@polarathene

Copy link
Copy Markdown

@BernardIgiri any reason for closing the PR? You opened it, received feedback almost a year ago, but chose not to continue? Did it seem like too much effort?

Or was this unintentional? It looks like you pushed the master branch to this PR branch, as the PR diff is now empty (which would close the PR automatically as a result), I'm assuming that was a mistake and you intended to rebase your PR to the latest master branch?

@BernardIgiri

Copy link
Copy Markdown
Author

@BernardIgiri any reason for closing the PR? You opened it, received feedback almost a year ago, but chose not to continue? Did it seem like too much effort?

Or was this unintentional? It looks like you pushed the master branch to this PR branch, as the PR diff is now empty (which would close the PR automatically as a result), I'm assuming that was a mistake and you intended to rebase your PR to the latest master branch?

Sorry, I was trying to rebase so I can update this PR. I made good progress last night but might need some help. I will reopen this PR this evening on EST time after work today.

@BernardIgiri BernardIgiri mentioned this pull request Jun 16, 2026
3 tasks
@BernardIgiri

Copy link
Copy Markdown
Author

Okay. I created a new PR since reopening this seemed harder to do. I will continue this work there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature Request: method to fill the remaining values from an instance of existing built object

3 participants