From f61819a499cc32c2aba428eea45020d5ff5eeef0 Mon Sep 17 00:00:00 2001 From: Ted Driggs Date: Wed, 6 Apr 2022 10:06:42 -0700 Subject: [PATCH] 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 | 3 + .../tests/compile-fail/vis_conflict.rs | 21 ++++ .../tests/compile-fail/vis_conflict.stderr | 41 ++++++++ .../src/macro_options/darling_opts.rs | 98 +++++++++++-------- 4 files changed, 123 insertions(+), 40 deletions(-) create mode 100644 derive_builder/tests/compile-fail/vis_conflict.rs create mode 100644 derive_builder/tests/compile-fail/vis_conflict.stderr diff --git a/derive_builder/CHANGELOG.md b/derive_builder/CHANGELOG.md index ff79cec6..63e39aea 100644 --- a/derive_builder/CHANGELOG.md +++ b/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(...)]` diff --git a/derive_builder/tests/compile-fail/vis_conflict.rs b/derive_builder/tests/compile-fail/vis_conflict.rs new file mode 100644 index 00000000..68e38a41 --- /dev/null +++ b/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(); +} diff --git a/derive_builder/tests/compile-fail/vis_conflict.stderr b/derive_builder/tests/compile-fail/vis_conflict.stderr new file mode 100644 index 00000000..c8c2ed82 --- /dev/null +++ b/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` diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index f955dbbb..c8cce54d 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/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>> { + /// + /// # Panics + /// This method panics if the input specifies both `public` and `private`. + fn as_expressed_vis(&self) -> Option> { 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(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 { + 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 { + 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,9 +575,7 @@ 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 { - 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. @@ -562,7 +583,6 @@ impl Options { pub fn build_method_vis(&self) -> Cow { 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 { 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)) }