Merge pull request #92 from TedDriggs/generic_bounds

Improved type param bound expressions (#91)

derive_builder/CHANGELOG.md

@@ -2,6 +2,14 @@
 All notable changes to this project will be documented in this file.
 This project adheres to [Semantic Versioning](http://semver.org/).
 
+## Unreleased
+
+### Fixed
+- for generic structs, apply the `T: Clone` type bound in builder impl
+  instead of struct definition #91
+- only emit the `T: Clone` type bound when it is actually needed, i.e.
+  mutable/immutable pattern, but not owned pattern.
+
 ## [0.4.6] - 2017-04-26
 
 ### Added

derive_builder/src/lib.rs

@@ -488,8 +488,6 @@
 //!
 //! - Tuple structs and unit structs are not supported as they have no field
 //!   names.
-//! - Generic structs need the boundary `where T: std::clone::Clone` if
-//!   used in combination with the immutable/mutable pattern
 //! - Generic setters introduce a type parameter `VALUE: Into<_>`. Therefore you can't use
 //!  `VALUE` as a type parameter on a generic struct in combination with generic setters.
 //! - The `try_setter` attribute and `owned` builder pattern are not compatible in practice;

derive_builder/src/options/struct_mode.rs

@@ -1,5 +1,6 @@
 use syn;
-use options::{OptionsBuilder, OptionsBuilderMode, parse_lit_as_string, parse_lit_as_bool, parse_lit_as_path, FieldMode, StructOptions};
+use options::{OptionsBuilder, OptionsBuilderMode, parse_lit_as_string, parse_lit_as_bool,
+              parse_lit_as_path, FieldMode, StructOptions};
 use derive_builder_core::{DeprecationNotes, Bindings};
 
 #[derive(Debug, Clone)]
@@ -48,31 +49,31 @@ impl StructMode {
         desc: "builder name",
         map: |x: String| { x },
     }
-    
+
     impl_setter!{
         ident: build_fn_name,
         desc: "build function name",
         map: |x: String| { x },
     }
-    
+
     impl_setter!{
         ident: build_fn_enabled,
         desc: "build function enabled",
         map: |x: bool| { x },
     }
-    
+
     impl_setter!{
         ident: validate_fn,
         desc: "validator function path",
         map: |x: syn::Path| { x },
     }
-    
+
     impl_setter!{
         ident: derive_traits,
         desc: "derive traits",
         map: |x: Vec<syn::Ident>| { x },
     }
-    
+
     #[allow(non_snake_case)]
     fn parse_build_fn_options_metaItem(&mut self, meta_item: &syn::MetaItem) {
         trace!("Build Method Options - Parsing MetaItem `{:?}`.", meta_item);
@@ -88,7 +89,7 @@ impl StructMode {
             }
         }
     }
-    
+
     #[allow(non_snake_case)]
     fn parse_build_fn_options_nameValue(&mut self, ident: &syn::Ident, lit: &syn::Lit) {
         trace!("Build fn Options - Parsing named value `{}` = `{:?}`", ident.as_ref(), lit);
@@ -107,7 +108,7 @@ impl StructMode {
             }
         }
     }
-    
+
     /// e.g `private` in `#[builder(build_fn(private))]`
     fn parse_build_fn_options_word(&mut self, ident: &syn::Ident) {
         trace!("Setter Options - Parsing word `{}`", ident.as_ref());
@@ -120,32 +121,28 @@ impl StructMode {
             }
         };
     }
-    
+
     #[allow(non_snake_case)]
