Trigger a compiler warning if #[tracing::instrument] is placed after the #[builder] instead of before

bon-macros/src/builder/builder_gen/input_fn.rs → bon-macros/src/builder/builder_gen/input_fn/mod.rs

@@ -1,3 +1,5 @@
+mod validation;
+
 use super::models::{AssocMethodReceiverCtxParams, FinishFnParams};
 use super::top_level_config::TopLevelConfig;
 use super::{
@@ -10,7 +12,6 @@ use crate::util::prelude::*;
 use std::borrow::Cow;
 use std::rc::Rc;
 use syn::punctuated::Punctuated;
-use syn::visit::Visit;
 use syn::visit_mut::VisitMut;
 
 pub(crate) struct FnInputCtx<'a> {
@@ -41,7 +42,7 @@ pub(crate) struct ImplCtx {
 }
 
 impl<'a> FnInputCtx<'a> {
-    pub(crate) fn new(params: FnInputCtxParams<'a>) -> Self {
+    pub(crate) fn new(params: FnInputCtxParams<'a>) -> Result<Self> {
         let start_fn = params.config.start_fn.clone();
 
         let start_fn_ident = start_fn
@@ -111,14 +112,18 @@ impl<'a> FnInputCtx<'a> {
             Some(prefix)
         });
 
-        Self {
+        let ctx = Self {
             namespace: params.namespace,
             fn_item: params.fn_item,
             impl_ctx: params.impl_ctx,
             config: params.config,
             self_ty_prefix,
             start_fn,
-        }
+        };
+
+        ctx.validate()?;
+
+        Ok(ctx)
     }
 
     fn assoc_method_ctx(&self) -> Result<Option<AssocMethodCtxParams>> {
@@ -260,30 +265,6 @@ impl<'a> FnInputCtx<'a> {
     pub(crate) fn into_builder_gen_ctx(self) -> Result<BuilderGenCtx> {
         let assoc_method_ctx = self.assoc_method_ctx()?;
 
-        if self.impl_ctx.is_none() {
-            let explanation = "\
-                but #[bon] attribute is absent on top of the impl block; this \
-                additional #[bon] attribute on the impl block is required for \
-                the macro to see the type of `Self` and properly generate \
-                the builder struct definition adjacently to the impl block.";
-
-            if let Some(receiver) = &self.fn_item.orig.sig.receiver() {
-                bail!(
-                    &receiver.self_token,
-                    "function contains a `self` parameter {explanation}"
-                );
-            }
-
-            let mut ctx = FindSelfReference::default();
-            ctx.visit_item_fn(&self.fn_item.orig);
-            if let Some(self_span) = ctx.self_span {
-                bail!(
-                    &self_span,
-                    "function contains a `Self` type reference {explanation}"
-                );
-            }
-        }
-
         let members = self
             .fn_item
             .apply_ref(|fn_item| fn_item.sig.inputs.iter().filter_map(syn::FnArg::as_typed))
@@ -496,34 +477,6 @@ fn merge_generic_params(
         .collect()
 }
 
-#[derive(Default)]
-struct FindSelfReference {
-    self_span: Option<Span>,
-}
-
-impl Visit<'_> for FindSelfReference {
-    fn visit_item(&mut self, _: &syn::Item) {
-        // Don't recurse into nested items. We are interested in the reference
-        // to `Self` on the current item level
-    }
-
-    fn visit_path(&mut self, path: &syn::Path) {
-        if self.self_span.is_some() {
-            return;
-        }
-        syn::visit::visit_path(self, path);
-
-        let first_segment = match path.segments.first() {
-            Some(first_segment) => first_segment,
-            _ => return,
-        };
-
-        if first_segment.ident == "Self" {
-            self.self_span = Some(first_segment.ident.span());
-        }
-    }
-}
-
 fn get_must_use_attribute(attrs: &[syn::Attribute]) -> Result<Option<syn::Attribute>> {
     let mut iter = attrs
         .iter()

bon-macros/src/builder/builder_gen/input_fn/validation.rs

@@ -0,0 +1,89 @@
+use crate::util::prelude::*;
+use syn::spanned::Spanned;
+use syn::visit::Visit;
+
+impl super::FnInputCtx<'_> {
+    pub(super) fn validate(&self) -> Result {
+        if self.impl_ctx.is_none() {
+            let explanation = "\
+                but #[bon] attribute is absent on top of the impl block; this \
+                additional #[bon] attribute on the impl block is required for \
+                the macro to see the type of `Self` and properly generate \
+                the builder struct definition adjacently to the impl block.";
+
+            if let Some(receiver) = &self.fn_item.orig.sig.receiver() {
+                bail!(
+                    &receiver.self_token,
+                    "function contains a `self` parameter {explanation}"
+                );
+            }
+
+            let mut ctx = FindSelfReference::default();
+            ctx.visit_item_fn(&self.fn_item.orig);
+            if let Some(self_span) = ctx.self_span {
+                bail!(
+                    &self_span,
+                    "function contains a `Self` type reference {explanation}"
+                );
+            }
+        }
+
+        Ok(())
+    }
+
+    pub(crate) fn warnings(&self) -> TokenStream {
+        let mut warnings = TokenStream::new();
+
+        let bon = self
+            .config
+            .bon
+            .clone()
+            .unwrap_or_else(|| syn::parse_quote!(::bon));
+
+        let instrument = self
+            .fn_item
+            .orig
+            .attrs
+            .iter()
+            .filter_map(|attr| attr.path().segments.last())
+            .find(|segment| segment.ident == "instrument");
+
+        if let Some(instrument) = instrument {
+            let span = instrument.span();
+
+            warnings.extend(quote_spanned! {span=>
+                use #bon::__::warnings::tracing_instrument_attribute_after_builder as _;
+            });
+        }
+
+        warnings
+    }
+}
+
+#[derive(Default)]
+struct FindSelfReference {
+    self_span: Option<Span>,
+}
+
+impl Visit<'_> for FindSelfReference {
+    fn visit_item(&mut self, _: &syn::Item) {
+        // Don't recurse into nested items. We are interested in the reference
+        // to `Self` on the current item level
+    }
+
+    fn visit_path(&mut self, path: &syn::Path) {
+        if self.self_span.is_some() {
+            return;
+        }
+        syn::visit::visit_path(self, path);
+
+        let first_segment = match path.segments.first() {
+            Some(first_segment) => first_segment,
+            _ => return,
+        };
+
+        if first_segment.ident == "Self" {
+            self.self_span = Some(first_segment.ident.span());
+        }
+    }
+}

bon-macros/src/builder/item_fn.rs

@@ -24,9 +24,10 @@ pub(crate) fn generate(
         fn_item,
         impl_ctx: None,
         config,
-    });
+    })?;
 
     let adapted_fn = ctx.adapted_fn()?;
