diff --git a/benchmarks/compilation/results.md b/benchmarks/compilation/results.md index f0429786..c9db6a18 100644 --- a/benchmarks/compilation/results.md +++ b/benchmarks/compilation/results.md @@ -1,10 +1,10 @@ | Command | Mean [s] | Min [s] | Max [s] | Relative | | :------------------------------------- | ------------: | ------: | ------: | -----------: | -| `structs_100_fields_10 bon` | 2.321 ± 0.023 | 2.282 | 2.351 | 21.40 ± 1.92 | -| `structs_100_fields_10 typed-builder` | 1.677 ± 0.023 | 1.649 | 1.726 | 15.47 ± 1.40 | -| `structs_100_fields_10 derive_builder` | 1.043 ± 0.023 | 1.013 | 1.084 | 9.62 ± 0.88 | -| `structs_100_fields_10 ` | 0.135 ± 0.015 | 0.114 | 0.162 | 1.24 ± 0.18 | -| `structs_10_fields_50 bon` | 2.098 ± 0.022 | 2.065 | 2.135 | 19.35 ± 1.74 | -| `structs_10_fields_50 typed-builder` | 1.996 ± 0.021 | 1.970 | 2.029 | 18.41 ± 1.65 | -| `structs_10_fields_50 derive_builder` | 0.434 ± 0.012 | 0.415 | 0.450 | 4.00 ± 0.37 | -| `structs_10_fields_50 ` | 0.108 ± 0.010 | 0.096 | 0.129 | 1.00 | +| `structs_100_fields_10 bon` | 1.561 ± 0.018 | 1.515 | 1.580 | 17.55 ± 2.05 | +| `structs_100_fields_10 typed-builder` | 1.656 ± 0.019 | 1.617 | 1.678 | 18.62 ± 2.18 | +| `structs_100_fields_10 derive_builder` | 1.027 ± 0.016 | 1.006 | 1.056 | 11.55 ± 1.36 | +| `structs_100_fields_10 ` | 0.110 ± 0.011 | 0.096 | 0.130 | 1.23 ± 0.19 | +| `structs_10_fields_50 bon` | 1.493 ± 0.022 | 1.464 | 1.525 | 16.79 ± 1.97 | +| `structs_10_fields_50 typed-builder` | 1.994 ± 0.021 | 1.963 | 2.032 | 22.42 ± 2.62 | +| `structs_10_fields_50 derive_builder` | 0.418 ± 0.016 | 0.402 | 0.449 | 4.70 ± 0.58 | +| `structs_10_fields_50 ` | 0.089 ± 0.010 | 0.073 | 0.117 | 1.00 | diff --git a/bon-macros/src/builder/builder_gen/builder_derives/mod.rs b/bon-macros/src/builder/builder_gen/builder_derives/mod.rs index 604b02c9..b633b296 100644 --- a/bon-macros/src/builder/builder_gen/builder_derives/mod.rs +++ b/bon-macros/src/builder/builder_gen/builder_derives/mod.rs @@ -9,7 +9,8 @@ use crate::util::prelude::*; use darling::ast::GenericParamExt; impl BuilderGenCtx { - pub(crate) fn builder_derives(&self) -> Result { + /// Returns a separate [`TokenStream`] for every generated item + pub(crate) fn builder_derives(&self) -> Result> { let DerivesConfig { clone, debug, @@ -17,25 +18,25 @@ impl BuilderGenCtx { into_future, } = &self.builder_type.derives; - let mut tokens = TokenStream::new(); + let mut items = vec![]; if let Some(derive) = clone { - tokens.extend(self.derive_clone(derive)); + items.push(self.derive_clone(derive)); } if let Some(derive) = debug { - tokens.extend(self.derive_debug(derive)); + items.push(self.derive_debug(derive)); } if into.is_present() { - tokens.extend(self.derive_into()?); + items.push(self.derive_into()?); } if let Some(derive) = into_future { - tokens.extend(self.derive_into_future(derive)?); + items.push(self.derive_into_future(derive)?); } - Ok(tokens) + Ok(items) } /// We follow the logic of the standard `#[derive(...)]` macros such as `Clone` and `Debug`. diff --git a/bon-macros/src/builder/builder_gen/member/named.rs b/bon-macros/src/builder/builder_gen/member/named.rs index 8448a90d..6033d82b 100644 --- a/bon-macros/src/builder/builder_gen/member/named.rs +++ b/bon-macros/src/builder/builder_gen/member/named.rs @@ -258,10 +258,6 @@ impl NamedMember { } } - pub(crate) fn is(&self, other: &Self) -> bool { - self.index == other.index - } - pub(crate) fn merge_on_config(&mut self, on: &[OnConfig]) -> Result { self.merge_config_default(on)?; diff --git a/bon-macros/src/builder/builder_gen/mod.rs b/bon-macros/src/builder/builder_gen/mod.rs index 016c41f7..59e8f5b1 100644 --- a/bon-macros/src/builder/builder_gen/mod.rs +++ b/bon-macros/src/builder/builder_gen/mod.rs @@ -64,46 +64,30 @@ impl BuilderGenCtx { deprecated )]); - let allows = self.allow_attrs.iter().cloned().chain([default_allows]); - - // -- Postprocessing -- - // Here we parse all items back and add the `allow` attributes to them. - let other_items = quote! { - #builder_decl - #builder_impl - #builder_derives - #state_mod - }; - - let other_items_str = other_items.to_string(); - - let other_items: syn::File = syn::parse2(other_items).map_err(|err| { - err!( - &Span::call_site(), - "bug in the `bon` crate: the macro generated code that contains syntax errors; \ - please report this issue at our Github repository: \ - https://github.com/elastio/bon;\n\ - syntax error in generated code: {err:#?};\n\ - generated code:\n\ - ```rust - {other_items_str}\n\ - ```", - ) - })?; - - let mut other_items = other_items.items; - - for item in &mut other_items { - if let Some(attrs) = item.attrs_mut() { - attrs.extend(allows.clone()); - } - } + let allows = self + .allow_attrs + .iter() + .cloned() + .chain([default_allows]) + .collect::>(); + + // Every item here is a single top-level item. We add the `allow` + // attributes to each of them directly. Previously this code converted + // the final token stream to string and parsed it to `syn::File`, which + // resulted in a significant performance hit. Ouch. Don't do that again! + let other_items = [builder_decl, builder_impl] + .into_iter() + .chain(builder_derives) + .chain([state_mod]) + .map(|item| quote!(#(#allows)* #item)); + + let other_items = quote!(#(#other_items)*); start_fn.attrs.extend(allows); Ok(MacroOutput { start_fn, - other_items: quote!(#(#other_items)*), + other_items, }) } @@ -148,7 +132,21 @@ impl BuilderGenCtx { #allows // Ignore dead code warnings because some setter/getter methods may // not be used - #[allow(dead_code)] + #[allow( + dead_code, + // This is intentional. We want the builder syntax to compile away + clippy::inline_always, + // We don't want to avoid using `impl Trait` in the setter. This way + // the setter signature is easier to read, and anyway if you want to + // specify a type hint for the method that accepts an `impl Into`, then + // your design of this setter already went wrong. + clippy::impl_trait_in_params, + clippy::missing_const_for_fn, + // When having a field which has one of the prefixes listed by + // `clippy::wrong_self_convention` you will end up getting said lint + // warning in your `bon::Builder` because we take self by value. + clippy::wrong_self_convention, + )] #[automatically_derived] impl< #(#generics_decl,)* diff --git a/bon-macros/src/builder/builder_gen/setters/mod.rs b/bon-macros/src/builder/builder_gen/setters/mod.rs index f08d62e2..f5a302b7 100644 --- a/bon-macros/src/builder/builder_gen/setters/mod.rs +++ b/bon-macros/src/builder/builder_gen/setters/mod.rs @@ -458,20 +458,6 @@ impl<'a> SettersCtx<'a> { quote_spanned! {self.member.span=> #( #docs )* - #[allow( - // This is intentional. We want the builder syntax to compile away - clippy::inline_always, - // We don't want to avoid using `impl Trait` in the setter. This way - // the setter signature is easier to read, and anyway if you want to - // specify a type hint for the method that accepts an `impl Into`, then - // your design of this setter already went wrong. - clippy::impl_trait_in_params, - clippy::missing_const_for_fn, - // When having a field which has one of the prefixes listed by - // `clippy::wrong_self_convention` you will end up getting said lint - // warning in your `bon::Builder` because we take self by value. - clippy::wrong_self_convention, - )] #[inline(always)] #(#fn_modifiers)* fn #name(#maybe_mut #self_, #( #pats: #types ),*) -> #return_type #where_clause @@ -700,13 +686,12 @@ fn well_known_default(ty: &syn::Type) -> Option { Some(value) } -/// Unfortunately there is no `syn::Parse` impl for `PatIdent` directly, -/// so we use this workaround instead. fn pat_ident(ident_name: &'static str) -> syn::PatIdent { - let ident = syn::Ident::new(ident_name, Span::call_site()); - let pat: syn::Pat = syn::parse_quote!(#ident); - match pat { - syn::Pat::Ident(pat_ident) => pat_ident, - _ => unreachable!("can't parse something else than PatIdent here: {pat:?}"), + syn::PatIdent { + attrs: vec![], + by_ref: None, + mutability: None, + ident: syn::Ident::new(ident_name, Span::call_site()), + subpat: None, } } diff --git a/bon-macros/src/builder/builder_gen/state_mod.rs b/bon-macros/src/builder/builder_gen/state_mod.rs index 65baa16b..f93b6110 100644 --- a/bon-macros/src/builder/builder_gen/state_mod.rs +++ b/bon-macros/src/builder/builder_gen/state_mod.rs @@ -5,8 +5,6 @@ pub(super) struct StateModGenCtx<'a> { base: &'a BuilderGenCtx, stateful_members_snake: Vec<&'a syn::Ident>, stateful_members_pascal: Vec<&'a syn::Ident>, - sealed_item_decl: TokenStream, - sealed_item_impl: TokenStream, } impl<'a> StateModGenCtx<'a> { @@ -23,17 +21,6 @@ impl<'a> StateModGenCtx<'a> { .stateful_members() .map(|member| &member.name.pascal) .collect(), - - // A const item in a trait makes it non-object safe, which is convenient, - // because we want that restriction in this case. - sealed_item_decl: quote! { - #[doc(hidden)] - const SEALED: sealed::Sealed; - }, - - sealed_item_impl: quote! { - const SEALED: sealed::Sealed = sealed::Sealed; - }, } } @@ -60,7 +47,7 @@ impl<'a> StateModGenCtx<'a> { // to expose this API surface. // // Also, there are some genuinely private items like the `Sealed` - // enum and members "name" enums that we don't want to expose even + // trait and members "name" enums that we don't want to expose even // to the module that defines the builder. These APIs are not // public, and users instead should only reference the traits // and state transition type aliases from here. @@ -73,7 +60,7 @@ impl<'a> StateModGenCtx<'a> { use #bon::__::{Set, Unset}; mod sealed { - #vis_child_child struct Sealed; + #vis_child_child trait Sealed {} } #state_trait @@ -85,65 +72,33 @@ impl<'a> StateModGenCtx<'a> { } fn state_transitions(&self) -> TokenStream { - // Not using `Iterator::zip` here to make it possible to scale this in - // case if we add more vecs here. We are not using `Itertools`, so - // its `multiunzip` is not available. - let mut set_members_structs = Vec::with_capacity(self.stateful_members_snake.len()); - let mut state_impls = Vec::with_capacity(self.stateful_members_snake.len()); - let vis_child = &self.base.state_mod.vis_child; - let sealed_item_impl = &self.sealed_item_impl; - - for member in self.base.stateful_members() { - let member_pascal = &member.name.pascal; + let stateful_members_snake = &self.stateful_members_snake; + let stateful_members_pascal = &self.stateful_members_pascal; - let docs = format!( + let docs = stateful_members_pascal.iter().map(|member_pascal| { + format!( "Represents a [`State`] that has [`IsSet`] implemented for [`State::{member_pascal}`].\n\n\ The state for all other members is left the same as in the input state.", - ); - - let struct_ident = format_ident!("Set{}", member.name.pascal_str); - - set_members_structs.push(quote! { - #[doc = #docs] - #vis_child struct #struct_ident( - // We `S` in an `fn() -> ...` to make the compiler think - // that the builder doesn't "own" an instance of `S`. - // This removes unnecessary requirements when evaluating the - // applicability of the auto traits. - ::core::marker::PhantomData S> - ); - }); - - let states = self.base.stateful_members().map(|other_member| { - if other_member.is(member) { - let member_snake = &member.name.snake; - quote! { - Set - } - } else { - let member_pascal = &other_member.name.pascal; - quote! { - S::#member_pascal - } - } - }); - - let stateful_members_pascal = &self.stateful_members_pascal; + ) + }); - state_impls.push(quote! { - #[doc(hidden)] - impl State for #struct_ident { - #( - type #stateful_members_pascal = #states; - )* - #sealed_item_impl - } - }); - } + let structs_idents = self + .base + .stateful_members() + .map(|member| format_ident!("Set{}", member.name.pascal_str)) + .collect::>(); - let stateful_members_snake = &self.stateful_members_snake; - let stateful_members_pascal = &self.stateful_members_pascal; + // Each separate token stream costs a call to the compiler when it is + // added to another token stream. So, each impl lists the members before + // and after the set member in repetitions of a single `quote!`. + let (members_before, members_after): (Vec<_>, Vec<_>) = (0..stateful_members_pascal.len()) + .filter_map(|i| { + let (before, rest) = stateful_members_pascal.split_at_checked(i)?; + let (_, after) = rest.split_first()?; + Some((before, after)) + }) + .unzip(); quote! { /// Represents a [`State`] that has [`IsUnset`] implemented for all members. @@ -151,18 +106,40 @@ impl<'a> StateModGenCtx<'a> { /// This is the initial state of the builder before any setters are called. #vis_child struct Empty(()); - #( #set_members_structs )* + #( + #[doc = #docs] + #vis_child struct #structs_idents( + // We `S` in an `fn() -> ...` to make the compiler think + // that the builder doesn't "own" an instance of `S`. + // This removes unnecessary requirements when evaluating the + // applicability of the auto traits. + ::core::marker::PhantomData S> + ); + )* #[doc(hidden)] impl State for Empty { #( type #stateful_members_pascal = Unset; )* - #sealed_item_impl } - #( #state_impls )* + impl sealed::Sealed for Empty {} + #( + #[doc(hidden)] + impl State for #structs_idents { + #( + type #members_before = S::#members_before; + )* + type #stateful_members_pascal = Set; + #( + type #members_after = S::#members_after; + )* + } + + impl sealed::Sealed for #structs_idents {} + )* } } @@ -176,7 +153,6 @@ impl<'a> StateModGenCtx<'a> { }); let vis_child = &self.base.state_mod.vis_child; - let sealed_item_decl = &self.sealed_item_decl; let stateful_members_pascal = &self.stateful_members_pascal; let docs_suffix = if stateful_members_pascal.is_empty() { @@ -194,12 +170,13 @@ impl<'a> StateModGenCtx<'a> { quote! { #[doc = #docs] - #vis_child trait State: ::core::marker::Sized { + // Code outside of this module can't name the `Sealed` trait, so it + // can't implement the `State` trait. + #vis_child trait State: ::core::marker::Sized + sealed::Sealed { #( #[doc = #assoc_types_docs] type #stateful_members_pascal; )* - #sealed_item_decl } } } @@ -213,8 +190,6 @@ impl<'a> StateModGenCtx<'a> { .collect::>(); let vis_child = &self.base.state_mod.vis_child; - let sealed_item_decl = &self.sealed_item_decl; - let sealed_item_impl = &self.sealed_item_impl; let builder_ident = &self.base.builder_type.ident; let finish_fn = &self.base.finish_fn.ident; @@ -225,11 +200,10 @@ impl<'a> StateModGenCtx<'a> { [`{builder_ident}::{finish_fn}()`](super::{builder_ident}::{finish_fn}())", ); + // This trait doesn't need its own sealing. Its supertrait `State` is sealed. quote! { #[doc = #docs] - #vis_child trait IsComplete: State< #( #required_members_pascal: IsSet, )* > { - #sealed_item_decl - } + #vis_child trait IsComplete: State< #( #required_members_pascal: IsSet, )* > {} #[doc(hidden)] impl IsComplete for S @@ -237,9 +211,7 @@ impl<'a> StateModGenCtx<'a> { #( S::#required_members_pascal: IsSet, )* - { - #sealed_item_impl - } + {} } } diff --git a/bon-macros/src/util/item.rs b/bon-macros/src/util/item.rs deleted file mode 100644 index d8575a03..00000000 --- a/bon-macros/src/util/item.rs +++ /dev/null @@ -1,28 +0,0 @@ -pub(crate) trait ItemExt { - fn attrs_mut(&mut self) -> Option<&mut Vec>; -} - -impl ItemExt for syn::Item { - fn attrs_mut(&mut self) -> Option<&mut Vec> { - let attrs = match self { - Self::Const(item) => &mut item.attrs, - Self::Enum(item) => &mut item.attrs, - Self::ExternCrate(item) => &mut item.attrs, - Self::Fn(item) => &mut item.attrs, - Self::ForeignMod(item) => &mut item.attrs, - Self::Impl(item) => &mut item.attrs, - Self::Macro(item) => &mut item.attrs, - Self::Mod(item) => &mut item.attrs, - Self::Static(item) => &mut item.attrs, - Self::Struct(item) => &mut item.attrs, - Self::Trait(item) => &mut item.attrs, - Self::TraitAlias(item) => &mut item.attrs, - Self::Type(item) => &mut item.attrs, - Self::Union(item) => &mut item.attrs, - Self::Use(item) => &mut item.attrs, - _ => return None, - }; - - Some(attrs) - } -} diff --git a/bon-macros/src/util/mod.rs b/bon-macros/src/util/mod.rs index 996945fb..4269ade5 100644 --- a/bon-macros/src/util/mod.rs +++ b/bon-macros/src/util/mod.rs @@ -3,7 +3,6 @@ mod expr; mod fn_arg; mod generic_param; mod ident; -mod item; mod iterator; mod meta_list; mod path; @@ -33,7 +32,6 @@ pub(crate) mod prelude { pub(crate) use super::fn_arg::FnArgExt; pub(crate) use super::generic_param::GenericParamExt; pub(crate) use super::ident::IdentExt; - pub(crate) use super::item::ItemExt; pub(crate) use super::iterator::{IntoIteratorExt, IteratorExt}; pub(crate) use super::meta_list::MetaListExt; pub(crate) use super::path::PathExt; diff --git a/bon-macros/tests/snapshots/setters_docs_and_vis.rs b/bon-macros/tests/snapshots/setters_docs_and_vis.rs index b55bd9e9..19c268e3 100644 --- a/bon-macros/tests/snapshots/setters_docs_and_vis.rs +++ b/bon-macros/tests/snapshots/setters_docs_and_vis.rs @@ -1,7 +1,13 @@ +#[allow(deprecated)] #[allow(unused_parens)] -#[allow(dead_code)] +#[allow( + dead_code, + clippy::inline_always, + clippy::impl_trait_in_params, + clippy::missing_const_for_fn, + clippy::wrong_self_convention, +)] #[automatically_derived] -#[allow(deprecated)] impl SutBuilder { /**_**Required.**_ diff --git a/bon/tests/integration/ui/compile_fail/sealed_state.rs b/bon/tests/integration/ui/compile_fail/sealed_state.rs new file mode 100644 index 00000000..f1004a48 --- /dev/null +++ b/bon/tests/integration/ui/compile_fail/sealed_state.rs @@ -0,0 +1,14 @@ +use bon::Builder; + +#[derive(Builder)] +struct Sut { + x1: u32, +} + +struct CustomState; + +impl sut_builder::State for CustomState { + type X1 = sut_builder::SetX1; +} + +fn main() {} diff --git a/bon/tests/integration/ui/compile_fail/sealed_state.stderr b/bon/tests/integration/ui/compile_fail/sealed_state.stderr new file mode 100644 index 00000000..15c3c4b0 --- /dev/null +++ b/bon/tests/integration/ui/compile_fail/sealed_state.stderr @@ -0,0 +1,29 @@ +error[E0277]: the trait bound `CustomState: Sealed` is not satisfied + --> tests/integration/ui/compile_fail/sealed_state.rs:10:29 + | +10 | impl sut_builder::State for CustomState { + | ^^^^^^^^^^^ unsatisfied trait bound + | +help: the trait `Sealed` is not implemented for `CustomState` + --> tests/integration/ui/compile_fail/sealed_state.rs:8:1 + | + 8 | struct CustomState; + | ^^^^^^^^^^^^^^^^^^ +help: the following other types implement trait `Sealed` + --> tests/integration/ui/compile_fail/sealed_state.rs:3:10 + | + 3 | #[derive(Builder)] + | ^^^^^^^ + | | + | `SetX1` + | `sut_builder::Empty` +note: required by a bound in `State` + --> tests/integration/ui/compile_fail/sealed_state.rs:3:10 + | + 3 | #[derive(Builder)] + | ^^^^^^^ required by this bound in `State` + = note: `State` is a "sealed trait", because to implement it you also need to implement `sut_builder::sealed::Sealed`, which is not accessible; this is usually done to force you to use one of the provided types that already implement it + = help: the following types implement the trait: + sut_builder::Empty + sut_builder::SetX1 + = note: this error originates in the derive macro `Builder` (in Nightly builds, run with -Z macro-backtrace for more info)