From f49ca28dd0003d7b7ce18efc1e627aa1a0aaf9f3 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 8 Mar 2022 15:13:11 +0000 Subject: [PATCH 01/72] ParsedLiteral: New utility type for parsing syn items from attrs No call site yet. --- derive_builder_core/src/lib.rs | 1 + derive_builder_core/src/parsed_literal.rs | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+) create mode 100644 derive_builder_core/src/parsed_literal.rs diff --git a/derive_builder_core/src/lib.rs b/derive_builder_core/src/lib.rs index 56670ba9..90696857 100644 --- a/derive_builder_core/src/lib.rs +++ b/derive_builder_core/src/lib.rs @@ -41,6 +41,7 @@ mod doc_comment; mod initializer; mod macro_options; mod options; +mod parsed_literal; mod setter; pub(crate) use block::BlockContents; diff --git a/derive_builder_core/src/parsed_literal.rs b/derive_builder_core/src/parsed_literal.rs new file mode 100644 index 00000000..9714e48a --- /dev/null +++ b/derive_builder_core/src/parsed_literal.rs @@ -0,0 +1,23 @@ +use proc_macro2::TokenStream; +use quote::ToTokens; +use syn; + +/// A wrapper for syn types which impl FromMeta, parsing them from a stirng literal +#[derive(Debug, Clone)] +pub struct ParsedLiteral(pub T); + +impl ToTokens for ParsedLiteral { + fn to_tokens(&self, tokens: &mut TokenStream) { + self.0.to_tokens(tokens) + } +} + +impl darling::FromMeta for ParsedLiteral { + fn from_value(value: &syn::Lit) -> darling::Result { + if let syn::Lit::Str(s) = value { + Ok(ParsedLiteral(s.parse()?)) + } else { + Err(darling::Error::unexpected_lit_type(value)) + } + } +} From 91f3ca23d6bcc75a944c78477c6f0e2e345cfd0b Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Mon, 14 Mar 2022 18:19:46 +0000 Subject: [PATCH 02/72] BlockContents: impl FromMeta We are going to introduce a setting that always takes a literal string containing the contents of a block. --- derive_builder_core/src/block.rs | 15 +++++++++++++++ derive_builder_core/src/default_expression.rs | 13 +------------ 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/derive_builder_core/src/block.rs b/derive_builder_core/src/block.rs index 5a626f33..d412f901 100644 --- a/derive_builder_core/src/block.rs +++ b/derive_builder_core/src/block.rs @@ -44,6 +44,21 @@ impl From for BlockContents { } } +impl darling::FromMeta for BlockContents { + fn from_value(value: &syn::Lit) -> darling::Result { + if let syn::Lit::Str(s) = value { + let contents = BlockContents::try_from(s)?; + if contents.is_empty() { + Err(darling::Error::unknown_value("").with_span(s)) + } else { + Ok(contents) + } + } else { + Err(darling::Error::unexpected_lit_type(value)) + } + } +} + #[cfg(test)] mod test { use std::convert::TryInto; diff --git a/derive_builder_core/src/default_expression.rs b/derive_builder_core/src/default_expression.rs index ed950bb5..c45f8ca3 100644 --- a/derive_builder_core/src/default_expression.rs +++ b/derive_builder_core/src/default_expression.rs @@ -1,5 +1,3 @@ -use std::convert::TryFrom; - use crate::BlockContents; use quote::{ToTokens, TokenStreamExt}; @@ -23,16 +21,7 @@ impl darling::FromMeta for DefaultExpression { } fn from_value(value: &syn::Lit) -> darling::Result { - if let syn::Lit::Str(s) = value { - let contents = BlockContents::try_from(s)?; - if contents.is_empty() { - Err(darling::Error::unknown_value("").with_span(s)) - } else { - Ok(Self::Explicit(contents)) - } - } else { - Err(darling::Error::unexpected_lit_type(value)) - } + Ok(Self::Explicit(BlockContents::from_value(value)?)) } } From 2986ee420bebe5d7ee3a2a05cc94d3e84d1cc2d4 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 8 Mar 2022 17:18:39 +0000 Subject: [PATCH 03/72] Introduce BuilderFieldType enum We are going to make some fields in the builder be a bare value. This enum allows us to avoid accidentally using the field the wrong way. It impl ToTokens, which adds the Option (where applicable), and provides access to the target field type (i.e., usually, the version without the Option). Right now there is only one variant - no functional change yet. --- derive_builder_core/src/builder_field.rs | 40 +++++++++++++++---- derive_builder_core/src/lib.rs | 2 +- .../src/macro_options/darling_opts.rs | 10 +++-- derive_builder_core/src/setter.rs | 11 ++--- 4 files changed, 47 insertions(+), 16 deletions(-) diff --git a/derive_builder_core/src/builder_field.rs b/derive_builder_core/src/builder_field.rs index 32a40475..c69c226b 100644 --- a/derive_builder_core/src/builder_field.rs +++ b/derive_builder_core/src/builder_field.rs @@ -34,9 +34,7 @@ pub struct BuilderField<'a> { /// Name of the target field. pub field_ident: &'a syn::Ident, /// Type of the target field. - /// - /// The corresonding builder field will be `Option`. - pub field_type: &'a syn::Type, + pub field_type: BuilderFieldType<'a>, /// Whether the builder implements a setter for this field. /// /// Note: We will fallback to `PhantomData` if the setter is disabled @@ -49,20 +47,36 @@ pub struct BuilderField<'a> { pub attrs: &'a [syn::Attribute], } +/// The type of a field in the builder struct, implementing `quote::ToTokens` +#[derive(Debug, Clone)] +pub enum BuilderFieldType<'a> { + /// The corresonding builder field will be `Option`. + Optional(&'a syn::Type), +} + +impl<'a> BuilderFieldType<'a> { + /// Some call sites want the target field type + pub fn target_type(&'a self) -> &'a syn::Type { + match self { + BuilderFieldType::Optional(ty) => ty, + } + } +} + impl<'a> ToTokens for BuilderField<'a> { fn to_tokens(&self, tokens: &mut TokenStream) { if self.field_enabled { let vis = &self.field_visibility; let ident = self.field_ident; - let ty = self.field_type; + let ty = &self.field_type; let attrs = self.attrs; tokens.append_all(quote!( - #(#attrs)* #vis #ident: ::derive_builder::export::core::option::Option<#ty>, + #(#attrs)* #vis #ident: #ty, )); } else { let ident = self.field_ident; - let ty = self.field_type; + let ty = self.field_type.target_type(); let attrs = self.attrs; tokens.append_all(quote!( @@ -72,6 +86,18 @@ impl<'a> ToTokens for BuilderField<'a> { } } +impl<'a> ToTokens for BuilderFieldType<'a> { + fn to_tokens(&self, tokens: &mut TokenStream) { + match self { + BuilderFieldType::Optional(ty) => { + tokens.append_all(quote!( + ::derive_builder::export::core::option::Option<#ty> + )) + }, + } + } +} + impl<'a> BuilderField<'a> { /// Emits a struct field initializer that initializes the field to `Default::default`. pub fn default_initializer_tokens(&self) -> TokenStream { @@ -88,7 +114,7 @@ macro_rules! default_builder_field { () => {{ BuilderField { field_ident: &syn::Ident::new("foo", ::proc_macro2::Span::call_site()), - field_type: &parse_quote!(String), + field_type: BuilderFieldType::Optional(Box::leak(Box::new(parse_quote!(String)))), field_enabled: true, field_visibility: ::std::borrow::Cow::Owned(parse_quote!(pub)), attrs: &[parse_quote!(#[some_attr])], diff --git a/derive_builder_core/src/lib.rs b/derive_builder_core/src/lib.rs index 90696857..3a42e85a 100644 --- a/derive_builder_core/src/lib.rs +++ b/derive_builder_core/src/lib.rs @@ -47,7 +47,7 @@ mod setter; pub(crate) use block::BlockContents; pub(crate) use build_method::BuildMethod; pub(crate) use builder::Builder; -pub(crate) use builder_field::BuilderField; +pub(crate) use builder_field::{BuilderField, BuilderFieldType}; use darling::FromDeriveInput; pub(crate) use default_expression::DefaultExpression; pub(crate) use deprecation_notes::DeprecationNotes; diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index c8cce54d..612ffc47 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -10,7 +10,7 @@ use syn::Meta; use syn::{self, spanned::Spanned, Attribute, Generics, Ident, Path}; use crate::{ - Builder, BuilderField, BuilderPattern, DefaultExpression, DeprecationNotes, Each, Initializer, + Builder, BuilderField, BuilderFieldType, BuilderPattern, DefaultExpression, DeprecationNotes, Each, Initializer, Setter, }; @@ -757,6 +757,10 @@ impl<'a> FieldWithDefaults<'a> { .unwrap_or(Cow::Owned(syn::Visibility::Inherited)) } + pub fn field_type(&'a self) -> BuilderFieldType<'a> { + BuilderFieldType::Optional(&self.field.ty) + } + pub fn pattern(&self) -> BuilderPattern { self.field.pattern.unwrap_or(self.parent.pattern) } @@ -782,7 +786,7 @@ impl<'a> FieldWithDefaults<'a> { attrs: &self.field.setter_attrs, ident: self.setter_ident(), field_ident: self.field_ident(), - field_type: &self.field.ty, + field_type: self.field_type(), generic_into: self.setter_into(), strip_option: self.setter_strip_option(), deprecation_notes: self.deprecation_notes(), @@ -814,7 +818,7 @@ impl<'a> FieldWithDefaults<'a> { pub fn as_builder_field(&'a self) -> BuilderField<'a> { BuilderField { field_ident: self.field_ident(), - field_type: &self.field.ty, + field_type: self.field_type(), field_enabled: self.field_enabled(), field_visibility: self.field_vis(), attrs: &self.field.field_attrs, diff --git a/derive_builder_core/src/setter.rs b/derive_builder_core/src/setter.rs index 68b8d359..9eb5fb99 100644 --- a/derive_builder_core/src/setter.rs +++ b/derive_builder_core/src/setter.rs @@ -5,6 +5,7 @@ use proc_macro2::{Span, TokenStream}; use quote::{ToTokens, TokenStreamExt}; use syn; +use BuilderFieldType; use BuilderPattern; use DeprecationNotes; use Each; @@ -57,7 +58,7 @@ pub struct Setter<'a> { /// Type of the target field. /// /// The corresonding builder field will be `Option`. - pub field_type: &'a syn::Type, + pub field_type: BuilderFieldType<'a>, /// Make the setter generic over `Into`, where `T` is the field type. pub generic_into: bool, /// Make the setter remove the Option wrapper from the setter, remove the need to call Some(...). @@ -72,7 +73,7 @@ pub struct Setter<'a> { impl<'a> ToTokens for Setter<'a> { fn to_tokens(&self, tokens: &mut TokenStream) { if self.setter_enabled { - let field_type = self.field_type; + let field_type = self.field_type.target_type(); let pattern = self.pattern; let vis = &self.visibility; let field_ident = self.field_ident; @@ -273,7 +274,7 @@ macro_rules! default_setter { attrs: &vec![], ident: syn::Ident::new("foo", ::proc_macro2::Span::call_site()), field_ident: &syn::Ident::new("foo", ::proc_macro2::Span::call_site()), - field_type: &parse_quote!(Foo), + field_type: BuilderFieldType::Optional(Box::leak(Box::new(parse_quote!(Foo)))), generic_into: false, strip_option: false, deprecation_notes: &Default::default(), @@ -393,7 +394,7 @@ mod tests { let ty = parse_quote!(Option); let mut setter = default_setter!(); setter.strip_option = true; - setter.field_type = &ty; + setter.field_type = BuilderFieldType::Optional(&ty); #[rustfmt::skip] assert_eq!( @@ -418,7 +419,7 @@ mod tests { let mut setter = default_setter!(); setter.strip_option = true; setter.generic_into = true; - setter.field_type = &ty; + setter.field_type = BuilderFieldType::Optional(&ty); #[rustfmt::skip] assert_eq!( From c934e3d47370b2a285fa533341cc862f19f2db68 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 16 Mar 2022 12:06:00 +0000 Subject: [PATCH 04/72] Break wrap_expression_in_some and BuilderFieldType::wrap_some Whether to wrap is going to change depending on the field type, so take care of that now. No functional change. --- derive_builder_core/src/builder_field.rs | 8 ++++++++ derive_builder_core/src/lib.rs | 2 +- derive_builder_core/src/setter.rs | 15 +++++++++++---- 3 files changed, 20 insertions(+), 5 deletions(-) diff --git a/derive_builder_core/src/builder_field.rs b/derive_builder_core/src/builder_field.rs index c69c226b..293eef73 100644 --- a/derive_builder_core/src/builder_field.rs +++ b/derive_builder_core/src/builder_field.rs @@ -3,6 +3,7 @@ use std::borrow::Cow; use proc_macro2::TokenStream; use quote::{ToTokens, TokenStreamExt}; use syn; +use crate::wrap_expression_in_some; /// Field for the builder struct, implementing `quote::ToTokens`. /// @@ -61,6 +62,13 @@ impl<'a> BuilderFieldType<'a> { BuilderFieldType::Optional(ty) => ty, } } + + /// Returns expression wrapping `bare_value` in Some, if appropriate + pub fn wrap_some(&'a self, bare_value: TokenStream) -> TokenStream { + match self { + BuilderFieldType::Optional(_) => wrap_expression_in_some(bare_value), + } + } } impl<'a> ToTokens for BuilderField<'a> { diff --git a/derive_builder_core/src/lib.rs b/derive_builder_core/src/lib.rs index 3a42e85a..a29d3644 100644 --- a/derive_builder_core/src/lib.rs +++ b/derive_builder_core/src/lib.rs @@ -54,7 +54,7 @@ pub(crate) use deprecation_notes::DeprecationNotes; pub(crate) use doc_comment::doc_comment_from; pub(crate) use initializer::Initializer; pub(crate) use options::{BuilderPattern, Each}; -pub(crate) use setter::Setter; +pub(crate) use setter::{wrap_expression_in_some, Setter}; const DEFAULT_STRUCT_NAME: &str = "__default"; diff --git a/derive_builder_core/src/setter.rs b/derive_builder_core/src/setter.rs index 9eb5fb99..594caa65 100644 --- a/derive_builder_core/src/setter.rs +++ b/derive_builder_core/src/setter.rs @@ -128,9 +128,10 @@ impl<'a> ToTokens for Setter<'a> { into_value = quote!(value); } if stripped_option { - into_value = - quote!(::derive_builder::export::core::option::Option::Some(#into_value)); + into_value = wrap_expression_in_some(into_value); } + into_value = self.field_type.wrap_some(into_value); + tokens.append_all(quote!( #(#attrs)* #[allow(unused_mut)] @@ -139,7 +140,7 @@ impl<'a> ToTokens for Setter<'a> { { #deprecation_notes let mut new = #self_into_return_ty; - new.#field_ident = ::derive_builder::export::core::option::Option::Some(#into_value); + new.#field_ident = #into_value; new } )); @@ -148,6 +149,7 @@ impl<'a> ToTokens for Setter<'a> { let try_ty_params = quote!(>); let try_ident = syn::Ident::new(&format!("try_{}", ident), Span::call_site()); + let converted = self.field_type.wrap_some(quote!{converted}); tokens.append_all(quote!( #(#attrs)* @@ -156,7 +158,7 @@ impl<'a> ToTokens for Setter<'a> { { let converted : #ty = value.try_into()?; let mut new = #self_into_return_ty; - new.#field_ident = ::derive_builder::export::core::option::Option::Some(converted); + new.#field_ident = #converted; Ok(new) } )); @@ -213,6 +215,11 @@ impl<'a> ToTokens for Setter<'a> { } } +/// Returns expression wrapping `bare_value` in `Some` +pub fn wrap_expression_in_some(bare_value: TokenStream) -> TokenStream { + quote!( ::derive_builder::export::core::option::Option::Some(#bare_value) ) +} + // adapted from https://stackoverflow.com/a/55277337/469066 // Note that since syn is a parser, it works with tokens. // We cannot know for sure that this is an Option. From 363c966d8dd95a958a8a9746d178e0d055bf1a50 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 16 Mar 2022 12:53:29 +0000 Subject: [PATCH 05/72] BuilderFieldType: Provide Precisely variant For on-Option-wrapped builder fields. Nothing uses this yet... --- derive_builder_core/src/builder_field.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/derive_builder_core/src/builder_field.rs b/derive_builder_core/src/builder_field.rs index 293eef73..d78adb21 100644 --- a/derive_builder_core/src/builder_field.rs +++ b/derive_builder_core/src/builder_field.rs @@ -53,6 +53,9 @@ pub struct BuilderField<'a> { pub enum BuilderFieldType<'a> { /// The corresonding builder field will be `Option`. Optional(&'a syn::Type), + /// The corresponding builder field will be just this type + #[allow(dead_code)] + Precisely(&'a syn::Type), } impl<'a> BuilderFieldType<'a> { @@ -60,6 +63,7 @@ impl<'a> BuilderFieldType<'a> { pub fn target_type(&'a self) -> &'a syn::Type { match self { BuilderFieldType::Optional(ty) => ty, + BuilderFieldType::Precisely(ty) => ty, } } @@ -67,6 +71,7 @@ impl<'a> BuilderFieldType<'a> { pub fn wrap_some(&'a self, bare_value: TokenStream) -> TokenStream { match self { BuilderFieldType::Optional(_) => wrap_expression_in_some(bare_value), + BuilderFieldType::Precisely(_) => bare_value, } } } @@ -102,6 +107,7 @@ impl<'a> ToTokens for BuilderFieldType<'a> { ::derive_builder::export::core::option::Option<#ty> )) }, + BuilderFieldType::Precisely(ty) => ty.to_tokens(tokens), } } } From 1c18f6eb588cd99342624bd6f4b94f7050ad9bbb Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 16 Mar 2022 12:34:01 +0000 Subject: [PATCH 06/72] initializer: Swap two if arms This makes the next changes clearer --- derive_builder_core/src/initializer.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/derive_builder_core/src/initializer.rs b/derive_builder_core/src/initializer.rs index 08cd8006..7da1e414 100644 --- a/derive_builder_core/src/initializer.rs +++ b/derive_builder_core/src/initializer.rs @@ -65,7 +65,12 @@ impl<'a> ToTokens for Initializer<'a> { fn to_tokens(&self, tokens: &mut TokenStream) { let struct_field = &self.field_ident; - if self.field_enabled { + if ! self.field_enabled { + let default = self.default(); + tokens.append_all(quote!( + #struct_field: #default, + )); + } else { let match_some = self.match_some(); let match_none = self.match_none(); let builder_field = &*struct_field; @@ -75,11 +80,6 @@ impl<'a> ToTokens for Initializer<'a> { #match_none, }, )); - } else { - let default = self.default(); - tokens.append_all(quote!( - #struct_field: #default, - )); } } } From c4b0e55cb25381686c4cf43814afe47fcce4a479 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 16 Mar 2022 12:34:06 +0000 Subject: [PATCH 07/72] initializer: Hoist builder_field variable, and move framing out of if This reduces the amount of duplication slightly. We're going to add another situation here. No functional change. --- derive_builder_core/src/initializer.rs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/derive_builder_core/src/initializer.rs b/derive_builder_core/src/initializer.rs index 7da1e414..6575ecc0 100644 --- a/derive_builder_core/src/initializer.rs +++ b/derive_builder_core/src/initializer.rs @@ -64,23 +64,31 @@ pub struct Initializer<'a> { impl<'a> ToTokens for Initializer<'a> { fn to_tokens(&self, tokens: &mut TokenStream) { let struct_field = &self.field_ident; + let builder_field = &*struct_field; + + tokens.append_all(quote!( + #struct_field: + )); if ! self.field_enabled { let default = self.default(); tokens.append_all(quote!( - #struct_field: #default, + #default )); } else { let match_some = self.match_some(); let match_none = self.match_none(); - let builder_field = &*struct_field; tokens.append_all(quote!( - #struct_field: match self.#builder_field { + match self.#builder_field { #match_some, #match_none, - }, + } )); } + + tokens.append_all(quote!( + , + )); } } From 4983781ab6156c5541b91807bc96c3dd5f18093d Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 8 Mar 2022 11:33:36 +0000 Subject: [PATCH 08/72] initializer: Allow specifying a custom conversion for initialisation --- derive_builder_core/src/initializer.rs | 9 ++++++++- derive_builder_core/src/macro_options/darling_opts.rs | 1 + 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/derive_builder_core/src/initializer.rs b/derive_builder_core/src/initializer.rs index 6575ecc0..eb884964 100644 --- a/derive_builder_core/src/initializer.rs +++ b/derive_builder_core/src/initializer.rs @@ -4,7 +4,7 @@ use syn; use BuilderPattern; use DEFAULT_STRUCT_NAME; -use crate::DefaultExpression; +use crate::{DefaultExpression, BlockContents}; /// Initializer for the target struct fields, implementing `quote::ToTokens`. /// @@ -59,6 +59,10 @@ pub struct Initializer<'a> { /// they may have forgotten the conversion from `UninitializedFieldError` into their specified /// error type. pub custom_error_type_span: Option, + /// Method to use to to convert the builder's field to the target field + /// + /// For sub-builder fields, this will be `build` (or similar) + pub custom_conversion: Option<&'a BlockContents>, } impl<'a> ToTokens for Initializer<'a> { @@ -75,6 +79,8 @@ impl<'a> ToTokens for Initializer<'a> { tokens.append_all(quote!( #default )); + } else if let Some(conv) = &self.custom_conversion { + conv.to_tokens(tokens); } else { let match_some = self.match_some(); let match_none = self.match_none(); @@ -199,6 +205,7 @@ macro_rules! default_initializer { builder_pattern: BuilderPattern::Mutable, default_value: None, use_default_struct: false, + custom_conversion: None, custom_error_type_span: None, } }; diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index 612ffc47..4bd94709 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -806,6 +806,7 @@ impl<'a> FieldWithDefaults<'a> { builder_pattern: self.pattern(), default_value: self.field.default.as_ref(), use_default_struct: self.use_parent_default(), + custom_conversion: None, custom_error_type_span: self .parent .build_fn From 6b52753cefa578e724bbb6848e9a2a7fe9a238a7 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Mon, 14 Mar 2022 18:58:50 +0000 Subject: [PATCH 09/72] Support builder(custom) attribute on fields --- derive_builder_core/src/builder_field.rs | 1 - derive_builder_core/src/lib.rs | 1 + .../src/macro_options/darling_opts.rs | 29 +++++++++++++++++-- 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/derive_builder_core/src/builder_field.rs b/derive_builder_core/src/builder_field.rs index d78adb21..34aefb35 100644 --- a/derive_builder_core/src/builder_field.rs +++ b/derive_builder_core/src/builder_field.rs @@ -54,7 +54,6 @@ pub enum BuilderFieldType<'a> { /// The corresonding builder field will be `Option`. Optional(&'a syn::Type), /// The corresponding builder field will be just this type - #[allow(dead_code)] Precisely(&'a syn::Type), } diff --git a/derive_builder_core/src/lib.rs b/derive_builder_core/src/lib.rs index a29d3644..0d16e987 100644 --- a/derive_builder_core/src/lib.rs +++ b/derive_builder_core/src/lib.rs @@ -55,6 +55,7 @@ pub(crate) use doc_comment::doc_comment_from; pub(crate) use initializer::Initializer; pub(crate) use options::{BuilderPattern, Each}; pub(crate) use setter::{wrap_expression_in_some, Setter}; +pub(crate) use parsed_literal::ParsedLiteral; const DEFAULT_STRUCT_NAME: &str = "__default"; diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index 4bd94709..35844722 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -11,7 +11,7 @@ use syn::{self, spanned::Spanned, Attribute, Generics, Ident, Path}; use crate::{ Builder, BuilderField, BuilderFieldType, BuilderPattern, DefaultExpression, DeprecationNotes, Each, Initializer, - Setter, + BlockContents, ParsedLiteral, Setter, }; /// `derive_builder` uses separate sibling keywords to represent @@ -286,6 +286,9 @@ pub struct Field { /// This property only captures the first two, the third is computed in `FieldWithDefaults`. default: Option, try_setter: Flag, + /// Custom builder field type and build method + #[darling(default)] + custom: Option, #[darling(default)] field: FieldMeta, #[darling(skip)] @@ -294,6 +297,14 @@ pub struct Field { setter_attrs: Vec, } +/// Options for the field-level `custom` property +#[derive(Debug, Clone, FromMeta)] +pub struct CustomField { + #[darling(rename = "type")] + ty: ParsedLiteral, + build: BlockContents, +} + impl Field { fn no_visibility_conflicts(&self) -> darling::Result<()> { let mut errors = Error::accumulator(); @@ -758,7 +769,19 @@ impl<'a> FieldWithDefaults<'a> { } pub fn field_type(&'a self) -> BuilderFieldType<'a> { - BuilderFieldType::Optional(&self.field.ty) + if let Some(custom) = self.field.custom.as_ref() { + BuilderFieldType::Precisely(&custom.ty.0) + } else { + BuilderFieldType::Optional(&self.field.ty) + } + } + + pub fn custom_conversion(&'a self) -> Option<&'a BlockContents> { + if let Some(custom) = &self.field.custom { + Some(&custom.build) + } else { + None + } } pub fn pattern(&self) -> BuilderPattern { @@ -806,7 +829,7 @@ impl<'a> FieldWithDefaults<'a> { builder_pattern: self.pattern(), default_value: self.field.default.as_ref(), use_default_struct: self.use_parent_default(), - custom_conversion: None, + custom_conversion: self.custom_conversion(), custom_error_type_span: self .parent .build_fn From 3375e27d40961adc4040f809ba11a02fd57fc3f0 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 15 Mar 2022 12:05:25 +0000 Subject: [PATCH 10/72] Document (and doctest) builder(custom) field attribute --- derive_builder/src/lib.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/derive_builder/src/lib.rs b/derive_builder/src/lib.rs index cc671fc8..f772aff9 100644 --- a/derive_builder/src/lib.rs +++ b/derive_builder/src/lib.rs @@ -635,6 +635,32 @@ //! # } //! ``` //! +//! # Completely custom fields in the builder +//! +//! Instead of having an `Option`, you can have whatever type you like: +//! +//! ```rust +//! # #[macro_use] +//! # extern crate derive_builder; +//! #[derive(Debug, PartialEq, Default, Builder, Clone)] +//! #[builder(derive(Debug, PartialEq))] +//! struct Lorem { +//! #[builder(custom(type = "u32", build = "self.ipsum"), setter(into))] +//! ipsum: u32, +//! +//! #[builder(custom(type = "String", build = "()"))] +//! dolor: (), +//! } +//! +//! # fn main() { +//! let mut builder = LoremBuilder::default(); +//! builder.ipsum(42u16).dolor("sit".into()); +//! assert_eq!(builder, LoremBuilder { ipsum: 42, dolor: "sit".into() }); +//! let lorem = builder.build().unwrap(); +//! assert_eq!(lorem, Lorem { ipsum: 42, dolor: () }); +//! # } +//! ``` +//! //! # **`#![no_std]`** Support (on Nightly) //! //! You can activate support for `#![no_std]` by adding `#[builder(no_std)]` to your struct From b7ab2ed3190282616bff371967309339d3f1b2f4 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Mon, 14 Mar 2022 18:54:38 +0000 Subject: [PATCH 11/72] Make CustomConversion be an enum We are going to add another variant. No functional change. --- derive_builder_core/src/initializer.rs | 14 ++++++++++++-- derive_builder_core/src/lib.rs | 2 +- .../src/macro_options/darling_opts.rs | 6 +++--- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/derive_builder_core/src/initializer.rs b/derive_builder_core/src/initializer.rs index eb884964..68b4040f 100644 --- a/derive_builder_core/src/initializer.rs +++ b/derive_builder_core/src/initializer.rs @@ -62,7 +62,13 @@ pub struct Initializer<'a> { /// Method to use to to convert the builder's field to the target field /// /// For sub-builder fields, this will be `build` (or similar) - pub custom_conversion: Option<&'a BlockContents>, + pub custom_conversion: Option>, +} + +#[derive(Debug, Clone)] +pub enum CustomConversion<'a> { + /// Custom conversion is a block contents expression + Block(&'a BlockContents), } impl<'a> ToTokens for Initializer<'a> { @@ -80,7 +86,11 @@ impl<'a> ToTokens for Initializer<'a> { #default )); } else if let Some(conv) = &self.custom_conversion { - conv.to_tokens(tokens); + match conv { + CustomConversion::Block(conv) => { + conv.to_tokens(tokens); + }, + } } else { let match_some = self.match_some(); let match_none = self.match_none(); diff --git a/derive_builder_core/src/lib.rs b/derive_builder_core/src/lib.rs index 0d16e987..c8469cbb 100644 --- a/derive_builder_core/src/lib.rs +++ b/derive_builder_core/src/lib.rs @@ -52,7 +52,7 @@ use darling::FromDeriveInput; pub(crate) use default_expression::DefaultExpression; pub(crate) use deprecation_notes::DeprecationNotes; pub(crate) use doc_comment::doc_comment_from; -pub(crate) use initializer::Initializer; +pub(crate) use initializer::{CustomConversion, Initializer}; pub(crate) use options::{BuilderPattern, Each}; pub(crate) use setter::{wrap_expression_in_some, Setter}; pub(crate) use parsed_literal::ParsedLiteral; diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index 35844722..ab6bd536 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -11,7 +11,7 @@ use syn::{self, spanned::Spanned, Attribute, Generics, Ident, Path}; use crate::{ Builder, BuilderField, BuilderFieldType, BuilderPattern, DefaultExpression, DeprecationNotes, Each, Initializer, - BlockContents, ParsedLiteral, Setter, + BlockContents, ParsedLiteral, Setter, CustomConversion, }; /// `derive_builder` uses separate sibling keywords to represent @@ -776,9 +776,9 @@ impl<'a> FieldWithDefaults<'a> { } } - pub fn custom_conversion(&'a self) -> Option<&'a BlockContents> { + pub fn custom_conversion(&'a self) -> Option> { if let Some(custom) = &self.field.custom { - Some(&custom.build) + Some(CustomConversion::Block(&custom.build)) } else { None } From 5dcb274b315db27b54632c15e47d8a87c68d2973 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 16 Mar 2022 12:12:32 +0000 Subject: [PATCH 12/72] builder(custom) defaults to simply moving value into target --- derive_builder/src/lib.rs | 2 +- derive_builder_core/src/initializer.rs | 5 +++++ derive_builder_core/src/macro_options/darling_opts.rs | 8 ++++++-- 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/derive_builder/src/lib.rs b/derive_builder/src/lib.rs index f772aff9..d80667c5 100644 --- a/derive_builder/src/lib.rs +++ b/derive_builder/src/lib.rs @@ -645,7 +645,7 @@ //! #[derive(Debug, PartialEq, Default, Builder, Clone)] //! #[builder(derive(Debug, PartialEq))] //! struct Lorem { -//! #[builder(custom(type = "u32", build = "self.ipsum"), setter(into))] +//! #[builder(custom(type = "u32"), setter(into))] //! ipsum: u32, //! //! #[builder(custom(type = "String", build = "()"))] diff --git a/derive_builder_core/src/initializer.rs b/derive_builder_core/src/initializer.rs index 68b4040f..2421e9a4 100644 --- a/derive_builder_core/src/initializer.rs +++ b/derive_builder_core/src/initializer.rs @@ -69,6 +69,8 @@ pub struct Initializer<'a> { pub enum CustomConversion<'a> { /// Custom conversion is a block contents expression Block(&'a BlockContents), + /// Custom conversion is just to move the field from the builder + Move, } impl<'a> ToTokens for Initializer<'a> { @@ -90,6 +92,9 @@ impl<'a> ToTokens for Initializer<'a> { CustomConversion::Block(conv) => { conv.to_tokens(tokens); }, + CustomConversion::Move => { + tokens.append_all(quote!( self.#builder_field )) + }, } } else { let match_some = self.match_some(); diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index ab6bd536..cd67c4ec 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -302,7 +302,8 @@ pub struct Field { pub struct CustomField { #[darling(rename = "type")] ty: ParsedLiteral, - build: BlockContents, + #[darling(default)] + build: Option, } impl Field { @@ -778,7 +779,10 @@ impl<'a> FieldWithDefaults<'a> { pub fn custom_conversion(&'a self) -> Option> { if let Some(custom) = &self.field.custom { - Some(CustomConversion::Block(&custom.build)) + Some(match &custom.build { + Some(block) => CustomConversion::Block(block), + None => CustomConversion::Move, + }) } else { None } From b670413167e0d38cda65ba597aa591bdebfd64e1 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 16 Mar 2022 13:56:50 +0000 Subject: [PATCH 13/72] Enhance documentation (and doctest) for custom fields --- derive_builder/src/lib.rs | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/derive_builder/src/lib.rs b/derive_builder/src/lib.rs index d80667c5..c5968c92 100644 --- a/derive_builder/src/lib.rs +++ b/derive_builder/src/lib.rs @@ -650,17 +650,31 @@ //! //! #[builder(custom(type = "String", build = "()"))] //! dolor: (), +//! +//! #[builder(custom(type = "&'static str", build = "self.amet.parse()?"))] +//! amet: u32, //! } //! +//! impl From for LoremBuilderError { // ... +//! # fn from(e: std::num::ParseIntError) -> LoremBuilderError { +//! # e.to_string().into() +//! # } +//! # } +//! //! # fn main() { //! let mut builder = LoremBuilder::default(); -//! builder.ipsum(42u16).dolor("sit".into()); -//! assert_eq!(builder, LoremBuilder { ipsum: 42, dolor: "sit".into() }); +//! builder.ipsum(42u16).dolor("sit".into()).amet("12"); +//! assert_eq!(builder, LoremBuilder { ipsum: 42, dolor: "sit".into(), amet: "12" }); //! let lorem = builder.build().unwrap(); -//! assert_eq!(lorem, Lorem { ipsum: 42, dolor: () }); +//! assert_eq!(lorem, Lorem { ipsum: 42, dolor: (), amet: 12 }); //! # } //! ``` //! +//! The builder field type (`type =`) must implement `Default`. +//! +//! The argument to `build` must be a literal string containing Rust code for the contents of a block. +//! It may refer to the builder struct as `self`, use `?`, etc. +//! //! # **`#![no_std]`** Support (on Nightly) //! //! You can activate support for `#![no_std]` by adding `#[builder(no_std)]` to your struct From 4904204b2b64535de7d6146add1ae7906231c164 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 24 Mar 2022 18:51:34 +0000 Subject: [PATCH 14/72] Custom fields: Further error checking and testing --- derive_builder/tests/builder_field_custom.rs | 57 +++++++++++++++++++ .../compile-fail/builder_field_custom.rs | 10 ++++ .../compile-fail/builder_field_custom.stderr | 11 ++++ .../src/macro_options/darling_opts.rs | 26 ++++++++- 4 files changed, 101 insertions(+), 3 deletions(-) create mode 100644 derive_builder/tests/builder_field_custom.rs create mode 100644 derive_builder/tests/compile-fail/builder_field_custom.rs create mode 100644 derive_builder/tests/compile-fail/builder_field_custom.stderr diff --git a/derive_builder/tests/builder_field_custom.rs b/derive_builder/tests/builder_field_custom.rs new file mode 100644 index 00000000..e2941d1d --- /dev/null +++ b/derive_builder/tests/builder_field_custom.rs @@ -0,0 +1,57 @@ +#[macro_use] +extern crate pretty_assertions; +#[macro_use] +extern crate derive_builder; + +use std::num::ParseIntError; + +#[derive(Debug, PartialEq, Default, Builder, Clone)] +pub struct Lorem { + #[builder(custom(type = "Option", build = "self.ipsum.unwrap_or(42) + 1"))] + ipsum: usize, + + #[builder(custom(type = "String", build = "self.dolor.parse()?"))] + dolor: u32, +} + +impl From for LoremBuilderError { + fn from(e: ParseIntError) -> LoremBuilderError { + LoremBuilderError::ValidationError(e.to_string()) + } +} + +#[test] +fn custom_fields() { + let x = LoremBuilder::default() + .dolor("7".into()) + .build() + .unwrap(); + + assert_eq!( + x, + Lorem { + ipsum: 43, + dolor: 7, + } + ); + + let x = LoremBuilder::default() + .ipsum(Some(12)) + .dolor("66".into()) + .build() + .unwrap(); + + assert_eq!( + x, + Lorem { + ipsum: 13, + dolor: 66, + } + ); + + let x = LoremBuilder::default() + .build() + .unwrap_err() + .to_string(); + assert_eq!(x, "cannot parse integer from empty string"); +} diff --git a/derive_builder/tests/compile-fail/builder_field_custom.rs b/derive_builder/tests/compile-fail/builder_field_custom.rs new file mode 100644 index 00000000..783274f7 --- /dev/null +++ b/derive_builder/tests/compile-fail/builder_field_custom.rs @@ -0,0 +1,10 @@ +#[macro_use] +extern crate derive_builder; + +#[derive(Debug, PartialEq, Default, Builder, Clone)] +pub struct Lorem { + #[builder(default = "88", custom(type = "usize", build = "self.ipsum.unwrap_or_else(42) + 1"))] + ipsum: usize, +} + +fn main() {} diff --git a/derive_builder/tests/compile-fail/builder_field_custom.stderr b/derive_builder/tests/compile-fail/builder_field_custom.stderr new file mode 100644 index 00000000..6cd8c5f3 --- /dev/null +++ b/derive_builder/tests/compile-fail/builder_field_custom.stderr @@ -0,0 +1,11 @@ +error: Unexpected meta-item format `Specified both #[builder(default=)] and #[builder(custom(build=))] on same field, but this is not meaningful` + --> tests/compile-fail/builder_field_custom.rs:6:25 + | +6 | #[builder(default = "88", custom(type = "usize", build = "self.ipsum.unwrap_or_else(42) + 1"))] + | ^^^^ + +error: Unexpected meta-item format `Specified both #[builder(default=)] and #[builder(custom(build=))] on same field, but this is not meaningful` + --> tests/compile-fail/builder_field_custom.rs:6:62 + | +6 | #[builder(default = "88", custom(type = "usize", build = "self.ipsum.unwrap_or_else(42) + 1"))] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index cd67c4ec..ce543cff 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -252,7 +252,7 @@ fn field_setter(meta: &Meta) -> darling::Result { #[darling( attributes(builder), forward_attrs(doc, cfg, allow, builder_field_attr, builder_setter_attr), - and_then = "Self::unnest_attrs" + and_then = "Self::resolve" )] pub struct Field { ident: Option, @@ -314,8 +314,28 @@ impl Field { errors.finish() } - /// Populate `self.field_attrs` and `self.setter_attrs` by draining `self.attrs` - fn unnest_attrs(mut self) -> darling::Result { + /// Resolve and check (post-parsing) options which come from multiple darling options + /// + /// * Check that we don't have a custom field builder *and* a default value + /// * Populate `self.field_attrs` and `self.setter_attrs` by draining `self.attrs` + fn resolve(mut self) -> darling::Result { + // Perhaps at some future point there would be a way to specify the default value of the field in the builder. + // That ought possibly to be distinct from the default value of the field in the target. + // For now, reject this situation. + if let Field { + default: Some(default_spec), + custom: Some(CustomField { build: Some(build_spec), .. }), + .. + } = &self { + let m = || darling::Error::unsupported_format( + "Specified both #[builder(default=)] and #[builder(custom(build=))] on same field, but this is not meaningful", + ); + return Err(darling::Error::multiple(vec![ + m().with_span(default_spec), + m().with_span(build_spec), + ])); + }; + distribute_and_unnest_attrs( &mut self.attrs, &mut [ From dba50b78bc8f3fcff675006ada84a6a34d4ee47f Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Fri, 25 Mar 2022 10:19:11 +0000 Subject: [PATCH 15/72] Run rustfmt --- derive_builder/tests/builder_field_custom.rs | 10 ++-------- derive_builder_core/src/builder_field.rs | 12 +++++------- derive_builder_core/src/initializer.rs | 10 ++++------ derive_builder_core/src/lib.rs | 2 +- .../src/macro_options/darling_opts.rs | 19 +++++++++++++------ derive_builder_core/src/setter.rs | 2 +- 6 files changed, 26 insertions(+), 29 deletions(-) diff --git a/derive_builder/tests/builder_field_custom.rs b/derive_builder/tests/builder_field_custom.rs index e2941d1d..d937352e 100644 --- a/derive_builder/tests/builder_field_custom.rs +++ b/derive_builder/tests/builder_field_custom.rs @@ -22,10 +22,7 @@ impl From for LoremBuilderError { #[test] fn custom_fields() { - let x = LoremBuilder::default() - .dolor("7".into()) - .build() - .unwrap(); + let x = LoremBuilder::default().dolor("7".into()).build().unwrap(); assert_eq!( x, @@ -49,9 +46,6 @@ fn custom_fields() { } ); - let x = LoremBuilder::default() - .build() - .unwrap_err() - .to_string(); + let x = LoremBuilder::default().build().unwrap_err().to_string(); assert_eq!(x, "cannot parse integer from empty string"); } diff --git a/derive_builder_core/src/builder_field.rs b/derive_builder_core/src/builder_field.rs index 34aefb35..39be3a92 100644 --- a/derive_builder_core/src/builder_field.rs +++ b/derive_builder_core/src/builder_field.rs @@ -1,9 +1,9 @@ use std::borrow::Cow; +use crate::wrap_expression_in_some; use proc_macro2::TokenStream; use quote::{ToTokens, TokenStreamExt}; use syn; -use crate::wrap_expression_in_some; /// Field for the builder struct, implementing `quote::ToTokens`. /// @@ -57,7 +57,7 @@ pub enum BuilderFieldType<'a> { Precisely(&'a syn::Type), } -impl<'a> BuilderFieldType<'a> { +impl<'a> BuilderFieldType<'a> { /// Some call sites want the target field type pub fn target_type(&'a self) -> &'a syn::Type { match self { @@ -101,11 +101,9 @@ impl<'a> ToTokens for BuilderField<'a> { impl<'a> ToTokens for BuilderFieldType<'a> { fn to_tokens(&self, tokens: &mut TokenStream) { match self { - BuilderFieldType::Optional(ty) => { - tokens.append_all(quote!( - ::derive_builder::export::core::option::Option<#ty> - )) - }, + BuilderFieldType::Optional(ty) => tokens.append_all(quote!( + ::derive_builder::export::core::option::Option<#ty> + )), BuilderFieldType::Precisely(ty) => ty.to_tokens(tokens), } } diff --git a/derive_builder_core/src/initializer.rs b/derive_builder_core/src/initializer.rs index 2421e9a4..3a24a512 100644 --- a/derive_builder_core/src/initializer.rs +++ b/derive_builder_core/src/initializer.rs @@ -4,7 +4,7 @@ use syn; use BuilderPattern; use DEFAULT_STRUCT_NAME; -use crate::{DefaultExpression, BlockContents}; +use crate::{BlockContents, DefaultExpression}; /// Initializer for the target struct fields, implementing `quote::ToTokens`. /// @@ -82,7 +82,7 @@ impl<'a> ToTokens for Initializer<'a> { #struct_field: )); - if ! self.field_enabled { + if !self.field_enabled { let default = self.default(); tokens.append_all(quote!( #default @@ -91,10 +91,8 @@ impl<'a> ToTokens for Initializer<'a> { match conv { CustomConversion::Block(conv) => { conv.to_tokens(tokens); - }, - CustomConversion::Move => { - tokens.append_all(quote!( self.#builder_field )) - }, + } + CustomConversion::Move => tokens.append_all(quote!( self.#builder_field )), } } else { let match_some = self.match_some(); diff --git a/derive_builder_core/src/lib.rs b/derive_builder_core/src/lib.rs index c8469cbb..ff6fda65 100644 --- a/derive_builder_core/src/lib.rs +++ b/derive_builder_core/src/lib.rs @@ -54,8 +54,8 @@ pub(crate) use deprecation_notes::DeprecationNotes; pub(crate) use doc_comment::doc_comment_from; pub(crate) use initializer::{CustomConversion, Initializer}; pub(crate) use options::{BuilderPattern, Each}; -pub(crate) use setter::{wrap_expression_in_some, Setter}; pub(crate) use parsed_literal::ParsedLiteral; +pub(crate) use setter::{wrap_expression_in_some, Setter}; const DEFAULT_STRUCT_NAME: &str = "__default"; diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index ce543cff..9433b042 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -10,8 +10,8 @@ use syn::Meta; use syn::{self, spanned::Spanned, Attribute, Generics, Ident, Path}; use crate::{ - Builder, BuilderField, BuilderFieldType, BuilderPattern, DefaultExpression, DeprecationNotes, Each, Initializer, - BlockContents, ParsedLiteral, Setter, CustomConversion, + BlockContents, Builder, BuilderField, BuilderFieldType, BuilderPattern, CustomConversion, + DefaultExpression, DeprecationNotes, Each, Initializer, ParsedLiteral, Setter, }; /// `derive_builder` uses separate sibling keywords to represent @@ -324,12 +324,19 @@ impl Field { // For now, reject this situation. if let Field { default: Some(default_spec), - custom: Some(CustomField { build: Some(build_spec), .. }), + custom: + Some(CustomField { + build: Some(build_spec), + .. + }), .. - } = &self { - let m = || darling::Error::unsupported_format( + } = &self + { + let m = || { + darling::Error::unsupported_format( "Specified both #[builder(default=)] and #[builder(custom(build=))] on same field, but this is not meaningful", - ); + ) + }; return Err(darling::Error::multiple(vec![ m().with_span(default_spec), m().with_span(build_spec), diff --git a/derive_builder_core/src/setter.rs b/derive_builder_core/src/setter.rs index 594caa65..823e1413 100644 --- a/derive_builder_core/src/setter.rs +++ b/derive_builder_core/src/setter.rs @@ -149,7 +149,7 @@ impl<'a> ToTokens for Setter<'a> { let try_ty_params = quote!(>); let try_ident = syn::Ident::new(&format!("try_{}", ident), Span::call_site()); - let converted = self.field_type.wrap_some(quote!{converted}); + let converted = self.field_type.wrap_some(quote! {converted}); tokens.append_all(quote!( #(#attrs)* From 531cadd1b2d60150af848a882b24c8d49f84e1df Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 13 Apr 2022 10:39:25 +0100 Subject: [PATCH 16/72] Use ? operator in FieldWithDefaults::custom_conversion Co-authored-by: Ted Driggs --- .../src/macro_options/darling_opts.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index 9433b042..5eb33735 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -805,14 +805,10 @@ impl<'a> FieldWithDefaults<'a> { } pub fn custom_conversion(&'a self) -> Option> { - if let Some(custom) = &self.field.custom { - Some(match &custom.build { - Some(block) => CustomConversion::Block(block), - None => CustomConversion::Move, - }) - } else { - None - } + Some(match &self.field.custom?.build { + Some(block) => CustomConversion::Block(block), + None => CustomConversion::Move, + }) } pub fn pattern(&self) -> BuilderPattern { From a7d545e4852300911a60e33bff9779dc1a892068 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 13 Apr 2022 10:39:39 +0100 Subject: [PATCH 17/72] Make a comment more terse Co-authored-by: Ted Driggs --- derive_builder_core/src/builder_field.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/derive_builder_core/src/builder_field.rs b/derive_builder_core/src/builder_field.rs index 39be3a92..9223eb1a 100644 --- a/derive_builder_core/src/builder_field.rs +++ b/derive_builder_core/src/builder_field.rs @@ -48,7 +48,7 @@ pub struct BuilderField<'a> { pub attrs: &'a [syn::Attribute], } -/// The type of a field in the builder struct, implementing `quote::ToTokens` +/// The type of a field in the builder struct #[derive(Debug, Clone)] pub enum BuilderFieldType<'a> { /// The corresonding builder field will be `Option`. From 81d946a8c36edcdc7b39c128f54382c766e3ca8f Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 13 Apr 2022 10:41:14 +0100 Subject: [PATCH 18/72] Reword error for custom= and default= Co-authored-by: Ted Driggs --- derive_builder_core/src/macro_options/darling_opts.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index 5eb33735..a2e0d227 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -334,7 +334,7 @@ impl Field { { let m = || { darling::Error::unsupported_format( - "Specified both #[builder(default=)] and #[builder(custom(build=))] on same field, but this is not meaningful", + "#[builder(default)] and #[builder(custom(build="..."))] cannot be used together", ) }; return Err(darling::Error::multiple(vec![ From 73037408afba5828edc17eb7eb03d1daf9d77bd0 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 13 Apr 2022 10:49:40 +0100 Subject: [PATCH 19/72] fixup! Reword error for custom= and default= --- derive_builder/tests/compile-fail/builder_field_custom.stderr | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/derive_builder/tests/compile-fail/builder_field_custom.stderr b/derive_builder/tests/compile-fail/builder_field_custom.stderr index 6cd8c5f3..cfb4881e 100644 --- a/derive_builder/tests/compile-fail/builder_field_custom.stderr +++ b/derive_builder/tests/compile-fail/builder_field_custom.stderr @@ -1,10 +1,10 @@ -error: Unexpected meta-item format `Specified both #[builder(default=)] and #[builder(custom(build=))] on same field, but this is not meaningful` +error: Unexpected meta-item format `#[builder(default)] and #[builder(custom(build="..."))] cannot be used together` --> tests/compile-fail/builder_field_custom.rs:6:25 | 6 | #[builder(default = "88", custom(type = "usize", build = "self.ipsum.unwrap_or_else(42) + 1"))] | ^^^^ -error: Unexpected meta-item format `Specified both #[builder(default=)] and #[builder(custom(build=))] on same field, but this is not meaningful` +error: Unexpected meta-item format `#[builder(default)] and #[builder(custom(build="..."))] cannot be used together` --> tests/compile-fail/builder_field_custom.rs:6:62 | 6 | #[builder(default = "88", custom(type = "usize", build = "self.ipsum.unwrap_or_else(42) + 1"))] From 58a776f221b0513cc306251f9a7d410fd6b8ee5f Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 13 Apr 2022 10:49:46 +0100 Subject: [PATCH 20/72] fixup! Reword error for custom= and default= --- derive_builder_core/src/macro_options/darling_opts.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index a2e0d227..2f7baa12 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -334,7 +334,7 @@ impl Field { { let m = || { darling::Error::unsupported_format( - "#[builder(default)] and #[builder(custom(build="..."))] cannot be used together", + r#"#[builder(default)] and #[builder(custom(build="..."))] cannot be used together"#, ) }; return Err(darling::Error::multiple(vec![ From 713171e7e60c3f231c583b00526cc2c24db75b5d Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 13 Apr 2022 10:49:59 +0100 Subject: [PATCH 21/72] fixup! Use ? operator in FieldWithDefaults::custom_conversion --- derive_builder_core/src/macro_options/darling_opts.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index 2f7baa12..ce059d9a 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -805,7 +805,7 @@ impl<'a> FieldWithDefaults<'a> { } pub fn custom_conversion(&'a self) -> Option> { - Some(match &self.field.custom?.build { + Some(match &self.field.custom.as_ref()?.build { Some(block) => CustomConversion::Block(block), None => CustomConversion::Move, }) From a5d67d7f7e044ddbb7bb37d85bfafde0090f60c5 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 13 Apr 2022 10:52:20 +0100 Subject: [PATCH 22/72] Move impl<'a> ToTokens for BuilderFieldType<'a> As per https://github.com/colin-kiegel/rust-derive-builder/pull/245#discussion_r843042088 --- derive_builder_core/src/builder_field.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/derive_builder_core/src/builder_field.rs b/derive_builder_core/src/builder_field.rs index 9223eb1a..f3091c09 100644 --- a/derive_builder_core/src/builder_field.rs +++ b/derive_builder_core/src/builder_field.rs @@ -75,6 +75,17 @@ impl<'a> BuilderFieldType<'a> { } } +impl<'a> ToTokens for BuilderFieldType<'a> { + fn to_tokens(&self, tokens: &mut TokenStream) { + match self { + BuilderFieldType::Optional(ty) => tokens.append_all(quote!( + ::derive_builder::export::core::option::Option<#ty> + )), + BuilderFieldType::Precisely(ty) => ty.to_tokens(tokens), + } + } +} + impl<'a> ToTokens for BuilderField<'a> { fn to_tokens(&self, tokens: &mut TokenStream) { if self.field_enabled { @@ -98,17 +109,6 @@ impl<'a> ToTokens for BuilderField<'a> { } } -impl<'a> ToTokens for BuilderFieldType<'a> { - fn to_tokens(&self, tokens: &mut TokenStream) { - match self { - BuilderFieldType::Optional(ty) => tokens.append_all(quote!( - ::derive_builder::export::core::option::Option<#ty> - )), - BuilderFieldType::Precisely(ty) => ty.to_tokens(tokens), - } - } -} - impl<'a> BuilderField<'a> { /// Emits a struct field initializer that initializes the field to `Default::default`. pub fn default_initializer_tokens(&self) -> TokenStream { From 7ddbbb5bdd79fef5f4ccb2b9bbbf9bf50311193d Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 13 Apr 2022 10:54:19 +0100 Subject: [PATCH 23/72] Mark default_builder_field with cfg(test) Apropos https://github.com/colin-kiegel/rust-derive-builder/pull/245#discussion_r843042402 --- derive_builder_core/src/builder_field.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/derive_builder_core/src/builder_field.rs b/derive_builder_core/src/builder_field.rs index f3091c09..a6188fbf 100644 --- a/derive_builder_core/src/builder_field.rs +++ b/derive_builder_core/src/builder_field.rs @@ -119,6 +119,7 @@ impl<'a> BuilderField<'a> { /// Helper macro for unit tests. This is _only_ public in order to be accessible /// from doc-tests too. +#[cfg(test)] // This contains a Box::leak, so is suitable only for tests #[doc(hidden)] #[macro_export] macro_rules! default_builder_field { From 82c78e357253a7bb62c8b3d4205f43c3ded41a38 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 13 Apr 2022 11:01:43 +0100 Subject: [PATCH 24/72] Report conflicting field options error only on builder(custom) As per https://github.com/colin-kiegel/rust-derive-builder/pull/245#discussion_r843054513 --- .../tests/compile-fail/builder_field_custom.stderr | 6 ------ .../src/macro_options/darling_opts.rs | 14 ++++---------- 2 files changed, 4 insertions(+), 16 deletions(-) diff --git a/derive_builder/tests/compile-fail/builder_field_custom.stderr b/derive_builder/tests/compile-fail/builder_field_custom.stderr index cfb4881e..d98ef0f1 100644 --- a/derive_builder/tests/compile-fail/builder_field_custom.stderr +++ b/derive_builder/tests/compile-fail/builder_field_custom.stderr @@ -1,9 +1,3 @@ -error: Unexpected meta-item format `#[builder(default)] and #[builder(custom(build="..."))] cannot be used together` - --> tests/compile-fail/builder_field_custom.rs:6:25 - | -6 | #[builder(default = "88", custom(type = "usize", build = "self.ipsum.unwrap_or_else(42) + 1"))] - | ^^^^ - error: Unexpected meta-item format `#[builder(default)] and #[builder(custom(build="..."))] cannot be used together` --> tests/compile-fail/builder_field_custom.rs:6:62 | diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index ce059d9a..a77d3ffe 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -323,7 +323,7 @@ impl Field { // That ought possibly to be distinct from the default value of the field in the target. // For now, reject this situation. if let Field { - default: Some(default_spec), + default: Some(_), custom: Some(CustomField { build: Some(build_spec), @@ -332,15 +332,9 @@ impl Field { .. } = &self { - let m = || { - darling::Error::unsupported_format( - r#"#[builder(default)] and #[builder(custom(build="..."))] cannot be used together"#, - ) - }; - return Err(darling::Error::multiple(vec![ - m().with_span(default_spec), - m().with_span(build_spec), - ])); + return Err(darling::Error::unsupported_format( + r#"#[builder(default)] and #[builder(custom(build="..."))] cannot be used together"# + ).with_span(build_spec)) }; distribute_and_unnest_attrs( From a2cf02018628b96e30876b1b3dbdf9293f7492ed Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 13 Apr 2022 11:12:45 +0100 Subject: [PATCH 25/72] Rely on FromMeta impl for syn::Type in darling 0.13.3 As per https://github.com/colin-kiegel/rust-derive-builder/pull/245#discussion_r843055780 --- derive_builder_core/src/lib.rs | 1 - derive_builder_core/src/macro_options/darling_opts.rs | 6 +++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/derive_builder_core/src/lib.rs b/derive_builder_core/src/lib.rs index ff6fda65..fad8f643 100644 --- a/derive_builder_core/src/lib.rs +++ b/derive_builder_core/src/lib.rs @@ -54,7 +54,6 @@ pub(crate) use deprecation_notes::DeprecationNotes; pub(crate) use doc_comment::doc_comment_from; pub(crate) use initializer::{CustomConversion, Initializer}; pub(crate) use options::{BuilderPattern, Each}; -pub(crate) use parsed_literal::ParsedLiteral; pub(crate) use setter::{wrap_expression_in_some, Setter}; const DEFAULT_STRUCT_NAME: &str = "__default"; diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index a77d3ffe..701d3a76 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -11,7 +11,7 @@ use syn::{self, spanned::Spanned, Attribute, Generics, Ident, Path}; use crate::{ BlockContents, Builder, BuilderField, BuilderFieldType, BuilderPattern, CustomConversion, - DefaultExpression, DeprecationNotes, Each, Initializer, ParsedLiteral, Setter, + DefaultExpression, DeprecationNotes, Each, Initializer, Setter, }; /// `derive_builder` uses separate sibling keywords to represent @@ -301,7 +301,7 @@ pub struct Field { #[derive(Debug, Clone, FromMeta)] pub struct CustomField { #[darling(rename = "type")] - ty: ParsedLiteral, + ty: syn::Type, #[darling(default)] build: Option, } @@ -792,7 +792,7 @@ impl<'a> FieldWithDefaults<'a> { pub fn field_type(&'a self) -> BuilderFieldType<'a> { if let Some(custom) = self.field.custom.as_ref() { - BuilderFieldType::Precisely(&custom.ty.0) + BuilderFieldType::Precisely(&custom.ty) } else { BuilderFieldType::Optional(&self.field.ty) } From 13013b6202fcfd8cdc7725f86343f0b081f5bf45 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 13 Apr 2022 11:05:34 +0100 Subject: [PATCH 26/72] Revert "ParsedLiteral: New utility type for parsing syn items from attrs" This reverts commit 08923df20a834f22779cef565a6be8bab4b19cd2. --- derive_builder_core/src/lib.rs | 1 - derive_builder_core/src/parsed_literal.rs | 23 ----------------------- 2 files changed, 24 deletions(-) delete mode 100644 derive_builder_core/src/parsed_literal.rs diff --git a/derive_builder_core/src/lib.rs b/derive_builder_core/src/lib.rs index fad8f643..6168e146 100644 --- a/derive_builder_core/src/lib.rs +++ b/derive_builder_core/src/lib.rs @@ -41,7 +41,6 @@ mod doc_comment; mod initializer; mod macro_options; mod options; -mod parsed_literal; mod setter; pub(crate) use block::BlockContents; diff --git a/derive_builder_core/src/parsed_literal.rs b/derive_builder_core/src/parsed_literal.rs deleted file mode 100644 index 9714e48a..00000000 --- a/derive_builder_core/src/parsed_literal.rs +++ /dev/null @@ -1,23 +0,0 @@ -use proc_macro2::TokenStream; -use quote::ToTokens; -use syn; - -/// A wrapper for syn types which impl FromMeta, parsing them from a stirng literal -#[derive(Debug, Clone)] -pub struct ParsedLiteral(pub T); - -impl ToTokens for ParsedLiteral { - fn to_tokens(&self, tokens: &mut TokenStream) { - self.0.to_tokens(tokens) - } -} - -impl darling::FromMeta for ParsedLiteral { - fn from_value(value: &syn::Lit) -> darling::Result { - if let syn::Lit::Str(s) = value { - Ok(ParsedLiteral(s.parse()?)) - } else { - Err(darling::Error::unexpected_lit_type(value)) - } - } -} From ccc2e0dfe8f97490210ac4d6a22252c176710839 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 13 Apr 2022 11:16:38 +0100 Subject: [PATCH 27/72] Rename BuilderFieldType::Precise from Precisely As per https://github.com/colin-kiegel/rust-derive-builder/pull/245#discussion_r843032307 This may yet go away (or something), apropos further review comments. --- derive_builder_core/src/builder_field.rs | 8 ++++---- derive_builder_core/src/macro_options/darling_opts.rs | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/derive_builder_core/src/builder_field.rs b/derive_builder_core/src/builder_field.rs index a6188fbf..39328afc 100644 --- a/derive_builder_core/src/builder_field.rs +++ b/derive_builder_core/src/builder_field.rs @@ -54,7 +54,7 @@ pub enum BuilderFieldType<'a> { /// The corresonding builder field will be `Option`. Optional(&'a syn::Type), /// The corresponding builder field will be just this type - Precisely(&'a syn::Type), + Precise(&'a syn::Type), } impl<'a> BuilderFieldType<'a> { @@ -62,7 +62,7 @@ impl<'a> BuilderFieldType<'a> { pub fn target_type(&'a self) -> &'a syn::Type { match self { BuilderFieldType::Optional(ty) => ty, - BuilderFieldType::Precisely(ty) => ty, + BuilderFieldType::Precise(ty) => ty, } } @@ -70,7 +70,7 @@ impl<'a> BuilderFieldType<'a> { pub fn wrap_some(&'a self, bare_value: TokenStream) -> TokenStream { match self { BuilderFieldType::Optional(_) => wrap_expression_in_some(bare_value), - BuilderFieldType::Precisely(_) => bare_value, + BuilderFieldType::Precise(_) => bare_value, } } } @@ -81,7 +81,7 @@ impl<'a> ToTokens for BuilderFieldType<'a> { BuilderFieldType::Optional(ty) => tokens.append_all(quote!( ::derive_builder::export::core::option::Option<#ty> )), - BuilderFieldType::Precisely(ty) => ty.to_tokens(tokens), + BuilderFieldType::Precise(ty) => ty.to_tokens(tokens), } } } diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index 701d3a76..8e8a486e 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -792,7 +792,7 @@ impl<'a> FieldWithDefaults<'a> { pub fn field_type(&'a self) -> BuilderFieldType<'a> { if let Some(custom) = self.field.custom.as_ref() { - BuilderFieldType::Precisely(&custom.ty) + BuilderFieldType::Precise(&custom.ty) } else { BuilderFieldType::Optional(&self.field.ty) } From beaec832c6e50d75d232080d4e6a9838b4007ebb Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 13 Apr 2022 11:38:56 +0100 Subject: [PATCH 28/72] Replace BuilderField::field_enabled with BuilderFieldType::Phantom This was rather anomalous: field_enabled == false meant "use PhantomData"; true meant "use the BuilderFieldType". Visibility needs slightly special handling because we specially make the PhantomData non-visible. --- derive_builder_core/src/builder_field.rs | 50 +++++++++---------- .../src/macro_options/darling_opts.rs | 21 +++++--- 2 files changed, 37 insertions(+), 34 deletions(-) diff --git a/derive_builder_core/src/builder_field.rs b/derive_builder_core/src/builder_field.rs index 39328afc..bea3a240 100644 --- a/derive_builder_core/src/builder_field.rs +++ b/derive_builder_core/src/builder_field.rs @@ -36,12 +36,6 @@ pub struct BuilderField<'a> { pub field_ident: &'a syn::Ident, /// Type of the target field. pub field_type: BuilderFieldType<'a>, - /// Whether the builder implements a setter for this field. - /// - /// Note: We will fallback to `PhantomData` if the setter is disabled - /// to hack around issues with unused generic type parameters - at - /// least for now. - pub field_enabled: bool, /// Visibility of this builder field, e.g. `syn::Visibility::Public`. pub field_visibility: Cow<'a, syn::Visibility>, /// Attributes which will be attached to this builder field. @@ -55,6 +49,12 @@ pub enum BuilderFieldType<'a> { Optional(&'a syn::Type), /// The corresponding builder field will be just this type Precise(&'a syn::Type), + /// The corresponding builder field will be a PhantomData + /// + /// We do this if if the field is disabled + /// to hack around issues with unused generic type parameters - at + /// least for now. + Phantom(&'a syn::Type), } impl<'a> BuilderFieldType<'a> { @@ -63,6 +63,7 @@ impl<'a> BuilderFieldType<'a> { match self { BuilderFieldType::Optional(ty) => ty, BuilderFieldType::Precise(ty) => ty, + BuilderFieldType::Phantom(ty) => ty, } } @@ -71,6 +72,7 @@ impl<'a> BuilderFieldType<'a> { match self { BuilderFieldType::Optional(_) => wrap_expression_in_some(bare_value), BuilderFieldType::Precise(_) => bare_value, + BuilderFieldType::Phantom(_) => panic!("setter_enabled but BFT::PHantom"), } } } @@ -82,30 +84,23 @@ impl<'a> ToTokens for BuilderFieldType<'a> { ::derive_builder::export::core::option::Option<#ty> )), BuilderFieldType::Precise(ty) => ty.to_tokens(tokens), + BuilderFieldType::Phantom(ty) => tokens.append_all(quote!( + ::derive_builder::export::core::marker::PhantomData<#ty> + )), } } } impl<'a> ToTokens for BuilderField<'a> { fn to_tokens(&self, tokens: &mut TokenStream) { - if self.field_enabled { - let vis = &self.field_visibility; - let ident = self.field_ident; - let ty = &self.field_type; - let attrs = self.attrs; - - tokens.append_all(quote!( - #(#attrs)* #vis #ident: #ty, - )); - } else { - let ident = self.field_ident; - let ty = self.field_type.target_type(); - let attrs = self.attrs; - - tokens.append_all(quote!( - #(#attrs)* #ident: ::derive_builder::export::core::marker::PhantomData<#ty>, - )); - } + let ident = self.field_ident; + let vis = &self.field_visibility; + let ty = &self.field_type; + let attrs = self.attrs; + tokens.append_all(quote!( + #(#attrs)* #vis #ident: #ty, + )); + // K let ty = self.field_type.target_type(); } } @@ -127,7 +122,6 @@ macro_rules! default_builder_field { BuilderField { field_ident: &syn::Ident::new("foo", ::proc_macro2::Span::call_site()), field_type: BuilderFieldType::Optional(Box::leak(Box::new(parse_quote!(String)))), - field_enabled: true, field_visibility: ::std::borrow::Cow::Owned(parse_quote!(pub)), attrs: &[parse_quote!(#[some_attr])], } @@ -155,7 +149,11 @@ mod tests { #[test] fn setter_disabled() { let mut field = default_builder_field!(); - field.field_enabled = false; + field.field_visibility = syn::Visibility::Inherited; + field.field_type = match field.field_type { + BuilderFieldType::Optional(ty) => BuilderFieldType::Phantom(ty), + _ => panic!(), + }; assert_eq!( quote!(#field).to_string(), diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index 8e8a486e..1d8e719d 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -782,16 +782,22 @@ impl<'a> FieldWithDefaults<'a> { .expect("Tuple structs are not supported") } - pub fn field_vis(&self) -> Cow { - self.field - .field - .as_expressed_vis() - .or_else(|| self.parent.field.as_expressed_vis()) - .unwrap_or(Cow::Owned(syn::Visibility::Inherited)) + pub fn field_vis(&self) -> Visibility { + if ! self.field_enabled() { + Cow::Owned(syn::Visibility::Inherited) + } else { + self.field + .field + .as_expressed_vis() + .or_else(|| self.parent.field.as_expressed_vis()) + .unwrap_or(Cow::Owned(syn::Visibility::Inherited)) + } } pub fn field_type(&'a self) -> BuilderFieldType<'a> { - if let Some(custom) = self.field.custom.as_ref() { + if ! self.field_enabled() { + BuilderFieldType::Phantom(&self.field.ty) + } else if let Some(custom) = self.field.custom.as_ref() { BuilderFieldType::Precise(&custom.ty) } else { BuilderFieldType::Optional(&self.field.ty) @@ -864,7 +870,6 @@ impl<'a> FieldWithDefaults<'a> { BuilderField { field_ident: self.field_ident(), field_type: self.field_type(), - field_enabled: self.field_enabled(), field_visibility: self.field_vis(), attrs: &self.field.field_attrs, } From fda17081d8c55b778a20da2429a5af3afa2788c3 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 13 Apr 2022 13:50:10 +0100 Subject: [PATCH 29/72] Setter: Move field type handling together Pure code motion --- derive_builder_core/src/setter.rs | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/derive_builder_core/src/setter.rs b/derive_builder_core/src/setter.rs index 823e1413..ea4c1a9f 100644 --- a/derive_builder_core/src/setter.rs +++ b/derive_builder_core/src/setter.rs @@ -73,23 +73,12 @@ pub struct Setter<'a> { impl<'a> ToTokens for Setter<'a> { fn to_tokens(&self, tokens: &mut TokenStream) { if self.setter_enabled { - let field_type = self.field_type.target_type(); let pattern = self.pattern; let vis = &self.visibility; let field_ident = self.field_ident; let ident = &self.ident; let attrs = self.attrs; let deprecation_notes = self.deprecation_notes; - let (ty, stripped_option) = { - if self.strip_option { - match extract_type_from_option(field_type) { - Some(ty) => (ty, true), - None => (field_type, false), - } - } else { - (field_type, false) - } - }; let self_param: TokenStream; let return_ty: TokenStream; @@ -118,6 +107,19 @@ impl<'a> ToTokens for Setter<'a> { let param_ty: TokenStream; let mut into_value: TokenStream; + let field_type = self.field_type.target_type(); + + let (ty, stripped_option) = { + if self.strip_option { + match extract_type_from_option(field_type) { + Some(ty) => (ty, true), + None => (field_type, false), + } + } else { + (field_type, false) + } + }; + if self.generic_into { ty_params = quote!(>); param_ty = quote!(VALUE); From 22fe6ca0e2d556f72ebeb74792a70b2b5fdc50a6 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 13 Apr 2022 14:06:20 +0100 Subject: [PATCH 30/72] Abolish BuilderFieldType::target_type in favour of setter_type_info target_type was wrong; this is not necessarily the target field type: for PhantomData it isn't. At the one call site, this didn't matter, because it could never happen. wrap_expression_in_some was rather funky. Instead, use a pattern similar to strip_option, where we record in a bool whether to nest in an option. --- derive_builder_core/src/builder_field.rs | 25 ++++++++++-------------- derive_builder_core/src/lib.rs | 2 +- derive_builder_core/src/setter.rs | 14 ++++++++++--- 3 files changed, 22 insertions(+), 19 deletions(-) diff --git a/derive_builder_core/src/builder_field.rs b/derive_builder_core/src/builder_field.rs index bea3a240..579687b9 100644 --- a/derive_builder_core/src/builder_field.rs +++ b/derive_builder_core/src/builder_field.rs @@ -1,6 +1,5 @@ use std::borrow::Cow; -use crate::wrap_expression_in_some; use proc_macro2::TokenStream; use quote::{ToTokens, TokenStreamExt}; use syn; @@ -58,21 +57,17 @@ pub enum BuilderFieldType<'a> { } impl<'a> BuilderFieldType<'a> { - /// Some call sites want the target field type - pub fn target_type(&'a self) -> &'a syn::Type { - match self { - BuilderFieldType::Optional(ty) => ty, - BuilderFieldType::Precise(ty) => ty, - BuilderFieldType::Phantom(ty) => ty, - } - } - - /// Returns expression wrapping `bare_value` in Some, if appropriate - pub fn wrap_some(&'a self, bare_value: TokenStream) -> TokenStream { + /// Onbtain type information for the builder field setter + /// + /// Return value: + /// * `.0`: type of the argument to the setter function + /// (before application of `strip_option`, `generic_into`) + /// * `.1`: whether the builder field is `Option` rather than just `type` + pub fn setter_type_info(&'a self) -> (&'a syn::Type, bool) { match self { - BuilderFieldType::Optional(_) => wrap_expression_in_some(bare_value), - BuilderFieldType::Precise(_) => bare_value, - BuilderFieldType::Phantom(_) => panic!("setter_enabled but BFT::PHantom"), + BuilderFieldType::Optional(ty) => (ty, true), + BuilderFieldType::Precise(ty) => (ty, false), + BuilderFieldType::Phantom(_ty) => panic!("setter_enabled but BFT::PHantom"), } } } diff --git a/derive_builder_core/src/lib.rs b/derive_builder_core/src/lib.rs index 6168e146..c412753d 100644 --- a/derive_builder_core/src/lib.rs +++ b/derive_builder_core/src/lib.rs @@ -53,7 +53,7 @@ pub(crate) use deprecation_notes::DeprecationNotes; pub(crate) use doc_comment::doc_comment_from; pub(crate) use initializer::{CustomConversion, Initializer}; pub(crate) use options::{BuilderPattern, Each}; -pub(crate) use setter::{wrap_expression_in_some, Setter}; +pub(crate) use setter::Setter; const DEFAULT_STRUCT_NAME: &str = "__default"; diff --git a/derive_builder_core/src/setter.rs b/derive_builder_core/src/setter.rs index ea4c1a9f..793f7a0d 100644 --- a/derive_builder_core/src/setter.rs +++ b/derive_builder_core/src/setter.rs @@ -107,7 +107,7 @@ impl<'a> ToTokens for Setter<'a> { let param_ty: TokenStream; let mut into_value: TokenStream; - let field_type = self.field_type.target_type(); + let (field_type, builder_field_is_option) = self.field_type.setter_type_info(); let (ty, stripped_option) = { if self.strip_option { @@ -129,10 +129,14 @@ impl<'a> ToTokens for Setter<'a> { param_ty = quote!(#ty); into_value = quote!(value); } + // If both `stripped_option` and `builder_field_is_option`, the target field is `Option`, + // the builder field is `Option>`, and the setter takes `file_type`, so we must wrap it twice. if stripped_option { into_value = wrap_expression_in_some(into_value); } - into_value = self.field_type.wrap_some(into_value); + if builder_field_is_option { + into_value = wrap_expression_in_some(into_value); + } tokens.append_all(quote!( #(#attrs)* @@ -151,7 +155,11 @@ impl<'a> ToTokens for Setter<'a> { let try_ty_params = quote!(>); let try_ident = syn::Ident::new(&format!("try_{}", ident), Span::call_site()); - let converted = self.field_type.wrap_some(quote! {converted}); + + let mut converted = quote! {converted}; + if builder_field_is_option { + converted = wrap_expression_in_some(converted); + } tokens.append_all(quote!( #(#attrs)* From 89ca85ba2fb584407a6d44270336a5774d05aee2 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 13 Apr 2022 14:12:10 +0100 Subject: [PATCH 31/72] Correct comments for BuilderFieldType values This field became more than just the target field type in "Introduce BuilderFieldType enum", but I omitted to adjust the comments. --- derive_builder_core/src/builder_field.rs | 2 +- derive_builder_core/src/setter.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/derive_builder_core/src/builder_field.rs b/derive_builder_core/src/builder_field.rs index 579687b9..6279ead9 100644 --- a/derive_builder_core/src/builder_field.rs +++ b/derive_builder_core/src/builder_field.rs @@ -33,7 +33,7 @@ use syn; pub struct BuilderField<'a> { /// Name of the target field. pub field_ident: &'a syn::Ident, - /// Type of the target field. + /// Type of the builder field. pub field_type: BuilderFieldType<'a>, /// Visibility of this builder field, e.g. `syn::Visibility::Public`. pub field_visibility: Cow<'a, syn::Visibility>, diff --git a/derive_builder_core/src/setter.rs b/derive_builder_core/src/setter.rs index 793f7a0d..e8fdc0b5 100644 --- a/derive_builder_core/src/setter.rs +++ b/derive_builder_core/src/setter.rs @@ -55,7 +55,7 @@ pub struct Setter<'a> { pub ident: syn::Ident, /// Name of the target field. pub field_ident: &'a syn::Ident, - /// Type of the target field. + /// Type of the builder field. /// /// The corresonding builder field will be `Option`. pub field_type: BuilderFieldType<'a>, From 4c789d90695d6585b045faef30df8fc5ba55c550 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 13 Apr 2022 14:33:13 +0100 Subject: [PATCH 32/72] Prevent accidental loss of trailing comma by rearranging control flow --- derive_builder_core/src/initializer.rs | 53 +++++++++++++------------- 1 file changed, 26 insertions(+), 27 deletions(-) diff --git a/derive_builder_core/src/initializer.rs b/derive_builder_core/src/initializer.rs index 3a24a512..96c1b4e8 100644 --- a/derive_builder_core/src/initializer.rs +++ b/derive_builder_core/src/initializer.rs @@ -78,36 +78,35 @@ impl<'a> ToTokens for Initializer<'a> { let struct_field = &self.field_ident; let builder_field = &*struct_field; - tokens.append_all(quote!( - #struct_field: - )); - - if !self.field_enabled { - let default = self.default(); - tokens.append_all(quote!( - #default - )); - } else if let Some(conv) = &self.custom_conversion { - match conv { - CustomConversion::Block(conv) => { - conv.to_tokens(tokens); + // This structure prevents accidental failure to add the trailing `,` due to incautious `return` + let append_rhs = |tokens: &mut TokenStream| { + if !self.field_enabled { + let default = self.default(); + tokens.append_all(quote!( + #default + )); + } else if let Some(conv) = &self.custom_conversion { + match conv { + CustomConversion::Block(conv) => { + conv.to_tokens(tokens); + } + CustomConversion::Move => tokens.append_all(quote!( self.#builder_field )), } - CustomConversion::Move => tokens.append_all(quote!( self.#builder_field )), + } else { + let match_some = self.match_some(); + let match_none = self.match_none(); + tokens.append_all(quote!( + match self.#builder_field { + #match_some, + #match_none, + } + )); } - } else { - let match_some = self.match_some(); - let match_none = self.match_none(); - tokens.append_all(quote!( - match self.#builder_field { - #match_some, - #match_none, - } - )); - } + }; - tokens.append_all(quote!( - , - )); + tokens.append_all(quote!(#struct_field:)); + append_rhs(tokens); + tokens.append_all(quote!(,)); } } From c6b9ec23fa71a39a494f4459b464d3e1e2dc898e Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 13 Apr 2022 15:33:50 +0100 Subject: [PATCH 33/72] Run rustfmt --- derive_builder_core/src/macro_options/darling_opts.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index 1d8e719d..0baeaec8 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -334,7 +334,7 @@ impl Field { { return Err(darling::Error::unsupported_format( r#"#[builder(default)] and #[builder(custom(build="..."))] cannot be used together"# - ).with_span(build_spec)) + ).with_span(build_spec)); }; distribute_and_unnest_attrs( @@ -783,7 +783,7 @@ impl<'a> FieldWithDefaults<'a> { } pub fn field_vis(&self) -> Visibility { - if ! self.field_enabled() { + if !self.field_enabled() { Cow::Owned(syn::Visibility::Inherited) } else { self.field @@ -795,7 +795,7 @@ impl<'a> FieldWithDefaults<'a> { } pub fn field_type(&'a self) -> BuilderFieldType<'a> { - if ! self.field_enabled() { + if !self.field_enabled() { BuilderFieldType::Phantom(&self.field.ty) } else if let Some(custom) = self.field.custom.as_ref() { BuilderFieldType::Precise(&custom.ty) From 55d036c55973394fd4c93a572823ac20ec7c5eaa Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 14 Apr 2022 10:38:56 +0100 Subject: [PATCH 34/72] Fix typo in comment Co-authored-by: Ted Driggs --- derive_builder_core/src/builder_field.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/derive_builder_core/src/builder_field.rs b/derive_builder_core/src/builder_field.rs index 6279ead9..20530690 100644 --- a/derive_builder_core/src/builder_field.rs +++ b/derive_builder_core/src/builder_field.rs @@ -57,7 +57,7 @@ pub enum BuilderFieldType<'a> { } impl<'a> BuilderFieldType<'a> { - /// Onbtain type information for the builder field setter + /// Obtain type information for the builder field setter /// /// Return value: /// * `.0`: type of the argument to the setter function From 5a062bfd4e403fd13400b3294643a17968568b6f Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 14 Apr 2022 10:39:09 +0100 Subject: [PATCH 35/72] Change panic message Co-authored-by: Ted Driggs --- derive_builder_core/src/builder_field.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/derive_builder_core/src/builder_field.rs b/derive_builder_core/src/builder_field.rs index 20530690..968e9825 100644 --- a/derive_builder_core/src/builder_field.rs +++ b/derive_builder_core/src/builder_field.rs @@ -67,7 +67,7 @@ impl<'a> BuilderFieldType<'a> { match self { BuilderFieldType::Optional(ty) => (ty, true), BuilderFieldType::Precise(ty) => (ty, false), - BuilderFieldType::Phantom(_ty) => panic!("setter_enabled but BFT::PHantom"), + BuilderFieldType::Phantom(_ty) => panic!("phantom fields should never have setters"), } } } From fed087a16987e54b15da64ee568fcc6c39ec7c31 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 14 Apr 2022 10:41:05 +0100 Subject: [PATCH 36/72] Drop two unneeded #[darling(default)] annotations on Optional fields As per https://github.com/colin-kiegel/rust-derive-builder/pull/245#discussion_r849847273 https://github.com/colin-kiegel/rust-derive-builder/pull/245#discussion_r849847555 --- derive_builder_core/src/macro_options/darling_opts.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index 0baeaec8..d77c5444 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -287,7 +287,6 @@ pub struct Field { default: Option, try_setter: Flag, /// Custom builder field type and build method - #[darling(default)] custom: Option, #[darling(default)] field: FieldMeta, @@ -302,7 +301,6 @@ pub struct Field { pub struct CustomField { #[darling(rename = "type")] ty: syn::Type, - #[darling(default)] build: Option, } From 2f6355fcdc036e5552bf1924e9ef4c74030342e5 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 14 Apr 2022 10:43:31 +0100 Subject: [PATCH 37/72] Make wrap_expression_in_some not `pub` As per https://github.com/colin-kiegel/rust-derive-builder/pull/245#discussion_r849853837 --- derive_builder_core/src/setter.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/derive_builder_core/src/setter.rs b/derive_builder_core/src/setter.rs index e8fdc0b5..ef780d58 100644 --- a/derive_builder_core/src/setter.rs +++ b/derive_builder_core/src/setter.rs @@ -226,7 +226,7 @@ impl<'a> ToTokens for Setter<'a> { } /// Returns expression wrapping `bare_value` in `Some` -pub fn wrap_expression_in_some(bare_value: TokenStream) -> TokenStream { +fn wrap_expression_in_some(bare_value: TokenStream) -> TokenStream { quote!( ::derive_builder::export::core::option::Option::Some(#bare_value) ) } From c4d09bbcfabfd87b10b4392f12584be2cf063e6b Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 14 Apr 2022 10:47:47 +0100 Subject: [PATCH 38/72] Move BuilderFieldType out from in the middle of BuilderField As per https://github.com/colin-kiegel/rust-derive-builder/pull/245#discussion_r849845935 --- derive_builder_core/src/builder_field.rs | 62 ++++++++++++------------ 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/derive_builder_core/src/builder_field.rs b/derive_builder_core/src/builder_field.rs index 968e9825..f4e291d1 100644 --- a/derive_builder_core/src/builder_field.rs +++ b/derive_builder_core/src/builder_field.rs @@ -41,37 +41,6 @@ pub struct BuilderField<'a> { pub attrs: &'a [syn::Attribute], } -/// The type of a field in the builder struct -#[derive(Debug, Clone)] -pub enum BuilderFieldType<'a> { - /// The corresonding builder field will be `Option`. - Optional(&'a syn::Type), - /// The corresponding builder field will be just this type - Precise(&'a syn::Type), - /// The corresponding builder field will be a PhantomData - /// - /// We do this if if the field is disabled - /// to hack around issues with unused generic type parameters - at - /// least for now. - Phantom(&'a syn::Type), -} - -impl<'a> BuilderFieldType<'a> { - /// Obtain type information for the builder field setter - /// - /// Return value: - /// * `.0`: type of the argument to the setter function - /// (before application of `strip_option`, `generic_into`) - /// * `.1`: whether the builder field is `Option` rather than just `type` - pub fn setter_type_info(&'a self) -> (&'a syn::Type, bool) { - match self { - BuilderFieldType::Optional(ty) => (ty, true), - BuilderFieldType::Precise(ty) => (ty, false), - BuilderFieldType::Phantom(_ty) => panic!("phantom fields should never have setters"), - } - } -} - impl<'a> ToTokens for BuilderFieldType<'a> { fn to_tokens(&self, tokens: &mut TokenStream) { match self { @@ -123,6 +92,37 @@ macro_rules! default_builder_field { }}; } +/// The type of a field in the builder struct +#[derive(Debug, Clone)] +pub enum BuilderFieldType<'a> { + /// The corresonding builder field will be `Option`. + Optional(&'a syn::Type), + /// The corresponding builder field will be just this type + Precise(&'a syn::Type), + /// The corresponding builder field will be a PhantomData + /// + /// We do this if if the field is disabled + /// to hack around issues with unused generic type parameters - at + /// least for now. + Phantom(&'a syn::Type), +} + +impl<'a> BuilderFieldType<'a> { + /// Obtain type information for the builder field setter + /// + /// Return value: + /// * `.0`: type of the argument to the setter function + /// (before application of `strip_option`, `generic_into`) + /// * `.1`: whether the builder field is `Option` rather than just `type` + pub fn setter_type_info(&'a self) -> (&'a syn::Type, bool) { + match self { + BuilderFieldType::Optional(ty) => (ty, true), + BuilderFieldType::Precise(ty) => (ty, false), + BuilderFieldType::Phantom(_ty) => panic!("phantom fields should never have setters"), + } + } +} + #[cfg(test)] mod tests { #[allow(unused_imports)] From 75cfbfdb46094c770f484e0749f025b673a314b1 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 14 Apr 2022 11:00:47 +0100 Subject: [PATCH 39/72] Promote custom() builder field contents to per-field arguments Ie, abolish the nesting in custom(). Incidentally, this now makes it possible to specify `build` without `type`. As per https://github.com/colin-kiegel/rust-derive-builder/pull/245#discussion_r849846959 --- derive_builder/src/lib.rs | 8 ++-- derive_builder/tests/builder_field_custom.rs | 4 +- .../compile-fail/builder_field_custom.rs | 2 +- .../compile-fail/builder_field_custom.stderr | 8 ++-- .../src/macro_options/darling_opts.rs | 39 ++++++++----------- 5 files changed, 27 insertions(+), 34 deletions(-) diff --git a/derive_builder/src/lib.rs b/derive_builder/src/lib.rs index c5968c92..63c13faa 100644 --- a/derive_builder/src/lib.rs +++ b/derive_builder/src/lib.rs @@ -645,13 +645,13 @@ //! #[derive(Debug, PartialEq, Default, Builder, Clone)] //! #[builder(derive(Debug, PartialEq))] //! struct Lorem { -//! #[builder(custom(type = "u32"), setter(into))] +//! #[builder(type = "u32", setter(into))] //! ipsum: u32, //! -//! #[builder(custom(type = "String", build = "()"))] +//! #[builder(type = "String", build = "()")] //! dolor: (), //! -//! #[builder(custom(type = "&'static str", build = "self.amet.parse()?"))] +//! #[builder(type = "&'static str", build = "self.amet.parse()?")] //! amet: u32, //! } //! @@ -672,7 +672,7 @@ //! //! The builder field type (`type =`) must implement `Default`. //! -//! The argument to `build` must be a literal string containing Rust code for the contents of a block. +//! The argument to `build` must be a literal string containing Rust code for the contents of a block, which must evaluate to the type of the target field. //! It may refer to the builder struct as `self`, use `?`, etc. //! //! # **`#![no_std]`** Support (on Nightly) diff --git a/derive_builder/tests/builder_field_custom.rs b/derive_builder/tests/builder_field_custom.rs index d937352e..dbaa585a 100644 --- a/derive_builder/tests/builder_field_custom.rs +++ b/derive_builder/tests/builder_field_custom.rs @@ -7,10 +7,10 @@ use std::num::ParseIntError; #[derive(Debug, PartialEq, Default, Builder, Clone)] pub struct Lorem { - #[builder(custom(type = "Option", build = "self.ipsum.unwrap_or(42) + 1"))] + #[builder(type = "Option", build = "self.ipsum.unwrap_or(42) + 1")] ipsum: usize, - #[builder(custom(type = "String", build = "self.dolor.parse()?"))] + #[builder(type = "String", build = "self.dolor.parse()?")] dolor: u32, } diff --git a/derive_builder/tests/compile-fail/builder_field_custom.rs b/derive_builder/tests/compile-fail/builder_field_custom.rs index 783274f7..ba32223f 100644 --- a/derive_builder/tests/compile-fail/builder_field_custom.rs +++ b/derive_builder/tests/compile-fail/builder_field_custom.rs @@ -3,7 +3,7 @@ extern crate derive_builder; #[derive(Debug, PartialEq, Default, Builder, Clone)] pub struct Lorem { - #[builder(default = "88", custom(type = "usize", build = "self.ipsum.unwrap_or_else(42) + 1"))] + #[builder(default = "88", type = "usize", build = "self.ipsum.unwrap_or_else(42) + 1")] ipsum: usize, } diff --git a/derive_builder/tests/compile-fail/builder_field_custom.stderr b/derive_builder/tests/compile-fail/builder_field_custom.stderr index d98ef0f1..e559b970 100644 --- a/derive_builder/tests/compile-fail/builder_field_custom.stderr +++ b/derive_builder/tests/compile-fail/builder_field_custom.stderr @@ -1,5 +1,5 @@ -error: Unexpected meta-item format `#[builder(default)] and #[builder(custom(build="..."))] cannot be used together` - --> tests/compile-fail/builder_field_custom.rs:6:62 +error: Unexpected meta-item format `#[builder(default)] and #[builder(build="...")] cannot be used together` + --> tests/compile-fail/builder_field_custom.rs:6:55 | -6 | #[builder(default = "88", custom(type = "usize", build = "self.ipsum.unwrap_or_else(42) + 1"))] - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +6 | #[builder(default = "88", type = "usize", build = "self.ipsum.unwrap_or_else(42) + 1")] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index d77c5444..f7700d01 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -286,8 +286,11 @@ pub struct Field { /// This property only captures the first two, the third is computed in `FieldWithDefaults`. default: Option, try_setter: Flag, - /// Custom builder field type and build method - custom: Option, + /// Custom builder field type + #[darling(rename = "type")] + builder_type: Option, + /// Custom builder field method, for making target struct field value + build: Option, #[darling(default)] field: FieldMeta, #[darling(skip)] @@ -296,14 +299,6 @@ pub struct Field { setter_attrs: Vec, } -/// Options for the field-level `custom` property -#[derive(Debug, Clone, FromMeta)] -pub struct CustomField { - #[darling(rename = "type")] - ty: syn::Type, - build: Option, -} - impl Field { fn no_visibility_conflicts(&self) -> darling::Result<()> { let mut errors = Error::accumulator(); @@ -322,17 +317,14 @@ impl Field { // For now, reject this situation. if let Field { default: Some(_), - custom: - Some(CustomField { - build: Some(build_spec), - .. - }), + build: Some(custom_build), .. } = &self { return Err(darling::Error::unsupported_format( - r#"#[builder(default)] and #[builder(custom(build="..."))] cannot be used together"# - ).with_span(build_spec)); + r#"#[builder(default)] and #[builder(build="...")] cannot be used together"#, + ) + .with_span(custom_build)); }; distribute_and_unnest_attrs( @@ -795,18 +787,19 @@ impl<'a> FieldWithDefaults<'a> { pub fn field_type(&'a self) -> BuilderFieldType<'a> { if !self.field_enabled() { BuilderFieldType::Phantom(&self.field.ty) - } else if let Some(custom) = self.field.custom.as_ref() { - BuilderFieldType::Precise(&custom.ty) + } else if let Some(custom_ty) = self.field.builder_type.as_ref() { + BuilderFieldType::Precise(&custom_ty) } else { BuilderFieldType::Optional(&self.field.ty) } } pub fn custom_conversion(&'a self) -> Option> { - Some(match &self.field.custom.as_ref()?.build { - Some(block) => CustomConversion::Block(block), - None => CustomConversion::Move, - }) + match (&self.field.builder_type, &self.field.build) { + (_, Some(block)) => Some(CustomConversion::Block(block)), + (Some(_), None) => Some(CustomConversion::Move), + (None, None) => None, + } } pub fn pattern(&self) -> BuilderPattern { From 5777b049d0263f2299180094ea4cfdf0cf4ca9e6 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 14 Apr 2022 11:08:47 +0100 Subject: [PATCH 40/72] Replace Option with FieldConversion CustomConversion was only ever found in an Option. This is an antipattern. Instead, make the None be an variant. --- derive_builder_core/src/initializer.rs | 35 ++++++++++--------- derive_builder_core/src/lib.rs | 2 +- .../src/macro_options/darling_opts.rs | 14 ++++---- 3 files changed, 27 insertions(+), 24 deletions(-) diff --git a/derive_builder_core/src/initializer.rs b/derive_builder_core/src/initializer.rs index 96c1b4e8..d4cbaf06 100644 --- a/derive_builder_core/src/initializer.rs +++ b/derive_builder_core/src/initializer.rs @@ -62,11 +62,13 @@ pub struct Initializer<'a> { /// Method to use to to convert the builder's field to the target field /// /// For sub-builder fields, this will be `build` (or similar) - pub custom_conversion: Option>, + pub conversion: FieldConversion<'a>, } #[derive(Debug, Clone)] -pub enum CustomConversion<'a> { +pub enum FieldConversion<'a> { + /// Usual conversion: unwrap the Option from the builder, or (hope to) use a default value + OptionOrDefault, /// Custom conversion is a block contents expression Block(&'a BlockContents), /// Custom conversion is just to move the field from the builder @@ -85,22 +87,23 @@ impl<'a> ToTokens for Initializer<'a> { tokens.append_all(quote!( #default )); - } else if let Some(conv) = &self.custom_conversion { - match conv { - CustomConversion::Block(conv) => { + } else { + match &self.conversion { + FieldConversion::Block(conv) => { conv.to_tokens(tokens); } - CustomConversion::Move => tokens.append_all(quote!( self.#builder_field )), - } - } else { - let match_some = self.match_some(); - let match_none = self.match_none(); - tokens.append_all(quote!( - match self.#builder_field { - #match_some, - #match_none, + FieldConversion::Move => tokens.append_all(quote!( self.#builder_field )), + FieldConversion::OptionOrDefault => { + let match_some = self.match_some(); + let match_none = self.match_none(); + tokens.append_all(quote!( + match self.#builder_field { + #match_some, + #match_none, + } + )); } - )); + } } }; @@ -217,7 +220,7 @@ macro_rules! default_initializer { builder_pattern: BuilderPattern::Mutable, default_value: None, use_default_struct: false, - custom_conversion: None, + conversion: FieldConversion::OptionOrDefault, custom_error_type_span: None, } }; diff --git a/derive_builder_core/src/lib.rs b/derive_builder_core/src/lib.rs index c412753d..6d39dbf3 100644 --- a/derive_builder_core/src/lib.rs +++ b/derive_builder_core/src/lib.rs @@ -51,7 +51,7 @@ use darling::FromDeriveInput; pub(crate) use default_expression::DefaultExpression; pub(crate) use deprecation_notes::DeprecationNotes; pub(crate) use doc_comment::doc_comment_from; -pub(crate) use initializer::{CustomConversion, Initializer}; +pub(crate) use initializer::{FieldConversion, Initializer}; pub(crate) use options::{BuilderPattern, Each}; pub(crate) use setter::Setter; diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index f7700d01..1f8379f6 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -10,8 +10,8 @@ use syn::Meta; use syn::{self, spanned::Spanned, Attribute, Generics, Ident, Path}; use crate::{ - BlockContents, Builder, BuilderField, BuilderFieldType, BuilderPattern, CustomConversion, - DefaultExpression, DeprecationNotes, Each, Initializer, Setter, + BlockContents, Builder, BuilderField, BuilderFieldType, BuilderPattern, DefaultExpression, + DeprecationNotes, Each, FieldConversion, Initializer, Setter, }; /// `derive_builder` uses separate sibling keywords to represent @@ -794,11 +794,11 @@ impl<'a> FieldWithDefaults<'a> { } } - pub fn custom_conversion(&'a self) -> Option> { + pub fn conversion(&'a self) -> FieldConversion<'a> { match (&self.field.builder_type, &self.field.build) { - (_, Some(block)) => Some(CustomConversion::Block(block)), - (Some(_), None) => Some(CustomConversion::Move), - (None, None) => None, + (_, Some(block)) => FieldConversion::Block(block), + (Some(_), None) => FieldConversion::Move, + (None, None) => FieldConversion::OptionOrDefault, } } @@ -847,7 +847,7 @@ impl<'a> FieldWithDefaults<'a> { builder_pattern: self.pattern(), default_value: self.field.default.as_ref(), use_default_struct: self.use_parent_default(), - custom_conversion: self.custom_conversion(), + conversion: self.conversion(), custom_error_type_span: self .parent .build_fn From 848a12d0ce2e2e7e802b0d3b2017689528a6251a Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 14 Apr 2022 11:13:18 +0100 Subject: [PATCH 41/72] Move FieldConversion out from between Initializer and its impls Prompted by review comments requesting similar changes elsewhere. --- derive_builder_core/src/initializer.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/derive_builder_core/src/initializer.rs b/derive_builder_core/src/initializer.rs index d4cbaf06..7711669a 100644 --- a/derive_builder_core/src/initializer.rs +++ b/derive_builder_core/src/initializer.rs @@ -65,16 +65,6 @@ pub struct Initializer<'a> { pub conversion: FieldConversion<'a>, } -#[derive(Debug, Clone)] -pub enum FieldConversion<'a> { - /// Usual conversion: unwrap the Option from the builder, or (hope to) use a default value - OptionOrDefault, - /// Custom conversion is a block contents expression - Block(&'a BlockContents), - /// Custom conversion is just to move the field from the builder - Move, -} - impl<'a> ToTokens for Initializer<'a> { fn to_tokens(&self, tokens: &mut TokenStream) { let struct_field = &self.field_ident; @@ -152,6 +142,16 @@ impl<'a> Initializer<'a> { } } +#[derive(Debug, Clone)] +pub enum FieldConversion<'a> { + /// Usual conversion: unwrap the Option from the builder, or (hope to) use a default value + OptionOrDefault, + /// Custom conversion is a block contents expression + Block(&'a BlockContents), + /// Custom conversion is just to move the field from the builder + Move, +} + /// To be used inside of `#struct_field: match self.#builder_field { ... }` enum MatchNone<'a> { /// Inner value must be a valid Rust expression From 957532b405d1589049d43dbf6288a027f5564e26 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 14 Apr 2022 11:48:01 +0100 Subject: [PATCH 42/72] Adjust comment to explain why we have PhantomData for disabled fields And justify why we don't just put in Option. Prompted by https://github.com/colin-kiegel/rust-derive-builder/pull/245#discussion_r849858287 --- derive_builder_core/src/builder_field.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/derive_builder_core/src/builder_field.rs b/derive_builder_core/src/builder_field.rs index f4e291d1..6190cff0 100644 --- a/derive_builder_core/src/builder_field.rs +++ b/derive_builder_core/src/builder_field.rs @@ -101,9 +101,13 @@ pub enum BuilderFieldType<'a> { Precise(&'a syn::Type), /// The corresponding builder field will be a PhantomData /// - /// We do this if if the field is disabled - /// to hack around issues with unused generic type parameters - at - /// least for now. + /// We do this if if the field is disabled. We mustn't just completely omit the field from the builder: + /// if we did that, the builder might have unused generic parameters (since we copy the generics from + /// the target struct). Using a PhantomData of the original field type provides the right generic usage + /// (and the right variance). The alternative would be to give the user a way to separately control + /// the generics of the builder struct, which would be very awkward to use and complex to document. + /// We could just include the field anyway, as `Option`, but this is wasteful of space, and it + /// seems good to explicitly suppress the existence of a variable that won't be set or read. Phantom(&'a syn::Type), } From 342acde7ff3ef16e5bad21c6bc289ecde986f3b3 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 14 Apr 2022 11:58:18 +0100 Subject: [PATCH 43/72] Adjust comment explaining privacy of PhantomData for disabled fields Prompted by https://github.com/colin-kiegel/rust-derive-builder/pull/245#discussion_r849849852 --- derive_builder_core/src/macro_options/darling_opts.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index 1f8379f6..ee7c9479 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -774,6 +774,9 @@ impl<'a> FieldWithDefaults<'a> { pub fn field_vis(&self) -> Visibility { if !self.field_enabled() { + // Disabled fields become a PhantomData in the builder. We make that field non-public, + // even if the rest of the builder is public, since this field is just there to make + // sure the struct's generics are properly handled. Cow::Owned(syn::Visibility::Inherited) } else { self.field From f58d883ffdfc4d1111b11d88d436f69d8be5e6c9 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 14 Apr 2022 15:04:01 +0100 Subject: [PATCH 44/72] fixup! Move FieldConversion out from between Initializer and its impls Fix a clippy needless ref lint. --- derive_builder_core/src/macro_options/darling_opts.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index ee7c9479..ddf80f7d 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -791,7 +791,7 @@ impl<'a> FieldWithDefaults<'a> { if !self.field_enabled() { BuilderFieldType::Phantom(&self.field.ty) } else if let Some(custom_ty) = self.field.builder_type.as_ref() { - BuilderFieldType::Precise(&custom_ty) + BuilderFieldType::Precise(custom_ty) } else { BuilderFieldType::Optional(&self.field.ty) } From e24102e7d445af62928b4d472d0d55ba8d86e84e Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 19 Apr 2022 13:09:20 +0100 Subject: [PATCH 45/72] Improve doc comment about `into`. Co-authored-by: Ted Driggs --- derive_builder_core/src/builder_field.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/derive_builder_core/src/builder_field.rs b/derive_builder_core/src/builder_field.rs index 6190cff0..17cae361 100644 --- a/derive_builder_core/src/builder_field.rs +++ b/derive_builder_core/src/builder_field.rs @@ -116,7 +116,7 @@ impl<'a> BuilderFieldType<'a> { /// /// Return value: /// * `.0`: type of the argument to the setter function - /// (before application of `strip_option`, `generic_into`) + /// (before application of `strip_option`, `into`) /// * `.1`: whether the builder field is `Option` rather than just `type` pub fn setter_type_info(&'a self) -> (&'a syn::Type, bool) { match self { From f7b57f83db0ef388c80369ebac9a2bca19bd3ca3 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 19 Apr 2022 13:10:11 +0100 Subject: [PATCH 46/72] Genericise wrap_expression_in_some Co-authored-by: Ted Driggs --- derive_builder_core/src/setter.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/derive_builder_core/src/setter.rs b/derive_builder_core/src/setter.rs index ef780d58..5728e659 100644 --- a/derive_builder_core/src/setter.rs +++ b/derive_builder_core/src/setter.rs @@ -226,7 +226,7 @@ impl<'a> ToTokens for Setter<'a> { } /// Returns expression wrapping `bare_value` in `Some` -fn wrap_expression_in_some(bare_value: TokenStream) -> TokenStream { +fn wrap_expression_in_some(bare_value: impl ToTokens) -> TokenStream { quote!( ::derive_builder::export::core::option::Option::Some(#bare_value) ) } From 27d338c38088aeb6b061160963dbd9a0c1f2431b Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 19 Apr 2022 13:05:05 +0100 Subject: [PATCH 47/72] Adjust comment about conflicting default and custom fields Text more-or-less from https://github.com/colin-kiegel/rust-derive-builder/pull/245#discussion_r850499891 --- derive_builder_core/src/macro_options/darling_opts.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index ddf80f7d..533f9349 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -312,9 +312,9 @@ impl Field { /// * Check that we don't have a custom field builder *and* a default value /// * Populate `self.field_attrs` and `self.setter_attrs` by draining `self.attrs` fn resolve(mut self) -> darling::Result { - // Perhaps at some future point there would be a way to specify the default value of the field in the builder. - // That ought possibly to be distinct from the default value of the field in the target. - // For now, reject this situation. + /// `field.build` is stronger than `default`, as it contains both instructions on how to + /// deal with a missing value and conversions to do on the value during target type + /// construction. Because default will not be used, we disallow it. if let Field { default: Some(_), build: Some(custom_build), From ba5c46213a0a9a0087b760dd18beac8eb1483c87 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 19 Apr 2022 13:11:53 +0100 Subject: [PATCH 48/72] fixup! Adjust comment about conflicting default and custom fields --- derive_builder_core/src/macro_options/darling_opts.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index 533f9349..3abac6b6 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -312,9 +312,9 @@ impl Field { /// * Check that we don't have a custom field builder *and* a default value /// * Populate `self.field_attrs` and `self.setter_attrs` by draining `self.attrs` fn resolve(mut self) -> darling::Result { - /// `field.build` is stronger than `default`, as it contains both instructions on how to - /// deal with a missing value and conversions to do on the value during target type - /// construction. Because default will not be used, we disallow it. + // `field.build` is stronger than `default`, as it contains both instructions on how to + // deal with a missing value and conversions to do on the value during target type + // construction. Because default will not be used, we disallow it. if let Field { default: Some(_), build: Some(custom_build), From c81b9a81e68318039c88f146013ac120c38e6f77 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 19 Apr 2022 13:18:43 +0100 Subject: [PATCH 49/72] Move custom type and build_method into FieldMeta As per https://github.com/colin-kiegel/rust-derive-builder/pull/245#discussion_r850497269 --- derive_builder/src/lib.rs | 6 +++--- derive_builder/tests/builder_field_custom.rs | 4 ++-- .../tests/compile-fail/builder_field_custom.rs | 2 +- .../compile-fail/builder_field_custom.stderr | 6 +++--- .../src/macro_options/darling_opts.rs | 16 ++++++++-------- 5 files changed, 17 insertions(+), 17 deletions(-) diff --git a/derive_builder/src/lib.rs b/derive_builder/src/lib.rs index 63c13faa..bc3aaf4b 100644 --- a/derive_builder/src/lib.rs +++ b/derive_builder/src/lib.rs @@ -645,13 +645,13 @@ //! #[derive(Debug, PartialEq, Default, Builder, Clone)] //! #[builder(derive(Debug, PartialEq))] //! struct Lorem { -//! #[builder(type = "u32", setter(into))] +//! #[builder(setter(into), field(type = "u32"))] //! ipsum: u32, //! -//! #[builder(type = "String", build = "()")] +//! #[builder(field(type = "String", build = "()"))] //! dolor: (), //! -//! #[builder(type = "&'static str", build = "self.amet.parse()?")] +//! #[builder(field(type = "&'static str", build = "self.amet.parse()?"))] //! amet: u32, //! } //! diff --git a/derive_builder/tests/builder_field_custom.rs b/derive_builder/tests/builder_field_custom.rs index dbaa585a..7afbdea4 100644 --- a/derive_builder/tests/builder_field_custom.rs +++ b/derive_builder/tests/builder_field_custom.rs @@ -7,10 +7,10 @@ use std::num::ParseIntError; #[derive(Debug, PartialEq, Default, Builder, Clone)] pub struct Lorem { - #[builder(type = "Option", build = "self.ipsum.unwrap_or(42) + 1")] + #[builder(field(type = "Option", build = "self.ipsum.unwrap_or(42) + 1"))] ipsum: usize, - #[builder(type = "String", build = "self.dolor.parse()?")] + #[builder(field(type = "String", build = "self.dolor.parse()?"))] dolor: u32, } diff --git a/derive_builder/tests/compile-fail/builder_field_custom.rs b/derive_builder/tests/compile-fail/builder_field_custom.rs index ba32223f..4d2f05ce 100644 --- a/derive_builder/tests/compile-fail/builder_field_custom.rs +++ b/derive_builder/tests/compile-fail/builder_field_custom.rs @@ -3,7 +3,7 @@ extern crate derive_builder; #[derive(Debug, PartialEq, Default, Builder, Clone)] pub struct Lorem { - #[builder(default = "88", type = "usize", build = "self.ipsum.unwrap_or_else(42) + 1")] + #[builder(default = "88", field(type = "usize", build = "self.ipsum.unwrap_or_else(42) + 1"))] ipsum: usize, } diff --git a/derive_builder/tests/compile-fail/builder_field_custom.stderr b/derive_builder/tests/compile-fail/builder_field_custom.stderr index e559b970..4ba6403c 100644 --- a/derive_builder/tests/compile-fail/builder_field_custom.stderr +++ b/derive_builder/tests/compile-fail/builder_field_custom.stderr @@ -1,5 +1,5 @@ error: Unexpected meta-item format `#[builder(default)] and #[builder(build="...")] cannot be used together` - --> tests/compile-fail/builder_field_custom.rs:6:55 + --> tests/compile-fail/builder_field_custom.rs:6:61 | -6 | #[builder(default = "88", type = "usize", build = "self.ipsum.unwrap_or_else(42) + 1")] - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +6 | #[builder(default = "88", field(type = "usize", build = "self.ipsum.unwrap_or_else(42) + 1"))] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index 3abac6b6..0ab64aac 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -131,6 +131,11 @@ pub struct FieldMeta { public: Flag, private: Flag, vis: Option, + /// Custom builder field type + #[darling(rename = "type")] + builder_type: Option, + /// Custom builder field method, for making target struct field value + build: Option, } impl Visibility for FieldMeta { @@ -286,11 +291,6 @@ pub struct Field { /// This property only captures the first two, the third is computed in `FieldWithDefaults`. default: Option, try_setter: Flag, - /// Custom builder field type - #[darling(rename = "type")] - builder_type: Option, - /// Custom builder field method, for making target struct field value - build: Option, #[darling(default)] field: FieldMeta, #[darling(skip)] @@ -317,7 +317,7 @@ impl Field { // construction. Because default will not be used, we disallow it. if let Field { default: Some(_), - build: Some(custom_build), + field: FieldMeta { build: Some(custom_build), .. }, .. } = &self { @@ -790,7 +790,7 @@ impl<'a> FieldWithDefaults<'a> { pub fn field_type(&'a self) -> BuilderFieldType<'a> { if !self.field_enabled() { BuilderFieldType::Phantom(&self.field.ty) - } else if let Some(custom_ty) = self.field.builder_type.as_ref() { + } else if let Some(custom_ty) = self.field.field.builder_type.as_ref() { BuilderFieldType::Precise(custom_ty) } else { BuilderFieldType::Optional(&self.field.ty) @@ -798,7 +798,7 @@ impl<'a> FieldWithDefaults<'a> { } pub fn conversion(&'a self) -> FieldConversion<'a> { - match (&self.field.builder_type, &self.field.build) { + match (&self.field.field.builder_type, &self.field.field.build) { (_, Some(block)) => FieldConversion::Block(block), (Some(_), None) => FieldConversion::Move, (None, None) => FieldConversion::OptionOrDefault, From 02f1b73603b8d91e4ca63a32a53a1554d2e24ec0 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 19 Apr 2022 13:34:49 +0100 Subject: [PATCH 50/72] Default vs builder spec conflict: Use span of "default" As per https://github.com/colin-kiegel/rust-derive-builder/pull/245#discussion_r850501845 --- .../tests/compile-fail/builder_field_custom.stderr | 4 ++-- derive_builder_core/src/macro_options/darling_opts.rs | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/derive_builder/tests/compile-fail/builder_field_custom.stderr b/derive_builder/tests/compile-fail/builder_field_custom.stderr index 4ba6403c..77aba930 100644 --- a/derive_builder/tests/compile-fail/builder_field_custom.stderr +++ b/derive_builder/tests/compile-fail/builder_field_custom.stderr @@ -1,5 +1,5 @@ error: Unexpected meta-item format `#[builder(default)] and #[builder(build="...")] cannot be used together` - --> tests/compile-fail/builder_field_custom.rs:6:61 + --> tests/compile-fail/builder_field_custom.rs:6:25 | 6 | #[builder(default = "88", field(type = "usize", build = "self.ipsum.unwrap_or_else(42) + 1"))] - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^ diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index 0ab64aac..8584546e 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -316,15 +316,15 @@ impl Field { // deal with a missing value and conversions to do on the value during target type // construction. Because default will not be used, we disallow it. if let Field { - default: Some(_), - field: FieldMeta { build: Some(custom_build), .. }, + default: Some(field_default), + field: FieldMeta { build: Some(_custom_build), .. }, .. } = &self { return Err(darling::Error::unsupported_format( r#"#[builder(default)] and #[builder(build="...")] cannot be used together"#, ) - .with_span(custom_build)); + .with_span(field_default)); }; distribute_and_unnest_attrs( From 0f1b0d6885ea6dc3bb7a497c79b9c4de8bd97138 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 19 Apr 2022 14:38:33 +0100 Subject: [PATCH 51/72] Fix up semantic conflicts with #245 These changes are now needed for things to compile. This makes this branch less of a good story, but this seems like a plausible way to handle the situation. --- derive_builder_core/src/builder_field.rs | 2 +- derive_builder_core/src/macro_options/darling_opts.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/derive_builder_core/src/builder_field.rs b/derive_builder_core/src/builder_field.rs index 17cae361..65d79b8b 100644 --- a/derive_builder_core/src/builder_field.rs +++ b/derive_builder_core/src/builder_field.rs @@ -148,7 +148,7 @@ mod tests { #[test] fn setter_disabled() { let mut field = default_builder_field!(); - field.field_visibility = syn::Visibility::Inherited; + field.field_visibility = Cow::Owned(syn::Visibility::Inherited); field.field_type = match field.field_type { BuilderFieldType::Optional(ty) => BuilderFieldType::Phantom(ty), _ => panic!(), diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index 8584546e..e7f1abc8 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -772,7 +772,7 @@ impl<'a> FieldWithDefaults<'a> { .expect("Tuple structs are not supported") } - pub fn field_vis(&self) -> Visibility { + pub fn field_vis(&self) -> Cow { if !self.field_enabled() { // Disabled fields become a PhantomData in the builder. We make that field non-public, // even if the rest of the builder is public, since this field is just there to make From ba02e8b71c06082c24dab38fcb5dea17a347390d Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 19 Apr 2022 14:45:15 +0100 Subject: [PATCH 52/72] Separate StructLevelFieldMeta from FieldLevelFieldMeta As per https://github.com/colin-kiegel/rust-derive-builder/pull/245#discussion_r853023050 --- .../src/macro_options/darling_opts.rs | 36 ++++++++++++++++--- 1 file changed, 31 insertions(+), 5 deletions(-) diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index e7f1abc8..de4ae0ee 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -127,7 +127,33 @@ impl Visibility for BuildFn { /// Contents of the `field` meta in `builder` attributes. #[derive(Debug, Clone, Default, FromMeta)] -pub struct FieldMeta { +pub struct StructLevelFieldMeta { + public: Flag, + private: Flag, + vis: Option, +} + +impl Visibility for StructLevelFieldMeta { + fn public(&self) -> &Flag { + &self.public + } + + fn private(&self) -> &Flag { + &self.private + } + + fn explicit(&self) -> Option<&syn::Visibility> { + self.vis.as_ref() + } +} + +/// Contents of the `field` meta in `builder` attributes. +// +// TODO this struct and its Visibility impl duplicate code from StructLevelFieldMeta +// Ideally they would be combined somehow, but without adversely affecting diagnostics +// from darling, etc. +#[derive(Debug, Clone, Default, FromMeta)] +pub struct FieldLevelFieldMeta { public: Flag, private: Flag, vis: Option, @@ -138,7 +164,7 @@ pub struct FieldMeta { build: Option, } -impl Visibility for FieldMeta { +impl Visibility for FieldLevelFieldMeta { fn public(&self) -> &Flag { &self.public } @@ -292,7 +318,7 @@ pub struct Field { default: Option, try_setter: Flag, #[darling(default)] - field: FieldMeta, + field: FieldLevelFieldMeta, #[darling(skip)] field_attrs: Vec, #[darling(skip)] @@ -317,7 +343,7 @@ impl Field { // construction. Because default will not be used, we disallow it. if let Field { default: Some(field_default), - field: FieldMeta { build: Some(_custom_build), .. }, + field: FieldLevelFieldMeta { build: Some(_custom_build), .. }, .. } = &self { @@ -527,7 +553,7 @@ pub struct Options { try_setter: Flag, #[darling(default)] - field: FieldMeta, + field: StructLevelFieldMeta, #[darling(skip, default)] deprecation_notes: DeprecationNotes, From 2ef8ef2d502c6321806507c806645f6c96782780 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 19 Apr 2022 15:08:51 +0100 Subject: [PATCH 53/72] fixup! Separate StructLevelFieldMeta from FieldLevelFieldMeta rustfmt --- derive_builder_core/src/macro_options/darling_opts.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index de4ae0ee..56dcbe15 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -343,7 +343,11 @@ impl Field { // construction. Because default will not be used, we disallow it. if let Field { default: Some(field_default), - field: FieldLevelFieldMeta { build: Some(_custom_build), .. }, + field: + FieldLevelFieldMeta { + build: Some(_custom_build), + .. + }, .. } = &self { From 21069d8f8d63e39d689c50a9c5c8f683abd2a4a2 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 19 Apr 2022 15:08:13 +0100 Subject: [PATCH 54/72] Honour field-level explicit visibility even for disabled Phantom As per https://github.com/colin-kiegel/rust-derive-builder/pull/245#discussion_r849849852 --- .../src/macro_options/darling_opts.rs | 23 +++++++++---------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index 56dcbe15..d5f5b4db 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -803,18 +803,17 @@ impl<'a> FieldWithDefaults<'a> { } pub fn field_vis(&self) -> Cow { - if !self.field_enabled() { - // Disabled fields become a PhantomData in the builder. We make that field non-public, - // even if the rest of the builder is public, since this field is just there to make - // sure the struct's generics are properly handled. - Cow::Owned(syn::Visibility::Inherited) - } else { - self.field - .field - .as_expressed_vis() - .or_else(|| self.parent.field.as_expressed_vis()) - .unwrap_or(Cow::Owned(syn::Visibility::Inherited)) - } + self.field + .field + .as_expressed_vis() + .or_else( + // Disabled fields become a PhantomData in the builder. We make that field + // non-public, even if the rest of the builder is public, since this field is just + // there to make sure the struct's generics are properly handled. + || (!self.field_enabled()).then(|| Cow::Owned(syn::Visibility::Inherited)), + ) + .or_else(|| self.parent.field.as_expressed_vis()) + .unwrap_or(Cow::Owned(syn::Visibility::Inherited)) } pub fn field_type(&'a self) -> BuilderFieldType<'a> { From 6e984600662f0857b35a8bc8509c91b412157131 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 19 Apr 2022 16:17:40 +0100 Subject: [PATCH 55/72] Test setter(into) along with builder(field(type)) As per https://github.com/colin-kiegel/rust-derive-builder/pull/245#discussion_r853120803 --- derive_builder/tests/builder_field_custom.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/derive_builder/tests/builder_field_custom.rs b/derive_builder/tests/builder_field_custom.rs index 7afbdea4..35e5e8ec 100644 --- a/derive_builder/tests/builder_field_custom.rs +++ b/derive_builder/tests/builder_field_custom.rs @@ -10,7 +10,7 @@ pub struct Lorem { #[builder(field(type = "Option", build = "self.ipsum.unwrap_or(42) + 1"))] ipsum: usize, - #[builder(field(type = "String", build = "self.dolor.parse()?"))] + #[builder(setter(into), field(type = "String", build = "self.dolor.parse()?"))] dolor: u32, } @@ -22,7 +22,7 @@ impl From for LoremBuilderError { #[test] fn custom_fields() { - let x = LoremBuilder::default().dolor("7".into()).build().unwrap(); + let x = LoremBuilder::default().dolor("7").build().unwrap(); assert_eq!( x, @@ -34,7 +34,7 @@ fn custom_fields() { let x = LoremBuilder::default() .ipsum(Some(12)) - .dolor("66".into()) + .dolor("66") .build() .unwrap(); From 0605c9e15f6a75b4c1bfde0051f8bf8ca1e59576 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 19 Apr 2022 16:20:03 +0100 Subject: [PATCH 56/72] Fix compile fail test of defualt + field(build) Make the test one which compiles if the default= is removed. As per https://github.com/colin-kiegel/rust-derive-builder/pull/245#discussion_r853122193 --- derive_builder/tests/compile-fail/builder_field_custom.rs | 2 +- derive_builder/tests/compile-fail/builder_field_custom.stderr | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/derive_builder/tests/compile-fail/builder_field_custom.rs b/derive_builder/tests/compile-fail/builder_field_custom.rs index 4d2f05ce..1d5c5ade 100644 --- a/derive_builder/tests/compile-fail/builder_field_custom.rs +++ b/derive_builder/tests/compile-fail/builder_field_custom.rs @@ -3,7 +3,7 @@ extern crate derive_builder; #[derive(Debug, PartialEq, Default, Builder, Clone)] pub struct Lorem { - #[builder(default = "88", field(type = "usize", build = "self.ipsum.unwrap_or_else(42) + 1"))] + #[builder(default = "88", field(type = "usize", build = "self.ipsum + 42"))] ipsum: usize, } diff --git a/derive_builder/tests/compile-fail/builder_field_custom.stderr b/derive_builder/tests/compile-fail/builder_field_custom.stderr index 77aba930..6f847386 100644 --- a/derive_builder/tests/compile-fail/builder_field_custom.stderr +++ b/derive_builder/tests/compile-fail/builder_field_custom.stderr @@ -1,5 +1,5 @@ error: Unexpected meta-item format `#[builder(default)] and #[builder(build="...")] cannot be used together` --> tests/compile-fail/builder_field_custom.rs:6:25 | -6 | #[builder(default = "88", field(type = "usize", build = "self.ipsum.unwrap_or_else(42) + 1"))] +6 | #[builder(default = "88", field(type = "usize", build = "self.ipsum + 42"))] | ^^^^ From f8da6a0a69aabab9c710e13379b54cba3bac87ea Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 19 Apr 2022 16:21:44 +0100 Subject: [PATCH 57/72] Move trait impl to its proper place next to other impls on same type As per https://github.com/colin-kiegel/rust-derive-builder/pull/245#discussion_r853123880 --- derive_builder_core/src/builder_field.rs | 28 ++++++++++++------------ 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/derive_builder_core/src/builder_field.rs b/derive_builder_core/src/builder_field.rs index 65d79b8b..e849a28b 100644 --- a/derive_builder_core/src/builder_field.rs +++ b/derive_builder_core/src/builder_field.rs @@ -41,20 +41,6 @@ pub struct BuilderField<'a> { pub attrs: &'a [syn::Attribute], } -impl<'a> ToTokens for BuilderFieldType<'a> { - fn to_tokens(&self, tokens: &mut TokenStream) { - match self { - BuilderFieldType::Optional(ty) => tokens.append_all(quote!( - ::derive_builder::export::core::option::Option<#ty> - )), - BuilderFieldType::Precise(ty) => ty.to_tokens(tokens), - BuilderFieldType::Phantom(ty) => tokens.append_all(quote!( - ::derive_builder::export::core::marker::PhantomData<#ty> - )), - } - } -} - impl<'a> ToTokens for BuilderField<'a> { fn to_tokens(&self, tokens: &mut TokenStream) { let ident = self.field_ident; @@ -127,6 +113,20 @@ impl<'a> BuilderFieldType<'a> { } } +impl<'a> ToTokens for BuilderFieldType<'a> { + fn to_tokens(&self, tokens: &mut TokenStream) { + match self { + BuilderFieldType::Optional(ty) => tokens.append_all(quote!( + ::derive_builder::export::core::option::Option<#ty> + )), + BuilderFieldType::Precise(ty) => ty.to_tokens(tokens), + BuilderFieldType::Phantom(ty) => tokens.append_all(quote!( + ::derive_builder::export::core::marker::PhantomData<#ty> + )), + } + } +} + #[cfg(test)] mod tests { #[allow(unused_imports)] From 8661dc4f14b0d4b1f741749b1ec7d20ebf7c005a Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 19 Apr 2022 16:23:44 +0100 Subject: [PATCH 58/72] Move test helper macro to next to tests As per https://github.com/colin-kiegel/rust-derive-builder/pull/245#discussion_r853124397 --- derive_builder_core/src/builder_field.rs | 33 ++++++++++++------------ 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/derive_builder_core/src/builder_field.rs b/derive_builder_core/src/builder_field.rs index e849a28b..45fcb15f 100644 --- a/derive_builder_core/src/builder_field.rs +++ b/derive_builder_core/src/builder_field.rs @@ -62,22 +62,6 @@ impl<'a> BuilderField<'a> { } } -/// Helper macro for unit tests. This is _only_ public in order to be accessible -/// from doc-tests too. -#[cfg(test)] // This contains a Box::leak, so is suitable only for tests -#[doc(hidden)] -#[macro_export] -macro_rules! default_builder_field { - () => {{ - BuilderField { - field_ident: &syn::Ident::new("foo", ::proc_macro2::Span::call_site()), - field_type: BuilderFieldType::Optional(Box::leak(Box::new(parse_quote!(String)))), - field_visibility: ::std::borrow::Cow::Owned(parse_quote!(pub)), - attrs: &[parse_quote!(#[some_attr])], - } - }}; -} - /// The type of a field in the builder struct #[derive(Debug, Clone)] pub enum BuilderFieldType<'a> { @@ -127,6 +111,23 @@ impl<'a> ToTokens for BuilderFieldType<'a> { } } + +/// Helper macro for unit tests. This is _only_ public in order to be accessible +/// from doc-tests too. +#[cfg(test)] // This contains a Box::leak, so is suitable only for tests +#[doc(hidden)] +#[macro_export] +macro_rules! default_builder_field { + () => {{ + BuilderField { + field_ident: &syn::Ident::new("foo", ::proc_macro2::Span::call_site()), + field_type: BuilderFieldType::Optional(Box::leak(Box::new(parse_quote!(String)))), + field_visibility: ::std::borrow::Cow::Owned(parse_quote!(pub)), + attrs: &[parse_quote!(#[some_attr])], + } + }}; +} + #[cfg(test)] mod tests { #[allow(unused_imports)] From 0f53cf76305492ea2461ebbe35d8f2912faa7d56 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 19 Apr 2022 16:25:12 +0100 Subject: [PATCH 59/72] Remove a pointless deref and re-ref As per https://github.com/colin-kiegel/rust-derive-builder/pull/245#discussion_r853125532 --- derive_builder_core/src/initializer.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/derive_builder_core/src/initializer.rs b/derive_builder_core/src/initializer.rs index 7711669a..cf01053f 100644 --- a/derive_builder_core/src/initializer.rs +++ b/derive_builder_core/src/initializer.rs @@ -68,7 +68,7 @@ pub struct Initializer<'a> { impl<'a> ToTokens for Initializer<'a> { fn to_tokens(&self, tokens: &mut TokenStream) { let struct_field = &self.field_ident; - let builder_field = &*struct_field; + let builder_field = struct_field; // This structure prevents accidental failure to add the trailing `,` due to incautious `return` let append_rhs = |tokens: &mut TokenStream| { From ad5403c50f6eb02f95815a3c481edf5b1dd4748e Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 19 Apr 2022 16:27:09 +0100 Subject: [PATCH 60/72] Fix comment for StructLevelFieldBuilder Co-authored-by: Ted Driggs --- derive_builder_core/src/macro_options/darling_opts.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index d5f5b4db..4c77f5e6 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -125,7 +125,7 @@ impl Visibility for BuildFn { } } -/// Contents of the `field` meta in `builder` attributes. +/// Contents of the `field` meta in `builder` attributes at the struct level. #[derive(Debug, Clone, Default, FromMeta)] pub struct StructLevelFieldMeta { public: Flag, From b6ed16db72251f465bb08383a55e2170a8e5c7c9 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 19 Apr 2022 16:29:29 +0100 Subject: [PATCH 61/72] Fix doc comment for FieldLevelFieldMeta As per https://github.com/colin-kiegel/rust-derive-builder/pull/245#discussion_r853163079 --- derive_builder_core/src/macro_options/darling_opts.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index 4c77f5e6..8d187bf3 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -147,11 +147,9 @@ impl Visibility for StructLevelFieldMeta { } } -/// Contents of the `field` meta in `builder` attributes. +/// Contents of the `field` meta in `builder` attributes at the field level. // -// TODO this struct and its Visibility impl duplicate code from StructLevelFieldMeta -// Ideally they would be combined somehow, but without adversely affecting diagnostics -// from darling, etc. +// This is a superset of the attributes permitted in `field` at the struct level. #[derive(Debug, Clone, Default, FromMeta)] pub struct FieldLevelFieldMeta { public: Flag, From 30b2f99db6f09bc2d8773d260a2a5e2ef28f441e Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 19 Apr 2022 16:30:00 +0100 Subject: [PATCH 62/72] Add reference to darling(flatten) issue As per https://github.com/colin-kiegel/rust-derive-builder/pull/245#discussion_r853163079 --- derive_builder_core/src/macro_options/darling_opts.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index 8d187bf3..f31fdfde 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -150,6 +150,8 @@ impl Visibility for StructLevelFieldMeta { /// Contents of the `field` meta in `builder` attributes at the field level. // // This is a superset of the attributes permitted in `field` at the struct level. +// Perhaps in the future we will be able to use `#[darling(flatten0]`, but +// that does not exist right now: https://github.com/TedDriggs/darling/issues/146 #[derive(Debug, Clone, Default, FromMeta)] pub struct FieldLevelFieldMeta { public: Flag, From 7027fc49296a6148ef0a7b0b1897dd49b1d35a18 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 19 Apr 2022 16:32:56 +0100 Subject: [PATCH 63/72] Use error accumulator in Field::resolve As per https://github.com/colin-kiegel/rust-derive-builder/pull/245#discussion_r853164673 --- .../src/macro_options/darling_opts.rs | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index f31fdfde..9159e670 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -338,6 +338,8 @@ impl Field { /// * Check that we don't have a custom field builder *and* a default value /// * Populate `self.field_attrs` and `self.setter_attrs` by draining `self.attrs` fn resolve(mut self) -> darling::Result { + let mut errors = darling::Error::accumulator(); + // `field.build` is stronger than `default`, as it contains both instructions on how to // deal with a missing value and conversions to do on the value during target type // construction. Because default will not be used, we disallow it. @@ -351,21 +353,23 @@ impl Field { .. } = &self { - return Err(darling::Error::unsupported_format( - r#"#[builder(default)] and #[builder(build="...")] cannot be used together"#, - ) - .with_span(field_default)); + errors.push( + darling::Error::unsupported_format( + r#"#[builder(default)] and #[builder(build="...")] cannot be used together"#, + ) + .with_span(field_default), + ); }; - distribute_and_unnest_attrs( + errors.handle(distribute_and_unnest_attrs( &mut self.attrs, &mut [ ("builder_field_attr", &mut self.field_attrs), ("builder_setter_attr", &mut self.setter_attrs), ], - )?; + )); - Ok(self) + errors.finish_with(self) } } From f35f625ee9b84ad702b7c2ed0a4e9578d40bfcd9 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 19 Apr 2022 16:34:05 +0100 Subject: [PATCH 64/72] Fix a trailing newline. Not sure where this came from. --- derive_builder_core/src/builder_field.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/derive_builder_core/src/builder_field.rs b/derive_builder_core/src/builder_field.rs index 45fcb15f..9f0b4dcc 100644 --- a/derive_builder_core/src/builder_field.rs +++ b/derive_builder_core/src/builder_field.rs @@ -111,7 +111,6 @@ impl<'a> ToTokens for BuilderFieldType<'a> { } } - /// Helper macro for unit tests. This is _only_ public in order to be accessible /// from doc-tests too. #[cfg(test)] // This contains a Box::leak, so is suitable only for tests From 4e5b0d5276520b312cce7afd1ca3be30d8dfd91d Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 19 Apr 2022 16:35:53 +0100 Subject: [PATCH 65/72] Remove spuriously left-over commented-out line As per https://github.com/colin-kiegel/rust-derive-builder/pull/245#discussion_r850477607 --- derive_builder_core/src/builder_field.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/derive_builder_core/src/builder_field.rs b/derive_builder_core/src/builder_field.rs index 9f0b4dcc..bf336738 100644 --- a/derive_builder_core/src/builder_field.rs +++ b/derive_builder_core/src/builder_field.rs @@ -50,7 +50,6 @@ impl<'a> ToTokens for BuilderField<'a> { tokens.append_all(quote!( #(#attrs)* #vis #ident: #ty, )); - // K let ty = self.field_type.target_type(); } } From 9b32f3c2e9cba7ae5c3dc4cd245b3efb5daedb6b Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 19 Apr 2022 16:54:42 +0100 Subject: [PATCH 66/72] Fix build on MSRV 1.40, by replacing use of bool::then --- derive_builder_core/src/macro_options/darling_opts.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index 9159e670..3138f2d5 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -814,7 +814,8 @@ impl<'a> FieldWithDefaults<'a> { // Disabled fields become a PhantomData in the builder. We make that field // non-public, even if the rest of the builder is public, since this field is just // there to make sure the struct's generics are properly handled. - || (!self.field_enabled()).then(|| Cow::Owned(syn::Visibility::Inherited)), + || if self.field_enabled() { None } else { Some(Cow::Owned(syn::Visibility::Inherited)) }, + // ^ would prefer to use bool::then but that's only available in Rust 1.50, not in our MSRV ) .or_else(|| self.parent.field.as_expressed_vis()) .unwrap_or(Cow::Owned(syn::Visibility::Inherited)) From 1060b134628f860d9f2a4d4f1729dd2464b2c978 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 19 Apr 2022 16:57:01 +0100 Subject: [PATCH 67/72] Fix formatting of MSRV fix --- derive_builder_core/src/macro_options/darling_opts.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index 3138f2d5..a3ab1af3 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -814,8 +814,14 @@ impl<'a> FieldWithDefaults<'a> { // Disabled fields become a PhantomData in the builder. We make that field // non-public, even if the rest of the builder is public, since this field is just // there to make sure the struct's generics are properly handled. - || if self.field_enabled() { None } else { Some(Cow::Owned(syn::Visibility::Inherited)) }, - // ^ would prefer to use bool::then but that's only available in Rust 1.50, not in our MSRV + || { + // We would prefer to use bool::then but that's only available in Rust 1.50, not in our MSRV + if self.field_enabled() { + None + } else { + Some(Cow::Owned(syn::Visibility::Inherited)) + } + }, ) .or_else(|| self.parent.field.as_expressed_vis()) .unwrap_or(Cow::Owned(syn::Visibility::Inherited)) From a397b3c9cc6b3f514286bdc9ec2aa5ee43714f27 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 19 Apr 2022 18:08:32 +0100 Subject: [PATCH 68/72] Fix error message for builder(default) + builder(field(default)) --- derive_builder/tests/compile-fail/builder_field_custom.stderr | 2 +- derive_builder_core/src/macro_options/darling_opts.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/derive_builder/tests/compile-fail/builder_field_custom.stderr b/derive_builder/tests/compile-fail/builder_field_custom.stderr index 6f847386..425f929c 100644 --- a/derive_builder/tests/compile-fail/builder_field_custom.stderr +++ b/derive_builder/tests/compile-fail/builder_field_custom.stderr @@ -1,4 +1,4 @@ -error: Unexpected meta-item format `#[builder(default)] and #[builder(build="...")] cannot be used together` +error: Unexpected meta-item format `#[builder(default)] and #[builder(field(build="..."))] cannot be used together` --> tests/compile-fail/builder_field_custom.rs:6:25 | 6 | #[builder(default = "88", field(type = "usize", build = "self.ipsum + 42"))] diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index a3ab1af3..ed8798e6 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -355,7 +355,7 @@ impl Field { { errors.push( darling::Error::unsupported_format( - r#"#[builder(default)] and #[builder(build="...")] cannot be used together"#, + r#"#[builder(default)] and #[builder(field(build="..."))] cannot be used together"#, ) .with_span(field_default), ); From c421906b09389347e06e5dfde9685d73c0afcf5f Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 19 Apr 2022 23:09:35 +0100 Subject: [PATCH 69/72] Fix typo in comment Co-authored-by: Ted Driggs --- derive_builder_core/src/macro_options/darling_opts.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index ed8798e6..8abd7781 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -150,7 +150,7 @@ impl Visibility for StructLevelFieldMeta { /// Contents of the `field` meta in `builder` attributes at the field level. // // This is a superset of the attributes permitted in `field` at the struct level. -// Perhaps in the future we will be able to use `#[darling(flatten0]`, but +// Perhaps in the future we will be able to use `#[darling(flatten)]`, but // that does not exist right now: https://github.com/TedDriggs/darling/issues/146 #[derive(Debug, Clone, Default, FromMeta)] pub struct FieldLevelFieldMeta { From 147c8e1cb7c186f0e67b7e7fdd69d03518d0621a Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 19 Apr 2022 23:10:17 +0100 Subject: [PATCH 70/72] Drop comment about wanting to use bool::then Co-authored-by: Ted Driggs --- derive_builder_core/src/macro_options/darling_opts.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index 8abd7781..29e61bc9 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -815,7 +815,6 @@ impl<'a> FieldWithDefaults<'a> { // non-public, even if the rest of the builder is public, since this field is just // there to make sure the struct's generics are properly handled. || { - // We would prefer to use bool::then but that's only available in Rust 1.50, not in our MSRV if self.field_enabled() { None } else { From 95956fe6242206031ebd59584451bad5dc1344e3 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 19 Apr 2022 23:10:55 +0100 Subject: [PATCH 71/72] Change use of darling::Error::unsupported_format to custom Co-authored-by: Ted Driggs --- derive_builder_core/src/macro_options/darling_opts.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index 29e61bc9..b75f34cc 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -354,7 +354,7 @@ impl Field { } = &self { errors.push( - darling::Error::unsupported_format( + darling::Error::custom( r#"#[builder(default)] and #[builder(field(build="..."))] cannot be used together"#, ) .with_span(field_default), From 3bb2a347b481becedabb234616dc4eee263bf6f3 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 20 Apr 2022 10:14:35 +0100 Subject: [PATCH 72/72] Fix up test for changed error message --- derive_builder/tests/compile-fail/builder_field_custom.stderr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/derive_builder/tests/compile-fail/builder_field_custom.stderr b/derive_builder/tests/compile-fail/builder_field_custom.stderr index 425f929c..9730ed5a 100644 --- a/derive_builder/tests/compile-fail/builder_field_custom.stderr +++ b/derive_builder/tests/compile-fail/builder_field_custom.stderr @@ -1,4 +1,4 @@ -error: Unexpected meta-item format `#[builder(default)] and #[builder(field(build="..."))] cannot be used together` +error: #[builder(default)] and #[builder(field(build="..."))] cannot be used together --> tests/compile-fail/builder_field_custom.rs:6:25 | 6 | #[builder(default = "88", field(type = "usize", build = "self.ipsum + 42"))]