+    let warnings = ctx.warnings();
 
     let MacroOutput {
         start_fn,
@@ -41,6 +42,7 @@ pub(crate) fn generate(
         //
         // See this issue for details: https://github.com/rust-lang/rust-analyzer/issues/18438
         #adapted_fn
+        #warnings
 
         #start_fn
         #other_items

bon-macros/src/builder/item_impl.rs

@@ -132,9 +132,15 @@ pub(crate) fn generate(
                 fn_item,
                 impl_ctx: Some(impl_ctx.clone()),
                 config,
-            });
+            })?;
 
-            Result::<_>::Ok((ctx.adapted_fn()?, ctx.into_builder_gen_ctx()?.output()?))
+            let adapted_fn = ctx.adapted_fn()?;
+            let warnings = ctx.warnings();
+
+            let mut output = ctx.into_builder_gen_ctx()?.output()?;
+            output.other_items.extend(warnings);
+
+            Result::<_>::Ok((adapted_fn, output))
         })
         .collect::<Result<Vec<_>>>()?;
 

bon-macros/src/util/path.rs

@@ -1,7 +1,6 @@
 use crate::util::prelude::*;
 
 pub(crate) trait PathExt {
-    /// Check if the path starts with the given segment.
     fn starts_with_segment(&self, desired_segment: &str) -> bool;
 
     /// Returns an error if this path has some generic arguments.

bon/Cargo.toml

@@ -53,7 +53,8 @@ rustversion = "1"
 expect-test = "1.4.1"
 
 # Using a bit older version that supports our MSRV
-tokio = { version = "1.29.1", features = ["macros", "rt-multi-thread"] }
+tokio   = { version = "1.29.1", features = ["macros", "rt-multi-thread"] }
+tracing = "0.1"
 
 # Using a bit older version that supports our MSRV
 trybuild = "1.0.89"

bon/src/__/mod.rs

@@ -14,6 +14,8 @@ pub mod ide;
 
 pub mod better_errors;
 
+pub mod warnings;
+
 mod cfg_eval;
 
 // This reexport is a private implementation detail and should not be used

bon/src/__/warnings.rs

@@ -0,0 +1,9 @@
+#[doc(hidden)]
+#[deprecated(note = "\
+    #[tracing::instrument] attribute should be placed before the #[builder] attribute \
+    to make sure it uses the original function name as the span name;\n\n\
+    reason: when the #[tracing::instrument] attribute is placed after the #[builder] \
+    attribute it will run after the #[builder] attribute is expanded, at which point \
+    the original function will be renamed to __orig_{fn_name}, and this will make \
+    that extra __orig_ prefix appear in the span name, which is likely not what you want")]
+pub mod tracing_instrument_attribute_after_builder {}

bon/tests/integration/ui/compile_fail/warnings.rs

@@ -76,4 +76,38 @@ fn main() {
         builder.get_x2();
         builder.get_x3();
     }
+
+    // Test #[tracing::instrument] placement after #[builder]
+    {
+        use tracing::instrument;
+
+        #[builder]
+        #[instrument]
+        fn bare_instrument() {}
+
+        #[builder]
+        #[instrument(name = "name_override")]
+        fn instrument_with_args() {}
+
+        #[builder]
+        #[tracing::instrument]
+        fn prefixed_instrument() {}
+
+        struct Sut;
+
+        #[bon]
+        impl Sut {
+            #[builder]
+            #[instrument]
+            fn bare_instrument() {}
+
+            #[builder]
+            #[instrument(name = "name_override")]
+            fn instrument_with_args() {}
+
+            #[builder]
+            #[tracing::instrument]
+            fn prefixed_instrument() {}
+        }
+    }
 }

