Repository: elastio/bon (language: rust)

Issue:
`must_use` build()
While I like seeing the `struct FooBuilder<…>` having a `must_use` (#26) I'm not sure why the `build()` method does not have one. Some `Into` implementations could have side effects but other than that it should just create an object which hasn't been used so far. So its likely wrong when the `build()` output isn't used?


Discussion / hints:
I agree, `must_use` should be the on the `build()` method. It's an oversight that it isn't there.

If you want, you can make a PR with this change, or I can do it myself a bit later anyway. There change should go into the `FinishFunc` struct config of the `builder_gen` module (the shared module that is used to generate the builder for both struct and function inputs):
https://github.com/elastio/bon/blob/d49a3b56790768daafb4b0e869f0b5e2dd722d29/bon-macros/src/builder/builder_gen/mod.rs#L45-L52

We should configure it to enable `must_use` for struct inputs only. I believe `must_use` doesn't make sense for functions by default (only for structs). Additionally, maybe the code could also search for a `must_use` attribute on the function (if the input is an `fn`), and if it's present, then forward that attribute to the `FinishFunc`.
I'd be happy to add that as it would be a learning for me to tinker with proc macros. So I'd like to provide a PR later.

For now: adding `must_use` is considered a breaking change. For this case I would say its rather fixing a bug as it restores the intended behaviour. When there is no `must_use` it's likely a bug. So such a PR should not require a new major release in my opinion.
>  For this case I would say its rather fixing a bug as it restores the intended behaviour.

This is my thinking as well. I think this kind of breakage is the one people would like to see.

But.. I'd like to argue that adding `#[must_use]` is not a breaking change by itself. It's breaking if and *only if* you deny warnings on CI. The violation of `#[must_use]` is a warning, not a compile error.

According to the [cargo book](https://doc.rust-lang.org/cargo/reference/semver.html#new-lints) "[introducing new lints](https://doc.rust-lang.org/cargo/reference/semver.html#new-lints)" such as `#[deprecated]`, `#[must_use]`, or any other change to the crate API that doesn't physically produce a compile error can be regarded as a minor change.

Once this change lands in master a minor version of `bon` will be bumped.
Just noticed I remembered this wrongly, `cargo-semver-checks` also sees `must_use` as a minor change.

```plaintext
$ cargo semver-checks --list | rg must_use
enum_must_use_added                      minor An enum has been marked with #[must_use].
function_must_use_added                  minor A function has been marked with #[must_use].
inherent_method_must_use_added           minor An inherent method or associated fn has been marked #[must_use].
struct_must_use_added                    minor A struct has been marked with #[must_use].
trait_must_use_added                     minor A trait has been marked with #[must_use].
union_must_use_added                     minor A union has been marked with #[must_use].
```

Fix patch:
diff --git a/bon-macros/src/builder/builder_gen/input_func.rs b/bon-macros/src/builder/builder_gen/input_func.rs
index da75481c..566ce258 100644
--- a/bon-macros/src/builder/builder_gen/input_func.rs
+++ b/bon-macros/src/builder/builder_gen/input_func.rs
@@ -326,6 +326,7 @@ impl FuncInputCtx {
             ident: finish_func_ident,
             unsafety: self.norm_func.sig.unsafety,
             asyncness: self.norm_func.sig.asyncness,
+            must_use: get_must_use_attribute(&self.norm_func.attrs)?,
             body: Box::new(finish_func_body),
             output: self.norm_func.sig.output,
             docs: "Finishes building and performs the requested action.".to_owned(),
@@ -503,3 +504,17 @@ impl Visit<'_> for FindSelfReference {
         }
     }
 }
+
+fn get_must_use_attribute(attrs: &[syn::Attribute]) -> darling::Result<Option<syn::Attribute>> {
+    let mut iter = attrs
+        .iter()
+        .filter(|attr| attr.meta.path().is_ident("must_use"));
+    let result = iter.next();
+    if let Some(second) = iter.next() {
+        bail!(
+            second,
+            "Found multiple #[must_use], but bon only works with exactly one (or less)."
+        );
+    }
+    Ok(result.cloned())
+}
diff --git a/bon-macros/src/builder/builder_gen/input_struct.rs b/bon-macros/src/builder/builder_gen/input_struct.rs
index aabc6688..42e15a57 100644
--- a/bon-macros/src/builder/builder_gen/input_struct.rs
+++ b/bon-macros/src/builder/builder_gen/input_struct.rs
@@ -126,6 +126,9 @@ impl StructInputCtx {
             ident: finish_func_ident,
             unsafety: None,
             asyncness: None,
+            must_use: Some(syn::parse_quote! {
+                #[must_use = "Building a struct without using it is likely a bug"]
+            }),
             body: Box::new(finish_func_body),
             output: syn::parse_quote!(-> #struct_ty),
             docs: "Finishes building and returns the requested object.".to_owned(),
diff --git a/bon-macros/src/builder/builder_gen/mod.rs b/bon-macros/src/builder/builder_gen/mod.rs
index 0b0a6720..6104f388 100644
--- a/bon-macros/src/builder/builder_gen/mod.rs
+++ b/bon-macros/src/builder/builder_gen/mod.rs
@@ -46,6 +46,8 @@ pub(crate) struct FinishFunc {
     pub(crate) ident: syn::Ident,
     pub(crate) unsafety: Option<syn::Token![unsafe]>,
     pub(crate) asyncness: Option<syn::Token![async]>,
+    /// <https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-must_use-attribute>
+    pub(crate) must_use: Option<syn::Attribute>,
     pub(crate) body: Box<dyn FinishFuncBody>,
     pub(crate) output: syn::ReturnType,
     pub(crate) docs: String,
@@ -563,6 +565,7 @@ impl BuilderGenCtx {
         let body = &self.finish_func.body.generate(&member_exprs);
         let asyncness = &self.finish_func.asyncness;
         let unsafety = &self.finish_func.unsafety;
+        let must_use = &self.finish_func.must_use;
         let docs = &self.finish_func.docs;
         let vis = &self.vis;
         let builder_ident = &self.builder_ident;
@@ -605,6 +608,7 @@ impl BuilderGenCtx {
             {
                 #[doc = #docs]
                 #[inline(always)]
+                #must_use
                 #vis #asyncness #unsafety fn #finish_func_ident(self) #output {
                     #(#members_vars_decls)*
                     #body


Test patch: