From 486480b6493469692cce98f4463430d2317c6371 Mon Sep 17 00:00:00 2001 From: Tamme Schichler Date: Sat, 16 Jan 2021 15:19:40 +0100 Subject: [PATCH 01/16] Rudimentarily adjusted parser to accept `strip_extend` with optional seed assignment in `SetterSettings` --- src/field_info.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/field_info.rs b/src/field_info.rs index 62d41731..70e1e6a5 100644 --- a/src/field_info.rs +++ b/src/field_info.rs @@ -87,6 +87,7 @@ pub struct SetterSettings { pub doc: Option, pub skip: bool, pub auto_into: bool, + pub strip_extend: Option>, pub strip_option: bool, } @@ -229,6 +230,17 @@ impl SetterSettings { self.doc = Some(*assign.right); Ok(()) } + "strip_extend" => { + if self.strip_extend.is_some() { + Err(Error::new( + assign.span(), + "Illegal setting - field is already calling extend(...) with the argument", + )) + } else { + self.strip_extend = Some(Some(*assign.right)); + Ok(()) + } + } _ => Err(Error::new_spanned(&assign, format!("Unknown parameter {:?}", name))), } } @@ -247,6 +259,14 @@ impl SetterSettings { } } )* + "strip_extend" => { + if self.strip_extend.is_some() { + Err(Error::new(path.span(), "Illegal setting - field is already calling extend(...) with the argument")) + } else { + self.strip_extend = Some(None); + Ok(()) + } + } _ => Err(Error::new_spanned( &path, format!("Unknown setter parameter {:?}", name), @@ -281,6 +301,10 @@ impl SetterSettings { self.auto_into = false; Ok(()) } + "strip_extend" => { + self.strip_extend = None; + Ok(()) + } "strip_option" => { self.strip_option = false; Ok(()) From c824836755ff709c689953301fc4c55c0bb02da1 Mon Sep 17 00:00:00 2001 From: Tamme Schichler Date: Sun, 17 Jan 2021 07:23:20 +0100 Subject: [PATCH 02/16] Implement `strip_extend` proof of concept --- examples/example.rs | 73 +++++++++++++++++++++++++-- src/struct_info.rs | 120 ++++++++++++++++++++++++++++++++------------ 2 files changed, 158 insertions(+), 35 deletions(-) diff --git a/examples/example.rs b/examples/example.rs index 0027ecfc..33cb3568 100644 --- a/examples/example.rs +++ b/examples/example.rs @@ -1,5 +1,15 @@ +use std::{collections::HashMap, iter}; + use typed_builder::TypedBuilder; +macro_rules! extend { + [$init:expr; $($expr:expr),*$(,)?] => {{ + let mut e = $init; + $(e.extend(iter::once($expr));)* + e + }}; +} + #[derive(PartialEq, TypedBuilder)] struct Foo { // Mandatory Field: @@ -13,16 +23,73 @@ struct Foo { // Or you can set the default #[builder(default = 20)] z: i32, + + // #[builder(default)] without parameter - don't require this field + // #[builder(setter(strip_extend))] without parameter - start with the default and extend from there + #[builder(default, setter(strip_extend))] + v0: Vec, + + // No `default`: This field must be set at least once. + // You can explicitly set the value (but this is not required even without `default`). + #[builder(setter(strip_extend = { let mut vec = vec![]; vec.extend(iter::once(v1)); vec }))] + v1: Vec, + + // Other `Extend` types are also supported. + #[builder(default, setter(strip_extend))] + h: HashMap, } fn main() { - assert!(Foo::builder().x(1).y(2).z(3).build() == Foo { x: 1, y: Some(2), z: 3 }); + assert!( + Foo::builder().x(1).y(2).z(3).v0(4).v1(5).h((6, 7)).build() + == Foo { + x: 1, + y: Some(2), + z: 3, + v0: vec![4], + v1: vec![5], + h: extend![HashMap::new(); (6, 7)], + } + ); // Change the order of construction: - assert!(Foo::builder().z(1).x(2).y(3).build() == Foo { x: 2, y: Some(3), z: 1 }); + assert!( + Foo::builder().z(1).x(2).h((3, 4)).v1(5).v0(6).y(7).build() + == Foo { + x: 2, + y: Some(7), + z: 1, + v0: vec![6], + v1: vec![5], + h: extend![HashMap::new(); (3, 4)], + } + ); // Optional fields are optional: - assert!(Foo::builder().x(1).build() == Foo { x: 1, y: None, z: 20 }); + assert!( + Foo::builder().x(1).v1(2).build() + == Foo { + x: 1, + y: None, + z: 20, + v0: vec![], + v1: vec![2], + h: HashMap::new(), + } + ); + + // Extend fields can be set multiple times: + assert!( + Foo::builder().x(1).v0(2).v0(3).v0(4).v1(5).v1(6).h((7, 8)).h((9, 10)).build() + == Foo { + x: 1, + y: None, + z: 20, + v0: vec![3, 4], + v1: vec![5, 6], + h: extend![HashMap::new(); (7, 8), (9, 10)], + } + ); // This will not compile - because we did not set x: // Foo::builder().build(); diff --git a/src/struct_info.rs b/src/struct_info.rs index 9684c4f5..af0b8739 100644 --- a/src/struct_info.rs +++ b/src/struct_info.rs @@ -1,6 +1,7 @@ -use proc_macro2::TokenStream; -use quote::quote; +use proc_macro2::{Span, TokenStream}; +use quote::{quote, ToTokens}; use syn::parse::Error; +use syn::{self, parse_quote, Generics, Ident}; use crate::field_info::{FieldBuilderAttr, FieldInfo}; use crate::util::{ @@ -197,15 +198,18 @@ impl<'a> StructInfo<'a> { pub fn field_impl(&self, field: &FieldInfo) -> Result { let StructInfo { ref builder_name, .. } = *self; - let descructuring = self.included_fields().map(|f| { - if f.ordinal == field.ordinal { - quote!(_) - } else { - let name = f.name; - quote!(#name) - } - }); - let reconstructing = self.included_fields().map(|f| f.name); + let descructuring = self + .included_fields() + .map(|f| { + if f.ordinal == field.ordinal { + quote!(_) + } else { + let name = f.name; + quote!(#name) + } + }) + .collect::>(); + let reconstructing = self.included_fields().map(|f| f.name).collect::>(); let &FieldInfo { name: ref field_name, @@ -275,10 +279,33 @@ impl<'a> StructInfo<'a> { } else { field_type }; + let (arg_type, arg_expr, extend_generics) = if let Some(extend_init) = &field.builder_attr.setter.strip_extend { + let arg_expr = if let Some(extend_init) = extend_init { + extend_init.to_token_stream() + } else { + let default = if let Some(default) = &field.builder_attr.default { + default.to_token_stream() + } else { + quote!(::core::default::Default::default()) + }; + quote!({ + let mut extend: #arg_type = #default; + extend.extend(::core::iter::once(#field_name)); + extend + }) + }; + (quote!(Argument), arg_expr, { + let mut generics: Generics = parse_quote!(); + generics.where_clause = parse_quote!(where #arg_type: ::core::iter::Extend); + generics + }) + } else { + (arg_type.to_token_stream(), field_name.to_token_stream(), Generics::default()) + }; let (arg_type, arg_expr) = if field.builder_attr.setter.auto_into { - (quote!(impl core::convert::Into<#arg_type>), quote!(#field_name.into())) + (quote!(impl core::convert::Into<#arg_type>), quote!(#arg_expr.into())) } else { - (quote!(#arg_type), quote!(#field_name)) + (arg_type, arg_expr) }; let arg_expr = if field.builder_attr.setter.strip_option { quote!(Some(#arg_expr)) @@ -286,17 +313,58 @@ impl<'a> StructInfo<'a> { arg_expr }; - let repeated_fields_error_type_name = syn::Ident::new( - &format!("{}_Error_Repeated_field_{}", builder_name, field_name), - proc_macro2::Span::call_site(), - ); - let repeated_fields_error_message = format!("Repeated field {}", field_name); + let extend_where = &extend_generics.where_clause; + + let repeat_items = if field.builder_attr.setter.strip_extend.is_some() { + let field_hygienic = Ident::new( + // Changing the name here doesn't really matter, but makes it easier to debug with `cargo expand`. + &format!("{}_argument", field_name), + field_name.span().resolved_at(Span::mixed_site()), + ); + quote! { + #[allow(dead_code, non_camel_case_types, missing_docs)] + impl #impl_generics #builder_name < #( #target_generics ),* > #where_clause { + #doc + pub fn #field_name #extend_generics (self, #field_name: #arg_type) -> #builder_name < #( #target_generics ),* > #extend_where { + let #field_hygienic = #field_name; + let ( #(#reconstructing,)* ) = self.fields; + let mut #field_name = #field_name; + #field_name.0.extend(::core::iter::once(#field_hygienic)); + #builder_name { + fields: ( #(#reconstructing,)* ), + _phantom: self._phantom, + } + } + } + } + } else { + let repeated_fields_error_type_name = syn::Ident::new( + &format!("{}_Error_Repeated_field_{}", builder_name, field_name), + proc_macro2::Span::call_site(), + ); + let repeated_fields_error_message = format!("Repeated field {}", field_name); + quote! { + #[doc(hidden)] + #[allow(dead_code, non_camel_case_types, non_snake_case)] + pub enum #repeated_fields_error_type_name {} + #[doc(hidden)] + #[allow(dead_code, non_camel_case_types, missing_docs)] + impl #impl_generics #builder_name < #( #target_generics ),* > #where_clause { + #[deprecated( + note = #repeated_fields_error_message + )] + pub fn #field_name (self, _: #repeated_fields_error_type_name) -> #builder_name < #( #target_generics ),* > { + self + } + } + } + }; Ok(quote! { #[allow(dead_code, non_camel_case_types, missing_docs)] impl #impl_generics #builder_name < #( #ty_generics ),* > #where_clause { #doc - pub fn #field_name (self, #field_name: #arg_type) -> #builder_name < #( #target_generics ),* > { + pub fn #field_name #extend_generics (self, #[deny(dead_code)] #field_name: #arg_type) -> #builder_name < #( #target_generics ),* > #extend_where { let #field_name = (#arg_expr,); let ( #(#descructuring,)* ) = self.fields; #builder_name { @@ -305,19 +373,7 @@ impl<'a> StructInfo<'a> { } } } - #[doc(hidden)] - #[allow(dead_code, non_camel_case_types, non_snake_case)] - pub enum #repeated_fields_error_type_name {} - #[doc(hidden)] - #[allow(dead_code, non_camel_case_types, missing_docs)] - impl #impl_generics #builder_name < #( #target_generics ),* > #where_clause { - #[deprecated( - note = #repeated_fields_error_message - )] - pub fn #field_name (self, _: #repeated_fields_error_type_name) -> #builder_name < #( #target_generics ),* > { - self - } - } + #repeat_items }) } From 03e0e581a22b7a840e2b503705d1766c1fa2fce3 Mon Sep 17 00:00:00 2001 From: Tamme Schichler Date: Mon, 18 Jan 2021 05:35:28 +0100 Subject: [PATCH 03/16] Rework `strip_extend` into `strip_collection`, which has stricter typing but is more ergonomic This commit also fixes the error on faulty custom collection initialisers. --- examples/example.rs | 10 ++++---- src/field_info.rs | 19 +++++++-------- src/struct_info.rs | 56 +++++++++++++++++++++++++++------------------ 3 files changed, 49 insertions(+), 36 deletions(-) diff --git a/examples/example.rs b/examples/example.rs index 33cb3568..4b4dcca5 100644 --- a/examples/example.rs +++ b/examples/example.rs @@ -25,17 +25,17 @@ struct Foo { z: i32, // #[builder(default)] without parameter - don't require this field - // #[builder(setter(strip_extend))] without parameter - start with the default and extend from there - #[builder(default, setter(strip_extend))] + // #[builder(setter(strip_collection))] without parameter - start with the default and extend from there + #[builder(default, setter(strip_collection))] v0: Vec, // No `default`: This field must be set at least once. - // You can explicitly set the value (but this is not required even without `default`). - #[builder(setter(strip_extend = { let mut vec = vec![]; vec.extend(iter::once(v1)); vec }))] + // You can explicitly create the collection from the first item (but this is not required even without `default`). + #[builder(setter(strip_collection = vec![v1_first]))] v1: Vec, // Other `Extend` types are also supported. - #[builder(default, setter(strip_extend))] + #[builder(default, setter(strip_collection))] h: HashMap, } diff --git a/src/field_info.rs b/src/field_info.rs index 70e1e6a5..3b5c859d 100644 --- a/src/field_info.rs +++ b/src/field_info.rs @@ -2,6 +2,7 @@ use proc_macro2::TokenStream; use quote::quote; use syn::parse::Error; use syn::spanned::Spanned; +use syn::{self, Token}; use crate::util::ident_to_type; use crate::util::{expr_to_single_string, path_to_single_string}; @@ -87,7 +88,7 @@ pub struct SetterSettings { pub doc: Option, pub skip: bool, pub auto_into: bool, - pub strip_extend: Option>, + pub strip_collection: Option>, pub strip_option: bool, } @@ -230,14 +231,14 @@ impl SetterSettings { self.doc = Some(*assign.right); Ok(()) } - "strip_extend" => { - if self.strip_extend.is_some() { + "strip_collection" => { + if self.strip_collection.is_some() { Err(Error::new( assign.span(), "Illegal setting - field is already calling extend(...) with the argument", )) } else { - self.strip_extend = Some(Some(*assign.right)); + self.strip_collection = Some(Some((assign.eq_token, *assign.right))); Ok(()) } } @@ -259,11 +260,11 @@ impl SetterSettings { } } )* - "strip_extend" => { - if self.strip_extend.is_some() { + "strip_collection" => { + if self.strip_collection.is_some() { Err(Error::new(path.span(), "Illegal setting - field is already calling extend(...) with the argument")) } else { - self.strip_extend = Some(None); + self.strip_collection = Some(None); Ok(()) } } @@ -301,8 +302,8 @@ impl SetterSettings { self.auto_into = false; Ok(()) } - "strip_extend" => { - self.strip_extend = None; + "strip_collection" => { + self.strip_collection = None; Ok(()) } "strip_option" => { diff --git a/src/struct_info.rs b/src/struct_info.rs index af0b8739..eb37ee9d 100644 --- a/src/struct_info.rs +++ b/src/struct_info.rs @@ -1,7 +1,8 @@ use proc_macro2::{Span, TokenStream}; -use quote::{quote, ToTokens}; +use quote::{quote, quote_spanned, ToTokens}; use syn::parse::Error; -use syn::{self, parse_quote, Generics, Ident}; +use syn::spanned::Spanned; +use syn::{self, Ident}; use crate::field_info::{FieldBuilderAttr, FieldInfo}; use crate::util::{ @@ -212,8 +213,8 @@ impl<'a> StructInfo<'a> { let reconstructing = self.included_fields().map(|f| f.name).collect::>(); let &FieldInfo { - name: ref field_name, - ty: ref field_type, + name: field_name, + ty: field_type, .. } = field; let mut ty_generics: Vec = self @@ -279,9 +280,9 @@ impl<'a> StructInfo<'a> { } else { field_type }; - let (arg_type, arg_expr, extend_generics) = if let Some(extend_init) = &field.builder_attr.setter.strip_extend { - let arg_expr = if let Some(extend_init) = extend_init { - extend_init.to_token_stream() + let (arg_type, arg_expr) = if let Some(collection_init) = &field.builder_attr.setter.strip_collection { + let arg_expr = if let Some((_, collection_init)) = collection_init { + collection_init.to_token_stream() } else { let default = if let Some(default) = &field.builder_attr.default { default.to_token_stream() @@ -289,19 +290,16 @@ impl<'a> StructInfo<'a> { quote!(::core::default::Default::default()) }; quote!({ - let mut extend: #arg_type = #default; - extend.extend(::core::iter::once(#field_name)); - extend + let mut collection: #arg_type = #default; + ::core::iter::Extend::extend(&mut collection, ::core::iter::once(#field_name)); + collection }) }; - (quote!(Argument), arg_expr, { - let mut generics: Generics = parse_quote!(); - generics.where_clause = parse_quote!(where #arg_type: ::core::iter::Extend); - generics - }) + (quote!(<#arg_type as ::core::iter::IntoIterator>::Item), arg_expr) } else { - (arg_type.to_token_stream(), field_name.to_token_stream(), Generics::default()) + (arg_type.to_token_stream(), field_name.to_token_stream()) }; + let (arg_type, arg_expr) = if field.builder_attr.setter.auto_into { (quote!(impl core::convert::Into<#arg_type>), quote!(#arg_expr.into())) } else { @@ -313,9 +311,7 @@ impl<'a> StructInfo<'a> { arg_expr }; - let extend_where = &extend_generics.where_clause; - - let repeat_items = if field.builder_attr.setter.strip_extend.is_some() { + let repeat_items = if field.builder_attr.setter.strip_collection.is_some() { let field_hygienic = Ident::new( // Changing the name here doesn't really matter, but makes it easier to debug with `cargo expand`. &format!("{}_argument", field_name), @@ -325,11 +321,11 @@ impl<'a> StructInfo<'a> { #[allow(dead_code, non_camel_case_types, missing_docs)] impl #impl_generics #builder_name < #( #target_generics ),* > #where_clause { #doc - pub fn #field_name #extend_generics (self, #field_name: #arg_type) -> #builder_name < #( #target_generics ),* > #extend_where { + pub fn #field_name (self, #field_name: #arg_type) -> #builder_name < #( #target_generics ),* > { let #field_hygienic = #field_name; let ( #(#reconstructing,)* ) = self.fields; let mut #field_name = #field_name; - #field_name.0.extend(::core::iter::once(#field_hygienic)); + ::core::iter::Extend::extend(&mut #field_name.0, ::core::iter::once(#field_hygienic)); #builder_name { fields: ( #(#reconstructing,)* ), _phantom: self._phantom, @@ -360,11 +356,27 @@ impl<'a> StructInfo<'a> { } }; + let (arg_name, forbid_unused_first) = if let Some(Some((ref eq, ref init))) = field.builder_attr.setter.strip_collection { + ( + Ident::new( + &format!("{}_first", field_name), + // This places the `unused_variables` error caused by faulty collection seeds on the expression after `strip_collection =` + // (more or less - we might only get the first token, but this should eventually improve with proc macro improvements), + // which is much less confusing than having it on the field name. + init.span(), + ), + // Place "the lint level is defined here" on `strip_collection =`'s `=`. + Some(quote_spanned!(eq.span=> #[forbid(unused_variables)])), + ) + } else { + (field_name.clone(), None) + }; + Ok(quote! { #[allow(dead_code, non_camel_case_types, missing_docs)] impl #impl_generics #builder_name < #( #ty_generics ),* > #where_clause { #doc - pub fn #field_name #extend_generics (self, #[deny(dead_code)] #field_name: #arg_type) -> #builder_name < #( #target_generics ),* > #extend_where { + pub fn #field_name (self, #forbid_unused_first #arg_name: #arg_type) -> #builder_name < #( #target_generics ),* > { let #field_name = (#arg_expr,); let ( #(#descructuring,)* ) = self.fields; #builder_name { From b19a01badeb6e371bcf23fae9b8b466b66e7e0f5 Mon Sep 17 00:00:00 2001 From: Tamme Schichler Date: Mon, 18 Jan 2021 06:12:45 +0100 Subject: [PATCH 04/16] Use `FromIterator` to create initial stripped collection iff neither `default` nor a custom initialiser are present This commit should also improve collection type error localisation for `strip_collection`. --- src/field_info.rs | 20 ++++++++++++++++---- src/struct_info.rs | 34 +++++++++++++++++++--------------- 2 files changed, 35 insertions(+), 19 deletions(-) diff --git a/src/field_info.rs b/src/field_info.rs index 3b5c859d..64d98fb2 100644 --- a/src/field_info.rs +++ b/src/field_info.rs @@ -1,4 +1,4 @@ -use proc_macro2::TokenStream; +use proc_macro2::{Span, TokenStream}; use quote::quote; use syn::parse::Error; use syn::spanned::Spanned; @@ -88,10 +88,16 @@ pub struct SetterSettings { pub doc: Option, pub skip: bool, pub auto_into: bool, - pub strip_collection: Option>, + pub strip_collection: Option, pub strip_option: bool, } +#[derive(Debug, Clone)] +pub struct StripCollection { + pub keyword_span: Span, + pub custom_initializer: Option<(Token![=], syn::Expr)>, +} + impl FieldBuilderAttr { pub fn with(mut self, attrs: &[syn::Attribute]) -> Result { let mut skip_tokens = None; @@ -238,7 +244,10 @@ impl SetterSettings { "Illegal setting - field is already calling extend(...) with the argument", )) } else { - self.strip_collection = Some(Some((assign.eq_token, *assign.right))); + self.strip_collection = Some(StripCollection { + keyword_span: name.span(), + custom_initializer: Some((assign.eq_token, *assign.right)), + }); Ok(()) } } @@ -264,7 +273,10 @@ impl SetterSettings { if self.strip_collection.is_some() { Err(Error::new(path.span(), "Illegal setting - field is already calling extend(...) with the argument")) } else { - self.strip_collection = Some(None); + self.strip_collection = Some(StripCollection{ + keyword_span: name.span(), + custom_initializer: None + }); Ok(()) } } diff --git a/src/struct_info.rs b/src/struct_info.rs index eb37ee9d..749558e3 100644 --- a/src/struct_info.rs +++ b/src/struct_info.rs @@ -4,7 +4,7 @@ use syn::parse::Error; use syn::spanned::Spanned; use syn::{self, Ident}; -use crate::field_info::{FieldBuilderAttr, FieldInfo}; +use crate::field_info::{FieldBuilderAttr, FieldInfo, StripCollection}; use crate::util::{ empty_type, empty_type_tuple, expr_to_single_string, make_punctuated_single, modify_types_generics_hack, path_to_single_string, type_tuple, @@ -280,26 +280,26 @@ impl<'a> StructInfo<'a> { } else { field_type }; - let (arg_type, arg_expr) = if let Some(collection_init) = &field.builder_attr.setter.strip_collection { - let arg_expr = if let Some((_, collection_init)) = collection_init { - collection_init.to_token_stream() - } else { - let default = if let Some(default) = &field.builder_attr.default { - default.to_token_stream() - } else { - quote!(::core::default::Default::default()) - }; - quote!({ + let (arg_type, arg_expr) = if let Some(StripCollection { + keyword_span, + ref custom_initializer, + }) = field.builder_attr.setter.strip_collection + { + let arg_expr = if let Some((_, init)) = custom_initializer { + init.to_token_stream() + } else if let Some(default) = &field.builder_attr.default { + quote_spanned!(keyword_span=> { let mut collection: #arg_type = #default; ::core::iter::Extend::extend(&mut collection, ::core::iter::once(#field_name)); collection }) + } else { + quote_spanned!(keyword_span=> ::<#arg_type as ::core::iter::FromIterator>::from_iter(::core::iter::once(#field_name))) }; (quote!(<#arg_type as ::core::iter::IntoIterator>::Item), arg_expr) } else { (arg_type.to_token_stream(), field_name.to_token_stream()) }; - let (arg_type, arg_expr) = if field.builder_attr.setter.auto_into { (quote!(impl core::convert::Into<#arg_type>), quote!(#arg_expr.into())) } else { @@ -311,13 +311,13 @@ impl<'a> StructInfo<'a> { arg_expr }; - let repeat_items = if field.builder_attr.setter.strip_collection.is_some() { + let repeat_items = if let Some(StripCollection { keyword_span, .. }) = field.builder_attr.setter.strip_collection { let field_hygienic = Ident::new( // Changing the name here doesn't really matter, but makes it easier to debug with `cargo expand`. &format!("{}_argument", field_name), field_name.span().resolved_at(Span::mixed_site()), ); - quote! { + quote_spanned! {keyword_span=> #[allow(dead_code, non_camel_case_types, missing_docs)] impl #impl_generics #builder_name < #( #target_generics ),* > #where_clause { #doc @@ -356,7 +356,11 @@ impl<'a> StructInfo<'a> { } }; - let (arg_name, forbid_unused_first) = if let Some(Some((ref eq, ref init))) = field.builder_attr.setter.strip_collection { + let (arg_name, forbid_unused_first) = if let Some(StripCollection { + custom_initializer: Some((ref eq, ref init)), + .. + }) = field.builder_attr.setter.strip_collection + { ( Ident::new( &format!("{}_first", field_name), From 437656660f7818a7f04e3528084ef649b671cbaa Mon Sep 17 00:00:00 2001 From: Tamme Schichler Date: Mon, 18 Jan 2021 06:37:04 +0100 Subject: [PATCH 05/16] Accept any valid `Extend::extend` parameter in repeat setter calls This won't worsen type inference or complicate custom initialisers, since the argument is still concretely typed on the first call. --- src/struct_info.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/struct_info.rs b/src/struct_info.rs index 749558e3..fb5d6c7f 100644 --- a/src/struct_info.rs +++ b/src/struct_info.rs @@ -321,7 +321,12 @@ impl<'a> StructInfo<'a> { #[allow(dead_code, non_camel_case_types, missing_docs)] impl #impl_generics #builder_name < #( #target_generics ),* > #where_clause { #doc - pub fn #field_name (self, #field_name: #arg_type) -> #builder_name < #( #target_generics ),* > { + pub fn #field_name (self, #field_name: Argument) -> #builder_name < #( #target_generics ),* > + // Making this repeat setter here further generic has a potential performance benefit: + // Many collections are also `Extend<&T>` where `T: Copy`. Calling this overload will then + // copy the value direction into the collection instead of through a stack temporary. + where #field_type: ::core::iter::Extend, + { let #field_hygienic = #field_name; let ( #(#reconstructing,)* ) = self.fields; let mut #field_name = #field_name; From 8e4b301563971a5adc2869a746df899c259a8d06 Mon Sep 17 00:00:00 2001 From: Tamme Schichler Date: Fri, 22 Jan 2021 05:25:20 +0100 Subject: [PATCH 06/16] =?UTF-8?q?Accept=20e.g.=20`strip=5Fcollection(from?= =?UTF-8?q?=5Ffirst=20=3D=20|first|=20vec![first])`=20instead=20of=20`stri?= =?UTF-8?q?p=5Fcollection=20=3D=20vec![=E2=80=A6=5Ffirst]`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- examples/example.rs | 2 +- src/field_info.rs | 61 ++++++++++++++++++++++++++++++++++++++------- src/struct_info.rs | 39 ++++++++--------------------- 3 files changed, 63 insertions(+), 39 deletions(-) diff --git a/examples/example.rs b/examples/example.rs index 4b4dcca5..c71245b1 100644 --- a/examples/example.rs +++ b/examples/example.rs @@ -31,7 +31,7 @@ struct Foo { // No `default`: This field must be set at least once. // You can explicitly create the collection from the first item (but this is not required even without `default`). - #[builder(setter(strip_collection = vec![v1_first]))] + #[builder(setter(strip_collection(from_first = |first| vec![first])))] v1: Vec, // Other `Extend` types are also supported. diff --git a/src/field_info.rs b/src/field_info.rs index 64d98fb2..11debe2c 100644 --- a/src/field_info.rs +++ b/src/field_info.rs @@ -2,7 +2,6 @@ use proc_macro2::{Span, TokenStream}; use quote::quote; use syn::parse::Error; use syn::spanned::Spanned; -use syn::{self, Token}; use crate::util::ident_to_type; use crate::util::{expr_to_single_string, path_to_single_string}; @@ -95,7 +94,13 @@ pub struct SetterSettings { #[derive(Debug, Clone)] pub struct StripCollection { pub keyword_span: Span, - pub custom_initializer: Option<(Token![=], syn::Expr)>, + pub from_first: Option, +} + +#[derive(Debug, Clone)] +pub struct FromFirst { + pub keyword_span: Span, + pub closure: syn::ExprClosure, } impl FieldBuilderAttr { @@ -237,21 +242,59 @@ impl SetterSettings { self.doc = Some(*assign.right); Ok(()) } + _ => Err(Error::new_spanned(&assign, format!("Unknown parameter {:?}", name))), + } + } + syn::Expr::Call(call) => { + let name = + expr_to_single_string(&call.func).ok_or_else(|| Error::new_spanned(&call.func, "Expected identifier"))?; + match name.as_str() { "strip_collection" => { if self.strip_collection.is_some() { Err(Error::new( - assign.span(), + call.span(), "Illegal setting - field is already calling extend(...) with the argument", )) + } else if let Some(attr) = call.attrs.first() { + Err(Error::new_spanned(attr, "Unexpected attribute")) } else { - self.strip_collection = Some(StripCollection { + let mut strip_collection = StripCollection { keyword_span: name.span(), - custom_initializer: Some((assign.eq_token, *assign.right)), - }); + from_first: None, + }; + for arg in call.args { + match arg { + syn::Expr::Assign(assign) => { + let name = expr_to_single_string(&assign.left) + .ok_or_else(|| Error::new_spanned(&assign.left, "Expected identifier"))?; + match name.as_str() { + "from_first" => match *assign.right { + syn::Expr::Closure(closure) => { + strip_collection.from_first = Some(FromFirst { + keyword_span: assign.left.span(), + closure, + }) + } + other => { + return Err(Error::new_spanned(other, "Expected closure (|first| <...>)")) + } + }, + _ => { + return Err(Error::new_spanned( + &assign.left, + format!("Unknown parameter {:?}", name), + )) + } + } + } + _ => return Err(Error::new_spanned(arg, "Expected (<...>=<...>)")), + } + } + self.strip_collection = Some(strip_collection); Ok(()) } } - _ => Err(Error::new_spanned(&assign, format!("Unknown parameter {:?}", name))), + _ => Err(Error::new_spanned(&call.func, format!("Unknown parameter {:?}", name))), } } syn::Expr::Path(path) => { @@ -275,7 +318,7 @@ impl SetterSettings { } else { self.strip_collection = Some(StripCollection{ keyword_span: name.span(), - custom_initializer: None + from_first: None }); Ok(()) } @@ -328,7 +371,7 @@ impl SetterSettings { Err(Error::new_spanned(expr, "Expected simple identifier".to_owned())) } } - _ => Err(Error::new_spanned(expr, "Expected (<...>=<...>)")), + _ => Err(Error::new_spanned(expr, "Expected (<...>=<...>) or (<...>(<...>))")), } } } diff --git a/src/struct_info.rs b/src/struct_info.rs index fb5d6c7f..2cb54be6 100644 --- a/src/struct_info.rs +++ b/src/struct_info.rs @@ -1,10 +1,9 @@ use proc_macro2::{Span, TokenStream}; use quote::{quote, quote_spanned, ToTokens}; use syn::parse::Error; -use syn::spanned::Spanned; use syn::{self, Ident}; -use crate::field_info::{FieldBuilderAttr, FieldInfo, StripCollection}; +use crate::field_info::{FieldBuilderAttr, FieldInfo, FromFirst, StripCollection}; use crate::util::{ empty_type, empty_type_tuple, expr_to_single_string, make_punctuated_single, modify_types_generics_hack, path_to_single_string, type_tuple, @@ -281,20 +280,22 @@ impl<'a> StructInfo<'a> { field_type }; let (arg_type, arg_expr) = if let Some(StripCollection { - keyword_span, - ref custom_initializer, + ref keyword_span, + ref from_first, }) = field.builder_attr.setter.strip_collection { - let arg_expr = if let Some((_, init)) = custom_initializer { - init.to_token_stream() + let arg_expr = if let Some(FromFirst { keyword_span, closure }) = from_first { + // `Span::mixed_site()`-resolution suppresses `clippy::redundant_closure_call` on + // the generated code only. + quote_spanned!(keyword_span.resolved_at(Span::mixed_site())=> (#closure)(#field_name)) } else if let Some(default) = &field.builder_attr.default { - quote_spanned!(keyword_span=> { + quote_spanned!(*keyword_span=> { let mut collection: #arg_type = #default; ::core::iter::Extend::extend(&mut collection, ::core::iter::once(#field_name)); collection }) } else { - quote_spanned!(keyword_span=> ::<#arg_type as ::core::iter::FromIterator>::from_iter(::core::iter::once(#field_name))) + quote_spanned!(*keyword_span=> ::<#arg_type as ::core::iter::FromIterator>::from_iter(::core::iter::once(#field_name))) }; (quote!(<#arg_type as ::core::iter::IntoIterator>::Item), arg_expr) } else { @@ -361,31 +362,11 @@ impl<'a> StructInfo<'a> { } }; - let (arg_name, forbid_unused_first) = if let Some(StripCollection { - custom_initializer: Some((ref eq, ref init)), - .. - }) = field.builder_attr.setter.strip_collection - { - ( - Ident::new( - &format!("{}_first", field_name), - // This places the `unused_variables` error caused by faulty collection seeds on the expression after `strip_collection =` - // (more or less - we might only get the first token, but this should eventually improve with proc macro improvements), - // which is much less confusing than having it on the field name. - init.span(), - ), - // Place "the lint level is defined here" on `strip_collection =`'s `=`. - Some(quote_spanned!(eq.span=> #[forbid(unused_variables)])), - ) - } else { - (field_name.clone(), None) - }; - Ok(quote! { #[allow(dead_code, non_camel_case_types, missing_docs)] impl #impl_generics #builder_name < #( #ty_generics ),* > #where_clause { #doc - pub fn #field_name (self, #forbid_unused_first #arg_name: #arg_type) -> #builder_name < #( #target_generics ),* > { + pub fn #field_name (self, #field_name: #arg_type) -> #builder_name < #( #target_generics ),* > { let #field_name = (#arg_expr,); let ( #(#descructuring,)* ) = self.fields; #builder_name { From a3d3e31fcd115fc52348b802e6e16cea0d7ec9d2 Mon Sep 17 00:00:00 2001 From: Tamme Schichler Date: Thu, 4 Feb 2021 16:40:50 +0100 Subject: [PATCH 07/16] Rework `strip_collection` into `extend`, allow extending with an iterator, move extending by one item to a separate (chooseable) name --- examples/example.rs | 25 +++-- src/field_info.rs | 137 +++++++++++++++++++----- src/struct_info.rs | 255 ++++++++++++++++++++++++++++---------------- src/util.rs | 8 +- 4 files changed, 295 insertions(+), 130 deletions(-) diff --git a/examples/example.rs b/examples/example.rs index c71245b1..8474b439 100644 --- a/examples/example.rs +++ b/examples/example.rs @@ -25,23 +25,23 @@ struct Foo { z: i32, // #[builder(default)] without parameter - don't require this field - // #[builder(setter(strip_collection))] without parameter - start with the default and extend from there - #[builder(default, setter(strip_collection))] + // #[builder(setter(extend))] without parameter - start with the default and extend from there + #[builder(default, setter(extend(from_first, item_name = i0)))] v0: Vec, // No `default`: This field must be set at least once. // You can explicitly create the collection from the first item (but this is not required even without `default`). - #[builder(setter(strip_collection(from_first = |first| vec![first])))] + #[builder(setter(extend(from_first = |first| vec![first])))] v1: Vec, // Other `Extend` types are also supported. - #[builder(default, setter(strip_collection))] + #[builder(default, setter(extend))] h: HashMap, } fn main() { assert!( - Foo::builder().x(1).y(2).z(3).v0(4).v1(5).h((6, 7)).build() + Foo::builder().x(1).y(2).z(3).i0(4).v1_item(5).h_item((6, 7)).build() == Foo { x: 1, y: Some(2), @@ -54,7 +54,7 @@ fn main() { // Change the order of construction: assert!( - Foo::builder().z(1).x(2).h((3, 4)).v1(5).v0(6).y(7).build() + Foo::builder().z(1).x(2).h_item((3, 4)).v1_item(5).i0(6).y(7).build() == Foo { x: 2, y: Some(7), @@ -67,7 +67,7 @@ fn main() { // Optional fields are optional: assert!( - Foo::builder().x(1).v1(2).build() + Foo::builder().x(1).v1_item(2).build() == Foo { x: 1, y: None, @@ -80,7 +80,16 @@ fn main() { // Extend fields can be set multiple times: assert!( - Foo::builder().x(1).v0(2).v0(3).v0(4).v1(5).v1(6).h((7, 8)).h((9, 10)).build() + Foo::builder() + .x(1) + .i0(2) + .i0(3) + .i0(4) + .v1_item(5) + .v1_item(6) + .h_item((7, 8)) + .h_item((9, 10)) + .build() == Foo { x: 1, y: None, diff --git a/src/field_info.rs b/src/field_info.rs index 652ac8aa..2a55e8b3 100644 --- a/src/field_info.rs +++ b/src/field_info.rs @@ -3,7 +3,7 @@ use quote::quote; use syn::parse::Error; use syn::spanned::Spanned; -use crate::util::{expr_to_single_string, ident_to_type, path_to_single_string, strip_raw_ident_prefix}; +use crate::util::{expr_to_single_string, ident_to_type, path_to_single_ident, path_to_single_string, strip_raw_ident_prefix}; #[derive(Debug)] pub struct FieldInfo<'a> { @@ -89,20 +89,50 @@ pub struct SetterSettings { pub doc: Option, pub skip: bool, pub auto_into: bool, - pub strip_collection: Option, + pub extend: Option, pub strip_option: bool, } #[derive(Debug, Clone)] -pub struct StripCollection { +pub enum Configurable { + Unset, + Auto { keyword_span: Span }, + Custom { keyword_span: Span, value: T }, +} + +#[derive(Debug, Clone)] +pub struct Configured { pub keyword_span: Span, - pub from_first: Option, + pub value: T, +} + +impl Configurable { + pub fn into_configured(self, auto: impl FnOnce(Span) -> T) -> Option> { + match self { + Configurable::Unset => None, + Configurable::Auto { keyword_span } => Some(Configured { + keyword_span, + value: auto(keyword_span), + }), + Configurable::Custom { keyword_span, value } => Some(Configured { keyword_span, value }), + } + } + + pub fn ensure_unset(&self, error_span: Span) -> Result<(), Error> { + if matches!(self, Configurable::Unset) { + Ok(()) + } else { + Err(Error::new(error_span, "Duplicate option")) + } + } } #[derive(Debug, Clone)] -pub struct FromFirst { +pub struct ExtendField { pub keyword_span: Span, - pub closure: syn::ExprClosure, + pub from_first: Configurable, + pub from_iter: Configurable, + pub item_name: Option, } impl FieldBuilderAttr { @@ -251,8 +281,8 @@ impl SetterSettings { let name = expr_to_single_string(&call.func).ok_or_else(|| Error::new_spanned(&call.func, "Expected identifier"))?; match name.as_str() { - "strip_collection" => { - if self.strip_collection.is_some() { + "extend" => { + if self.extend.is_some() { Err(Error::new( call.span(), "Illegal setting - field is already calling extend(...) with the argument", @@ -260,9 +290,11 @@ impl SetterSettings { } else if let Some(attr) = call.attrs.first() { Err(Error::new_spanned(attr, "Unexpected attribute")) } else { - let mut strip_collection = StripCollection { + let mut extend = ExtendField { keyword_span: name.span(), - from_first: None, + from_first: Configurable::Unset, + from_iter: Configurable::Unset, + item_name: None, }; for arg in call.args { match arg { @@ -270,17 +302,47 @@ impl SetterSettings { let name = expr_to_single_string(&assign.left) .ok_or_else(|| Error::new_spanned(&assign.left, "Expected identifier"))?; match name.as_str() { - "from_first" => match *assign.right { - syn::Expr::Closure(closure) => { - strip_collection.from_first = Some(FromFirst { - keyword_span: assign.left.span(), - closure, - }) + "from_first" => { + extend.from_first.ensure_unset(assign.left.span())?; + match *assign.right { + syn::Expr::Closure(closure) => { + extend.from_first = Configurable::Custom { + keyword_span: assign.left.span(), + value: closure, + } + } + other => { + return Err(Error::new_spanned(other, "Expected closure (|first| <...>)")) + } } - other => { - return Err(Error::new_spanned(other, "Expected closure (|first| <...>)")) + } + "from_iter" => { + extend.from_iter.ensure_unset(assign.left.span())?; + match *assign.right { + syn::Expr::Closure(closure) => { + extend.from_iter = Configurable::Custom { + keyword_span: assign.left.span(), + value: closure, + } + } + other => { + return Err(Error::new_spanned(other, "Expected closure (|iter| <...>)")) + } + } + } + "item_name" => { + if extend.item_name.is_some() { + return Err(Error::new_spanned(assign.left, "Duplicate option")); } - }, + match *assign.right { + syn::Expr::Path(path) => { + let name = path_to_single_ident(&path.path) + .ok_or_else(|| Error::new_spanned(&path, "Expected identifier"))?; + extend.item_name = Some(name.clone()) + } + other => return Err(Error::new_spanned(other, "Expected identifier")), + } + } _ => { return Err(Error::new_spanned( &assign.left, @@ -289,10 +351,29 @@ impl SetterSettings { } } } - _ => return Err(Error::new_spanned(arg, "Expected (<...>=<...>)")), + syn::Expr::Path(path) => { + let name = path_to_single_string(&path.path) + .ok_or_else(|| Error::new_spanned(&path, "Expected identifier"))?; + match name.as_str() { + "from_first" => { + extend.from_first.ensure_unset(path.span())?; + extend.from_first = Configurable::Auto { + keyword_span: path.span(), + }; + } + "from_iter" => { + extend.from_iter.ensure_unset(path.span())?; + extend.from_iter = Configurable::Auto { + keyword_span: path.span(), + }; + } + _ => return Err(Error::new_spanned(&path, format!("Unknown parameter {:?}", name))), + } + } + _ => return Err(Error::new_spanned(arg, "Expected (<...>) or (<...>=<...>)")), } } - self.strip_collection = Some(strip_collection); + self.extend = Some(extend); Ok(()) } } @@ -314,13 +395,15 @@ impl SetterSettings { } } )* - "strip_collection" => { - if self.strip_collection.is_some() { + "extend" => { + if self.extend.is_some() { Err(Error::new(path.span(), "Illegal setting - field is already calling extend(...) with the argument")) } else { - self.strip_collection = Some(StripCollection{ + self.extend = Some(ExtendField { keyword_span: name.span(), - from_first: None + from_first: Configurable::Auto { keyword_span:name.span() }, + from_iter: Configurable::Auto { keyword_span:name.span() }, + item_name: None, }); Ok(()) } @@ -359,8 +442,8 @@ impl SetterSettings { self.auto_into = false; Ok(()) } - "strip_collection" => { - self.strip_collection = None; + "extend" => { + self.extend = None; Ok(()) } "strip_option" => { diff --git a/src/struct_info.rs b/src/struct_info.rs index cd7905f1..18cf8bd8 100644 --- a/src/struct_info.rs +++ b/src/struct_info.rs @@ -1,9 +1,8 @@ use proc_macro2::{Span, TokenStream}; -use quote::{quote, quote_spanned, ToTokens}; +use quote::{quote, quote_spanned}; use syn::parse::Error; -use syn::{self, Ident}; -use crate::field_info::{FieldBuilderAttr, FieldInfo, FromFirst, StripCollection}; +use crate::field_info::{Configured, ExtendField, FieldBuilderAttr, FieldInfo}; use crate::util::{ empty_type, empty_type_tuple, expr_to_single_string, make_punctuated_single, modify_types_generics_hack, path_to_single_string, strip_raw_ident_prefix, type_tuple, @@ -198,22 +197,19 @@ impl<'a> StructInfo<'a> { pub fn field_impl(&self, field: &FieldInfo) -> Result { let StructInfo { ref builder_name, .. } = *self; - let descructuring = self - .included_fields() - .map(|f| { - if f.ordinal == field.ordinal { - quote!(_) - } else { - let name = f.name; - quote!(#name) - } - }) - .collect::>(); - let reconstructing = self.included_fields().map(|f| f.name).collect::>(); + let descructuring = self.included_fields().map(|f| { + if f.ordinal == field.ordinal { + quote!(_) + } else { + let name = f.name; + quote!(#name) + } + }); + let reconstructing = self.included_fields().map(|f| f.name); let &FieldInfo { - name: field_name, - ty: field_type, + name: ref field_name, + ty: ref field_type, .. } = field; let mut ty_generics: Vec = self @@ -269,77 +265,153 @@ impl<'a> StructInfo<'a> { None => quote!(), }; - // NOTE: both auto_into and strip_option affect `arg_type` and `arg_expr`, but the order of - // nesting is different so we have to do this little dance. - let arg_type = if field.builder_attr.setter.strip_option { - let internal_type = field - .type_from_inside_option() - .ok_or_else(|| Error::new_spanned(&field_type, "can't `strip_option` - field is not `Option<...>`"))?; - internal_type - } else { - field_type - }; - let (arg_type, arg_expr) = if let Some(StripCollection { - ref keyword_span, - ref from_first, - }) = field.builder_attr.setter.strip_collection + if let Some(ExtendField { + keyword_span, + from_first, + from_iter, + item_name, + }) = &field.builder_attr.setter.extend { - let arg_expr = if let Some(FromFirst { keyword_span, closure }) = from_first { - // `Span::mixed_site()`-resolution suppresses `clippy::redundant_closure_call` on - // the generated code only. - quote_spanned!(keyword_span.resolved_at(Span::mixed_site())=> (#closure)(#field_name)) - } else if let Some(default) = &field.builder_attr.default { - quote_spanned!(*keyword_span=> { - let mut collection: #arg_type = #default; - ::core::iter::Extend::extend(&mut collection, ::core::iter::once(#field_name)); - collection + let item_name = item_name.clone().unwrap_or_else(|| { + syn::Ident::new( + (field.name.to_string() + "_item").trim_start().trim_start_matches("r#"), + field.name.span(), + ) + }); + + let descructuring = descructuring.collect::>(); + let reconstructing = reconstructing.collect::>(); + + let from_first = from_first + .clone() + .into_configured(|span| { + field + .builder_attr + .default + .as_ref() + .map(|default| { + syn::parse2(quote_spanned!(span.resolved_at(Span::mixed_site())=> |first| { + let mut extend = #default; + ::core::iter::Extend::extend(&mut extend, ::core::iter::once(first)); + extend + })) + .expect("extend from_first from default") + }) + .unwrap_or_else(|| { + syn::parse2(quote_spanned! {span.resolved_at(Span::mixed_site())=> + |first| ::core::iter::once(first).collect() + }) + .expect("extend from_first collect") + }) }) - } else { - quote_spanned!(*keyword_span=> ::<#arg_type as ::core::iter::FromIterator>::from_iter(::core::iter::once(#field_name))) - }; - (quote!(<#arg_type as ::core::iter::IntoIterator>::Item), arg_expr) - } else { - (arg_type.to_token_stream(), field_name.to_token_stream()) - }; - let (arg_type, arg_expr) = if field.builder_attr.setter.auto_into { - (quote!(impl core::convert::Into<#arg_type>), quote!(#arg_expr.into())) - } else { - (arg_type, arg_expr) - }; - let arg_expr = if field.builder_attr.setter.strip_option { - quote!(Some(#arg_expr)) - } else { - arg_expr - }; + .map({ + |Configured { keyword_span, value }| { + quote_spanned! {keyword_span.resolved_at(Span::mixed_site())=> + #doc + pub fn #item_name(self, #item_name: <#field_type as ::core::iter::IntoIterator>::Item) -> #builder_name<#(#target_generics),*> { + let #field_name = ((#value)(#item_name),); + let ( #(#descructuring,)* ) = self.fields; + #builder_name { + fields: ( #(#reconstructing,)* ), + _phantom: self._phantom, + } + } + } + }}); + let from_iter = from_iter.clone().into_configured(|span| { + field + .builder_attr + .default + .as_ref() + .map(|default| { + syn::parse2(quote_spanned!(span.resolved_at(Span::mixed_site())=> |iter| { + let mut extend = #default; + ::core::iter::Extend::extend(&mut extend, iter); + extend + })) + .expect("extend from_iter from default") + }) + .unwrap_or_else(|| { + syn::parse2(quote_spanned!(span.resolved_at(Span::mixed_site())=> |iter| iter.collect())) + .expect("extend from_iter collect") + }) + }).map(|Configured { keyword_span, value }| { + quote_spanned! {keyword_span.resolved_at(Span::mixed_site())=> + #doc + pub fn #field_name(self, #field_name: impl ::core::iter::Iterator::Item>) -> #builder_name<#(#target_generics),*> { + let #field_name = ((#value)(#field_name),); + let ( #(#descructuring,)* ) = self.fields; + #builder_name { + fields: ( #(#reconstructing,)* ), + _phantom: self._phantom, + } + } + } + }); + + Ok(quote_spanned! {keyword_span.resolved_at(Span::mixed_site())=> + #[allow(non_camel_case_types, missing_docs)] + impl #impl_generics #builder_name<#(#ty_generics),*> #where_clause { + #from_first + #from_iter + } + + impl #impl_generics #builder_name<#(#target_generics),*> #where_clause { + #doc + pub fn #item_name(self, #item_name: Item) -> #builder_name<#(#target_generics),*> + where + #field_type: ::core::iter::Extend, + { + let ( #(#reconstructing,)* ) = self.fields; + let mut #field_name = #field_name; + ::core::iter::Extend::extend(&mut #field_name.0, ::core::iter::once(#item_name)); + #builder_name { + fields: ( #(#reconstructing,)* ), + _phantom: self._phantom, + } + } - let repeat_items = if let Some(StripCollection { keyword_span, .. }) = field.builder_attr.setter.strip_collection { - let field_hygienic = Ident::new( - // Changing the name here doesn't really matter, but makes it easier to debug with `cargo expand`. - &format!("{}_argument", field_name), - field_name.span().resolved_at(Span::mixed_site()), - ); - quote_spanned! {keyword_span=> - #[allow(dead_code, non_camel_case_types, missing_docs)] - impl #impl_generics #builder_name < #( #target_generics ),* > #where_clause { #doc - pub fn #field_name (self, #field_name: Argument) -> #builder_name < #( #target_generics ),* > - // Making this repeat setter here further generic has a potential performance benefit: - // Many collections are also `Extend<&T>` where `T: Copy`. Calling this overload will then - // copy the value direction into the collection instead of through a stack temporary. - where #field_type: ::core::iter::Extend, + pub fn #field_name(self, #field_name: Iter) -> #builder_name<#(#target_generics),*> + where + Iter: ::core::iter::IntoIterator, + #field_type: ::core::iter::Extend, { - let #field_hygienic = #field_name; + let items = #field_name; let ( #(#reconstructing,)* ) = self.fields; let mut #field_name = #field_name; - ::core::iter::Extend::extend(&mut #field_name.0, ::core::iter::once(#field_hygienic)); + ::core::iter::Extend::extend(&mut #field_name.0, items); #builder_name { fields: ( #(#reconstructing,)* ), _phantom: self._phantom, } } } - } + }) } else { + //Not `extend`. + + // NOTE: both auto_into and strip_option affect `arg_type` and `arg_expr`, but the order of + // nesting is different so we have to do this little dance. + let arg_type = if field.builder_attr.setter.strip_option { + let internal_type = field + .type_from_inside_option() + .ok_or_else(|| Error::new_spanned(&field_type, "can't `strip_option` - field is not `Option<...>`"))?; + internal_type + } else { + field_type + }; + let (arg_type, arg_expr) = if field.builder_attr.setter.auto_into { + (quote!(impl core::convert::Into<#arg_type>), quote!(#field_name.into())) + } else { + (quote!(#arg_type), quote!(#field_name)) + }; + let arg_expr = if field.builder_attr.setter.strip_option { + quote!(Some(#arg_expr)) + } else { + arg_expr + }; + let repeated_fields_error_type_name = syn::Ident::new( &format!( "{}_Error_Repeated_field_{}", @@ -349,7 +421,20 @@ impl<'a> StructInfo<'a> { proc_macro2::Span::call_site(), ); let repeated_fields_error_message = format!("Repeated field {}", field_name); - quote! { + + Ok(quote! { + #[allow(dead_code, non_camel_case_types, missing_docs)] + impl #impl_generics #builder_name < #( #ty_generics ),* > #where_clause { + #doc + pub fn #field_name (self, #field_name: #arg_type) -> #builder_name < #( #target_generics ),* > { + let #field_name = (#arg_expr,); + let ( #(#descructuring,)* ) = self.fields; + #builder_name { + fields: ( #(#reconstructing,)* ), + _phantom: self._phantom, + } + } + } #[doc(hidden)] #[allow(dead_code, non_camel_case_types, non_snake_case)] pub enum #repeated_fields_error_type_name {} @@ -363,24 +448,8 @@ impl<'a> StructInfo<'a> { self } } - } - }; - - Ok(quote! { - #[allow(dead_code, non_camel_case_types, missing_docs)] - impl #impl_generics #builder_name < #( #ty_generics ),* > #where_clause { - #doc - pub fn #field_name (self, #field_name: #arg_type) -> #builder_name < #( #target_generics ),* > { - let #field_name = (#arg_expr,); - let ( #(#descructuring,)* ) = self.fields; - #builder_name { - fields: ( #(#reconstructing,)* ), - _phantom: self._phantom, - } - } - } - #repeat_items - }) + }) + } } pub fn required_field_impl(&self, field: &FieldInfo) -> Result { diff --git a/src/util.rs b/src/util.rs index ae3306d8..3d19329c 100644 --- a/src/util.rs +++ b/src/util.rs @@ -1,6 +1,6 @@ use quote::ToTokens; -pub fn path_to_single_string(path: &syn::Path) -> Option { +pub fn path_to_single_ident(path: &syn::Path) -> Option<&syn::Ident> { if path.leading_colon.is_some() { return None; } @@ -13,7 +13,11 @@ pub fn path_to_single_string(path: &syn::Path) -> Option { if segment.arguments != syn::PathArguments::None { return None; } - Some(segment.ident.to_string()) + Some(&segment.ident) +} + +pub fn path_to_single_string(path: &syn::Path) -> Option { + path_to_single_ident(path).map(|i| i.to_string()) } pub fn expr_to_single_string(expr: &syn::Expr) -> Option { From c9bba54800dcc81e9b63063de2d8ff6cd7b579b0 Mon Sep 17 00:00:00 2001 From: Tamme Schichler Date: Mon, 8 Feb 2021 19:27:03 +0100 Subject: [PATCH 08/16] Accept any matching `IntoIterator` instead of just matching `Iterator`s to initialise an `extend` field --- src/struct_info.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/struct_info.rs b/src/struct_info.rs index 18cf8bd8..0a7338d1 100644 --- a/src/struct_info.rs +++ b/src/struct_info.rs @@ -338,7 +338,7 @@ impl<'a> StructInfo<'a> { }).map(|Configured { keyword_span, value }| { quote_spanned! {keyword_span.resolved_at(Span::mixed_site())=> #doc - pub fn #field_name(self, #field_name: impl ::core::iter::Iterator::Item>) -> #builder_name<#(#target_generics),*> { + pub fn #field_name(self, #field_name: impl ::core::iter::IntoIterator::Item>) -> #builder_name<#(#target_generics),*> { let #field_name = ((#value)(#field_name),); let ( #(#descructuring,)* ) = self.fields; #builder_name { From 586d3dcc5801220162e67b104488da61a96281ed Mon Sep 17 00:00:00 2001 From: Tamme Schichler Date: Sat, 13 Feb 2021 15:41:59 +0100 Subject: [PATCH 09/16] Add some `extend` tests, fix issues with type inference and improve certain errors with explicit extend initialisers --- src/field_info.rs | 3 +- src/struct_info.rs | 75 +++++++++++++++++++++++++++++++++++++++++----- tests/extend.rs | 67 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 136 insertions(+), 9 deletions(-) create mode 100644 tests/extend.rs diff --git a/src/field_info.rs b/src/field_info.rs index 2a55e8b3..65cd1724 100644 --- a/src/field_info.rs +++ b/src/field_info.rs @@ -367,7 +367,8 @@ impl SetterSettings { keyword_span: path.span(), }; } - _ => return Err(Error::new_spanned(&path, format!("Unknown parameter {:?}", name))), + "item_name" => return Err(Error::new_spanned(path, "Expected (item_name = <...>)")), + _ => return Err(Error::new_spanned(path, format!("Unknown parameter {:?}", name))), } } _ => return Err(Error::new_spanned(arg, "Expected (<...>) or (<...>=<...>)")), diff --git a/src/struct_info.rs b/src/struct_info.rs index 0a7338d1..dec41333 100644 --- a/src/struct_info.rs +++ b/src/struct_info.rs @@ -1,6 +1,6 @@ use proc_macro2::{Span, TokenStream}; -use quote::{quote, quote_spanned}; -use syn::parse::Error; +use quote::{quote, quote_spanned, ToTokens}; +use syn::{parse::Error, spanned::Spanned}; use crate::field_info::{Configured, ExtendField, FieldBuilderAttr, FieldInfo}; use crate::util::{ @@ -309,7 +309,9 @@ impl<'a> StructInfo<'a> { quote_spanned! {keyword_span.resolved_at(Span::mixed_site())=> #doc pub fn #item_name(self, #item_name: <#field_type as ::core::iter::IntoIterator>::Item) -> #builder_name<#(#target_generics),*> { - let #field_name = ((#value)(#item_name),); + let from_item = #value; + let _: &dyn FnOnce(<#field_type as ::core::iter::IntoIterator>::Item) -> #field_type = &from_item; + let #field_name: (#field_type,) = ((from_item)(#item_name),); let ( #(#descructuring,)* ) = self.fields; #builder_name { fields: ( #(#reconstructing,)* ), @@ -318,7 +320,8 @@ impl<'a> StructInfo<'a> { } } }}); - let from_iter = from_iter.clone().into_configured(|span| { + let from_iter = from_iter.clone() + .into_configured(|span| { field .builder_attr .default @@ -336,18 +339,74 @@ impl<'a> StructInfo<'a> { .expect("extend from_iter collect") }) }).map(|Configured { keyword_span, value }| { - quote_spanned! {keyword_span.resolved_at(Span::mixed_site())=> + let syn::ExprClosure { + attrs, + asyncness, + movability, + capture: _, // No locals are in scope here, so this doesn't matter. + or1_token, + inputs, + or2_token, + output, + body, + } = value; + + if let Some(asyncness) = asyncness { + return Err(Error::new_spanned(asyncness, "Expected synchronous closure")) + } + + if let Some(movability) = movability { + return Err(Error::new_spanned(movability, "Expected movable closure")) + } + + let input = match inputs.len() { + 0 => return Err(Error::new_spanned(quote!(#or1_token #or2_token), "Expected one argument")), + 1 => inputs.into_iter().next().unwrap(), + _ => return Err(Error::new_spanned(inputs, "Expected only one argument")), + }; + + if let syn::Pat::Type(syn::PatType { colon_token, ty, .. }) = input { + return Err(Error::new_spanned(quote!(#colon_token #ty), "Did not expect argument type, since it is set explicitly by the macro")) + } + + // This is a bit roundabout. In short: + // Iff the closure has an explicit return type, and this return type mismatches the builder field type, + // then the "mismatched types" error is located on that explicit closure type rather than on `from_iter`. + let body = match output { + syn::ReturnType::Default => { + // Transforming this like below would bread the `unused_braces` warning on the closure body. + body.into_token_stream() + } + syn::ReturnType::Type(arrow, ty) => { + let colon = syn::Token![:](arrow.span()); + quote_spanned!{ty.span()=> + let output#colon #ty = #body; + output + } + } + }; + + Ok(quote_spanned! {keyword_span.resolved_at(Span::mixed_site())=> #doc pub fn #field_name(self, #field_name: impl ::core::iter::IntoIterator::Item>) -> #builder_name<#(#target_generics),*> { - let #field_name = ((#value)(#field_name),); + #(#attrs)* + #asyncness fn from_iter((#input): I) -> #field_type + where + I: ::core::iter::Iterator::Item> + { + #body + } + + let #field_name: (#field_type,) = (from_iter(#field_name.into_iter()),); let ( #(#descructuring,)* ) = self.fields; #builder_name { fields: ( #(#reconstructing,)* ), _phantom: self._phantom, } } - } - }); + }) + }) + .transpose()?; Ok(quote_spanned! {keyword_span.resolved_at(Span::mixed_site())=> #[allow(non_camel_case_types, missing_docs)] diff --git a/tests/extend.rs b/tests/extend.rs new file mode 100644 index 00000000..ed3abc67 --- /dev/null +++ b/tests/extend.rs @@ -0,0 +1,67 @@ +use typed_builder::TypedBuilder; + +#[test] +fn simple_vec() { + #[derive(TypedBuilder)] + struct A { + #[builder(setter(extend))] + v: Vec, + } + + assert_eq!(A::builder().v_item(2).build().v, vec![2]); + assert_eq!(A::builder().v(vec![3, 4]).build().v, vec![3, 4]); + assert_eq!(A::builder().v_item(5).v_item(6).build().v, vec![5, 6]); + assert_eq!(A::builder().v(vec![7, 8]).v_item(9).build().v, vec![7, 8, 9]); + assert_eq!(A::builder().v_item(0).v(vec![1, 2]).build().v, vec![0, 1, 2]); + assert_eq!(A::builder().v(vec![3, 4]).v(vec![5, 6]).build().v, vec![3, 4, 5, 6]); +} + +#[test] +fn item_name() { + #[derive(TypedBuilder)] + struct A { + #[builder(setter(extend(from_first, from_iter, item_name = i)))] + v: Vec, + } + + assert_eq!(A::builder().i(2).build().v, vec![2]); + assert_eq!(A::builder().i(5).i(6).build().v, vec![5, 6]); + assert_eq!(A::builder().v(vec![7, 8]).i(9).build().v, vec![7, 8, 9]); + assert_eq!(A::builder().i(0).v(vec![1, 2]).build().v, vec![0, 1, 2]); +} + +#[test] +fn extend_default() { + #[derive(TypedBuilder)] + struct A { + #[builder(default = vec![0], setter(extend))] + v: Vec, + } + + assert_eq!(A::builder().v_item(2).build().v, vec![0, 2]); + assert_eq!(A::builder().v(vec![3, 4]).build().v, vec![0, 3, 4]); +} + +#[test] +fn extend_default_explicit_auto() { + #[derive(TypedBuilder)] + struct A { + #[builder(default = vec![0], setter(extend(from_first, from_iter)))] + v: Vec, + } + + assert_eq!(A::builder().v_item(2).build().v, vec![0, 2]); + assert_eq!(A::builder().v(vec![3, 4]).build().v, vec![0, 3, 4]); +} + +#[test] +fn ignore_default() { + #[derive(TypedBuilder)] + struct A { + #[builder(default = vec![0], setter(extend(from_first = |first| vec![first], from_iter = |iter| iter.collect())))] + v: Vec, + } + + assert_eq!(A::builder().v_item(2).build().v, vec![2]); + assert_eq!(A::builder().v(vec![3, 4]).build().v, vec![3, 4]); +} From 23ca53a70d7b9d5c41ca32274f582593872ee4e7 Mon Sep 17 00:00:00 2001 From: Tamme Schichler Date: Sat, 13 Feb 2021 15:56:08 +0100 Subject: [PATCH 10/16] Ignore field default in automatic extend initialisers --- src/struct_info.rs | 40 ++++++---------------------------------- tests/extend.rs | 17 ++++++++++------- 2 files changed, 16 insertions(+), 41 deletions(-) diff --git a/src/struct_info.rs b/src/struct_info.rs index dec41333..87dc29a1 100644 --- a/src/struct_info.rs +++ b/src/struct_info.rs @@ -285,24 +285,10 @@ impl<'a> StructInfo<'a> { let from_first = from_first .clone() .into_configured(|span| { - field - .builder_attr - .default - .as_ref() - .map(|default| { - syn::parse2(quote_spanned!(span.resolved_at(Span::mixed_site())=> |first| { - let mut extend = #default; - ::core::iter::Extend::extend(&mut extend, ::core::iter::once(first)); - extend - })) - .expect("extend from_first from default") - }) - .unwrap_or_else(|| { - syn::parse2(quote_spanned! {span.resolved_at(Span::mixed_site())=> - |first| ::core::iter::once(first).collect() - }) - .expect("extend from_first collect") - }) + syn::parse2(quote_spanned! {span.resolved_at(Span::mixed_site())=> + |first| ::core::iter::once(first).collect() + }) + .expect("extend from_first collect") }) .map({ |Configured { keyword_span, value }| { @@ -322,22 +308,8 @@ impl<'a> StructInfo<'a> { }}); let from_iter = from_iter.clone() .into_configured(|span| { - field - .builder_attr - .default - .as_ref() - .map(|default| { - syn::parse2(quote_spanned!(span.resolved_at(Span::mixed_site())=> |iter| { - let mut extend = #default; - ::core::iter::Extend::extend(&mut extend, iter); - extend - })) - .expect("extend from_iter from default") - }) - .unwrap_or_else(|| { - syn::parse2(quote_spanned!(span.resolved_at(Span::mixed_site())=> |iter| iter.collect())) - .expect("extend from_iter collect") - }) + syn::parse2(quote_spanned!(span.resolved_at(Span::mixed_site())=> |iter| iter.collect())) + .expect("extend from_iter collect") }).map(|Configured { keyword_span, value }| { let syn::ExprClosure { attrs, diff --git a/tests/extend.rs b/tests/extend.rs index ed3abc67..0d41deb2 100644 --- a/tests/extend.rs +++ b/tests/extend.rs @@ -31,37 +31,40 @@ fn item_name() { } #[test] -fn extend_default() { +fn default_and_implicit_initializers() { #[derive(TypedBuilder)] struct A { #[builder(default = vec![0], setter(extend))] v: Vec, } - assert_eq!(A::builder().v_item(2).build().v, vec![0, 2]); - assert_eq!(A::builder().v(vec![3, 4]).build().v, vec![0, 3, 4]); + assert_eq!(A::builder().build().v, vec![0]); + assert_eq!(A::builder().v_item(2).build().v, vec![2]); + assert_eq!(A::builder().v(vec![3, 4]).build().v, vec![3, 4]); } #[test] -fn extend_default_explicit_auto() { +fn default_and_explicit_auto_initializers() { #[derive(TypedBuilder)] struct A { #[builder(default = vec![0], setter(extend(from_first, from_iter)))] v: Vec, } - assert_eq!(A::builder().v_item(2).build().v, vec![0, 2]); - assert_eq!(A::builder().v(vec![3, 4]).build().v, vec![0, 3, 4]); + assert_eq!(A::builder().build().v, vec![0]); + assert_eq!(A::builder().v_item(2).build().v, vec![2]); + assert_eq!(A::builder().v(vec![3, 4]).build().v, vec![3, 4]); } #[test] -fn ignore_default() { +fn default_and_explicit_initializer_closures() { #[derive(TypedBuilder)] struct A { #[builder(default = vec![0], setter(extend(from_first = |first| vec![first], from_iter = |iter| iter.collect())))] v: Vec, } + assert_eq!(A::builder().build().v, vec![0]); assert_eq!(A::builder().v_item(2).build().v, vec![2]); assert_eq!(A::builder().v(vec![3, 4]).build().v, vec![3, 4]); } From 094a82724191419d8dccc1b4d1ba9c5ff8fb22ef Mon Sep 17 00:00:00 2001 From: Tamme Schichler Date: Sat, 13 Feb 2021 17:24:53 +0100 Subject: [PATCH 11/16] Add generics support in combination with `extend` --- src/struct_info.rs | 39 ++++++++++++++++++++++++++++++++------- tests/extend.rs | 25 +++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 7 deletions(-) diff --git a/src/struct_info.rs b/src/struct_info.rs index 87dc29a1..726d27c4 100644 --- a/src/struct_info.rs +++ b/src/struct_info.rs @@ -311,6 +311,8 @@ impl<'a> StructInfo<'a> { syn::parse2(quote_spanned!(span.resolved_at(Span::mixed_site())=> |iter| iter.collect())) .expect("extend from_iter collect") }).map(|Configured { keyword_span, value }| { + // A plain closure like `|iter| iter.collect()` would require a generic type annotation on the parameter type, + // which unfortunately isn't possible. This means the closure is repackaged as nested function. let syn::ExprClosure { attrs, asyncness, @@ -323,12 +325,14 @@ impl<'a> StructInfo<'a> { body, } = value; + // A few error messages can be made much more direct on the way: + if let Some(asyncness) = asyncness { - return Err(Error::new_spanned(asyncness, "Expected synchronous closure")) + return Err(Error::new_spanned(asyncness, "Expected synchronous closure (Remove `async`)")) } if let Some(movability) = movability { - return Err(Error::new_spanned(movability, "Expected movable closure")) + return Err(Error::new_spanned(movability, "Expected movable closure (Remove `static`)")) } let input = match inputs.len() { @@ -358,18 +362,39 @@ impl<'a> StructInfo<'a> { } }; + let generics = self.modify_generics(|g| { + g.params.push(syn::parse2(quote_spanned!{keyword_span.resolved_at(Span::mixed_site())=> + __x: ::core::iter::Iterator::Item> + }).expect("extend from_iter __x")) + }); + let where_clause = generics.where_clause.as_ref(); + let generics_for_function_generics = self.modify_generics(|g| { + g.params.push(syn::GenericParam::Type(syn::TypeParam { + attrs: vec![], + ident: syn::Ident::new("_", keyword_span.resolved_at(Span::mixed_site())), + colon_token: None, + bounds: syn::punctuated::Punctuated::default(), + eq_token: None, + default: None, + })) + }); + let (_, type_generics, _) = generics_for_function_generics.split_for_impl(); + let type_generics = type_generics.into_token_stream(); + let function_generics = if type_generics.is_empty() { + type_generics + } else { + quote_spanned!(keyword_span.resolved_at(Span::mixed_site())=> ::#type_generics) + }; + Ok(quote_spanned! {keyword_span.resolved_at(Span::mixed_site())=> #doc pub fn #field_name(self, #field_name: impl ::core::iter::IntoIterator::Item>) -> #builder_name<#(#target_generics),*> { #(#attrs)* - #asyncness fn from_iter((#input): I) -> #field_type - where - I: ::core::iter::Iterator::Item> - { + #asyncness fn from_iter#generics((#input): __x) -> #field_type #where_clause { #body } - let #field_name: (#field_type,) = (from_iter(#field_name.into_iter()),); + let #field_name: (#field_type,) = (from_iter#function_generics(#field_name.into_iter()),); let ( #(#descructuring,)* ) = self.fields; #builder_name { fields: ( #(#reconstructing,)* ), diff --git a/tests/extend.rs b/tests/extend.rs index 0d41deb2..91522598 100644 --- a/tests/extend.rs +++ b/tests/extend.rs @@ -68,3 +68,28 @@ fn default_and_explicit_initializer_closures() { assert_eq!(A::builder().v_item(2).build().v, vec![2]); assert_eq!(A::builder().v(vec![3, 4]).build().v, vec![3, 4]); } + +#[test] +fn generic_inference() { + #[derive(TypedBuilder)] + struct A { + #[builder(setter(extend))] + v: Vec, + } + + #[derive(TypedBuilder)] + struct B { + #[builder(setter(extend))] + s: Vec, + #[builder(setter(extend))] + t: Vec, + } + + let A { v: _v } = A::builder().v(vec![true]).build(); + let _ = A::builder().v_item(0).build(); + + let B { s: _s, t: _t } = B::builder().s(vec![true]).t(vec![false]).build(); + let _ = B::builder().s(vec![0]).t_item(1).build(); + let _ = B::builder().s_item('a').t(vec![false]).build(); + let _ = B::builder().s_item("b").t_item(1).build(); +} From 4a2afb0e866d155bbaca4e059b2dd7218d22dcc7 Mon Sep 17 00:00:00 2001 From: Tamme Schichler Date: Sat, 13 Feb 2021 21:21:09 +0100 Subject: [PATCH 12/16] Remove extra parentheses These were left over from when I experimented with what's now the "Did not expect argument type, since it is set explicitly by the macro" error. --- src/struct_info.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/struct_info.rs b/src/struct_info.rs index 726d27c4..6037111b 100644 --- a/src/struct_info.rs +++ b/src/struct_info.rs @@ -390,7 +390,7 @@ impl<'a> StructInfo<'a> { #doc pub fn #field_name(self, #field_name: impl ::core::iter::IntoIterator::Item>) -> #builder_name<#(#target_generics),*> { #(#attrs)* - #asyncness fn from_iter#generics((#input): __x) -> #field_type #where_clause { + #asyncness fn from_iter#generics(#input: __x) -> #field_type #where_clause { #body } From a3006a13c574dff33a440ad73af43d3f3e1a3cb0 Mon Sep 17 00:00:00 2001 From: Tamme Schichler Date: Sat, 13 Feb 2021 21:26:08 +0100 Subject: [PATCH 13/16] Support combining `strip_option` and `extend` on the same field --- src/field_info.rs | 59 +++++++++++++++---------- src/lib.rs | 2 +- src/struct_info.rs | 106 ++++++++++++++++++++++++++++++++------------- src/util.rs | 12 +++-- tests/extend.rs | 42 ++++++++++++++++++ 5 files changed, 161 insertions(+), 60 deletions(-) diff --git a/src/field_info.rs b/src/field_info.rs index 65cd1724..d43f77c9 100644 --- a/src/field_info.rs +++ b/src/field_info.rs @@ -40,41 +40,52 @@ impl<'a> FieldInfo<'a> { ident_to_type(self.generic_ident.clone()) } - pub fn tuplized_type_ty_param(&self) -> syn::Type { + pub fn tuplized_type_ty_param(&self) -> Result { let mut types = syn::punctuated::Punctuated::default(); - types.push(self.ty.clone()); + types.push( + if matches!(self.builder_attr.setter, SetterSettings { extend: Some(_), strip_option: true, .. }) { + self.type_from_inside_option()? + } else { + self.ty + } + .clone(), + ); types.push_punct(Default::default()); - syn::TypeTuple { + Ok(syn::TypeTuple { paren_token: Default::default(), elems: types, } - .into() + .into()) } - pub fn type_from_inside_option(&self) -> Option<&syn::Type> { - let path = if let syn::Type::Path(type_path) = self.ty { - if type_path.qself.is_some() { + pub fn type_from_inside_option(&self) -> Result<&syn::Type, Error> { + pub fn try_<'a>(field_info: &'a FieldInfo) -> Option<&'a syn::Type> { + let path = if let syn::Type::Path(type_path) = field_info.ty { + if type_path.qself.is_some() { + return None; + } else { + &type_path.path + } + } else { + return None; + }; + let segment = path.segments.last()?; + if segment.ident != "Option" { + return None; + } + let generic_params = if let syn::PathArguments::AngleBracketed(generic_params) = &segment.arguments { + generic_params + } else { return None; + }; + if let syn::GenericArgument::Type(ty) = generic_params.args.first()? { + Some(ty) } else { - &type_path.path + None } - } else { - return None; - }; - let segment = path.segments.last()?; - if segment.ident != "Option" { - return None; - } - let generic_params = if let syn::PathArguments::AngleBracketed(generic_params) = &segment.arguments { - generic_params - } else { - return None; - }; - if let syn::GenericArgument::Type(ty) = generic_params.args.first()? { - Some(ty) - } else { - None } + + try_(self).ok_or_else(|| Error::new_spanned(&self.ty, "can't `strip_option` - field is not `Option<...>`")) } } diff --git a/src/lib.rs b/src/lib.rs index eca6abc0..80652ed0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -160,7 +160,7 @@ fn impl_my_derive(ast: &syn::DeriveInput) -> Result { .filter(|f| f.builder_attr.default.is_none()) .map(|f| struct_info.required_field_impl(f)) .collect::, _>>()?; - let build_method = struct_info.build_method_impl(); + let build_method = struct_info.build_method_impl()?; quote! { #builder_creation diff --git a/src/struct_info.rs b/src/struct_info.rs index 6037111b..5dbce4ac 100644 --- a/src/struct_info.rs +++ b/src/struct_info.rs @@ -2,10 +2,10 @@ use proc_macro2::{Span, TokenStream}; use quote::{quote, quote_spanned, ToTokens}; use syn::{parse::Error, spanned::Spanned}; -use crate::field_info::{Configured, ExtendField, FieldBuilderAttr, FieldInfo}; +use crate::field_info::{Configured, ExtendField, FieldBuilderAttr, FieldInfo, SetterSettings}; use crate::util::{ - empty_type, empty_type_tuple, expr_to_single_string, make_punctuated_single, modify_types_generics_hack, - path_to_single_string, strip_raw_ident_prefix, type_tuple, + empty_type, empty_type_tuple, expr_to_single_string, make_punctuated_single, path_to_single_string, strip_raw_ident_prefix, + try_modify_types_generics_hack, type_tuple, }; #[derive(Debug)] @@ -50,6 +50,15 @@ impl<'a> StructInfo<'a> { generics } + fn try_modify_generics Result<(), Error>>( + &self, + mut mutator: F, + ) -> Result { + let mut generics = self.generics.clone(); + mutator(&mut generics)?; + Ok(generics) + } + pub fn builder_creation_impl(&self) -> Result { let StructInfo { ref vis, @@ -64,9 +73,10 @@ impl<'a> StructInfo<'a> { g.params.insert(0, all_fields_param.clone()); }); let empties_tuple = type_tuple(self.included_fields().map(|_| empty_type())); - let generics_with_empty = modify_types_generics_hack(&ty_generics, |args| { + let generics_with_empty = try_modify_types_generics_hack(&ty_generics, |args| { args.insert(0, syn::GenericArgument::Type(empties_tuple.clone().into())); - }); + Ok(()) + })?; let phantom_generics = self.generics.params.iter().map(|param| { let t = match param { syn::GenericParam::Lifetime(lifetime) => { @@ -178,18 +188,25 @@ impl<'a> StructInfo<'a> { #[allow(dead_code, non_camel_case_types, non_snake_case)] pub trait #trait_name { fn into_value T>(self, default: F) -> T; + fn into_some_or_else ::core::option::Option>(self, default: F) -> ::core::option::Option; } impl #trait_name for () { fn into_value T>(self, default: F) -> T { default() } + fn into_some_or_else ::core::option::Option>(self, default: F) -> ::core::option::Option { + default() + } } impl #trait_name for (T,) { fn into_value T>(self, _: F) -> T { self.0 } + fn into_some_or_else ::core::option::Option>(self, _: F) -> ::core::option::Option { + ::core::option::Option::Some(self.0) + } } }) } @@ -230,11 +247,11 @@ impl<'a> StructInfo<'a> { .collect(); let mut target_generics_tuple = empty_type_tuple(); let mut ty_generics_tuple = empty_type_tuple(); - let generics = self.modify_generics(|g| { + let generics = self.try_modify_generics(|g| { for f in self.included_fields() { if f.ordinal == field.ordinal { ty_generics_tuple.elems.push_value(empty_type()); - target_generics_tuple.elems.push_value(f.tuplized_type_ty_param()); + target_generics_tuple.elems.push_value(f.tuplized_type_ty_param()?); } else { g.params.push(f.generic_ty_param()); let generic_argument: syn::Type = f.type_ident(); @@ -244,7 +261,8 @@ impl<'a> StructInfo<'a> { ty_generics_tuple.elems.push_punct(Default::default()); target_generics_tuple.elems.push_punct(Default::default()); } - }); + Ok(()) + })?; let mut target_generics = ty_generics.clone(); let index_after_lifetime_in_generics = target_generics @@ -272,6 +290,15 @@ impl<'a> StructInfo<'a> { item_name, }) = &field.builder_attr.setter.extend { + // Changing the builder field type entirely here (instead of just the argument) means there + // won't be a need to unwrap an `Option` in every repeat field setter call. + let field_type = if field.builder_attr.setter.strip_option { + let internal_type = field.type_from_inside_option()?; + internal_type + } else { + field_type + }; + let item_name = item_name.clone().unwrap_or_else(|| { syn::Ident::new( (field.name.to_string() + "_item").trim_start().trim_start_matches("r#"), @@ -450,10 +477,7 @@ impl<'a> StructInfo<'a> { // NOTE: both auto_into and strip_option affect `arg_type` and `arg_expr`, but the order of // nesting is different so we have to do this little dance. let arg_type = if field.builder_attr.setter.strip_option { - let internal_type = field - .type_from_inside_option() - .ok_or_else(|| Error::new_spanned(&field_type, "can't `strip_option` - field is not `Option<...>`"))?; - internal_type + field.type_from_inside_option()? } else { field_type }; @@ -535,7 +559,7 @@ impl<'a> StructInfo<'a> { }) .collect(); let mut builder_generics_tuple = empty_type_tuple(); - let generics = self.modify_generics(|g| { + let generics = self.try_modify_generics(|g| { for f in self.included_fields() { if f.builder_attr.default.is_some() { // `f` is not mandatory - it does not have it's own fake `build` method, so `field` will need @@ -550,7 +574,7 @@ impl<'a> StructInfo<'a> { } else if f.ordinal < field.ordinal { // Only add a `build` method that warns about missing `field` if `f` is set. If `f` is not set, // `f`'s `build` method will warn, since it appears earlier in the argument list. - builder_generics_tuple.elems.push_value(f.tuplized_type_ty_param()); + builder_generics_tuple.elems.push_value(f.tuplized_type_ty_param()?); } else if f.ordinal == field.ordinal { builder_generics_tuple.elems.push_value(empty_type()); } else { @@ -563,7 +587,8 @@ impl<'a> StructInfo<'a> { builder_generics_tuple.elems.push_punct(Default::default()); } - }); + Ok(()) + })?; let index_after_lifetime_in_generics = builder_generics .iter() @@ -603,15 +628,18 @@ impl<'a> StructInfo<'a> { }) } - pub fn build_method_impl(&self) -> TokenStream { + pub fn build_method_impl(&self) -> Result { let StructInfo { ref name, ref builder_name, .. } = *self; - let generics = self.modify_generics(|g| { + let generics = self.try_modify_generics(|g| { for field in self.included_fields() { + let strip_option_extends = + matches!(field.builder_attr.setter, SetterSettings { extend: Some(_), strip_option: true, .. }); + if field.builder_attr.default.is_some() { let trait_ref = syn::TraitBound { paren_token: None, @@ -622,7 +650,11 @@ impl<'a> StructInfo<'a> { arguments: syn::PathArguments::AngleBracketed(syn::AngleBracketedGenericArguments { colon2_token: None, lt_token: Default::default(), - args: make_punctuated_single(syn::GenericArgument::Type(field.ty.clone())), + args: make_punctuated_single(syn::GenericArgument::Type(if strip_option_extends { + field.type_from_inside_option()?.clone() + } else { + field.ty.clone() + })), gt_token: Default::default(), }), } @@ -633,26 +665,33 @@ impl<'a> StructInfo<'a> { g.params.push(generic_param.into()); } } - }); + Ok(()) + })?; let (impl_generics, _, _) = generics.split_for_impl(); let (_, ty_generics, where_clause) = self.generics.split_for_impl(); - let modified_ty_generics = modify_types_generics_hack(&ty_generics, |args| { + let modified_ty_generics = try_modify_types_generics_hack(&ty_generics, |args| { args.insert( 0, syn::GenericArgument::Type( - type_tuple(self.included_fields().map(|field| { - if field.builder_attr.default.is_some() { - field.type_ident() - } else { - field.tuplized_type_ty_param() - } - })) + type_tuple( + self.included_fields() + .map(|field| { + if field.builder_attr.default.is_some() { + Ok(field.type_ident()) + } else { + field.tuplized_type_ty_param() + } + }) + .collect::, _>>()? + .into_iter(), + ) .into(), ), ); - }); + Ok(()) + })?; let descructuring = self.included_fields().map(|f| f.name); @@ -663,13 +702,18 @@ impl<'a> StructInfo<'a> { // relax that restriction by calculating a DAG of field default dependencies and // reordering based on that, but for now this much simpler thing is a reasonable approach. let assignments = self.fields.iter().map(|field| { - let name = &field.name; + let name = field.name; + let wrap_some = matches!(field.builder_attr.setter, SetterSettings { extend: Some(_), strip_option: true, .. }); if let Some(ref default) = field.builder_attr.default { if field.builder_attr.setter.skip { quote!(let #name = #default;) + } else if wrap_some { + quote!(let #name = #helper_trait_name::into_some_or_else(#name, || #default);) } else { quote!(let #name = #helper_trait_name::into_value(#name, || #default);) } + } else if wrap_some { + quote!(let #name = ::core::option::Option::Some(#name.0);) } else { quote!(let #name = #name.0;) } @@ -688,7 +732,7 @@ impl<'a> StructInfo<'a> { } else { quote!() }; - quote!( + Ok(quote!( #[allow(dead_code, non_camel_case_types, missing_docs)] impl #impl_generics #builder_name #modified_ty_generics #where_clause { #doc @@ -700,7 +744,7 @@ impl<'a> StructInfo<'a> { } } } - ) + )) } } diff --git a/src/util.rs b/src/util.rs index 3d19329c..1171bbea 100644 --- a/src/util.rs +++ b/src/util.rs @@ -1,4 +1,5 @@ use quote::ToTokens; +use syn::Error; pub fn path_to_single_ident(path: &syn::Path) -> Option<&syn::Ident> { if path.leading_colon.is_some() { @@ -72,9 +73,12 @@ pub fn make_punctuated_single(value: T) -> syn::punctuated::Punct punctuated } -pub fn modify_types_generics_hack(ty_generics: &syn::TypeGenerics, mut mutator: F) -> syn::AngleBracketedGenericArguments +pub fn try_modify_types_generics_hack( + ty_generics: &syn::TypeGenerics, + mut mutator: F, +) -> Result where - F: FnMut(&mut syn::punctuated::Punctuated), + F: FnMut(&mut syn::punctuated::Punctuated) -> Result<(), Error>, { let mut abga: syn::AngleBracketedGenericArguments = syn::parse(ty_generics.clone().into_token_stream().into()) .unwrap_or_else(|_| syn::AngleBracketedGenericArguments { @@ -83,8 +87,8 @@ where args: Default::default(), gt_token: Default::default(), }); - mutator(&mut abga.args); - abga + mutator(&mut abga.args)?; + Ok(abga) } pub fn strip_raw_ident_prefix(mut name: String) -> String { diff --git a/tests/extend.rs b/tests/extend.rs index 91522598..d9b0c215 100644 --- a/tests/extend.rs +++ b/tests/extend.rs @@ -93,3 +93,45 @@ fn generic_inference() { let _ = B::builder().s_item('a').t(vec![false]).build(); let _ = B::builder().s_item("b").t_item(1).build(); } + +#[test] +fn strip_option() { + #[derive(TypedBuilder)] + struct A { + #[builder(default, setter(strip_option, extend))] + v: Option>, + } + + assert_eq!(A::builder().build().v, None); + assert_eq!(A::builder().v_item(2).build().v, Some(vec![2])); + assert_eq!(A::builder().v(vec![3, 4]).build().v, Some(vec![3, 4])); + assert_eq!(A::builder().v_item(5).v_item(6).build().v, Some(vec![5, 6])); + assert_eq!(A::builder().v(vec![7, 8]).v_item(9).build().v, Some(vec![7, 8, 9])); + assert_eq!(A::builder().v_item(0).v(vec![1, 2]).build().v, Some(vec![0, 1, 2])); + assert_eq!(A::builder().v(vec![3, 4]).v(vec![5, 6]).build().v, Some(vec![3, 4, 5, 6])); +} + +#[test] +fn strip_option_generic_inference() { + #[derive(TypedBuilder)] + struct A { + #[builder(default, setter(strip_option, extend))] + v: Option>, + } + + #[derive(TypedBuilder)] + struct B { + #[builder(default, setter(strip_option, extend))] + s: Option>, + #[builder(default, setter(strip_option, extend))] + t: Option>, + } + + let A { v: _v } = A::builder().v(vec![true]).build(); + let _ = A::builder().v_item(0).build(); + + let B { s: _s, t: _t } = B::builder().s(vec![true]).t(vec![false]).build(); + let _ = B::builder().s(vec![0]).t_item(1).build(); + let _ = B::builder().s_item('a').t(vec![false]).build(); + let _ = B::builder().s_item("b").t_item(1).build(); +} From 9bb9dbf9c8530086fb711a85917f8221cab02a6b Mon Sep 17 00:00:00 2001 From: Tamme Schichler Date: Sat, 13 Feb 2021 21:30:07 +0100 Subject: [PATCH 14/16] cargo +nightly fmt --- src/field_info.rs | 9 ++++++++- src/struct_info.rs | 19 ++++++++++++++++--- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/src/field_info.rs b/src/field_info.rs index d43f77c9..55ba5e63 100644 --- a/src/field_info.rs +++ b/src/field_info.rs @@ -43,7 +43,14 @@ impl<'a> FieldInfo<'a> { pub fn tuplized_type_ty_param(&self) -> Result { let mut types = syn::punctuated::Punctuated::default(); types.push( - if matches!(self.builder_attr.setter, SetterSettings { extend: Some(_), strip_option: true, .. }) { + if matches!( + self.builder_attr.setter, + SetterSettings { + extend: Some(_), + strip_option: true, + .. + } + ) { self.type_from_inside_option()? } else { self.ty diff --git a/src/struct_info.rs b/src/struct_info.rs index 5dbce4ac..03cded82 100644 --- a/src/struct_info.rs +++ b/src/struct_info.rs @@ -637,8 +637,14 @@ impl<'a> StructInfo<'a> { let generics = self.try_modify_generics(|g| { for field in self.included_fields() { - let strip_option_extends = - matches!(field.builder_attr.setter, SetterSettings { extend: Some(_), strip_option: true, .. }); + let strip_option_extends = matches!( + field.builder_attr.setter, + SetterSettings { + extend: Some(_), + strip_option: true, + .. + } + ); if field.builder_attr.default.is_some() { let trait_ref = syn::TraitBound { @@ -703,7 +709,14 @@ impl<'a> StructInfo<'a> { // reordering based on that, but for now this much simpler thing is a reasonable approach. let assignments = self.fields.iter().map(|field| { let name = field.name; - let wrap_some = matches!(field.builder_attr.setter, SetterSettings { extend: Some(_), strip_option: true, .. }); + let wrap_some = matches!( + field.builder_attr.setter, + SetterSettings { + extend: Some(_), + strip_option: true, + .. + } + ); if let Some(ref default) = field.builder_attr.default { if field.builder_attr.setter.skip { quote!(let #name = #default;) From 4c67cf4181dab0a8eebc6501317b59602e9b2641 Mon Sep 17 00:00:00 2001 From: Tamme Schichler Date: Sat, 13 Feb 2021 21:44:13 +0100 Subject: [PATCH 15/16] Assert `strip_option` flag inside `type_from_inside_option` This function really shouldn't be called without that macro option, so this might shortcut some troubleshooting in the future. --- src/field_info.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/field_info.rs b/src/field_info.rs index 55ba5e63..40cad7ce 100644 --- a/src/field_info.rs +++ b/src/field_info.rs @@ -66,6 +66,8 @@ impl<'a> FieldInfo<'a> { } pub fn type_from_inside_option(&self) -> Result<&syn::Type, Error> { + assert!(self.builder_attr.setter.strip_option); + pub fn try_<'a>(field_info: &'a FieldInfo) -> Option<&'a syn::Type> { let path = if let syn::Type::Path(type_path) = field_info.ty { if type_path.qself.is_some() { From c8abc4d83cc2d1a17d94370fc9a69a70245ff113 Mon Sep 17 00:00:00 2001 From: Tamme Schichler Date: Sun, 14 Feb 2021 17:09:57 +0100 Subject: [PATCH 16/16] Mention single item setter name in `early_build_error_message` --- src/field_info.rs | 14 ++++++++++++++ src/struct_info.rs | 15 +++++++-------- 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/src/field_info.rs b/src/field_info.rs index 40cad7ce..cf303525 100644 --- a/src/field_info.rs +++ b/src/field_info.rs @@ -36,6 +36,20 @@ impl<'a> FieldInfo<'a> { syn::GenericParam::Type(self.generic_ident.clone().into()) } + pub fn item_name(&self) -> String { + self.builder_attr + .setter + .extend + .as_ref() + .expect("Tried to retrieve item_name() on FieldInfo without extend.") + .item_name + .as_ref() + .map_or_else( + || self.name.to_string().trim_start().trim_start_matches("r#").to_string() + "_item", + |item_name| item_name.to_string(), + ) + } + pub fn type_ident(&self) -> syn::Type { ident_to_type(self.generic_ident.clone()) } diff --git a/src/struct_info.rs b/src/struct_info.rs index 03cded82..29c66b3c 100644 --- a/src/struct_info.rs +++ b/src/struct_info.rs @@ -1,5 +1,6 @@ use proc_macro2::{Span, TokenStream}; use quote::{quote, quote_spanned, ToTokens}; +use std::fmt::Write; use syn::{parse::Error, spanned::Spanned}; use crate::field_info::{Configured, ExtendField, FieldBuilderAttr, FieldInfo, SetterSettings}; @@ -287,7 +288,7 @@ impl<'a> StructInfo<'a> { keyword_span, from_first, from_iter, - item_name, + item_name: _, }) = &field.builder_attr.setter.extend { // Changing the builder field type entirely here (instead of just the argument) means there @@ -299,12 +300,7 @@ impl<'a> StructInfo<'a> { field_type }; - let item_name = item_name.clone().unwrap_or_else(|| { - syn::Ident::new( - (field.name.to_string() + "_item").trim_start().trim_start_matches("r#"), - field.name.span(), - ) - }); + let item_name = syn::Ident::new(&field.item_name(), field.name.span()); let descructuring = descructuring.collect::>(); let reconstructing = reconstructing.collect::>(); @@ -609,7 +605,10 @@ impl<'a> StructInfo<'a> { ), proc_macro2::Span::call_site(), ); - let early_build_error_message = format!("Missing required field {}", field_name); + let mut early_build_error_message = format!("Missing required field {}", field_name); + if field.builder_attr.setter.extend.is_some() { + write!(&mut early_build_error_message, " (single item setter: {})", field.item_name()).unwrap(); + } Ok(quote! { #[doc(hidden)]