Rework visibility conflicts to avoid losing diagnostics

This defers conflict checking until the end so that every level can
participate, and errors are consistently generated.

derive_builder/CHANGELOG.md

@@ -2,6 +2,9 @@
 All notable changes to this project will be documented in this file.
 This project adheres to [Semantic Versioning](http://semver.org/).
 
+## Unreleased
+- Allow restricted visibility using `vis = "..."` for builders, build methods, setters, and fields #247
+
 ## [0.11.1] - 2022-03-16
 - Forward `allow` and `cfg` attributes from the deriving struct to the builder and its impl block #222
 - Support passing attributes to the builder struct using `#[builder_struct_attr(...)]`

derive_builder/tests/compile-fail/vis_conflict.rs

@@ -0,0 +1,21 @@
+#[macro_use]
+extern crate derive_builder;
+
+#[derive(Builder)]
+#[builder(public, vis = "pub(crate)")]
+pub struct Example {
+    #[builder(public, private)]
+    field: String,
+}
+
+#[derive(Builder)]
+#[builder(public, vis = "pub(crate)", build_fn(private, public))]
+pub struct SecondExample {
+    #[builder(private, vis = "pub")]
+    a_field: String,
+}
+
+fn main() {
+    ExampleBuilder::default().build();
+    SecondExampleBuilder::default().build();
+}

derive_builder/tests/compile-fail/vis_conflict.stderr

@@ -0,0 +1,41 @@
+error: `public` and `private` cannot be used together
+ --> tests/compile-fail/vis_conflict.rs:7:15
+  |
+7 |     #[builder(public, private)]
+  |               ^^^^^^
+
+error: `vis="..."` cannot be used with `public` or `private`
+ --> tests/compile-fail/vis_conflict.rs:5:25
+  |
+5 | #[builder(public, vis = "pub(crate)")]
+  |                         ^^^^^^^^^^^^
+
+error: `public` and `private` cannot be used together
+  --> tests/compile-fail/vis_conflict.rs:12:57
+   |
+12 | #[builder(public, vis = "pub(crate)", build_fn(private, public))]
+   |                                                         ^^^^^^
+
+error: `vis="..."` cannot be used with `public` or `private`
+  --> tests/compile-fail/vis_conflict.rs:14:30
+   |
+14 |     #[builder(private, vis = "pub")]
+   |                              ^^^^^
+
+error: `vis="..."` cannot be used with `public` or `private`
+  --> tests/compile-fail/vis_conflict.rs:12:25
+   |
+12 | #[builder(public, vis = "pub(crate)", build_fn(private, public))]
+   |                         ^^^^^^^^^^^^
+
+error[E0433]: failed to resolve: use of undeclared type `ExampleBuilder`
+  --> tests/compile-fail/vis_conflict.rs:19:5
+   |
+19 |     ExampleBuilder::default().build();
+   |     ^^^^^^^^^^^^^^ use of undeclared type `ExampleBuilder`
+
+error[E0433]: failed to resolve: use of undeclared type `SecondExampleBuilder`
+  --> tests/compile-fail/vis_conflict.rs:20:5
+   |
+20 |     SecondExampleBuilder::default().build();
+   |     ^^^^^^^^^^^^^^^^^^^^ use of undeclared type `SecondExampleBuilder`

derive_builder_core/src/macro_options/darling_opts.rs

@@ -25,36 +25,50 @@ trait Visibility {
 
     /// Get the explicitly-expressed visibility preference from the attribute.
     /// This returns `None` if the input didn't include either keyword.
-    fn as_expressed_vis(&self) -> darling::Result<Option<Cow<syn::Visibility>>> {
+    ///
+    /// # Panics
+    /// This method panics if the input specifies both `public` and `private`.
+    fn as_expressed_vis(&self) -> Option<Cow<syn::Visibility>> {
         let declares_public = self.public().is_present();
         let declares_private = self.private().is_present();
-
-        // Start with `vis = "..."` because it will have a span for the error message.
-        if let Some(vis) = self.explicit() {
-            if declares_public || declares_private {
-                Err(
-                    Error::custom(r#"`vis="..."` cannot be used with `public` or `private`"#)
-                        .with_span(vis),
-                )
-            } else {
-                Ok(Some(Cow::Borrowed(vis)))
-            }
+        let declares_explicit = self.explicit().is_some();
+
+        if declares_private {
+            assert!(!declares_public && !declares_explicit);
+            Some(Cow::Owned(syn::Visibility::Inherited))
+        } else if let Some(vis) = self.explicit() {
+            assert!(!declares_public);
+            Some(Cow::Borrowed(vis))
         } else if declares_public {
-            if declares_private {
-                Err(Error::custom(
-                    r#"`public` and `private` cannot be used together"#,
-                ))
-            } else {
-                Ok(Some(Cow::Owned(syn::parse_quote!(pub))))
-            }
-        } else if declares_private {
-            Ok(Some(Cow::Owned(syn::Visibility::Inherited)))
+            Some(Cow::Owned(syn::parse_quote!(pub)))
         } else {
-            Ok(None)
+            None
         }
     }
 }
 
+fn no_visibility_conflict<T: Visibility>(v: &T) -> darling::Result<()> {
+    let declares_public = v.public().is_present();
+    let declares_private = v.private().is_present();
+    if let Some(vis) = v.explicit() {
+        if declares_public || declares_private {
+            Err(
+                Error::custom(r#"`vis="..."` cannot be used with `public` or `private`"#)
+                    .with_span(vis),
+            )
+        } else {
+            Ok(())
+        }
+    } else if declares_public && declares_private {
+        Err(
+            Error::custom(r#"`public` and `private` cannot be used together"#)
+                .with_span(v.public()),
+        )
+    } else {
+        Ok(())
+    }
+}
+
 /// Options for the `build_fn` property in struct-level builder options.
 /// There is no inheritance for these settings from struct-level to field-level,
 /// so we don't bother using `Option` for values in this struct.
@@ -281,22 +295,24 @@ pub struct Field {
 }
 
 impl Field {
-    /// Populate `self.field_attrs` and `self.setter_attrs` by draining `self.attrs`
-    fn unnest_attrs(mut self) -> darling::Result<Self> {
+    fn no_visibility_conflicts(&self) -> darling::Result<()> {
         let mut errors = Error::accumulator();
+        errors.handle(no_visibility_conflict(&self.field));
+        errors.handle(no_visibility_conflict(self));
+        errors.finish()
+    }
 
-        errors.handle(distribute_and_unnest_attrs(
+    /// Populate `self.field_attrs` and `self.setter_attrs` by draining `self.attrs`
+    fn unnest_attrs(mut self) -> darling::Result<Self> {
+        distribute_and_unnest_attrs(
             &mut self.attrs,
             &mut [
                 ("builder_field_attr", &mut self.field_attrs),
                 ("builder_setter_attr", &mut self.setter_attrs),
             ],
-        ));
-
-        // Check for conflicting visibility declarations
-        errors.handle(self.as_expressed_vis());
+        )?;
 
-        errors.finish_with(self)
+        Ok(self)
     }
 }
 
@@ -521,8 +537,15 @@ impl Options {
             ],
         ));
 
-        // Check for conflicting visibility declarations
-        errors.handle(self.as_expressed_vis());
+        // Check for conflicting visibility declarations. These cannot be pushed
+        // down into `FieldMeta` et al because of the call to `no_visibility_conflict(&self)`,
+        // as all sub-fields must be valid for this `Options` function to run.
+        errors.handle(no_visibility_conflict(&self.field));
+        errors.handle(no_visibility_conflict(&self.build_fn));
+        self.data
+            .as_ref()
+            .map_struct_fields(|f| errors.handle(f.no_visibility_conflicts()));
+        errors.handle(no_visibility_conflict(&self));
 
         errors.finish_with(self)
     }
@@ -552,17 +575,14 @@ impl Options {
     /// If a visibility was declared in attributes, that will be used;
     /// otherwise the struct's own visibility will be used.
     pub fn builder_vis(&self) -> Cow<syn::Visibility> {
-        self.as_expressed_vis()
-            .unwrap()
-            .unwrap_or(Cow::Borrowed(&self.vis))
+        self.as_expressed_vis().unwrap_or(Cow::Borrowed(&self.vis))
     }
 
     /// Get the visibility of the emitted `build` method.
     /// This defaults to the visibility of the parent builder, but can be overridden.
     pub fn build_method_vis(&self) -> Cow<syn::Visibility> {
         self.build_fn
             .as_expressed_vis()
-            .unwrap()
             .unwrap_or_else(|| self.builder_vis())
     }
 
@@ -716,8 +736,7 @@ impl<'a> FieldWithDefaults<'a> {
     pub fn setter_vis(&self) -> Cow<syn::Visibility> {
         self.field
             .as_expressed_vis()
-            .unwrap()
-            .or_else(|| self.parent.as_expressed_vis().unwrap())
+            .or_else(|| self.parent.as_expressed_vis())
             .unwrap_or_else(|| Cow::Owned(syn::parse_quote!(pub)))
     }
 
@@ -734,8 +753,7 @@ impl<'a> FieldWithDefaults<'a> {
         self.field
             .field
             .as_expressed_vis()
-            .unwrap()
-            .or_else(|| self.parent.field.as_expressed_vis().unwrap())
+            .or_else(|| self.parent.field.as_expressed_vis())
             .unwrap_or(Cow::Owned(syn::Visibility::Inherited))
     }