-    fn parse_build_fn_options_list(
-        &mut self,
-        ident: &syn::Ident,
-        nested: &[syn::NestedMetaItem]
-    ) {
+    fn parse_build_fn_options_list(&mut self, ident: &syn::Ident, nested: &[syn::NestedMetaItem]) {
         trace!("Build fn Options - Parsing list `{}({:?})`", ident.as_ref(), nested);
         match ident.as_ref() {
             _ => {
                 panic!("Unknown option `{}` {}.", ident.as_ref(), self.where_diagnostics())
             }
         }
     }
-    
+
     fn parse_build_fn_name(&mut self, lit: &syn::Lit) {
         trace!("Parsing build function name `{:?}`", lit);
         let value = parse_lit_as_string(lit).unwrap();
         self.build_fn_name(value.clone())
     }
-    
+
     #[allow(dead_code,unused_variables)]
     fn parse_build_fn_skip(&mut self, skip: &syn::Lit) {
         self.build_fn_enabled(!parse_lit_as_bool(skip).unwrap());
     }
-    
+
     fn parse_build_fn_validate(&mut self, lit: &syn::Lit) {
         trace!("Parsing build function validate path `{:?}`", lit);
         let value = parse_lit_as_path(lit).unwrap();
@@ -159,7 +156,7 @@ impl OptionsBuilderMode for StructMode {
         let value = parse_lit_as_string(name).unwrap();
         self.builder_name(value.clone());
     }
-    
+
     fn parse_build_fn_options(&mut self, nested: &[syn::NestedMetaItem]) {
         for x in nested {
             match *x {
@@ -182,13 +179,13 @@ impl OptionsBuilderMode for StructMode {
         let where_diag = self.where_diagnostics();
         for x in nested {
             match *x {
-                // We don't allow name-value pairs or further nesting here, so
-                // only look for words.
+        // We don't allow name-value pairs or further nesting here, so
+        // only look for words.
                 syn::NestedMetaItem::MetaItem(syn::MetaItem::Word(ref tr)) => {
                     match tr.as_ref() {
                         "Default" | "Clone" => { self.push_deprecation_note(
                             format!("The `Default` and `Clone` traits are automatically added to all \
-                            builders; explicitly deriving them is unnecessary ({})", where_diag)); 
+                            builders; explicitly deriving them is unnecessary ({})", where_diag));
                         },
                         _ => traits.push(tr.clone())
                     }
@@ -233,7 +230,7 @@ impl From<OptionsBuilder<StructMode>> for (StructOptions, OptionsBuilder<FieldMo
                 `field(<vis>)` to the builder attribute on the struct. (Found {})",
                 where_diagnostics));
         }
-        
+
         #[cfg(feature = "struct_default")]
         let (field_default_expression, struct_default_expression) = (None, b.default_expression);
         #[cfg(not(feature = "struct_default"))]
@@ -259,6 +256,11 @@ impl From<OptionsBuilder<StructMode>> for (StructOptions, OptionsBuilder<FieldMo
 
         let m = b.mode;
 
+        let pattern = b.builder_pattern.unwrap_or_default();
+        let bindings = Bindings {
+            no_std: b.no_std.unwrap_or(false)
+        };
+
         let struct_options = StructOptions {
             build_fn_enabled: m.build_fn_enabled.unwrap_or(true),
             build_fn_name: syn::Ident::new(
@@ -268,19 +270,17 @@ impl From<OptionsBuilder<StructMode>> for (StructOptions, OptionsBuilder<FieldMo
                 m.builder_name.unwrap_or(format!("{}Builder", m.build_target_name))
             ),
             builder_visibility: m.builder_vis.unwrap_or(m.build_target_vis),
-            builder_pattern: b.builder_pattern.unwrap_or_default(),
+            builder_pattern: pattern,
             build_target_ident: syn::Ident::new(m.build_target_name),
             derives: m.derive_traits.unwrap_or_default(),
             deprecation_notes: m.deprecation_notes,
             generics: m.build_target_generics,
             struct_size_hint: m.struct_size_hint,
-            bindings: Bindings {
-                no_std: b.no_std.unwrap_or(false),
-            },
+            bindings: bindings,
             default_expression: struct_default_expression,
             validate_fn: m.validate_fn,
         };
 
         (struct_options, field_defaults)
     }
-}
+}

derive_builder/src/options/struct_options.rs

@@ -20,8 +20,6 @@ pub struct StructOptions {
     /// Target struct name.
     pub build_target_ident: syn::Ident,
     /// Represents lifetimes and type parameters attached to the declaration of items.
-    ///
-    /// We assume that this is identical for the builder and its build target.
     pub generics: syn::Generics,
     /// Emit deprecation notes to the user,
     /// e.g. if a deprecated attribute was used in `derive_builder`.
@@ -43,13 +41,15 @@ impl StructOptions {
         Builder {
             enabled: true,
             ident: &self.builder_ident,
+            pattern: self.builder_pattern,
             derives: &self.derives,
             generics: Some(&self.generics),
             visibility: &self.builder_visibility,
             fields: Vec::with_capacity(self.struct_size_hint),
             functions: Vec::with_capacity(self.struct_size_hint),
             doc_comment: None,
             deprecation_notes: self.deprecation_notes.clone(),
+            bindings: self.bindings,
         }
     }
     /// Returns a `BuildMethod` according to the options.

derive_builder/tests/bounds_generation.rs

@@ -0,0 +1,75 @@
+#[macro_use]
+extern crate derive_builder;
+
+/// A struct that deliberately doesn't implement `Clone`.
+#[derive(Debug, Default, PartialEq, Eq)]
+pub struct Dolor(String);
+
+/// Notice that this type derives Builder without disallowing
+/// `Lorem<Dolor>`.
+#[derive(Debug, Clone, Builder, PartialEq, Eq, Default)]
+#[builder(field(private), setter(into))]
+pub struct Lorem<T> {
+    ipsum: T,
+}
+
+#[derive(Debug, Clone, Builder, PartialEq, Eq, Default)]
+#[builder(field(private))]
+pub struct VecLorem<T> {
+    ipsum: Vec<T>,
+}
+
+#[derive(Debug, Clone, Builder, PartialEq, Eq)]
+#[builder(pattern="owned", field(private))]
+pub struct OwnedLorem<T> {
+    ipsum: T,
+}
+
+#[test]
+fn generic_field_with_clone_has_builder_impl() {
+    assert_eq!(LoremBuilder::default()
+                   .ipsum(10)
+                   .build()
+                   .unwrap(),
+               Lorem {
+                   ipsum: 10
+               });
+}
+
+/// The `LoremBuilder` type does not require that `T: Clone`, so we should
+/// be able to name a `LoremBuilder<Dolor>`. But all the methods are on an impl
+/// bound with `T: Clone`, so we can't do anything with it.
+#[test]
+fn builder_with_non_clone_generic_compiles() {
+    let _ : LoremBuilder<Dolor>;
+}
+
+/// In this case, we're trying to emit something that converts into a thing that
+/// implements clone.
+#[test]
+fn builder_with_into_generic_compiles() {
+    assert_eq!(LoremBuilder::<String>::default().ipsum("").build().unwrap(), 
+               Lorem::default());
+}
+
+#[test]
+fn builder_with_vec_t_compiles() {
+    assert_eq!(VecLoremBuilder::<String>::default()
+                   .ipsum(vec!["Hello".to_string()])
+                   .build()
+                   .unwrap(), 
+               VecLorem {
+                   ipsum: vec!["Hello".to_string()]
+               });
+}
+
+#[test]
+fn generic_field_without_clone_has_owned_builder() {
+    assert_eq!(OwnedLoremBuilder::default()
+                   .ipsum(Dolor::default())
+                   .build()
+                   .unwrap(),
+               OwnedLorem {
+                   ipsum: Dolor::default()
+               });
+}