bon/tests/integration/ui/compile_fail/warnings.stderr

@@ -1,15 +1,65 @@
-error: unused `ExampleBuilder` that must be used
-  --> tests/integration/ui/compile_fail/warnings.rs:29:9
+error: use of deprecated module `bon::__::warnings::tracing_instrument_attribute_after_builder`: #[tracing::instrument] attribute should be placed before the #[builder] attribute to make sure it uses the original function name as the span name;
+
+       reason: when the #[tracing::instrument] attribute is placed after the #[builder] attribute it will run after the #[builder] attribute is expanded, at which point the original function will be renamed to __orig_{fn_name}, and this will make that extra __orig_ prefix appear in the span name, which is likely not what you want
+  --> tests/integration/ui/compile_fail/warnings.rs:85:11
    |
-29 |         Example::builder();
-   |         ^^^^^^^^^^^^^^^^^^
+85 |         #[instrument]
+   |           ^^^^^^^^^^
    |
-   = note: the builder does nothing until you call `build()` on it to finish building
 note: the lint level is defined here
   --> tests/integration/ui/compile_fail/warnings.rs:1:9
    |
 1  | #![deny(warnings)]
    |         ^^^^^^^^
+   = note: `#[deny(deprecated)]` implied by `#[deny(warnings)]`
+
+error: use of deprecated module `bon::__::warnings::tracing_instrument_attribute_after_builder`: #[tracing::instrument] attribute should be placed before the #[builder] attribute to make sure it uses the original function name as the span name;
+
+       reason: when the #[tracing::instrument] attribute is placed after the #[builder] attribute it will run after the #[builder] attribute is expanded, at which point the original function will be renamed to __orig_{fn_name}, and this will make that extra __orig_ prefix appear in the span name, which is likely not what you want
+  --> tests/integration/ui/compile_fail/warnings.rs:89:11
+   |
+89 |         #[instrument(name = "name_override")]
+   |           ^^^^^^^^^^
+
+error: use of deprecated module `bon::__::warnings::tracing_instrument_attribute_after_builder`: #[tracing::instrument] attribute should be placed before the #[builder] attribute to make sure it uses the original function name as the span name;
+
+       reason: when the #[tracing::instrument] attribute is placed after the #[builder] attribute it will run after the #[builder] attribute is expanded, at which point the original function will be renamed to __orig_{fn_name}, and this will make that extra __orig_ prefix appear in the span name, which is likely not what you want
+  --> tests/integration/ui/compile_fail/warnings.rs:93:20
+   |
+93 |         #[tracing::instrument]
+   |                    ^^^^^^^^^^
+
+error: use of deprecated module `bon::__::warnings::tracing_instrument_attribute_after_builder`: #[tracing::instrument] attribute should be placed before the #[builder] attribute to make sure it uses the original function name as the span name;
+
+       reason: when the #[tracing::instrument] attribute is placed after the #[builder] attribute it will run after the #[builder] attribute is expanded, at which point the original function will be renamed to __orig_{fn_name}, and this will make that extra __orig_ prefix appear in the span name, which is likely not what you want
+   --> tests/integration/ui/compile_fail/warnings.rs:101:15
+    |
+101 |             #[instrument]
+    |               ^^^^^^^^^^
+
+error: use of deprecated module `bon::__::warnings::tracing_instrument_attribute_after_builder`: #[tracing::instrument] attribute should be placed before the #[builder] attribute to make sure it uses the original function name as the span name;
+
+       reason: when the #[tracing::instrument] attribute is placed after the #[builder] attribute it will run after the #[builder] attribute is expanded, at which point the original function will be renamed to __orig_{fn_name}, and this will make that extra __orig_ prefix appear in the span name, which is likely not what you want
+   --> tests/integration/ui/compile_fail/warnings.rs:105:15
+    |
+105 |             #[instrument(name = "name_override")]
+    |               ^^^^^^^^^^
+
+error: use of deprecated module `bon::__::warnings::tracing_instrument_attribute_after_builder`: #[tracing::instrument] attribute should be placed before the #[builder] attribute to make sure it uses the original function name as the span name;
+
+       reason: when the #[tracing::instrument] attribute is placed after the #[builder] attribute it will run after the #[builder] attribute is expanded, at which point the original function will be renamed to __orig_{fn_name}, and this will make that extra __orig_ prefix appear in the span name, which is likely not what you want
+   --> tests/integration/ui/compile_fail/warnings.rs:109:24
+    |
+109 |             #[tracing::instrument]
+    |                        ^^^^^^^^^^
+
+error: unused `ExampleBuilder` that must be used
+  --> tests/integration/ui/compile_fail/warnings.rs:29:9
+   |
+29 |         Example::builder();
+   |         ^^^^^^^^^^^^^^^^^^
+   |
+   = note: the builder does nothing until you call `build()` on it to finish building
    = note: `#[deny(unused_must_use)]` implied by `#[deny(warnings)]`
 help: use `let _ = ...` to ignore the resulting value
    |